Skip to content

Follow-ups from #6283: TimeSeries components discard their file's page size, the by-id getOrCreateFile is unguarded, and a vector pool test asserts against its own contract #6314

Description

@lvca

Three findings from the work on #6283 (merged as dc341dcc4, PR #6305), kept in one issue because they were
all turned up by pulling on the same thread: what a component is allowed to believe about the file it was
handed. They are independent of each other and can be fixed in any order.

Severity is not uniform. Item 1 is a silent wrong-stride read and is the one worth doing first. Item 2 is
hardening of an invariant that today lives in its caller rather than in the API. Item 3 is a flaky test that
asserts the opposite of what the code it tests promises.

Everything below was established by reading the sources at dc341dcc4; where I have not reproduced something
end-to-end, it says so.


Item 1 - the TimeSeries components throw away the page size and version parsed from their own file name

Actionable, and silent when it bites.

ComponentFactory recovers the two facts that are baked into every component file's name and hands them to the
factory handler (ComponentFactory.java:48-53):

final int pageSize = file instanceof PaginatedComponentFile pcf ? pcf.getPageSize() : 0;
final int version = file.getVersion();
...
return handler.createOnLoad(database, fileName, file.getFilePath(), fileId, mode, pageSize, version);

LocalBucket's handler passes both straight through, which is what makes a reopen faithful to what was written
(LocalBucket.java:267-273):

return new LocalBucket(database, name, filePath, id, mode, pageSize, version);

Both TimeSeries handlers accept the same two parameters and discard them
(TimeSeriesBucket.java:136-142, TimeSeriesTagDictionary.java:150-156):

public PaginatedComponent createOnLoad(final DatabaseInternal database, final String name, final String filePath,
    final int id, final ComponentFile.MODE mode, final int pageSize, final int version) throws IOException {
  return new TimeSeriesBucket(database, name, filePath, id, new ArrayList<>(), null);   // pageSize, version dropped
}

The constructors they call re-derive the page size from the live configuration instead, and hard-code the
version (TimeSeriesBucket.java:161/:175, TimeSeriesTagDictionary.java:166/:176):

super(database, name, filePath, id, ComponentFile.MODE.READ_WRITE,
    database.getConfiguration().getValueAsInteger(GlobalConfiguration.BUCKET_DEFAULT_PAGE_SIZE), CURRENT_VERSION);

So a database whose arcadedb.bucketDefaultPageSize is not the value that was in force when the type was created
reopens its .tstb/.tstd files at a stride that is not the one they were written with. The setting is
SCOPE.DATABASE and user-settable (GlobalConfiguration.java:247, default 64 KB), so this needs no bug to
trigger - only a config change between two runs, or one database opened with a ContextConfiguration that
differs from the one that created it.

Nothing downstream catches it. PageManager resolves pages with the caller's page size, i.e. the
component's, and the only place that consults the file's own getPageSize() is the snapshot path
(PageManager.java:730, :741); there is no agreement check between PaginatedComponent.pageSize and
PaginatedComponentFile.getPageSize() anywhere. PaginatedComponent then also computes
pageCount = fileSize / pageSize from the wrong divisor. The failure is a misaligned read of real bytes, not an
exception.

Proposed fix, in this order:

  1. Pass the parsed values through, exactly as LocalBucket does, so the mismatch cannot be constructed. Both
    TimeSeries components need an id-and-page-size constructor for that; the version needs a moment's thought
    rather than a blind pass-through, because TimeSeriesBucket's row layout is chosen by whether a dictionary
    is supplied, not by this field - the point is that the component should not claim CURRENT_VERSION for a
    file whose name says otherwise.

  2. Then assert the invariant in PaginatedComponent's constructor, next to the file-id check Follow-ups from #6198: file-id/component-name mismatch, TimeSeriesBucket header accessors, and two investigated non-issues #6283 just added
    and in the same shape:

    if (file.getPageSize() != pageSize)
      throw new IllegalStateException("Component '" + name + "' was built with page size " + pageSize
          + " but its file '" + file.getFilePath() + "' has page size " + file.getPageSize());

    This is the same invariant as the id one, on the other field: the component and the file it holds must agree
    about how to address it. Note it converts today's silent misread into a loud failure at open for any database
    that is already in the mismatched state, which is the right trade but should be a deliberate one.

Not yet reproduced end-to-end. The first task on this issue is a test that creates a TimeSeries type under
one bucketDefaultPageSize, reopens the database under another, and shows what the reads do.


Item 2 - FileManager.getOrCreateFile(int, String) is the by-id mirror of #6283, and its invariant lives in its caller

Actionable, hardening - no live defect found.

#6283 fixed the by-name overload's hazard by asserting in PaginatedComponent that the file handed back
carries the id the component was built with. The by-id overload has the mirror shape and no such check
(FileManager.java:445-460):

public ComponentFile getOrCreateFile(final int fileId, final String filePath) throws IOException {
  ComponentFile file = fileIdMap.get(fileId);
  if (file != null)
    return file;          // whatever name/path that file carries, the caller's filePath is not consulted
  ...
}

The one production caller is the HA follower's file-creation path, and it already performs exactly this check
itself
, immediately before calling (ArcadeStateMachine.createNewFiles, ArcadeStateMachine.java:1932-1941):
a file id already registered under a different name throws SchemaException and is routed to quarantine plus a
full snapshot resync (added by #6063). So there is no reachable bug here today - I checked, and this is not a
"someone will hit it" claim.

What is worth fixing is that the invariant sits in the caller rather than in the API, one overload after the
sibling half was pushed down into the component layer. A second caller has to know to re-implement it, and the
two tests that call this overload document nothing about it.

Proposed fix. Push the name/path agreement check into the overload itself, so the guarantee belongs to
FileManager for both keys. The constraint to respect is the HA caller's behaviour: it needs its
SchemaException and the quarantine-and-resync handling that follows from it, so either the caller keeps its own
check on top (and FileManager's becomes the belt-and-braces case for everyone else), or the pushed-down check
throws something the HA apply path classifies the same way. Do not simply move it and change the exception type.


Item 3 - LSMVectorIndexSearcherPoolTest asserts a bound the pool documents it may exceed

Actionable, flaky test in the default CI lane.

concurrentSearchesNeverShareASearcher sets the pool size to 4, runs 200 searches across 8 threads, and then
asserts (LSMVectorIndexSearcherPoolTest.java:128, :134, :147):

GlobalConfiguration.VECTOR_INDEX_SEARCHER_POOL_SIZE.setValue(4);
final ExecutorService pool = Executors.newFixedThreadPool(8);
...
assertThat(stat(index, "pooledGraphSearchers")).isLessThanOrEqualTo(4);

The statistic is GraphSearcherPool.size(), i.e. idleCount (LSMVectorIndex.java:5898), and the release path
says in so many words that this count may exceed maxIdle (GraphSearcherPool.java:137-138):

// Racy by design: a transient overshoot of one or two searchers is cheaper than a lock on the search path.
if (idleCount.get() >= maxIdle) {

With 8 threads releasing into a pool of 4, several can read idleCount below the cap before any of them
increments, and the pool legitimately settles above it. The assertion therefore contradicts the contract of the
code under test.

Observed, not theorised: this failed as expected 5L to be less than or equal to 4L in a full engine suite run
while I was validating #6283, and passed both standalone and in a full baseline run of main from the same
commit - the concurrency it depends on is more likely to lose under full-suite CPU contention. The class carries
no @Tag, so it runs in the default lane and can redden an ordinary CI run.

Proposed fix. Assert what the pool actually promises rather than widening the number and moving on. The
property the test names - "never share a searcher" - is already covered by the per-task result comparison above
it; this last line is about the pool not growing without bound, so it should allow the documented overshoot
(e.g. maxIdle + concurrency, or a bound derived from the thread count) with a comment naming
GraphSearcherPool.release() as the reason. A bare bump to a magic number would leave the next person
re-deriving why 4 was not the answer.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions