test(budget): restore the 45s spawn signal and stop hardening a disposable port - #4834
Conversation
…sable port PR #4830 raised SPAWN_BUDGET_MS on win32 from 45s to 90s to fix one test case. 31 test files read that constant and nine hand it to setDefaultTimeout, so the edit reached 339 Windows cases: 315 went 45s to 90s, 22 more that multiply it went 90s to 180s, and one derivation chain in codex-sync-api reached 265s. The detectors that now report twice as late are the contention ones -- codex-write-lock, the cross-process history-lock exclusions, the shim process cases -- and several of the affected files never spawn anything. The measurement behind #4830 was also contaminated. The child published its port through atomicWriteFile, the production SECRET writer, which on Windows runs hardenSecretPath(..., required: true) twice, each able to spawn PowerShell for SID resolution and several 30s-budgeted icacls passes. That ACL ceremony ran inside the window the parent measures as "time to reach a port" -- on a disposable port number. - SPAWN_BUDGET_MS returns to 45s on every platform. - COLD_SPAWN_BUDGET_MS (90s, win32 only) is consumed exactly once, by the readiness wait of the first proxy child in native-profile-startup. - The child publishes its port and settled markers with a plain temp-file rename, preserving the #1061 no-partial-read contract without the ACL ceremony. - The child logs child-entry, start-server-begin, start-server-end and port-published, so the next slow run names its own phase. - codex-sync-api owns its bounds instead of deriving them, which returns that case to 130s and makes it immune to the next edit of a shared constant. It also reports its measured preparation window on green runs. Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
|
✅ Deterministic PR hygiene checks passed. |
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. |
github-actions
Bot
added
the
chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesThe tests now use a dedicated cold-start budget for the first native startup child. Native startup diagnostics include phase timing and cold/warm budget details. Fixture files publish through temporary files and renames. The sync-race test uses fixed preparation budgets and surfaces preparation timing. Startup timing and diagnostics
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The startup timing changes preserve the first-child allowance and do not show a current merge-blocking risk. 🚥 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 |
리뷰 · 우선순위 72 / 80지금 #4830 이 고치려던 증상은 그래서 이 PR이 하는 일은 네 갈래입니다. (1) 범위·근거·문서 주석이 맞물려 있고, MERGEABLE이며 직접 lidge-jun PR입니다. 호스티드 CI는 아직 돌고 있고(테스트/게이트/키링/macOS 등), 로컬 실행은 레인 제약으로 안 돌렸다고 본문에 명시되어 있습니다. Windows 샤드 로그에서 살아남은 readiness가 2.0~19.7초라는 주장과 50.7초의 오염 원인을 같이 보면, 45초 복원은 신호 회복이고 90초 콜드 한 방은 보험으로 읽힙니다. 머지 우선순위는 높은 편입니다. 라인 native-profile-startup.test.ts · coldSpawnPending / FIRST_CHILD_CASE_BUDGET_MS - 콜드 플래그는 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: d309d52655
ℹ️ 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".
| function publishFixtureFile(path: string, content: string): void { | ||
| const tmp = `${path}.${process.pid}.tmp`; | ||
| writeFileSync(tmp, content, "utf8"); | ||
| renameSync(tmp, path); |
There was a problem hiding this comment.
On Windows, if Defender, an indexer, or a sync client briefly opens the newly written temporary file, this raw renameSync can fail with EBUSY, EPERM, or EACCES, causing the startup child to exit before publishing its port or settled marker. The previous atomicWriteFile path delegated to renameAtomicFile (src/config/atomic-write.ts), whose bounded retries exist specifically for these transient Windows sharing violations (src/lib/windows-atomic-replace.ts); retain the non-secret writer but use that retrying rename or an equivalent local retry so this Windows-flake fix does not introduce another intermittent failure.
Useful? React with 👍 / 👎.
…sable port (lidge-jun#4834) PR lidge-jun#4830 raised SPAWN_BUDGET_MS on win32 from 45s to 90s to fix one test case. 31 test files read that constant and nine hand it to setDefaultTimeout, so the edit reached 339 Windows cases: 315 went 45s to 90s, 22 more that multiply it went 90s to 180s, and one derivation chain in codex-sync-api reached 265s. The detectors that now report twice as late are the contention ones -- codex-write-lock, the cross-process history-lock exclusions, the shim process cases -- and several of the affected files never spawn anything. The measurement behind lidge-jun#4830 was also contaminated. The child published its port through atomicWriteFile, the production SECRET writer, which on Windows runs hardenSecretPath(..., required: true) twice, each able to spawn PowerShell for SID resolution and several 30s-budgeted icacls passes. That ACL ceremony ran inside the window the parent measures as "time to reach a port" -- on a disposable port number. - SPAWN_BUDGET_MS returns to 45s on every platform. - COLD_SPAWN_BUDGET_MS (90s, win32 only) is consumed exactly once, by the readiness wait of the first proxy child in native-profile-startup. - The child publishes its port and settled markers with a plain temp-file rename, preserving the lidge-jun#1061 no-partial-read contract without the ACL ceremony. - The child logs child-entry, start-server-begin, start-server-end and port-published, so the next slow run names its own phase. - codex-sync-api owns its bounds instead of deriving them, which returns that case to 130s and makes it immune to the next edit of a shared constant. It also reports its measured preparation window on green runs. Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
1 participant
Summary
PR #4830 raised
SPAWN_BUDGET_MSon win32 from 45s to 90s to fix one slow test case. The blast radius was much wider than the fix: 31 test files read that constant and nine hand it straight tosetDefaultTimeout, so the edit reached 339 Windows cases — 315 went 45s → 90s, 22 that multiply it went 90s → 180s, and one derivation chain incodex-sync-api.test.tsreached 265s. Several of the affected files never spawn a process at all; the detectors that now report twice as late are exactly the contention ones (codex-write-lock, the cross-process history-lock exclusions, the sync-cachemodels_cachecase, the Windows shim and main-account startup subprocess cases).The 265s case matters beyond slow reporting. The Windows shards have a 30-minute job timeout (
.github/workflows/ci.ymlL772) and runbun test --isolate --timeout 60000 tests --shard=N/6(L846), so an explicit per-test timeout overrides the 60s default and the job timeout becomes the only outer bound. Run35129771091measured Windows 5/6 at 26m39s and 6/6 at 25m13s. One hang in that 265s case puts its shard within roughly half a minute of an opaque job cancellation instead of a readable Bun timeout.The measurement that justified #4830 was also contaminated by the test's own instrumentation. The child published its port with
atomicWriteFile, the production secret writer, which on Windows runshardenSecretPath(..., required: true)twice (src/config/atomic-write.ts), each able to spawn PowerShell for SID resolution and severalicaclspasses budgeted at 30s apiece. That ACL ceremony ran inside the window the parent measures as "time to reach a port" — performed on a disposable, non-secret port number.What this changes:
SPAWN_BUDGET_MSreturns to45_000on every platform and every call site.COLD_SPAWN_BUDGET_MS(90s on win32, 45s elsewhere) is consumed exactly once: the readiness wait of the first proxy child spawned innative-profile-startup.test.ts. Every later child in that file and every other importer is back to 45s.renameSyncin the same directory. That preserves the only contract the parent has (macOS CI: native-profile process-exit phase test fails or hangs on release-train runs #1061: never observe the file between create and write) and removes the PowerShell/icaclsceremony from the measured window.child-entry,start-server-begin,start-server-end,port-publish-beginandport-published, so the next slow run says where instead of forcing another forensic round.waitForPort's timeout message now also reportscoldandbudgetMs.codex-sync-api.test.tsowns its bounds as literals instead of deriving them from a shared constant, and reports its own preparation window on every green run.test-budget.tsdoc comment records the ablation reasoning for the one remaining Windows widening, as its own stated standard requires.The fix is confirmed, not assumed
The added instrumentation settled both open questions on its first Windows run.
The 50.7s outlier behind #4830 was the ACL ceremony. With the port published by rename, run
35141541461measured every readiness wait innative-profile-startup.test.tsat 2.0s–4.9s across all six Windows shards, first child included. The cold-start outlier does not reappear.The Windows preparation reserve in
codex-sync-api.test.tswas guarding a number nobody had measured ("CI observed 52.7s before the flip could even start"). The child now reports it: 2740ms on Windows, 423–575ms on Linux and macOS, with the whole case at 3675ms and 660–780ms. That evidence is what sizes the new bounds — boot 40s → 30s and the Windows preparation reserve 80s → 55s — taking the case from 265s to 95s. The bound still fits the 52.7s outlier it was written for: that much preparation leaves the flip its full boot budget and its reap insideCOMPETING_OFF_CHILD_MS, so the tightening removes compounding without removing the protection.COLD_SPAWN_BUDGET_MSis kept for the part one run cannot rule out — a genuinely cold runner rather than ACL work. It costs nothing while the fix holds, because nothing approaches it, and the comment says to delete rather than raise it if it is ever breached.Verification
No local test suite, focused test, typecheck, build, or install was run for this change — this lane is under a standing no-local-execution constraint. Verification is static reasoning plus hosted GitHub Actions CI.
Hosted CI, head
4f89f0a7dcc6f82f772eb36a8a558ce6abac3e40:35145438663(workflow_dispatch, laneall— Windows only runs on manual dispatch). Linuxtest 1/4–4/4, all gates,npm-global,keyring,docker smoke,api usage,storage policy, macOS1/2and2/2: success. Windows 1/6, 2/6, 3/6, 4/6, 6/6: success on the first attempt; Windows 5/6 failed on the pre-existingcodex-write-lockflake analysed below and is success on re-run (job104975598799), where the same case passed in 2458ms with zero failures in the shard. Shard 6/6, which carries the retimedcodex-sync-apicase, completed in 25m29s.Hosted CI, head
d309d52655f1069ef46d15524bf820b5f209079c(this branch's first commit, same test-side change except thecodex-sync-apiconstants):35141017582(pull_request): success, all jobs.35141541461(workflow_dispatch): all six Windows shards success (4/6 13m39s, 2/6 22m05s, 5/6 22m17s, 1/6 22m11s, 3/6 22m18s, 6/6 24m23s). The run as a whole showscancelledonly because the newer dispatch superseded it by concurrency group while themacos controlserial lane was still running.Windows 5/6 is a pre-existing flake in another file, not a regression
codex-write-lock.test.ts > two real processes contend for one lock > a second process is excluded while the first holdsfailed at 15014ms onerror: timed out waiting for ...\held.That is the file's internal
waitFordeadline (INTERNAL_DEADLINE_MS, 15s), which this PR does not change; the case's Bun budget never expired. The failure reproduces ondevwithout this branch: run35121570658, windows 5/6, failed the same way at 15695ms onheld-env-HOME-differs-USERPROFILE-shared, before #4830 merged and before this branch existed.The cause is visible in the source rather than inferred.
tests/helpers/codex-write-lock-child.tswrites theheldmarker inside the lock callback, so the parent's 15s wait has to coverbunprocess spawn, the module graph including the production lock and its SQLite dependencies, and lock acquisition. The helper's own comment incodex-write-lock.test.tsrecords that boot as "8-19 s on a loaded windows-latest shard" — an observed range that straddles the 15s deadline waiting on it, so the case is red by construction under contention.spawnChildalso resolvesbunthroughPATHrather thanprocess.execPath, which adds shim resolution to the same window on Windows.The correct fix is the same shape as this PR's — publish a phase marker before the child attempts the lock, so "still booting" is distinguishable from "never acquired" — not a larger deadline. Both files are outside this lane's write scope, so it is reported rather than patched here.
Static checks performed:
SPAWN_BUDGET_MS(31 files, ninesetDefaultTimeoutconsumers, four multiplication sites) to confirm the restore returns all of them to their pre-test(budget): give a Windows child spawn the cold-start headroom it measures #4830 bounds and thatCOLD_SPAWN_BUDGET_MShas exactly one consumer.native-profile-crash-boundaries.test.ts, the other spawner of the modified child helper, carries its own budget and is unaffected except by a faster publish.tests/fixtures/file-size-baseline.jsonand that no test file is added, so the layout and size guards are untouched.structure/ownership coverssrc/areas; this change is confined totests/, so no structure doc is implicated.src/config/atomic-write.tsto confirm the doublehardenSecretPath(..., required: true)path that the rename-based publication avoids.Checklist
On the last box: this PR stops a test fixture from routing a disposable port number through the production secret writer. It changes no production code path, no default, and no authorization surface.
src/config/atomic-write.tsand the Windows ACL helpers are untouched here and belong to a separate lane.Co-authored-by: lidge-jun lidge-jun@users.noreply.github.com