Route recovery header-fault tests via SegmentRing

The SF store-and-forward recovery header-fault test flaked the JDK 8
CI. It called MmapSegment.openExisting directly from the test lambda,
so under C2 HotSpot inlined openExisting into the handler-less test
frame and delivered the async "recent unsafe memory access"
InternalError past openExisting's own catch -- the conversion to a
MmapSegmentException never ran and a raw InternalError escaped,
failing the test.

Route the two header-skippable tests through SegmentRing.openExisting,
production's real recovery caller. There the faulting header read sits
behind a genuine, non-inlined call frame whose catch converts the
fault to a MmapSegmentException, which SegmentRing's per-file catch
skips -- so recovery of the sole unreadable segment yields no ring,
exactly as in production. Fail-on-revert is preserved: revert the
header-block conversion and the raw InternalError escapes SegmentRing
and the test errors.

Add a facade-aware SegmentRing.openExisting(FilesFacade, ...) overload
so the length-injecting test facade reaches MmapSegment.openExisting,
mirroring MmapSegment's own existing facade seam. The INSTANCE
overload production uses is unchanged.

Verified with the client's native lib built locally: the routed header
tests pass under full-suite JIT warmup on JDK 11 (6/6, the pre-21
proxy for the JDK 8 CI) and on JDK 17 (13/13).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: glasstiger

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));
         });
     }
 
@@ -270,13 +281,30 @@ 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
+     * guard, driven through the real recovery entry point
+     * {@link SegmentRing#openExisting}. 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.
+     * {@link MmapSegment#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}.
+     * catches to skip just this {@code .sfa} -- so recovery of the sole
+     * (unreadable) segment yields no ring rather than aborting or surfacing a
+     * raw {@code InternalError}. Revert the header-block conversion and the raw
+     * {@code InternalError} escapes {@code openExisting}, propagates out of
+     * {@code SegmentRing}'s recovery loop, and this errors instead of returning
+     * {@code null}.
+     * <p>
+     * Routing through {@code SegmentRing} rather than calling
+     * {@code MmapSegment.openExisting} directly is deliberate, and is what makes
+     * this test stable on the JDK 8 CI. It mirrors production, where the
+     * faulting header read sits behind a real (non-inlined) call frame whose
+     * {@code catch (Throwable)} runs the fault-to-{@code MmapSegmentException}
+     * conversion. A direct call instead lets HotSpot's C2 inline
+     * {@code openExisting} into the handler-less test frame and deliver the
+     * async "recent unsafe memory access" {@code InternalError} past that
+     * inlined catch -- an artifact of the test call shape, not of any
+     * production path -- which flaked the direct-call form on the larger
+     * (JIT-warming) CI suite.
      */
     @Test
     public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Exception {
@@ -285,16 +313,15 @@ 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.
-            }
+            // SegmentRing opens the sole segment through the facade, catches the
+            // converted MmapSegmentException, skips the file, and returns no ring.
+            assertNull("a beyond-EOF header page must be skipped, not recovered or fatal",
+                    SegmentRing.openExisting(ff, tmpDir, SEGMENT_BYTES));
         });
     }