You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
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):
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):
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:
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 claimCURRENT_VERSION for a
file whose name says otherwise.
if (file.getPageSize() != pageSize)
thrownewIllegalStateException("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):
publicComponentFilegetOrCreateFile(finalintfileId, finalStringfilePath) throwsIOException {
ComponentFilefile = fileIdMap.get(fileId);
if (file != null)
returnfile; // 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):
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.
Three findings from the work on #6283 (merged as
dc341dcc4, PR #6305), kept in one issue because they wereall 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 somethingend-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.
ComponentFactoryrecovers the two facts that are baked into every component file's name and hands them to thefactory handler (
ComponentFactory.java:48-53):LocalBucket's handler passes both straight through, which is what makes a reopen faithful to what was written(
LocalBucket.java:267-273):Both TimeSeries handlers accept the same two parameters and discard them
(
TimeSeriesBucket.java:136-142,TimeSeriesTagDictionary.java:150-156):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):So a database whose
arcadedb.bucketDefaultPageSizeis not the value that was in force when the type was createdreopens its
.tstb/.tstdfiles at a stride that is not the one they were written with. The setting isSCOPE.DATABASEand user-settable (GlobalConfiguration.java:247, default 64 KB), so this needs no bug totrigger - only a config change between two runs, or one database opened with a
ContextConfigurationthatdiffers from the one that created it.
Nothing downstream catches it.
PageManagerresolves pages with the caller's page size, i.e. thecomponent'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 betweenPaginatedComponent.pageSizeandPaginatedComponentFile.getPageSize()anywhere.PaginatedComponentthen also computespageCount = fileSize / pageSizefrom the wrong divisor. The failure is a misaligned read of real bytes, not anexception.
Proposed fix, in this order:
Pass the parsed values through, exactly as
LocalBucketdoes, so the mismatch cannot be constructed. BothTimeSeries 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 dictionaryis supplied, not by this field - the point is that the component should not claim
CURRENT_VERSIONfor afile whose name says otherwise.
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 addedand in the same shape:
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 callerActionable, hardening - no live defect found.
#6283 fixed the by-name overload's hazard by asserting in
PaginatedComponentthat the file handed backcarries the id the component was built with. The by-id overload has the mirror shape and no such check
(
FileManager.java:445-460):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
SchemaExceptionand is routed to quarantine plus afull 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
FileManagerfor both keys. The constraint to respect is the HA caller's behaviour: it needs itsSchemaExceptionand the quarantine-and-resync handling that follows from it, so either the caller keeps its owncheck on top (and
FileManager's becomes the belt-and-braces case for everyone else), or the pushed-down checkthrows something the HA apply path classifies the same way. Do not simply move it and change the exception type.
Item 3 -
LSMVectorIndexSearcherPoolTestasserts a bound the pool documents it may exceedActionable, flaky test in the default CI lane.
concurrentSearchesNeverShareASearchersets the pool size to 4, runs 200 searches across 8 threads, and thenasserts (
LSMVectorIndexSearcherPoolTest.java:128,:134,:147):The statistic is
GraphSearcherPool.size(), i.e.idleCount(LSMVectorIndex.java:5898), and the release pathsays in so many words that this count may exceed
maxIdle(GraphSearcherPool.java:137-138):With 8 threads releasing into a pool of 4, several can read
idleCountbelow the cap before any of themincrements, 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 4Lin a fullenginesuite runwhile I was validating #6283, and passed both standalone and in a full baseline run of
mainfrom the samecommit - 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 namingGraphSearcherPool.release()as the reason. A bare bump to a magic number would leave the next personre-deriving why 4 was not the answer.