fix(coordinator): give each tab ownership of its execution so a retarget cannot be overwritten

[datlechin]

The bug

Clicking table B while table A's query was still running left the tab showing A's rows under B's name, and B never loaded at all. Reproduced live and captured with the new tracing:

#27 analytics_events  +21.9ms   executeStarted generation=19
#27 analytics_events  +114.1ms  ANOMALY supersededByNewNavigation replacedBy=#28

#28 account_tokens    +0.1ms    openTableTab from=analytics_events wasExecuting=true hasInFlightQuery=true
#28 account_tokens    +9.8ms    ANOMALY blockedByInFlightExecution
#28 account_tokens    +9.8ms    END outcome=blocked          <- account_tokens never ran a query

#27 analytics_events  +1164.4ms driverFetchEnd rows=1000 driverMs=1127
#27 analytics_events  +1164.7ms ANOMALY resultTableMismatch resultTable=analytics_events tabNowShows=account_tokens
#27 analytics_events  +1166.0ms gridReloadBegin rows=1000    <- painted anyway

It fired three times in one short browsing session. The tab is retargeted durably, not transiently: applyPhase1Result writes tableContext.tableName back, and Refresh then confirms the wrong table rather than recovering.

Why it needed a refactor rather than a patch

"Which navigation owns this tab's execution" was spread across five uncoordinated mechanisms, and a tab retarget participated in none of them:

Mechanism	Scope	Sites
queryGeneration	per window	5 writes / 16 reads
execution.isExecuting	per tab, stored Bool	19 writes / 15 reads (11 blocking guards)
currentQueryTask	per window	22 writes across 6 files
tableLoadTasks	per tab, wrapper only	never owned the real fetch
ConnectionToolbarState.isExecuting	per window	a third independent flag

35 sites mutate tab state after an await. None validated both "same tab" and "same navigation".

The counter could not represent this bug's shape. It only advanced when a successor execution started, and here the successor was refused by a stale flag, so nothing advanced and the old result stayed valid all the way into the grid.

The change

TabExecutionRegistry, keyed per tab, modelled on the existing ConnectionAttemptRegistry one level down.

• claim(tabId) mints an epoch and invalidates the predecessor.
• invalidate(tabId) removes the entry rather than bumping a counter, so "the user navigated away and no successor ever ran" still invalidates.
• Busy state is derived from membership, never stored, so a retargeted tab cannot stay busy forever.
• QueryTabManager.replaceTabContent fires onTabRetargeted, wired to cancel the driver read and invalidate. The retarget is now an event the model sees.
• The staleness gate moved ahead of every shared-state write in the result handler. A superseded result no longer clears the spinner or nils the task handle belonging to its successor.

Two traps a design bake-off caught

Two independent reviewers each found a real defect in the other's preferred design. Both are avoided here:

1. The apply gate is epoch-only. It deliberately does not compare the tab's live (tableName, databaseName, schemaName). resolveTableTabSchemaIfNeeded rewrites schemaName mid-flight whenever the schema was unresolvable at tab creation, which is the ordinary session-restore path, so a field comparison would discard valid rows on restore.
2. The tableName write is kept for query tabs. resolveTableEditability derives a query tab's tableName from the SQL per result, so that write is load-bearing there and a tautology for table tabs.

Data-loss fix shipped alongside

cancelRunningQuery iterated every running driver and its fallback ignored scoping, so Stop could already KILL QUERY a commit or a DDL statement. Making navigation issue cancels would have turned that from rare into routine. DriverCancellationPolicy (untracked / cancellableRead / protectedWrite) now classifies every lease. The parameter has no default value, so a new call site cannot silently pick wrong.

Behaviour change

The 11 blocking guards are gone: an entry point now supersedes instead of being silently ignored. CHANGELOG updated under Changed and Fixed.

Testing

• TabExecutionRegistryTests (12) and TabRetargetInvalidationTests (10) pin each proven step of the chain, including the no-successor case the old counter could not express.
• DriverCancellationPolicyTests pins that a protected write is tracked but never a cancellation target.
• MainContentCoordinatorRefreshTests converted to the new model. Its in-flight setup was rewritten to take a real claim rather than setting the stale bool; assertions were not weakened.
• 114 tests green locally across the execution, tracing, pagination and tab-manager suites; swiftlint --strict clean across TablePro/.

No deterministic TableProUITests coverage is possible (CLAUDE.md rule 4): reproducing the race needs a query still in flight when a second table is clicked, which needs a live server with a slow query. The regression is pinned by the pure suites plus the retarget tests instead.

Reviewer notes

• Worth exercising Redis, DuckDB, CSV and SQLite: plugins whose driver has no cancelQuery fall back to the no-op default, so supersede there means the first statement still runs to completion holding the connection gate.
• The .inPlace (Redis) navigation branch now invalidates via the retarget hook, but pluginDispatchAsync installs no cancellation handler, so a late SELECT can still leave the connection on a database no tab asked for. Not introduced here, not fixed here.

[chatgpt-codex-connector]

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

[datlechin]

Verified against a live run

Captured a trace from a build of this branch, browsing ~195 tables across two Postgres connections (one over an SSH tunnel, one local), deliberately clicking through the sidebar faster than queries could finish.

The bug is gone.

anomaly	before	after
resultTableMismatch	3	0
blockedByInFlightExecution	3	0

The intended behaviour replaces it:

#71 ads_ads          +59.3ms ANOMALY supersededByNewNavigation replacedBy=#72
#72 ads_asset_links  +0.1ms  openTableTab from=ads_ads hasInFlightQuery=true
#72 ads_asset_links  +31.6ms executeStarted generation=11     <- the clicked table now runs
#71 ads_ads          +75.3ms ANOMALY executionCancelled -> END outcome=cancelled
#72 ads_asset_links  +62.2ms END outcome=completed

One trace (#38 beta_signups) shows a result arriving after its retarget and being dropped by the claim gate before any write. That is precisely the path that used to paint the wrong table's rows.

Two follow-ups this run exposed

Neither blocks the correctness fix, but both are mine and should not be left implicit.

1. Superseding stalls the main thread over SSH

Perfect correlation across the run:

hasInFlightQuery	connection	replaceTabContent
false	either	0.1-0.2ms
true	SSH tunnel	68-157ms
true	local	0.1ms

supersedeExecution calls cancelRunningQuery synchronously, and a PostgreSQL cancel opens a second connection to deliver the request (Client connected, relaying to localhost:5432 appears at exactly that point). Through a tunnel that costs 70-160ms of main-thread time on every fast click. Fix is to move driver.cancelQuery() off the main actor; the claim gate already guarantees correctness, so the cancel is only resource hygiene and does not need to be synchronous.

2. execution.isExecuting is now vestigial

executeQueryInternal no longer sets it true, but the field still exists and MainEditorContentView still reads it in three places to gate which result view renders. Visible in the trace as wasExecuting=false on every line, including where hasInFlightQuery=true.

Migrating those reads to the registry needs the registry to be observable first: tabExecution is @ObservationIgnored, so SwiftUI cannot re-render off it. That is the part of the original design (TabExecutionController as @Observable) that this change deliberately reduced to a plain registry. Worth completing before the field is deleted.

[datlechin]

Both follow-ups are now in this PR

No separate PR. 7ce88c116 closes the two gaps the live run exposed.

1. Superseding no longer blocks the main thread

cancelRunningQuery now splits target resolution from delivery. Stop stays synchronous, because the user is waiting on it. A navigation supersede hands the blocking cancelQuery() calls to a utility queue:

guard reach == .userStop else {
    DispatchQueue.global(qos: .utility).async {
        for driver in targets { try? driver.cancelQuery() }
    }
    return
}

This is safe precisely because of the claim gate: correctness never depended on the cancel landing before the successor starts, since the superseded claim already discards whatever comes back. The cancel exists only to stop the server doing unwanted work. That removes the 68-157ms hitch measured on every fast click over an SSH tunnel.

2. execution.isExecuting is deleted, not just unused

The field is gone from TabExecutionState entirely, with a comment recording why a stored flag cannot work here. Everything that read or wrote it now goes through the registry:

• 19 vestigial writes removed across Helpers, MultiStatement, Parameters, ClickHouse, QueryHelpers and the coordinator.
• The three MainEditorContentView gates read coordinator.tabExecution.isExecuting(tab.id).
• clearAbandonedExecutingFlagIfNeeded and the cancelCurrentQuery sweep operate on claims.
• Failure paths (finishFailedQuery, resetExecutionState, the ensureConnected bail) settle or invalidate the claim, so a tab is never left busy after an error.

tabExecution lost its @ObservationIgnored, which is what makes the derived state usable from SwiftUI at all. It is a value type, so every claim, settle and invalidate is a write to that property and invalidates its readers. That was the piece of the original design this change had reduced away, and it is back.

Verification

131 tests green across the execution, tracing, refresh, lazy-load, pagination and tab-manager suites. swiftlint --strict clean across TablePro/. CHANGELOG gained a line for the stutter fix, since it is user-visible.

A grep for execution.isExecuting across TablePro/ and TableProTests/ now returns nothing.

[datlechin]

Cleanup pass

f12aaf9fb. Net -79 lines. Driven by an actual usage census rather than taste, so everything removed had zero production readers.

Speculative API removed

symbol	production readers
TabExecutionPhase	0
advance(_:to:)	0 (2 write sites, nothing read them)
phase(for:)	0
executingTabIds	0
forget(_:)	0

The phase concept was the clearest offender: I maintained .preparing / .executing / .applying on every claim and nothing ever asked what phase a tab was in. Entry is now just an epoch. forget(_:) went too rather than being wired up: the registry lives and dies with its window's coordinator, and a window hosts one or two tab ids in its lifetime, so there is nothing to reclaim.

Trace noise removed

• loadAlreadyInFlight fired twice on every window open. That is onAppear and .task both calling lazyLoadCurrentTabIfNeeded, with the dedup guard doing its job. Reporting correct behaviour as an anomaly is worse than not reporting it. It now only fires when a navigation minted its own trace and was then refused, which is the case actually worth seeing.
• [switch] coordinator.lazyLoadCurrentTabIfNeeded executing tabId=… duplicated the lazyLoadScheduled stage emitted two lines later. Gone.
• preparationAbandoned claimed stillSelected=true, which is not why it bailed. The reason is Task.isCancelled from the supersede, so that is all it says now.

Stale naming from the refactor

staleGenerationDropped and traceExecutionStarted(generation:) were named after the per-window counter that no longer exists. Now staleResultDropped and epoch:.

One thing deleted, then put back

traceExecutionBlocked went dead when the navigation guards were removed, so the census flagged it. But seven guards still return silently, and two of them (runQuery, runExplainQuery) are things the user triggers directly with Cmd+R. Hitting Run while a query is in flight silently does nothing. Rather than delete a working diagnostic, I wired it to those two. The five internal ones are left alone; they are correct refusals on paths a user cannot reach directly.

Verification

127 tests green, swiftlint --strict clean, build clean. Tests for the deleted API were removed rather than left asserting on nothing.

[datlechin]

Follow-ups filed

• Schema column loading is not cancelled when a navigation is superseded #2058 — schema column loading is not cancelled when a navigation is superseded (75-90ms wasted per fast click, pre-existing, measured)
• currentRowCountTask is never cancelled on teardown and is overwritten without cancelling #2059 — currentRowCountTask never cancelled on teardown and overwritten without cancelling (verified in tree)
• TabSession mirror is not resynced when a tab is retargeted to another table #2060 — TabSession mirror not resynced on tab retarget (mechanism verified, consequence not traced)
• Verify the superseded-navigation cancel no longer stalls the main thread over SSH #2061 — verify the superseded-navigation cancel no longer stalls the main thread over SSH (this PR's fix is unproven at runtime)

#2061 is the one to look at before merging. It is not a defect report, it is the honest state of this PR: the stall fix is sound in reasoning and green in tests, but the capture taken afterwards never hit the path, so nobody has seen it work.