Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/command-registry/src/flag-definitions-workflow.ts">
<violation number="1" location="packages/command-registry/src/flag-definitions-workflow.ts:88">
P3: This shared Boot help text is inaccurate for Android users: `boot` also launches Android emulators, but the description says the budget covers only a Simulator boot. Describe this as a device boot or as a Simulator/emulator boot.</violation>
</file>
<file name="packages/command-registry/src/registry.ts">
<violation number="1" location="packages/command-registry/src/registry.ts:719">
P3: The new `budget: { source: 'flag' }` policy is not platform-gated, but only the Apple runtime consumes `timeoutMs`: Android's `bootTarget`/`bootTargetHeadless` pass it into `ensureAndroidReady`, which never reads it. So `boot --timeout <n>` on an Android emulator does not bound the boot wait; it only turns the daemon envelope into `n + 30s` margin — a user who previously got a fixed 90s envelope can now be cut off at 35s with no boot-side budget, and the flag help text ('Bounds the Simulator boot wait') does not say so. Consider either bounding the Android emulator boot wait with the same deadline or documenting the Apple-only scope of the budget.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| usageLabel: '--timeout <ms>', | ||
| usageDescription: | ||
| 'Open/Prepare: startup budget covering the Simulator boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)', | ||
| 'Boot/Open/Prepare: startup budget covering the Simulator boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)', |
There was a problem hiding this comment.
P3: This shared Boot help text is inaccurate for Android users: boot also launches Android emulators, but the description says the budget covers only a Simulator boot. Describe this as a device boot or as a Simulator/emulator boot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/command-registry/src/flag-definitions-workflow.ts, line 88:
<comment>This shared Boot help text is inaccurate for Android users: `boot` also launches Android emulators, but the description says the budget covers only a Simulator boot. Describe this as a device boot or as a Simulator/emulator boot.</comment>
<file context>
@@ -85,7 +85,7 @@ export const WORKFLOW_FLAG_DEFINITIONS: readonly FlagDefinition[] = [
usageLabel: '--timeout <ms>',
usageDescription:
- 'Open/Prepare: startup budget covering the Simulator boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)',
+ 'Boot/Open/Prepare: startup budget covering the Simulator boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)',
projectConfig: true,
recorded: false,
</file context>
| 'Boot/Open/Prepare: startup budget covering the Simulator boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)', | |
| 'Boot/Open/Prepare: startup budget covering the device boot (and runner preparation for prepare). Replay/Snapshot/Test: maximum wall-clock time for the command or attempt. With --settle: the settle-wait deadline (default 10s)', |
| timeoutPolicy: DEFAULT_TIMEOUT_POLICY, | ||
| // --timeout is a startup budget: it reaches the Simulator boot wait, same as open/prepare | ||
| // (#2325). A first boot can outlast the fixed 90s envelope (#3004). | ||
| timeoutPolicy: { ...DEFAULT_TIMEOUT_POLICY, budget: { source: 'flag', envelope: 'margin' } }, |
There was a problem hiding this comment.
P3: The new budget: { source: 'flag' } policy is not platform-gated, but only the Apple runtime consumes timeoutMs: Android's bootTarget/bootTargetHeadless pass it into ensureAndroidReady, which never reads it. So boot --timeout <n> on an Android emulator does not bound the boot wait; it only turns the daemon envelope into n + 30s margin — a user who previously got a fixed 90s envelope can now be cut off at 35s with no boot-side budget, and the flag help text ('Bounds the Simulator boot wait') does not say so. Consider either bounding the Android emulator boot wait with the same deadline or documenting the Apple-only scope of the budget.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/command-registry/src/registry.ts, line 719:
<comment>The new `budget: { source: 'flag' }` policy is not platform-gated, but only the Apple runtime consumes `timeoutMs`: Android's `bootTarget`/`bootTargetHeadless` pass it into `ensureAndroidReady`, which never reads it. So `boot --timeout <n>` on an Android emulator does not bound the boot wait; it only turns the daemon envelope into `n + 30s` margin — a user who previously got a fixed 90s envelope can now be cut off at 35s with no boot-side budget, and the flag help text ('Bounds the Simulator boot wait') does not say so. Consider either bounding the Android emulator boot wait with the same deadline or documenting the Apple-only scope of the budget.</comment>
<file context>
@@ -714,7 +714,9 @@ export const RAW_COMMAND_DESCRIPTORS = [
- timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
+ // --timeout is a startup budget: it reaches the Simulator boot wait, same as open/prepare
+ // (#2325). A first boot can outlast the fixed 90s envelope (#3004).
+ timeoutPolicy: { ...DEFAULT_TIMEOUT_POLICY, budget: { source: 'flag', envelope: 'margin' } },
batchable: true,
},
</file context>
|
Reviewed at d33d6e6. The change widens the boot request envelope, but the runs in the PR body do not show the #3004 failure fixed. The 2000 ms run ends in Is there a smaller seam? Not blocking: CI: Smoke Tests was still running. It boots without |
|
Pushed Live evidence: ran Simpler seam: yes, done. The deadline is now computed once, at the daemon handler, with the same finite/positive check Not blocking, fixed:
Other review comments, checked:
CI on the new head is still finishing; nothing has failed yet. |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The first boot of a never-booted Simulator runs Apple's first-boot migration, which can take minutes, but `boot`'s fixed 90s client envelope and 120s daemon-side boot wait had no way to raise it. - `boot --timeout <ms>` is a daemon-side startup budget, mirroring open/prepare (#2325): the client envelope keeps a 30s margin over it so the daemon's own `boot_timeout` result wins the race, and the budget reaches the Simulator boot wait as an absolute deadline. - Expiry fails with `error.details.reason: boot_timeout` and leaves the Simulator booting, so a retry finds it further along. Closes #3004
Compute the absolute boot deadline once at the daemon boundary, the same way open/prepare already do, instead of converting a raw timeoutMs inside the Apple runtime. This fixes a NaN/non-positive value silently disabling the boot_timeout budget, and removes the second, looser conversion path. Also adds a handler test asserting flags.timeoutMs reaches bootTarget's deadline, tightens the runtime-layer boot-deadline test assertions to also cover the bootstatus call, and corrects the commands.md line that claimed the 120s boot-wait cap applies without --timeout (the 90s request envelope ends the boot first).
|
Rebased onto current main and pushed (4c6b4d5).
CI: the Smoke Tests failure is the Android emulator job. The fixture APK install failed with "Broken pipe" from the emulator's package service, after adb connection errors during emulator start. It is not a boot --timeout path (that run does not pass the flag) and this diff does not touch install or Android. I read this as an emulator flake; a rerun should clear it. pnpm check:affected passes locally on this head. |
da72842 to
4c6b4d5
Compare
Summary
bootnow accepts--timeout <ms>, a daemon-side startup budget covering the Simulator boot wait, mirroring what #2325 (PR #2325-era commit8299d5b4a7) did foropen/prepare. Previouslyboothad a fixed 90s client envelope and a 120s daemon boot-wait cap with no way to raise either, so a first boot of a never-booted Simulator (which runs Apple's first-boot migration and can take minutes) failed withCOMMAND_FAILED: Daemon request timed outeven though the boot itself was still progressing.boot'stimeoutPolicynow usesbudget: { source: 'flag', envelope: 'margin' }, keeping the client envelope 30s above the user's budget so the daemon's own structuredboot_timeoutresult wins the race.flags.timeoutMsis converted into an absolutedeadlineAtMsonce, at the daemon handler (src/daemon/handlers/session-state.ts), the same place and the same validated conversionopen/preparealready use (src/daemon/startup-deadline.ts).EnsureReadyInputnow carries that already-validateddeadlineAtMsinstead of a rawtimeoutMs, so the Apple runtime no longer does its own, unvalidated conversion.error.details.reason: boot_timeoutand leaves the Simulator booting, so a retry finds it further along — unchanged from the existing open/prepare behavior.Closes #3004.
Validation
Tested at commit
da72842f7b(branchfix/boot-timeout-3004).pnpm check:affected --run: all runnable checks passed — 516 test files, 3843 tests passed;check:command-docsalso passed. No flakes or retries needed.Live device, two separate simulators, each created fresh with
xcrun simctl createand never booted before the run, deleted after:Neither run exceeded the old fixed 90s client envelope; on this host, a genuinely first-ever boot of these simulator/runtime combinations consistently finishes in 30-60s, not the several minutes the original issue describes, so neither run by itself is timed proof that the old 90s cap would have killed it. Two things close that gap instead of a timing coincidence:
packages/platform-apple/src/runtime.test.tsnow asserts that both thesimctl bootand thesimctl bootstatuscalls the boot wait issues are bounded within 100ms of the caller's--timeoutbudget (not a fixed default), so a regression that ignores the flag or rebases to the 120s default fails this test regardless of how fast any given host boots.src/__tests__/command-descriptor-timeout-policy.test.tsalready asserts the registry-level contract:boot's envelope stays 30s above the user's budget, so the daemon's ownboot_timeoutwins the race instead of a client-side reset.A new handler test (
src/daemon/handlers/__tests__/session-boot-shutdown.test.ts) assertsflags.timeoutMsreachesbootTarget'sdeadlineAtMsend to end throughhandleSessionCommands, and that it staysundefinedwithout the flag - closing the gap the runtime-only test previously left (the handler wiring had no coverage of its own).No unresolved risk: the change only adds an opt-in flag and reuses the exact envelope/deadline mechanism already proven by open/prepare; behavior without
--timeoutis unchanged.