test(update): give the launcher-recovery case a budget that contains its own work - #2666
Conversation
…its own work This test failed on macOS shards at 46-47s across three unrelated PRs while the product was healthy. Rerunning made it pass, which is how a broken budget disguises itself as an infrastructure blip. The arithmetic never worked. The case spawns `node launcher update` (30s timeout), waits for the recovered proxy, then spawns `node launcher stop` (30s timeout) in its finally block - inside a 60s Bun timeout. When 34ef539 raised the readiness wait from 15s to 45s to survive a loaded runner, it fixed the wait and left 60s of spawn budget with nowhere to go: 30 + 45 + 30 does not fit in 60, so on any runner where the proxy was slower than about 15s the case could not finish at all. Locally the proxy boots in ~2s and the whole case takes 2.4s, so the ceiling was invisible until CI was loaded enough to need the headroom that was promised but not reserved. The budget is now derived from its parts rather than guessed against them, and a contract test asserts the relationship instead of the numbers - so raising any single timeout later cannot silently recreate an impossible one. Falsified: restoring the 60s value fails that test. The readiness deadline stays 45s deliberately. It exists to stop a hung proxy, not to assert a boot deadline the suite never intended to enforce; a slow-but-live proxy must still pass. bun test tests/update-stop-first.test.ts: 15 pass, 0 fail (was 14). bun x tsc --noEmit: exit 0.
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe recovery test now uses named timeout constants for process spawning, proxy readiness, and total execution. A new assertion verifies that the total timeout includes required operation budgets and fixture I/O headroom. ChangesRecovery test timeout budget
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized test change derives the recovery-case timeout from its constituent work and adds coverage to prevent an impossible budget from returning; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f1a9344b8
ℹ️ 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".
| const UPDATE_SPAWN_TIMEOUT_MS = 30_000; | ||
| const PROXY_READY_TIMEOUT_MS = 45_000; | ||
| /** Spawn + readiness + teardown spawn, plus headroom for fixture IO on a loaded runner. */ | ||
| const RECOVERY_CASE_TIMEOUT_MS = UPDATE_SPAWN_TIMEOUT_MS + PROXY_READY_TIMEOUT_MS + UPDATE_SPAWN_TIMEOUT_MS + 15_000; |
There was a problem hiding this comment.
On Linux CI, this constant evaluates to 120,000 ms, exactly matching BUN_TEST_BATCH_TIMEOUT_SECONDS in scripts/ci/run-bun-test-batches.sh:6; that script wraps the entire 12-file batch—and every singleton retry—in timeout 120s at lines 112-114. GNU timeout --help defines this as “Start COMMAND, and kill it if still running after DURATION,” so when the recovery case needs the newly promised headroom, the outer watchdog terminates Bun before this per-test deadline can complete, including before teardown, and the same 120-second singleton retry cannot recover it. Raise the batch/singleton watchdog above this case plus process overhead, isolate this file in a longer lane, or reduce the per-test budget accordingly.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 70 / 80설명 이 풀 리퀘스트는 테스트만 고칩니다. 런처가 실패한 업데이트 뒤에 멈춘 프록시를 다시 켜는 경우가, macOS CI에서 46-47초에 실패하던 문제입니다. 지금 dev의 tests/update-stop-first.test.ts 148-237줄이 그 경우입니다. 198-204줄은 런처 update를 30초 제한으로 돌리고, 211줄은 waitForProxy를 기다립니다. waitForProxy는 23-30줄에서 45초 동안 /healthz를 봅니다. finally의 217-223줄은 런처 stop을 다시 30초 제한으로 돌립니다. 그런데 테스트 전체 제한은 236줄의 60000ms입니다. 숫자를 더하면 30 + 45 + 30 = 105초입니다. 60초 안에는 들어갈 수 없습니다. 로컬에서는 프록시가 약 2초에 떠서 전체가 2.4초에 끝납니다. 그래서 천장 부족이 작성자 컴퓨터에서는 보이지 않습니다. CI가 바빠져 준비 대기가 15초를 넘기면, 제품이 멀쩡해도 테스트가 타임아웃으로 실패합니다. 본문이 말한 대로 이것은 제품 버그로 보이지만 제품 버그가 아닙니다. 고침은 예산을 부분의 합으로 만듭니다. UPDATE_SPAWN_TIMEOUT_MS 30초, PROXY_READY_TIMEOUT_MS 45초, 그리고 같은 스폰을 한 번 더 더한 뒤 15초 여유를 붙여 RECOVERY_CASE_TIMEOUT_MS를 만듭니다. 두 스폰 자리와 waitForProxy 기한이 그 이름을 씁니다. 새 테스트는 합이 두 스폰과 준비 대기보다 크고, 남은 시간이 10초 이상이라고만 검사합니다. 숫자 자체를 고정하지 않아, 나중에 한 값만 올리면 불가능한 예산이 다시 생기는 일을 막습니다. 베이스는 지금 dev입니다. 방향은 맞습니다. 준비 기한 45초를 줄이지 않은 것도 본문 설명과 맞습니다. 그 기한은 멈춘 프록시를 끊는 용도이지, 빠른 부팅을 강제하는 용도가 아닙니다. 다만 합이 120초라서, 프록시가 정말 멈추면 CI 한 칸이 2분 가까이 기다립니다. UPDATE_SPAWN_TIMEOUT_MS 이름이 stop 스폰에도 쓰이므로, 나중에 update 제한만 올리고 stop은 짧게 두고 싶을 때 이름이 방해가 됩니다. 이 파일만 바뀌므로 타입 분리 캠페인과 무관합니다. tests/update-stop-first.test.ts:236 - 전체 제한 60초가 update 30초 + 준비 45초 + stop 30초를 담지 못합니다. 메인테이너의 판단이 필요한 지점
너의 추천 합치는 것을 추천합니다. 테스트 예산 산수만 고치고 제품 코드를 건드리지 않습니다. 지금 dev에 바로 넣어도 됩니다. 미리보기 배포는 계획에 없습니다. 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
npm launcher restarts the stopped runtime after a staged update failurefailed on macOS shards at 46-47s across three unrelated PRs (#2659, #2661, and an earlier #2646 run) while the product was healthy. Rerunning made it pass, which is how a broken budget disguises itself as an infrastructure blip.The arithmetic never worked. The case spawns
node launcher update(30s timeout), waits for the recovered proxy, then spawnsnode launcher stop(30s timeout) in itsfinally— all inside a 60s Bun timeout.When
34ef53966raised the readiness wait from 15s to 45s to survive a loaded runner, it fixed the wait and left 60s of spawn budget with nowhere to go:On any runner where the proxy took longer than roughly 15s to bind, the case could not finish at all. Locally the proxy boots in ~2s and the whole case takes 2.4s, so the ceiling stayed invisible until CI was loaded enough to need the headroom that was promised but never reserved.
What changed
The budget is derived from its parts instead of guessed against them:
Both spawn sites now use the named constant rather than a repeated literal, and a contract test asserts the relationship rather than the numbers — so raising any single timeout later cannot silently recreate an impossible one.
The readiness deadline stays 45s deliberately. It exists to stop a hung proxy, not to assert a boot deadline the suite never intended to enforce; a slow-but-live proxy must still pass.
Verification
bun test tests/update-stop-first.test.ts— 15 pass, 0 fail (was 14)bun x tsc --noEmit— exit 0Falsified: restoring the old
60_000value fails the new contract test (14 pass / 1 fail), so it is not vacuous.No GUI change, so no screenshot applies.
Checklist
Summary by CodeRabbit