fix(#6340): a component takes its file's mode too, the version joins the guards it belongs with, and CHECK DATABASE can finally read a TimeSeries file - #6347
Conversation
…the guards it belongs with, and CHECK DATABASE can finally read a TimeSeries file Items 1 to 3 are the last members of the set #6283 and #6314 have been closing one at a time: a component and the file it holds must agree, and the agreement belongs to the API rather than to whoever remembered to check. 1. ComponentFile.getMode() - the mode was the one file property with no accessor, so TimeSeriesTagDictionary's build-on-an-existing-file constructor read the id, the page size and the version off the file and then hard-coded READ_WRITE in the middle of them. All four now come from the file. 2. The by-name getOrCreateFile consulted the caller's mode only on the miss path. Decided, and stated rather than implied: the mode is a request the caller is entitled to, so a hit that cannot satisfy it throws. The direction that settles it is the quiet one - asking for READ_ONLY and being handed a READ_WRITE file is a weaker guarantee than the caller asked for. Reopening the file to satisfy the request is deliberately not the alternative: a registered file is shared, so upgrading the channel would widen it under a reader that had asked for the narrower one. 3. PaginatedComponent now asserts the version alongside the file id and the page size. It decides how pages are interpreted where the other two decide where they are, so a disagreement is a misread of real bytes rather than an exception. A tripwire and not a compatibility gate: every load path takes the version from the file, so component and file agree by construction whatever build wrote it. 4. CHECK DATABASE had no TimeSeries coverage at all - DatabaseChecker held zero references to it. The checker walks record buckets and indexes; a TimeSeries type has neither, so its three on-disk formats were the only storage in the engine an integrity check could not see, and the pages #6314's bug wrote at the wrong stride were undetectable. Each format now validates itself in the shape IndexInternal.checkIntegrity() uses: - TimeSeriesBucket: page 0's magic, version and column count, and every counter it declares reconciled against the data pages - the sample count, the min and max timestamp, and that the pages it announces are in the file. Page headers only, no row decoding. - TimeSeriesTagDictionary: page 0's magic and version, and the entries walked the way load() walks them. - TimeSeriesSealedStore: header, block directory, offsets against the file length, a tail belonging to no block, and the per-block CRC32 - recomputed from the file, since it is verified lazily on first read and a block nothing queries is a block nothing verifies. Report-only in both modes, and rows are not decompressed: what a repair means for an append-only sealed store is the design question the issue flagged, and it wants an answer before code. Reported as corruptedTimeSeries plus warnings, with totalTimeSeriesTypes/Shards/Samples/SealedBlocks seeded on every run. One progress step, and only when the database has a TimeSeries type.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 31 |
🟢 Coverage 80.85% diff coverage · -6.88% coverage variation
Metric Results Coverage variation ✅ -6.88% coverage variation Diff coverage ✅ 80.85% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7f748a2) 160508 127979 79.73% Head commit (6f067b0) 193079 (+32571) 140658 (+12679) 72.85% (-6.88%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#6347) 282 228 80.85% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewSolid, well-scoped change. The three Went through the diff in detail (not just the description) - a few notes, nothing blocking: Correctness checks that passed:
Minor observations (not blockers, mostly already called out by the PR description itself):
Test coverage: the 11 new cases look genuinely adversarial rather than happy-path-only - especially No security or license concerns - no new dependencies, and the new code is read-only (no |
…th the totals measured where the verdict is Code review on #6347 noted that checkTimeSeries visited each type's shards three times - once for the sealed block count, once for countSamples(), once for the check itself - each with its own lock acquisitions. Folded into one pass, and the reason it is worth doing is not only the two walks it saves: the totals used to be sampled OUTSIDE the shard's append and compaction locks, so on a live database the sample count reported alongside a verdict described a different instant than the walk that produced the verdict, and a compaction landing in between moves rows from the mutable half to the sealed one. Each shard now measures itself inside the same lock window it checks itself in, and returns both together as an IntegrityReport.
ReviewThis is a well-scoped, well-tested PR that closes out the file/component agreement work from #6283/#6314 and adds real (if intentionally limited) integrity checking for TimeSeries storage. I read the full diff plus the two commits (including the follow-up that folded the three-walks-per-type check into one under a single lock window - nice catch on that already). Overall this looks solid; a few things worth a look before merge. Possible concern:
|
…t reads stop reading as corruption, and one missing page reports once Three points from the second code review on #6347. 1. The expensive half no longer stalls writers. TimeSeriesShard.checkIntegrity() held appendLock and the compaction read lock across BOTH halves, so a CHECK DATABASE blocked ingestion on the shard for as long as it took to CRC32 the whole .ts.sealed file. It only ever needed them for the mutable half, whose counters are raised by every append: the sealed store's own directoryLock read lock already excludes every path that mutates that file, all six of which take its write lock. The short window now covers the mutable check and both totals - so the numbers still describe one instant - and the sequential read runs outside it. 2. The two raw indexChannel.read() calls in the sealed-store check go through readBytes(), which loops. A FileChannel may return fewer bytes than asked for and leave the rest of the buffer zeroed; anywhere else that costs a retry, but in an integrity check it turns a short read into a header or CRC that does not match - a false accusation of corruption from the code whose job is to tell the two apart. 3. A single missing or unwalkable data page produced two findings: the page itself, and then the aggregate "declares X but the pages hold Y" that the short count tripped. The aggregates are statements about every page, so they are now skipped when the walk did not reach the end - the same rule the sealed-store walk already followed for its own totals.
Review of #6347Read through all three commits (initial change + two review-driven follow-ups) end to end. This is a well-scoped, well-documented change, and it's clear the two rounds of self-review already caught the substantive issues (lock scope creep in Code quality / correctness
Minor observations (non-blocking)
Test coverageBoth new test classes are solid:
I wasn't able to run No blocking issues found. Nice work tightening the locking and read-correctness during self-review - that's exactly the kind of thing that's easy to miss on a first pass with this much new I/O code. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6347 +/- ##
==========================================
- Coverage 70.92% 70.71% -0.22%
==========================================
Files 1849 1850 +1
Lines 160508 160714 +206
Branches 33762 33787 +25
==========================================
- Hits 113845 113650 -195
- Misses 32570 33084 +514
+ Partials 14093 13980 -113 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nothing exercised Codecov flagged 62.41% of the diff hit against a 70.92% target, and on an integrity checker that number means something specific: most of the uncovered lines are the branches that only ever run when a file IS damaged, so each one was a message that had never been proven to fire. Seven more shapes are now pinned - a header format version disagreeing with the file name, a header column count disagreeing with the schema, a header announcing data pages the file does not hold, a file that is not a whole number of its own pages, a tag dictionary claiming more entries than its pages hold, a sealed header miscounting its blocks, and bytes belonging to no block after the last one. The data-page one also regression-tests the round-2 fix that a single missing page reports ONCE: it asserts exactly one mutable-bucket warning, where before the aggregate sample-count check would have added a second finding for the same root cause. One shape is deliberately NOT tested, and it turned up a separate defect while trying: corrupting the sealed store's own header magic produces no finding at all, because TimeSeriesSealedStore's constructor throws on a bad magic, initEngine() then fails during schema load, and the TimeSeries type disappears from the schema entirely - the database reopens cleanly with the type simply gone (probed: totalTimeSeriesTypes=0, existsType=false). A check cannot report a type that is no longer there. That silent disappearance is in the schema load path rather than in this pass, and it is recorded as a comment where the test would have been rather than pinned, because a test asserting the current behaviour would enshrine it.
|
Follow-up filed from this work: #6356 - corrupting a |
ReviewWent through the full diff (engine changes + both new test classes) with the PR description and commit history as context. This is a careful piece of work, and it shows: the three self-review commits already fixed the lock-contention issue (holding Things I specifically checked and found sound:
Minor observations, not blockers:
Test coverage: the two new test classes (11 cases total) are thorough - they cover both the refusal and legitimate paths for each of the three Overall: no correctness, security, or performance issues found beyond what's already been self-identified and either fixed or explicitly deferred with justification. Nice work. |
…6398, #6394, #6356) #6398: HttpAuthSessionManagerTest's idle/absolute timeout assertions raced Thread.sleep against short timeouts, leaving as little as 40ms of headroom against a JVM stop-the-world pause. Both HttpAuthSession and HttpAuthSessionManager now accept an injectable clock (defaulting to the wall clock in production); the whole test class drives a fake clock instead of sleeping, so it cannot flake and runs in a fraction of the time. #6394: PageManager.openSnapshotInternal's t0 barrier throws PageSnapshotException from two call sites with opposite operational meaning (a transient suspend-timeout under load vs. a fatal in-flight-flush timeout), distinguishable before this change only by parsing getMessage(). A test for the fatal case inferred which one happened from elapsed time instead, which flakes whenever trySuspendUntil loses its own race first under a full-suite run. PageSnapshotException now carries a Reason enum; the test asserts the reason directly and drops the timing-based proxy. #6356: a TimeSeries type whose .ts.sealed store failed to load (e.g. one bit-flipped byte) used to vanish from the schema entirely - LocalSchema.readConfiguration wrapped the failure in a ConfigurationException that aborted the whole type's registration, and the outer catch-and-log in the same method swallowed it, so the database reopened cleanly with the type simply gone and no warning. DatabaseChecker#checkTimeSeries already had an unreachable branch for exactly this case ("the storage engine is not initialised"), from #6340/#6347, which is the strongest signal for the intended design: register the type with its engine unavailable rather than refusing the whole database open (the sealed store is HA-derived and rebuildable by recompaction) or silently discarding it. LocalTimeSeriesType now tracks why its engine is unavailable and exposes requireEngine() for read/write call sites to fail loudly and by name instead of NPEing or silently falling through to generic document handling. Co-authored-by: Luca Garulli <l.garulli@arcadedata.com>
1 participant
Closes #6340.
Four items. The first three are the last members of a set issues #6283 and #6314 have been closing one at a
time - a component and the file it holds must agree, and the agreement belongs to the API rather than to
whoever remembered to check. The fourth is the one that left a real defect undetectable.
Item 1 -
ComponentFile.getMode()Added, and used where it was missing:
TimeSeriesTagDictionary's build-on-an-existing-file constructor tookthe id, the page size and the version off the file and then hard-coded
MODE.READ_WRITEin the middle ofthem, because there was nothing to read the mode back out of. All four now come from the file.
Item 2 - the by-name
getOrCreateFileand the caller's modeThe issue asked for a decision, so here it is stated rather than implied: the mode is a request the caller
is entitled to, and a hit that cannot satisfy it now throws instead of handing back something else. The
direction that decided it is the quiet one - a caller asking for
READ_ONLYand being given aREAD_WRITEfile gets a weaker guarantee than it asked for, and mode is the one file property whose whole purpose is to
be a guarantee.
Reopening the file to satisfy the request was considered and rejected: a registered file is shared by every
component addressing it, so upgrading the channel would silently widen it under a reader that had asked for
the narrower one - the same shape being removed, with a worse failure mode.
IllegalStateException, not theSchemaExceptionits by-id sibling raises: a file name mismatch is afile-id space that diverged from the leader's and an HA follower classifies it as quarantine-and-resync; a
mode mismatch is a programming error, on the same footing as the file-id and page-size guards in
PaginatedComponent, which is this overload's only caller and which throws exactly this.Nothing reaches it today, and that is checked rather than assumed - the one caller that legitimately hits a
registered file is the tag dictionary constructor above, which now asks for the file's own mode.
Item 3 - the version guard
PaginatedComponentasserted the file id (#6283) and the page size (#6314) against the file it ends upholding; the version, the third fact baked into
name.fileId.pageSize.vVersion.ext, was not. It decides howthe pages are interpreted where the other two decide where they are - a
LocalBucketversion selects therecord-header layout, a
TimeSeriesBucketversion selects whether a TAG column is a 4-byte dictionary id oran inline string - so a disagreement is a misread of real bytes, never an exception.
A tripwire, not a compatibility gate, and worth repeating because #6314 had to work through the same
point twice: every load constructor passes the parsed version straight through and every creation path bakes
the version into the name it generates, so a component and its file agree by construction whatever build
wrote the file.
aComponentBuiltOnTheVersionItsFileNameCarriesOpensNormallypins that.Item 4 -
CHECK DATABASEcan now read a TimeSeries fileDatabaseCheckerhad zero references to TimeSeries. It walks record buckets and indexes; a TimeSeries typehas neither - its shards are registered with the schema as files, and its compacted data never goes through
the paginated layer - so the type fell into the document arm, found no bucket to scan, and all three of the
formats TimeSeries owns were outside the reach of the only tool whose job is to find damage in them.
Each format now validates itself, in the shape
IndexInternal.checkIntegrity()already uses:TimeSeriesBucket- page 0's magic, format version against the file name, column count against theschema, and then every counter page 0 declares reconciled against the data pages themselves: the sample
count, the min and the max timestamp, and that the data pages it announces are actually in the file. That
last set is what makes the residue of 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 visible: a session that wrote at the wrong stride put real rows
at offsets nothing will address again and counted them in a header that still does. Page headers only, no
row decoding.
TimeSeriesTagDictionary- page 0's magic and version, and the entries walked the wayload()walksthem, so the declared entry count and the bytes have to agree. A truncated dictionary does not fail a query;
it makes every tag written since the damage read back as
null, on every row.TimeSeriesSealedStore- header, block directory, offsets against the file length, a trailing regionthat belongs to no block, and the per-block CRC32. That last one is the expensive part and it is the
point: the CRC is verified lazily on first read, so a block nothing queries is a block nothing verifies.
It is recomputed from the file rather than delegated to
validateBlockCRC(), so a secondCHECK DATABASEin the same process cannot answer "clean" without having read a byte.
Rows are deliberately not decompressed and there is no
FIXarm. A record bucket can be repairedbecause its records are self-describing and its indexes derive from them; a sealed store is append-only
columnar data whose blocks are the only copy, so "repair" means deciding which samples to discard - the
design question the issue itself flagged, which wants an answer before code. This change makes the state
visible, which is the part that was missing entirely.
Reported as
corruptedTimeSeriesplus warnings, withtotalTimeSeriesTypes/Shards/Samples/SealedBlocksseeded on every run so "was this looked at?" is answerable from a clean result rather than from silence.
Scoped by
TYPElike every other per-type pass. One progress step for all TimeSeries types and only when thedatabase has one, so a database without them keeps the step plan every existing expectation was written
against.
Two things found on the way, neither fixed here
BlockEntry.blockStartOffsetandstoredCRCare populated byloadDirectory()alone. A block appended bythe running process carries both on disk and zero in the fields, with
crcValidatedpre-set to true.Nothing reads them for such a block today, so it is latent rather than a defect - but it is why the check
reads both sides from the file instead of trusting the directory.
checkBucketsalready runs over every bucket, but on a very large TimeSeries type it is the dominant costof a
CHECK DATABASE, and whether it should become opt-in is worth deciding once someone has one.Verification
Issue6340ComponentFileAgreementTest(items 1-3, both the refusal andthe legitimate case for each guard) and
Issue6340TimeSeriesCheckDatabaseTest(item 4 - the healthy walkwith its counters, the 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 residue shape, a missing bucket header, a missing dictionary header, a flipped
byte in a sealed block caught twice in a row, the
TYPEscope, and a database with no TimeSeries type).enginemodule: 12,474 tests green, which includes every existing TimeSeries test running the new passin its teardown integrity check.
ha-raftArcadeStateMachineCreateFilesTestand theserverread-only/check-database subset green.