Skip to content

fix(ios): prepare snapshot helpers within a shared readiness budget - #4181

Open
andrewfree wants to merge 11 commits into
callstack:mainfrom
andrewfree:fix/ios-detached-bridge-readiness-upstream
Open

andrewfree wants to merge 11 commits into
callstack:mainfrom
andrewfree:fix/ios-detached-bridge-readiness-upstream

Conversation

@andrewfree

@andrewfree andrewfree commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Cold Simulator AX startup can consume a capture deadline. Preparation and connection share two seconds; detached startup has a ten-second ceiling. Pending startup permits XCTest fallback without retiring the generation. Initialize the native runtime before listener readiness and await owned work on close.

Retain late startup failures for their starting generation without replay to new generations. Fix three fixture issues. 13 files change; startup notes move to apple/snapshot-bridge/README.md. No public API change or linked issue.

Rebased on main 4abff15e, after #4172 and #4217. The readiness fixture now links #4172's SnapshotBridgeSurfaceHosts.m, and adapter helpers moved to adapter.fixtures.ts to keep adapter.test.ts under the line limit. Production diffs are unchanged by the rebase.

Validation

Head bfbead48f2c9b02bcac1cf5d94b8ee6756bc7afb; base 4abff15edaf7a2634f1b2a00edd742a14e808630.

  • pnpm check:affected --run --head HEAD: 41 checks passed, exact commit; 25 lanes skipped, not passes. Broad unit: 17,364 passed, 1 skipped.
  • Focused snapshot-source, detached-attempt and route tests: 245/245. Darwin cache-native-runtime.test.ts (included): 51/51, 0 skipped, including both readiness scenarios.
  • Accepted live evidence predates the fix. Exact cold-AX pending recovery and the wider runtime matrix remain unobserved; XCTest fallback remains a workaround.
  • With a capture budget of 2 s or less, one helper death can surface twice; the route retires the generation on the first. The default is 5 s, and no production caller sets a shorter budget.
  • The Vercel check fails on deployment authorization, unrelated to this change.

AI-assisted under Andrew's direction using OpenAI Codex and Claude.

@vercel

vercel Bot commented Nov 3, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the Callstack Team on Vercel.

A member of the Team first needs to authorize it.

@thymikee

thymikee commented Nov 3, 2026

Copy link
Copy Markdown
Member

Thanks for the change. The code at 34236db looks sound, but it is device-facing and has no live run yet, so it is not ready to merge.

This change reorders native helper startup (runtime init before bind) and moves helper creation out of the capture into a detached 10 s attempt (lifecycle.ts#L231). The PR body says live qualification on the current head is pending, and unit fakes cannot show the main risk. If the cold AX cost sits in the app's first snapshotForProcess and not in sharedRuntime, the fix only moves the timeout. The first bridge capture would still burn its 5 s deadline and retire the generation, and earlier captures would pay the 2 s wait and fall back to XCTest. Please run on the PR head with a just-booted iOS Simulator (shutdown, boot, launch an app) and a prebuilt bridge binary. The run should show: (1) the first snapshot falls back to XCTest with warning code bridge-readiness-pending, and its total latency stays within the capture budget; (2) within 10 s a later snapshot is served by simulator-ax-bridge, with the same helper pid and inside its 5 s deadline; (3) the generation is not disabled, so the snapshot after that is also bridge-served; (4) closing the session or daemon while startup is pending leaves no SnapshotBridge process in the simulator and no socket file. Please also run cache-native-runtime.test.ts on darwin and report the native readiness pass.

Not blocking, and fine to take or leave: (a) with retryAfterMs: 60000 (lifecycle.ts#L91) a failed startup is replayed to every capture for 60 s, so after an app relaunch the new generation inherits the stored failure and is disabled although no helper ever ran for it; the rule should be that a startup failure retires only the generation whose capture saw the attempt fail, so either omit retryAfterMs or classify a replayed stored failure as capture-scoped; (b) the 2 s readiness budget is written as separate literals in adapter.ts:67, lifecycle.ts:88, lifecycle.ts:228 and preparation.ts, retryAfterMs duplicates the exported SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS, and the 10000 ceiling appears twice, so one readiness-allowance constant, one startup-ceiling constant and a required readinessDeadline on SnapshotBridgeRequest (dropping the manager fallback) would leave the adapter as the only constructor; (c) lifecycle.test.ts builds readinessDeadline by hand, so passing deadline instead of readinessDeadline in adapter.ts still passes, and a test through createSimulatorSnapshotSource with about 1.5 s of preparation plus about 1 s of connect that returns bridge-readiness-pending at about 2 s would cover it; (d) the running Set in preparation.ts#L71 re-tracks what DetachedAttempts already holds, so making DetachedAttempts.close() abort and then await each attempt's settled would remove it; (e) sharedRuntime memoizes with dispatch_once and serve() now exits before bind, so the per-request unsupported runtime-unavailable branch in SnapshotBridge.m:95 can no longer fire and a runtime failure surfaces only as bridge-exited, so please delete it or keep the typed code, and coordinate with #4172; (f) the new snapshot-source/README.md restates the code and is the only README under a packages/*/src directory, so deleting it or moving the two native-ordering sentences into apple/snapshot-bridge/README.md would fit better.

The only failing check is Vercel, which needs a team member to authorize the fork deploy. The diff touches no website or docs-site files, so this looks unrelated. No GitHub Actions job ran on this fork head, so there is no unit or gate evidence yet. Before merge, the live cold-Simulator run above is needed, followed by a full check:affected pass, and the PR is still a draft.

@andrewfree

Copy link
Copy Markdown
Contributor Author

Initial simulator evidence for 34236dba9f1f119ce5375cfac7ef4d3d63eaab80, built and run through the source CLI with a fresh private daemon/state directory. Device: M5 arm64, iOS 27.0 simulator, Xcode 27.2 (27B52). This is separate from the installed v0.21.31 fleet tooling.

Commands used node bin/agent-device.mjs, --platform ios, the assigned simulator UDID, session pr4181-primary-ax, --state-dir "$STATE", --debug --json: open settings --relaunch, snapshot -i, another snapshot -i, click @e6, snapshot -i, and screenshot "$PNG" --pixel-density 3.

Observed phase evidence:

  • Initial open's background acquisition returned typed bridge-preparation-pending after 2,003 ms.
  • First snapshot returned typed bridge-readiness-pending after 2,001 ms, used XCTest preparation, then recovered within the same command to simulator-ax-bridge: acquisition 1,588 ms, presentation 9 ms. Total command time was 76.172 seconds, including runner preparation. This proves bounded preparation and recovery, not fast or fallback-free cold capture.
  • The following Settings snapshot used simulator-ax-bridge, acquisition 1,336 ms and presentation 53 ms, with no warnings.
  • Fresh ref e6 opened General. Its snapshot also used simulator-ax-bridge, acquisition 2,571 ms and presentation 8 ms. Both source and T3 PNGs were saved and inspected.

The native cache manifest matches the six current source inputs: source fingerprint 609d845f857129fe3e2abba609c1ba177bcbbdef0e842c2e84f6b4dbde0c223e. The actual running helper executable matched that cache entry and binary SHA256 ff2576721c8944d2ac0e400ffc57e59530ac14451cc6d7f80331b646ba7b19aa. Source close completed, daemon stop was graceful/clean with no pending or orphaned claims, and the observed helper PID exited.

The full affected gate passes all 41 runnable checks at this SHA; 25 skipped lanes are not passes. Draft retained: exclusively primary cold acceptance, the earlier separate application-server/foreground failure, and the wider runtime matrix remain unproven. The fallback remains a workaround. No capture deadline or behavioral assertion was weakened.

Verification: OpenAI Codex, parent gpt-6-astra; independent source review gpt-6.1-sol.

Follow-up at 312cdcfba1574f15db99bbb8cc3e15ca42fb144f: accepted review item (a). Removed the helper-startup failure cooldown so a stale failure cannot disable a new app generation. Binary preparation retains its separate cooldown, and pending helper creation remains shared. The new recovery regression fails before this correction and passes afterward. The existing expiry test encoded stale-failure replay and was corrected to preserve finite expiry/SIGTERM/no-socket cleanup, then require a fresh-generation attempt. Focused lifecycle/adapter/preparation: 45/45. No timeout was widened.

On the preceding head with a prebuilt bridge and a freshly rebooted simulator, one helper PID served all captures. Preparation took 145 ms. Open encountered foreground-owner-unverified, then three subsequent snapshot commands used primary AX (acquisitions 2,697/109 ms within the first command, then 203 ms and 88 ms). That run did not reproduce bridge-readiness-pending; it does not establish the exact requested pending/fallback trajectory. The Darwin native runtime file passed 51/51, including both native listener-readiness cases.

Nonblocking refactors (shared constants, required readiness deadline, shared cleanup ownership, README placement, and typed native startup-protocol cleanup) are deferred. The suggested adapter-boundary timing test is a coverage gap; current source already passes the same readiness deadline to preparation and manager. The unreachable runtime-unavailable response is a diagnostic cleanup, not a newly observed capture defect. No unrelated refactor was included.

Final correction head 312cdcfba1574f15db99bbb8cc3e15ca42fb144f is now pushed. A fresh complete check:affected --run --head 312cdcfba passes all 41 runnable checks, exact commit; 25 lanes remain skipped. Broad unit: 1,915 files, 17,107 passed, one skipped.

The current-head cleanup probe observed the real prebuilt helper with no socket yet, then requested daemon stop --clean on its private state. Open returned the expected intentional request_canceled; source acquisition recorded cancelled: abort-signal. Stop was graceful/clean, and both the observed helper PID and its socket were absent afterward. No synthetic startup delay was inserted.

A subsequent fresh current-head daemon captured Settings through primary AX (754 ms and a 188 ms stabilization acquisition), navigated by General's fresh ref, then captured General through primary AX in 719 ms. Source and T3 General PNGs were saved and inspected. The 8.235-second click included 6.458 seconds of XCTest runner readiness preflight; it was not a fast-click benchmark. Open still reported the transient foreground-owner refusal before primary recovery. Final source session/daemon cleanup passed, with owned helper/daemon PIDs and socket absent. This closes the pending-startup cleanup evidence gap, while the exact requested cold pending/fallback trajectory and runtime matrix remain unqualified.

@thymikee

thymikee commented Nov 3, 2026

Copy link
Copy Markdown
Member

I reviewed 312cdcf. The live evidence I asked for at 34236db is now in, so that part is resolved. One defect remains in the new readiness path.

With retryAfterMs removed, a failed readiness attempt is deleted when it settles (detached-attempt.ts:141-142). Only a caller still inside its wait at that moment sees the failure. With the first-caller grant that is at most the first capture, for at most 2 s. If the helper crashes or hits the 10 s bridge-startup deadline after that, no capture gets the error. The first capture already returned preparing, which is capture-scoped (snapshot-route-failure-policy.ts:46) and opens no circuit. The next capture starts a fresh attempt. So a helper that keeps failing never opens the generation circuit and leaves no route diagnostic. Each capture, or one per 10 s in the timeout case, pays the 2 s wait again, spawns and kills another helper, and falls back to XCTest. At 34236db and on main the next capture saw the failure and retired the generation, so this weakens the persistent-failure route the first-caller grant was meant to bound (#2491). The corrected expiry test at lifecycle.test.ts:743 encodes this: the timeout is never surfaced.

The rule the code must satisfy: every helper startup failure is delivered to exactly one capture, and it retires a generation only when that capture belongs to the generation whose capture started the attempt. A capture of any other generation starts a fresh attempt. SnapshotBridgeManager (lifecycle.ts:88) should own this, because only it knows the starting generation. One way: when createSession fails, record { generation: input.target.generation, failure } for the udid. Clear the record when a readiness waiter observes the rejection. In ensureSession, before readiness.value, throw and clear a record whose generation matches input.target.generation, and clear a non-matching one. To validate, add a lifecycle test where the helper exits after the 2 s wait. The next same-generation capture must reject with process-crash (or timeout with bridge-startup-deadline), and a generation-2 capture must start a fresh helper.

Not blocking, take or leave: the new regression at lifecycle.test.ts:83 throws a hand-built process-crash synchronously from host.start, while production reports a crash through process exit and bridgeExitOrClose, and it asserts "does not retire" as a second manager attempt rather than through the route's disabledGenerations. Driving the failure through the fixture's process exit, and folding the late-failure case into the same test, would cover both.

There are no conflicts. The only failing check is Vercel, which waits for a team member to authorize the fork deploy. The diff touches only packages/platform-apple/src/snapshot-source and apple/snapshot-bridge, so it is unrelated. No GitHub Actions job ran on this fork head, and I did not run tests locally, so the 45/45 focused, 41-check check:affected and native 51/51 results are the author's. The live pending to bridge-served run happened at 34236db with a bridge build in progress, not a cold-AX pending. A rebooted-simulator run did not reproduce pending. The delta does not change those paths, so I accepted that evidence, but the cold-AX pending case is still unobserved. Before merge, fix the late-failure delivery with the lifecycle test above, then run a full check:affected on the new head and take the PR out of draft.

@andrewfree

Copy link
Copy Markdown
Contributor Author

Fixed and pushed at 76ce0327da6deda9329dd779f6213f2cfacd4acc.

The manager now retains a failed startup with its starting generation. The next capture of that generation receives and consumes the failure; a different generation discards it and starts a fresh helper. A readiness waiter that observes the original rejection clears the retained failure, so it is not replayed.

The regression drives a real fixture process exit after the first 2-second wait through bridgeExitOrClose. Crash and 10-second startup expiry both cover same-generation delivery, consumption without replay, and direct generation-2 recovery. The expiry test now requires timeout/bridge-startup-deadline for the next same-generation capture because its previous expectation skipped failure delivery. Cleanup and fresh-helper assertions are retained. No deadline was widened.

The final tests on c7d87ea produce 4 expected failures, 2 passes and 31 filtered skips; on the new head all 6 cases pass with 31 filtered skips. Focused lifecycle, adapter, preparation, route and route-failure-policy tests pass 99/99. The full affected gate passes all 41 runnable checks with exactCommit=true; 25 lanes are skipped, not passes. Broad unit results: 17,112 passed, 1 skipped.

The optional direct disabledGenerations assertion was not added; existing route/failure-policy tests were included. No new live-device run was performed.

@andrewfree
andrewfree marked this pull request as ready for review November 4, 2026 01:46

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 10 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

At 76ce032, the manager now retains the previously requested readiness state, and I found no new blocking defect since the earlier review. The review is static; I did not run tests or device commands. The exact cold-AX recovery trajectory remains unobserved, as previously accepted.

The null fixture thread and two lower-priority fixture threads still apply. The binary path thread does not apply because production capture uses one fixed binary per runtime.

The only failing check is the Vercel deployment authorization, which is not a code problem. There are no known conflicts. Before merge, please address or reply to the three fixture threads.

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

This conflicts with main now. Please rebase it so review can continue. The three fixture threads from the review at 76ce032 still apply.

f and others added 11 commits November 4, 2026 17:59
Cover a helper crash observed inside the starting capture's wait: the next
same-generation capture starts a fresh helper instead of receiving the
failure again. Require close to stay pending while adopted startup is
paused instead of checking one microtask.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…exit codes

The fixture identity cases still create real directories and sockets, so a
100 ms deadline could fail them before identity validation on a loaded
runner. The readiness fixture now reports fork and waitpid failures with
their own exit codes instead of reusing the behavioral failure code.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@andrewfree
andrewfree force-pushed the fix/ios-detached-bridge-readiness-upstream branch from 76ce032 to bfbead4 Compare November 5, 2026 03:25
@andrewfree

Copy link
Copy Markdown
Contributor Author

Rebased on main 4abff15e at bfbead48f and replied in the fixture threads. Commits since your review at 76ce032 are tests and docs only (3534d8408, d228cac5b, e123cd71b, bfbead48f). The only conflict was adapter.test.ts; its helpers moved unchanged to adapter.fixtures.ts for the line limit (git show --color-moved 753672917), and the readiness fixture now links SnapshotBridgeSurfaceHosts.m. Production diffs match the pre-rebase PR.

@thymikee

thymikee commented Nov 5, 2026

Copy link
Copy Markdown
Member

At bfbead4, the code looks good. The rebase is in, there are no conflicts, and production code is unchanged from the revision that had live device evidence, so I am not asking for a new live run. The fixture threads are fixed and can be resolved: null return, exit codes, pid mismatch. The binary path thread does not apply, since adapter.ts prepares one binary per runtime.

Coverage failed at bfbead4, but its log stops at "building macOS helper" and names no failing test. The same cut-off log appeared on another PR and later passed, and main Coverage passed at 4abff15, so it looks unrelated. Please re-run Coverage so there is a green result or a complete log that names a test.

One design question, since the change adds about 800 production lines: could the adapter be the single owner of the readiness allowance, with one named constant and readinessDeadline required on manager.request and readyBinary? Today the 2 s allowance is a bare literal at adapter.ts:68, lifecycle.ts:94 and lifecycle.ts:236, next to the named SNAPSHOT_BRIDGE_PREPARATION_WAIT_BUDGET_MS in preparation.ts:34, and the readinessDeadline ?? createSnapshotSourceDeadline(...) fallback and the default in preparation cannot run in production because adapter.ts always passes the deadline. The manager would then only consume it, and the unreachable fallback and default would go away. If you prefer the current shape, a short note on why would settle it.

Not blocking, take or leave: the 10 s startup ceiling is also a literal twice at lifecycle.ts:297 and :299.

This branch has not been deployed

No deployments
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.

2 participants