Skip to content

fix(ios): mark a memory-stopped query before cancelling it and hold the activity through its end - #2663

Merged
datlechin merged 1 commit into
mainfrom
fix/ios-query-stop-and-activity-ownership
Sep 7, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/ios-query-stop-and-activity-ownership

Conversation

@datlechin

Copy link
Copy Markdown
Member

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

handlePressure cancelled the fetch task and then returned early when the row buffer was empty, before marking the truncation:

self.fetchTask?.cancel()
guard !self.buffer.isEmpty else { return }   // skips the mark
self.buffer.markTruncated(.memoryPressure)

Cancelling a continuation-based AsyncThrowingStream resumes the iterator with nil rather than throwing, so run exits its loop normally and resolvePhase() promotes a still-.running phase to .finished. The query then ends its Live Activity as completed, fires the success haptic, and writes wasSuccessful: 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 .cancelled before cancelling for exactly this reason. Memory pressure now does the same.

A second run could overlap the first run's terminal writes

executeQueryDirect set isExecuting = false before 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 clear appError, and A then resume and install its stale error over B's result. resultSection prioritises appError, 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 await that a suspension could cut short.

An activity lost ownership before its end completed

end(token:) removed the execution from the map and then awaited handle.end. Main-actor isolation does not span that suspension, so a concurrent reapOrphans() could see the activity as unowned and issue a competing .interrupted end against the intended outcome. Executions now carry an isEnding state and stay in ownedActivityIds until 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, .stopped and .interrupted into wasSuccessful == false is wrong, and it has a point worth recording: for MySQL and Redis, whose cancelCurrentQuery() are no-ops, a stopped write may still have committed server-side, so presenting it as a definite failure invites a duplicate retry. Deduplication in QueryHistoryStorage also 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 QueryHistoryItem with 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

  • iOS build and QueryActivityControllerTests, QueryActivityContentStateDecodingTests, QueryExecutionOutcomeTests, QueryEditorViewModelTests, BackgroundReleaseCoordinatorTests, DataBrowserViewModelTests.
  • Two new regression tests: memory pressure applied before the first stream element, and a reap racing an in-flight end.
  • SwiftLint --strict clean on every changed file. TableProMobile sits outside .swiftlint.yml's included:, 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 @TaskLocal fails 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

@datlechin
datlechin merged commit d09255b into main Sep 7, 2026
10 checks passed
@datlechin
datlechin deleted the fix/ios-query-stop-and-activity-ownership branch September 7, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant