Repository navigation
Conversation
Three Windows fixture teardowns exhausted the removal retry budget (#7515). Their lock diagnostics show what blocked removal: in two of them the native-main owner or claim lock file under the home had an empty DACL. Real async icacls had rewritten it in suites that assert nothing about ACLs. Rename still worked, but recursive rm got EPERM. - codex-auth-context: the suite meant to keep real icacls out, but it stubbed only the sync runner, and the claim lock is hardened through the async one. Stub both, and tear down with drainAndRemoveFixtureRoots in place of the blocking remover, which never waited for ACL children. - server-management-auth: stub the async runner for the suite; the stub stays installed until removal ends. - main-account-hard-lock-fallback-routing: two cases reached removal with the config-dir ACL harden still in flight. Use the shared drain teardown. - fixture-teardown: before removing a root, refuse it and name any native-main owner still holding its SQLite lock below it, via the new nativeMainOwnerHomesOpenAtOrBelow probe. The teardown now fails at once, without spending the 15 s retry budget on EBUSY. No retry count, sleep, or timeout changes. Refs #7515
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: waiting for Windows ACL work and native owner locks before fixture removal. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
github-actions
Bot
added
the
chore
![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: 05772674fd
ℹ️ 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".
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. |
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed all six changed files at 05772674fd0c277b39969377ad15366abf1ff7cd. Waiting for fixture-owned ACL work and retaining per-root failure handling are improvements, but the new owner probe has a reproducible preparation gap.
src/codex/native-main-owner.ts:315: prepareOwnerDatabase opens a StableLockFile descriptor and SQLite database, then awaits hardenPath. Until that await completes, both handles are local variables and neither entry.file nor entry.database is published. nativeMainOwnerHomesOpenAtOrBelow skips that entry, so assertNoOpenLocks can permit removal even though the process still owns handles.
Independent Linux probe on this exact head, using only an owned temporary home and the existing injected hardenPath seam: pause the hardener, count actual /proc/self/fd targets beneath that home, then query the new export. Result: 2 open owned descriptors, 0 detected owners. The probe resumes the hardener and awaits owner release before cleanup; it never tries to delete an open Windows database. Please track preparation/in-flight handles, or conservatively treat that acquiring state as open, including detached entries being released. Add a delayed-hardener regression that verifies teardown preserves the root and that it becomes removable only after the preparation/release finishes. The existing new test waits for held, so it cannot detect this gap.
Also update the owning structure/ops/test-sandbox-cleanup.md paragraph: it currently says ACL drain proceeds to root removal and omits the new owner-lock check, per-root preservation and immediate owner diagnostic. This is a contract change even though production ACL behavior is unchanged.
Independent bounded Bun 1.4.3 / Linux batch of fixture-teardown-helper, codex-auth-context, main-account-hard-lock-fallback-routing and server-management-auth: 171 passed / 0 failed, 821 assertions. That passing batch does not cover the demonstrated preparation state. The manually dispatched native lane remains in progress and ordinary Cross-platform CI is cancelled at this snapshot; neither is reported as passed. No retry/timeout increase, production config, service or real native client change was performed here. #7515's unproven empty-DACL interleaving and third pattern must not be declared resolved by these receipts.
The teardown probe saw only published handles. While prepareOwnerDatabase awaits its hardener, the stable descriptor and SQLite database live in locals, so the probe reported no owner and teardown could remove a home the process still held. Treat an in-flight acquisition, a pending retry timer and any detached (closing) entry as open. Regression: a real owner with its hardener paused keeps its root through acquisition and release, and the root is removed only once release ends. Document the owner check in structure/ops/test-sandbox-cleanup.md. Refs #7515
|
Thanks, the preparation gap is real. Fixed in e542512.
Local (macOS, isolated homes): the 13 files that use or test this teardown pass 312/312; Agreed on scope: this PR does not claim the empty-DACL interleaving or #7515 as a whole is resolved. It removes the ACL writer from these fixtures and makes teardown refuse open owners. The PR body now says that explicitly. |
|
@Ingwannu Response to your CHANGES_REQUESTED review on 0577267, point by point. Current head: e542512.
Verification at e542512:
Requesting your re-review. |
This branch has not been deployed
2 participants
Summary
#7515 reported hosted-Windows fixture teardowns running out of their 15 s removal budget with EPERM or EBUSY after every assertion had passed. Its EBUSY case in
main-account-hard-lock-fallback-routing.test.tshad its own cause on #7482's branch (routing-history.sqliteheld open by the hop classifier) and was fixed there, merged todevas1025acd4b1(2.91.0). This PR covers the remaining EPERM cases (server-management-auth.test.ts,codex-auth-context.test.ts) and makes the shared teardown refuse homes that a native-main owner still holds. No retry count, sleep, or timeout changes.What was blocking deletion. In the two EPERM cases, the lock diagnostic shows the same blocker. The native-main lock file under the test home (
.opencodex-native-main.owner.sqlitein server-management-auth,.claim.sqlitein codex-auth-context) had an empty DACL. Every other file kept normal entries. I reproduced that signature on a real Windows host: a file with zero ACEs still renames through the parent's delete-child right, which is why the diagnostic printedrename:ok, but recursivermfails. The only thing that writes that file's ACL is real asyncicacls, running in suites that assert nothing about ACLs.tests/codex-integration/codex-auth-context.test.ts: the suite already says real icacls doesn't belong in it, but it stubbed only the sync runner. The claim lock is hardened through the async runner. It now stubs both and tears down throughdrainAndRemoveFixtureRoots. The old teardown used the blocking remover directly, which never waited for ACL children and blocked the event loop.tests/server/server-management-auth.test.ts: stubs the async runner for the suite (sync-runner cases still inject their own). The stub stays installed until removal finishes.tests/codex-integration/main-account-hard-lock-fallback-routing.test.ts: separate from the EBUSY fixed by feat(fallback): OCX-11 fallback off per scope, with home_unreachable on Children #7482. An instrumented Windows run ondevfound 2 of 29 cases reaching removal while the config-dir ACL harden (config-dir-acl-harden, a live icacls child under the home) was still in flight. Teardown now uses the shared drain helper. This does not touch feat(fallback): OCX-11 fallback off per scope, with home_unreachable on Children #7482's code.tests/helpers/fixture-teardown.ts+src/codex/native-main-owner.ts: before removing a root, the shared teardown asks the newnativeMainOwnerHomesOpenAtOrBelow(root)whether a native-main owner below it is still open. That includes an owner still acquiring (descriptor and SQLite handle held in locals while its hardener runs), one waiting on a retry timer, and one that is closing. If so, it keeps that root and names the owner immediately, without spending the retry budget on an EBUSY that names nothing.structure/ops/test-sandbox-cleanup.mddocuments the check.This does not cover the third pattern in the issue thread. #7501 (e501371) fixed timing races; it changed none of these teardowns.
Refs #7515
Verification
testRunnerBun), isolatedHOME/USERPROFILE/LOCALAPPDATA/TEMP/OPENCODEX_HOME/CODEX_HOME/CLAUDE_CONFIG_DIR:bun teston the four changed suites → 171 pass / 0 fail. A temporary probe at the removal step (not committed) loggedacl-pending=false bg=[]for all 174 removals.devbefore the fix: the probe on the hard-lock fallback file showedacl-pending=true bg=["config-dir-acl-harden"]for 2 of 29 cases.rename:okwhile rm fails;icacls /Tlists a zero-ACE file as a bare path, which matches the CI logs.drainAndRemoveFixtureRootsuser plus native-main-owner-lifetime) → 282 pass / 0 fail. Hard-lock fallback file plus helper suite → 43 pass.bun run test:changedwith Bun 1.4.3 (ran 1542 files) → 34521 pass / 42 skip / 5 fail. All 5 are Antigravity UA/discovery cases that fail only becauseGOOGLE_ANTIGRAVITY_USER_AGENTis set in my shell; unset, those files pass 22/22.tsc --noEmitexit 0,bun run structure:check,bun scripts/file-size-ratchet.ts,bun run privacy:scan, andgit diff --checkall pass.ci.yml lane=allat 0577267: run 39790983126. All 9 Windows shards succeeded, but the run concludedfailurebecause theciaggregator hit GitHub API faults (null conclusions, HTTP 502, a partial job list). At e542512: https://github.com/lidge-jun/opencodex/actions/runs/39804032261 (pending).Scope: this does not claim #7515 as a whole is resolved. The empty-DACL interleaving remains unexplained, and the EPERM fixes are verified only as far as the runs below. Not proven: I could not reproduce the empty-DACL interleaving itself off CI. Real owner, claim, and dir-harden flows under concurrency on Windows produced 0 of 15; directory propagation, killed icacls, and Bun mode handling were each ruled out. The fix removes the only writer of that ACL from these suites; it does not explain how production reached that state.
Checklist
Summary by CodeRabbit