Separate pool for the passive and address proposals

[scottf]

This PR Addresses

✅ Proposal 1 — ApPassiveServerPool: Add setLameDuckServer() API

✅ Proposal 4 — ApOptions.Builder: Add passiveServerPool(ServerPool) Method

✅ Proposal 7 — ApConnection.reconnectImplConnect(): Guard Against Dead Passive Socket

✅ Proposal 8 — ApConnection.reconnectImplConnect(): Socket-steal causes infinite reconnect loop

✅ Proposal 9 — ApConnection.reconnectImplConnect(): pingTask / cleanupTask zombie after socket steal

✅ Proposal 10 — DataPort: No lifecycle hook for connection rebinding after socket steal

✅ Api to manually switch to passive connection

[synadia-claude-reviewer]

Claude finished @scottf's task in 3m 14s —— View job

---

PR Review

• Gather context
• Read PR diff and changed files
• Analyze for correctness, security, and performance
• Post inline comments on specific issues
• Post summary review

---

The PR cleanly decouples the active and passive server pools via the new ApServerPool interface and associated plumbing. The cast safety invariant in connect() is sound — activeSp is guaranteed to be ApServerPool by the time it can be assigned to passiveSp. Renaming passive → passiveConnection and apServerPool → activeServerPool/passiveServerPool improves clarity throughout.

Three issues worth addressing:

1. IOException cause is dropped in newPassive() (inline comment on line 125 of ApConnection.java). The wrapping RuntimeException should chain the original cause — throw new RuntimeException("...", e) — or diagnosing passive-connect failures in production will be painful.

2. Double-dispatch when pools are shared (inline comment on line 67 of ApConnection.java). In the default configuration activeServerPool == passiveServerPool, so BridgeConnectionListener fires activeConnectionEvent (or passiveConnectionEvent) twice on the same object per connection event. Harmless now since both methods are no-ops, but the TODO lame-duck implementation will need a guard here: if (activeServerPool != passiveServerPool) { passiveServerPool.activeConnectionEvent(...); }.

3. passiveServerRef is tracked but never consulted (inline comment on line 43 of ApPassiveServerPool.java). The pool stores the passive server URI but peekNextServer()/nextServer() only avoid the activeServerRef. Whether this is intentional (future-work placeholder) or an oversight isn't clear from the code alone — a brief comment noting the design intent would help.

No security or thread-safety regressions observed relative to the pre-existing baseline.

[scottf]

Writing tests for this turned up three things worth calling out, all of which affect ordinary failover too, not just the manual switch:

1. Outstanding PINGs were being carried across the promotion. A normal (re)connect gets cleanUpPongQueue() for free inside tryToConnect, but the steal never goes through tryToConnect, so pong futures belonging to the dead socket stayed queued. maxPingsOut defaults to 2, and the socket you're failing away from usually has unanswered PINGs on it precisely because it stopped responding — so the first ping after promotion could raise "Max outgoing Ping count exceeded" and spawn a reconnect on the connection we had just successfully failed over to. The queue is now cleared at the swap, which also releases anything parked in flush() instead of leaving it to time out.

2. The promoted socket never got a ping. Every normal connect ends with a ping/pong round trip; the steal was the one path that reached CONNECTED without ever touching the wire. It now sends one — softPing, and not waited on, since the socket was already warm in the passive's hands and if it does turn out to be dead the ordinary ping machinery reconnects us the same as any other connection. This one needed needPing reset at the swap as well: it's per-connection state describing the current socket and deliverMessage clears it, so after the swap the value we're holding describes the socket we just discarded. Without the reset, an active that delivered a message shortly before its socket failed would have suppressed the promoted socket's ping entirely.

3. The passive connection listener was getting a CLOSED event on every promotion. A promoted passive isn't lost — its socket is alive and now serving the active — but it was still being closed with its listeners attached, so an app had no way to tell a successful failover from a passive failure. Its listeners are now detached before that close, so the passive listener sees only the CONNECTED for the newly armed standby.