perf(mobile): speed up multi-environment reconnect on foreground - #4885
perf(mobile): speed up multi-environment reconnect on foreground#4885t3dotgg wants to merge 1 commit into
Conversation
Three serialization points made reconnecting to several environments feel sequential even though the supervisor fan-out is parallel: - The foreground liveness probe waited 15s on half-open sockets before starting a reconnect. It now times out after 5s. - Remote DPoP tokens lived inside the single connection-catalog SecureStore document, so every environment's token read/write serialized on one semaphore and rewrote the whole catalog to the Keychain. Tokens now live under per-environment keys with an in-memory cache and lazy migration. - The relay access-token cache held its lock across the network token exchange, so cache hits queued behind a stale account's round trip. Cache hits now take a lock-free fast path. Also reset the retry backoff ladder on application-active, since network conditions usually changed while backgrounded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
| ); | ||
| if (decoded === null || decoded.environmentId !== environmentId) { | ||
| return Option.none<TokenStore.RemoteDpopAccessToken>(); | ||
| } |
There was a problem hiding this comment.
🟡 Medium connection/token-store.ts:142
When the standalone SecureStore item is corrupt, getLocked deletes it and returns Option.none() without checking the catalog document for a legacy token. If a prior migration wrote the standalone key but its catalog cleanup failed (an error that is explicitly swallowed), the valid legacy copy remains in the catalog but is never tried — get reports no token and all subsequent reads do the same, permanently losing access to the token. Consider falling through to migrateLegacyToken after deleting the corrupt item so the catalog copy can recover the token.
Effect.catch((error) =>
Effect.logWarning("Discarding corrupt persisted environment access token", error).pipe(
Effect.andThen(
storage
.removeItem(tokenKey(environmentId))
.pipe(Effect.mapError((cause) => tokenError("delete", cause))),
),
Effect.as(undefined),
),
),
);
- if (decoded === null || decoded.environmentId !== environmentId) {
+ if (decoded === undefined || decoded.environmentId !== environmentId) {
return yield* migrateLegacyToken(environmentId);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/connection/token-store.ts around lines 142-145:
When the standalone SecureStore item is corrupt, `getLocked` deletes it and returns `Option.none()` without checking the catalog document for a legacy token. If a prior migration wrote the standalone key but its catalog cleanup failed (an error that is explicitly swallowed), the valid legacy copy remains in the catalog but is never tried — `get` reports no token and all subsequent reads do the same, permanently losing access to the token. Consider falling through to `migrateLegacyToken` after deleting the corrupt item so the catalog copy can recover the token.
There was a problem hiding this comment.
One convention finding in the new mobile token store: the error wrapper embeds a stringified cause in detail. Everything else (namespace imports, Effect.fn construction, dependency acquisition via MobileSecureStorage, focused behavior tests for the supervisor/relay/token changes) looks consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
| function tokenError(operation: string, cause: unknown) { | ||
| return new ConnectionTransientError({ | ||
| reason: "remote-unavailable", | ||
| detail: `Could not ${operation} the environment access token: ${String(cause)}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
detail stringifies an arbitrary cause, and it is used for encode/decode of the DPoP token, so schema issue text containing token material can end up in a caller-visible error message. Consider keeping detail bounded to the stable structural context (operation) and surfacing the underlying failure through the log/annotation at the mapping sites instead of copying it into detail.
| function tokenError(operation: string, cause: unknown) { | |
| return new ConnectionTransientError({ | |
| reason: "remote-unavailable", | |
| detail: `Could not ${operation} the environment access token: ${String(cause)}`, | |
| }); | |
| } | |
| function tokenError(operation: string, cause: unknown) { | |
| return new ConnectionTransientError({ | |
| reason: "remote-unavailable", | |
| detail: `Could not ${operation} the environment access token`, | |
| }); | |
| } |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8fa1088. Configure here.
| ), | ||
| ); | ||
| if (decoded === null || decoded.environmentId !== environmentId) { | ||
| return Option.none<TokenStore.RemoteDpopAccessToken>(); |
There was a problem hiding this comment.
Corrupt token delete fails get
Medium Severity
When a persisted DPoP token fails to decode, get maps SecureStore removeItem failures into a ConnectionTransientError instead of treating the entry as missing. That makes authorization fail for the environment whenever Keychain delete is broken, which conflicts with discarding corrupt tokens so the connection can continue. Nearby mobile cache loading swallows delete failures in the same situation.
Reviewed by Cursor Bugbot for commit 8fa1088. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new token storage architecture, changes mobile reconnection behavior, and adds concurrency optimizations. Multiple unresolved review comments identify potential bugs in error handling paths. The scope of runtime behavior changes and unresolved issues warrant human review. You can customize Macroscope's approvability policy. Learn more. |


Problem
Reconnecting to multiple environments on mobile after foregrounding is slow and feels sequential, even though the supervisor fan-out is fully parallel. Three serialization points and one oversized timeout were responsible:
SynchronizedReflock across the network token exchange, so every environment's cache hit queued behind one round trip whenever the token was stale.Fix
All behavior is pinned by tests (probe timeout bound, ladder reset, non-blocking cache hit, token migration, and the get/remove serialization race), each verified to fail against the unfixed code. Docs updated in
docs/architecture/connection-runtime.md.🤖 Generated with Claude Code (Claude Fable 5)
Note
Medium Risk
Changes mobile token persistence, reconnect timing, and relay token caching on auth-critical paths, but behavior is heavily tested and includes lazy migration and ordered removal to limit orphaned credentials.
Overview
Improves parallel mobile reconnect after foregrounding by removing three serialization bottlenecks and tightening liveness checks.
Mobile persistence: Remote DPoP access tokens are no longer stored in the monolithic connection catalog SecureStore document. Each environment uses its own SecureStore key (base64url-encoded id), a write-through in-memory cache, and per-environment locks. Legacy tokens in the catalog migrate on first read; environment removal deletes the standalone token key before the catalog entry.
Connection supervisor: Foreground liveness probe timeout drops from 15s to 5s. When the app returns to the foreground during backoff, the retry ladder resets (documented in
connection-runtime.md). Tests cover ladder reset and stalled probe behavior.Managed relay: Relay DPoP token cache hits read outside the
SynchronizedRefcritical section so concurrent environment authorizations do not block on an in-flight token exchange; misses still coalesce inside the lock.Reviewed by Cursor Bugbot for commit 8fa1088. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Speed up multi-environment reconnect on app foreground by resetting backoff and reducing probe timeout
EnvironmentSupervisornow resets the retry ladder and interrupts the current backoff delay when anapplication-activewakeup is received, so the next attempt after foregrounding retries as attempt one rather than continuing an exponential backoff.CONNECTION_PROBE_TIMEOUTis reduced from 15 seconds to 5 seconds insupervisor.ts.SecureStorekeys intoken-store.ts, with an in-memory write-through cache and per-environment operation serialization.ManagedRelayClientnow use a lock-free fast path, reducing contention on cache hits.📊 Macroscope summarized 8fa1088. 6 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.