Skip to content

Commit 8722bf1

Browse files
committed
docs(qwp): state the recovery no-mutation guarantee exactly as strong as it is
Two in-diff comments claimed more than the code proves. The recovery-scan comment asserted 'a failed recovery never mutates the slot (single exception: ...sanitizeSealedResidue)'. There are more windows where a recovery that later fails has already durably mutated: the legacy-migration sanitize zeroes residue before SfManifest.create (or any later step) can fail, and validated-extra cleanup plus corrupt-file quarantine run before the active-sanitize barrier or ring construction can fail. Restate the invariant as what the code actually guarantees -- a failed recovery never mutates COMMITTED CHAIN BYTES -- and enumerate the windows, each confined to proven-dead zeroes or preserve-by-rename quarantines. The db8938e-derived javadoc claimed sanitizeTornTail runs 'only on residue that validation proves non-load-bearing', gliding over the resumed active with '(reclaimed by appends anyway)'. No such proof exists for the active -- it has no successor to bound its accounting, and past a mid-file tear its residue can hold valid-CRC frames of real unacked payloads. Zeroing them is a deliberate POLICY (replay cannot cross the tear; unzeroed residue risks the reseal-brick and stale-frame resurrection hazards), not a validation-derived proof. Say so at every site that carried the stronger wording: openExisting's javadoc, the scan-time observe-only comment, sanitizeTornTail's contract, and the active-sanitize call site in SegmentRing. Comments only; no behavior change.
1 parent 98b7397 commit 8722bf1

2 files changed

Lines changed: 46 additions & 20 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -299,11 +299,16 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) {
299299
* still hold unreachable frames with valid CRCs -- the only surviving copy
300300
* of real payloads -- so whether it may be destroyed is a chain-level
301301
* decision this method cannot make. {@link SegmentRing} recovery invokes
302-
* {@link #sanitizeTornTail()} only once the chain has fully validated and
303-
* only on residue that validation proves non-load-bearing (the resumed
304-
* active's tail, which appends are about to reclaim anyway, and sealed
305-
* suffixes whose frame accounting is proven complete); every fail-closed
306-
* path leaves the bytes intact on disk for operator extraction. Clean
302+
* {@link #sanitizeTornTail()} only once the chain has fully validated,
303+
* and the justification differs by role: sealed suffixes are zeroed on
304+
* PROOF (frame accounting validated complete, so the residue can hold no
305+
* replayable frame -- a tear that cost frames fails closed instead, with
306+
* every byte left on disk for operator extraction), while the resumed
307+
* active's tail is zeroed by POLICY -- past a mid-file tear it can still
308+
* hold valid-CRC frames of real unacked payloads, but they are
309+
* unreachable by replay (the FSN sequence breaks at the tear) and
310+
* leaving them risks the reseal-brick and stale-frame-resurrection
311+
* hazards. Clean
307312
* partial fills (writer never attempted to write past the last valid
308313
* frame) do not log and report {@code 0}.
309314
*/
@@ -375,13 +380,17 @@ public static MmapSegment openExisting(FilesFacade ff, String path) {
375380
// Observe-only: report the residue, never touch it here. A
376381
// torn tail may be an interrupted append (dead bytes) OR a
377382
// mid-file tear with unreachable valid-CRC frames past it --
378-
// the only surviving copy of real payloads. Only chain-level
379-
// validation can tell the two apart, so the destroy/preserve
380-
// decision belongs to SegmentRing.recover: it invokes
381-
// sanitizeTornTail on the segment it resumes as active (and
382-
// on proven-dead sealed suffixes) AFTER the chain proves the
383-
// residue is not load-bearing, and fails closed with every
384-
// byte left on disk in all other cases.
383+
// the only surviving copy of real payloads. The
384+
// destroy/preserve decision belongs to SegmentRing.recover.
385+
// For sealed members chain validation PROVES the residue
386+
// dead (complete frame accounting) before sanitizeTornTail
387+
// runs, and a tear that cost frames fails closed with every
388+
// byte left on disk. For the segment resumed as active no
389+
// such proof exists (there is no successor to bound it): its
390+
// residue is zeroed by policy once the rest of the chain
391+
// validates, because replay cannot cross the tear and
392+
// unzeroed residue risks reseal-brick and stale-frame
393+
// resurrection.
385394
LOG.warn("SF segment {}: torn tail of {} bytes at offset {} "
386395
+ "(file size {}, frames recovered {}). "
387396
+ "The residue is preserved pending chain validation. "
@@ -752,9 +761,13 @@ public long tornTailBytes() {
752761
* {@link #openExisting}: after a mid-file tear the residue can hold
753762
* unreachable valid-CRC frames that are the only surviving copy of
754763
* unacked payloads. The caller ({@link SegmentRing} recovery) must first
755-
* prove the residue is not load-bearing -- the resumed active's tail
756-
* (reclaimed by appends anyway) or a sealed suffix whose frame accounting
757-
* validated complete against the chain. Must run before any append
764+
* establish the residue is not load-bearing, and the strength of that
765+
* claim differs by role: a sealed suffix is PROVEN dead (frame
766+
* accounting validated complete against the chain), while the resumed
767+
* active's tail past a mid-file tear is discarded by POLICY, not proof
768+
* -- it can hold valid-CRC frames of real unacked payloads that are
769+
* unreachable by replay (the FSN sequence breaks at the tear), and
770+
* leaving them risks the two hazards above. Must run before any append
758771
* resumes; appending first would put live frames inside the zeroed range,
759772
* so that ordering is rejected. Idempotent; no-op when no residue was
760773
* observed. The fsync is load-bearing: in MEMORY durability mode rotation

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -243,10 +243,18 @@ static Recovery recover(
243243
// Files whose own bytes prove corruption (bad magic, sub-header size,
244244
// negative baseSeq). They are excluded from the chain and quarantined
245245
// to <name>.corrupt — but only AFTER the surviving chain validates (or
246-
// resolves to EMPTY), so a failed recovery never mutates the slot
247-
// (single exception: proven-dead sealed residue is zeroed just before
248-
// the fail-closed first-sight throw in sanitizeSealedResidue -- bytes
249-
// the already-validated chain proves no replay can ever need).
246+
// resolves to EMPTY). The precise invariant: a failed recovery never
247+
// mutates COMMITTED CHAIN BYTES. It is not "never mutates the slot"
248+
// -- several windows durably mutate before a later step can still
249+
// fail: proven-dead sealed residue is zeroed just before the
250+
// fail-closed first-sight throw in sanitizeSealedResidue; the
251+
// legacy-migration sanitize zeroes proven-dead residue before
252+
// SfManifest.create (or any later step) can fail; and validated-
253+
// extra cleanup plus corrupt-file quarantine run before the
254+
// active-sanitize barrier or ring construction can fail. Every such
255+
// window is confined to bytes the already-validated chain proves no
256+
// replay can ever need, or to preserve-by-rename quarantines that
257+
// keep the bytes on disk.
250258
// Whether a quarantined file was load-bearing is decided by the
251259
// manifest-boundary / contiguity checks below, not by the skip itself.
252260
// Operational open/stat/read/mmap errors, observed size instability,
@@ -555,7 +563,12 @@ static Recovery recover(
555563
// where rotation does not sync the sealed predecessor's data
556564
// pages. A failed barrier aborts recovery (fail closed); the
557565
// retry re-observes the same residue because openExisting never
558-
// mutates.
566+
// mutates. Unlike sealed suffixes, the active's residue is NOT
567+
// proven dead -- past a mid-file tear it can hold valid-CRC
568+
// frames of real unacked payloads. Zeroing is policy: replay
569+
// cannot cross the tear, and preserving the bytes would trade
570+
// the two hazards above for data no recovery path can use (see
571+
// sanitizeTornTail).
559572
active.sanitizeTornTail();
560573
for (int i = 1, n = chain.size(); i < n; i++) {
561574
chain.get(i - 1).linkSuccessor(chain.get(i));

0 commit comments

Comments
 (0)