chore(qwp): read the SF recovery scan via pread instead of the mapping

[mtopolnik]

Fixes #69 — the MmapSegmentRecoveryFaultTest InternalError flake on the JDK 8 CI — by removing the fault instead of trying to catch it better.

Why catching cannot be made reliable. On pre-21 HotSpot, a SIGBUS from an Unsafe read of an unbacked mapped page in compiled code is not thrown at the faulting instruction: the signal handler resumes execution and the InternalError ("a fault occurred in a recent unsafe memory access operation…") is raised later, at the next safepoint transition. Once C2 inlines caller → openExisting → scanFrames into one compiled unit, the delivery point can land past every lexical try — which is why #69's stack trace shows the error attributed to the test's line with no openExisting frame, and why the same-frame catch (InternalError) pattern in scanFrames escaped too (both tests failed in the first CI run). #69's suggested mitigations (non-inlinable helper, SegmentRing-level catch) narrow the window but can't close it, and none of it is verifiable off JDK 8.

The fix. Every recovery-time read — header block, frame scan, frame CRC checks, torn-tail probe — now goes through pread into a private buffer, in one pass over a sliding 1 MiB window. Unreadable regions become ordinary read results with one deterministic outcome per input on every filesystem, JDK, and JIT state:

• a sparse hole reads back as zeros → fails the frame CRC → boundary of recoverable data;
• a region past real end-of-file → short read → boundary;
• a media error → failed read → boundary (WARN, as before).

The mapping is created only after the scan, for the append path (whose writes materialize pages rather than faulting). With no fault left to catch, isMmapAccessFault, the table-driven CRC fallback, and the per-method catch (InternalError) arms are deleted, and the frame CRC goes back through the native Crc32c — safe on a process-private buffer; a frame larger than the read window is checksummed incrementally, each window-sized chunk feeding the running CRC, producing the same value tryAppend writes in one pass. The production diff nets out to −16 lines (227 insertions, 243 deletions across MmapSegment, SegmentRing, FilesFacade).

Tests. The recovery suite is renamed MmapSegmentRecoveryBoundaryTest — there is no fault delivery left to branch on, so the former two/three-outcome tests each assert a single deterministic result, and the suite gains a bad-sector case driven through the facade's read seam. It also gains a multi-window case — a 4 MiB segment whose 3 MiB frame makes the scan reload the window four times and checksum it chunk by chunk, with a second frame verified out of the window the last chunk loaded; every earlier case fit inside one window, so the window-sliding machinery was otherwise untested. A deliberate mutation that reseeds the CRC on each window reload fails exactly that test and nothing else. A further case fails the read partway through the large frame's CRC span — the one path where the incremental CRC meets an unreadable region; the other unreadable-region cases all fail on a frame-header read — and asserts recovery discards the partially checksummed frame whole and skips the torn-tail probe; a mutation that drops the unreadable flag there fails exactly that test. The FilesFacade.length(String) seam doc is updated to the pread semantics.

Review follow-ups on the branch. openExisting unmaps the mapping when a failure lands between a successful mmap and the constructor return, instead of only closing the fd; RecoveryScan's constructor parameters follow the field declaration order and the nested helper classes sort alphabetically.

Validation. Full core suite green locally (2541 tests, 0 failures) on JDK 25 — and unlike the fault-delivery behavior this replaces, the pread semantics carry no JDK sensitivity, so local validation is meaningful; the JDK 8 CI job confirms on the shipping floor.

Out of scope, noted for completeness: post-recovery mapped reads (e.g. findLastFrameFsnWithoutPayloadFlag) touch only CRC-verified regions whose pages the scan just read, and the append path's SIGBUS-on-ENOSPC story is unchanged from #64.

🤖 Generated with Claude Code

[mtopolnik]

Code Review — PR #71

Level 2 · 4 review agents (correctness, perf+resource, concurrency+quality+metadata, tests) + my own adversarial pass. No binary artifacts. Issue #69 confirmed OPEN (real JDK 8 CI flake). 9 draft findings verified, 2 dropped as non-defects.

This is a well-engineered change. The core rewrite — scanning the .sfa file through a sliding pread window into a process-private buffer instead of reading through the mmap — is correct on every path I and the agents traced, and it eliminates the uncatchable-SIGBUS hazard rather than papering over it.

Critical

None.

Moderate

1. Untested code path: unreadable region partway through a large frame's CRC span (break scan). — in-diff, MmapSegment.java:605-607
The inner require(crcPos, m) failure inside the incremental-CRC loop is the one spot where this PR's two new mechanisms intersect — the multi-window CRC loop and unreadable-region handling — and it has zero coverage. Every unreadable-region test (testReadErrorDuringScanEndsRecoveryAtBoundary, testScanPastRealEofStopsAtBoundaryAnyFilesystem) makes the read fail on a frame header (require(pos, FRAME_HEADER_SIZE) at line 584 → the outer break); testRecoverySurvivesPayloadReachingUnbackedPage crosses a hole but on ext4/xfs the hole zero-fills so require returns true and it exits via the CRC-mismatch break. Nothing drives the labeled break scan. This correlates with the PR's own coverage report (80/85 new lines; this is one of the 5 uncovered). The path is simple and I verified it correct by inspection (a plain break here would be a bug — it would fall through to if (crc != crcRead) with a partial CRC; the code correctly uses the labeled break), but "verified by inspection" is exactly what a regression test should replace in recovery code.
Fix: add one test — a frame with payload > 1 MiB (so the CRC loop iterates) whose header reads fine but whose payload crosses a RecoverySeamFacade bad-sector (failReadsFromOffset) or short-read boundary. That is the only case combining the multi-window loop with a failed read mid-span.

Minor

4. Multi-window test javadoc imprecision. — MmapSegmentRecoveryBoundaryTest.java:268-279. The javadoc says the small second frame "is then verified out of a repositioned window," but the 64-byte frame at offset 3 MiB + 32 already sits inside the last CRC-chunk window [3 MiB+28, 4 MiB), so no reposition occurs. Cosmetic; the test itself is correct and non-vacuous.

(Optional, not filed: the PR title describes the internal mechanism rather than user impact — borderline and acceptable given QWP is beta/internal.)

Downgraded (dropped after verification)

• "New per-segment Unsafe.malloc throws CairoException on OOM, propagating past SegmentRing's per-file catch (MmapSegmentException) and aborting all-sibling recovery." — Dismissed. This matches the documented design: SegmentRing.openExisting's per-file catch explicitly intends OOM/resource-exhaustion to propagate to the outer catch and abort ("continuing the loop after an OOM would just fail again"). And under genuine exhaustion the up-to-16K retained mappings dwarf the transient, immediately-freed 1 MiB scan buffer, so the outcome is unchanged in practice.
• "The three punchSparseTail sparse-hole tests don't fail-on-revert." — Not a defect. On ext4/xfs the within-EOF hole zero-fills under both the old mmap path and the new pread path, so they stay green either way — exactly as the old test's own javadoc admitted. Portable fail-on-revert coverage comes from the two facade tests plus the multi-window test. Worth a sentence in the PR body, but nothing to fix.

Summary

Verdict: approve — merge after adding labels; the coverage gap (finding #1) is worth closing but need not block.

• 9 findings verified, 2 dropped as non-defects. Split: all in-diff; 0 out-of-diff (justified — the only production caller is SegmentRing.openExisting, which I read in full; its frameCount()==0 && tornTailBytes()>0 → quarantine contract and per-file MmapSegmentException skip are both preserved unchanged, and RecoveryScan/PreadWindow are private and thread-confined).
• No regressions. Correctness (incremental CRC is bit-identical to tryAppend by the streaming-CRC associativity the Crc32c contract guarantees), resource handling (malloc-before-try/free-in-finally; fd closed exactly once; mapping created only post-scan, unmapped only when mapped — commit 58bb7c80 is sound), and concurrency (single-threaded startup, no shared state) are all clean. Performance is linear cold-path I/O with ~2× worst-case read amplification and a single 1 MiB peak buffer; the tryAppend hot path is untouched.
• Tradeoff worth stating: recovery now always issues at least one 1 MiB pread up front (buffer capped at 1 MiB regardless of fill), versus the old zero-syscall mapped scan. Immaterial on the one-time cold recovery path, and the correct trade for deterministic, JDK/filesystem-independent behavior.

[mtopolnik]

[PR Coverage check]

😍 pass : 79 / 82 (96.34%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java	79	82	96.34%

[mtopolnik]

Superseded by #67 — closing.

This PR's change (reading the SF recovery scan through pread into a private
buffer instead of scanning straight out of the mmap, to keep recovery free of
SIGBUS on sparse/unbacked pages) was independently implemented and merged to
master in #67 ("keep SF slot locked until manager worker quiesces").

The two are parallel solutions to the same problem:

• this branch: scanFile + PreadWindow (1 MiB window)
• fix(qwp): keep SF slot locked until manager worker quiesces #67: scanForRecovery + RecoveryReader (64 KiB buffer), plus a
  MmapSegmentCorruptionException corruption-vs-operational split,
  before/after/after-mmap length re-checks against truncate/extend races, and
  mmap/msync/close threaded through FilesFacade.

All six files this branch touches fall entirely inside #67's rewrite, so
rebasing onto master nets to zero — nothing unique remains to land.

One behavioral difference, for the record: on an unreadable region mid-scan (a
media error, a short read, or a size change — not a sparse page, which both
treat as a clean CRC boundary), this branch keeps the frames below it and
continues (recover-what-you-can), while #67 treats it as an operational failure
and aborts recovery (fail-closed). #67's fail-closed behavior is the
durability-first choice and is what now ships. If recover-what-you-can is ever
wanted, it should be a separate PR proposing that behavior change on top of #67,
not this pread-plumbing branch.