fix(qwp): recover an SF slot whose segment tail is an unbacked mapped page

[puzpuzpuz]

Problem

MmapSegment.openExisting maps a recovered .sfa file to its stat length and scans it in place: scanFrames and detectTornTail read the mapping via Unsafe. When a prior session left a sparse segment tail — a truncate-based pre-allocation that never materialized the tail blocks, which is what happens on filesystems like ZFS — reading an unbacked page raises the JVM's recoverable InternalError("a fault occurred in an unsafe memory access operation") (a translated SIGBUS).

That InternalError is not a MmapSegmentException, so SegmentRing.openExisting's per-file skip did not catch it. It propagated to the outer catch and aborted recovery of the whole slot. In practice this surfaced through a BackgroundDrainer (or a slot-lock probe) as a spurious "unsafe memory access" failure — observed as an intermittent failure of BackgroundDrainerPoolInterruptedCloseTest on the ZFS CI runner (linux-x64-zfs), and green on ext4/xfs runners because those zero-fill mmap reads of unwritten regions instead of faulting.

Fix

scanFrames and detectTornTail now catch the mmap access fault and treat the unreadable region exactly like unwritten space or a torn tail — the boundary of recoverable data. Every frame verified below the fault is kept and the slot recovers, instead of the whole recovery aborting. This mirrors the existing torn-tail semantics: a region past the last valid frame is a boundary, not a fatal error.

isMmapAccessFault matches on the JVM's stable message string, so a genuine VirtualMachineError (real OOM, StackOverflowError) is never swallowed — only the documented, recoverable mmap-access fault is.

Tradeoffs and scope

• The catch is deliberately narrow (message match on InternalError). If a future JDK changes that message the guard would stop matching and the pre-fix behavior (abort recovery) would return — no silent data corruption, just the old failure mode.
• If a page were only transiently unreadable mid-file rather than a genuine sparse tail, the scan would stop at that offset and drop frames above it. This matches the file's write ordering (frames are contiguous from the header; an unbacked page is always at or past the true tail), and it is strictly better than the current behavior, which drops the entire slot's data.
• This does not change the allocation path. create still pre-allocates via FilesFacade.allocate; on filesystems where the native posix_fallocate/F_PREALLOCATE is unavailable and it falls back to ftruncate, the sparse tail can still occur — this change makes recovery survive it rather than eliminating it.

Test plan

• New MmapSegmentRecoveryFaultTest reproduces the fault deterministically on any filesystem: it maps a valid one-frame segment, truncates the backing under the still-larger mapping so the tail page is beyond EOF (the one case POSIX mmap always faults on), then asserts scanFrames stops at that page and keeps the frame below it, and detectTornTail reports clean. The test errors with the exact InternalError: a fault occurred in an unsafe memory access operation before the fix and passes after (verified by reverting the production change).
• MmapSegmentTest (13), SegmentRingRecoveryUnlinkTest (2) and BackgroundDrainerPoolInterruptedCloseTest (the test that flaked on ZFS) all pass locally.

🤖 Generated with Claude Code

[puzpuzpuz]

Code review — level 3 (full pass, per-finding source verification)

Re-review of the current state of the branch (three commits past the initial version), verifying the follow-up fixes independently — including deriving the hand-rolled CRC by hand and running experiments on JDK 17/21/25.

Scope: 5 files, no binaries (committed-binary gate passes). MmapSegment.java (+156/−17), SegmentRing.java (+10/−3), new MmapSegmentRecoveryFaultTest.java (+234), MmapSegmentTest.java dedup (+2/−24), TestUtils.java helper (+43).

Verdict: Approve with minor follow-ups. No Critical or correctness findings.

The production code is correct. The remaining issues are in the test safety-net and CI/verification matrix, not the shipping code — but one I proved empirically and it matters for a mission-critical, hard-to-reproduce fix.

---

Moderate

M1 — The three new regression tests provide zero fail-on-revert protection on ext4/xfs (in-diff, empirically proven)

I reverted both production files to the pre-branch base, kept the new test, and ran it on an ext4 host: Tests run: 3, Failures: 0, Errors: 0 — all green without the fix. Reason: ext4/xfs zero-fill a within-EOF hole, so scanFrames takes the CRC-mismatch branch (crcCalc != crcRead → return pos) and openExisting takes the bad-magic branch — both behave identically with and without the try/catch (InternalError). The fault path is only reached on a hole-faulting filesystem (ZFS).

Since the bug is ZFS-specific and the main CI runners are ext4/xfs (green there; only flaked on linux-x64-zfs), CI on its primary runners will not catch a future regression of this fix. The guard is only real if the ZFS runner is a required check.

Suggested fix (pick one):

• Make the ZFS runner a required check for this path, or
• Give openExisting a FilesFacade/length-injection seam (the create path already takes a FilesFacade) so a fault-injecting facade can throw the InternalError("...unsafe memory access operation...") deterministically on any filesystem — a portable, cross-FS regression guard.

This is the difference between the fix being permanently guarded and being one refactor away from silently regressing on the exact ZFS path it targets.

M2 — Fix mechanism unverified on JDK 8 (the shipping/CI source-of-truth) (verification gate, low residual risk)

The fix depends on HotSpot delivering a catchable InternalError (message containing "unsafe memory access operation") at an Unsafe access, and on a direct try/catch catching it. Verified empirically:

JDK	fragment match	direct try/catch catches (JIT)	catches (-Xint)
17 (pre-21 wording, JDK-8 proxy)	✅ "...a recent unsafe memory access operation in compiled Java code"	✅	✅
21 / 25	✅	✅	✅

JDK 17 shares JDK 8's exact pre-21 wording and the async-delivery quirk, and catches the fault in both interpreter and JIT — so residual risk is low. But JDK 8 is the shipping/CI target and the original flake was on a JDK-8 ZFS runner, so the new test must be run on JDK 8 (ideally the ZFS runner) before merge. I could not run JDK 8 locally (only 17/21/25). (The imprecise pre-21 delivery only escapes a reflective Method.invoke frame — production and the rewritten tests use direct calls, so they're unaffected.)

Minor

m1 — PR description is stale and its test-plan claim is misleading

The body's Fix section describes only the original scanFrames/detectTornTail catch and omits the three largest/riskiest parts of the current diff: the CRC-over-Unsafe fold replacing native Crc32c on the recovery path, the openExisting header-read guard, and the JDK-8 message-fragment broadening. The Test plan describes a "beyond EOF" truncation, but the shipped test uses truncate-down-then-up (a within-EOF sparse hole), and claims the test "errors … before the fix and passes after" — which is false on ext4 (green before and after; holds only on ZFS). Please update the description to match the shipped change and qualify the revert-claim as ZFS-only.

m2 — Cosmetic

• buildCrc32cTable (MmapSegment.java:651) uses a // block comment while sibling private helpers isMmapAccessFault/crc32cRecovery use /** */ javadoc.
• MmapSegmentRecoveryFaultTest.java:144 message "frame 2's header must end on the page boundary" is imprecise — the header ends 8 bytes below the boundary (the asserted value boundary - 8 is correct; only the wording is off).

---

Verified correct — do not re-chase

• Hand-rolled CRC bit-correctness — derived table entry [1] = 0xF26B8303 by hand (matches native crc32c_table[1]); confirmed seed/complement (~INIT…~crc), the signed-byte-masked-by-0xFF, and that crc32cRecovery(addr+pos+4, 4+payloadLen) reproduces exactly what tryAppend stores. Triple-confirmed: the passing test writes a frame with native CRC and accepts it with Unsafe CRC — a single-bit disagreement would reject it.
• Read bounds — every Unsafe read in scan/CRC/detect/header stays within [addr, addr+fileSize) (bounds-checked), so a fault is always a genuinely-unbacked page correctly treated as a data boundary — never a wild-pointer swallow.
• countFrames/findLastFrameFsnWithoutPayloadFlag (unguarded) — safe; they only walk the already-CRC-verified (backed) region.
• Empty-hot-spare deletion of real data — unreachable; page granularity makes header-backed ⟹ first-frame-backed, so a fault can't produce frameCount==0 on a file with real data.
• Resource leaks / double-free across openExisting↔SegmentRing on all paths — none. punchSparseTail asserts fd >= 0 before the try, so no fd leak.
• Concurrency — recovery is single-threaded; CRC32C_TABLE safely published via class-init.
• Dedup — TestUtils.createTmpDir/removeTmpDir extracted verbatim; MmapSegmentTest (13) + SegmentRingRecoveryUnlinkTest (2) still green.

---

Summary

• Verdict: Approve; address M1 (regression-guard the fix on ZFS or via fault injection) and run the test on JDK 8 (M2) before merge; m1/m2 are quick cleanups.
• Out-of-diff correctness bugs: zero — a real conclusion, not an underrun: the changed symbols are private helpers plus one recovery caller (SegmentRing), whose every callsite I walked (openExisting recovery loop + tornTailBytes() consumer at SegmentRing.java:207).
• The production code is correct and verified deeply, including empirically on JDK 17/21/25. The gaps are entirely in the test/CI safety net for a filesystem- and JDK-specific fix.

_{🤖 Generated with Claude Code}

[mtopolnik]

[PR Coverage check]

😍 pass : 44 / 46 (95.65%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java	42	44	95.45%
🔵	io/questdb/client/std/DefaultFilesFacade.java	1	1	100.00%
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java	1	1	100.00%