fix(connections): cancel a pending connection for real and stop replaying it on relaunch (#1358)#1874
Conversation
…ying it on relaunch (#1358)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4772e326cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let connection = connection else { | ||
| throw LibPQPluginError.connectionFailed | ||
| private func pollUntilConnected(_ connection: OpaquePointer) throws { | ||
| let deadline = PQgetCurrentTimeUSec() + Self.connectTimeoutMicroseconds |
There was a problem hiding this comment.
Preserve libpq multi-host failover
When the PostgreSQL host field contains a comma-separated failover list and an earlier host blackholes, this single 10s deadline aborts the whole PQconnectPoll sequence before libpq can advance to the next host. The previous connect_timeout=10 behavior allowed libpq to time out each host separately, so a healthy later host could still be reached; with this global deadline those HA connections fail as soon as the first host consumes the budget.
Useful? React with 👍 / 👎.
Fixes #1358.
Reopened against 0.57.0: launch restores a window for an unreachable host, Cancel appears to do nothing lasting, and every relaunch reconnects to the same dead host.
Root cause
Three defects were stacked. Each earlier fix (#1232, #1367, #1370) addressed a symptom, so the issue kept coming back.
1. Cancel was cosmetic.
LibPQPluginConnection.connect()ran a blockingPQconnectdbon a GCD queue throughpluginDispatchAsyncCancellable, whose cancellation handler was empty (onCancel: {}).Task.cancel()is cooperative and cannot interrupt a blocking C call, so Cancel hid the UI while the attempt kept running to libpq's own 10s timeout. This is the literal issue title and it was never actually fixed.2. Attempt bookkeeping was not transactional.
OnceTask.canceldrops its dedup entry immediately while the abandoned attempt keeps running. A later attempt for the same connection could then have its session torn down by the older attempt's cleanup. The previous fix guarded only the destructive path, not the adopt path. This is the original "cancelled connect aborts a later successful connect" report.3. The recovery list replayed cancelled connections.
LastOpenConnections.jsonwas written in exactly one place,applicationWillTerminate, and snapshotted every registered coordinator including windows that never finished connecting. Cancel never updated it, and a force quit never runs that callback at all. So a connection the user explicitly cancelled was restored and re-attempted on every launch. This is the loop in the reporter's video.The fix
True cancellation (PostgreSQL driver). Replaces blocking
PQconnectdbwith libpq's documented non-blocking path:PQconnectStartplus aPQsocketPoll/PQconnectPollloop on a 100ms slice, checking a cancel flag between polls, withPQfinishon the owning thread (libpq requires one thread perPGconn, so aborting from another thread is not safe). The flag is flipped by a realwithTaskCancellationHandlerhandler, and bydisconnect().connect_timeoutis documented as ignored underPQconnectPoll, so the app now owns the deadline; it stays at 10s to preserve current behavior.Attempt generations (
ConnectionAttemptRegistry). Every attempt takes a generation token. Before adopting a driver intoactiveSessionsor tearing session state down, an attempt validates its generation; a cancelled or superseded attempt disconnects its own driver and returns instead of touching the winner's session. This also covers the drivers whose blocking connects still cannot be aborted mid-flight (MySQL and others), and it closes therecoverDeadTunnelpath that bypasses theOnceTaskdedup entirely.Event-driven recovery list (
SessionRecoveryTracker). The list is now derived from windows that actually connected (activated coordinators) and rewritten on activate, teardown, and cancel, not only at quit. A cancelled connection is dropped immediately, so neither a clean quit nor a force quit can replay it. Crash recovery, the file's stated purpose, still works because windows that were genuinely open were activated.Also adds the missing invariant to CLAUDE.md, since this area has now shipped the same class of bug four times.
Verification
The plugin connect loop cannot run in
TableProTests(the C-bridge is gated out of that target), so I verified it with a harness linking the real bundled libpq against a blackholed address (192.0.2.1, TEST-NET-1, where TCP connect never completes):Before this change that connect ran the full 10s regardless of Cancel.
Unit tests (20 passing, all new ones ran):
ConnectionAttemptRegistryTests: a late cancelled attempt cannot claim the retry that replaced it; a newer attempt supersedes the one in flight.RecoveryConnectionListTests: a cancelled attempt is not restored; connected windows survive; a connection with several windows is listed once.OnceTaskTests: added a case with genuinely non-cooperative work (ignores cancellation, like a driver blocked in C) proving a same-key rerun does not adopt the abandoned attempt. The existing test usedTask.sleep, which is cooperatively cancellable, so it could not model the real hazard.CancelledConnectionCleanupTests: extended to cover attempt invalidation on cancel.Notes for review
TABLEPRO_UI_TESTING=1makes the app skip launch setup, so the restore-then-cancel flow cannot run under XCUITest without new app-side test hooks, and it depends on a real TCP connect to an unreachable host. I chose not to add a nondeterministic test; coverage is at the unit level plus the harness above.applicationShouldTerminateAfterLastWindowClosed, so closing the last window correctly leaves the app running, which is standard macOS behavior. I found no synchronous main-thread wait in the quit path, so I did not change the termination flow. If the reporter can still reproduce a real beachball, a spindump would settle it.pluginDispatchAsyncCancellablesignature is untouched (itscancellationCheckparameter was already there and unused by this driver), so no version bump and no plugin re-release. PostgreSQL is a bundled plugin and ships with the app.PQconnectStart, bounded by the system resolver. The reported case (a literal IP) is fully covered.