fix(coordinator): carry a cancelled navigation into the schema column fetch it started - #2063
Merged
Merged
Conversation
… fetch it started
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Aug 10, 2026
Merged
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.
Closes #2058.
Clicking through the sidebar quickly burned 75-90ms per navigation on schema-column loading that was thrown away. From the trace in the issue, ten consecutive clicks each reached
schemaColumnsEndwell after being superseded, and only the eleventh displayed anything.Why cancelling did nothing
cancelTableLoadcancels the wrapper task, andprepareTableTabFirstLoadchecksTask.isCancelledafterwards, so the result was correctly discarded. The work still ran to completion, and the reason is one line inSchemaColumnStore:That inner task is unstructured, so it inherits nothing from whoever awaits it. Cancelling the caller could not reach the fetch at all. Every
Task.checkCancellation()further down, inMetadataConnectionPool.runSeriallyandwithPinnedSessionDriver, was checking a task that was never cancelled.This is not only wasted server work. Both of those paths are serial, so an abandoned fetch holds the metadata connection and every table clicked after it queues behind it. That is what makes ten fast clicks feel progressively slower rather than merely wasteful.
What this changes
SchemaColumnStore.loadnow wraps its wait inwithTaskCancellationHandlerand carries the caller's cancellation into the shared fetch.The fetch is shared by every caller asking for the same key, so it cannot simply be cancelled when one of them leaves. The store counts live waiters and only cancels once the last has withdrawn. Two tabs opening the same table still share one fetch, and clicking past one of them does not take the result away from the other.
A cancelled load is also never joined. Between the last waiter leaving and its
loadclearing the entry there is a window where the task is cancelled but still in the map; a caller adopting it would wait on a fetch that will never produce anything. Clicking away from a table and straight back to it lands exactly there.Two related fixes in the same area:
loadSchemaColumnslogged everycatchat error level, so normal navigation would have filled the log with errors once cancellation started working.rebuildSelectedTableColumnScopedQuerycaptured a tab index before its await and rebuilt at that index afterwards, with only a bounds check. After a tab close or reorder that index is a different tab. It now re-resolves by tab id and re-checks the table name, matching the guardprepareTableTabFirstLoadalready used.Tests
SchemaColumnStoreCancellationTestscovers the sole waiter cancelling the fetch, a second waiter keeping it alive, a caller arriving after a cancel starting fresh, coalescing, the cached path,removeAll, and a failed fetch leaving the next caller free to retry. The store takes its fetch as a closure, so all of it runs without a database.swiftlint lint --strictreports 0 violations in 1299 files.Not fixed here
Cancellation is cooperative, so a fetch already blocked inside the driver still runs to completion. The win is on queued fetches, which is what the measured case is: clicks 5 through 13 in the issue's trace were all waiting behind the serial gate. See also #2061.