Skip to content

fix(connections): stop showing a live window over a driver that has stopped answering - #2554

Merged
datlechin merged 1 commit into
mainfrom
fix/connection-liveness
Aug 27, 2026
Merged

fix(connections): stop showing a live window over a driver that has stopped answering#2554
datlechin merged 1 commit into
mainfrom
fix/connection-liveness

Conversation

@datlechin

Copy link
Copy Markdown
Member

A connection window keeps showing rows, tabs and an enabled toolbar over a driver that has already been disconnected: for the whole of a server outage, and permanently once the automatic reconnect gives up. Queries go to a dead handle, Reconnect does nothing, and the connections strip paints the failure while the pane beside it disagrees.

Found while investigating #2545, alongside the chrome defect fixed in #2551.

Why the window cannot tell

ConnectionSession has two liveness signals and neither answers the question.

driver is a handle. The app holds it from the moment it is built until something replaces it, and a socket the server closed is indistinguishable from a working one until a ping asks. reconnectDriver (DatabaseManager+Health.swift:151) opens with session.driver?.disconnect() on a struct-copy parameter, and reconnectSession (:267) repeats it, so activeSessions[id].driver keeps pointing at the object they just closed. ConnectionWindowPhaseMachine.onSessionChanged returned .connected on hasDriver before asking anything else, so the pane stayed on content.

status cannot stand in for it. It is .connecting for the whole of an ordinary database switch on PostgreSQL, Redshift, CockroachDB and PGlite, which all reconnect to perform one, and .disconnected is the struct's own default value. Four of the six give-up sites write .disconnected; only two write .error.

This is not only the give-up. reconnectDriver runs on every attempt and the monitor's loop is documented "Loops indefinitely", so any server that is merely down produces the same state for as long as it stays down.

The fix

ConnectionSession.liveness is one explicit answer to "can this driver be believed": live, recovering, unreachable(info). onSessionChanged reads it before hasDriver.

recovering is deliberately not a failure. A reconnect that finishes in a few seconds is a blip the user should never see, so the rows, the tabs and the toolbar stay exactly as they were. It becomes unreachable at the fifth attempt: the backoff is 2, 4, 8, 16 seconds and each attempt is announced before its own wait, so attempt 5 is the first announced a full 30 seconds after the ping that failed. Thirty seconds is also the ping interval, so the threshold is one whole cycle of not answering.

The driver is never nil'd to signal this, which is the part that took two design passes to get right. It is the handle every metadata read, every query route and the reconnect itself still goes through. Clearing it makes an ordinary database switch on those four engines look like a dropped connection, and it trips onSessionChanged's exists && !hasDriver arm into the permanent fake "Preparing the session" that failTunnelRecovery's own comment records having already been fixed once.

Every give-up site and the threshold go through markSessionUnreachable(_:startedWith:info:). Its driver-identity check is the generation guard: a reconnect blocked inside a C call cannot be cancelled and completes late, and without the fence a losing attempt would mark a connection unreachable that a later one had already restored. Every path that installs a working driver calls markSessionLive, so the mark and the reason it carried go together.

A workspace that owns the attempt reports .connecting rather than .unavailable, so pressing Reconnect dials instead of flipping back to the error, and a stale report cannot drag it back.

ConnectionSession.reportedStatus is what the connections strip, the toolbar and the MCP status tool now read, so the two channels finally agree.

Two more things this closes:

  • Reconnect was a silent no-op. ensureConnected and connectToSession both returned the moment they saw an installed driver, on exactly the connection that needed replacing. Both are liveness-aware now.
  • The health monitor was a zombie. .abort bare-returned with the state latched at .reconnecting(n), so performHealthCheck's state == .healthy guard failed every 30 seconds for the life of the app. HealthState gains .aborted and the loop exits, rather than a flag beside the state that could disagree with it.

Tests

ConnectionLivenessTests is new: 16 cases across the phase arm, the reporting accessor, the degradation threshold and the monitor's give-up.

Covered: an unreachable session leaves .connected; a recovering one does not; a live session with a driver stays connected, which is the database-switch regression guard; an owned attempt dials instead of reporting unavailable; a snapshot with no liveness given behaves exactly as before; early attempts only mark recovering while the threshold attempt marks unreachable; the driver stays installed throughout; a stale attempt cannot mark a connection someone else restored; coming back clears the reason; an unreachable session is not handed out as .live; and the monitor stops asking after it gives up.

Neutralising the phase arm fails 2 of them. 83 cases pass across the liveness, phase-machine, pane-synchronization, chrome, pane-resolver, workspace-registry, tunnel and multi-connection suites. macOS Debug build passes; SwiftLint reports 0 violations on the changed files.

No TableProUITests coverage: reproducing it needs a server taken down mid-session and a 30-second backoff with no launch-environment override, so it is not deterministic. That is why the threshold and the give-up are driven directly instead.

Review

A Codex review of the branch found three issues, all fixed here:

  • ensureConnected was made liveness-aware but connectToSession kept its own driver != nil guard, so Reconnect still no-opped. Both are fixed, and a test now asserts an unreachable session is not resolved as live.
  • adoptWorkspace still picked its phase straight from driver != nil, so a connection adopted after the monitor had given up would mount content over the dead handle with no further event coming to correct it. It resolves through the same phase machine now.
  • MCP read raw status and connectionState classified any installed driver as live, so an external client could keep routing work to the closed socket. Both go through liveness.

Not done

An indefinite non-auth outage still retries forever rather than ever reaching a terminal state; performHealthMonitorReconnect returns .retry for everything that is not an authentication failure. The window is honest about it throughout now, which is the defect this fixes, but whether a dead server should eventually stop being retried is a separate product decision.

Before / After

No screenshots. The state needs a server taken down under a live session and held down past the backoff, and the claim is that a window stops asserting a connection it no longer has; a still frame of a grid does not show which of the two channels is lying. The regression suite is the evidence instead.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit ed65eed into main Aug 27, 2026
8 checks passed
@datlechin
datlechin deleted the fix/connection-liveness branch August 27, 2026 07:11
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.

1 participant