fix(toolbar): derive the window's busy state from the execution registry - #2362
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
|
Reviewed with the context from the driver side of #2342 (#2355 and #2363). The design holds up: the registry already owned the fact, the duplicate could only be lowered behind two ownership checks, and One correction to offer, about scope rather than correctness. The body says the reported state "has exactly one shape: that flag was raised when the query started and nothing lowered it". There is a second shape that produces an identical symptom, and it is measured:
That shape is not fixed by this PR, and this PR is not wrong for it: with your change the indicator correctly reports work that is genuinely in flight. It is fixed by #2363, which swaps the busy timeout for a handler so Stop ends the wait in 0.74s instead of 60.46s. Worth knowing because this PR carries Unrelated and non-blocking: the second CHANGELOG line ("session context buttons no longer empty out") reads like a separate user-visible change from the same refactor, which is fine, but it has no issue reference while the line above it does. |
Found investigating #2342. It does not close it: the reported stall never reproduced here, and #2355 and #2363 fix a second mechanism with the same symptom.
What the report describes
Open a SQLite database, click a table, and the toolbar shows "Executing…" with a live Stop control and never any rows. Press Stop and everything works: the same table loads, and so does every other one.
What is actually stuck
"Executing…" and the ⊗ beside it are
ExecutionIndicatorView, and it renders only when itsisExecutingparameter is true. Its one caller passedConnectionToolbarState.isExecuting.That symptom has two shapes. Either a query really is still running and the indicator is telling the truth, or the flag was raised when the query started and nothing lowered it. This PR is about the second. The first is a wait on a locked database, which #2355 and #2363 cover: before them Stop could not end such a wait at all, and the result rendered as a successful empty table.
That flag was a stored, window-scoped duplicate of a fact
TabExecutionRegistryalready owned. It was raised and lowered by hand at ten call sites, and every lowering ran behind two independent ownership checks:tabExecution.settle(claim)had to return true andcurrentQueryTaskOwner == claim. Any path that ended an execution without satisfying both left the indicator, Stop,Cmd+.and the disconnect warning all describing work that was over, with no route back except Stop.TabExecutionRegistry.swiftalready records the lesson from the previous incarnation of this bug: "busy state is DERIVED from membership here, never stored, because a stored flag is exactly what let a retargeted tab stay busy forever." That refactor applied it to the per-tab flag and stopped at the window. #548 was "always reset isExecuting on query completion to prevent stuck tabs", and the class came back.Paths in the shipping code that end an execution without lowering the flag:
clearAbandonedExecutingFlagIfNeededheals a claim that has no task behind it and never touches the toolbar, so the titlebar keeps reporting a query with nothing left to finish it.PaginationCoordinator.performFetchAllinstalls the window's task handle with anilowner and does not cancel the handle it displaces, so its own completion lowers the flag while another tab's query is still running (verified, reproducible on a cross-database PostgreSQL setup).ToolbarConnectionState.executinghad a second, unrelated producer:mapSessionStatusmapped a connection's.connectingonto the same case, and wroteconnectionStatedirectly, bypassingisExecuting.The fix
The window's "is anything running" state is derived from
TabExecutionRegistry, andConnectionToolbarState.isExecutingandsetExecuting(_:)are gone.claimmints a new content epoch, which is the value the fetch validates against before writing its rows back, so claiming would discard the result it runs to extend.invalidateandinvalidateAllrelease those tokens on the same terms as a claim, and Fetch All brackets itself with adeferso every exit including cancellation releases it.ToolbarConnectionState.executingis removed. The connection's state and query activity are two axes, and sharing one case is what let a dialing connection paint the query indicator. The three copies of the status mapping, two of which dropped the failure's message so the same failed connection compared unequal to itself, collapse into oneToolbarConnectionState(status:).MainContentCommandActions.isQueryExecutingandhasRunningQuery(forConnection:)all readtabExecution.isAnyExecuting.retireQueryTaskstill owns the window's Stop handle and no longer reports anything, so a completion that cannot retire the handle can no longer leave the window claiming to be busy.Two things fall out of removing the shared case.
loadSessionContextsguarded onconnectionState == .connected, which was false for the whole duration of every query, and the toolbar's session-context buttons were keyed on the same state. So they emptied when a query started and refilled when it ended, and the refill cost two round trips on Snowflake, the only driver that answersfetchSessionContexts. They now reload on a connection change only.hasLiveSessionused to be reached with.connectingalready laundered into.executing, so a reconnect counted as live. It still does, explicitly: the health monitor writes.connectingon every retry while the window keeps showing the session's tabs and rows, and graying the toolbar out for the length of a backoff would take Sidebar Toggle with it.ToolbarConnectionState.indicatorColor,description,label,isAnimatingandConnectionToolbarState.statusTooltiphad no readers anywhere in the app or the tests, so they go with the case rather than being edited to survive it.Why the reporter's case looks like this shape rather than the other
Two facts point away from a lock wait, which is the other shape.
An empty table stalls exactly like a large one, which the reporter states explicitly, so there is no work for
sqlite3_stepto be doing.And a lock wait would not have ended when they pressed Stop. Measured against the SQLite the plugin links (3.54.0,
import SQLite3): with one connection holding anEXCLUSIVEwrite lock and a second waiting on a 60ssqlite3_busy_timeout, callingsqlite3_interrupton the waiter did nothing. It ran the full timeout and came backSQLITE_BUSY.Stop is
sqlite3_interrupt. So if the reporter had been waiting on a lock, Stop would have changed nothing and they would have waited out the minute. They got an immediate recovery instead, which is what clearing app-side state looks like. That is an inference from their description, not a reproduction, which is why this does not close the issue.What this does not fix
If the query somehow never returned for a reason neither of those covers, the rows still would not arrive; only the stuck indicator, Stop,
Cmd+.and the disconnect warning are fixed. I have asked on the issue for aTableLoadtrace, which distinguishes the two in one line.This also does not close Fetch All's cancellation gap: it can still displace another execution's Stop handle without cancelling it, so that query becomes uncancellable. Reported separately rather than folded in here.
Why I could not reproduce the stall
I built 0.66.0 and current
mainas Debug apps underTABLEPRO_UI_TEST_SANDBOXand captured theTableLoadtrace against SQLite in six scenarios: a session-restored window with zero tabs, a fresh connection from the welcome list, opening a.sqlitedocument into a running app, cold launching with the document, a 400-table database, and the reused-tab path with a blank SQL editor tab open. Every first click ranorigin=sidebar path=addFirstTab … END outcome=completedin 68 to 140 ms.I also measured, with a compiled probe, that an unstructured
Task {}created inside an already cancelled task is not itself cancelled, which rules outsupersedeExecutioncancelling the query it goes on to start.Tests
TabExecutionRegistryTests: unclaimed work counts as busy without claiming the tab, two tokens on one tab end independently, ending a released token is a no-op, a retarget releases a tab's unclaimed work with its claim, and every way an execution ends leaves the window idle.MainContentCoordinatorLazyLoadTests: healing an abandoned claim leaves the window reporting idle.QueryFailureReportingTests: superseding a tab with no successor leaves the window idle; the existing handle-ownership cases now assert the busy state follows the executions rather than the handle.MainWindowToolbarValidationTests: only a connected session is live, a running query does not change what the connection state says, and a connection error keeps its message through the mapping.WindowBusyStateGuardTests: scansTablePro/and fails the build if a second stored copy of the busy state comes back, or if the indicator stops reading the registry.WindowExecutionIndicatorUITests: opens the sample SQLite database, clicks three tables, and asserts nothing in the toolbar still claims a query is running once each result has landed. A second case runs a recursive CTE slow enough to observe and asserts the indicator and Stop appear while it runs and clear afterwards, so the first case cannot pass against an indicator wired to something that is never true.One limit worth stating:
SampleDatabaseLauncher.openroutes to the Track table, so any suite built onlaunchWithSampleDatabaseis already past the first query, withneedsMetadataFetchfalse. The three-table case therefore runs against a warm connection. It still tests what it claims, and the slow-query case does not depend on warmth, but neither covers the cold first open the reporter is in. fix(plugin-sqlite): report a query that stops early instead of returning the rows it got #2355 adds a suite that does.Verified
generate,build: PASStest: TabExecutionRegistryTests, WindowBusyStateGuardTests, QueryFailureReportingTests, MainContentCoordinatorLazyLoadTests, MainWindowToolbarValidationTestsuitest: WindowExecutionIndicatorUITests (4 cases)lint --strictover the changed pathsNo docs change: no page in
docs/describes the toolbar's execution indicator. No screenshots: the change removes a state that should never have been on screen, so there is no new visual to show.