Skip to content

fix(swift-ios): ignore stale device reads after revocation - #14683

Closed
saphid wants to merge 3 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-device-revocation-20261002
Closed

saphid wants to merge 3 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-device-revocation-20261002

Conversation

@saphid

@saphid saphid commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Replaces #10748. That PR was closed under the verification rule; it cannot be reopened by the author, and its base branch has since been rewritten. This PR carries the same fix, replayed onto the current base, plus the requested race reproduction.

On the SwiftUI Devices screen, a device-list reload that finishes after a revocation overwrites newer state. The removed device comes back, or the reload replaces newer feedback: a successful revocation's cleared error is replaced by a stale reload error, or a failed revocation's error is wiped. A second removal started while the first is still running can clear the first one's error the same way. In every case, an older device-access operation finishes and writes over the result of a newer one.

Fix: one access change runs at a time, and starting it invalidates any reload still in flight, so that reload's late result or error is discarded. Reloads do not start while a change is running, and a reload started afterwards applies normally. The model refuses an overlapping change, and the row remove actions are disabled while one is running. The toolbar's "Remove all other devices" menu was already disabled in that state.

Why this is a small fix for an obvious bug: a revoked device reappearing, and error messages being lost or replaced by stale ones, contradicts what the user just did. The fix only orders operations on this one screen. It changes no contract, server code, default, or other client.

How the parts relate: the state moves from @State properties into a small @MainActor DevicesModel only so a test can drive reload and revocation directly. The view's layout and copy are unchanged. A pre-existing case is not addressed: if "Remove all other devices" fails partway, the list can still show a device that was removed. That is a different failure and needs its own fix.

Verification

Reproduction: the new tests use HeldDeviceManager, a test double that holds loadDeviceSessions() (and, in one test, the first revocation) open with continuations. Each test releases the held calls in the race order:

  • staleReloadCannotRestoreRevokedDevice: with [current, laptop] loaded, hold a reload, revoke laptop, then release the stale [current, laptop]. The list stays ["current"], and loading and revoking clear.
  • staleReloadKeepsNewerRevocationFeedback: a failed revocation's error survives a stale successful reload.
  • staleReloadFailureDoesNotReplaceRevocationSuccess: a stale reload error does not replace a successful revocation.
  • secondRevocationWaitsForFirstAndKeepsItsFeedback: a second removal is not sent while the first is running, and the first one's failure stays visible.
  • currentReloadStillAppliesAfterRevocation: a reload started after the revocation still applies.

Environment: iOS Simulator iPhone 17, iOS 26.5, Xcode 26.6. Command, from apps/swift-ios:

xcodebuild -project T3Code.xcodeproj -scheme T3Code -destination 'platform=iOS Simulator,id=<udid>' CODE_SIGNING_ALLOWED=NO test -only-testing:T3CodeTests/DeviceManagementTests

Results (test output and the overlay diff: https://gist.github.com/saphid/e41be48dd5edd953b80276a74d904fb1):

  • With the fix: 9 passed, 0 failed (exit 0). Tested at ac67be1d01 on the previous base 285d2ab80c. This PR's head carries the same diff, replayed onto the rewritten base cf7902896b. DevicesView.swift, FeatureDeviceManagement.swift, and DeviceManagementTests.swift are byte-identical on both bases, but this head was not run locally. The native CI job on this PR ran the five new device tests on f633e6790f, and they passed. That job is red only because FeaturePastedTextTests.attachmentAvailabilityIncludesPendingItemsAndTheFileCapability and UsagePresentationTests.receivedResultStacksProvidersPerDayAndOrdersRows fail, and both fail the same way on the base itself (base run). The latest commit only makes the test helper's results Sendable, fixing a data-race warning CI reported in the new test file (an error in Swift 6 mode).
  • Without the fix: 4 failed, 5 passed (exit 65). This is not an upstream revision, because the new tests need DevicesModel, which the base does not have. It is a local overlay of the tested revision: the tests and fixtures are unchanged, and DevicesModel's reload, revoke, and revokeOthers are replaced with the base's DevicesView code (verified identical by text diff). The four failures were:
    • the revoked laptop came back (["current", "laptop"])
    • the revocation error became nil
    • a stale error replaced the success
    • the second removal was sent (["laptop", "tablet"])

Not checked:

  • No end-to-end run against a real server; the race is shown with the test double above.
  • No before/after screenshot or recording of the row remove actions while they are disabled. A fair capture needs a server that holds a revocation open long enough to swipe a second row.

Model and harness: Claude Opus 5.5 / Claude Code (re-port, model extraction, tests); original fix GPT-6 / Codex. Independent read-only review: GPT-6-Sol / Codex.

🤖 Generated with Claude Code

saphid and others added 2 commits October 2, 2026 09:39
A device list reload that finishes after a revoke could put the removed
device back on screen. Each reload and revoke now takes a generation, and
only the newest operation may write results or clear its spinner.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move device access state into DevicesModel so the race is testable. The
tests hold a reload open, revoke a device, then release the stale result and
check that the device stays removed and the newer feedback survives.

Only reloads are generation-checked. One access change runs at a time:
removal actions are disabled while access is changing, and the model refuses
an overlapping change, so a second removal cannot clear the first one's error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 1, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This is a focused and well-tested concurrency fix, but it changes the sequencing and feedback around device-session revocation and authenticated access management. The affected operations can unregister devices or revoke client sessions, so the authentication-related impact warrants human review.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

The held-call helper resumed continuations with a non-Sendable result, which
CI reported as a data-race warning and Swift 6 mode rejects.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
t3dotgg pushed a commit that referenced this pull request Oct 2, 2026
Port the focused fix from #14683 onto the current SwiftUI branch.

Source-Commit: 4a09541
Source-Commit: f633e67
Source-Commit: d1c473b

Ported by GPT-6.1-Sol through the Codex harness in T3 Code.
@t3dotgg

t3dotgg commented Oct 2, 2026

Copy link
Copy Markdown
Member

Note

🤖 GPT-6.1-Sol responding on behalf of Theo

Applied the focused fix to t3code/rebuild-mobile-app-swift in fb39fcb6c3. Closing this source PR after the port.

The combined focused native checks passed: 241 tests, one existing skip, no failures.

@t3dotgg t3dotgg closed this Oct 2, 2026
t3dotgg pushed a commit that referenced this pull request Oct 7, 2026
Port the focused fix from #14683 onto the current SwiftUI branch.

Source-Commit: 4a09541
Source-Commit: f633e67
Source-Commit: d1c473b

Ported by GPT-6.1-Sol through the Codex harness in T3 Code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants