Repository navigation
fix(startup): skip unused native Codex ownership probes - #7184
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 7
✨ Finishing Touches
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 |
|
✅ Deterministic PR hygiene checks passed. |
|
Being carried into #7215 with your authorship preserved; this PR will be closed when the bundle lands. |
Carried from #7184. Co-authored-by: GOAT <goat@localhost>
|
Held out of #7215 after remote validation: with the native ownership probes skipped, startup returns sooner and on Windows the config-dir ACL hardening can still hold the test home at teardown, so tests/codex-integration/bearer-admission-routed-provider.test.ts fails with EBUSY and cascades (33 failures in the union full run on mini; 1 of 2 isolated runs). The same union without this change passes that file 3/3. Switching that file's teardown to removeTreeAfterBackgroundWork alone made it worse (5 s test timeouts), so the fix needs a deliberate look. |
…remote-list RFC (#7186, #7203, #6157) (#7215) Carried from #7186, #7203, #6157 (one commit per source PR, original authors preserved). #7154 and #7184 held out. Remote exact-head validation (hosted GitHub Actions stalled; local tests not run by maintainer rule): union candidate codex/prr-integ-6 (368006f, tree 27fdc37) on mini (Windows) and lidge (Linux), typecheck + full suite, compared with base-dev 4ae4936 on each host. Co-authored-by: Epinephrine <luvs01@hanmail.net> Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Correction to my earlier note: the bearer-admission EBUSY / spend-ledger teardown failure on Windows also appeared in later union runs that did not contain this change (integ-8 and integ-9), and the file passes 4/4 in isolation on both dev and the union. So it is an intermittent Windows teardown race in that test, not caused by #7184. This PR was held out of #7215 on a wrong attribution; it can land on its own merits. |
… settle teardown Each case starts a real server. On Windows the config-dir ACL hardening can exceed the 5 s default and still hold the home at teardown, which then cascades into spend-ledger owner conflicts. Use SERVER_BUDGET_MS for the file and removeTreeAfterBackgroundWork.
|
Maintainer security disposition before landing (head e9fe073): no ownership bypass. With the startup gate closed, skipped probes yield null. Cache writes require an explicit owned verdict, manager preparation is skipped, and startup selects the existing no-op lifecycle with homeId null, so a skipped probe never turns foreign or unknown ownership into authority. Later sync keeps the persisted-intent checks and write admission. With local Codex integration enabled, both ownership inspections still run (the second one fresh). The scheduler-race regression and the Codex-OFF zero-probe regression pass on Linux and Windows. Validation: the lidge full suite shows 0 new failures against the dev baseline. On mini, the focused files were compared twice each at dev d93965b and this head: 5 failures reproduce identically on both (mini environment, and they pass in hosted Windows CI), with no failure reproducible only on the head. Windows cold-start samples were dev 2.88/2.39/2.35s and head 2.36/2.32/2.29s, which is too noisy for a speedup claim, so the claim stays "removes unnecessary probes". Hosted PR CI is green on the exact head. |
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: e9fe073781
ℹ️ 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".
| const startupCacheOwnership = shouldSyncCodexOnStart(config) ? inspectStartupOwnership( | ||
| deps, | ||
| startupOwnershipHomes, | ||
| startupOwnershipStatePaths, | ||
| startupWindowsTaskListingCache, | ||
| ); | ||
| ) : null; |
There was a problem hiding this comment.
For Codex-OFF, sibling, or hub-without-local-integration starts, this branch now skips the first ownership probe and the analogous branch below skips the second, but structure/ops/service-and-sidecars.md:109-113 still states that every proxy startup asks twice and that the second query is mandatory. Update that mapped architecture document to scope the two-probe contract to enabled startup so it does not contradict the runtime.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
1 participant
Summary
This addresses demonstrated unnecessary readiness-path work while investigating the 45-second owner-start failure in Windows 9/9, run 38699763674. Six Windows fresh-process/home timing samples are recorded below. They do not establish that the full reported timeout is resolved; the claim remains limited to removing unnecessary probes. The separate DPAPI implementation in #7181 is not included or duplicated.
Verification
Current validation head:
e9fe0737818763db4f3778ee087235b6b7300b8b, after mergingorigin/devat947b9b6ab6f259eae060c576dbc368a58d54a487. No additional runtime changes were made for this validation.ssh lidge, Bun 1.4.3):~/bin/ocx-lane-ci2.sh e9fe0737818763db4f3778ee087235b6b7300b8b h2-7184-linux— full suite, 450s; typecheck exit 0, test exit 1.~/bin/ocx-lane-cmp.sh h2-7184-linux dev-7a42860reports 49,339 pass / 29 fail / 147 skip, versus baseline 49,138 / 30 / 146, 0 new failure identities. The exact typecheck diagnostic sets are both empty. The additional skip is the Windows-onlyWindows never resolves a captured remote candidatecase added on dev since the baseline (tests/cli/cli-codex-cli-update-boundary.test.ts:18). The Codex-OFF zero-probe test and enabled-startup fresh-listing race pass in this run. This is baseline comparison evidence, not an all-green suite.ssh mini, Bun 1.4.3):OCX_LANE_SLOTS=3 ~/bin/ocx-lane-ci2.sh e9fe0737818763db4f3778ee087235b6b7300b8b h2-7184-win— full suite completed in 2,104s, typecheck exit 0, test exit 1.~/bin/ocx-lane-cmp.sh h2-7184-win dev-7a42860reports 48,625 pass / 81 fail / 821 skip, versus baseline 48,436 / 81 / 809, with 7 new failure identities and 7 baseline identities now passing/absent. The exact typecheck diagnostic sets are both empty. These seven identities are differences against the older baseline, not attribution to this PR; the current-dev comparison below supersedes that interpretation. Both the Codex-OFF zero-probe regression (840.75ms) and enabled-startup fresh-listing race (390.65ms) passed. The +12 skips are accounted for by nine additional Unix-only cases (four CLI-provenance, two FIFO-boundary, two UID-scanning and one npm-config case) plus the Unix shutdown fixture expanding three cases into six; these changes are inherited from dev since the pinned baseline, not new skips in this PR's delta. The initial task stopped at that comparison; the coordinator subsequently authorized the focused current-dev comparison below. No source fixes were made.947b9b6ab6and PRe9fe073781, measured from spawningbun run src/cli/index.ts start --port <unused port>to HTTP 200/healthzwithstatus: ok,service: opencodex, and the spawned PID. Codex/Grok/Desktop integration and Claude interception were explicitly OFF, with isolated HOME/USERPROFILE/CODEX_HOME/OPENCODEX_HOME. Dev: 2.876 / 2.388 / 2.346 s; PR: 2.364 / 2.318 / 2.292 s. Each process was terminated afterward; follow-up checks confirmed all six PIDs exited and ports closed (52934, 53062, 53199, 53305, 53419, 53531; never 10100). The middle pair ran PR-first to reduce order bias. These samples share a machine with the full suite and do not flush OS caches; the small, noisy difference is not reliable evidence of a general Windows speedup or resolution of the reported 45-second timeout. The PR claim is limited to removes unnecessary probes.pull_request, attempt 1 — SUCCESS, exact heade9fe0737818763db4f3778ee087235b6b7300b8b. All four Ubuntu test shards, aggregateci,gates(including typecheck/privacy), structure, storage policy, API usage, keyring, Docker and npm-global smoke passed. Catalog async contracts, React Doctor and PR policy checks also passed. Windows/macOS matrix legs were policy-skipped; Windows validation is the separate mini run above.git diff --checkpassed; working tree is clean. No local test suite was run; full-suite evidence is from the requested remote hosts.Apples-to-apples Windows follow-up (coordinator-authorized):
Fetched dev tip
d93965b0245d11eb8e3cab0b188cad5d83ca81ea; compared it with PR heade9fe0737818763db4f3778ee087235b6b7300b8b. Ran all five explicitly supplied paths (the packet said six but listed five):codex-composed-acceptance.test.ts,claude-picker-windows.test.ts,sibling-home-client-sync.test.ts,server-management-auth.test.ts, andcodex-cli-update-plan.test.ts. Their source bytes are identical at both revisions. Both revisions usedocx-lane-ci2.sh, Bun 1.4.3,OCX_LANE_SLOTS=3,OCX_TEST_NO_QUEUE=1, and the same file arguments. The runs were serial: dev1, head1, head2, dev2.h2-dev-tip-7d93965b024h2-dev-tip-7-r2d93965b024h2-head-7e9fe073781h2-head-7-r2e9fe073781No reproducible head-only named-test failure was observed. All four test commands still exit 1. Every run reported
CLI watchdog: ocx ensure. Head repeat 2 additionally hit a management-origin fixture teardown timeout and an unnamed cleanup-hook timeout; the named test passed on head repeat 1 and both dev runs. This intermittent result remains recorded rather than excused as passing. Focused results do not reproduce all full-suite interactions; they do refute attribution of the original seven identities solely from their absence indev-7a42860. No fixes or further reruns were performed.Each cell below is run 1 / run 2:
/api/syncapplies on-disk configEarlier implementation evidence (before this merge) included the regression red/green proof, focused lifecycle/probe tests, test:changed, structure/privacy checks. Current-head results above supersede the earlier profiling dispatch as validation evidence.
Checklist
structure/runtime.mddocuments the activation gate.)