fix(qwp): fix store-and-forward recovery data loss and unbounded client shutdown - #73
Open
bluestreak01 wants to merge 13 commits into
Open
fix(qwp): fix store-and-forward recovery data loss and unbounded client shutdown#73bluestreak01 wants to merge 13 commits into
bluestreak01 wants to merge 13 commits into
Conversation
…ing slot resources SegmentManager.close() gives up after a bounded join, but CursorSendEngine.close() could not observe the incomplete shutdown: it closed the ring and watermark, unlinked segment files and released the slot flock while the worker could still be mid service pass - able to unlink a spare/trim path inside a slot directory that a replacement engine had already re-acquired (SF data loss after restart). - serviceRing now claims the entry as in-service under the manager lock and skips entries deregistered before the pass starts - new SegmentManager.awaitRingQuiescence(ring): bounded, interrupt-preserving barrier that confirms the worker can never touch the ring/slot again - CursorSendEngine.close() releases ring/watermark/segment files/slot lock only after confirmed quiescence (or a reaped owned worker); otherwise it leaks them deliberately, logs, and allows close() to be retried - a timed-out SegmentManager.close() hands pathScratch ownership to the worker, which frees it on exit - no permanent native leak when the worker outlives the join - regression: CursorSendEngineSlotReacquisitionTest (slot retained while worker mid-pass + retry completes cleanup; same-slot reacquisition after normal close) and an awaitRingQuiescence contract test
…er the mid-join interrupt testInterruptedCallerDoesNotAbandonReapableWorker relied on a negative wall-clock assertion (closeReturned must NOT count down within 300 ms) that never proved the closer reached the join path: a descheduled closer plus the old immediate-InterruptedException behavior could satisfy every assertion. The 'interrupt arrives during join' branch of the retry loop had no test. - SegmentManager gains a @testonly beforeJoinAttemptHook that runs on the closer thread after interrupt clearing, immediately before each join attempt - reworked test positively awaits the hook and asserts the interrupt is clear and the worker still live at the join point (fails on both revert shapes) - new testInterruptDuringJoinRetriesUntilWorkerReaped drives a mid-join interrupt and uses the second hook invocation as deterministic proof the loop retried instead of abandoning the reap Verified: both tests fail against a simulated revert (single join, no interrupt clearing, abandon on InterruptedException) and pass 3/3 runs with the fix.
Cursor engine closure now retains slot ownership until manager quiescence and transfers stalled cleanup to a dedicated daemon. Recovery validates persisted segments through bounded positional reads before mmap, preserves facade ownership, and retains corrupt data for diagnosis. Deterministic tests cover lifecycle races, compatibility, recovery faults, and slot reuse.
The server's role-change CLOSE now carries the private-use code 4001 (RFC 6455 s7.4.2) instead of NORMAL_CLOSURE, so the client's verbatim CLOSE echo proves it actually read the server's CLOSE frame. Classify ROLE_CHANGE as orderly in the send loop (strike-exempt, paced recycle, reconnect-eligible), matching the treatment the handoff got as 1000. Pin the classification with a poison-frame test: repeated demote closes at an unadvanced head FSN must never latch a PROTOCOL_VIOLATION terminal.
Recovery mmap'd the segment file's complete logical length RW and left appendCursor at lastGoodOffset, exposing the unvalidated tail as writable capacity. If that tail was sparse/unbacked (fallocate reservations do not survive external sparsification, and FilesFacade.allocate cannot retroactively fill holes below EOF), the next producer append stored straight into the hole — which under ENOSPC raises SIGBUS and aborts the JVM. MmapSegment.openExisting now maps only the validated prefix [0, lastGoodOffset), making recovered segments born full: the first append backpressures until the segment manager installs a hot spare (created with genuine block reservation and clean ENOSPC failure), then rotates onto it. Consumers are unaffected — they only read below publishedOffset(), which equals lastGoodOffset. Because a recovered segment's mapped size no longer reflects its on-disk footprint, sf_max_total_bytes accounting (register seed and trim decrement) now uses the new MmapSegment.fileBytes() — the file's logical length — instead of sizeBytes(). Regression tests: recovered sparse-tail segment must be born full and refuse tryAppend; a ring recovered around a sparse-tail active must backpressure the first append and resume with a contiguous FSN after spare rotation.
SegmentRing.openExisting() previously skipped every per-file MmapSegmentException and treated directory-enumeration failure (findFirst < 0) as an empty store. The interior FSN-contiguity check cannot expose a skipped segment at the lowest, highest, or sole position -- the survivors stay mutually contiguous -- so recovery silently retired the skipped segment's unacked frames (lowest/sole: the engine seeds ackedFsn past them), minted duplicate FSNs (highest: nextSeq resumes inside the skipped range), or -- for a sole unreadable segment -- returned null, routing CursorSendEngine onto the fresh-start path whose truncating openCleanRW (O_CREAT|O_TRUNC / CREATE_ALWAYS) destroyed the only surviving sf-initial.sfa. Recovery now fails closed: - an unreadable .sfa aborts recovery with MmapSegmentException naming the file and remediation options (restore, or move the file out to explicitly accept the loss); - enumeration failure throws instead of returning null; - quarantine-rename and empty-leftover-unlink failures throw instead of warn-and-continue; - CursorSendEngine's fresh-start path refuses to create sf-initial.sfa over an existing file (defense in depth), checked before the stale-watermark unlink to preserve forensic state. openExisting() returning null now strictly means "no recoverable data existed". BackgroundDrainer already quarantines a slot (markFailed + FAILED) when engine construction throws, so corrupt orphan slots are preserved for postmortem rather than drained over. New SegmentRingEdgeRecoveryTest pins the contract with sole / lowest / highest unreadable-segment tests, an engine-level no-truncation test, and a POSIX enumeration-failure test; all five reproduced the data loss before the fix.
CursorSendEngine.closeEventually() used to submit a forever-retrying task to an unbounded Executors.newCachedThreadPool(): each stuck engine pinned one daemon thread in a while-loop whose every close() attempt can block ~10s (5s awaitRingQuiescence + 5s owned-manager join), so E stuck engines retained E threads plus E rings, mmaps, watermarks and flocks with no global bound and no give-up. Replace it with a single shared single-thread scheduled daemon. Each task performs exactly ONE close() attempt and reschedules itself with exponential backoff (10ms doubling to a 10s cap), so E stuck engines cost E queued tasks, not E threads. After 40 attempts the engine is abandoned: terminal for the daemon, one ERROR logged, resources deliberately stay leaked until process exit (the kernel releases the flock; releasing them early would hand the slot to a replacement engine the stale worker could still corrupt), and the process-wide count is observable via getAbandonedDeferredCloseCount(). close() may still be invoked directly after abandonment. The per-engine deferredCloseScheduled flag keeps scheduling deduplicated. Add a many-stuck-engines test pinning the single-thread bound plus eventual completion, and a permanently-stuck-manager test pinning the retry budget, the retained slot lock after abandonment, and the direct close() recovery path. Also reattach the unlinkAllSegmentFiles javadoc that had drifted onto the old retry-loop method.
SegmentRing.openExisting walked every slot-directory entry to recover segments, then SegmentManager.register immediately re-enumerated the same directory (scanMaxGeneration) to seed the spare file-generation counter, re-allocating a name String plus substring per entry. The recovery scan now tracks the maximum sf-<gen:016x>.sfa generation it observes and carries it on the returned ring (maxRecoveredFileGeneration); register() uses that value and only falls back to the legacy directory rescan for rings that were not built by recovery (FILE_GENERATION_UNKNOWN: fresh start, memory mode, tests), so behavior for non-recovered rings is unchanged. Safety properties, pinned by SegmentRingRecoveryGenerationTest: - the maximum covers every scanned entry, including empty leftovers recovery unlinks and corrupt files it quarantines, so a generation is never reused even for files gone by registration time; - name->generation parsing is a single shared helper (SegmentRing.parseFileGeneration) used by both the recovery scan and the fallback rescan, so the two can never disagree; - freshly minted spares always land strictly above everything on disk -- never at a name whose truncating openCleanRW would destroy a recovered segment. The carried value cannot be stale for its intended (first) registration: recovery and register run on one thread under the engine slot lock, and no spare can be minted into the slot before register wires the ring up. Same-manager re-registration stays safe because advanceFileGeneration only ratchets upward.
drainTrimmable retired fully-acked sealed segments with per-element remove(0), shifting the remaining suffix on every iteration -- a bulk trim of S segments cost O(S^2) element moves. Collect the eligible prefix first, then remove it with one ObjList.remove(0, eligible - 1) range removal (inclusive bounds), all under the same monitor as before, so the I/O thread's snapshotSealedSegments() still cannot observe a partially shifted list. Covered by the existing multi-segment bulk-drain assertion in SegmentRingTest.
Add deterministic gray-box coverage for BackgroundDrainer and the cursor WebSocket send loop. The tests stall SegmentManager quiescence, verify callers return without releasing the slot lock, and confirm deferred cleanup releases the slot after quiescence resumes. Add narrow test-only seams for engine injection and I/O thread joining so the tests avoid a live server and wait for the complete exit path.
Merge the latest client main, which supersedes the branch's earlier store-and-forward lifecycle and recovery implementation. Retain the QWP ROLE_CHANGE close code and classify it as orderly so demotions remain reconnect-eligible without poison-frame strikes.
The lifecycle tests inferred that close had left its creation wait whenever ReentrantLock.hasWaiters() briefly returned false. Interrupt delivery moves a condition waiter to the lock queue while awaitNanos() reacquires the lock, so the observer could mistake that transition for deadline expiry. Add test-only retry hooks that fire only after each pool catches an interrupt and confirms the original deadline still permits another wait. The tests now send each next interrupt after that acknowledgement, preventing coalescing and removing the transient queue-membership check while retaining deadline-restart coverage.
bluestreak01
added a commit
to questdb/questdb
that referenced
this pull request
Jul 27, 2026
The previous "bump to the latest main" moved the submodule to 1d54a253, the tip of client main. That commit predates the client half of this feature: main carries only the deflake work re-landed via client PR #72, not 9f3cb329 "Add ROLE_CHANGE (4001) close code for the role-change handoff". The two share a subject line, which hid the loss. Without 9f3cb329 the client classifies the 4001 CLOSE this branch emits as non-orderly. CursorWebSocketSendLoop.onClose then records a poison strike per demote, takes failPaced instead of failExemptPaced, and after four strikes at an unadvanced head FSN escalates to a PROTOCOL_VIOLATION terminal. BackgroundDrainer quarantines the orphan store-and-forward slot holding exactly the rows this branch protects. Pin dde3c15d8f instead: the tandem client branch, which already merges client main (1d54a253 is an ancestor) and adds ROLE_CHANGE to both WebSocketCloseCode and the orderly-close classification. Tandem: questdb/java-questdb-client#73
Reverts 9f3cb32. The server keeps NORMAL_CLOSURE on the role-change CLOSE, so there is no 4001 on the wire for this client to classify. 4001 existed so the client's verbatim CLOSE echo would prove to the server that it had read the server's CLOSE rather than having sent a voluntary CLOSE that crossed it. The server takes identical action on both branches -- the code only selected a log line -- while every client released before 9f3cb32 treats a code outside NORMAL_CLOSURE/GOING_AWAY as a head-of-line poison strike, escalating a routine demote to a PROTOCOL_VIOLATION terminal that quarantines the store-and-forward slot. Putting the code on the wire therefore costs data on mixed-version fleets and buys a diagnostic; it needs a negotiated capability first. Carrying the classification here anyway would leave the client claiming to understand a code nothing sends, and the poison-frame test would pin a contract with no counterparty. Strip both, and record in qwp-nack-policy-v2.md that a distinguishable role-change code is gated behind capability negotiation rather than simply absent.
bluestreak01
changed the title
Contributor
[PR Coverage check]😍 pass : 16 / 16 (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.
Important
Tandem PR — merge all three together.
questdb/questdb#7366 pins this branch through its
java-questdb-clientsubmodule, and questdb/questdb-enterprise#1113 pins that branch in turn. All three use the branch namefix/qwp-deferred-close-echo-wait.Problem
The QWP role-change handoff (questdb/questdb#7366) flushes a final durable ACK, sends a WebSocket CLOSE, and drains the tail before tearing down TCP so a store-and-forward producer cannot replay rows the promoted replica already owns. Reviewing that work surfaced a set of independent store-and-forward defects on the client side — SIGBUS on recovered sparse segments, silent data loss on unreadable segments, an unbounded shutdown daemon, and a slot released before its worker quiesced. This PR carries those fixes plus the test-stability work the same review required.
Changes
Store-and-forward recovery correctness
drainTrimmable, instead of per-entry removal.Shutdown and lifecycle
SenderPool/QueryClientPooland abandon permanently stuck engines rather than hanging shutdown indefinitely.Tests
Close-code handling — added then reverted
This branch briefly classified a private-use
ROLE_CHANGE(4001) close as orderly, to match a server that emitted 4001 on the role-change CLOSE. Both halves are now reverted: questdb/questdb#7366 keepsNORMAL_CLOSUREon the wire, and this PR strips the client-side classification.The reasoning, recorded in
design/qwp-nack-policy-v2.md: 4001 only let the server distinguish a genuine CLOSE echo from a voluntary client CLOSE that crossed it, and the server takes identical action on both branches — the code selected a log line. Meanwhile every client released before that change treats any code outsideNORMAL_CLOSURE/GOING_AWAYas a head-of-line poison strike, escalating a routine demote to aPROTOCOL_VIOLATIONterminal that quarantines the store-and-forward slot holding the un-acked rows. That trades data on mixed-version fleets for a diagnostic. A distinguishable role-change code needs a negotiated capability withNORMAL_CLOSUREas the fallback; the design doc now says so explicitly rather than leaving the absence unexplained.Net effect on this PR's diff: zero. The commits remain in history because the branch is squash-merged.
Test plan
CursorWebSocketSendLoopPoisonFrameTest,CursorWebSocketSendLoopErrorClassificationTest— 25 tests, green.CursorSendEngineSlotReacquisitionTest,CursorDeferredCloseCallerTest.MmapSegmentRecoveryFaultTest,SegmentRingEdgeRecoveryTest,SegmentRingRecoveryGenerationTest,SegmentRingTest.QuestDBImplCloseLifecycleTest; repeat both acknowledged-interrupt cases 15× pinned to one CPU; verify a deliberately restarted deadline fails the regression assertion.SqlFailoverQwpDeferredCloseExactlyOnceTestfrom questdb/questdb-enterprise#1113 with fix(qwp): fix duplicate writes and store-and-forward recovery during failover questdb#7366 pinned to this branch.