test(catalog): budget the K-owner file and reap children before teardown (Windows stack 3/3) - #3550
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 (8)
📝 WalkthroughWalkthroughThe PR updates Windows-suite stabilization records, retracts an ACL diagnosis, documents an unresolved OAuth cleanup failure, records confirmation runs, and hardens retained-root integration tests by reaping child processes before sandbox deletion. ChangesWindows suite stabilization
Estimated code review effort: 3 (Moderate) | ~25 minutes 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
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The substantive test-harness direction is sound: all asynchronous children are registered before they can escape teardown, holders are released before kill/await/delete, the late rejecting race branch is marked handled, and the named spawn budget replaces local-machine-derived timeouts without removing the behavioral assertions. The retained-root ablation evidence is also useful.
This PR currently targets codex/win-2-unlinked-cwd, which is not an accepted integration target under the repository policy. Please keep the stack relationship while the predecessors are open, but after #3548 and #3549 land, retarget/rebase this layer onto the then-current dev and run exact-head CI on that final diff. Do not merge it into the temporary stack branch. I will re-review the retargeted head, including the final multi-agent-keep-native-v1 documentation-only delta.
리뷰 · 우선순위 68 / 80설명 이 PR은 Windows 스위트 안정화 스택의 마지막 장(3/3)이다. 베이스는 실패 이야기는 측정으로 잘 정리돼 있다. CI run 33920624827의 windows 2/4에서 첫 커밋이 그 한 케이스만 올리면 실패가 파일 안 다음 케이스로 미끄러졌다. 18.7초로 통과한 뒤, convergence POST 케이스가 20초를 140ms 넘기며 또 타임아웃하고, 배리어 이후 죽은 자식의 143 거부가 unhandled로 위에 얹혔다. 두 번째 커밋이 그걸 클래스로 고친다. 명시 예산 네 곳을 모두 같이 실린 계획 문서( 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Implementation review found three overclaims, none in behaviour: - featureActionOf never sees `file` and accepts any .cmd/.bat target, so it cannot reject a non-Codex invocation. It extracts semantic arguments; the executable identity is pinned by codex-v2-gate and win-exec. Said so in the helper, renamed the case to 'rejects malformed argv'. - 007 claimed the killed process kept its own handle. Windows closes a terminated process's handles, and no handle-owner snapshot was taken, so the honest claim is that the killed run left a holder that was not identified. The operational rule does not depend on which. - 002 still read as a live ACL diagnosis; it now carries a superseded banner and 000 labels it as raw shard counts only.
…failure Confirmation run 1: 25 fail -> 1. Both fixes verified on Windows; shards 1, 3 and 4 are green. The survivor is documented to the limit of what was measured, including three dead ends: it is not the file alone (0/5 solo), not rmSync (isolated probe passes all four shapes), and not the leftover directory (pre-creating it passes). A two-file minimal repro exists but only reproduces under load - eight consecutive idle runs pass in 5.5s while every failing run took 119s with 5.2s teardowns, which is removeTreeWithRetry's full retry budget. The handle-owner snapshot that would close it failed: a 1 Hz watcher polling through powershell.exe became the load itself, broke 72 cases across ten unrelated suites, and never sampled the millisecond window. That run is discarded. Recorded because it is the same mistake as 007 - adding a process to the box and reading the result as a property of the code. No fix attempted for it: one case in 17807, no idle reproduction, and two prior read-only diagnoses in this unit were both wrong.
…cally Same case, same log offset, on a fully idle box. Three shard-2 runs, three identical failures, so the load hypothesis from measurement 6 is corrected: load explains why the two-file pair needed it, not why the shard fails. Binary search over shard 2 with oauth-store-multi pinned last is the cheapest next step.
…before teardown CI run 33920624827 (windows 2/4): the case timed out at its 15 s per-test budget, Bun killed two dangling children, and the finally then wrote a release marker into a sandbox root afterEach had already removed (ENOENT). The case spawns a lock holder that stays alive plus a startup probe and a real CLI sync-cache in series - three Bun boots, two importing the server/CLI graphs, at 8-11 s each on windows-latest. 15 s was sized from the ~450 ms local run. Budget: SPAWN_BUDGET_MS, the repository's named bound for real child processes. Verified per test-budget.ts rule 2 - a one-token ablation (BEGIN IMMEDIATE -> BEGIN at catalog-write-serialization.ts:188) turns the behavioural existsSync assertion red, so the wait is intrinsic and the case is not vacuous. Teardown: children are tracked on the sandbox and teardownSandbox() releases holders, kills survivors, and AWAITS every exit before the root is deleted; afterEach is async and runs it first, and holder.release() tolerates ENOENT. Forced to a 1 ms budget the case now fails cleanly with no unhandled ENOENT. The Bun.spawnSync CLI step stays synchronous; converting it is a follow-up.
…nd detach the barrier race CI run 33923803071 (windows 2/4): the first fix moved the failure one case down the file. The startup+CLI case ran 18.7 s and passed under SPAWN_BUDGET_MS; the convergence case then timed out at its 20 s by 140 ms, with 'killed 1 dangling process' and an unhandled 'sync exited before provider barrier (143)' on top. Budgeting one case instead of the class was the mistake. All four explicit budgets in the file become SPAWN_BUDGET_MS: every case boots the same kind of Bun child on the same runner and was sized the same way. The four inline Bun.spawn calls now register with the sandbox so the reap- before-delete teardown covers them; the previous commit only reached the two helpers. raceBarrier() replaces the duplicated Promise.race: the early-exit branch is a rejecting promise that stays pending after the barrier wins, and when a timeout later kills the child that rejection has no awaiter - which is the unhandled error CI showed. A no-op catch attached up front marks it handled without changing the race. Verified by inserting a 5 s sleep after the barrier under a 3 s budget: the original reports the timeout PLUS an unhandled error; this reports the timeout alone. Probe removed; 6/6, typecheck clean.
60425ed to
ffa6881
Compare
… 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.
…erver (Windows stack 5/5) (#3558) * docs(devlog): run 5 exposes two more sub-floor internal waits; plan the class inventory (080) * docs(devlog): inventory every sub-floor internal wait in tests/ and classify it (080) * test: budget every sub-floor internal wait that gates on a child or a 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. * test: keep internal waits under their case budgets - INTERNAL_DEADLINE_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. --------- Co-authored-by: t <a@b.com>
2 participants
Summary
Windows suite stabilization, PR 3 of 3 (stacked on #3549). Fixes the
codex-retained-root-serializationtimeouts on the hosted Windows runner and the teardown race they exposed.Run 33920624827 (windows 2/4): the three-child K-owner case timed out at its 15 s budget, Bun reported "killed 2 dangling processes", and the
finallythen wrote a release marker into a sandbox rootafterEachhad already removed (ENOENT). Budgeting only that case (first commit) moved the failure one case down the file — 18.7 s passed under the new budget, the next case then timed out at 20 s by 140 ms with an unhandled "sync exited before provider barrier (143)" on top. The second commit fixes the class:SPAWN_BUDGET_MS, the repository's named bound for real child processes. Every case boots the same kind of Bun child on the same runner (8-19 s each, 2× run-to-run variance) and was sized the same way from a ~450 ms local run.Bun.spawnregisters with the sandbox;teardownSandbox()releases holders, kills survivors, and awaits every exit before the root is deleted.afterEachis async and runs it first.raceBarrier()attaches a no-op catch to the early-exit branch up front, so a child killed after the barrier wins cannot surface as an unhandled rejection.Ablation per
test-budget.tsrule 2 was run before the fix: the one-token mutationBEGIN IMMEDIATE → BEGINincatalog-write-serialization.tsturns the behaviouralexistsSyncassertion red, so the wait is intrinsic and the case is not vacuous. The barrier fix was verified by a probe (5 s sleep after the barrier under a 3 s budget): original code reports the timeout plus an unhandled 143, fixed code reports the timeout alone. Two intermediate attempts that failed that probe were discarded.No product change.
Verification
bun test tests/codex-integration/codex-retained-root-serialization.test.ts(macOS): 6 passbun run typecheckcleanChecklist
Summary by CodeRabbit
Documentation
Tests