Repository navigation
test(cli): measure cold status setup before timed projections - #4948
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
✅ Deterministic PR hygiene checks passed. |
|
Merging this on a recorded exception, so the reasoning is in the record rather than in a thread. The evidence this change needs is Windows, and it has it. At exact head The one red is Recording the boundary explicitly: this PR is merged with Windows and Linux evidence at its exact head, with a known unrelated macOS failure outstanding and owned elsewhere. It is not a claim that macOS is green at this SHA, and the release candidate is still held for exactly that reason. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cca06b1693
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Keep this bounded and observable: a failed warmup is a setup failure, never a retry. | ||
| // The 45s hook reserves removeTreeWithRetry's existing 15s Windows cleanup bound plus 5s | ||
| // to reap the child, leaving 25s for the cold status invocation itself. | ||
| const result = await runTimedStatus(home, codexHome, true, 25_000); |
There was a problem hiding this comment.
Use the Windows cold-spawn budget for the warmup
On a Windows runner where the first Bun child needs more than 25 seconds, this setup still kills the cold child and fails the entire describe block before any 15-second projection assertion runs. The repository already defines COLD_SPAWN_BUDGET_MS as 90 seconds on Windows because a first proxy child has taken about 50 seconds in CI, so the new warmup merely moves the same cold-start flake from 15 to 25 seconds. Give this one setup invocation the cold-spawn budget and enlarge the enclosing hook enough to retain the cleanup margin, while leaving the measured projection children at INTERNAL_DEADLINE_MS.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
Separate the cold full-CLI status path from the version-projection assertions. Windows run 35277467396 killed the first status child after 15,587 ms; the next row took 13,074 ms and subsequent two-child rows took 1,518–1,741 ms. The first row was measuring cold setup plus behavior while later rows measured the warmed path. A later passing run does not resolve that defect.
One Windows beforeAll fixture now executes the same real status and health path before the matrix. Setup must succeed and is bounded by the existing 45-second spawn budget; its child has 25 seconds, leaving the existing 15-second Windows tree-removal bound plus five seconds for reap. All sixteen original JSON/human children retain their 15-second deadlines and result assertions. Cold setup and individual child timings are logged for hosted evidence. No retry, global budget increase or platform skip is added.
The observed cold-path dependence does not isolate Bun transpilation, Defender, filesystem caches or external diagnostic probes. This change makes that setup cost explicit and measured without claiming an unobserved subsystem cause.
Verification
Checklist