Repository navigation
fix(windows): server.stop drains the config-dir ACL flight; retrying test teardown - #3257
Conversation
…shutdown test, exact line cites
hardenConfigDir() spawns icacls.exe as a fire-and-forget flight (e5d5886). The child holds the config directory open until it exits, and Windows locks are mandatory, so removing the directory right after a clean server.stop() returned EPERM/EBUSY — which is what every Windows CI shard has hit since 2026-08-30 in the account-store, auth-api and live-server fixtures. flushConfigDirHardening(dir) is now a production function scoped to one directory; startServer captures its config dir before loadConfig and the composite stop() awaits the flight after the listeners close. Regression test drives stop() with a held flight and proves it stays pending until release (driven red before the fix).
…ed, retrying teardown The fixed repo-local .tmp-codex-accounts-test meant one EPERM teardown poisoned every later case (49/49 errors in run 33590540220 were hooks). The sync-only icacls stub also no longer covered hardenConfigDir(), which uses the async runner since e5d5886.
…suites; per-test homes for auth-api and oauth-status Fourteen suites removed their scratch home with a bare rmSync right after server.stop(). Even with stop() now draining the ACL flight, antivirus and indexer handles on a fresh temp dir are real on the hosted image, so every teardown goes through removeTreeWithRetry (EPERM/EBUSY/ENOTEMPTY, bounded). codex-auth-api and oauth-status-privacy also move off fixed repo-local dirs and stub both icacls runners, the same shape as codex-account-store.
…ase rejects Reviewer P2: the flush sat after backgroundLifecycle/native-main release in one finalizer, so a rejected release skipped it and the EPERM/EBUSY window came back on the failure path. The flush now runs in finally; the original rejection still propagates. Regression test mocks a throwing native release and proves stop() stays pending until the flight settles (driven red).
|
✅ Deterministic PR hygiene checks passed. |
|
Owner admin merge, user-authorized ('샤드 그린되면 ... admin 머지하고 릴리즈 force'). Head 1c41988. Evidence: Windows dispatch shards 1-3 green on four consecutive stack-tip runs (33597649234, 33601508392, 33605723635, 33610501053); Linux x4/macOS green on each; two reviewer rounds. |
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. |
|
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 (23)
📝 WalkthroughWalkthroughThe PR documents Windows CI failures and a remediation plan. It adds public config-directory ACL flushing, awaits that flush during server shutdown, adds lifecycle tests, and updates test cleanup to use retries and isolated temporary directories. ChangesWindows CI release hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Server
participant ACLFlush
participant Icacls
Caller->>Server: stop(true)
Server->>Server: release lifecycle resources
Server->>ACLFlush: flushConfigDirHardening(startupConfigDir)
ACLFlush->>Icacls: await active hardening process
Icacls-->>ACLFlush: settle
ACLFlush-->>Server: resolve
Server-->>Caller: stop settles
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 |
![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: 1c41988ada
ℹ️ 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".
| // removes the dir right after stop() settles would hit EPERM/EBUSY on Windows | ||
| // otherwise. Runs even when an earlier release rejected — that rejection still | ||
| // propagates, but not before the child is drained. | ||
| await flushConfigDirHardening(startupConfigDir); |
There was a problem hiding this comment.
When icacls.exe reaches its timeout, this await does not actually guarantee that the child has released the directory: defaultAsyncIcaclsRunner delegates to waitForSubprocessExit, whose timeout branch calls kill() and then resolves immediately without awaiting proc.exited (src/lib/bounded-subprocess.ts:12-34). Consequently, flushConfigDirHardening() can settle and server.stop() can return while the terminating child still holds the config directory, preserving the same EPERM/EBUSY race this change is intended to eliminate. Track the child-exit promise through the hardening flight and have shutdown await actual exit after issuing the kill; the new tests should also cover this timeout branch.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 74 / 80이 PR은 Windows에서 서버를 끈 뒤 설정 폴더를 지우려고 할 때 나오던 EPERM/EBUSY를 고칩니다. 예전에 제품 쪽 핵심은 두 파일입니다. 테스트 쪽은 라인 122 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…test teardown (lidge-jun#3257) * devlog(windows): CI failure inventory and repair roadmap (000-040) * devlog(windows): audit fixes — test filename, win32 platform seam in shutdown test, exact line cites * fix(windows): server.stop drains the config-dir ACL flight it started hardenConfigDir() spawns icacls.exe as a fire-and-forget flight (e5d5886). The child holds the config directory open until it exits, and Windows locks are mandatory, so removing the directory right after a clean server.stop() returned EPERM/EBUSY — which is what every Windows CI shard has hit since 2026-08-30 in the account-store, auth-api and live-server fixtures. flushConfigDirHardening(dir) is now a production function scoped to one directory; startServer captures its config dir before loadConfig and the composite stop() awaits the flight after the listeners close. Regression test drives stop() with a held flight and proves it stays pending until release (driven red before the fix). * test(account-store): per-test scratch home, both icacls runners stubbed, retrying teardown The fixed repo-local .tmp-codex-accounts-test meant one EPERM teardown poisoned every later case (49/49 errors in run 33590540220 were hooks). The sync-only icacls stub also no longer covered hardenConfigDir(), which uses the async runner since e5d5886. * test(windows): retrying scratch-home teardown across the live-server suites; per-test homes for auth-api and oauth-status Fourteen suites removed their scratch home with a bare rmSync right after server.stop(). Even with stop() now draining the ACL flight, antivirus and indexer handles on a fresh temp dir are real on the hosted image, so every teardown goes through removeTreeWithRetry (EPERM/EBUSY/ENOTEMPTY, bounded). codex-auth-api and oauth-status-privacy also move off fixed repo-local dirs and stub both icacls runners, the same shape as codex-account-store. * fix(windows): drain the ACL flight even when an earlier shutdown release rejects Reviewer P2: the flush sat after backgroundLifecycle/native-main release in one finalizer, so a rejected release skipped it and the EPERM/EBUSY window came back on the failure path. The flush now runs in finally; the original rejection still propagates. Regression test mocks a throwing native release and proves stop() stays pending until the flight settles (driven red). --------- Co-authored-by: jun <jun@lidge.dev>
…test teardown (lidge-jun#3257) * devlog(windows): CI failure inventory and repair roadmap (000-040) * devlog(windows): audit fixes — test filename, win32 platform seam in shutdown test, exact line cites * fix(windows): server.stop drains the config-dir ACL flight it started hardenConfigDir() spawns icacls.exe as a fire-and-forget flight (e2f4af3). The child holds the config directory open until it exits, and Windows locks are mandatory, so removing the directory right after a clean server.stop() returned EPERM/EBUSY — which is what every Windows CI shard has hit since 2026-08-30 in the account-store, auth-api and live-server fixtures. flushConfigDirHardening(dir) is now a production function scoped to one directory; startServer captures its config dir before loadConfig and the composite stop() awaits the flight after the listeners close. Regression test drives stop() with a held flight and proves it stays pending until release (driven red before the fix). * test(account-store): per-test scratch home, both icacls runners stubbed, retrying teardown The fixed repo-local .tmp-codex-accounts-test meant one EPERM teardown poisoned every later case (49/49 errors in run 33590540220 were hooks). The sync-only icacls stub also no longer covered hardenConfigDir(), which uses the async runner since e2f4af3. * test(windows): retrying scratch-home teardown across the live-server suites; per-test homes for auth-api and oauth-status Fourteen suites removed their scratch home with a bare rmSync right after server.stop(). Even with stop() now draining the ACL flight, antivirus and indexer handles on a fresh temp dir are real on the hosted image, so every teardown goes through removeTreeWithRetry (EPERM/EBUSY/ENOTEMPTY, bounded). codex-auth-api and oauth-status-privacy also move off fixed repo-local dirs and stub both icacls runners, the same shape as codex-account-store. * fix(windows): drain the ACL flight even when an earlier shutdown release rejects Reviewer P2: the flush sat after backgroundLifecycle/native-main release in one finalizer, so a rejected release skipped it and the EPERM/EBUSY window came back on the failure path. The flush now runs in finally; the original rejection still propagates. Regression test mocks a throwing native release and proves stop() stays pending until the flight settles (driven red). --------- Co-authored-by: jun <jun@lidge.dev>
Summary
server.stop()now drains the config-directory ACL harden it started. Since e5d5886hardenConfigDir()spawnsicacls.exeas a fire-and-forget flight; the child holds the directory open, Windows locking is mandatory, and anything that removed the directory after a "clean" stop got EPERM/EBUSY. That is what turned every Windows CI shard red on every branch (incl.main) since 2026-08-30.flushConfigDirHardening(dir)is a production function scoped to one directory;startServercaptures its dir beforeloadConfigand the compositestop()awaits it in afinallyso an earlier release rejection still drains the child.hardenConfigDir); 14 live-server suites remove their scratch home viaremoveTreeWithRetry.async-child-holds-dir-after-stop(lidge-jun/fuck-powershell 56e1801) records the landmine.Plan:
devlog/_plan/260902_windows_ci_release/{000,010,011}.Stack
codex/win-acl-lifecycledevcodex/win-fsync-fixturescodex/win-acl-lifecycleVerification
tests/server-stop-config-hardening.test.ts(4): stop() pending until the held flight settles; rejected native release still drains and propagates its own error; both driven red before the fix.bun run typecheck,bun run privacy:scangreen.Checklist
Summary by CodeRabbit
Bug Fixes
EPERMandEBUSYerrors when removing configuration or temporary directories after stopping the application.Tests