Connection Listener Stale State Tests

[scottf]

Add ConnectionListener callback state tests

Follow up to #1606. No client code changes — the investigation concluded the reported behavior is correct, and these tests lock down why, so the next person to look at it does not have to redo the analysis.

The report was that a connection listener callback logged a null connectionUrl after a reconnect that happened very quickly after a previous connect or reconnect. It reproduces, on every version including current, and it is not a defect. getConnectedUrl() is a live read: by the time a queued callback runs, the client has re-entered tryToConnect, which clears currentServer, so null is the truth about that moment. It is a correct answer to the wrong question. The question the listener means to ask is answered by the time and uriDetails arguments on the four argument connectionEvent, captured when the event is raised — which is the reason to implement that method rather than the deprecated two argument one.

Two tests in ConnectionListenerStaleStateTests:

• testConnectEventDetailsStayCorrectWhenTheCallbackIsProcessedLate parks a listener inside a connect callback, tears the connection down while it is parked, and releases it only once currentServer has actually been observed cleared — so there is no timing luck. It asserts that getConnectedUrl() and getStatus() describe the moment the callback runs (documented as correct rather than treated as failure), that uriDetails still names the server the event was raised for, and that time predates the callback being released. It fails if the event detail is ever made lazy, computed when the callback runs instead of when the event is raised.

• testEventDerivedStateSettlesToTheTruthAfterRapidReconnects drives ten forceReconnect() calls past a deliberately slow listener and asserts that once the callback queue drains, an application tracking connectedness purely from events believes exactly what is true. Callbacks reading a status that disagrees with their own event is expected and transient; a listener view that stays wrong is not.

Both are @Isolated and use NatsTestServer.

[synadia-claude-reviewer]

Claude finished @scottf's task in 5m 59s —— View job

---

Code Review

• Gather context
• Read PR diff
• Examine relevant source files
• Analyze for correctness, thread safety, and exception handling
• Post review findings

---

The test design is sound. The GatedListener correctly uses CountDownLatch for synchronisation, all writes to shared state happen-before the corresponding countDown()/await() barriers, and the callback executor is a single-thread executor so sequential delivery is guaranteed — the armed = false guard is safe. The contract being pinned (event details are captured at raise-time, not read lazily when the callback runs) is a real and useful one to have as a test.

Three issues found:

1. Double close of NatsTestServer (lines 123 and 151) — ts.close() is called in the test body to simulate the server disappearing, then again unconditionally in the finally block. Every comparable test in ConnectionListenerTests and ReconnectTests that closes a server mid-test does not attempt a second close. Inline comment at line 151.

2. assertNotEquals vs assertTrue(x != y) (line 134) — the assertTrue form produces an opaque failure message with no indication of what the actual status was. Inline comment at line 134.

3. Drain loop timeout is silent (line 244) — if all 40 iterations exhaust without the event count stabilising, the test silently proceeds and the subsequent assertEquals will fail with a misleading message. Inline comment at line 244.

[synadia-claude-reviewer]

ts.close() is called here in the finally block, but it was already called on line 123 inside the try body to simulate the server going away. That means ts is closed twice on the happy path. Everywhere else in ConnectionListenerTests and ReconnectTests that needs to close a server mid-test simply doesn't close it again in cleanup. The finally block should guard with a flag, or—since ts must stay open until the mid-test close—just remove the redundant finally call and leave cleanup to the test body:

Suggested change

	finally {
	if (nc != null) {
	standardCloseConnection(nc);
	}

[scottf]

Not taking this one — the premise is wrong and the suggested change would introduce a process leak.

NatsTestServer extends NatsServerRunner, whose close() calls shutdown(true), which opens with if (process != null) and ends by setting process = null. Verified against the bytecode in jnats-server-runner-3.1.0. The second close() is a guarded no-op, not a double shutdown.

More importantly, the finally call is not redundant — it is the only cleanup on the failure paths. If the test fails before the mid-test close (the arrived.await(...) assertion timing out, for example), removing it leaves a nats-server process running after the test.

The comparison to ReconnectTests does not hold either: those use try-with-resources, which gives them failure-path cleanup for free. This test cannot, because the server has to be shut down mid-test while the connection stays alive, so the explicit finally is doing the job try-with-resources does elsewhere.

[synadia-claude-reviewer]

assertTrue(x != y) gives a poor failure message ("expected true but was false"). For an enum assertion assertNotEquals prints the actual value, which is essential for debugging CI failures where the status is something unexpected like RECONNECTING.

Suggested change

	"getConnectedUrl() inside a callback is a live read");
	assertNotEquals(Connection.Status.CONNECTED, listener.statusWhenCallbackRan.get(),

[scottf]

Agreed, applied in 3db0f70. assertTrue(x != y) reports "expected true but was false" and drops the value; assertNotEquals prints the actual status, which is what you want when CI shows something unexpected like RECONNECTING.

[synadia-claude-reviewer]

If the loop exhausts all 40 iterations without the count stabilising, it silently falls through to the assertEquals below, which will then produce a confusing assertion failure ("belief was DISCONNECTED but status is CONNECTED") with no indication that draining timed out. Adding a post-loop guard makes the failure mode explicit:

Suggested change

	seen = listener.eventsSeen.get();
	for (int i = 0; i < 40 && seen != listener.eventsSeen.get(); i++) {
	seen = listener.eventsSeen.get();
	Thread.sleep(listener.workMs + 100);
	}
	assertEquals(seen, listener.eventsSeen.get(), "callback queue did not drain within timeout");

[scottf]

Good catch, applied in 3db0f70. If the loop exhausted all 40 iterations without stabilising it fell straight through to the belief assertion, which would then fail with a message describing the wrong problem. Asserting the drain completed first makes the real failure mode explicit.

[mtmk]

LGTM