fix(ios): honor the startup budget through a cold Simulator boot - #2325
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The daemon startup-deadline wrapper can incorrectly treat requests as successful past the deadline due to reliance on a timer-fired boolean rather than checking the absolute deadline.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the --timeout startup budget semantics for iOS simulator startup so a single caller-specified deadline is honored across cold simulator boot initialization and Apple runner readiness, while preserving device-claim ownership and improving cleanup/claim retention behavior when shutdown cannot be confirmed.
Changes:
- Introduces a daemon-owned startup deadline (
startupDeadlineAtMs) propagated through open/prepare execution into Apple readiness and runner preparation. - Adjusts client timeout envelope calculation to keep a cleanup margin beyond the startup budget and validates timer budgets before dispatch.
- Updates device-claim cleanup behavior and adds regression tests/docs covering cold-boot deadlines, contention, cancellation, and cleanup retention.
File summaries
| File | Description |
|---|---|
| website/docs/docs/sessions.md | Documents using open --timeout for never-booted iOS simulators and claim/cleanup semantics. |
| src/platform-runtime-apple-application-tools.ts | Enforces runner readiness within a shared startup deadline during prewarm. |
| src/daemon/types.ts | Adds internal startupDeadlineAtMs to daemon request typing. |
| src/daemon/session-lifecycle/internal/session-open.ts | Wraps open flow with startup-deadline enforcement helper. |
| src/daemon/session-lifecycle/internal/session-open-prepare.ts | Propagates startup deadline into execution context used by runtime operations. |
| src/daemon/session-lifecycle/internal/session-open-execution.ts | Extracts and enhances claim rollback behavior (typed cleanup failure retention). |
| src/daemon/session-lifecycle/internal/session-open-deadline.ts | Implements daemon-side startup deadline + cancellation for open. |
| src/daemon/session-lifecycle/internal/session-open-claim-rollback.ts | Centralizes rollback logic that abandons claims when cleanup cannot be confirmed. |
| src/daemon/session-lifecycle/internal/tests/session-open-runtime.test.ts | Updates runtime tests to mock runner session prewarm with new startup behavior. |
| src/daemon/session-lifecycle/internal/tests/session-open-deadline.test.ts | Adds tests for startup-deadline validation, cancellation, and cleanup behavior. |
| src/daemon/session-lifecycle/internal/tests/session-open-claim-rollback.test.ts | Adds tests ensuring typed cleanup failures retain claims as intended. |
| src/daemon/request-execution-scope.ts | Routes admitted execution through claim-admission wrapper to track cleanup failures. |
| src/daemon/handlers/session-prepare.ts | Validates/normalizes prepare timeoutMs and applies max startup budget bound. |
| src/daemon/device-claim-admission.ts | Adds run() wrapper and abandons claims on typed cleanup failure during disposal. |
| src/daemon/client/daemon-client-timeout.ts | Adds budget-plus-margin envelope behavior and validates max startup timeout budget. |
| src/daemon/client/tests/daemon-client.test.ts | Updates expected request envelopes to include cleanup margin where applicable. |
| src/daemon/client/tests/daemon-client-timeout.test.ts | Adds tests for overflow validation and cleanup-margin envelope behavior. |
| src/daemon/application-lifecycle-execution.ts | Projects daemon internal startup deadline into lifecycle execution context. |
| src/daemon/tests/device-claim-admission.test.ts | Adds coverage ensuring typed cleanup failures retain claims and block competitors. |
| src/core/command-descriptor/types.ts | Extends timeout budget model with budget-plus-margin envelope mode. |
| src/core/command-descriptor/timeout-policy.ts | Defines MAX_STARTUP_TIMEOUT_MS to stay within Node timer limits with margin. |
| src/core/command-descriptor/registry.ts | Applies budget-plus-margin policies to open and prepare. |
| src/core/command-descriptor/tests/timeout-policy.test.ts | Updates policy-set tests to include new envelope mode and command membership. |
| src/commands/management/prepare.ts | Updates CLI schema bounds/help for prepare startup budget. |
| src/commands/management/app.ts | Adds timeoutMs to open schema, help text, and CLI reader. |
| src/commands/management/app.test.ts | Adds tests for open timeout parsing/validation and dispatch rejection on overflow. |
| src/commands/cli-grammar/flag-definitions-workflow.ts | Updates --timeout help text to reflect open/prepare startup budget semantics. |
| packages/platform-apple/src/readiness/runtime.ts | Threads deadline into simctl boot/bootstatus and adds exact-target shutdown on cancel. |
| packages/platform-apple/src/readiness/runtime.test.ts | Adds readiness tests for shared budgets, cancellation, and bootstatus behavior. |
| packages/platform-apple/src/lifecycle.ts | Ensures open waits for runner readiness under startup deadline; shares prepare budget across boot + runner prep. |
| packages/platform-apple/src/lifecycle.test.ts | Adds tests ensuring deadline requires runner readiness and prepare shares boot/prep timeout. |
| packages/contracts/src/client-app.ts | Exposes timeoutMs on AppOpenOptions contract. |
| packages/contracts/src/application-lifecycle-runtime.ts | Adds startupDeadlineAtMs to ApplicationLifecycleExecution contract. |
Review details
- Files reviewed: 33/33 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const timedRequest = { | ||
| ...req, | ||
| internal: { ...req.internal, startupDeadlineAtMs: Date.now() + timeoutMs }, | ||
| }; | ||
| let expired = false; | ||
| const timer = setTimeout(() => { | ||
| expired = true; | ||
| markRequestCanceled(req.meta?.requestId); | ||
| }, timeoutMs); | ||
| try { | ||
| const response = await open(timedRequest); | ||
| return expired && !response.ok ? startupTimeoutResponse(timeoutMs) : response; | ||
| } catch (error) { | ||
| if (!expired) throw error; | ||
| if (error instanceof AppError && error.details?.reason === 'ios_boot_cleanup_failed') | ||
| throw error; | ||
| return startupTimeoutResponse(timeoutMs); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes timeout/cancellation/claim-fencing behavior across daemon execution and Apple simulator readiness paths, where subtle edge cases can affect session ownership and cleanup reliability.
Review details
- Files reviewed: 35/35 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
One issue at Please stop the startup timer before taking the snapshot, and add a test where startup finishes within the budget but the snapshot finishes after it. The test should fail before the fix and pass after it. The cold-start and cleanup runs are documented in the PR, but CI on this commit still requires action before it can run. |
|
The startup-timeout finding still applies at |
8bdb85b to
963e58b
Compare
thymikee
changed the title
|
I replaced the branch head with a smaller implementation of the same fix (previous head What stayed: the boot wait honors an absolute deadline, What went: the daemon-side timer that cancelled the whole request, the typed cleanup-failure error and claim-abandonment path, forcing runner readiness inside Live validation on three freshly created never-booted iPhone 17 Simulators is in the PR description; |
|
The foreground-snapshot issue is fixed at 963e58b. One deadline gap remains: after bootstatus, waitForSimulatorBoot calls simulatorState with a fresh 15-second timeout and never checks the deadline on success. If bootstatus uses nearly all the budget, the final state query can cross it and still return success. Please pass the remaining budget to that query and add a regression where it finishes after the deadline; the result should be boot_timeout. CI is still unreported on this head. |
A never-booted Simulator runs Apple's first-boot migration, which can take minutes, but the boot wait was capped at a fixed 120 seconds that neither `prepare --timeout` nor `open` could reach (callstack#2324). - The boot wait takes an absolute deadline. `prepare --timeout` now covers the boot and the runner preparation as one budget; `open --timeout` is new and bounds the boot. Expiry fails with `boot_timeout` and leaves the Simulator booting, so a retry finds it further along. - The client envelope for open/prepare keeps the 30s margin over the budget so the daemon's structured timeout wins the race against the client's reset. - `close --shutdown` no longer trusts the session device's selection-time `booted: false`; it always asks simctl. A session opened on a cold Simulator otherwise reported a shutdown that never happened. Supersedes the original implementation of callstack#2325 by @PrinceD96 (head 8bdb85b), which found the bug, the shutdown shortcut, and the validation recipe. Closes callstack#2324 Co-authored-by: PrinceD96 <53633741+PrinceD96@users.noreply.github.com>
963e58b to
bc1df93
Compare
After bootstatus, the listing that confirms the Booted state ran on its own 15-second timeout and the success path never re-checked the deadline, so a bootstatus that used nearly the whole budget could still return success past it. The listing now gets the remaining budget (capped at its own 15s), and a confirmation that lands after the deadline is reported as boot_timeout.
|
Fixed in
|
|
The Coverage failure on this head is main-side: |
|
Reviewed c03332e: the final boot-state query now shares the deadline, and the regression covers a successful response arriving too late. No remaining code findings; the reported cold-Simulator runs cover the startup path. Ready for human review. Coverage fails on a stale planned-operations.ts closure approval, which looks unrelated; one smoke lane is still running. |
thymikee
added
the
ready-for-human
Summary
Closes #2324.
Supersedes the original implementation by @PrinceD96 (head
8bdb85b7a3ea749ad9c631a8acb3fe573c831124), who found the bug, the staleclose --shutdownshortcut, and the cold-Simulator validation recipe; the commit carries aCo-authored-bytrailer for them.A never-booted iOS Simulator runs Apple's first-boot data migration, which can take several minutes. The boot wait in
ensureAppleReadywas capped at a fixed 120 seconds that neitherprepare --timeoutnoropencould reach, so the cap preempted the caller's budget.This head replaces the earlier implementation with the smallest change that makes the budget reach the boot wait:
simctl bootandsimctl bootstatus -bshare it; expiry fails witherror.details.reason: boot_timeoutand a hint. The Simulator is left booting (a deadline is not a cancellation), so a retry finds it further along or ready. Ctrl+C keeps its existing shutdown-on-cancel behaviour.prepare --timeoutis one budget for the boot and the runner. The runner preparation gets what the boot left instead of the full budget again.open --timeoutis new and bounds the boot for a fresh session. The device claim is held fromopenonward, so a competing workspace getsDEVICE_IN_USEthroughout the cold start without any extra claim mechanism.openandprepareuse the existingbudget + 30s margin, never below the base envelopearithmetic (envelope: 'margin', the same shapewaitalready uses), so the daemon's structured timeout wins the race against the client-side daemon reset.prepare --timeout 240000now yields a 270s envelope instead of 240s.close --shutdownalways asks simctl. The session device carriesbooted: falsefrom selection time for its whole life, so the old shortcut reported a successful shutdown on a Simulator that stayed Booted. Reproduced live before the fix, verified fixed after.Dropped from the earlier head, on purpose: the daemon-side timer that cancelled the whole request (the budget is scoped to the boot, so
--foreground's snapshot is never cut by it), the typed cleanup-failure error and claim abandonment path, forcing runner readiness insideopen, thewaitForSimulatorBooton an already-Booted device, and theMAX_STARTUP_TIMEOUT_MSplumbing. Timer overflow for absurd--timeoutvalues is pre-existing for every--timeoutcommand and is out of scope here.Validation
Unit: readiness (deadline shared by boot + bootstatus, expiry →
boot_timeout, no shutdown; default 120s kept), lifecycle (prepare shares the budget; open forwards its deadline), timeout policy (envelope arithmetic + reviewed sets), open CLI/structured input, shutdown runtime.pnpm check:affected --rungreen.Live, local Xcode 26 / iOS 26.2 runtime, three freshly created never-booted iPhone 17 Simulators, isolated
--state-dir:open com.apple.Preferences --timeout 5000on a never-booted Simulator: failed after 8s total withboot_timeout+ hint; request log showssimctl boot1.8s thenbootstatuscut at 3.1s (one shared budget). Simulator left Booted.open com.apple.Preferences --timeout 600000on a never-booted Simulator: success in 77s (boot 4s, bootstatus 52s). This Mac finishes the first boot under the old cap, so the unit test carries the cap-removal proof; the live run proves the budget propagates and the flow completes.prepare ios-runner --timeout 600000on a never-booted Simulator: success in 104s (bootstatus 40s, then runner connect + health check).close --shutdownafter opening a Simulator selected while Shutdown: before the fix it returnedshutdown.success: truewith empty stdout/stderr and the Simulator stayed Booted; after the fix simctl is called and the Simulator reaches Shutdown.