Update the status before tearing the socket down in closeSocket

[scottf]

Fixes #1608

Fixes the window where getStatus() returns CONNECTED while getConnectedUrl() returns null.

closeSocket called closeSocketImpl(forceClose) and updated the status afterwards. Since closeSocketImpl clears the current server as its first statement and then waits on the reader and writer stop futures (1 second each), everything between those two points reported a connection that was CONNECTED to nowhere. Any health check, metric or log line sampling the connection during a communication failure teardown could see it, with no callback delay involved.

The change moves updateStatus(Status.DISCONNECTED) up into the block that already sets disconnecting = true, after wasConnected is captured so the reconnect decision is unaffected:

this.disconnecting = true;
this.exceptionDuringConnectChange = null;
wasConnected = (this.status == Status.CONNECTED);

// Update the status before tearing the socket down, not after. ...
updateStatus(Status.DISCONNECTED);

statusChanged.signalAll();

This is the order forceReconnectImpl already used, so the four paths that touch both fields now agree.

Notes for review

• wasConnected is captured before the status update, so else if (wasConnected && tryReconnectIfConnected) still behaves the same.
• updateStatus takes statusLock, which is reentrant and was already being taken this way in the block that was removed.
• The DISCONNECTED event now fires before the reader and writer are stopped rather than after. Delivery was always asynchronous on the callback executor so nothing was tightly sequenced against it, but it is the one observable change and worth a second opinion.
• updateStatus(Status.DISCONNECTED) derives its uriDetails from currentServer == null ? lastServer : currentServer. Running before clearCurrentServer() means it now takes the value from currentServer directly instead of the lastServer fallback. Same string either way, since clearCurrentServer() copies one into the other.
• exceptionDuringConnectChange = null and disconnecting = false stay after closeSocketImpl, unchanged.

Test

ConnectionStateConsistencyTests.testStatusAndConnectedUrlAreNeverInconsistentDuringTeardown polls both fields from a thread while the server is killed, and asserts the pair is never observed inconsistent.

It fails on the unfixed code and passes with the change:

before: expected: <0> but was: <293>   (5 of 5 runs failed, 128 to 445 observations)
after:  PASSED

The test kills the server rather than calling forceReconnect(), deliberately — forceReconnect goes through forceReconnectImpl, which was already correct, and a test driven that way passes with or without the fix.

[synadia-claude-reviewer]

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

---

PR Review

• Gather context (PR diff, changed files)
• Review NatsConnection.java changes for thread safety and correctness
• Review ConnectionStateConsistencyTests.java for test quality
• Check for observable behavioral changes and edge cases
• Post inline comments and summary

---

Summary

The fix is correct and well-scoped. The root cause is clear: closeSocketImpl immediately calls clearCurrentServer() (making getConnectedUrl() return null), but status wasn't updated to DISCONNECTED until after closeSocketImpl finished — a window of up to 2 seconds under default timeouts. Moving updateStatus(Status.DISCONNECTED) before the socket tear-down closes that window cleanly.

The concurrency analysis in the PR description is accurate. No new issues are introduced.

---

Thread Safety

• statusLock is a ReentrantLock (line 61), so the nested acquisition inside updateStatus called from within an already-held statusLock block is safe. Notably, this same nested pattern existed in the old code (the second statusLock block after closeSocketImpl also called updateStatus), so this is not a new risk.
• wasConnected is captured before updateStatus, so the else if (wasConnected && tryReconnectIfConnected) reconnect path at line 859 is unaffected.

One Harmless Redundancy

After the move, statusChanged.signalAll() is called twice within the same lock acquisition: once inside updateStatus (line 2195) and once explicitly on line 838. The explicit signal at line 838 is now redundant — updateStatus already signals via the same condition. This is benign but slightly noisy.

Observable Behavioral Change

The DISCONNECTED connection event now fires before the reader and writer threads are stopped, instead of after. The PR correctly identifies this as the one observable change and justifies it: the callback is always delivered asynchronously on the callback executor, so nothing was tightly sequenced against the teardown order. This is acceptable.

uriDetail Correctness

updateStatus(Status.DISCONNECTED) resolves uriDetail from currentServer == null ? lastServer : currentServer (line 2179). Called before clearCurrentServer(), it reads currentServer directly. Called after clearCurrentServer(), it would fall back to lastServer. Since clearCurrentServer() copies currentServer → lastServer before nulling it (lines 730–734), the resulting string is identical either way. The PR's note on this is accurate.

---

Test: testStatusAndConnectedUrlAreNeverInconsistentDuringTeardown

The test design is sound — it polls both fields from a tight spin loop while killing the server, which is exactly the right way to expose this race. @Isolated is appropriate. The finally block is thorough.

One concern worth noting: the assertion assertTrue(samples.get() > 1_000_000, ...) could be fragile on slow or heavily-loaded CI runners. The effective polling window is roughly: time from server kill until disconnect is detected (typically well under a second) plus the fixed 1-second sleep — potentially as little as ~1.1 s. At 1 M iterations/second from a tight spin loop, this is borderline on a constrained runner. If this assertion flakes in CI, raising the post-teardown sleep (e.g. from 1 s to 2–3 s) or reducing the threshold would be the straightforward fix.

---

Overall: The production change is a minimal, correct fix for a real consistency window. Approved with the above observations noted for awareness.