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.

[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%

[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.