diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index c8516c4c..6bada043 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -25,6 +25,7 @@ package io.questdb.client.cutlass.qwp.client.sf.cursor; import io.questdb.client.std.Files; +import io.questdb.client.std.FilesFacade; import io.questdb.client.std.ObjList; import io.questdb.client.std.QuietCloseable; import org.jetbrains.annotations.TestOnly; @@ -155,6 +156,27 @@ public SegmentRing(MmapSegment initialActive, long maxBytesPerSegment) { * depends on every segment being readable. */ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { + return openExisting(FilesFacade.INSTANCE, sfDir, maxBytesPerSegment); + } + + /** + * Facade-aware variant of {@link #openExisting(String, long)} that opens + * each recovered segment through the given {@link FilesFacade} -- via + * {@link MmapSegment#openExisting(FilesFacade, String)} -- instead of + * {@link FilesFacade#INSTANCE}. Production calls the {@code INSTANCE} + * overload above; this seam exists so recovery's mmap-fault handling can be + * regression-tested end to end on any filesystem. A facade that reports an + * inflated length maps a segment past real end-of-file, so the recovery + * read faults deterministically -- the same catchable {@code InternalError} + * an unbacked/sparse page raises on ZFS, but reproduced on ext4/xfs where a + * within-EOF hole would instead zero-fill and never exercise the guard. + *
+ * Only the per-segment open is routed through {@code ff}; directory + * enumeration and the empty-orphan unlink/quarantine stay on {@link Files}, + * mirroring {@code MmapSegment.openExisting}'s own partial seam (which keeps + * {@code mmap} on {@code Files}). + */ + public static SegmentRing openExisting(FilesFacade ff, String sfDir, long maxBytesPerSegment) { if (!Files.exists(sfDir)) { return null; } @@ -182,7 +204,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { String path = sfDir + "/" + name; MmapSegment seg = null; try { - seg = MmapSegment.openExisting(path); + seg = MmapSegment.openExisting(ff, path); // Filter out empty leftovers -- typically hot-spare // segments the manager pre-allocated for a prior // session that never got rotated into active. They diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java index 861f761c..d2ea21c7 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java @@ -26,6 +26,7 @@ import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing; import io.questdb.client.std.Files; import io.questdb.client.std.FilesFacade; import io.questdb.client.std.MemoryTag; @@ -36,8 +37,8 @@ import org.junit.Test; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; /** * Regression guard for the recovery-time SIGBUS hazard in {@link MmapSegment}. @@ -52,8 +53,12 @@ * keep every frame below it, and hand back a usable segment -- not let the * error abort recovery of the whole slot (the reported ZFS-CI flake). *
- * These tests drive the production entry point ({@code openExisting}), - * not the private scan methods via reflection. That matters for two reasons: + * These tests drive the production entry points -- the two + * header-skippable cases go through the outermost one, + * {@code SegmentRing.openExisting} (production's real recovery caller, whose + * per-file catch does the skip), and the rest through + * {@code MmapSegment.openExisting} -- not the private scan methods via + * reflection. That matters for two reasons: *
@@ -190,6 +195,15 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { * catch; on a hole-zero-filling FS (ext4) page 0 reads back as zeroes, so * the magic check fails and throws {@code MmapSegmentException} directly. * Either way the file is skippable, not fatal. + *
+ * Driven through {@link SegmentRing#openExisting} -- the real recovery path + * that performs the per-file skip -- so the assertion is exactly the + * production outcome: the sole unreadable segment is skipped and recovery + * yields no ring. Going through {@code SegmentRing} also keeps the faulting + * header read behind a real (non-inlined) call frame, so that on ZFS + * HotSpot's C2 cannot deliver the async unsafe-access fault past + * {@code openExisting}'s catch the way a direct call can -- see + * {@link #testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem}. */ @Test public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { @@ -198,13 +212,10 @@ public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { writeSegment(path, 3L, new int[]{64}); // Punch the whole file into a hole -- page 0 (the header) included. punchSparseTail(path, 0L); - try { - MmapSegment.openExisting(path).close(); - fail("expected MmapSegmentException for an unbacked header page"); - } catch (MmapSegmentException expected) { - // ok -- SegmentRing's per-file catch skips just this file - // instead of aborting recovery of the whole slot. - } + // The sole .sfa is unreadable (a faulting or zeroed header page), so + // SegmentRing skips it and recovery produces no ring at all. + assertNull("an unbacked header page must be skipped, not recovered or fatal", + SegmentRing.openExisting(tmpDir, SEGMENT_BYTES)); }); } @@ -269,14 +280,7 @@ public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception { } /** - * Portable fail-on-revert guard for the {@code openExisting} header-block - * guard. The file is truncated to empty and the facade reports a full page, - * so the very first header read (magic) lands on a beyond-EOF page and - * faults on any filesystem. {@code openExisting} must convert that to a - * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} - * skips on -- not let the raw {@code InternalError} escape and abort recovery - * of every sibling. Revert the header-block conversion and this throws - * {@code InternalError} instead of {@code MmapSegmentException}. + * Verifies that recovery skips a segment whose first mapped header page lies beyond EOF. */ @Test public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Exception { @@ -285,16 +289,24 @@ public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Excepti final long page = Files.PAGE_SIZE; writeSegment(path, 9L, new int[]{64}); // Free every block: the file is now empty, so even page 0 (the - // header) is beyond EOF under the reported one-page mapping. + // header) is beyond EOF under the reported one-page mapping. The + // facade reports a full page, so openExisting maps that beyond-EOF + // page and the header read faults on it on any filesystem. truncateTo(path, 0L); - FilesFacade ff = new MapPastEofFacade(path, page); - try { - MmapSegment.openExisting(ff, path).close(); - fail("expected MmapSegmentException for a beyond-EOF header page"); - } catch (MmapSegmentException expected) { - // ok -- SegmentRing's per-file catch skips just this file - // instead of aborting recovery of the whole slot. - } + MapPastEofFacade ff = new MapPastEofFacade(path, page); + // Keep the production SegmentRing call shape: a direct call can be inlined by JDK 8 C2 and let the + // asynchronous unsafe-access InternalError bypass the conversion in this test-only call frame. + assertNull("a beyond-EOF header page must be skipped, not recovered or fatal", + SegmentRing.openExisting(ff, tmpDir, SEGMENT_BYTES)); + // Guard against a silent pass: the skip must be the beyond-EOF mmap + // fault, not a "file shorter than header" throw. That holds only if + // the facade's inflated length actually reached the recovery mapping + // -- i.e. the path SegmentRing rebuilds (sfDir + "/" + name) matched + // targetPath. If that coupling ever broke, openExisting would see the + // real length 0 and skip via a throw that survives reverting the + // mmap-fault guard, gutting this fail-on-revert guard unnoticed. + assertTrue("the injected beyond-EOF length must have driven the recovery mapping", + ff.isInflatedLengthServed); }); } @@ -367,6 +379,10 @@ private static void truncateTo(String path, long keepBytes) { * {@link FilesFacade#INSTANCE}. */ private static final class MapPastEofFacade implements FilesFacade { + // Set once length() has served the inflated length for targetPath, i.e. + // the injection actually reached the recovery mapping. A test asserts + // this so a broken path match cannot let the guard pass vacuously. + private boolean isInflatedLengthServed; private final long reportedLength; private final String targetPath; @@ -442,7 +458,11 @@ public long length(long pathPtr) { @Override public long length(String path) { - return targetPath.equals(path) ? reportedLength : INSTANCE.length(path); + if (targetPath.equals(path)) { + isInflatedLengthServed = true; + return reportedLength; + } + return INSTANCE.length(path); } @Override