fix(lib): wait for a killed subprocess to die before abandoning it - #4825
Merged
Merged
Conversation
…ing it Windows shard 1/6 has now failed three times in the same afterEach, and each fix aimed at the wrong thing. #4789 blamed the removal retry budget and asked for more than 2.5s. #4796 gave it a 15s exponential schedule. My last commit awaited the config-dir hardening flight from the hook. Run 35108652486 failed through all three, burning the whole 15s budget and still throwing EPERM on ocx-management-auth-fDchUb, and run 35118018849 failed identically at the same call site. None of them could have worked, because the directory was held by a live process and none of them made it exit. waitForSubprocessExit killed at the deadline and resolved in the same tick: try { proc.kill(); } catch {} try { proc.unref?.(); } catch {} finish({ exitCode: null, timedOut: true }); kill() only REQUESTS termination. It returns before the kernel has torn the process down, and every handle that process holds stays held until it does. On Windows file locking is mandatory, so a directory an abandoned icacls.exe still has open cannot be removed by anyone - the removal fails with EPERM rather than waiting, which is why more waiting never helped. This also made a documented contract false. flushConfigDirHardening exists so shutdown owns every icacls.exe it started; it awaited a promise that had already settled while the child was still alive, so the contract read as satisfied and the tree stayed locked. Only the async path can leak this way - spawnSync waits for its child by definition - which is why the sync hardening callers were never implicated. The deadline now kills the child and waits for it to actually be reaped, bounded by a 2s grace, after which abandonment is still the fallback so a genuinely unkillable child cannot hang shutdown. The classification does not move: a child that missed its deadline is reported timed out whether or not it dies during the grace, because it did time out, and hardenSecretPath keys its ETIMEDOUT memo on exactly that flag. Only the moment of resolution changes. awaitAsyncIcaclsRunner had to move with it. Its belt fired at exactly timeoutMs, so it would have resolved while the new grace was still running and reintroduced the abandonment one layer up. It now outlasts the runner it guards. windows-user-principal.ts shares the helper, so an abandoned PowerShell SID lookup is fixed by the same change. tests/lib/bounded-subprocess.test.ts drives the contract with a fake subprocess rather than a real one: what matters is WHEN the promise resolves relative to the child dying, and that is observable without spawning anything, on every platform, deterministically. The case that would have caught this asserts the promise is still pending after kill. I could not execute any of this, so the behaviour is verified by transliterating the helper and running its eight cases outside the repo; hosted Windows CI is the real validator. The existing ACL tests all install fake async runners, so none of them reaches the real spawn path or changes timing.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change adds bounded subprocess termination, tracks native startup releases for teardown, adjusts Windows cleanup timing, stabilizes integration tests, and adds coverage for subprocess exit timing. ChangesReliability and cleanup updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant awaitAsyncIcaclsRunner
participant waitForSubprocessExit
participant ChildProcess
awaitAsyncIcaclsRunner->>waitForSubprocessExit: Wait with timeout and kill grace
waitForSubprocessExit->>ChildProcess: Kill on deadline
ChildProcess-->>waitForSubprocessExit: Exit or rejection
waitForSubprocessExit-->>awaitAsyncIcaclsRunner: Return timed-out result
✨ 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
lidge-jun
force-pushed
the
codex/r2570-win-cleanup-handle
branch
from
September 16, 2026 16:22
d68c70e to
78ba06f
Compare
…leased port Two failures from dispatch 35121570658. One is mine; the other is not, and the difference matters for what to do about it. Layout. I put the new helper test in tests/lib/ and registered it as lib, but `^bounded-` is a seed for the server domain, so the basename and the explicit entry disagreed and test-layout-tooling flagged it. The code under test is src/lib/bounded-subprocess.ts, so lib is the right home and the name is what had to move: stall-subprocess-exit.test.ts, which the lib seed `^stall-` already claims. Seed and explicit now agree, and the layout.json and fixture entries stay byte-identical. cli-status-json. This is not the subprocess change. waitForSubprocessExit is reached only from windows-secret-acl.ts and windows-user-principal.ts, both gated on platform win32, and this failed on Linux. The same commit passed this test in PR run 35121524520 on the identical head; only the dispatch lost it. What my commit did contribute is a new test file, which shifts the Linux round-robin batch composition and re-rolled the dice on a race that was already there. Status did not lose a signal and the new behaviour is not "a pid that is genuinely gone". probeUncleanExitState reports a stale record only when the recorded port REFUSES (status-probes.ts:148-163), and allocateFreePort hands back a port it has already released. This case spawns the CLI twice, so it was exposed for the whole gap between the two runs. The --json run saw the refusal and reported true; the human run a moment later found the port answering and correctly said nothing about a previous exit. Both reports were right. The fixture was asserting against a port it no longer owned and read a correct report as a lost signal. The fallback-port case one block below already documents this exact hazard and guards it by confirming refusal before and after the probe and re-allocating when something takes the port. This case now does the same. That re-establishes a precondition rather than retrying an assertion: a stolen port repeats the setup, and a genuine regression still fails, because the loop only accepts a run where the port refused throughout. Not touched: macos 1/2 also failed tests/server/server-auth.test.ts:4264 with "fixture upstream connection reset". That one is pre-existing and unrelated - it appears on unrelated heads, including PR run 35102729176 on 320ece6 and dispatch 35093667426 on 89bdf5f, neither of which carries any of this work. I have no diagnosis for it and did not guess.
…he kill grace on startup
Dispatch 35124906412 left three annotations. One is not a failure at all,
and the other two are budgets that the measured data says cannot hold.
server-auth.test.ts:4264 is noise. The test that owns that line,
"native passthrough upstream reset still logs 502 and penalizes the
pool", PASSED in 1120ms. Line 4264 is the case deliberately injecting
`new Error("fixture upstream connection reset")` into its fixture stream;
Bun prints it and the annotations API reports it as a failure. That is
why it keeps appearing on unrelated heads. Nothing to fix, and nothing
changed for it here.
codex-composed-acceptance B-reduced was not a stream-draining artifact.
The diagnostics added for exactly this said `child exit=null;
pid-record=missing; runtime-record=missing` with both streams open and
zero bytes ever captured. The banner is printed after startup completes,
so a child that has not finished starting has produced no output by
definition: this is a child still starting, not a wedged one, which is
the distinction those diagnostics exist to draw.
The budget is the defect. On one shard of that run this file's passing
cases measured 5.0s, 7.4s, 8.1s, 10.7s, 14.8s and 38.8s against a 45s
watchdog. The slowest healthy startup consumed 86% of the bound meant to
catch a hang. The child-start watchdog now takes 120s on CI, roughly
three times the slowest healthy start, and the per-case ceiling already
in place (CASE_TIMEOUT_MS, 150s on CI) still exceeds it, so the watchdog
keeps reporting first and the diagnostics survive. The 150s ceiling was
never the constraint; the 45s floor was. Local runs keep the short
watchdog, because this is a property of the loaded six-shard Windows leg
and not of the code.
codex-log-guard-maintenance reclaim is the same shape. It builds a real
SQLite log, fragments it and incrementally vacuums it, and measured 1.6s,
20.2s and 67.0s across three Windows runs - past the suite-wide 60s
ceiling while its siblings in the same file finished in 0.4s to 5.0s. The
case gets its own 180s budget for that spread.
And a regression of mine, found while reading the above. The kill grace I
added waits up to 2s after killing a stalled child, which is right where
a handle must be released before a directory is removed, and wrong on the
startup path: windows-user-principal's PowerShell lookup holds no path
anyone deletes, and it runs inside the very startups measured at 38.8s
above. It now passes a zero grace and abandons immediately, as before.
windows-secret-acl keeps the grace, which is the caller the EPERM fix was
for. The helper treats a non-positive grace as explicit opt-out rather
than clamping it to a millisecond.
Verified by transliterating the amended helper and running its cases
outside the repo, including the new opt-out; hosted CI remains the
validator.
Enumerating what server-management-auth leaves under its home found one close that nothing can wait for. A native-main release closes the owner's SQLite lease and its stable lock file, both of which live under CODEX_HOME - which in this file is the directory afterEach then removes. The normal path is sound: server.stop goes through releaseNativeMainStartupLifecycle, which awaits the flight, and then awaits flushConfigDirHardening. The FAILED-start path is not. startServer is synchronous by contract, so its rollback can only fire `void nativeMainLifecycle.release()` and rethrow (src/server/index.ts:757). Nothing could then wait for those handles, and on Windows an open handle does not delay an unlink, it refuses it outright with EPERM. That is invisible in production, where a failed start is followed by exit rather than by deleting the home. It is exactly visible to a test whose cleanup removes the home it just used. The flight is now tracked where it is created rather than at the call site, so startServer stays synchronous and is not touched at all - it remains byte-identical at its 893-line ratchet. Any caller that cannot await, present or future, still leaves the release drainable through the new flushNativeMainStartupReleases(), which loops rather than awaiting a single snapshot because a release can retire an owner whose own teardown starts another. The management-auth afterEach awaits it before removing. What I ruled out while looking. The success path awaits its release and its hardening flush. The other two voided calls in that rollback, index.ts:729 and :748, stop Bun listeners - those hold sockets, not files under the home. The removal helper and the subprocess teardown were the previous three attempts and are not the problem; the host was right that hardening the remover further was never going to reach this. Stated plainly: I have the gap, not a proof that it fired in this run. The job log for dispatch 35129771091 had not been published while the run was still in progress, so I could not read back which case failed. What makes this the likely trigger is the shape of the evidence rather than a log line: two cases in this file bind a management ingress on the fixed port 10101, which nine other test files also use, so a collision on the six-shard Windows leg converts a passing start into the rollback path. A trigger that lives in another shard is what "same file, same line, shard 1 then 2 then 3, every other shard green" looks like. No regression test for this specific path. Constructing a failed start means injecting into a delicate ownership module, and I cannot execute anything to check that such a test passes; adding an unverifiable test to a stabilisation branch is worse than adding none. The flush is exercised by every case in this file, and hosted CI is the validator.
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. |
![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: 1504caaa83
ℹ️ 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".
Comment on lines
375
to
+377
| timer = setTimeout( | ||
| () => finish({ success: false, exitCode: null, timedOut: true, stdout: "" }), | ||
| Math.max(1, timeoutMs), | ||
| Math.max(1, timeoutMs) + SUBPROCESS_KILL_GRACE_MS + ASYNC_ICACLS_BELT_MARGIN_MS, |
There was a problem hiding this comment.
If the event loop is blocked until both timers are overdue, waitForSubprocessExit first runs its deadline callback and starts a fresh two-second grace timer, but this already-overdue outer belt can then resolve immediately while icacls.exe is still alive. The caller may consequently remove the protected directory before the child releases its handle, reproducing the Windows EPERM race this change is intended to fix. Start the belt relative to the actual kill callback, or otherwise coordinate it with the inner settlement rather than adding a fixed margin to the original deadline.
Useful? React with 👍 / 👎.
| /** | ||
| * How long to wait for a killed child to actually be reaped before abandoning it. | ||
| * | ||
| * `kill()` only REQUESTS termination. It returns before the kernel has torn the process down, and |
There was a problem hiding this comment.
This changes subprocess timeout semantics in src/lib/ and native-main lifecycle behavior in src/codex/, but the commit updates none of the structure documents that own those areas. Update the documents mapped to these source areas so the documented lifecycle and teardown invariants remain synchronized with the implementation.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
This was referenced Sep 16, 2026
lidge-jun
added a commit
that referenced
this pull request
Sep 16, 2026
…r-the-fact dispatch (#4840) The release record claimed the candidate run covered all six Windows shards. It did not: platform-windows is dispatch-only, so both the candidate push run 35131181996 and the release-SHA push run 35133242171 skipped it, and the aggregate ci check accepts a skipped producer as a pass. The green Windows evidence that existed belonged to 1504caa, the #4825 lane head, not to the commit 2.57.0 was published from. Dispatch 35139132889 has now run lane=all at 44de45d, the exact published SHA, and all six shards passed individually. The release is sound; the record was not. No local suite, typecheck, build, or install was run.
5 tasks done
lidge-jun
added a commit
that referenced
this pull request
Sep 17, 2026
…it holds Windows has been failing one shard per lane=all dispatch for weeks, and the shard number kept moving while the case family did not. The repository already wrote the right conclusion down once - the blame moved, the cause did not - and #4825 was the attempt at it. It did not land the fix. #4825 replaced "kill and resolve in the same tick" with "kill, wait up to two seconds, then abandon". That is the same false ownership contract on a slower clock. When the grace expired the caller was told its child was gone, cleanup removed the directory, and Windows returned EPERM because icacls still held it: EPERM: operation not permitted, rm '...\Temp\opencodex-test-vwyZZv\tmp\ocx-management-auth-kgvXCT' (fail) codex app-server restart routes ride the management gate > the data-plane token does not authorize the restart route [15367.90ms] A handle-bearing caller now has no second deadline after the kill. The child's actual exit is the only release signal, because it is the only thing that is true. Only the icacls runner takes that path; windows-user-principal passes 0 and still abandons immediately, since its PowerShell lookup sits on the startup critical path and holds no path anyone removes. The icacls runner's own outer watchdog still bounds the caller, so nothing can wait forever - the reap continues in the background and teardown drains it through flushRequiredSubprocessReapsForTests. The regression test no longer sleeps. A manual deadline seam orders kill against exit directly, so it proves the sequence rather than waiting for it. The Desktop copy-coherence probes are the same mechanism seen from the other end. Those scenarios perform different numbers of secure writes, each able to launch PowerShell or icacls on Windows, and that contention was spending a deadline the probe shares. The generated child now installs the existing synthetic principal and ACL runner seams before it loads any client module. Files, SQLite, locking, rotation, recovery and Desktop projection stay real; only the ACL subprocesses become synthetic, and an ACL descendant can no longer outlive the parent's kill holding a temp path. Causation was checked before writing any of this. Across 48 lane=all dispatches that completed all six Windows shards without PR #4836, the EPERM class appears in 3 and the probe-timeout class in 1, so both predate that PR and neither is attributable to it. No local suite, focused test, typecheck, build, or install was run.
lidge-jun
added a commit
that referenced
this pull request
Sep 17, 2026
…it holds (#4849) * fix(lib): let a killed child actually die before anyone deletes what it holds Windows has been failing one shard per lane=all dispatch for weeks, and the shard number kept moving while the case family did not. The repository already wrote the right conclusion down once - the blame moved, the cause did not - and #4825 was the attempt at it. It did not land the fix. #4825 replaced "kill and resolve in the same tick" with "kill, wait up to two seconds, then abandon". That is the same false ownership contract on a slower clock. When the grace expired the caller was told its child was gone, cleanup removed the directory, and Windows returned EPERM because icacls still held it: EPERM: operation not permitted, rm '...\Temp\opencodex-test-vwyZZv\tmp\ocx-management-auth-kgvXCT' (fail) codex app-server restart routes ride the management gate > the data-plane token does not authorize the restart route [15367.90ms] A handle-bearing caller now has no second deadline after the kill. The child's actual exit is the only release signal, because it is the only thing that is true. Only the icacls runner takes that path; windows-user-principal passes 0 and still abandons immediately, since its PowerShell lookup sits on the startup critical path and holds no path anyone removes. The icacls runner's own outer watchdog still bounds the caller, so nothing can wait forever - the reap continues in the background and teardown drains it through flushRequiredSubprocessReapsForTests. The regression test no longer sleeps. A manual deadline seam orders kill against exit directly, so it proves the sequence rather than waiting for it. The Desktop copy-coherence probes are the same mechanism seen from the other end. Those scenarios perform different numbers of secure writes, each able to launch PowerShell or icacls on Windows, and that contention was spending a deadline the probe shares. The generated child now installs the existing synthetic principal and ACL runner seams before it loads any client module. Files, SQLite, locking, rotation, recovery and Desktop projection stay real; only the ACL subprocesses become synthetic, and an ACL descendant can no longer outlive the parent's kill holding a temp path. Causation was checked before writing any of this. Across 48 lane=all dispatches that completed all six Windows shards without PR #4836, the EPERM class appears in 3 and the probe-timeout class in 1, so both predate that PR and neither is attributable to it. No local suite, focused test, typecheck, build, or install was run. * fix(windows): separate "may I stop waiting" from "is it safe to delete" The first attempt made `waitForSubprocessExit` wait for a killed child to really exit, and Windows still failed the same way. The abandonment had simply moved up a layer. `awaitAsyncIcaclsRunner`'s belt fires at timeoutMs + kill grace + 250ms. That margin was sized against the old bounded 2-second grace, so once the inner wait became unbounded the belt was once again the deadline: on a stalled icacls it resolved timedOut while the child was alive, the caller logged "continuing without it", and teardown deleted a directory icacls.exe still held. [opencodex] ACL hardening timed out (ETIMEDOUT) - transient icacls stall EPERM: operation not permitted, rm '...\tmp\ocx-management-auth-Ge98cW' (fail) codex app-server restart routes ride the management gate > both routes reject an unauthenticated caller and a cross-origin caller [15261.56ms] The 15.26s there is removeTreeWithRetry's ladder exhausting, not a probe deadline. These are two different questions and they now have two different answers. The belt still releases its caller, so a genuinely stuck child cannot hang startup, shutdown or uninstall - that bound is the whole reason the belt exists. But when it gives up it registers the outstanding reap against the target path, and code that is about to REMOVE something must settle that separately: a test tree awaits flushWindowsSecretAclReapsBeforeRemoval, the async writer leaves a residual temp rather than racing an unlink against a live handle, and uninstall refuses promptly rather than waiting, because handing a stuck child the power to hang `ocx uninstall` would give back the bound we just protected. The holder was confirmed rather than assumed: the failing proxy is in-process and auth rejection stops the restart child from spawning, so icacls.exe is the only child holding the fixture path. removeTreeWithRetry stays. It is a fair accommodation of a short filesystem release race; it should just never have been absorbing a live child. No local suite, focused test, typecheck, build, or install was run.
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…idge-jun#4825) Last release blocker for 2.57.0. The PR aggregate ci check is green at this exact head, and the lane=all dispatch 35134620067 on the same head passed all 24 real jobs including every one of the six Windows shards individually, with only the optional macos control job outstanding. Windows shard failures had moved between shards across five dispatches while always landing on the same management-auth cleanup, which is what said the shard number was noise and the held handle was the defect. The chain was a killed subprocess abandoned before it died, then two real Windows timeouts being paid on the startup path, then a failed-start native-main release that was not awaitable. All three are fixed at the cause. Host-owned merge decision; no local suite, typecheck, build, or install was run.
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…r-the-fact dispatch (lidge-jun#4840) The release record claimed the candidate run covered all six Windows shards. It did not: platform-windows is dispatch-only, so both the candidate push run 35131181996 and the release-SHA push run 35133242171 skipped it, and the aggregate ci check accepts a skipped producer as a pass. The green Windows evidence that existed belonged to 1504caa, the lidge-jun#4825 lane head, not to the commit 2.57.0 was published from. Dispatch 35139132889 has now run lane=all at 44de45d, the exact published SHA, and all six shards passed individually. The release is sound; the record was not. No local suite, typecheck, build, or install was run.
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…it holds (lidge-jun#4849) * fix(lib): let a killed child actually die before anyone deletes what it holds Windows has been failing one shard per lane=all dispatch for weeks, and the shard number kept moving while the case family did not. The repository already wrote the right conclusion down once - the blame moved, the cause did not - and lidge-jun#4825 was the attempt at it. It did not land the fix. lidge-jun#4825 replaced "kill and resolve in the same tick" with "kill, wait up to two seconds, then abandon". That is the same false ownership contract on a slower clock. When the grace expired the caller was told its child was gone, cleanup removed the directory, and Windows returned EPERM because icacls still held it: EPERM: operation not permitted, rm '...\Temp\opencodex-test-vwyZZv\tmp\ocx-management-auth-kgvXCT' (fail) codex app-server restart routes ride the management gate > the data-plane token does not authorize the restart route [15367.90ms] A handle-bearing caller now has no second deadline after the kill. The child's actual exit is the only release signal, because it is the only thing that is true. Only the icacls runner takes that path; windows-user-principal passes 0 and still abandons immediately, since its PowerShell lookup sits on the startup critical path and holds no path anyone removes. The icacls runner's own outer watchdog still bounds the caller, so nothing can wait forever - the reap continues in the background and teardown drains it through flushRequiredSubprocessReapsForTests. The regression test no longer sleeps. A manual deadline seam orders kill against exit directly, so it proves the sequence rather than waiting for it. The Desktop copy-coherence probes are the same mechanism seen from the other end. Those scenarios perform different numbers of secure writes, each able to launch PowerShell or icacls on Windows, and that contention was spending a deadline the probe shares. The generated child now installs the existing synthetic principal and ACL runner seams before it loads any client module. Files, SQLite, locking, rotation, recovery and Desktop projection stay real; only the ACL subprocesses become synthetic, and an ACL descendant can no longer outlive the parent's kill holding a temp path. Causation was checked before writing any of this. Across 48 lane=all dispatches that completed all six Windows shards without PR lidge-jun#4836, the EPERM class appears in 3 and the probe-timeout class in 1, so both predate that PR and neither is attributable to it. No local suite, focused test, typecheck, build, or install was run. * fix(windows): separate "may I stop waiting" from "is it safe to delete" The first attempt made `waitForSubprocessExit` wait for a killed child to really exit, and Windows still failed the same way. The abandonment had simply moved up a layer. `awaitAsyncIcaclsRunner`'s belt fires at timeoutMs + kill grace + 250ms. That margin was sized against the old bounded 2-second grace, so once the inner wait became unbounded the belt was once again the deadline: on a stalled icacls it resolved timedOut while the child was alive, the caller logged "continuing without it", and teardown deleted a directory icacls.exe still held. [opencodex] ACL hardening timed out (ETIMEDOUT) - transient icacls stall EPERM: operation not permitted, rm '...\tmp\ocx-management-auth-Ge98cW' (fail) codex app-server restart routes ride the management gate > both routes reject an unauthenticated caller and a cross-origin caller [15261.56ms] The 15.26s there is removeTreeWithRetry's ladder exhausting, not a probe deadline. These are two different questions and they now have two different answers. The belt still releases its caller, so a genuinely stuck child cannot hang startup, shutdown or uninstall - that bound is the whole reason the belt exists. But when it gives up it registers the outstanding reap against the target path, and code that is about to REMOVE something must settle that separately: a test tree awaits flushWindowsSecretAclReapsBeforeRemoval, the async writer leaves a residual temp rather than racing an unlink against a live handle, and uninstall refuses promptly rather than waiting, because handing a stuck child the power to hang `ocx uninstall` would give back the bound we just protected. The holder was confirmed rather than assumed: the failing proxy is in-process and auth rejection stops the restart child from spawning, so icacls.exe is the only child holding the fixture path. removeTreeWithRetry stays. It is a fair accommodation of a short filesystem release race; it should just never have been absorbing a live child. No local suite, focused test, typecheck, build, or install was run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Windows shard 1/6 failed the release candidate twice while every other shard passed, both times removing a test-owned home with EPERM. The first fix awaited an icacls child and the failure moved seventeen lines, which said the tree was still held rather than that one child was unawaited.
The cause is a step earlier: a subprocess that was killed was abandoned without waiting for it to actually die, so on Windows the handle outlived the kill and the directory could not be removed. Killing is a request, not a completion. The process is now awaited after the kill before anything relies on its handles being gone.
This only became visible because the runtime-crash masking was removed in #4821: the same shard had been dying in Bun's module loader and being swept into a green report.
Verification
lane=alldispatch with all six Windows shards green is the gate for this PR.Checklist
lane=alldispatch with all six Windows shards passing on their own.Summary by CodeRabbit