fix(test): contain and reclaim Windows temporary roots (#4785, #4789) - #4796
Merged
Merged
Conversation
Co-authored-by: Valerio Coltre <colthreepv@gmail.com> (cherry picked from commit 8e9e443)
… 2.5 seconds The suite's one removal path waited a flat 50 attempts x 50ms, so the documented icacls release race in `src/config/paths.ts` got 2.5 seconds and no more. That budget was tuned on a lightly loaded machine: under six concurrent Windows shards it is exceeded, and the helper rethrows the EPERM it exists to absorb, failing a test that had already finished asserting. Two dispatches of the same lane with a byte-identical helper disagreed, which is what makes this load and not code. The schedule now grows -- 50ms, 100ms, 200ms, then capped at 250ms -- until a 15 second budget is spent. A removal that succeeds never sleeps, and the first retry still lands at 50ms, so nothing on the passing path gets slower; only the tail that used to fail now waits longer. The policy lives in `scripts/test-temp` so the wrapper's own cleanup and every fixture teardown share one schedule rather than two copies that drift. `removeTestTempTree` joins the destructive-call list the source oracle scans, so the new shared removal helper is inside the guard that exists because a real home was lost on 2026-09-15. Closes #4789
The carried change reclaimed a stale root on a name match: a directory called `opencodex-test-XXXXXX` or `ocx-<name>-XXXXXX` that was old enough, contained directly in TEMP and free of links was removed whether or not it carried the ownership marker this code writes. That is the wrong side of the line. Every one of the thousands of directories already accumulated on a user's machine was written by a version that stamped nothing, so a name match is exactly the rule that turns "reclaim what we left behind" into "delete a TEMP tree we cannot show we created". An absent marker is now as disqualifying as a corrupt one, and the owning pid must be dead rather than merely unknown. Reclamation therefore applies to roots this release and later stamp; the existing accumulation is scanned, skipped, and left for the user to clear, which is what the release intends -- it changes future runs. The broad `ocx-*` candidate class goes with it. Those roots never carried a marker, so under the marker requirement they could only ever be scanned and skipped, and the regex wide enough to match them was also wide enough to put an unrelated tool's directory on the candidate list. Recovery gains a liveness seam so the tests prove the dead-owner and live-owner branches instead of depending on which pids the host happens to have. Co-authored-by: Valerio Coltre <colthreepv@gmail.com>
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 (8)
📝 WalkthroughWalkthroughThe test system now contains temporary files under each test root, records root ownership, retries Windows cleanup with bounded backoff, recovers eligible stale roots, and validates cleanup across wrapper, preload, parallel, and direct test runs. ChangesTest temporary storage lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: High Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant TestTemp
participant ChildProcess
participant Filesystem
TestRunner->>TestTemp: recover stale roots once
TestRunner->>Filesystem: create owned test root and root/tmp
TestTemp->>Filesystem: write owner marker
TestRunner->>ChildProcess: pass TEMP, TMP, TMPDIR
ChildProcess->>Filesystem: create temporary artifacts
TestRunner->>TestTemp: remove test root
TestTemp->>Filesystem: retry removal on transient Windows errors
✨ 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. |
3 of 7 tasks
The ownership marker stores `realpathSync(root)` and recovery compares it against `realpathSync(candidate)`, so both halves of the reclamation decision speak the canonical path. The assertion did not: it held the path `mkdtempSync` returned. On Linux and Windows those are the same string, which is why this only surfaced on the macOS shard, where `tmpdir()` hands back /var/folders/... and the real path is /private/var/folders/.... The writer is the side that is right. Resolving on both ends is what lets a run recognise a root it created through a symlinked ancestor, and dropping the resolution to match the test would have reintroduced exactly the mismatch the comparison exists to avoid. The extra `createdAtMs` in the received object was never the failure -- `toMatchObject` permits extra keys, and every non-macOS shard passed this test with that field present. It is now asserted explicitly rather than left implied.
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: 0476f7d02c
ℹ️ 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".
|
|
||
| let names: string[]; | ||
| try { | ||
| names = readdirSync(tempRoot).sort(); |
There was a problem hiding this comment.
On Windows, readdirSync(tempRoot).sort() materializes and sorts every entry before either maxCandidates or maxDurationMs is checked. A heavily accumulated TEMP—the exact environment this recovery targets—can therefore consume unbounded memory and stall test startup beyond the advertised 30-second limit. Iterate the directory incrementally and stop when the deadline or candidate budget is reached.
AGENTS.md reference: scripts/AGENTS.md:L14-L15
Useful? React with 👍 / 👎.
This was referenced Sep 16, 2026
Closed
lidge-jun
added a commit
that referenced
this pull request
Sep 16, 2026
…uth home
Windows shard 1/6 of run 35108652486 threw out of this file's afterEach:
error: EPERM: operation not permitted, rm
...\Temp\opencodex-test-WXhR6c\tmp\ocx-management-auth-fDchUb
at removeTestTempTree (scripts/test-temp.ts:198)
at removeTreeWithRetry (tests/helpers/remove-tree.ts:27)
at tests/server/server-management-auth.test.ts:206
It is a leaked handle, not a timing race, and the budget was never the
answer. hardenConfigDir() spawns icacls.exe, which holds the directory
open until it exits, and Windows file locking is mandatory. #4789 filed
this exact failure at this exact line when the retry budget was 2.5s;
#4796 raised it to a 15s exponential schedule, and this run exhausted all
15 seconds and still got EPERM. Six times the wait changed nothing,
because the handle does not close on the remover's schedule. The process
that started the child has to wait for it.
src/config/paths.ts already documents this and already ships the wait.
The afterEach now awaits flushConfigDirHardeningForTests() first. Twenty
other test files do the same; this one starts servers and drives the ACL
path hardest and did not.
The all-directories variant is the required one. server.stop already
flushes, but through flushConfigDirHardening(), which defaults to
getConfigDir() read at stop time - and this hook restores OPENCODEX_HOME
to the real home a few lines before the removal, so a directory-scoped
flush would settle the wrong tree and leave this one held.
Three things deliberately unchanged. The 15s budget stays: a wider retry
would be treating the symptom, and this run proves it does not work
anyway. The fake icacls runners this file installs are all synchronous,
so awaiting the flush cannot hang on one. And removeTestTempTree keeps
throwing rather than reporting and continuing - that throw is what
surfaced the leak, and softening it would re-hide exactly the class of
defect the batch-runner change stopped hiding this morning.
79 other test files start a server, remove a tree in a cleanup hook, and
never flush. None has failed this way, because this is the file that
drives icacls deliberately, so they are left alone rather than edited
blind. The general guard belongs in the removal helper and is a separate
change.
lidge-jun
added a commit
that referenced
this pull request
Sep 16, 2026
…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.
3 tasks done
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
lidge-jun#4789) (lidge-jun#4796) Maintainer integration for the 2.57.0 stabilization scope. The exact head has a green aggregate ci check with no failing job. Carries lidge-jun#4785 and folds lidge-jun#4789. The containment half closes the leak in lidge-jun#4762 for future runs. The reclamation half needed correcting before it could ship: as authored, an absent ownership marker fell through to removal, and every directory users have accumulated today was written by a version that stamped nothing, so the rule would have deleted TEMP trees the tool cannot show it created. Reclamation now treats a missing marker as disqualifying and requires the owning pid to be dead. An already-affected workstation is not cleaned by this change; those roots are scanned, skipped and left to the user. Host-owned merge decision; no local suite, 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
2 participants
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
Carries #4785 onto the current dev head and adds two fixes the review found.
The containment half of #4785 is the real fix for #4762: the test runner routes TEMP, TMP and TMPDIR into a subtree of a run-owned root, stamps that root, and moves cleanup to afterAll because Bun workers do not reliably run exit handlers. That closes the leak for future runs.
The reclamation half needed correcting. As authored, a stale root was removed on a name match, and an absent ownership marker fell through to removal, so the directories users have accumulated today, all written by versions that stamped nothing, were exactly the ones it would delete. Reclamation now treats a missing marker as disqualifying, requires the owning pid to be dead rather than merely unknown, and drops the broad ocx-* candidate class whose pattern was wide enough to shortlist an unrelated tool's directory.
This also folds #4789. The flat 50 attempts at 50 ms gave the documented icacls race 2.5 seconds; the retry now backs off to a 15-second budget. A removal that succeeds never sleeps and the first retry still lands at 50 ms, so the passing path keeps its timing and only the tail that previously threw waits longer.
An already-affected workstation is not cleaned by this change. Those roots are scanned, skipped, and left to the user.
Closes #4762
Closes #4789
Verification
Checklist
Summary by CodeRabbit
Bug Fixes
Tests