fix(qwp): keep SF slot locked until manager worker quiesces#67
Open
bluestreak01 wants to merge 37 commits into
Open
fix(qwp): keep SF slot locked until manager worker quiesces#67bluestreak01 wants to merge 37 commits into
bluestreak01 wants to merge 37 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.
QuestDBImpl.close() set the volatile `closed` flag before running the pool teardown chain, so a second concurrent close() caller could observe closed==true and return while the first caller was still draining and releasing pool resources -- a premature return that breaks the AutoCloseable expectation that shutdown has completed once close() returns. Make close() synchronized so the losing caller blocks on the monitor until the winner finishes teardown, then enters, sees `closed`, and no-ops. A bare CAS is insufficient: the losing caller would still return early. Adds a latch-controlled two-closer regression test (QuestDBImplCloseTest) that is red without the fix.
CursorWebSocketSendLoop.close() did an untimed shutdownLatch.await() while the I/O thread could be blocked in an in-flight foreground connect (connect_timeout=0 => OS SYN-retry ~60-130s). The connecting WebSocketClient is a walk-local in QwpWebSocketSender.connectWalk, invisible to close() (the `client` field is null on async-initial connect / stale on reconnect), so closeTraffic() could not reach it and close() hung -- risking Sender.close() exceeding the sidecar's 120s deadline. Publish a race-safe per-loop cancellation handle (ConnectCancellation) through the transport seam: connectWalk publishes the client it is about to block on before connect(); close() calls closeTraffic() on it to unwind a black-holed connect. The ReconnectFactory seam gains a Java-8 `default reconnect(ConnectCancellation)`, so every existing implementor stays source/binary compatible. Add a bounded backstop: close() awaits the shutdown latch for at most DEFAULT_CLOSE_SHUTDOWN_AWAIT_MILLIS (30s, ~4x under the sidecar deadline) and, on timeout, runs the same loud failed-stop protocol as the interrupt branch (delegates final teardown to the I/O thread's exit path, frees nothing under the live worker) -- so close() returns bounded even in the rare TOCTOU window where cancellation is a no-op. Also clear the in-flight handle on connect-failure paths so it never dangles at a disposed client. Adds CursorWebSocketSendLoopConnectPhaseCloseTest (async-initial + mid-flight cancellation, plus bounded-backstop-without-cancellation).
…anifest SF crash recovery previously failed OPEN on operational errors: - findFirst() < 0 (permission/transient enumeration error) was treated as an empty directory, so the engine started fresh and openCleanRW(O_TRUNC) truncated the existing durable log (sf-initial.sfa) and deleted the ack watermark; - findNext() < 0 silently accepted an incomplete directory listing; - per-file openRW/mmap failures (EMFILE, ENOMEM, transient I/O) on valid segments were swallowed by the same catch as data corruption and skipped; - the FSN contiguity check only compares adjacent pairs, so a skipped LEADING segment silently dropped unacked rows (ackedFsn seeded past them) and a skipped TRAILING segment silently re-issued its FSNs to new payloads (overlapping FSNs on disk). Recovery now fails closed and proves slot integrity before mutating: - Enumeration errors (findFirst/findNext < 0) abort startup. - Operational open/mmap failures on recognized segments abort startup; only positively-identified corruption (new MmapSegmentCorruptionException: bad magic, sub-header size, negative baseSeq, unreadable header page) is skippable, and quarantine renames (<name>.corrupt) are deferred until the surviving chain validates, so a failed recovery mutates nothing. Unsupported-version segments stay fatal (they belong to another client build; renaming them would strand that build's frames). - sf-manifest.bin: dual-slot, CRC-protected, generation-versioned boundary record (headBase/activeBase) written on rotation and ack-driven trim. Recovery validates the chain against the committed boundaries, which catches missing leading/trailing segments that contiguity alone cannot. Segments carry a manifest-required header flag so a deleted manifest next to migrated segments fails closed. Boundary updates are monotonic (clamped) and serialized with trim under the ring monitor; the trim path recomputes the successor under that monitor so a concurrent rotation can never make the head leapfrog a still-unacked sealed segment. - Segment files are created with O_EXCL (new Files.openRWExclusive natives) instead of O_TRUNC, so no code path can truncate an existing log. - Crash-window protocol, verified state by state: fresh start orders initial-segment durability -> manifest create -> flag stamp; rotation syncs the promoted spare's rebased header before the manifest references it and commits the manifest before any ring mutation; clean-drain close durably collapses boundaries to head==active before unlinking and removes the manifest last. Every window either recovers (equivalent empties from a fresh-start kill, an empty active at the chain end after a rotation crash, record-less manifest creation debris, drain-window survivors, stale below-head files) or fails closed with intact evidence. The one deliberate mutation on a failing path is quarantining a provably record-less manifest (it contains no boundaries by construction; flagged segments still fail closed afterwards). Regression tests (SegmentRecoveryIntegrityTest, 24 cases) cover the full required matrix: findFirst=-1, partial findNext=-1, openRW=-1, mmap failure -- each asserting byte-for-byte slot preservation; the engine-level O_TRUNC reproduction; leading/trailing/interior boundary loss; corrupt stray quarantine with sibling recovery; deferred-quarantine no-mutation on failure; flagged-segments-without-manifest; manifest creation-crash debris self-heal (zero-byte and record-less); drain-crash spare survivor; corrupt-active-with-empty-stand-in fail-closed; dual-slot generation selection and torn-record fallback. Independently reviewed for crash-consistency holes, lock ordering (manager lock -> ring monitor -> manifest monitor, acyclic), unacked-frame loss, and fd/mmap leaks; both review blockers (creation-debris brick, drain-window spare brick) are fixed and regression-tested. Windows native openRWExclusive0 uses CREATE_NEW; CI rebuilds platform binaries from the .c change.
sortByBaseSeq was a median-of-three Lomuto quicksort. Median-of-three covers the readdir orders a healthy slot produces (lexicographic enumeration yields already-sorted baseSeqs, hashed directory order is effectively random), but Lomuto partitioning is O(N^2) on organ-pipe, duplicate-heavy and median-of-three-killer orders. Exact simulation at the documented 16K-segment ceiling: 22.6M comparisons for organ-pipe, 50.7M for Musser's med3-killer permutation, 134M (full N^2/2) for mass-duplicate baseSeqs -- versus ~221K on the healthy paths. Such orders are only reachable through corrupted-yet-parseable or operator-copied headers, and recovery validation rejects those slots -- but the quadratic stall lands BEFORE validation gets to reject them. The sort is now an introsort: the median-of-three fast path is unchanged, and each root-to-leaf partition path carries a budget of 2*floor(log2(N)) passes. Loop-on-larger iterations count against the budget, so bad-split chains cannot hide in the loop. Ranges that exhaust it fall back to in-place heapsort (still Long.compareUnsigned, zero allocation), capping every adversarial pattern at ~3.8*N*log2(N) comparisons (organ-pipe 773K, duplicates 507K, med3-killer 861K at N=16384). Recursion depth stays under log2(N). The sortComparisons counter ticks +2 per sift-down level so it remains a strict upper bound in the fallback. New adversarial test drives the sort directly with in-memory segments at N=16384 across organ-pipe, all-duplicate, few-distinct, med3-killer and sign-bit-key patterns, asserting unsigned order plus comparisons < 8*N*log2(N) -- >2x headroom over the worst measured pattern, ~12x below the mildest quadratic blow-up. The existing sorted-input regression test is unchanged and still passes.
Protect the durable ACK watermark with redundant CRC records so torn writes cannot skip unacknowledged frames after restart. Bound sender and query pool shutdown waits while retaining late cleanup ownership, make segment recovery cleanup linear and ownership-safe, and count only genuine reconnect sends as replay telemetry. Add deterministic Java 8 regression coverage for crash recovery, pool shutdown, segment-count complexity, cleanup failures, and reconnect metrics.
EngineClosePublishAfterFlockReleaseTest started the shared retry driver after injecting an invalid slot-lock descriptor. It then returned before the driver's initial park expired. A following test could observe the global driver as active and fail, as Windows CI did. Inject a synchronous retry-driver start failure for this explicit-close retry test. Restore the factory in a nested finally block. Dedicated retry-driver tests continue to cover the asynchronous lifecycle.
Direct SenderPool instances now continue store-and-forward recovery after a transient failure or startup budget exhaustion. The recovery driver uses elapsed-time budget accounting, throttles retries, and shuts down before delegate teardown. Guard driver startup so constructor failures quiesce the driver and close prewarmed delegates. Add deterministic recovery, lifecycle, cleanup, and mutation-sensitive tests.
… test closedMidCreationTeardownRunsOutsideThePoolLock started the pool-closer thread and the idempotent-re-close lock probe with nothing ordering their entry into close(). On a slow scheduler (Windows CI, build 251972) the probe won the closeStarted race, became the primary closer and legitimately parked in the new bounded 5s in-flight-creation wait -- the parked teardown deliberately holds inFlightCreations at 1 -- so probe.join(5s) expired and the lock-free assertion failed even though the pool lock was free the whole time. Wait on the hasCreationWaiterForTesting() seam until the pool-closer has claimed closeStarted and parked in the creation wait (releasing the lock) before starting the probe, making the probe a true re-close. Also swap the reflective markClosing() invoke for the markClosingForTesting() seam. Reproduced by delaying the closer thread 400ms: the old test fails with the exact CI assertion in 5.1s; the fixed test passes in 0.5s under the same delay.
…nd close test close() returns when the worker counts shutdownLatch down inside ioLoop's finally -- before the worker's exit tail and actual thread termination -- so Thread.isAlive() can be observably true for a scheduling beat after close() returns (linux-x64 failure in build 252736). Quiescence is the latch plus the cleanup-before-countdown ordering (already asserted), not thread death; every sibling test already uses join-then-assert. Reproduced by sleeping 200ms after the countdown: the old assert fails exactly like CI; the joined assert passes under the same window.
Validate persisted segment bytes through bounded positioned reads before mmap. Fail closed on read and size instability, preserve corruption quarantine semantics, and cover sparse tails, buffer boundaries, and recovery I/O faults.
Add configurable background checkpoints for store-and-forward payloads.\nUse checked mmap and file-descriptor barriers and gate segment rotation\nuntil the predecessor is durable.\n\nPropagate the interval through foreground senders and orphan drainers,\npreserve failures for producer visibility, and sync replayable data on\nrecovery and close. Persist parent directory entries in periodic mode.\n\nDocument the bounded-RPO contract and cover configuration, scheduling,\nbarrier failures, rotation, recovery, and compatibility in tests.
…no segment files Recovery treated every valid manifest with zero .sfa files as EMPTY and removed it, even when headBase < activeBase. No in-protocol crash can produce that state: the close-time drain durably collapses the boundaries to head == active before its first unlink, and a fresh start writes (0,0). Uncollapsed boundaries with no segment files therefore prove durable, never-declared-acked frames vanished outside the protocol (manual wipe, partial restore) -- yet recovery silently started fresh and deleted the manifest, destroying the only evidence of the loss. Accept the segment-less slot as EMPTY only when headBase == activeBase, matching the existing guard on the some-files clean-drain window; otherwise throw without mutating the slot. Fix the drain-window test to model the real crash state (9,9) and add a fail-closed (7,9) test asserting the directory is left byte-identical.
Member
Author
Client SF-contract clearance: the bumped revision keeps the steady-state SF drainer reconnect unbounded — Review — QWP multi-slot recovery & close-drain failover (tandem)Tandem review of Enterprise #1127 + OSS questdb/questdb#7387 (client #67 SF-contract-checked). Full mission-critical pass: 14 review dimensions across independent fresh-context reviewers (Rust N/A — no What this PR isAn e2e test PR (ENT, ~3.5k lines of harness/tests) plus a submodule bump carrying one OSS server behavioral change: Tandem chain verified consistent (despite stale descriptions — see Minor): OSS #7387 head CriticalNone. The production change was deep-traced end-to-end and confirmed correct; its fix ships with an effective regression test. No confirmed correctness, data-integrity, resource, concurrency, or performance/IO defect on a reachable production path remains open. (One "Blocker" candidate was raised and dropped after verification — see Downgraded.) ModerateTest-efficacy / coverage-clarity items (they do not touch production correctness/perf). All are cheap to fix in this PR. M1 — [ENT] M2 — [OSS] Peer-close fast-exit ( M3 — [OSS] The Minor
Downgraded (false positives / refuted after source verification)
Coverage map (OSS QWP close-drain — the only production behavioral change)
SummaryVerdict — ENT #1127: approve · OSS #7387: approve, with the Moderate coverage clarifications (M1–M3) and Minors recommended for THIS PR (all cheap; not deferred).
🤖 Generated with tandem PR review (level 3). |
On POSIX, shutdown(SHUT_RDWR) unblocks a recv() already in progress on another thread, which is how closeTraffic() wakes a worker parked in recv without releasing the fd. Winsock's shutdown(SD_BOTH) does not do this: an already-blocked recv() stays parked until data arrives, the peer resets, or the socket is closed. That left the QWP I/O worker stuck and failed SocketTrafficShutdownTest.testPlainSocketShutdownWakesWindowsRecvAndRetainsFd on the windows-msvc-2022-x64 job. Cancel outstanding I/O on the socket handle with CancelIoEx before the shutdown(). This wakes the blocked recv() while leaving the descriptor allocated, so the fd is retained (no reuse race) until close() releases it, matching the POSIX/macOS behaviour the sibling tests already rely on. Net.shutdown() is only reached via PlainSocket.closeTraffic(), so no other caller is affected.
…ger.close()
The bounded-join fallback in close() reaped the manager worker
(workerThread=null => isWorkerReaped()) as soon as it observed
workerLoopExited, which the worker publishes at the START of its exit
block -- BEFORE it runs the deferred exit cleanups (the owning engine's
finishClose(), which releases the slot flock and only then publishes
closeCompleted). When the bounded join timed out mid-finishClose, close()
reaped while the flock release was still in flight, so a caller reading
isCloseCompleted() saw a stale false and a spurious flock-release retry
could be scheduled. On the slow JDK 8 CI leg this raced
SenderPoolSfTest.testPreallocatedExitHandoff* to failure ("worker exit
must complete startup-recoverer cleanup without a sender or engine close
retry"), and the leaked shared retry driver then cascaded into the
FlockReleaseRetryDriverTest suite.
Once workerLoopExited is true the worker has left its (possibly wedged)
service loop and is running only finite exit cleanups, so give it a
second bounded join to terminate before reaping. This reuses the same
join-under-monitor pattern as the first join -- the worker uses a
lock-free CAS for exactly-once cleanup and never blocks on the manager
monitor -- and still reaps on timeout, so a pathologically slow cleanup
cannot hang close(). The second join is budgeted with the fixed
WORKER_JOIN_TIMEOUT_MILLIS, not the tunable workerJoinTimeoutMillis: the
first join bounds a possibly-wedged service pass (tests shrink it to
force the timed-out path), but the finite exit cleanups must be allowed
to finish regardless. In production both values are equal.
Validated under JDK 8: a deterministic repro (400ms finishClose delay)
reproduces the exact CI assertion without this change and passes with it;
the full previously-failing set (SenderPoolSfTest, FlockReleaseRetryDriverTest,
EngineClosePublishAfterFlockReleaseTest, CursorWebSocketSendLoopRotationRaceTest)
is green.
Contributor
[PR Coverage check]😍 pass : 2004 / 2310 (86.75%) 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.