fix(connections): keep table loads working while a database switch reconnects - #2826
Open
datlechin wants to merge 3 commits into
Open
fix(connections): keep table loads working while a database switch reconnects#2826datlechin wants to merge 3 commits into
datlechin wants to merge 3 commits into
Conversation
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.
Stacked on #2824. Review and merge that first; this branch targets
fix/connections-gate-ownership.Problem
On an engine that reconnects to change database (PostgreSQL, Redshift, CockroachDB), a table tab could fail or come up empty while its connection switched database.
app. A slow load in A holds the driver, the user picksorders, and the switch queues. Paging B decides its route whileappis still the browsed database, so it queues for the session driver behind the switch. The switch moves the browsed database toorders, and when B's turn comes it fails with "This tab is on app. Switch the connection to that database to run it." Running B again works, on a pooled connection.reportsloads through a pooled connection that is still dialing. The user switches tosales, the switch's reconnect closes every pooled connection and cancels every open in progress, libpq's connect throwsCancellationError, and the tab treats that as a user cancel: no rows, no error. Every idle pooled connection to other databases was closed too.Root cause
resolvedBrowseDatabase), and every execution caller makes it before taking the FIFO session driver gate. Inside the gatepincan only refuse. For user SQL that refusal is right, because the session the statement was written against is gone. An app-built table SELECT depends on nothing the session holds, and at that momentexecutionRoutewould answer.pooled.DatabaseTreeMetadataService.handleReconnectand ran for every reconnect. A pooled entry depends only on the session's effective connection plus its own database. A reconnect-based switch over a direct connection changes neither. What does change it is a rebuilt tunnel, which binds a new local port, or a recovery from a connection that stopped answering.Fix
Table reads follow the route they waited through.
DatabaseManager.withTableReadDriver(scope:cancellation:_:), besidewithScopedDriverand modelled onwithMetadataDriver, resolvesexecutionRouteitself..sessionDriverit takes the session driver's turn. After the existingisUsablecheck it asksexecutionRouteagain inside the gate. If the answer is no longer.sessionDriver, it returns without running the body, leaves the gate, and dispatches again:.pooledgoes toMetadataConnectionPool,.unavailablethrows its message.executionRoute, so it readssession.resolvedBrowseDatabaseexactly as the first one did, never the driver's own connection.withScopedDriverandpinare unchanged, so every other caller is still refused. The tracked-cancellation wrapper and the gate turn (verify,isUsable, gate,trackOperation, secondisUsable) are now private helpers both paths share.MainContentCoordinator.withExecutionDriver(scope:isTableTab:_:), used byexecuteQueryInternaland by Fetch All. It keys ontab.tabType == .tableonly, never onQueryClassifier,isAutoLoador the cancellation policy, because an editor SELECT can read a temp table or sit inside the user's open transaction. First load, pagination, filters, sort and refresh all reachexecuteQueryInternalthroughexecuteTableTabQueryDirectly. A driver-built EXPLAIN run from a table tab also goes throughexecuteQueryInternal, so it takes this path too; its SQL is built from the tab's own SELECT.The owner of the transport fences the pool.
handleReconnectno longer touches the pool.MetadataConnectionPoolgainedbeginTransportReplacement(connectionId:)andendTransportReplacement(connectionId:).beginwithdraws pending opens tagged.transportReplaced, closes idle entries, defers in-flight ones and parks new acquires.endresumes them. Replacements nest by count.end, whether the withdrawn open threw (libpq'sCancellationError) or returned without keeping its entry. It checks its own cancellation first, so a stopped load does not reopen. A parked caller that is cancelled leaves at once, through tickets likeSessionDriverGate's.closeAll(connectionId:database:)(rename) andcloseAll(connectionId:)(disconnect) tag.closed: the waiter still fails and nothing reopens. A caller parked for a replacement whose scope the close matches is failed too, so it never wakes to open a connection for a session or a database that has gone.reconnectSessioncallsbeginonly when the connection has an active tunnel kind, or when the session was not.liveas the reconnect started. Its only caller is the reconnect-based database switch, so the liveness check is what tells a switch that recovers a failing session from a plain one; no purpose parameter was added for a single caller.performHealthMonitorReconnectalways calls it, being a recovery.recoverDeadTunnelis left exactly as it was; see "Found in review, not fixed here".endis adeferplaced right after itsbegin, so it runs after the replacement effective connection is installed and on every exit: a thrown error, a cancellation and a declined password prompt.openSessionDriver(for:)that the pool takes as its opener, so a DEBUG pool can stand in a test opener.Tests
Every schedule is ordered with explicit handshakes: a latch the holder opens from inside the gate,
waiterCountand the newtransportWaiterCountpolled with a bound. Nothing counts yields to decide an order. Every injected session setsstatus = .connected.DatabaseSwitchLeaseOrderingTests, PostgreSQL session onappwith a pooledappconnection seeded:tableReadFollowsTheSwitchOntoThePool: hold the gate, queue a table read forapp, move browse toorders, release. The body runs on the pooled driver.otherWorkQueuedThroughASwitchIsStillRefused(guard): the same schedule throughwithScopedDriver(route: executionRoute)still throwsqueryFailednamingapp, and the body never runs. This is what keeps a COMMIT from being re-routed.reroutedTableReadDoesNotHoldTheGate: while the re-routed read blocks on a latch inside the pool,sessionDriverGate.withExclusiveAccessacquires straight away and nothing is queued.tableReadBehindADeadDriverIsRefused(guard): a holder that marks the session.unreachablemakes the read thrownotConnected, and it never reaches the pool.tableReadWithNowhereToGoNamesItsDatabase(guard): a fake reconnect-switch type that cannot pool throws the unavailable message namingapp.MetadataConnectionPoolTransportReplacementTests, on an isolated pool with a recording opener:withdrawnOpenIsRetriedAfterTheReplacement: a lease waiting on a slow open survives begin and end and runs on the second driver.leaseDuringAReplacementWaitsToOpen: afterbegin, a new lease calls no opener untilend, then exactly one.overlappingReplacementsHoldUntilTheLastEnds: twobegins need twoends.cancelledWaiterStopsWaiting: a parked lease that is cancelled throwsCancellationErrorat once and leaves no waiter.closingADatabaseFailsItsWaiter(guard):closeAll(connectionId:database:)withdrawing a pending open fails its waiter, and the opener runs once.closingAConnectionFailsParkedLeases(added in review): a disconnect during a replacement fails the parked lease at once, and it opens nothing.closingADatabaseFailsOnlyItsParkedLeases(added in review): closingshopfails the lease parked forshop, and the one parked forreportsstill runs afterend.SwitchDatabasePooledConnectionTests:switchLeavesOtherDatabasesPooled: a direct session, a pooledreportsconnection, a failingswitchDatabase.pooledDriverCountstays 1,disconnectCallCountstays 0, and no replacement is left open.treeReconnectLeavesThePool:handleReconnectno longer closes an injected entry.recoveringSwitchClosesThePool(guard): the same failing switch on a session that stopped answering still closes the pooled connection, and the fence is released after the failure.healthReconnectClosesThePool(guard):performHealthMonitorReconnectcloses the pooled connection and releases the fence.Negative control. The base branch's versions of the 8 changed production files were swapped in. Only the seams the new tests call were added back, each with the base behaviour:
withTableReadDriverdelegating towithScopedDriver(route: executionRoute(for:)),beginTransportReplacementascloseAll(connectionId:),endTransportReplacementdoing nothing, the waiter count andisReplacingTransportanswering 0 and false, and an opener override. Only the 3 suites with new cases were run: 20 cases, 8 failed, 12 passed. The 8 failures are the 8 non-guard cases above. The 6 guard cases and the 6 existing cases inDatabaseSwitchLeaseOrderingTestspassed. The files were then restored, and each one's hash matched its committed blob.The three cases added in round one were controlled the same way against the first commit: its
MetadataConnectionPool.swiftandDatabaseManager+Tunnel.swiftswapped in, the two suites holding them run. 12 cases, 3 failed, 9 passed, and the 3 failures were exactly those three. Both files were restored and their hashes matched. Round two then cut the tunnel recovery change back, and its case went with it; the two pool cases remain.No UI automation: the change is in driver and pool ordering, which the unit tests control deterministically and a UI test could only reach through timing.
Verification
All through
.claude/skills/fix-issue/scripts/verify.sh.TableProscheme): PASS.DatabaseManagerTunnelTests: PASS, 116 of 116. The same set on b2e1759, before the cut-back removed one case: PASS, 117 of 117.MetadataConnectionPoolTransportReplacementTests,SwitchDatabasePooledConnectionTests,DatabaseSwitchLeaseOrderingTests,MetadataConnectionPoolTests,MetadataConnectionPoolIdleEvictionTests,MetadataConnectionPoolPlanTests,MetadataConnectionPoolLostEntryTests,HealthMonitorReconnectTests,DatabaseManagerTunnelTests,DatabaseManagerSchemaChangeRoutingTests,ScopedDriverRoutingTests,DatabaseManagerDisconnectTests: PASS, 91 of 91.DatabaseSwitchLeaseOrderingTests,SwitchDatabasePooledConnectionTests,ScopedDriverRoutingTests,ScopedDriverPinningTests,MetadataConnectionPoolTests,MetadataConnectionPoolIdleEvictionTests,MetadataConnectionPoolLostEntryTests,MetadataConnectionPoolPlanTests,MetadataConnectionPoolTransportReplacementTests,SwitchDatabaseReconnectFailureTests,DatabaseManagerSchemaChangeRoutingTests,CancelledExecutionOwnershipTests,ConnectionVerificationTests,PluginDriverAdapterLostConnectionTests,HealthMonitorReconnectTests.ScopedDriverCancellationTests,DatabaseManagerDisconnectTests,TriggerInfoMappingTests,StructureTabTriggersTests,TriggerApplyStrategyTests,TriggerEditingBridgeTests,TriggerApplyExecutionTests,SchemaProviderRegistryTests,SchemaRefreshAfterWriteTests,DatabaseTreeCatalogRefreshPlanTests,DatabaseTreeMetadataServiceTests,DatabaseTreeMetadataServiceRefreshTests,DatabaseTreeMetadataServiceRefreshDatabasesTests,DatabaseTreeMetadataServiceRefreshObjectsTests,SchemaRefreshServiceTests,PaginationCoordinatorTests,MainContentCoordinatorLazyLoadTests,StructureEditingSessionTests,TableStructureLoaderScopeTests,ForeignKeyReferenceMenusTests,QueryCompletionProfileRegistryTests,SessionDriverGateTests,RedisDatabaseSelectionGateTests,SwitchDatabaseTests,SwitchSchemaTests,DatabaseManagerDatabaseSwitchTests,DatabaseManagerSessionTests.Review. Codex is out of usage until 2026-09-19, so both rounds used Claude's
code-reviewskill, reading the committed branch diff againstfix/connections-gate-ownership. Every finding was checked against the code.Round one, 4 findings:
recoverDeadTunnelheld the pool across its whole backoff loop, up to 10 attempts with waits reaching 120 seconds, so every pooled read on the connection spun with no error for the whole recovery where it used to fail at once. Real, and introduced here. Fixed in round one by holding the pool around each connect attempt only, with a test. Round two showed that shape leaves the dead port reachable during the backoff, so the change was cut back; see round two..closedwithdrawal fails the parked callers whose scope it matches, as the gate drain does for gate waiters. Tests:closingAConnectionFailsParkedLeases,closingADatabaseFailsOnlyItsParkedLeases. A stale count now lasts at most one reconnect. See below.Round two, reading both commits, 4 findings:
recoverDeadTunnelis byte for byte the base branch's, and its test went with it. See "Found in review, not fixed here".effectiveConnectionthat still names the replaced port, and on the health monitor path that repeats at every retry. Real, and already true on the base branch, where nothing parked them at all; it is the second item under "Found while investigating fix(connections): hold the session driver while a database switch reconnects #2820".reconnectSessionholds the pool across its password prompts. Real. The approved design ends the hold after a declined prompt, so this is that design's cost and was not changed. See below.CancellationError, which a table tab reports as a user cancel. Real, and the same as a pending open withdrawn by a rename on the base branch, so it is folded into that item below. A disconnect failing a parked caller withCancellationErrormatches how the gate drain reports queued work.No third round was run.
Found while investigating #2820
closeReconnectTunnels) but leaveseffectiveConnectionnaming that port, so leases resumed byenddial a port no tunnel owns, which another connection's tunnel could bind.CancellationError, which a table tab reports as a user cancel: blank, no error. A caller parked for a replacement and failed by the rename gets the same error. A typed error for the rename withdrawal would cover both.Found in review, not fixed here
endand can open a backend on the database being renamed, which PostgreSQL then refuses to rename. Nothing fences new acquires during a rename today either.SessionDriverGateowns its turns, would close this.reconnectSessionholds the pool across its password prompts, so on a tunneled connection whose password changed, pooled reads for that connection wait until the sheet is answered.https://claude.ai/code/session_01SNC28GzXZvLwbgAB8G583A