test(coordinator): pin the superseded-navigation cancel off the main thread - #2065
Merged
Conversation
|
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.
Refs #2061.
#2055 moved the driver cancel for a superseded navigation off the main thread. The reasoning was sound and the unit tests were green, but nothing had ever observed the new code path run: the capture taken afterwards contained no
hasInFlightQuery=true, so every supersede in it happened before a query existed to cancel.This adds the automated guard. No production code changes.
What it pins
ScopedDriverCancellationTestsseedsDatabaseManager.runningDriverswith a fake driver that records which threadcancelQuery()ran on, then callscancelRunningQuery(for:reach:)directly:.supersededNavigationcancel never runs on the main thread. This is the property the stall depended on. A PostgreSQL cancel opens a second connection to deliver the request, which through an SSH tunnel measured 68-157ms, and it used to be paid inline on the main actor..protectedWritelease is never aborted, by either reach. The existing test for this only compared enum cases; this drives the real function.The path needed no new seam.
cancelQuery()is anonisolatedprotocol requirement,runningDriversandcancelRunningQueryare alreadyinternal, and the fake-driver pattern is used in 15 or so suites already. The fake is local to the file rather than reusing the sharedMockDatabaseDriver, whose call counter is an unsynchronisedvarthat would be a genuine data race now that the cancel runs off the main actor.What it does not prove
It proves the dispatch, not the end-to-end number. The test says
cancelQuery()runs on a background thread andcancelRunningQuerydoes not wait for it, which means the main thread cannot block on that round trip. It does not say thereplaceTabContentgap over a real SSH tunnel is now single-digit milliseconds, because other work on that path could be slow for unrelated reasons. #2058 is one such thing.So I would keep #2061 open until someone runs the capture in it. This changes it from unverified to mechanically guarded, which is not the same as observed.
Verification
7 tests pass.
swiftlint lint --strictreports 0 violations. No CHANGELOG entry: tests only, no user-visible change.