Skip to content

fix(ios): fall back to memory when the simulator support directory is missing - #16032

Merged
teamleaderleo merged 2 commits into
manaflow-ai:mainfrom
teamleaderleo:fix/ios-simulator-store-fallback-memory
Sep 30, 2026
Merged

teamleaderleo merged 2 commits into
manaflow-ai:mainfrom
teamleaderleo:fix/ios-simulator-store-fallback-memory

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

MobileAuthComposition.tokenStore has two ways to fail to pick a store. #14302 fixed one
of them: a missing app identity now selects .memory rather than .none, because
StackClientApp maps .none to a NullTokenStore and then calls fatalError on the
next authenticated operation:

if !hasDefaultTokenStore {
    fatalError("This StackClientApp was created with tokenStore: .none. ...")
}

The other return kept the trap. In a simulator build, when the application-support
directory does not resolve, the function still returned .none with a perfectly valid
app namespace. Surfaced while reviewing #16024.

Change

Return .memory there too. Losing the session on relaunch is recoverable; trapping the
process is not. No return .none remains in tokenStore.

The directory lookup is now an injected parameter defaulting to the previous expression,
so the failure path is reachable from a test. The one production call site omits the
argument and gets the default.

One consequence worth stating plainly: the lookup used to sit inside
#if DEBUG && targetEnvironment(simulator) and was absent from the device binary. A
default argument is emitted at the call site instead, so a Release device build now
evaluates one NSSearchPathForDirectoriesInDomains call at launch that it previously did
not. The #else keychain branch still ignores the value.

Verification

Two commits, so the split is visible in the Commits tab:

  1. 005e06b501a adds unresolvableSupportDirectoryFallsBackToMemory() and the injection
    point, with the source still returning .none.
  2. 49e063197ab returns .memory.

Both were pushed together, so pull-request CI only checked the head and the first commit
carries zero check runs. The red half was therefore established by a separate dispatch of
test-ios.yml at 005e06b501a on the fork:

Commit unresolvableSupportDirectoryFallsBackToMemory()
005e06b501a (test only, source returns .none) fails
49e063197ab (source returns .memory) passes on iphone and ipad

The iOS simulator lane routes on this pull request because it touches ios/, and
cmuxFeatureTests is in the default test plan. Both shards are green. The iphone shard
first failed on an unrelated TerminalSurfaceMountOwnershipTests flake in
Packages/iOS/CmuxMobileShellUI and passed on rerun; see the comments below.

Changelog

Fixed: a simulator build whose application-support directory cannot be resolved no longer
crashes on the first authenticated request.

🤖 Generated with Claude Code

teamleaderleo and others added 2 commits September 30, 2026 06:59
`MobileAuthComposition.tokenStore` still returns `.none` when the
application-support directory does not resolve in a simulator build.
`StackClientApp` maps `.none` to a `NullTokenStore`, so the next
authenticated operation calls `fatalError`. That is the same trap
manaflow-ai#14302 removed from the missing-app-identity path, left on the other
return.

Inject the directory so the path is reachable from a test, and assert
`.memory`. This commit is red on purpose; the fix follows.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Return `.memory` instead of `.none`, so a simulator build whose
application-support directory does not resolve loses its session on
relaunch rather than trapping the process on the next authenticated
operation. No `return .none` remains in `tokenStore`.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7ac07982-8c43-4096-a7ce-9523a48d0af7

📥 Commits

Reviewing files that changed from the base of the PR and between aed397a and 49e0631.

📒 Files selected for processing (2)
  • ios/cmuxPackage/Sources/cmuxFeature/MobileAuthComposition.swift
  • ios/cmuxPackage/Tests/cmuxFeatureTests/MobileSimulatorAuthPersistenceTests.swift
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Status on the iOS lane.

Both new assertions pass. ios-simulator (ipad) is green, and on the iphone shard:

✔ Test missingAppIdentityCannotPersistTokens() passed after 5.109 seconds.
✔ Test unresolvableSupportDirectoryFallsBackToMemory() passed after 5.109 seconds.

The iphone shard is red on something unrelated. Its one failure is
TerminalSurfaceMountOwnershipTests.terminalPrimesViewportBeforeClaimingOutputOnEachMount()
in Packages/iOS/CmuxMobileShellUI, which this change does not touch:

✘ recorded an issue at TerminalSurfaceMountOwnershipTests.swift:125:9:
  Expectation failed: (store.terminalViewportGeneration(for: surfaceID) → 2) == 1

The surrounding log shows the grid settling at 66x55 and then a second report at
72x61 (zoom.viewport.retry_budget_new_grid grid=72x61), so a second viewport
generation was issued where the test expects the sticky lease to hold at one. That looks
like a simulator-geometry-dependent race rather than anything in this diff: the same test
passed on the ipad shard here, and it passed on #15909's iphone shard about an hour
earlier, where the only failure was the auth assertion #16024 has since fixed. The shard
recorded only attempt-1, so it did not consume its retry.

I have rerun that job to establish whether it is flaky or newly broken, and will report
the result rather than merge on an unexplained red.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

The red half is now demonstrated, so the two-commit evidence is complete.

test-ios.yml dispatched at 005e06b501a on the fork, the commit that adds the test with
the source still returning .none:

✘ Test unresolvableSupportDirectoryFallsBackToMemory() recorded an issue at
  MobileSimulatorAuthPersistenceTests.swift:80:25: Issue recorded
✘ Test unresolvableSupportDirectoryFallsBackToMemory() failed after 22.786 seconds with 1 issue.

Failing tests:
	MobileSimulatorAuthPersistenceTests.unresolvableSupportDirectoryFallsBackToMemory()

Against the head commit 49e063197ab, the same test passes on both shards. So the test
does catch the bug rather than merely agreeing with the new behavior:

Commit unresolvableSupportDirectoryFallsBackToMemory()
005e06b501a (test only, source returns .none) fails
49e063197ab (source returns .memory) passes on iphone and ipad

missingAppIdentityCannotPersistTokens() passes at both commits, as expected, since
#16024 already aligned it with main.

Two notes on that dispatch run, neither affecting the result above. It also failed
MobileIrohReleaseGateRunnerTests.probeFailurePreservesTheVerifiedIrohRouteAndPath(),
which is unrelated to this change and which I have not investigated, and its
mobile-core-package job was cancelled. The run exists only to establish the red, so I
did not chase either.

Remaining blocker here is the iphone shard's unrelated
TerminalSurfaceMountOwnershipTests failure described above; its rerun is in flight.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Independent review pass before merge. No blocking defect; recording what it checked and the three things it raised that the description did not.

Verified rather than assumed

  • TokenStoreInit.none maps to NullTokenStore with hasDefault = false (vendor/stack-auth-swift-sdk-prerelease/Sources/StackAuth/StackClientApp.swift:68-70) and resolveTokenStore traps at :1158-1166. .memory leaves hasDefault = true and returns nil, so the client falls through to its default store. The premise holds.
  • The new test executes rather than compiling out. The test-file guard at MobileSimulatorAuthPersistenceTests.swift:1 matches the source guard at MobileAuthComposition.swift:430; cmuxFeatureTests is the first target in ios/cmux/cmux.xctestplan; test-ios.yml:884 builds -configuration Debug, :900 selects -testPlan cmux, and :1269 skips only cmuxUITests.
  • The red/green pair is genuine, read from the job logs rather than from this description.
  • No TokenStoreInit producer outside this function returns .none: the only others are Sources/Auth/MacAuthComposition.swift:94 and Sources/Mobile/MobileHostAuthorizationSupport.swift:148, both .custom. No caller passes a .none override.
  • cmuxFeature is not imported from Sources/, CLI/ or TunnelExtension/, so the Swift 6.0 constraint does not reach this code; the diff would satisfy it regardless.
  • All four callers of tokenStore found. One production (MobileAuthComposition.swift:105-110, reached once from ios/cmux/cmuxApp.swift:43), three in tests. Only the new test passes the new argument.

Raised, and not previously disclosed

  1. The default argument changes the device binary. The lookup used to live inside #if DEBUG && targetEnvironment(simulator) and was absent from a Release device build. As a default argument it is emitted at the call site, so Release now evaluates one NSSearchPathForDirectoriesInDomains at launch that it did not before. Negligible in cost, but the description claimed production call sites were unchanged, which was not literally true. The description has been corrected.

  2. .memory diverges from how the repo handles the same condition elsewhere. Both sibling sites fall back to a directory and keep persistence: Packages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Push/PushRegistrationService.swift:99-102 uses ?? FileManager.default.temporaryDirectory, and Sources/Auth/MacAuthComposition.swift:240-243 falls back to Library/Application Support under NSHomeDirectory(). Either would have kept .custom(FileStackTokenStore(...)), preserved the simulator relaunch-persistence the #if block exists to provide, and needed no production parameter. Relatedly, .memory leaves CMUXAuthSessionCache asserting hasTokens (MobileAuthComposition.swift:113-117) for a store that cannot produce them after relaunch, so each relaunch primes "restoring" and then signs out.

    Still strictly better than trapping the process, and the condition is not reachable in the shipping app, so this is a preference rather than a correctness objection. Keeping the proven .memory change rather than reshaping a green fix into a second untested one.

  3. The same hazard survives twice in this package, with a harder failure. ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift:257 and ios/cmuxPackage/Sources/cmuxFeature/MobileIrohV2Configuration.swift:77 both subscript FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask)[0], which traps on index out of range under exactly the condition this change defends against. Out of scope here; opening a follow-up.

TokenStoreRegistry.shared.getMemoryStore(projectId:) being keyed on projectId alone, without the bundle identifier that .custom(FileStackTokenStore) is namespaced by, was checked and is not reachable: one process is one bundle id, the composition is built once, and the test process only reaches it through a nil support directory that never occurs. It is also pre-existing from #14302's appNamespace == nil path.

🤖 Generated with Claude Code

@teamleaderleo
teamleaderleo merged commit 7e39c92 into manaflow-ai:main Sep 30, 2026
80 of 82 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 49e063197a: every check was green at merge (19 verified; 17 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 30, 2026
a302b3a fix(cloud): replay placement only for new daemon tabs and display views (manaflow-ai#16030)
ec42b7e fix(cloud): keep the link client's last stderr lines in its exit error (manaflow-ai#16057)
8793407 Keep Cloud terminal prompts intact when resizing (manaflow-ai#15924)
2dbe472 Bound Iroh release gate phases (manaflow-ai#16084)
4df2a40 fix(agent-chat): keep ACP Stop off live turns and quiet cancelled startups (manaflow-ai#16093)
d916e5c Merge pull request manaflow-ai#16006 from manaflow-ai/feat-dashboard-settings-hub-plans
6844b12 coderouter: no empty state while shared accounts are unreachable
7b51cc9 test: an unreachable shared-account service must not show the empty state
0fcbc54 ci: make E2E rescue and video capture fail soft (manaflow-ai#16027)
56b06d1 dashboard: capitalize remaining labels, buttons, and the LLM/CLI acronyms
b76ad61 billing: show the upgrade welcome only once the plan confirms it
2eb9bee ci: simplify macOS pool picker (manaflow-ai#15988)
fe2dd0e Preserve Cloud chat row measurements when appending turns (manaflow-ai#16011)
5ae227e test: a stale welcome link must not hide the upgrade prompt
7e39c92 fix(ios): fall back to memory when the simulator support directory is missing (manaflow-ai#16032)
87c78fe ci: do not wait on a busy producer root for tests (manaflow-ai#16077)
aae7dae test: keep the hosted client's real exports in the coderouter procedure mock
ef01450 coderouter: name an unreachable shared-account service and log account failures
0183942 Settle the session status when Stop cancels ACP startup (manaflow-ai#16081)
d664799 test: an unreachable shared-account service is its own state
1c2d14c Merge remote-tracking branch 'origin/main' into feat-dashboard-settings-hub-plans
3fc0c8d billing: one price shape on every plan card; clearer Cloud empty text
167d1a3 test: every plan card shows its price in one shape
d3a63a6 settings: list the subnav's teams from the team catalog
258cd09 test: the settings subnav lists teams from the team catalog
74d2419 billing: say a reason all other plans share once, and no price for a granted plan
aa820ae test: a reason all other plan cards share shows once
504df35 billing: report a downgrade's net credit
7c48432 test: a downgrade credit is net of the new plan's remaining time
952940a billing: plan picker with in-app switching, cancel with reasons, and upgrade prompts
4dc8964 test: Plan & billing defaults to the personal plan
9a69fe7 test: plan picker states and the optional cancel reason
0163f67 billing: in-app plan switch, cancel reasons, and checkout returnTo
31048db test: in-app plan change, cancel reasons, and checkout returnTo
596ff2a dashboard: make Settings the hub for billing and teams, title-case the navigation
ff11627 test: settings is the hub for billing and teams, with title-case navigation

# Conflicts:
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci-macos.yml
#	.github/workflows/ci.yml
#	.github/workflows/ios-screenshots.yml
#	.github/workflows/iroh-release-gate.yml
#	.github/workflows/test-e2e.yml
#	.github/workflows/test-ios.yml
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