fix(qwp): fix a memory-access fault and worker leak during shutdown

[kafka1991]

Summary

When the QWP client shuts down while a background drainer is still delivering a slot's buffered data, the drainer thread is interrupted as part of teardown. SegmentManager.close() reaps its worker thread with a bounded t.join(...), but a pending interrupt on the calling thread makes join() throw InterruptedException immediately. close() then returned without the worker having stopped, leaking a live worker thread together with its native buffers.

The leaked worker keeps creating hot spares and trimming/unlinking on-disk segment files. A later CursorSendEngine that memory-maps one of those files can race the concurrent truncation/unlink and take a SIGBUS, surfacing as:

java.lang.InternalError: a fault occurred in an unsafe memory access operation

The fault is intermittent -- it only triggers when the leaked worker happens to be mutating the exact file being mapped -- and showed up as a flaky failure in BackgroundDrainerInterruptIsStopSignalTest.

Fix

SegmentManager.close() now saves and clears the caller's pending interrupt before joining the worker, waits the worker out to the same 5s deadline, then restores the interrupt so the rest of the interrupted-teardown protocol still observes it. The worker is a passive daemon that exits within microseconds of running=false, so in the common case close() still returns immediately; the deadline is only consumed when the worker is genuinely wedged -- the same bound the original code intended before the interrupt collapsed it to zero.

The change is deliberately narrow: only the worker join is hardened against a pending interrupt. The loop.close() latch await on the same teardown path stays interrupt-sensitive by design, since it must throw under a wedged I/O thread.

Tradeoffs

• Under a pending caller interrupt, close() can now block up to the worker-join deadline (5s) where it previously returned at once. In practice the worker exits in microseconds; the wait only materializes when the worker is stuck, which is exactly the case where leaking it is unsafe.

Tests

• Add testInterruptedCallerDoesNotAbandonReapableWorker, a deterministic regression: it wedges the worker, calls close() on an interrupted thread, and asserts close() waits for and reaps the worker. It fails against the pre-fix code and passes with the fix.
• Adjust testCloseDoesNotFreePathScratchWhenWorkerStillAlive to reach the join-timeout branch via a new @TestOnly worker-join-timeout seam instead of the interrupt shortcut the fix removes.
• Full SegmentManager* and BackgroundDrainer* suites pass, including BackgroundDrainerInterruptedTeardownTest, which pins the deliberately-preserved interrupt status.

[kafka1991]

Code review — level 3 (blocking, mission-critical pass)

Scope: 2 files, +109/−7. SegmentManager.java (production close() hardening) + SegmentManagerCloseRaceTest.java (one new regression test, one modified test). No binary/build artifacts — committed-binary gate passes.

The fix is correct. It does exactly what the commit claims: close() saves-and-clears a pending caller interrupt so its bounded join can actually reap the file-owning worker daemon (instead of join() throwing immediately and leaking a live worker that keeps truncating/unlinking segment files → SIGBUS in a later mmap), then restores the interrupt for the rest of the interrupted-teardown protocol.

Verified against source and by executing the suite both ways: fixed code 3/3 green; with the production hunk reverted, the new regression test goes red at SegmentManagerCloseRaceTest.java:255 (~42 ms) exactly as predicted.

Critical

None.

Moderate

None.

Minor

1. SegmentManagerCloseRaceTest.java — ~30 lines of duplicated scaffolding between the two blocked-worker tests (test-code quality). The 11-line setBeforeInstallSyncHook lambda (lines 155–166 vs 226–237) and the finally cleanup block (195–203 vs 274–282) are byte-identical, and the setup preamble differs only in the slot name. The two tests genuinely diverge only after the worker blocks (one drives close() on the caller thread via the timeout seam; the other on a separate interrupted-closer thread). Suggested fix: extract a private blockWorkerAtInstallHook(...) setup helper and a shared teardown helper; keep the divergent bodies per-test. Not blocking.

2. The modified test testCloseDoesNotFreePathScratchWhenWorkerStillAlive is not a regression guard for this fix (test efficacy). Verified by reverting the production hunk: this test still passes, because the old interrupted join(50) also throws immediately and lands in the same "worker still alive, don't free pathScratch" timeout-expiry branch that both old and new code reach. It legitimately exercises the new setWorkerJoinTimeoutMillis seam / timeout branch, but the sole regression guard for the actual interrupt bug is testInterruptedCallerDoesNotAbandonReapableWorker. Consistent with the PR's own framing — informational, no change required.

3. SegmentManager.java:178 — deadlineNanos arithmetic is not overflow-hardened (minor/defensive). System.nanoTime() + workerJoinTimeoutMillis * 1_000_000L can overflow only via the @TestOnly setWorkerJoinTimeoutMillis with an absurd value (or ~292-year JVM uptime); on overflow the worker gets zero wait. Unreachable in production (the field keeps its 5s default). Worth a guard or a documented precondition on the setter only if you want defense-in-depth; otherwise ignorable.

4. SegmentManagerCloseRaceTest.java:255-256 — the 300 ms regression window is theoretically weak under pathological CI load (test robustness). If the closer thread failed to run manager.close() to completion within 300 ms, the assertFalse would false-pass against buggy code (masking the regression). Extremely unlikely for a microsecond operation (measured buggy failure end-to-end: 42 ms), and it can never cause a flaky failure of the fixed code — close() blocks in join until releaseWorker.countDown(), which the test calls only after the assert. Noting for the record.

Downgraded (candidates dismissed after source verification)

• Busy-spin under repeated interrupt — not reachable. join() clears the interrupt flag when it throws, so a single interrupt costs one immediate-return, not a spin; the only production interrupt source (BackgroundDrainerPool.shutdownNow) interrupts each thread once; the loop is bounded by the deadline regardless.
• join(0) = wait-forever — the remainingMillis <= 0 → break guard sits between the computation and the join, so join only ever sees >= 1.
• Double-free / leak of pathScratch — DirectByteSink.deflate() guards if (impl == 0) return; and zeroes impl, so close() is idempotent; the wedged-worker early-return intentionally leaks the one 256-byte buffer because the live worker may still touch it (correct — avoids use-after-free).
• workerJoinTimeoutMillis non-volatile — test-only field, written and read on the same thread before close(); program-order happens-before; no production write path. Harmless.
• 5s block under interrupted shutdown regresses "leave now" — intended tradeoff, not a violation. This is teardown, not a drainer reconnect budget. The block only materializes if the worker is wedged in a hung-disk syscall; in every production path it lands on a daemon thread (drainer or I/O loop) after BackgroundDrainerPool.close() has already returned, so it is not user-visible shutdown latency and never blocks JVM exit. The drain loop itself still exits immediately on the folded interrupt.
• close() doesn't reset rings/totalBytes — pre-existing, not touched by this PR; owned managers are never restarted, so irrelevant.

Note (harness, cross-cutting)

assertMemoryLeak calls skipChecks() on any throwable, so on an assertion-failure path the harness does not additionally enforce a per-tag leak check — leak-on-failure safety rests solely on the finally blocks. Those blocks are correct in both tests, so nothing leaks; just don't rely on the harness to catch a leak when an assertion fails.

Summary

Verdict: approve. A correct, narrowly-scoped fix for a real data-corruption bug (leaked worker → concurrent truncation/unlink → SIGBUS/InternalError). No regressions or data-loss paths introduced; store-and-forward invariants hold (bounded teardown join, not a reconnect budget; unacked data stays on disk for the next orphan scan). Java 8 floor respected, member ordering compliant, PR metadata clean.

• In-diff vs out-of-diff: all kept findings are in-diff. The cross-context pass walked every SegmentManager.close() callsite (sole production owner CursorSendEngine under ownsManager, traced through all four paths: foreground sender, drainer teardown, delegated I/O-thread close, constructor-failure) — each resolves to SAFE. Zero out-of-diff findings is legitimate given how small and self-contained the change is.
• Regression coverage: testInterruptedCallerDoesNotAbandonReapableWorker is verified as the true (and sole) regression guard — fails against the reverted production hunk, passes with it, with no flaky-fail risk against the fixed code.

The only things worth changing before merge are the two test-code minors (#1 duplication, #2 which test carries the regression value), and both are optional.

[mtopolnik]

[PR Coverage check]

😍 pass : 14 / 16 (87.50%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java	14	16	87.50%

[bluestreak01]

Code review — PR #65, level 3 (blocking, mission-critical pass)

fix(qwp): fix a memory-access fault and worker leak during shutdown

Scope: exactly 2 files, +109/−7 — SegmentManager.java (production close() hardening) and SegmentManagerCloseRaceTest.java (one new regression test + one modified test). Committed-binary gate: PASS — the .so/.dylib/.dll entries that appeared in a raw git diff are an artifact of a stale local main; gh pr diff --name-only and git diff <base> <head> both confirm the real PR touches only those 2 Java files, and git ls-files shows the native libs are not tracked (CI builds them from source).

Verification method

I reproduced the bug and the fix by execution, not just reading:

• With the fix: SegmentManagerCloseRaceTest 3/3 green; the broader SegmentManager* + BackgroundDrainerInterruptIsStopSignalTest (the previously-flaky test) + BackgroundDrainerInterruptedTeardownTest (pins the deliberately-preserved interrupt) = 18 tests, 0 failures.
• With the production hunk reverted (temporarily restored the old t.join(5_000)/catch): testInterruptedCallerDoesNotAbandonReapableWorker fails at SegmentManagerCloseRaceTest.java:255 — "interrupted close() abandoned a live worker instead of waiting" (total test time 0.077 s) — exactly as claimed. The production file was then restored byte-identical to PR HEAD (clean git status).

The fix is correct. BackgroundDrainerPool.shutdownNow() interrupts the drainer runner thread and deliberately leaves the flag set (stopRequestedOrInterrupted() uses isInterrupted()); the runner's finally then calls engine.close() → manager.close() on that interrupted thread (drainer engines own their manager — CursorSendEngine(sfDir, size, maxTotalBytes, deadline) sets ownsManager=true). Pre-fix, t.join(5_000) threw InterruptedException immediately, the worker got zero time to see running=false, and close() returned early leaking a live worker that kept truncating/unlinking segment files → SIGBUS/InternalError in a later mmap. The fix saves+clears the interrupt, does a deadline-bounded join that actually reaps the worker, then restores the interrupt for the rest of the teardown protocol.

Critical

None.

Moderate

None.

Minor

1. Duplicated test scaffolding between the two blocked-worker tests (SegmentManagerCloseRaceTest.java, test-code quality — in-diff). The setBeforeInstallSyncHook(...) lambda (~lines 155–166 vs 226–237) and the setup preamble are byte-identical across testCloseDoesNotFreePathScratchWhenWorkerStillAlive and testInterruptedCallerDoesNotAbandonReapableWorker; the tests genuinely diverge only after the worker blocks. Extract a blockWorkerAtInstallHook(...) helper and keep the divergent bodies per-test. Not blocking.

2. deadlineNanos arithmetic is not overflow-hardened (SegmentManager.java:179, defensive — in-diff). System.nanoTime() + workerJoinTimeoutMillis * 1_000_000L can overflow only via the @TestOnly setWorkerJoinTimeoutMillis with an absurd value, or ~292-year JVM uptime; on overflow the worker gets a zero-wait early-return (degrades gracefully, no crash). Unreachable in production (the field keeps its 5 s default; verified no production caller). Optional guard/precondition on the setter only.

3. The 300 ms regression window is theoretically weak under pathological scheduling starvation (SegmentManagerCloseRaceTest.java:254-255, test robustness — in-diff). If the closer thread failed to reach manager.close() within 300 ms, assertFalse would false-pass against buggy code (masking the regression). It can never flaky-fail the fixed code (close() blocks in join until releaseWorker.countDown(), which the test calls only after the assert), and the measured buggy path failed at 77 ms ≪ 300 ms, so this is a note for the record, not a defect.

4. Metadata: PR has no labels; a Core/Bug label would match scope. Trivial.

Downgraded (false positives dismissed after source + execution verification)

• Busy-spin under repeated interrupt — not reachable. join() clears the interrupt flag when it throws, so each interrupt delivery costs one loop iteration; the sole production interrupt source (BackgroundDrainerPool.shutdownNow) interrupts once, and the loop is deadline-bounded regardless.
• join(0) waits forever — the remainingMillis <= 0 → break guard sits before the join, so join only ever sees >= 1.
• Non-volatile workerJoinTimeoutMillis visibility — production never writes it (stays at the 5 s field-init default); the cross-thread test establishes happens-before via closer.start(). Harmless.
• 5 s block under interrupted teardown regresses "leave now" — intended tradeoff, not an SF-invariant violation. This is a bounded teardown join, not a drainer reconnect budget (the drainer's reconnect in CursorWebSocketSendLoop is untouched). It only materializes on a genuinely wedged worker, and only on the pool's daemon runner thread after BackgroundDrainerPool.close() has already returned via shutdownNow() without awaiting — so it never blocks the user, the pool's close(), or JVM exit, and unacked data stays on disk for the next orphan scan (no data loss).
• pathScratch double-free / wedged-worker leak — pre-existing and deliberate (DirectByteSink.deflate() is idempotent; the wedged-worker early-return intentionally leaks the one 256-byte buffer to avoid use-after-free by the still-live worker). Not touched by this PR.
• testCloseDoesNotFreePathScratchWhenWorkerStillAlive is now the regression guard — false; verified by revert that it still passes without the fix (the old immediate-throw also lands in the same timeout-expiry branch). It legitimately exercises the new setWorkerJoinTimeoutMillis seam / timeout branch, but the sole regression guard is testInterruptedCallerDoesNotAbandonReapableWorker. Consistent with the PR's own framing — informational.

Coverage map

Change (symbol + branch)	Test (local)	Failure link	Dimensions	Verdict
SegmentManager.close() — interrupt-robust reap: clear+restore interrupt, deadline-bounded join actually reaps worker under pending interrupt	testInterruptedCallerDoesNotAbandonReapableWorker (local; proven by revert)	assertFalse(closeReturned.await(300ms)) + assertNull(readWorkerThread) — reverting the hunk makes close() return instantly (worker abandoned) → assert fails (verified, 77 ms)	happy ✓, interrupt-preserved ✓, resource cleanup/assertMemoryLeak ✓; concurrency ✓ (cross-thread closer); real-server N-A (in-JVM interrupt, no server observable)	TESTED
SegmentManager.close() — timeout branch: worker still alive past deadline → warn + early return, pathScratch not freed, workerThread retained	testCloseDoesNotFreePathScratchWhenWorkerStillAlive (local; reaches branch via new setWorkerJoinTimeoutMillis(50L) seam)	asserts worker still alive, pathScratch impl != 0, interrupt preserved after timed-out close	timeout-branch ✓, interrupt-preserved ✓, native-mem-not-freed ✓, assertMemoryLeak ✓	TESTED (branch; not the interrupt-bug regression guard)
New workerJoinTimeoutMillis field + WORKER_JOIN_TIMEOUT_MILLIS constant	—	Default 5 s preserves the pre-fix bound; production never mutates it (verified)	no behavioral change	EXEMPT
New @TestOnly setWorkerJoinTimeoutMillis(long)	—	Test-only seam; rg confirms zero production callers	no behavioral change	EXEMPT

Tandem gate (Step 2.7): no tandem required — unit-testable-only. The change is a purely in-JVM thread-lifecycle reap on an in-process interrupt (shutdownNow), with no server-observable effect (no wire/protocol/handshake/flush-retry change) and no dependence on a kill -9. It is deterministically reproduced by the local regression test. Searches recorded: gh pr list --repo questdb/questdb --head fix-segmentmanager-interrupted-close and --repo questdb/questdb-enterprise both empty; no tandem/e2e links in the body — correctly, since none is required.

Summary

Verdict: approve. A correct, narrowly-scoped fix for a real crash-safety bug (interrupted-teardown → leaked file-owning worker → concurrent truncation/unlink → SIGBUS/InternalError). The fix strictly hardens close(); every traced callsite is unaffected or improved.

• Test & tandem gate: PASS. The fix ships with a regression test proven (by revert-execution) to fail without it; no UNTESTED-Critical rows; no required-but-missing tandem (unit-testable-only, verified).
• Zero-GC gate: N-A/PASS. close() is a once-per-lifecycle teardown path, not a per-row/per-producer path; the new loop allocates nothing (primitives only). No hot-path allocation introduced.
• Store-and-forward invariants: hold. Bounded teardown join (not a reconnect budget); no transport error propagated to any caller; no terminal latching; runs on an abandoned daemon; unacked data stays on disk.
• Java 8 floor / conventions: OK. Uses only Java 8 APIs; member ordering compliant (new static const, instance field, and method all alphabetical within their groups); Conventional-Commits title with fix(qwp) scope; commit message plain-English < 50 chars with proper body.
• Coverage map totals: 4 behavioral rows — 2 TESTED (0 via tandem), 2 EXEMPT (verified no-behavioral-change), 0 UNTESTED.
• Findings: 0 Critical, 0 Moderate, 4 Minor kept; 6 candidate findings dropped as false positives after source + execution verification.
• In-diff vs out-of-diff: all 4 kept findings in-diff; 0 out-of-diff — legitimate here, as every SegmentManager.close() callsite (CursorSendEngine constructor-fail catch ×2, primary close() reached from the foreground sender, the interrupted drainer runner finally, and the delegated I/O-thread exit) was walked and each resolves to SAFE under the new interrupt-robust join.

The only optional cleanups before merge are Minor #1 (test duplication) and, if you want defense-in-depth, Minor #2 (setter precondition). Neither blocks.