chore(qwp): de-flake the SF recovery header-fault test

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.
+     * <p>
+     * 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

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).
  * <p>
- * These tests drive the <b>production entry point</b> ({@code openExisting}),
- * not the private scan methods via reflection. That matters for two reasons:
+ * These tests drive the <b>production entry points</b> -- 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:
  * <ul>
  *   <li>It exercises the real recovery path end to end.</li>
  *   <li>On pre-21 JDKs the mmap-fault {@code InternalError} is delivered
@@ -86,7 +91,7 @@
  * mmap-fault guard reverted -- no regression protection on the ext4/xfs CI
  * runners. The two {@code MapPastEof} tests below close that gap portably.
  * They truncate the file <em>down</em> (freeing the tail blocks) and hand
- * {@code openExisting} a {@link FilesFacade} that reports the original, larger
+ * recovery a {@link FilesFacade} that reports the original, larger
  * length, so the mapping extends past real end-of-file. A read of a page beyond
  * real EOF raises SIGBUS on <em>every</em> filesystem -- the same catchable
  * {@code InternalError} an unbacked ZFS page raises -- so they exercise the
@@ -181,7 +186,7 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception {
      * M1 regression: the header block (magic/version/baseSeq) is read before
      * {@code scanFrames}, so an unbacked page 0 faults ahead of the guarded
      * scan. {@link MmapSegment#openExisting} must surface that as a
-     * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing}
+     * {@link MmapSegmentException} -- the per-file signal {@link SegmentRing}
      * catches to skip just this {@code .sfa} -- and never let the raw
      * {@code InternalError} escape and abort recovery of every sibling segment.
      * <p>
@@ -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.
+     * <p>
+     * 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