test: budget every sub-floor internal wait that gates on a child or server (Windows stack 5/5) - #3558
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe changes replace Windows-sensitive literal timeouts with shared internal and enclosing test budgets across integration, server, storage, OAuth, and helper tests. The storage policy responsiveness test now asserts that the job reaches the expected idle state. ChangesWindows timing budget stabilization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
리뷰 · 우선순위 66 / 80이 PR은 Windows 스위트 안정화 스택의 다섯 번째(제목 기준 5/5) 조각입니다. 베이스는 왜 한 곳씩 고치면 안 되냐면, 그 러너에서 Bun 자식이 마커를 쓰기까지 대략 8–19초가 걸리고 실행마다 어느 대기가 그 바닥 아래로 걸리는지가 바뀌기 때문입니다. 25분짜리 디스패치를 돌릴 때마다 “다음 잔여”만 고치면 손댄 파일은 통합·oauth·server·storage 테스트 12개와 계획서 라인 459 - 경로 라인 221 근처 - 경로 080 계획 vs 구현 - 계획 B는 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 3b431b41390c57d75c6571ff73626c124ada0531.
The class-based inventory is the right direction, but the new internal budgets are not yet effective at several call sites. watchdogMs(10_000/20_000) becomes 45 seconds on Windows CI, while the enclosing tests still have explicit 30-second case timeouts. Bun will terminate the test before the helper can use its new deadline. This occurs at least in:
server-background-lifecycle.test.ts(live-worker wait, case timeout 30s)storage-policy-job-responsive.test.ts(server/worker poll, case timeout 30s)storage-worker-lifecycle.test.ts(live/idle waits, case timeouts 30s)storage-worker-teardown-isolate.test.ts(live-worker wait in 30s cases)
Move the enclosing cases onto named case budgets that exceed the largest internal watchdog with cleanup/assertion headroom, or keep the inner deadline below the explicit case ceiling. Add a source/meta regression so a future class expansion cannot create inner watchdog >= enclosing test timeout again.
There is also a vacuous wait in storage-policy-job-responsive.test.ts: the loop exits when the deadline expires without asserting that the job reached the expected idle state. Raising that deadline cannot prove the lifecycle completed. Track the terminal observation and fail explicitly if it was never reached.
Finally, this PR targets codex/win-4-quorum-observer, while #3555 is still changes-requested and not on dev. Keep this stacked until the lower PRs land, then retarget/rebase onto the resulting current dev and rerun the full exact-head matrix. The stated two-consecutive-green Windows acceptance bar should be applied only after the timeout hierarchy above is valid.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c477a02a77684eb712cad955c6b0ce19b16d3fa6. The outer/inner timeout inversion from the previous head is fixed, and the storage-policy idle poll now fails closed. Two blockers remain.
-
The PR body and devlog record normal
windows-latestchild readiness at 8–19 seconds, but every migrated child/server wait now usesINTERNAL_DEADLINE_MS = 15_000. That is already below an observed successful startup, so the stated two-consecutive-green goal still cannot be met reliably. Choose an internal child-ready bound with headroom over the measured 19-second tail while keeping each enclosing case comfortably larger; do not knowingly set the fix below the data that motivated it. -
tests/codex-integration/codex-retained-root-serialization.test.tsstill has a vacuous two-child rendezvous. The embedded route script loops until both barrier markers exist, but after the deadline it unconditionally returns a model response. If the peer starts late or never reaches the seam, the intended concurrent serialization proof runs sequentially and may pass. After the loop, explicitly fail when both markers were not observed, and include this path in the ablation/source guard promised by the test-budget policy.
Please also refresh the PR description: it still says twelve files and watchdogMs() even though this head touches nineteen test/helper files and intentionally moved the affected internal waits to INTERNAL_DEADLINE_MS. Keep the PR stacked until the lower layers land and rerun the exact-head Windows acceptance after these corrections.
fd786be to
70d9041
Compare
…he class inventory (080)
… server (Windows class fix) Five dispatches of this stack on windows-latest failed on five DIFFERENT literal deadlines, none of them an assertion: 15 s and 20 s case budgets (#3550), then a 10 s waitFor default in codex-write-lock and a 30 s per-request abort in codex-composed-acceptance on run 33930757649. Each run samples one or two new ones because a spawned Bun child boots in 8-19 s on that runner and the suite has many waits sized below that from local timings. Inventory in devlog 080: 58 literal deadlines under tests/, classified by what they gate on. This commit changes the ones that gate on a spawned child or a live server (class A/B): helper DEFAULTS (waitFor, waitForPath, waitForPort, waitForLiveWorker, waitForIdle) and inline deadlines now go through watchdogMs(), which keeps the local number and applies the CI/platform floor (45 s on Windows). The held-request in composed-acceptance gets two server budgets because it spans a startup plus a held gather, not one round-trip. Deliberately-short bounds (500 ms /healthz probes, refused-connection checks, in-process polls) are untouched; class D sites are left for the next pass. 12 test files; typecheck clean. Local verification: 59 pass across the three codex-integration files run in isolation; a full local sweep was blocked by another session holding the user test lock, so the CI dispatch is the gate.
…E_MS inside, SPAWN_BUDGET_MS outside Review of 3b431b4 (FAIL, 6 blockers) caught a composition error: watchdogMs() is 45 s on Windows CI, and I had put it on INTERNAL waits inside cases budgeted at 15-30 s. On the platform the fix targets, Bun's per-test timeout would fire before the wait's own diagnostic - a bare timeout instead of the marker name, which is where this unit started. test-budget.ts:64 states the invariant: an internal deadline stays a few times under its enclosing budget. Corrected shape, two knobs moved together: - every child/server-gated internal wait uses INTERNAL_DEADLINE_MS (15 s), the repository's named in-test bound, as windows-tray and cli-models already do; helper DEFAULTS changed so callers inherit - every case whose body spawns a child gets SPAWN_BUDGET_MS (45 s): the four 15 s and three 30 s spawn cases in native-profile-manager, all four codex-write-lock cases, history-lock, history-worker, oauth-refresh-lock Other blockers folded: - retained-root :515 (8 s two-child barrier) and :555 (20 s attempt loop) were in my own inventory and unchanged; now INTERNAL_DEADLINE_MS / SPAWN_BUDGET_MS - storage-policy-job-responsive fell through on expiry with no assertion, so a job that never went idle still passed; expect(settled) added - a real vacuous test independent of Windows - composed-acceptance: SERVER_BUDGET_MS * 2 was arithmetic on a wrong model (the held request and the OFF mutation overlap; startup precedes both). Named HELD_REQUEST_BUDGET_MS = SERVER_BUDGET_MS + INTERNAL_DEADLINE_MS with the actual shape in its comment - ten more A/B candidates from the reviewer's re-grep read one by one: five budgeted (codex-shim, codex-prompt-route, codex-prompt-text-probe, helpers/storage-policy-api, storage-mutation-race), five left with a reason each in devlog 080 (in-process polls, an asserted-latency bound, a preflight whose failure is a skip) Ablation (test-budget.ts rule 2), both run on macOS with the lock free: - history-lock: holder's ready-marker write disabled -> 'timed out waiting for .../held' from the helper at 15.0 s, the enclosing 45 s case intact - storage-policy-job-responsive: idle transition masked -> expect(settled) Expected true / Received false; before this commit the same mutation passed 17 test files + 1 helper; each file run individually on macOS: 331 pass / 0 fail; typecheck clean.
c477a02 to
3201181
Compare
2 participants
Summary
Windows suite stabilization, PR 5 of 5 (stacked on #3555). Budgets every internal wait in
tests/that gates on a spawned child or a live server, as a class.Five dispatches of this stack on
windows-latestfailed on five different literal deadlines and zero assertions: 15 s and 20 s case budgets (#3550), then on run 33930757649 a 10 swaitFordefault incodex-write-lockand a 30 s per-request abort incodex-composed-acceptance— both of which had passed on the four previous runs. A spawned Bun child boots in 8-19 s on that runner with 2× run-to-run variance, so each run samples a new wait sized below that floor. Fixing them one per 25-minute cycle cannot converge; the class had to be enumerated once.devlog/_plan/260905_windows_suite_stabilization/080_run_variance_residuals.mdinventories 58 literal deadlines undertests/and classifies them by what they gate on. This PR changes class A (child-boot markers) and class B (server round-trips, worker lifecycle): helper defaults (waitFor,waitForPath,waitForPort,waitForLiveWorker,waitForIdle) and inline deadlines go throughwatchdogMs(), which keeps the local number and applies the CI/platform floor (45 s on Windows). The held-request incomposed-acceptancegetsSERVER_BUDGET_MS * 2because it spans a startup plus a held gather, not one round-trip. Class C (deliberately short: 500 ms/healthzprobes, refused-connection checks, in-process polls) is untouched; class D (ambiguous 2-5 s deadlines) is listed for a later pass.No product change. Twelve test files.
Verification
bun run typecheckcleanbun teston the three codex-integration files run in isolation: 59 passChecklist
Summary by CodeRabbit
Bug Fixes
Documentation