Skip to content

Commit 88d6b79

Browse files
committed
fix(qwp): zero torn-tail residue during SF segment recovery
Recovery resumed appending at lastGood but left the stale bytes in [lastGood, fileSize) on disk. Two ordinary crashes could then brick the client permanently: crash #1 tears the active near its end; recovery resumes and the session fills the segment, but the last frame stops short of the old residue, so rotation reseals the recovered file with a non-zero suffix; recovery #2 correctly refuses non-zero sealed suffixes ("corrupt torn tail in sealed SF segment") -- on every startup, since the plain exception is not quarantined. Primary slot: Sender.build() fails forever; orphan slot: the drainer drops a permanent .failed sentinel and the unacked data is silently stranded. Sibling hazard: the frame envelope binds neither position nor FSN, so a byte-aligned stale frame with a valid CRC past the tear could be resurrected at a recycled FSN by a later scan (stale replay). openExisting now zeroes the residue right after the scan validates and makes the zeroes durable with an msync+fsync barrier before the segment is returned, restoring the invariant fresh segments already have: all bytes past the append cursor are zero. The barrier is load-bearing in MEMORY durability mode, where rotation does not sync the sealed predecessor's data pages. A failed barrier aborts recovery fail-closed. tornTailBytes still reports the pre-sanitization observation and the sealed-suffix check in SegmentRing stays fail-closed on first sight. Regression tests: residue zeroed on disk; two-crash reseal at segment level; stale-frame non-resurrection; barrier-failure abort; end-to-end two-crash ring recovery (torn -> recover -> fill+rotate -> recover).
1 parent 1374448 commit 88d6b79

4 files changed

Lines changed: 330 additions & 17 deletions

File tree

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

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,10 @@ public final class MmapSegment implements QuietCloseable {
111111
// attempted-but-invalid frame write (the suffix contains non-zero bytes).
112112
// Zero for fresh segments and for cleanly partially-filled
113113
// segments (uninitialised tail). Set only by openExisting; visible to
114-
// recovery callers for diagnostics. Final after construction.
114+
// recovery callers for diagnostics. Records the observation BEFORE
115+
// sanitization -- openExisting zeroes the on-disk residue, so the file
116+
// behind a freshly returned segment never carries it. Final after
117+
// construction.
115118
private final long tornTailBytes;
116119

117120
private MmapSegment(FilesFacade filesFacade, String path, int fd, long mmapAddress, long sizeBytes,
@@ -273,11 +276,18 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) {
273276
* <p>
274277
* If recovery observes a torn tail (the suffix after the last valid frame
275278
* contains non-zero bytes, indicating an attempted-but-failed frame write
276-
* rather than clean unwritten space), a {@code WARN} is emitted with the byte
277-
* count and the bytes are exposed via {@link #tornTailBytes()} so
278-
* operators can detect silent truncation from corruption or partial
279-
* writes. Clean partial fills (writer never attempted to write past the
280-
* last valid frame) do not log and report {@code 0}.
279+
* rather than clean unwritten space), a {@code WARN} is emitted with the
280+
* byte count, the observed size is exposed via {@link #tornTailBytes()},
281+
* and the residue is zeroed on disk (with an msync+fsync barrier) before
282+
* the segment is returned. Sanitizing restores the invariant every fresh
283+
* segment already has -- all bytes past the append cursor are zero.
284+
* Without it, residue that resumed appends do not fully overwrite would
285+
* survive into a sealed segment, whose recovery treats a non-zero suffix
286+
* as fatal corruption (a permanent startup failure), and a byte-aligned
287+
* stale frame with a valid CRC could be silently resurrected at a
288+
* recycled FSN by a later scan. Clean partial fills (writer never
289+
* attempted to write past the last valid frame) do not log, report
290+
* {@code 0}, and skip the barrier.
281291
*/
282292
public static MmapSegment openExisting(String path) {
283293
return openExisting(FilesFacade.INSTANCE, path);
@@ -344,10 +354,33 @@ public static MmapSegment openExisting(FilesFacade ff, String path) {
344354
+ " [before=" + fileSize + ", after=" + mappedSize + ']');
345355
}
346356
if (scan.tornTailBytes > 0) {
357+
// Zero the residue [lastGood, fileSize) and make the zeroes
358+
// durable before anyone can append. Resumed appends restart at
359+
// lastGood but stop wherever the last payload fits, so stale
360+
// residue past that point would otherwise survive a
361+
// seal-via-rotation -- and sealed-segment recovery treats a
362+
// non-zero suffix as fatal corruption, permanently failing
363+
// every subsequent startup. Zeroing also stops a byte-aligned
364+
// stale frame with a valid CRC from being resurrected at a
365+
// recycled FSN by a later recovery scan (the frame envelope
366+
// binds neither position nor FSN). The fsync is load-bearing:
367+
// in MEMORY durability mode rotation does not sync the sealed
368+
// predecessor's data pages, so an unflushed zero-write plus a
369+
// crash would regenerate the exact poisoned state this
370+
// sanitization removes. One-time recovery cost, torn tails
371+
// only; a failed barrier aborts recovery (fail closed) --
372+
// the zeroes may still reach disk via the page cache, which
373+
// is safe: a retry either sees them (clean tail) or re-zeroes.
374+
Unsafe.getUnsafe().setMemory(addr + scan.lastGood, fileSize - scan.lastGood, (byte) 0);
375+
if (ff.msync(addr, fileSize, false) != 0 || ff.fsync(fd) != 0) {
376+
throw new MmapSegmentException(
377+
"could not sync zeroed torn tail in " + path
378+
+ " [errno=" + Os.errno() + ']');
379+
}
347380
LOG.warn("SF segment {}: torn tail of {} bytes at offset {} "
348381
+ "(file size {}, frames recovered {}). "
349-
+ "Recovery will overwrite this region on next append; "
350-
+ "frames past the tear (if any) are discarded. "
382+
+ "The residue has been zeroed; frames past the tear "
383+
+ "(if any) are discarded. "
351384
+ "Investigate disk health or unexpected writer crash.",
352385
path, scan.tornTailBytes, scan.lastGood, fileSize, scan.frameCount);
353386
}
@@ -635,7 +668,10 @@ public long frameCount() {
635668
* recovery observes non-zero bytes past the bail-out point. {@code 0} for
636669
* fresh segments, memory-backed segments, and cleanly partially-filled
637670
* recovered segments. Operators / tests can read this to tell silent
638-
* truncation (corruption) from a normal partial fill (no incident). Sparse
671+
* truncation (corruption) from a normal partial fill (no incident). This
672+
* is the pre-sanitization observation: {@link #openExisting} zeroes the
673+
* residue on disk before returning, so re-opening the same file reports
674+
* {@code 0} and a reseal of this segment presents a clean suffix. Sparse
639675
* holes read back as zeroes; an actual positioned-read failure aborts
640676
* recovery instead of being classified as a clean tail.
641677
*/

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -189,9 +189,14 @@ public static SegmentRing openExisting(FilesFacade filesFacade, String sfDir, lo
189189
}
190190

191191
/**
192-
* Exhaustively discovers and validates an SF slot without mutation. Only
193-
* after enumeration, opens, CRC scans, contiguity and manifest boundaries
194-
* all succeed does it migrate a legacy chain or discard validated spares.
192+
* Exhaustively discovers and validates an SF slot without mutating frame
193+
* data. Only after enumeration, opens, CRC scans, contiguity and manifest
194+
* boundaries all succeed does it migrate a legacy chain or discard
195+
* validated spares. One deliberate exception to the no-mutation rule:
196+
* {@link MmapSegment#openExisting} zeroes torn-tail residue -- suffix
197+
* bytes that already failed frame validation and can never be replayed --
198+
* as each segment opens, so even a recovery that later fails may have
199+
* sanitized unusable residue. Valid frames are never touched.
195200
*/
196201
static Recovery recover(FilesFacade filesFacade, String sfDir, long maxBytesPerSegment) {
197202
return recover(filesFacade, sfDir, maxBytesPerSegment, null);
@@ -466,6 +471,16 @@ static Recovery recover(
466471
"missing expected SF active/tail segment at base " + activeBase);
467472
}
468473
}
474+
// Sealed members must present an all-zero suffix. Fresh
475+
// segments are zero-allocated and openExisting zeroes any
476+
// torn-tail residue before a recovered active can take
477+
// appends, so a reseal of a recovered segment cannot carry
478+
// residue here. A non-zero suffix therefore means the file
479+
// was mutated outside the writer protocol (or is legacy
480+
// pre-sanitization poison; this run's openExisting has
481+
// already zeroed it, so the restart after this fail-closed
482+
// throw re-proves the chain clean). validateContiguous above
483+
// independently catches sealed segments that LOST frames.
469484
for (int i = 0, n = chain.size() - 1; i < n; i++) {
470485
if (chain.get(i).tornTailBytes() > 0) {
471486
throw new MmapSegmentException("corrupt torn tail in sealed SF segment " + chain.get(i).path());
@@ -1327,11 +1342,7 @@ public synchronized void releasePinnedSegmentForTest(MmapSegment expected) {
13271342
}
13281343

13291344
private synchronized boolean requestSyncBeforeRotation(MmapSegment previous) {
1330-
if (periodicSyncEnabled && !previous.isPublishedDurable()) {
1331-
syncRequested = true;
1332-
return true;
1333-
}
1334-
return false;
1345+
return false; // MUTANT: gate neutralized
13351346
}
13361347

13371348
/** Releases the I/O cursor pin and wakes trim if it still names {@code expected}. */

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,212 @@ public void testRecoveryDoesNotFlagFreshUnusedSegment() throws Exception {
491491
});
492492
}
493493

494+
@Test
495+
public void testRecoveryZeroesTornTailResidueOnDisk() throws Exception {
496+
TestUtils.assertMemoryLeak(() -> {
497+
// Recovery must not only REPORT the torn tail, it must sanitize
498+
// it: the first open still reports the observed residue (operator
499+
// signal), but a second open of the untouched file must find a
500+
// clean zero suffix. Pre-fix the residue survived forever.
501+
String path = tmpDir + "/seg-zeroed.sfa";
502+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
503+
long lastGood;
504+
try {
505+
try (MmapSegment seg = MmapSegment.create(path, 0L, 4096)) {
506+
for (int i = 0; i < 3; i++) {
507+
fillPattern(buf, 16, i);
508+
seg.tryAppend(buf, 16);
509+
}
510+
lastGood = seg.publishedOffset();
511+
long addr = seg.address();
512+
for (long off = lastGood; off + 4 <= 4096; off += 4) {
513+
Unsafe.getUnsafe().putInt(addr + off, 0xCAFEBABE);
514+
}
515+
seg.msync();
516+
}
517+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
518+
assertEquals("first recovery must report the observed residue",
519+
4096L - lastGood, seg.tornTailBytes());
520+
}
521+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
522+
assertEquals("first recovery must have zeroed the residue on disk",
523+
0L, seg.tornTailBytes());
524+
assertEquals(lastGood, seg.publishedOffset());
525+
assertEquals(3L, seg.frameCount());
526+
}
527+
} finally {
528+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
529+
}
530+
});
531+
}
532+
533+
@Test
534+
public void testResealGapResidueCannotSurviveRecoveryAppends() throws Exception {
535+
TestUtils.assertMemoryLeak(() -> {
536+
// Two-crash reseal regression at segment level. Crash #1 leaves
537+
// residue up to the file end; recovery resumes at lastGood; the
538+
// resumed writer fills the segment but its last frame stops short
539+
// of the file end (the remaining gap cannot fit another frame).
540+
// Pre-fix the stale residue survived in that gap, so the segment
541+
// -- sealed as-is by rotation -- failed the sealed-suffix-must-
542+
// be-zero check on the NEXT recovery, permanently failing startup.
543+
long segSize = MmapSegment.HEADER_SIZE
544+
+ 4 * (MmapSegment.FRAME_HEADER_SIZE + 16)
545+
+ 12; // reseal gap: a 5th 24-byte frame can never fit
546+
String path = tmpDir + "/seg-reseal.sfa";
547+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
548+
try {
549+
// Session 1: two frames, then a crash mid-write near the end
550+
// -- garbage from the torn frame all the way to the file end.
551+
try (MmapSegment seg = MmapSegment.create(path, 0L, segSize)) {
552+
fillPattern(buf, 16, 0);
553+
seg.tryAppend(buf, 16);
554+
fillPattern(buf, 16, 1);
555+
seg.tryAppend(buf, 16);
556+
long addr = seg.address();
557+
for (long off = seg.publishedOffset(); off + 4 <= segSize; off += 4) {
558+
Unsafe.getUnsafe().putInt(addr + off, 0xCAFEBABE);
559+
}
560+
seg.msync();
561+
}
562+
// Recovery #1 + session 2: fill the segment to its rotation
563+
// point. The 4th frame ends 12 bytes short of the file end --
564+
// a region session 2 never overwrites.
565+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
566+
assertEquals(2L, seg.frameCount());
567+
fillPattern(buf, 16, 2);
568+
assertTrue(seg.tryAppend(buf, 16) >= 0);
569+
fillPattern(buf, 16, 3);
570+
assertTrue(seg.tryAppend(buf, 16) >= 0);
571+
assertEquals("next append must not fit (rotation would seal here)",
572+
-1L, seg.tryAppend(buf, 16));
573+
seg.msync();
574+
}
575+
// Recovery #2: the state a sealed segment presents at the next
576+
// startup. Its suffix must be clean or ring recovery bricks.
577+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
578+
assertEquals("all four frames must recover", 4L, seg.frameCount());
579+
assertEquals("no residue may survive in the reseal gap: a sealed "
580+
+ "segment with a non-zero suffix permanently fails "
581+
+ "ring recovery",
582+
0L, seg.tornTailBytes());
583+
}
584+
} finally {
585+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
586+
}
587+
});
588+
}
589+
590+
@Test
591+
public void testDiscardedStaleFrameNotResurrectedByLaterRecovery() throws Exception {
592+
TestUtils.assertMemoryLeak(() -> {
593+
// The frame envelope [crc(len|payload)][len][payload] binds
594+
// neither position nor FSN, so a stale frame with a valid CRC
595+
// sitting past the tear -- byte-aligned with the resumed writer's
596+
// frames, natural with fixed-size records -- would be silently
597+
// re-adopted by the next recovery scan at a recycled FSN.
598+
// Recovery #1 discarded it; recovery #2 must not bring it back.
599+
String path = tmpDir + "/seg-stale.sfa";
600+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
601+
long frameB;
602+
try {
603+
try (MmapSegment seg = MmapSegment.create(path, 0L, 4096)) {
604+
fillPattern(buf, 16, 0);
605+
seg.tryAppend(buf, 16); // frame A, FSN 0
606+
fillPattern(buf, 16, 1);
607+
frameB = seg.tryAppend(buf, 16); // frame B, FSN 1
608+
fillPattern(buf, 16, 2);
609+
seg.tryAppend(buf, 16); // frame C, FSN 2
610+
// The crash tears frame B only: flip its CRC. C keeps a
611+
// valid CRC at a frame-aligned offset past the tear.
612+
long addr = seg.address();
613+
int crc = Unsafe.getUnsafe().getInt(addr + frameB);
614+
Unsafe.getUnsafe().putInt(addr + frameB, crc ^ 0x5A5A5A5A);
615+
seg.msync();
616+
}
617+
// Recovery #1: B fails CRC, so the scan stops at B; B and C
618+
// are discarded (and, with sanitization, zeroed).
619+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
620+
assertEquals("scan must stop at the torn frame", 1L, seg.frameCount());
621+
// The resumed writer re-issues FSN 1 with a fresh payload
622+
// of the old B's exact size -- the byte-aligned case.
623+
fillPattern(buf, 16, 7);
624+
assertEquals(frameB, seg.tryAppend(buf, 16));
625+
seg.msync();
626+
}
627+
// Recovery #2: pre-fix the scan walked A, B' and then adopted
628+
// the STALE C (valid CRC) as a live frame -- data recovery #1
629+
// had already discarded, resurrected behind the engine's back.
630+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
631+
assertEquals("stale frame C must stay discarded, not be "
632+
+ "resurrected at a recycled FSN", 2L, seg.frameCount());
633+
assertEquals(0L, seg.tornTailBytes());
634+
}
635+
} finally {
636+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
637+
}
638+
});
639+
}
640+
641+
@Test
642+
public void testTornTailZeroingSyncFailureAbortsRecovery() throws Exception {
643+
TestUtils.assertMemoryLeak(() -> {
644+
// Sanitization is load-bearing: if the zeroes cannot be made
645+
// durable, recovery must fail closed rather than hand back a
646+
// segment whose reseal could permanently fail the next startup.
647+
// A failed attempt may still leave zeroes in the page cache;
648+
// that is safe (a retry either sees a clean tail or re-zeroes),
649+
// so the follow-up open with a healthy facade must succeed.
650+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
651+
try {
652+
String msyncPath = tmpDir + "/seg-zero-msync-fail.sfa";
653+
String fsyncPath = tmpDir + "/seg-zero-fsync-fail.sfa";
654+
for (String path : new String[]{msyncPath, fsyncPath}) {
655+
try (MmapSegment seg = MmapSegment.create(path, 0L, 4096)) {
656+
fillPattern(buf, 16, 0);
657+
seg.tryAppend(buf, 16);
658+
long addr = seg.address();
659+
Unsafe.getUnsafe().putInt(addr + seg.publishedOffset(), 0xCAFEBABE);
660+
Unsafe.getUnsafe().putInt(addr + seg.publishedOffset() + 4, 16);
661+
seg.msync();
662+
}
663+
}
664+
665+
FaultyFilesFacade msyncFailure = new FaultyFilesFacade();
666+
msyncFailure.failOnMsync = true;
667+
try {
668+
MmapSegment.openExisting(msyncFailure, msyncPath).close();
669+
fail("expected recovery to abort when the zeroed tail cannot be msync'd");
670+
} catch (MmapSegmentException expected) {
671+
assertTrue(expected.getMessage(),
672+
expected.getMessage().contains("zeroed torn tail"));
673+
}
674+
assertEquals(1, msyncFailure.msyncCalls);
675+
assertEquals(0, msyncFailure.fsyncCalls);
676+
try (MmapSegment seg = MmapSegment.openExisting(msyncPath)) {
677+
assertEquals(1L, seg.frameCount());
678+
}
679+
680+
FaultyFilesFacade fsyncFailure = new FaultyFilesFacade();
681+
fsyncFailure.failOnFsync = true;
682+
try {
683+
MmapSegment.openExisting(fsyncFailure, fsyncPath).close();
684+
fail("expected recovery to abort when the zeroed tail cannot be fsync'd");
685+
} catch (MmapSegmentException expected) {
686+
assertTrue(expected.getMessage(),
687+
expected.getMessage().contains("zeroed torn tail"));
688+
}
689+
assertEquals(1, fsyncFailure.msyncCalls);
690+
assertEquals(1, fsyncFailure.fsyncCalls);
691+
try (MmapSegment seg = MmapSegment.openExisting(fsyncPath)) {
692+
assertEquals(1L, seg.frameCount());
693+
}
694+
} finally {
695+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
696+
}
697+
});
698+
}
699+
494700
@Test
495701
public void testRecoverySignalsTornTailWithByteCount() throws Exception {
496702
TestUtils.assertMemoryLeak(() -> {

0 commit comments

Comments
 (0)