fix(ios): mark a memory-stopped query before cancelling it and hold the activity through its end - #2663
Merged
Conversation
…he activity through its end Claude-Session: https://claude.ai/code/session_01DeXDeLTqEXdRgsPkvPkdPP
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.
Follow-up to #2661. An adversarial review of that branch returned a no-ship verdict after it had already merged; these are the findings it raised. One of them means #2661 only half delivered a fix it claimed.
A memory-stopped query was still recorded as successful
handlePressurecancelled the fetch task and then returned early when the row buffer was empty, before marking the truncation:Cancelling a continuation-based
AsyncThrowingStreamresumes the iterator withnilrather than throwing, sorunexits its loop normally andresolvePhase()promotes a still-.runningphase to.finished. The query then ends its Live Activity as completed, fires the success haptic, and writeswasSuccessful: true.A long aggregate or a write commonly streams no rows at all, so the empty-buffer case is the common one, not the edge case. This is the same shape #2661 fixed for Stop, left in the sibling path: that fix pre-marks
.cancelledbefore cancelling for exactly this reason. Memory pressure now does the same.A second run could overlap the first run's terminal writes
executeQueryDirectsetisExecuting = falsebefore awaiting the Live Activity teardown, so Run was enabled while the first execution still had error, history and haptic writes pending. Query A could fail, query B start and clearappError, and A then resume and install its stale error over B's result.resultSectionprioritisesappError, so B's successful result stayed hidden.All durable and UI writes now happen before the execution slot is released. ActivityKit teardown is last, which also means query history is no longer written behind an
awaitthat a suspension could cut short.An activity lost ownership before its end completed
end(token:)removed the execution from the map and then awaitedhandle.end. Main-actor isolation does not span that suspension, so a concurrentreapOrphans()could see the activity as unowned and issue a competing.interruptedend against the intended outcome. Executions now carry anisEndingstate and stay inownedActivityIdsuntil the terminal call returns; progress updates are rejected once ending starts.Whether ActivityKit still enumerates an activity during its asynchronous end is not documented, so this is a guard against an inferred race rather than a measured one. It costs one flag and makes the controller's ownership invariant actually true.
Query history accessibility
The history row's icon hardcoded an accessibility label of "Failed" for anything unsuccessful, so VoiceOver announced a stopped query as failed. It now reads the item's own message, "Stopped before the query finished."
Not in this PR
The review also argued that collapsing
.failed,.stoppedand.interruptedintowasSuccessful == falseis wrong, and it has a point worth recording: for MySQL and Redis, whosecancelCurrentQuery()are no-ops, a stopped write may still have committed server-side, so presenting it as a definite failure invites a duplicate retry. Deduplication inQueryHistoryStoragealso keys on that Boolean, so a stopped and a failed attempt of the same query collapse into one.Fixing it properly means a persisted outcome enum on
QueryHistoryItemwith backward-compatible decoding, distinct UI states, and new dedup. That is a query-history feature change rather than part of this defect, so it is left out deliberately.Verification
QueryActivityControllerTests,QueryActivityContentStateDecodingTests,QueryExecutionOutcomeTests,QueryEditorViewModelTests,BackgroundReleaseCoordinatorTests,DataBrowserViewModelTests.--strictclean on every changed file.TableProMobilesits outside.swiftlint.yml'sincluded:, so the paths were passed explicitly.No UI automation: the Lock Screen and Dynamic Island render outside the app process, so XCUITest cannot observe a Live Activity. The controller is covered through its injected store instead.
Building locally needs a workaround unrelated to this change:
oracle-nio's single@TaskLocalfails to expand under the Xcode 27 beta. CI pins Xcode 26.4.1, where it compiles. The workaround was applied to the DerivedData checkout only and reverted.https://claude.ai/code/session_01DeXDeLTqEXdRgsPkvPkdPP