fix(codex): retry the native-main ACL timeout once, fail closed otherwise - #1135
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. |
This was referenced Aug 6, 2026
lidge-jun
changed the base branch from
codex/260806-stack02-native-profile-sweep
to
dev
August 7, 2026 01:05
7 tasks
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Aug 7, 2026
On a machine where icacls is slow — Defender real-time scanning, a roaming profile, a domain-controller round trip — a complete ACL sequence could not finish inside the 5-second envelope. The harden failed closed, the native-main owner published a permanent `unavailable`, and every native request returned 503 until the user restarted. One correction to the issue's framing: PR #1135's retry is not the problem. Owner-level recovery calls hardenSecret again and does receive a fresh deadline. The defect is that one complete sequence — `/grant:r`, `/inheritance:r`, `/remove:g`, plus the conditional `/findsid` verification — had only five seconds for all of it. Raises the default to 30s and keeps everything else: the 60s cap, the OPENCODEX_ACL_TIMEOUT_MS override, the clamp, and the shared-envelope structure. Independent per-command budgets were rejected: with /findsid fallbacks they multiply into the multi-minute startup stall the shared budget was introduced to prevent. The cost is stated in the source comment rather than hidden. Because loadConfig hardens three paths sequentially, the timeout-path worst case at load becomes ~90s, and the owner path ~60.25s. Both need icacls to be pathologically slow on every call; a healthy machine finishes in milliseconds. A slow start is recoverable, a permanent 503 is not, and the failure stays fail-closed either way. Four existing tests depended on the 5s default while actually asserting something else — envelope sharing, fresh-budget-on-second-call, recovery cardinality. Each now pins OPENCODEX_ACL_TIMEOUT_MS explicitly so it tests its real subject, and beforeEach/afterEach isolate the variable so a stray value in a developer's environment cannot change what any of them assert. The new test deliberately does not pin: it exercises the shipped default with 13s of slow-but-successful work. Confirmed to fail with the default reverted to 5s.
iF2007
pushed a commit
to iF2007/opencodex
that referenced
this pull request
Aug 7, 2026
…1156) On a machine where icacls is slow — Defender real-time scanning, a roaming profile, a domain-controller round trip — a complete ACL sequence could not finish inside the 5-second envelope. The harden failed closed, the native-main owner published a permanent `unavailable`, and every native request returned 503 until the user restarted. One correction to the issue's framing: PR lidge-jun#1135's retry is not the problem. Owner-level recovery calls hardenSecret again and does receive a fresh deadline. The defect is that one complete sequence — `/grant:r`, `/inheritance:r`, `/remove:g`, plus the conditional `/findsid` verification — had only five seconds for all of it. Raises the default to 30s and keeps everything else: the 60s cap, the OPENCODEX_ACL_TIMEOUT_MS override, the clamp, and the shared-envelope structure. Independent per-command budgets were rejected: with /findsid fallbacks they multiply into the multi-minute startup stall the shared budget was introduced to prevent. The cost is stated in the source comment rather than hidden. Because loadConfig hardens three paths sequentially, the timeout-path worst case at load becomes ~90s, and the owner path ~60.25s. Both need icacls to be pathologically slow on every call; a healthy machine finishes in milliseconds. A slow start is recoverable, a permanent 503 is not, and the failure stays fail-closed either way. Four existing tests depended on the 5s default while actually asserting something else — envelope sharing, fresh-budget-on-second-call, recovery cardinality. Each now pins OPENCODEX_ACL_TIMEOUT_MS explicitly so it tests its real subject, and beforeEach/afterEach isolate the variable so a stray value in a developer's environment cannot change what any of them assert. The new test deliberately does not pin: it exercises the shipped default with 13s of slow-but-successful work. Confirmed to fail with the default reverted to 5s.
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
Separates "the Windows ACL call timed out" from "the ACL was refused" on the native-main owner path, and retries once for the former.
Previously a transient ACL stall was indistinguishable from a permanent denial, so a slow call could permanently fence the native-main owner.
src/lib/windows-secret-acl.tsnow returns a coded error taxonomy,src/codex/native-main-owner.tsretries exactly once on the timeout class, andsrc/codex/native-main-lock-file.tspropagates the coded failure.The fail-closed property is preserved. A refused ACL still denies — the retry applies only to the timeout class. That distinction is load-bearing because this path guards a physical credential: a retry that swallowed a denial would be a security regression, and the tests pin that it does not.
Attribution
This is @luvs01's work from #1130, cherry-picked with their authorship intact. #1130 stays open for them. The coded-error approach is the right shape for this — it makes the recoverable case explicit rather than widening the catch.
Stack 3 of the 260806 attribution campaign, stacked on #1134. Planning unit:
devlog/_plan/260806_stacked_bug_campaign/.Verification
bun test tests/windows-secret-acl.test.ts tests/native-main-owner-lifetime.test.ts— 166 pass, 0 failbun run typecheck— exit 0bun run privacy:scan— passedbun run testvia the repository prepush gate — passedPlatform limitation stated honestly: verification ran on macOS, so the Windows ACL boundary is exercised through injection rather than natively. The tests cover the permanent-error, timeout, and retry-exhaustion classes; they do not prove real Windows ACL behavior. A Windows CI leg is the check that would.
Checklist