fix(connections): stop showing a live window over a driver that has stopped answering - #2554
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
ConnectionSessionhas two liveness signals and neither answers the question.driveris 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 withsession.driver?.disconnect()on a struct-copy parameter, andreconnectSession(:267) repeats it, soactiveSessions[id].driverkeeps pointing at the object they just closed.ConnectionWindowPhaseMachine.onSessionChangedreturned.connectedonhasDriverbefore asking anything else, so the pane stayed on content.statuscannot stand in for it. It is.connectingfor the whole of an ordinary database switch on PostgreSQL, Redshift, CockroachDB and PGlite, which all reconnect to perform one, and.disconnectedis 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.
reconnectDriverruns 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.livenessis one explicit answer to "can this driver be believed":live,recovering,unreachable(info).onSessionChangedreads it beforehasDriver.recoveringis 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 becomesunreachableat 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'sexists && !hasDriverarm into the permanent fake "Preparing the session" thatfailTunnelRecovery'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 callsmarkSessionLive, so the mark and the reason it carried go together.A workspace that owns the attempt reports
.connectingrather than.unavailable, so pressing Reconnect dials instead of flipping back to the error, and a stale report cannot drag it back.ConnectionSession.reportedStatusis what the connections strip, the toolbar and the MCP status tool now read, so the two channels finally agree.Two more things this closes:
ensureConnectedandconnectToSessionboth returned the moment they saw an installed driver, on exactly the connection that needed replacing. Both are liveness-aware now..abortbare-returned with the state latched at.reconnecting(n), soperformHealthCheck'sstate == .healthyguard failed every 30 seconds for the life of the app.HealthStategains.abortedand the loop exits, rather than a flag beside the state that could disagree with it.Tests
ConnectionLivenessTestsis 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
TableProUITestscoverage: 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:
ensureConnectedwas made liveness-aware butconnectToSessionkept its owndriver != nilguard, so Reconnect still no-opped. Both are fixed, and a test now asserts an unreachable session is not resolved as live.adoptWorkspacestill picked its phase straight fromdriver != 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.statusandconnectionStateclassified 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;
performHealthMonitorReconnectreturns.retryfor 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.