fix(qwp): keep SF slot locked until manager worker quiesces#67
Open
bluestreak01 wants to merge 19 commits into
Open
fix(qwp): keep SF slot locked until manager worker quiesces#67bluestreak01 wants to merge 19 commits into
bluestreak01 wants to merge 19 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
…se retries Close-time segment cleanup (review finding: failed cleanup published as reusable slot): - finishClose now persists the final acked FSN through the still-mapped watermark before closing the ring and before any unlink, so any cleanup failure or crash leaves the residue covered by a current watermark - unlinkAllSegmentFiles enumerates fully first, aborts with no unlinks on a failed directory walk, removes segments in ascending generation order and stops at the first failure, so residue is always a contiguous top slice that recovery seeds as fully acked (no replay, no duplicates) - the ack watermark is removed only after every segment is confirmed gone Flock-release retry driver (review finding: unbounded retry threads): - the shared retry driver now backs off exponentially per fully-failed round (100ms base, 5s cap), resets on progress, and is unparked when a fresh engine enqueues - after an injected driver-start failure, pool probes re-arm the retry via ensureFlockReleaseRetryScheduled() from isSlotLockReleased(), so retained capacity recovers without a second explicit close() New regression tests: CursorSendEngineCloseUnlinkFailureTest (close-time unlink fault injection; successor must not see replayable frames) and three FlockReleaseRetryDriverTest cases (backoff schedule, reset-on-progress, pool-probe recovery after start failure).
Make acknowledged segment deletion crash-consistent, bound retry and cleanup paths, improve pool recovery complexity, and add deterministic lifecycle/native regression coverage.
Treat ENOTCONN and WSAENOTCONN as successful traffic shutdown because the peer may finish a graceful close before the owner cancels the I/O thread. Preserve all other failures and descriptor ownership. Add real loopback coverage for peer-first close and a synthetic failure case that verifies non-benign shutdown errors remain visible.
Contributor
[PR Coverage check]😍 pass : 805 / 919 (87.60%) 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
Fix store-and-forward shutdown so a slot is never reused while a manager or I/O worker can still touch its ring, watermark, transport, or segment files. The change also makes deferred cleanup observable by sender pools, makes close-time segment cleanup crash-safe, and keeps transient startup recovery retryable for the life of the pool.
This addresses the lifecycle race behind the intermittent unsafe-memory failure in QuestDB build 249990 and prevents stale workers from mutating files owned by a replacement sender.
Root cause
SegmentManager.deregister(ring)removed the ring from the live registry, but the worker could already have copied itsRingEntryinto a snapshot and enteredserviceRing(). Close then unmapped the ring and watermark, unlinked files, and released the slot flock without proving that the in-flight pass had ended.A replacement engine could acquire the same directory while the stale pass was still provisioning, trimming, or unlinking files. The outcomes included slot corruption, data loss after restart, and mmap/SIGBUS-style failures.
Changes
Worker-quiescence barrier
SegmentManager.serviceRing()claims each entry with an atomic registered/in-service state transition.Confirmed slot release and pool recovery
SlotLock.release()explicitly unlocks through platform JNI and reports whether release was confirmed.closeCompletedandisSlotLockReleased()become true only after confirmed unlock.SenderPoolimmediately; parked borrowers do not wait for a timeout or housekeeper tick.Crash-safe segment cleanup
Retryable recovery and flock release
Bounded transport shutdown
Attachment and observability guards
Failure behavior
Safety takes precedence over capacity: if worker quiescence or slot release cannot be confirmed, the slot remains unavailable rather than being handed to a replacement. Normally, worker-exit cleanup, immediate pool notification, and the shared retry driver restore capacity once the blocking condition clears. A genuinely wedged worker or permanently failing OS unlock can keep the slot retired until process exit.
Test coverage
Deterministic tests cover:
Enterprise PR #1127 adds real-server multi-slot crash recovery, committed-but-unacked replay with WAL/DEDUP, and close-drain failover across deterministic and seeded role-change schedules.
Compatibility
The implementation retains the Java 8 language/API floor. Platform-specific slot unlock and traffic shutdown are implemented for POSIX and Windows and exercised by the tandem CI matrix.