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: the header block (magic/version/baseSeq), scanFrames, and detectTornTail all 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 whose message contains "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

Recovery now treats an unbacked/unreadable mapped page as the boundary of recoverable data — keeping every frame verified below it and returning a usable segment — instead of letting the fault abort the slot. Four parts:

• Scan-path guard. scanFrames and detectTornTail catch the mmap access fault and stop at that offset, exactly as they already treat unwritten space or a torn tail. Frames below the fault are kept; this mirrors the existing torn-tail semantics (a region past the last valid frame is a boundary, not a fatal error).
• CRC folded over Unsafe on the recovery path. The recovery-time frame CRC is now computed by crc32cRecovery, a table-driven CRC-32C read through Unsafe (bit-identical to the native Crc32c, verified). A torn write can leave a real payloadLen whose payload spans a backed→unbacked page edge; the native JNI Crc32c reading across that edge raises a SIGBUS HotSpot cannot translate, aborting the whole JVM. Folding the CRC through Unsafe makes that fault a catchable InternalError instead. Native Crc32c stays on the append hot path (recovery is a cold scan).
• openExisting header-read guard. The header reads run before scanFrames, so an unbacked page 0 faults ahead of the scan guard. openExisting's catch now converts an mmap fault into a MmapSegmentException, so SegmentRing skips just that .sfa rather than aborting recovery of every sibling segment.
• JDK-8-safe message match. isMmapAccessFault matches the fragment "unsafe memory access operation", present in both the pre-21 wording ("a fault occurred in a recent unsafe memory access operation in compiled Java code", JDK 8/11/17) and the 21+ wording ("a fault occurred in an unsafe memory access operation"). This makes the guard live on JDK 8 — the shipping/CI Java floor and the exact JDK the original flake was reported on. (The previous needle matched only 21+.)

A genuine VirtualMachineError (real OOM, StackOverflowError) is never swallowed — only the documented, recoverable mmap-access fault is (the match is message-gated on InternalError).

Tradeoffs and scope

• The catch is deliberately narrow (message match on InternalError). The matched fragment is shared across every current JDK (8→25); if a future JDK reworded it again the guard would stop matching and the pre-fix behavior (abort recovery) would return — no silent data corruption, just the old failure mode.
• Dropping frames above the fault is safe: frames are contiguous from the header, so an unbacked page is always at or past the true tail, and the dropped frames are the highest-FSN, never-acked ones. When the bail-out region's bytes are non-zero (a torn write, not a clean hole) it is reported via tornTailBytes(); the scanFrames WARN names both the benign sparse-tail cause and the indistinguishable mid-file media-error (bad-sector) cause, and asks operators to check disk health.
• This does not change the allocation path. create still pre-allocates via FilesFacade.allocate; 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.

How this relates to #65

#65 fixes a different bug with the same signature. Both issues surface as an intermittent java.lang.InternalError: a fault occurred in an unsafe memory access operation out of drainer/teardown tests, which is why they were initially easy to conflate — but the root causes are independent. Here the fault is read-side and environmental: a sparse segment tail left by a truncate-based pre-allocation faults when recovery maps and scans the file on a hole-faulting filesystem (ZFS); no other thread is involved, the file is simply not fully backed. In #65 the fault is a shutdown lifecycle race: SegmentManager.close() called on an already-interrupted thread abandoned its live worker, and the leaked worker kept truncating/unlinking segment files underneath a later CursorSendEngine mapping — a fault that can hit on any filesystem, sparse tail or not.

The fixes are complementary and neither subsumes the other. #65 removes one producer of the fault (a leaked worker mutating files it no longer owns) but does nothing for sparse tails, which would still fault recovery on ZFS without this PR. Conversely, this PR hardens only the recovery consumer (MmapSegment.openExisting treats a faulting page as a data boundary); the send-path mapping #65's race hit is outside its scope — and rightly so, since the correct fix there is to eliminate the race, not to catch the fault. For future triage of an unsafe memory access report: a fault during recovery on a hole-faulting FS points here; a fault during send/teardown with an interrupt in play points at #65.

Test plan

New MmapSegmentRecoveryFaultTest drives the production entry point (openExisting), not the private scan methods via reflection — reflection changes fault delivery on pre-21 JDKs and would spuriously fail on the shipping JDK. It covers the unbacked tail via two complementary mechanisms:

• Sparse-hole cases (punchSparseTail: truncate the file down, then back up to the mapping size, leaving a within-EOF hole): reproduce the ZFS fault directly. On a hole-faulting FS (ZFS) the read faults; on a hole-zero-filling FS (ext4/xfs) the hole reads back as zeroes and the scan stops via the CRC-mismatch / bad-magic branch. These are only fail-on-revert on ZFS — on ext4/xfs they stay green with the guard reverted. Cases: clean unbacked tail kept + boundary correct + no torn tail; header-backed / payload-unbacked frame rejected via the Unsafe CRC fold (the case that would SIGBUS through native Crc32c); fully-holed file → header read surfaces a skippable MmapSegmentException.
• Map-past-EOF cases (truncate down + a length-injecting FilesFacade that reports the original larger length, so the mapping extends past real EOF): a read past real EOF faults on every filesystem, giving a portable fail-on-revert guard that also protects the ext4/xfs CI runners. Cases: a scan fault past EOF is handled (interpreter/C1 → graceful partial recovery; C2 → the inlined async fault escapes to openExisting's outer catch and becomes a per-file skip, asserted to carry the "unsafe memory access operation" message so a revert still fails); a header fault past EOF is converted to a skippable MmapSegmentException.

Correcting the earlier revert-claim in this description: the sparse-hole tests error-before / pass-after only on ZFS (green either way on ext4/xfs); the portable regression protection comes from the map-past-EOF tests.

The fault-delivery mechanism the fix rests on was verified directly on JDK 8 (Temurin 1.8.0_492) — the shipping/CI floor — in both interpreter (-Xint) and JIT modes, plus on JDK 17/21/25: HotSpot emits the pre-21 message, the shared fragment matches, and a direct try/catch catches the fault in interpreter, C1, and C2. MmapSegmentTest (13), SegmentRingRecoveryUnlinkTest (2) and BackgroundDrainerPoolInterruptedCloseTest (the ZFS flake) pass locally. TestUtils.createTmpDir/removeTmpDir were extracted and are now shared (removing ~24 duplicated lines from MmapSegmentTest).

🤖 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}

[bluestreak01]

Review of PR #64 — fix(qwp): recover an SF slot whose segment tail is an unbacked mapped page

Reviewed at level 3 (full mission-critical pass: all reviewer lenses, per-finding source verification, empirical revert experiment on this host).

Scope: 7 files, +771/−47. No binaries — committed-binary gate passes (git diff --numstat shows no -/- binary rows). Production: MmapSegment.java (+182/−20), SegmentRing.java (+10/−3, message/comment only), FilesFacade.java (+16, one new method), DefaultFilesFacade.java (+5). Tests: new MmapSegmentRecoveryFaultTest.java (+508), MmapSegmentTest.java (dedup), TestUtils.java (+43, shared helpers).

What I verified independently (not taken on faith)

• CRC bit-identity, mechanically. Extracted the native crc32c_table[256] from core/src/main/c/share/crc32c.c and re-ran buildCrc32cTable()'s recurrence in Python: tables identical, all 256 entries (entry[1]=0xf26b8303 both). Seed/complement (~INIT…~crc) and the byte-at-a-time loop match the native tail loop. The tests also cross-validate at runtime (frames written with native Crc32c are accepted by the Unsafe crc32cRecovery), so a single-bit divergence would reject them.
• Portable fail-on-revert, empirically. I reverted the guard (restored native Crc32c in scanFrames, removed the three InternalError catches) keeping only the FilesFacade seam, on this xfs/tmpfs + JDK 25 host. The two MapPastEof tests failed with java.lang.InternalError: a fault occurred in an unsafe memory access operation; the three sparse-hole tests stayed green (tmpfs zero-fills within-EOF holes). This proves the map-past-EOF tests are genuine portable regression guards, then restored the file (tree clean).
• All tests pass unreverted: 110 tests across the full sf.cursor + Crc32cTest + RecoveryReplayTest suite, including MmapSegmentRecoveryFaultTest (5), MmapSegmentTest (13), SegmentRingRecoveryUnlinkTest (2). Full test-compile clean.

Critical

None.

Moderate

None. The two open items from the prior maintainer review are both resolved in the current branch, and I confirmed each:

• Prior M1 (no fail-on-revert off ZFS): resolved by the new FilesFacade.length(String) map-past-EOF seam + the two MapPastEof* tests — proven above to fail-on-revert on non-ZFS.
• Prior M2 (fix unverified on JDK 8): CI's build-jdk8 job runs the entire test suite on JDK 8 (mvn … clean install), so MmapSegmentRecoveryFaultTest — including the portable guards — runs on the shipping floor on every PR. GitHub ubuntu-latest is ext4, where the portable tests are the live regression guard.

Minor

• m1 — Test facade boilerplate duplicated. MmapSegmentRecoveryFaultTest.MapPastEofFacade (test lines 369–508) re-implements all 26 FilesFacade methods delegating to INSTANCE, the same delegate-everything shape already in MmapSegmentTest.FaultyFilesFacade. A shared DelegatingFilesFacade test adapter (override-only-what-you-inject) would DRY both. Optional — the new facade matches the pre-existing in-repo pattern and no shared adapter exists yet.
• m2 — Informational, not blocking (documented tradeoff, unreachable in production). Under C2 the inlined async unsafe-access fault is delivered to openExisting's outer catch (Throwable) → MmapSegmentException → SegmentRing skips the whole .sfa, dropping the recoverable frames below the fault, whereas interpreter/C1 partially recovers them. Production recovery is one-shot at startup on cold code (never near the ~10k-invocation C2 threshold), and the dropped frames are the highest-FSN un-acked tail, so the store-and-forward durability guarantee holds in practice. Worth keeping in mind only if recovery were ever made hot/repeated. The author already documents and tests both outcomes.
• m3 — No Fixes #NN. The body references #65 as a related (distinct-root-cause) bug but cites no filed issue for the ZFS flake. Optional if none exists.

Downgraded (false positives — verified against source)

• "detectTornTail returning 0 on fault could make SegmentRing unlink a segment holding real data." — False. Page-granularity: if the header (page 0, offset 0) read succeeded, frame[0]'s header (offset 24–31, also page 0) is backed, so the [lastGood, lastGood+8) probe at lastGood==HEADER_SIZE cannot fault — it reads real bytes (→ tornTailBytes>0 → quarantine to .corrupt, not unlink) or zeros (→ genuinely empty hot-spare → correct unlink). The fault-catch only fires at lastGood>HEADER_SIZE, where frameCount>0, so the unlink branch is never taken. Unreachable.
• "The InternalError message-match could swallow a wild-pointer logic bug as end-of-data." — False. Every Unsafe read in scanFrames/crc32cRecovery/detectTornTail/header stays within [addr, addr+fileSize) (loop guard + the pos+8+payloadLen>fileSize check), so a fault is always a genuinely unbacked page inside the mapping, never a stray pointer.
• "Adding length(String) to FilesFacade breaks other implementors." — False. Exactly three implementors (DefaultFilesFacade, test FaultyFilesFacade, test MapPastEofFacade), all updated; full test-compile clean.
• "crc32cRecovery may diverge from native Crc32c (sign extension / len=0)." — False. Table bit-identical; low-byte XOR is sign-agnostic under &0xFF; only ever called with len≥4; runtime cross-validation passes on payloads containing high-bit bytes.
• "SegmentRing FSN-gap message change breaks SegmentRingTest." — False. New message still begins "FSN gap in recovered segments:"; SegmentRingTest:353 asserts .contains("FSN gap") — still matches, test green.

Coverage map

#	Behavioral change	Test (local)	Failure link	Dimensions	Verdict
1	openExisting header-read mmap fault → MmapSegmentException (was: raw InternalError aborts slot)	testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem (portable) + testUnbackedHeaderPageIsSkippableNotFatal	On revert, raw InternalError escapes openExisting → test errors (proven empirically)	error ✓, cleanup ✓ (assertMemoryLeak), boundary ✓; concurrency N-A (single-threaded recovery)	TESTED
2	scanFrames in-mapping fault → data boundary, keep frames below (was: abort)	testScanFaultOnMapPastEofIsHandledAnyFilesystem (portable) + testRecoveryKeepsFramesBeforeUnbackedTail / …PayloadReachingUnbackedPage (ZFS)	On revert, InternalError escapes/JVM abort (proven)	happy ✓, error ✓, boundary ✓, cleanup ✓	TESTED
3	Recovery CRC via crc32cRecovery (Unsafe) instead of native Crc32c — bit-identical	Every writeSegment-based case (native-write / Unsafe-verify) + 13 MmapSegmentTest recoveries	A 1-bit table/sign error rejects a valid frame → frameCount assertion fails	happy ✓, NULL N-A, boundary (0-len, page-edge) ✓	TESTED
4	detectTornTail in-mapping fault → return 0 (clean) (was: abort)	testRecoveryKeepsFramesBeforeUnbackedTail (asserts tornTailBytes==0) + testScanFaultOnMapPastEofIsHandled…	On revert, fault escapes probe loop → test errors	error ✓, boundary ✓	TESTED
5	FilesFacade.length(String) / DefaultFilesFacade.length(String) — new delegating method	Exercised by every openExisting(String) recovery test	Wrong length → mapping wrong size → recovery assertion fails	happy ✓	TESTED
6	SegmentRing.openExisting FSN-gap message text appended; comment reworded	SegmentRingTest (.contains("FSN gap")) still green	—	—	EXEMPT (verified: message-text only, no control-flow delta)
7	TestUtils.createTmpDir/removeTmpDir extracted; MmapSegmentTest dedup	All tests using them (110 green)	—	—	EXEMPT (test-infra, verbatim extraction)

Summary

• Verdict: Approve. The production change is correct, minimal, and well-guarded; the two prior-review Moderates are resolved and I re-verified both.
• Test & tandem gate: PASS. This is a fix PR with regression tests whose failure links I proved empirically (portable fail-on-revert). It is unit-testable-only — the fault is a pure filesystem/JVM interaction reproduced in-process; the originally-flaking BackgroundDrainerPoolInterruptedCloseTest is itself a unit test in this repo. No OSS/Enterprise/e2e-python tandem required (no drainer send-loop, reconnect budget, pool-startup, replication, or kill -9 dependence is touched; gh pr list --head fix/recovery-mmap-unbacked-page finds none, as expected). The store-and-forward drainer invariants checklist is N-A here.
• Zero-GC gate: PASS. The producer hot path (tryAppend) is untouched and still uses native Crc32c. New allocations (CRC32C_TABLE, the recovery CRC loop, isMmapAccessFault's String.contains) are class-init / cold-recovery only — no per-row / per-producer-call allocation.
• Coverage map: 7 behavioral changes — 5 TESTED (2 with portable fail-on-revert proven on this host), 2 EXEMPT (verified no-behavioral-change). 0 UNTESTED.
• Required-tandem status: none required (unit-testable-only).
• Findings: 5 draft findings dropped as false positives after source verification, 3 Minor retained. In-diff vs out-of-diff: all retained findings in-diff; the sole out-of-diff callsite (SegmentRing.openExisting's per-file skip / unlink / .corrupt quarantine) was walked and verified SAFE under the new fault-derived MmapSegmentException and tornTailBytes==0 behavior.

[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%