fix(qwp): recover an SF slot whose segment tail is an unbacked mapped page by puzpuzpuz · Pull Request #64 · questdb/java-questdb-client

puzpuzpuz · 2026-07-08T14:43:31Z

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

MmapSegment.openExisting maps a recovered .sfa to its stat length and scanFrames/detectTornTail read the mapping directly. When a prior session left a sparse segment tail -- a truncate-based pre-allocation that never materialized the tail blocks, as happens on ZFS -- a read of 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.open Existing's per-file skip did not catch it: it propagated to the outer catch, aborted recovery of the whole slot, and surfaced (via a drainer or a slot-lock probe) as a spurious "unsafe memory access" failure on ZFS CI runners. 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; the slot recovers instead of failing. isMmapAccessFault matches the JVM's stable message so a genuine VirtualMachineError (real OOM, StackOverflow) is never swallowed. MmapSegmentRecoveryFaultTest reproduces the fault deterministically on any filesystem: it maps a valid one-frame segment, then truncates the backing under the still-larger mapping so the tail page is beyond EOF (the one case POSIX mmap always faults on), and asserts the scan stops at that page and keeps the frame below it. The test errors with the exact InternalError before the fix and passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Recovery reads a mapped .sfa directly; an unbacked page (a ZFS sparse pre-allocation tail, or a torn write leaving a real payloadLen pointing into an unwritten hole) faults on read. Two gaps on the shipping JDK 8: - The mmap-fault guard matched "a fault occurred in an unsafe memory access", which is only the JDK 21+ wording. Pre-21 (incl. JDK 8) emits "...a recent unsafe memory access operation in compiled Java code", so the guard was inert there. Match the shared fragment "unsafe memory access operation". - The payload CRC used the native Crc32c over the mapping. A SIGBUS inside JNI is not translated into a catchable InternalError (that happens only at Unsafe intrinsic sites), so a payload reaching an unbacked page aborted the whole JVM before the CRC check could reject the frame. Fold the recovery-time CRC over Unsafe reads (table-based CRC-32C, bit-identical to native), so the fault is catchable or degrades to a CRC mismatch. Native Crc32c stays on the append hot path. (A pre-touch guard before the native read was tried and proven unreliable: once an earlier native CRC ran in the same scan, it does not reliably fault first.) Rewrite the tests to drive the production openExisting path rather than reflecting into scanFrames: on pre-21 the imprecise fault escapes a reflective invoke frame, so reflection-based tests spuriously failed on JDK 8/11/17 even though the direct-call production path catches it. Induce the unbacked tail portably (truncate down then back up to leave a sparse hole) and size off Files.PAGE_SIZE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ARN framing M1: openExisting reads magic/version/baseSeq before scanFrames with no guard, so an unbacked page 0 (unflushed header on a truncate-based-allocate FS after a crash, or a file truncated under the mapping) faults with an InternalError that SegmentRing's per-file catch (MmapSegmentException) does not catch -- it escapes to the outer catch (Throwable) and aborts recovery of every sibling segment. Convert an isMmapAccessFault into a MmapSegmentException in openExisting's catch so only that one .sfa is skipped. Adds a portable regression test. M2: the scanFrames in-mapping-fault WARN framed the outcome as "Expected when a prior session left a sparse segment tail", downplaying a possible mid-file media error. Reword it to state frames beyond the fault are discarded and that a bad sector is indistinguishable here, and document that tornTailBytes() reports 0 for an unbacked bail-out region by design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…the FSN-gap error Follow-up on the PR #64 review minors. m3: MmapSegmentTest and MmapSegmentRecoveryFaultTest carried verbatim copies of the native tmp-dir setUp/tearDown (~28 lines each). Hoist them into TestUtils.createTmpDir / removeTmpDir so both classes and any future SF cursor test share one implementation. m5: the FSN-gap MmapSegmentException fired on a sealed segment that under-recovered (a sparse/unbacked or media-truncated tail), but its message and comment blamed only "partial-write/manual-deletion". Name the truncated-tail cause and ask operators to check disk health, matching the diagnostics-honesty wording already added to the scanFrames WARN. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

puzpuzpuz · 2026-07-08T18:24:57Z

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}

The three MmapSegmentRecoveryFaultTest cases only fault on ZFS: on ext4/xfs a within-EOF sparse hole zero-fills, so the scan stops via the CRC-mismatch / bad-magic branch and every test stays green even with the recovery mmap-fault guard fully reverted -- zero fail-on-revert protection on the primary CI runners (PR #64 review, item M1). Proven by reverting both production files to the pre-branch base: 3/3 still passed on ext4. Add a portable seam and two tests that fault deterministically on any filesystem. A read of a mapped page beyond the file's real EOF raises SIGBUS everywhere (POSIX), which HotSpot translates to a catchable InternalError at an Unsafe intrinsic site -- the same fault an unbacked ZFS page raises. openExisting gains a FilesFacade overload (mirroring create) whose file length flows through the facade; a test facade reports a length larger than the real (truncated-down) file so the mapping extends past EOF. - FilesFacade.length(String) + DefaultFilesFacade delegate; openExisting(String) now delegates to openExisting(FilesFacade, String), which also routes openRW and close through the facade. - testScanFaultOnMapPastEofIsHandledAnyFilesystem and testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem fault on ext4 and fail on revert (verified: raw InternalError escapes when the guard is neutralized, while the three sparse-hole tests stay green -- exactly the M1 gap). The scan test accepts both handled outcomes: under the interpreter/C1 the fault is caught in scanFrames (graceful partial recovery), but once C2 inlines scanFrames into openExisting the async unsafe-access InternalError is delivered to openExisting's outer catch instead, converting the file to a skippable MmapSegmentException. Both are safe (no JVM abort, no raw error into recovery); a revert lets a raw InternalError through and fails the test either way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…nism (#64 M2) The recovery fix rests on HotSpot delivering a catchable InternalError (message containing "unsafe memory access operation") at an Unsafe access and on a direct try/catch catching it. Prior review verified this on JDK 17/21/25 and inferred JDK 8 from the adjacent pre-21 LTS releases, but JDK 8 is the shipping/CI source of truth and the original flake was on a JDK-8 ZFS runner, so it had to be checked there directly. Verified on Temurin 1.8.0_492: MmapSegmentRecoveryFaultTest passes 5/5 in both -Xint and JIT modes; HotSpot emits "a fault occurred in a recent unsafe memory access operation in compiled Java code"; a direct try/catch catches the fault in interpreter, C1, and C2 modes; isMmapAccessFault's shared fragment matches while the JDK 21+-only needle it replaced does not. The build-jdk8 CI job already runs this test on JDK 8 on every push, so the gate stays enforced. Documents the result in the test's class javadoc; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Address the two cosmetic follow-ups from the #64 review: - buildCrc32cTable now uses a /** */ javadoc, matching its sibling private helpers crc32cRecovery/detectTornTail (was a // comment). - The payload-unbacked test asserted the frame-2 header "must end on the page boundary"; it actually ends 8 bytes below it (the asserted boundary-8 value, per the javadoc above). Reword to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

mtopolnik · 2026-07-09T07:44:41Z

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

puzpuzpuz added the bug Something isn't working label Jul 8, 2026

puzpuzpuz and others added 3 commits July 8, 2026 19:27

puzpuzpuz and others added 3 commits July 8, 2026 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(qwp): recover an SF slot whose segment tail is an unbacked mapped page#64
puzpuzpuz wants to merge 7 commits into
mainfrom
fix/recovery-mmap-unbacked-page

puzpuzpuz commented Jul 8, 2026 •

edited

Loading

Uh oh!

puzpuzpuz commented Jul 8, 2026

Uh oh!

mtopolnik commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

puzpuzpuz commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tradeoffs and scope

How this relates to #65

Test plan

Uh oh!

puzpuzpuz commented Jul 8, 2026

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

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

Moderate

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

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

Minor

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

m2 — Cosmetic

Verified correct — do not re-chase

Summary

Uh oh!

mtopolnik commented Jul 9, 2026

[PR Coverage check]

file detail

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

puzpuzpuz commented Jul 8, 2026 •

edited

Loading