Skip to content

fix(connections): cancel a pending connection for real and stop replaying it on relaunch (#1358)#1874

Merged
datlechin merged 1 commit into
mainfrom
fix/1358-cancel-connection-attempt
Jul 14, 2026
Merged

fix(connections): cancel a pending connection for real and stop replaying it on relaunch (#1358)#1874
datlechin merged 1 commit into
mainfrom
fix/1358-cancel-connection-attempt

Conversation

@datlechin

Copy link
Copy Markdown
Member

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 blocking PQconnectdb on a GCD queue through pluginDispatchAsyncCancellable, 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.cancel drops 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.json was 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 PQconnectdb with libpq's documented non-blocking path: PQconnectStart plus a PQsocketPoll/PQconnectPoll loop on a 100ms slice, checking a cancel flag between polls, with PQfinish on the owning thread (libpq requires one thread per PGconn, so aborting from another thread is not safe). The flag is flipped by a real withTaskCancellationHandler handler, and by disconnect(). connect_timeout is documented as ignored under PQconnectPoll, 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 into activeSessions or 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 the recoverDeadTunnel path that bypasses the OnceTask dedup 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):

[1] Cancel after 0.3s (user clicks Cancel):
    outcome=CANCELLED  elapsed=0.40s
[2] No cancel (app-owned 10s deadline):
    outcome=TIMED OUT (deadline)  elapsed=10.01s

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 used Task.sleep, which is cooperatively cancellable, so it could not model the real hazard.
  • CancelledConnectionCleanupTests: extended to cover attempt invalidation on cancel.

Notes for review

  • No UI automation. TABLEPRO_UI_TESTING=1 makes 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.
  • The "unresponsive window" in the video is probably not a hang. TablePro has no 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.
  • No PluginKit ABI change. The existing pluginDispatchAsyncCancellable signature is untouched (its cancellationCheck parameter 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.
  • Residual limitation: a hostname that fails DNS resolution can still block inside PQconnectStart, bounded by the system resolver. The reported case (a literal IP) is fully covered.

@datlechin datlechin merged commit be9b144 into main Jul 14, 2026
2 checks passed
@datlechin datlechin deleted the fix/1358-cancel-connection-attempt branch July 14, 2026 12:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Canceling connection does not actually cancel

1 participant