Skip to content

Commit 2721f39

Browse files
puzpuzpuzclaude
andcommitted
fix(qwp): guard header reads in recovery; drop the sparse-tail-only WARN framing
M1: openExisting reads magic/version/baseSeq before scanFrames with no guard, so an unbacked page 0 (unflushed header on a truncate-based-allocate FS after a crash, or a file truncated under the mapping) faults with an InternalError that SegmentRing's per-file catch (MmapSegmentException) does not catch -- it escapes to the outer catch (Throwable) and aborts recovery of every sibling segment. Convert an isMmapAccessFault into a MmapSegmentException in openExisting's catch so only that one .sfa is skipped. Adds a portable regression test. M2: the scanFrames in-mapping-fault WARN framed the outcome as "Expected when a prior session left a sparse segment tail", downplaying a possible mid-file media error. Reword it to state frames beyond the fault are discarded and that a bad sector is indistinguishable here, and document that tornTailBytes() reports 0 for an unbacked bail-out region by design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 779dfd0 commit 2721f39

2 files changed

Lines changed: 63 additions & 2 deletions

File tree

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

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,23 @@ public static MmapSegment openExisting(String path) {
310310
Files.munmap(addr, fileSize, MemoryTag.MMAP_DEFAULT);
311311
}
312312
Files.close(fd);
313+
// The header reads above (magic/version/baseSeq) run before
314+
// scanFrames and are otherwise unguarded. An unbacked page 0 --
315+
// an unflushed header on a truncate-based-allocate filesystem
316+
// after a crash (create() does not msync), or a file truncated
317+
// under the mapping -- faults at the Unsafe intrinsic site as a
318+
// catchable InternalError. Convert it to a MmapSegmentException so
319+
// SegmentRing's per-file catch skips just this .sfa, instead of
320+
// letting the raw InternalError escape to SegmentRing's outer catch
321+
// and abort recovery of every sibling segment. scanFrames and
322+
// detectTornTail already handle their own in-mapping faults; this
323+
// covers the header block and any future reader placed ahead of the
324+
// scan.
325+
if (isMmapAccessFault(t)) {
326+
throw new MmapSegmentException(
327+
"unreadable mapped header page in " + path
328+
+ " (unbacked/sparse page 0): " + t.getMessage(), t);
329+
}
313330
throw t;
314331
}
315332
}
@@ -502,6 +519,13 @@ public long frameCount() {
502519
* fresh segments, memory-backed segments, and cleanly partially-filled
503520
* recovered segments. Operators / tests can read this to tell silent
504521
* truncation (corruption) from a normal partial fill (no incident).
522+
* <p>
523+
* One case this does NOT count: when the scan stops because the bail-out
524+
* region is itself an unbacked mapped page (an in-mapping fault, not a
525+
* backed torn header), that region cannot be probed, so this returns
526+
* {@code 0} even though frames may have been discarded. That outcome is
527+
* surfaced by the {@code WARN} in {@link #scanFrames} instead -- see it for
528+
* the benign-tail-vs-media-error caveat.
505529
*/
506530
public long tornTailBytes() {
507531
return tornTailBytes;
@@ -595,8 +619,12 @@ private static long scanFrames(long addr, long fileSize) {
595619
throw e;
596620
}
597621
LOG.warn("SF segment recovery: unreadable mapped page at offset {} (file size {}); "
598-
+ "treating it as the end of recoverable data. Expected when a prior "
599-
+ "session left a sparse segment tail; investigate disk health if it recurs.",
622+
+ "treating it as the end of recoverable data -- any frames beyond this "
623+
+ "offset are discarded. The usual cause is a benign sparse pre-allocation "
624+
+ "tail (e.g. a truncate-based allocate on ZFS) left by a prior session, "
625+
+ "but a mid-file media error (bad sector) is indistinguishable here; "
626+
+ "check disk health if this segment was expected to be fully written or "
627+
+ "if this recurs.",
600628
pos, fileSize);
601629
}
602630
return pos;

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
package io.questdb.client.test.cutlass.qwp.client.sf.cursor;
2626

2727
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment;
28+
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException;
2829
import io.questdb.client.std.Files;
2930
import io.questdb.client.std.MemoryTag;
3031
import io.questdb.client.std.Unsafe;
@@ -37,6 +38,7 @@
3738

3839
import static org.junit.Assert.assertEquals;
3940
import static org.junit.Assert.assertTrue;
41+
import static org.junit.Assert.fail;
4042

4143
/**
4244
* Regression guard for the recovery-time SIGBUS hazard in {@link MmapSegment}.
@@ -176,6 +178,37 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception {
176178
});
177179
}
178180

181+
/**
182+
* M1 regression: the header block (magic/version/baseSeq) is read before
183+
* {@code scanFrames}, so an unbacked page 0 faults ahead of the guarded
184+
* scan. {@link MmapSegment#openExisting} must surface that as a
185+
* {@link MmapSegmentException} -- the per-file signal {@code SegmentRing}
186+
* catches to skip just this {@code .sfa} -- and never let the raw
187+
* {@code InternalError} escape and abort recovery of every sibling segment.
188+
* <p>
189+
* Portable across filesystems: on a hole-faulting FS (ZFS) the fault is
190+
* converted to a {@code MmapSegmentException} in {@code openExisting}'s
191+
* catch; on a hole-zero-filling FS (ext4) page 0 reads back as zeroes, so
192+
* the magic check fails and throws {@code MmapSegmentException} directly.
193+
* Either way the file is skippable, not fatal.
194+
*/
195+
@Test
196+
public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception {
197+
TestUtils.assertMemoryLeak(() -> {
198+
final String path = tmpDir + "/seg-unbacked-header.sfa";
199+
writeSegment(path, 3L, new int[]{64});
200+
// Punch the whole file into a hole -- page 0 (the header) included.
201+
punchSparseTail(path, 0L);
202+
try {
203+
MmapSegment.openExisting(path).close();
204+
fail("expected MmapSegmentException for an unbacked header page");
205+
} catch (MmapSegmentException expected) {
206+
// ok -- SegmentRing's per-file catch skips just this file
207+
// instead of aborting recovery of the whole slot.
208+
}
209+
});
210+
}
211+
179212
/**
180213
* Creates a segment at {@code path} and appends one frame per entry in
181214
* {@code payloadLens} (each filled with non-zero bytes so recovery can tell

0 commit comments

Comments
 (0)