fix(qwp): keep SF slot locked until manager worker quiesces#67
Open
bluestreak01 wants to merge 15 commits into
Open
fix(qwp): keep SF slot locked until manager worker quiesces#67bluestreak01 wants to merge 15 commits into
bluestreak01 wants to merge 15 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
Every production CursorSendEngine (Sender.build, BackgroundDrainer, QwpWebSocketSender.connect) owns its SegmentManager, so close() takes the manager.close() + isWorkerReaped() branch - yet all deterministic retention tests exercised the test-only shared-manager branch (awaitRingQuiescence). A regression confined to the owned path - reporting quiescence unconditionally, or isWorkerReaped() returning true while the worker is alive - would have gone green through the whole suite and silently reintroduced the SF-data-loss hazard on the only path production runs. testOwnedEngineCloseRetainsSlotWhileWorkerIsMidServicePass builds the production shape (2-arg ctor, private owned manager), waits for the initial hot-spare install so the park hook can neither be missed nor fire early, rotates onto the spare to force the worker back into an install pass, parks it there, and drives close() with a 50 ms join budget. It asserts the incomplete close stays observable (isCloseCompleted() == false), the slot flock is retained (SlotLock.acquire throws), and a retried close after worker release completes the full cleanup and frees the slot. Mutation-verified red on all three reverts: - owned branch forcing workerQuiescent = true (only this test catches it) - finally gate reverted to unconditional slotLock.close() - SegmentManager.isWorkerReaped() returning true while the worker is alive (previously zero coverage anywhere)
serviceRing's per-pass finally called lock.notifyAll() unconditionally - with the default 1 ms poll that is ~1000 wakeups/sec per registered ring on the production worker, paid for a barrier (awaitRingQuiescence) that production never takes: all three production constructions own their manager, so close() goes through manager.close()+isWorkerReaped() and never parks on the lock. - new quiescenceWaiters count, incremented/decremented around the awaitRingQuiescence wait loop under the same lock as the worker's check - no lost-wakeup window, and the timed wait remains a fallback - the per-pass finally notifies only when quiescenceWaiters > 0; in steady state it never fires - notifyAll (not notify) retained when a waiter exists: with a shared manager, distinct waiters may await different rings
… retired pool slots When an owned manager's bounded join timed out during engine close, the engine leaked ring + watermark mmaps and the slot flock until process exit, the sender latched slotLockReleased=false forever, and SenderPool retired the slot permanently (leakedSlots++) - a transiently-slow SF filesystem op at close time (> workerJoinTimeoutMillis, default 5 s) permanently ratcheted pool capacity down, even though the worker often exits moments later. - new SegmentManager.deferUntilWorkerExit(cleanup): hands an action to the worker-loop exit block, which runs strictly after the final service pass - the last point the worker can touch any slot path. Registration and the exit block's workerLoopExited flip share the manager lock, so the handoff is exactly-once: accepted while the worker is live, rejected (caller cleans up inline) once it exited - CursorSendEngine.close(): on an owned-manager join timeout, terminal cleanup (ring, watermark, drained-file unlink, flock release) now transfers to the worker's exit path instead of leaking; the quiescence gate is unchanged - the slot stays locked until the pass provably ends - exactly-once via a terminalCleanupClaimed CAS, deliberately not the engine monitor: a retried close() holds the monitor while joining the worker, and the worker cannot die until its cleanup returns - monitor-based exclusion would stall that close() for its full join budget. With the CAS the worker never blocks and the join returns as soon as the pass ends - QwpWebSocketSender.isSlotLockReleased() is now monotonic, not frozen: it re-probes the retained engine (volatile reads only, safe under the pool lock) and flips true once the deferred cleanup - worker exit path or delegated I/O-thread close - releases the flock - SenderPool re-probes retired slots (housekeeper reapIdle tick and capacity-starved borrows just before parking) and returns a recovered index to the free set: leakedSlots goes back down and a would-be borrow timeout becomes an immediate creation. A persistent non-zero leakedSlotCount() now means a genuinely wedged worker - shared-manager engines (test-only construction) keep the old leak-and-retry-close contract: their worker serves other rings and has no exit to defer to; startup-recovery retirements also stay permanent - regression: deferUntilWorkerExit contract test, owned-close handoff test (slot reacquirable after worker exit with NO close() retry), pool recovery via reapIdle and via capacity-starved borrow; the SlotLockReleasedContractTest leak-path pin updated to the new monotonic-getter contract
…ails A throw from deferUntilWorkerExit (allocation failure while building the handoff) carries no worker-liveness information, so close() must retain every worker-reachable resource instead of running terminal cleanup inline under a possibly-live worker. Add a test seam that throws from the registration path while the worker is provably mid service pass and assert the slot flock, ring and watermark are retained, close stays incomplete, and a retried close() after worker exit converges and releases the slot.
…lease finishClose() wrote closeCompleted=true before slotLock.close(), so a pool thread could observe completion (isSlotLockReleased -> reprobeRetiredSlots), free the slot index, and admit a replacement sender whose SlotLock.acquire collided with the still-open flock fd -- a spurious "sf slot already in use" naming the process's own pid. SlotLock.close() also discarded the Files.close() result, reporting an unconfirmed release as completion. Reorder the terminal cleanup: release the flock first via the new SlotLock.release() (checks the close rc, retains the fd on failure so the lock state is never misreported), and publish closeCompleted only on a confirmed release. An unconfirmed release keeps closeCompleted false, degrading into the existing retired/leaked-slot accounting with the kernel's process-exit backstop. Add a @testonly hook between cleanup and release so the otherwise microsecond-wide window is deterministically testable; the new test parks the closer inside it and asserts completion is not observable while the flock is provably still held, then latches once released.
…estores pool capacity isSlotLockReleased() is no longer a one-shot snapshot: deferred engine cleanup on a worker/I/O-thread exit path can release the SF slot flock after close() returned. The runtime reclaim paths (discardBroken/reapIdle via reclaimSlot) already keep such slots in retiredSlots and re-probe them, but the in-range startup-recovery pass only ticked leakedSlots and dropped the recoverer, making the retirement permanent even after the release -- fatal at maxSize=1, where every later borrow timed out until process restart. Hand the retained recoverer out of drainCandidateSlotForRecovery (retainedOut replaces the flockHeld boolean) and add it to retiredSlots alongside the leakedSlots tick, so the existing reprobeRetiredSlots() drivers (capacity-starved borrow, housekeeper tick) recover the capacity once the worker finally exits. Out-of-range recoverers stay excluded: they carry no leakedSlots tick and freeSlotIndex(idx) would index past the slotInUse array.
…fore the timeout check borrow() ran the terminal timeout check before reprobeRetiredSlots(), so a zero-timeout (try-once) borrow threw without its one probe, and a borrower whose awaitNanos budget expired mid-wait timed out on capacity that a deferred engine cleanup had already returned (the delegate-side flock release never signals slotReleased). Hoist the probe above the timeout check so both paths recover the capacity instead of failing. Also pre-size retiredSlots to maxSize: every entry keeps a distinct in-range slot index reserved, so add() can never grow the backing array -- a retire (leakedSlots++ then add, under lock) can no longer fail on allocation and strand a counted-but-untracked slot that reprobeRetiredSlots() could never recover. Tests: - testZeroTimeoutBorrowProbesRetiredSlotBeforeThrowing (red pre-fix) - testParkedBorrowerGetsFinalProbeAfterBudgetExpiry (red pre-fix) - testPoolRetiresAndRecoversSlotThroughRealManagerWorkerWedge: full-stack retire/recover cycle with no forged flags -- real wedged manager worker, real timed-out close handoff, real flock release, and a re-borrow on the recovered index proving the slot dir is genuinely reusable
Deterministic regression tests for the shutdown paths the coverage review flagged as untested: - SegmentManagerCloseRaceTest#testStaleSnapshotEntrySkippedAfterDeregisterBeforeServiceClaim: a snapshot entry deregistered before the worker claims it must be skipped at claim time (serviced rings recorded from the worker's own trim-sync point via inService, so the assertion is exact -- no sleeps). - SegmentManagerCloseRaceTest#testWorkerAloneFreesPathScratchAfterTimedOutClose: after a timed-out close hands pathScratch to the worker, the worker's exit block alone must free it -- no retried close() runs in production, so the sibling test's retry-then-assert shape masked a regression here. - CursorSendEngineSlotReacquisitionTest#testTerminalCleanupRunsExactlyOnceWhenRetriedCloseRacesWorkerHandoff: a retried close() racing the worker parked mid-finishClose must lose the terminalCleanupClaimed CAS -- no double cleanup, no premature completion, flock untouched until the worker's release. - CursorSendEngineSlotReacquisitionTest#testMemoryModeOwnedCloseHandsCleanupToWorkerExit: memory-mode (null sfDir/slotLock/watermark) timed-out close must take the same worker-exit handoff without NPE and free the ring's native memory. - EngineClosePublishAfterFlockReleaseTest#testUnconfirmedFlockReleaseKeepsCloseIncomplete: a failed flock release must never publish closeCompleted, and a retried close() must neither throw nor fabricate completion. - SlotLockTest#testFailedCloseRetainsFdAndReportsFalse: release()==false retains the fd for retry and keeps reporting false while the failure persists. - SlotLockReleasedContractTest#testDelegatedIoThreadEngineCloseFlipsSlotLockReleased: the delegated-I/O-close branch (delegateEngineClose()==true) must retain the engine so isSlotLockReleased() flips true once the I/O thread's exit path releases the flock; the existing forged I/O-refusal test throws from delegateEngineClose() before the retained-engine assignment and can never reach this branch. Mutation-verified: dropping the retainedEngine assignment fails the test with the pinned message.
Release the retired slot from a test-only hook after the positive borrow wait has actually exhausted its budget. This removes the scheduler-dependent sleep and guarantees the test reaches the final post-wait probe.
…code Five doc sites documented behavior the code deliberately does not have; each invited a future 'fix' that would reintroduce the hazard the quiescence work eliminated. Comment-only change. - CursorSendEngine.closeCompleted field doc: carve the failed-flock- release case out of the retry sentence. A retried close() exits at the consumed terminalCleanupClaimed CAS and never calls SlotLock.release() again — deliberate, pinned by testUnconfirmedFlockReleaseKeepsCloseIncomplete. - CursorSendEngine.finishClose javadoc: 'must hold the engine monitor' was false for completeDeferredClose (deliberately monitor-free to avoid the join livelock). State the real contract: monitor (close path) OR worker exit path; in all cases the CAS must be won. - CursorSendEngine.isCloseCompleted javadoc: admit the third, unrecoverable state — failed flock release never flips; only process exit frees the flock. - SegmentManager.isWorkerReaped javadoc: the fall-through reap nulls workerThread while the thread may still be running deferred engine cleanups; the engine-side CAS, not this predicate, is the exclusion. - SegmentManager.serviceRing0 trim comment: narrow 'stale snapshots' to mid-pass-deregistered entries — the claim gate makes the trim block unreachable for pre-pass-deregistered entries. - SenderPool reclaimSlot/retireLease: 'retired permanently' contradicted retiredSlots.add + reprobeRetiredSlots recovery three lines down. - QwpWebSocketSender post-guard comment: the incomplete-close branch is not only the owned-manager handoff; document the no-handoff cases (shared manager, failed handoff registration, failed flock release) where the re-probe never flips.
…fix/qwp-worker-quiescence # Conflicts: # core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java
Contributor
[PR Coverage check]😍 pass : 302 / 362 (83.43%) 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.
Summary
Prevent
CursorSendEngine.close()from releasing a store-and-forward slot while itsSegmentManagerworker can still touch that slot's ring, watermark, or segment files.This closes the lifecycle gap behind the intermittent unsafe-memory failure in QuestDB build 249990 and, more importantly, prevents a stale manager pass from mutating a slot after a replacement engine has acquired it.
Root cause
SegmentManager.deregister(ring)removes the ring from the live registry, but the single manager worker may already have copied itsRingEntryintoringSnapshotand enteredserviceRing().Before this change,
CursorSendEngine.close()continued immediately after deregistration:If the manager worker was still provisioning/abandoning a spare or trimming a segment, a replacement engine could acquire the same slot and race that stale pass. The stale worker could then unlink or mutate paths owned by the replacement, causing SF data loss or an mmap/SIGBUS-style
InternalError.#65 fixed the specific path where a pending interrupt collapsed
SegmentManager.close()'s join to zero. It did not establish a general per-ring quiescence barrier for shared managers or for a genuinely timed-out bounded join.Fix
SegmentManager.serviceRing()now claims its entry asinServiceunder the same lock that guards registration.finally.awaitRingQuiescence(ring)for shared managers.CursorSendEngine.close()now releases ring/watermark/files/flock only after:pathScratchownership to the worker, which frees it on exit exactly once.QwpWebSocketSenderreports a retained flock accurately;SenderPoolretires that slot rather than reusing it. RepeatedSender.close()remains a no-op, preserving the public API contract.Failure-mode tradeoff
A genuinely wedged manager worker can now reduce pool capacity by retaining one slot until process exit. This is intentional: leaking/retiring a locked slot is safer than handing it to a new engine while a stale worker can still mutate its files. Normal shutdown remains immediate; the bounded wait is reached only for an in-flight or stuck service pass.
Tests
Added deterministic latch/hook regressions for:
Sender.close()idempotence on the retained-flock path;Red/green evidence:
testCloseRetainsSlotWhileWorkerIsMidServicePassfailed deterministically with:engine.close() released the slot lock while the manager worker was still mid service pass.mvn clean install -DskipTests: passed on JDK 25.