Close a dead passive during reconnect, freeing up resources.

[scottf]

Addresses #24

[synadia-claude-reviewer]

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

---

PR Review

• Gather PR diff
• Read source files for context
• Analyze changes
• Post review

---

Summary

The fix is correct and addresses a real resource leak. When both servers are down (whole-cluster path), the old code did passiveConnection = null without closing the connection, leaving the passive's internal reconnect thread (from maxReconnects(-1)) running as an orphan. It would compete with the active's reconnect and, on recovery, reconnect as a phantom that nothing would ever close.

The fix properly captures the reference, nulls the field first (re-entrancy guard), then closes.

Observations

Correctness — The null-before-close ordering is correct. Nulling passiveConnection first ensures any re-entrant reconnectImplConnect() call (e.g. if handleCommunicationIssue fires during deadPassive.close()) short-circuits at the passiveConnection == null guard rather than trying to close again.

Synchronous close on the reconnect thread — deadPassive.close() runs synchronously on the active's reconnect thread, potentially blocking super.reconnectImplConnect(). Since the passive's socket is already dead at this point, the close should complete quickly (it interrupts the passive's reconnect loop rather than waiting for a live socket to drain). Acceptable tradeoff.

Test design — The test is well-structured. Using noRandomize() for deterministic server assignment, capturing deadPassive before triggering the outage, and the two-phase shutdown (passive server first, then active server) correctly isolates the whole-cluster code path. The waitFor helper's final cond.getAsBoolean() after the loop avoids false-negatives at the boundary.

One minor note: if the passive happens to be in RECONNECTING state when close() is called (which it likely will be after step 1), NatsConnection.close() needs to interrupt that reconnect loop. Assuming the underlying jnats close is interruptible, the 20-second timeout in the test is generous enough.

No issues found. The fix is minimal, targeted, and the test directly exercises the bug path.

[scottf]

Good call to flag the assumption rather than take it on faith — I traced it through the current jnats source and it holds. close() interrupts the reconnect two ways: it sets closing = true and calls reconnectWaiter.cancel(true), which immediately wakes any in-progress backoff wait. The passive's reconnect loop (reconnectImplConnect) then bails at the top of the next server iteration via its isClosed() / isDisconnectingOrClosed() || isClosing() checks. The only genuinely-blocking point left is an in-flight tryToConnect socket attempt, and that's hard-bounded by connectionTimeout (~2s default). So worst case close() returns in about one connection timeout — well inside the test's 20s.

And the test already lands in exactly the state you described: it kills the passive's server first (passive drops into RECONNECTING), then the active's, so close() runs against a mid-reconnect passive — and it still completes in ~9s.