chore(qwp): de-flake the SF recovery header-fault test#68
Closed
glasstiger wants to merge 3 commits into
Closed
Conversation
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>
glasstiger
changed the title
The routed header-fault test asserted only that recovery returned no ring. But SegmentRing.openExisting rebuilds the segment path as sfDir + "/" + name and swallows the per-file MmapSegmentException, so the test could no longer see why the skip happened. If the facade's targetPath ever stopped matching that rebuilt path, openExisting would read the real length 0, throw "file shorter than header" before the mmap-fault guard, still skip, and still return null -- the test would pass even with the guard reverted, silently gutting the only portable fail-on-revert guard. MapPastEofFacade now records whether it served the inflated length for its target path, and the test asserts it did. That proves the beyond-EOF fault, not a short-file throw, drove the skip, restoring the structural guarantee the pre-routing reference-identity form had. A mutation that breaks the path match now fails loudly at the assertion instead of passing green (verified: it fails at the new assertTrue with the coupling broken, and passes on JDK 11 and JDK 17 intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
[PR Coverage check]😍 pass : 2 / 2 (100.00%) file detail
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tandem: questdb/questdb#7404 bumps this branch into OSS core — merge together.
What
The SF store-and-forward recovery header-fault regression test
(
MmapSegmentRecoveryFaultTest.testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem)flaked the JDK 8 CI with:
The test called
MmapSegment.openExisting(ff, path)directly from the testlambda. Under C2, HotSpot inlines
openExistinginto the handler-less testframe and delivers the async unsafe-access
InternalErrorpastopenExisting's owncatch (Throwable), so the fault-to-MmapSegmentExceptionconversion never runs and a raw
InternalErrorescapes. This is a property ofhow HotSpot delivers the async "recent unsafe memory access" fault for
JIT-compiled code, not a defect in the recovery code itself: production
recovery runs once at startup (interpreted/C1), where the fault is delivered
precisely and caught.
Change
SegmentRing.openExisting(...),production's real recovery caller. The faulting header read then sits behind a
genuine, non-inlined call frame whose
catch (Throwable)converts the faultto a
MmapSegmentException, whichSegmentRing's per-file catch skips — sorecovery of the sole unreadable segment returns no ring, exactly as production
behaves.
SegmentRing.openExisting(FilesFacade, String, long)overload so the length-injecting test facade reaches
MmapSegment.openExisting. It mirrorsMmapSegment.openExisting's existingfacade seam; the
FilesFacade.INSTANCEoverload production uses is unchanged.Fail-on-revert is preserved: revert the header-block conversion and the raw
InternalErrorescapesSegmentRing, so the test errors instead of returningnull.Verification
The client's native lib was built locally so the tests run. Under full-suite
JIT warmup the routed header tests are stable:
JDK 8 was not available locally. JDK 11 shares the same pre-21 async-fault
delivery family as JDK 8, so it is the best available proxy; the JDK 8 CI is the
final confirmation.
Known limitation, left unchanged
testScanFaultOnMapPastEofIsHandledAnyFilesystemis a separate, pre-existingflake of the same class and is not touched by this PR. It is green on the JDK 8
CI (both
mainand the failing branch reported only the header test failing)and passes in isolation, but fails in-suite on JDK 11/17 because the sibling
methods warm the JIT and the scan-path async fault then escapes. I verified that
neither routing through
SegmentRingnor-XX:CompileCommand=dontinlinefixesit — the fault escapes every Java-level catch under C2 regardless. Stabilizing
it on JDK 11/17 would require either relaxing it to accept the raw
InternalError(which weakens its fail-on-revert guard) or isolating it in adedicated fork (fragile under Surefire fork reuse). Because it is green on the
JDK 8 CI and production is unaffected, it is left as-is here; it can be a
follow-up if JDK 11/17 local-dev stability is wanted.
Test plan
mvn -pl core test -Dtest=MmapSegmentRecoveryFaultTest— the twoheader-skippable tests exercise the real
SegmentRingrecovery path🤖 Generated with Claude Code