test(qwp): make failed segment cleanup test deterministic

[jerrinot]

When deleting an acknowledged store-and-forward segment fails, the client keeps the file so cleanup can be retried safely.

The test failed deletion only once, allowing the background worker to delete the file before the test checked it.

Keep deletion failing until those checks finish, then allow and verify cleanup.

[jerrinot]

Both tests pass on JDK 8 (the source-of-truth target). The earlier failure was a pre-existing environment issue (modern JDK compiling the java11 shim), not this PR. Review complete.

---

Review: PR #81 — test(qwp): make failed segment cleanup test deterministic (level 1)

Scope: single test file, +23/−4. No production code changed; the diff relies on pre-existing production seams (the 5-arg @TestOnly SegmentManager constructor taking a LongSupplier clock, and wakeWorker()). No binary/build artifacts in the diff — committed-binary gate passes.

Verification summary

I traced the full retry mechanism in SegmentManager.serviceRing0 to confirm the deflake is real and race-free, not merely papered over:

• The old flake: unlinkFailuresRemaining = 1 was a one-shot fault. The worker's poll-park can return early (SegmentManager.java:1394 parks up to pollNanos, woken by any unpark), so the automatic trim retry fired on the real clock and succeeded — deleting failedPath before the test's Files.exists(failedPath) / bookkeeping assertions ran. Genuinely non-deterministic; the CLAUDE.md "prove it, don't assume flaky" bar is met here.
• The fix is correct and deterministic. Retry eligibility is gated at SegmentManager.java:1165-1168 on now = ticks.getAsLong() vs trimRetryAtNanos. Injecting a fake clock (ticks::get, initially 0) pins that gate: after the first failure sets trimRetryAtNanos = now + 4ms with now = 0, no retry can fire until the test advances the clock. I verified the happens-before chain that removes the last race: the worker reads now (line 1165) before calling remove() (which counts down removeAttempted), and the test sets ticks = 1 min only after observing removeAttempted. So the first pass's now is always 0, trimRetryAtNanos is always ~4ms, and the post-wakeWorker() pass at now = 1 min always clears the gate → removeRetried always fires. A spurious wake in the gap is harmless (deferred). Keeping the fault latched (unlinkFailureEnabled) until allowUnlink() keeps the file present for the later assertions.
• Empirically green: both tests pass on JDK 8.

Critical

None.

Moderate

None.

Minor

• Pre-existing, out-of-diff (optional): readTotalBytes(...) (SegmentManagerUnlinkFailureTest.java:214-223) reaches totalBytes/lock via reflection, duplicating the public @TestOnly SegmentManager.getTotalBytesForTesting() (SegmentManager.java:729-734), which does the identical synchronized(lock) read. Not introduced or touched by this PR, so not blocking — worth a one-line swap if the file is revisited.

Test-code quality of the actual diff is clean: unlinkFailureEnabled is correctly volatile (test→worker), unlinkFailureCount is worker-thread-confined, the added comment accurately states the race being closed, and everything is Java 8-compliant (AtomicLong, method ref, TimeUnit). PR title/description follow conventions and describe end-user-neutral test impact accurately.

Downgraded (false positives)

None — no false positives to dismiss.

Summary

Verdict: approve. A well-reasoned, correctly-implemented deflake that also improves coverage (it now deterministically exercises the deferred-trim-retry gate, previously untested). 0 findings verified as blocking; 1 minor pre-existing/out-of-diff observation. In-diff/out-of-diff split: 0 blocking in-diff, 0 out-of-diff (small test-only diff to a symbol with no production callers — cross-context surface is nil, as expected).