competing reconnect loops when the entire cluster goes down — important for production stability

[scottf]

When the whole cluster goes down, handleCommunicationIssue fires on both active and passive simultaneously. Both connections independently spin up reconnect threads and hammer the same dead brokers. This doubles the reconnect load hitting the brokers the moment they come back up. At scale — many application instances, each with two competing threads firing every 0–21 ms — the recovering brokers can be flooded before they stabilise.

More critically: even when active reconnects successfully via super.reconnectImplConnect(), newPassive() is never called. The warm standby is silently lost with no log entry or alert. The application then runs in active-only mode — with none of the AP failover guarantees — until the next restart, with nothing in the logs to indicate the degraded state.

Current code path (simplified):

// ApConnection.reconnectImplConnect() — whole-cluster path
if (!passive.isConnected()) {
    super.reconnectImplConnect(); // active reconnects, but...
    return;                       // passive's independent thread still running
                                  // newPassive() never called — warm standby silently lost
}

Proposed fix — close the passive connection before reconnecting active so its competing thread is stopped, then re-establish the warm standby once active is back:

if (!passive.isConnected()) {
    NatsConnection deadPassive = this.passiveConnection;
    this.passiveConnection = null;  // null before close so re-entrant guard still works
    deadPassive.close();            // stops passive's independent reconnect thread

    super.reconnectImplConnect();   // active is now the sole reconnect driver
    if (isConnected()) {
        apServerPool.setActiveServer(currentServer); // update pool before newPassive()
        newPassive();               // re-establish warm standby once active is back
    }
    return;
}

One additional consequence worth flagging: if maxReconnects is finite, passive's racing thread accumulates connectFailed() calls against the shared underlying pool before it is stopped. Once a broker's failed-attempt counter reaches the limit, that broker is physically removed from entryList. By the time active recovers and newPassive() runs, the pool may already have entries removed — nextServer() returns null immediately and the passive is never re-established. With maxReconnects = -1 this does not apply, but it is a real risk for any deployment using a finite reconnect limit.

[scottf]

Review by Claude

Issue #24 — competing reconnect loops on whole-cluster outage — analysis vs. current code

Verdict: partially addressed. The issue's headline — "even when active reconnects, newPassive() is never called; the warm standby is silently lost" — is stale: the current code already re-arms the passive after the active recovers. But the issue's root concern — the passive's independent reconnect thread is never stopped — is still real, and in the current code it also causes a resource/thread leak (the dead passive is dropped without being closed). The finite-maxReconnects sub-concern is moot here. Details below, then a concrete fix.

The current whole-cluster path

ApConnection.reconnectImplConnect(), lines 236-250:

if (!passiveConnection.isConnected()) {
    passiveConnection = null;          // (243) reference dropped, but the object is NOT closed
    super.reconnectImplConnect();      // (244) active reconnects through the pool
    if (isConnected()) {
        activeConnectSucceeded();       // (246) updates the server pool's active server
        reArmPassive();                 // (247) -> newPassive() re-establishes the warm standby
    }
    return;
}

Claim-by-claim

1. "newPassive() is never called — warm standby silently lost." → ALREADY FIXED.
Line 247 calls reArmPassive() → newPassive() once the active is back. The standby is re-established. The issue's "simplified current code path" (which shows only super.reconnectImplConnect(); return;) predates this and no longer matches.

2. "Update the pool's active server before newPassive()." → ALREADY DONE.
Line 246 activeConnectSucceeded() calls activeServerPool.activeConnectSucceeded(currentServer) (and the passive pool if separate), which sets the pool's active-server reference before reArmPassive() runs — so the new passive is chosen distinct from the recovered active. This is the equivalent of the issue's proposed setActiveServer(currentServer).

3. "Competing reconnect loops — the passive's independent thread keeps hammering the brokers." → STILL REAL, and worse: it leaks.
Line 243 sets passiveConnection = null but never calls close() on the dead passive. The passive is a separate NatsConnection with maxReconnects(-1), so its own reconnect thread keeps running. Two consequences:

• Competition: during super.reconnectImplConnect() (line 244) — which can span the entire outage while the cluster is down — the dead passive's thread retries in parallel, doubling the reconnect load on the recovering brokers. Exactly the issue's concern.
• Leak (not in the issue, but the same root cause): newPassive() closes its predecessor only via oldPassive = passiveConnection (line 157) and if (oldPassive != null) (line 179). But we already nulled passiveConnection, so oldPassive == null and the dead passive is never closed by anyone. Its reconnect thread (and executors) run forever, and when the cluster returns it reconnects as a phantom connection that nothing owns or closes. Every whole-cluster outage leaks one.

4. "Finite maxReconnects → pool entries physically removed → passive never re-armed." → MOOT here.
ApConnection forces maxReconnects(-1) on both sides — active (connect(), activeBuilder.maxReconnects(-1)) and passive (constructor, passiveOptions … maxReconnects(-1)). A caller cannot make these finite; the value is overridden. So the failed-attempt-counter → entryList removal path the issue describes cannot occur for AP connections. Worth a one-line note in any fix comment, but nothing to build.

The fix

Close the dead passive before letting the active reconnect, so its thread stops competing and it can't leak. This is the issue's proposed shape, and it is safe re: the executor lifecycle that newPassive()'s comment (lines 150-156) worries about — for two reasons specific to this path:

• That comment is about the socket-steal branch, where the adopted reader is live on the passive's executor; shutting it down would kill a reader that's now serving the active. In the whole-cluster branch there is no steal and no adopted reader on the passive executor, so shutting it down kills nothing live except the reconnect thread we want stopped.
• Even if closing drops the shared passiveOptions executor refcount to zero and shuts the internal executors, jnats re-creates them: Options.getScheduledExecutor() / getCallbackExecutor() rebuild when resolved == null || resolved.isShutdown(). So the subsequent newPassive() → new NatsConnection(passiveOptions) gets fresh executors.

if (!passiveConnection.isConnected()) {
    // Stop the dead passive's OWN reconnect thread before the active reconnects: otherwise both hammer the
    // recovering brokers, and - because newPassive() re-arms from a now-null field - the dead passive is
    // never closed, so its maxReconnects(-1) thread runs forever and reconnects as a phantom on recovery.
    // Null the field FIRST so the re-entrant guard (passiveConnection == null) short-circuits a concurrent
    // handleCommunicationIssue during the close.
    NatsConnection deadPassive = passiveConnection;
    passiveConnection = null;
    deadPassive.close();               // safe: no adopted reader on the passive executor in this path, and
                                       // Options rebuilds shut-down internal executors on next use, so the
                                       // re-armed passive below still gets a working pool.
    super.reconnectImplConnect();
    if (isConnected()) {
        activeConnectSucceeded();
        reArmPassive();
    }
    return;
}

Notes on the fix:

• close() is synchronous and may block up to ~connectionTimeout awaiting executor termination. During a whole-cluster outage that delay is negligible, and stopping the competing thread before the active starts is the point — so keep it synchronous and before super.reconnectImplConnect().
• close() doesn't call connectFailed(), so it doesn't perturb the shared server pool.

Optional secondary improvement (the issue's "no log / silent degraded state")

reArmPassive() already logs a failed re-arm via processException. What's silent is the window where the active is up but the passive isn't yet. If visibility of that degraded (active-only) state matters, emit one event/log when a whole-cluster reconnect leaves us temporarily without a standby, and clear it once newPassive() succeeds. This is additive and independent of the correctness fix above.

Test plan

• Leak/competition fixed: simulate the whole-cluster path (both sockets down), assert the original passive object is CLOSED after recovery (no orphan), and that exactly one passive exists afterward.
• Standby re-armed: after the active recovers, assert getPassiveStatus() == CONNECTED on a distinct server (already the intended behavior; add a regression test for the close).
• No executor breakage: assert the re-armed passive is fully functional (passiveRTT()), proving the executors survived/were rebuilt across the dead-passive close.
• Verify non-vacuously: with the deadPassive.close() removed, the "old passive is CLOSED" assertion fails (the orphan is left running).

[scottf]

addressed by #25