Fix Test Flappers

[scottf]

No description provided.

[synadia-claude-reviewer]

Claude finished @scottf's task in 2m 40s —— View job

---

PR Review

• Gather context and understand request
• Read CLAUDE.md for repo guidelines (none found)
• Review PR diff and changed files
• Analyze correctness, thread safety, and test quality
• Post findings

---

The changes are clean and the motivation is sound. A few observations:

NatsConnectionImplTests.java — testOutgoingPendingCountCoverage

The writer-stop approach is a good idea — stopping the writer before publishing guarantees a deterministic backlog in normalOutgoing. The original test was genuinely unwinnable: on a fast machine the queue drains to zero before the read, on a slow machine the reconnect buffer fills and publish throws.

One thing worth confirming: after stop() calls normalOutgoing.pause() and normalOutgoing.filter(), are the subsequently-published messages still visible to normalOutgoing.length() inside outgoingPendingMessageCount()? The test relies on filter() not clearing future enqueues — only messages already in the queue at stop time. If that assumption holds (which the design suggests), the logic is correct.

Also worth noting: with the writer stopped, the reader is still running. If the server sends a PING before the test completes, no PONG can be queued, and the server may eventually time out the connection. For a test this short this is not a practical concern, but it's something to be aware of if the test ever flaps with a "connection closed" error.

KeyValueTests.java — poll-loop refactors

Converting from ++safety < 10000 to System.currentTimeMillis() < timeoutAt is the right call. An unthrottled round-trip loop has wildly different wall-time behaviour across machines; 10 seconds is a clear, machine-independent bound. The added sleep(10) prevents spin-waiting. Good.

The mark reordering in the third test section — capturing mark before kv.delete(key) instead of after — is a subtle fix. The 1-second TTL assertion (gotZero - mark >= 1000) measures elapsed time since the operation; capturing mark after the operation was always slightly wrong. The new ordering is correct.

ConnectTests.java — removal of testConnectPendingCountCoverage

Appropriate. The combination of a polling background thread and 3000 unthrottled publishes made it prone to both races and reconnect-buffer overflow on slow/contended machines.

No correctness, security, or thread-safety issues found. The refactors are minimal and targeted — they fix the flakiness without adding unrelated changes.

[philpennock]

This still seems prone to flakiness, isn't there a fake clock which is appropriate to use here, tied into the thing being tested?

[scottf]

This still seems prone to flakiness, isn't there a fake clock which is appropriate to use here, tied into the thing being tested?

I think it's better, but will watch it to see if it's still flaky.