Repository navigation
fix(native-chat): resting chat skill checks share one capped Codex process per account - #34349
Conversation
…ocess per account A resting chat's slash list started one hidden codex app-server per folder, each kept alive 70 s outside the two-check limit, and a scan waiting in line for 30 s was treated as hung and doubled. With many resting folders this kept about a hundred Codex processes alive. - The catalog runs one scan per folder at a time; readers join it. Failures retry three times, then wait for the TTL. Entries unread for an hour are dropped. - Codex folders that share an account, binary, arguments and environment are answered by one app-server, batched into skills/list, with at most two probe processes alive until their teardown ends. A probe that cannot start backs off, and a refresh does not reset it; an explicit refresh asks a live probe to reload. - Batched replies are matched to their own folder. Tracked in #34141.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description provides detailed problem, change, and validation information, but it does not follow the required template. It omits the required ELI5, Linked Issue with a Fixes #... entry, Visual … |
Restructure the description using all template headings. Add a plain-language ELI5 summary, link the issue with Fixes #34141 if applicable, state N/A with a reason for Visual Proof if there is no UI change, complete or explicitly addres… |
-
Check name Status Explanation Title check The title names the main change: resting chat skill checks now share one capped Codex process per account. The changeset matches this. The shared probe pool, the eight-folder batching, and the two-pro… 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: Title check
-
Explanation
The title names the main change: resting chat skill checks now share one capped Codex process per account. The changeset matches this. The shared probe pool, the eight-folder batching, and the two-process global limit are all in the diff. The title is specific and does not need a file list. It is a little long at 86 characters, but it stays clear.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 28 functions across 20 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Description check
-
Explanation
The description provides detailed problem, change, and validation information, but it does not follow the required template. It omits the required ELI5, Linked Issue with a
Fixes #...entry, Visual Proof or an explicitN/Astatement, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections.Resolution
Restructure the description using all template headings. Add a plain-language ELI5 summary, link the issue with
Fixes #34141if applicable, stateN/Awith a reason for Visual Proof if there is no UI change, complete or explicitly address AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist items, and retain the existing detailed testing evidence under Testing.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
90f086d7-ed4f-4973-bc2f-6cec564e0759
-
src/main/codex/codex-at-rest-skills-probe.test.tssrc/main/codex/codex-at-rest-skills-probe.tssrc/main/codex/codex-skill-list.tssrc/main/native-chat/agent-session-wire/at-rest-command-catalog.tssrc/main/native-chat/agent-session-wire/structured-agent-session-at-rest-commands.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
-
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
cwd match in the pooled at-rest path. · codex-skill-list.ts:53-62
src/main/codex/codex-skill-list.ts:53-62
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire an exact
cwdmatch in the pooled at-rest path.
answer()first checkscodexFolderSkillReport, then callscodexSkillRowsfor a single-folder batch.codexSkillRowsfalls back toentries[0]. Therefore, a reply for/work/onecontaining an entry for/work/twocan publish/work/two's skills for/work/one.Scope the correction to the pooled at-rest path. Do not change
codexSkillRowsglobally without preserving its intentional legacy use byCodexSkillReport.
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ad064889-b3a2-4bba-8e99-7a1402e41531
-
config/flaky-test-pattern-baseline.txtsrc/main/codex/codex-at-rest-skills-probe.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
-
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ece1e5cd-4209-43bb-9abd-e03a008a31ca
-
src/main/claude/claude-at-rest-commands.tssrc/main/claude/claude-command-catalog-probe.tssrc/main/claude/claude-command-folder-scan.tssrc/main/claude/claude-model-catalog-probe.tssrc/main/claude/claude-structured-child-env.tssrc/main/claude/claude-structured-launch-resolution.tssrc/main/codex/codex-app-server-session.tssrc/main/codex/codex-at-rest-skills-probe-pools.tssrc/main/codex/codex-at-rest-skills-probe.test.tssrc/main/codex/codex-at-rest-skills-probe.tssrc/main/codex/codex-structured-launch-resolution.tssrc/main/native-chat/agent-session-wire/at-rest-command-catalog.tssrc/main/native-chat/agent-session-wire/structured-agent-session-at-rest-commands.test.tssrc/main/native-chat/structured-agent-command-resolution.test.tssrc/main/native-chat/structured-agent-command-resolution.tssrc/main/runtime/runtime-client-agent-launch-settings-durability.test.tssrc/main/skills/claude-plugin-skill-sources.tssrc/main/skills/discovery.test.tssrc/main/skills/discovery.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
-
|
Resting Codex chats could keep roughly 100 hidden discovery processes alive and repeatedly start more, pinning Windows CPU (tracked in #34141). This PR joins duplicate folder checks and shares a bounded Codex discovery process across folders with the same launch identity. Folder replies stay isolated, retries back off, obsolete scans cancel, and process permits remain held through teardown. Sending stays independent of discovery. The full review converged after three production rounds. Fixes covered stuck preparation and cancellation (including late directory ownership), refreshing the current runtime home before reusing a process, quadratic queue work, and unbounded failed-identity retention. Final correctness, performance and reference checks found no open proven production issue. QA ran on Windows QA used real Codex 0.159.2 and Claude Code 2.1.311: six baseline-created chats survived graceful upgrade; restored git and folder chats produced real replies; folder-specific lists, Refresh, disabled-skill omission and restoration passed. All 23 rounds of the 100-folder corpus returned correct rows (2,300 responses). A fresh 16-round interval returned 1,600 correct responses over 11m15s, crossing multiple five-minute refreshes. Samples observed at most two logical discovery servers, separately from native chat servers; all four recorded launch sets had zero survivors. The samples are not a kernel-complete process trace or a baseline speedup benchmark. Test copies are stopped and real-config hashes are unchanged. Fourteen private actual-source lifecycle/performance schedules passed. Three actual-CLI replay tests covered current enabled/disabled replies and the official archived 0.63.0 unsupported response through runner, catalog and picker. Mobile consumer tests passed, including private stalled-discovery/live-precedence schedules. Exact-head CI passed, including all five unit shards, static analysis/typecheck, mobile verification, packaging, persistence, cross-version and WSL checks. The PR is non-draft, conflict-free and all three addressed bot threads are resolved. Retained limitations and deferrals: the original Windows scheduler-suite invocation had two EBUSY fixture-database teardown failures; a private cleanup adapter passed both tests, without relabelling the original invocation. Bulk-registration React update-depth errors reproduced on the independently verified baseline; their component cause remains unknown. Initial list screenshots compare a live baseline with a cold-restored branch. Conversation-only Codex roots and existing Claude classification/picker limits explain the source distinctions; exact initial cold/live parity is not certified because the first alpha-Claude native response was not retained. Later recovery alone is not used to certify it. Missing-path protocol fixtures remain synthetic. These do not establish a PR-only failing input or action refusal. For Brennan's review
The Codex baseline/list/reply captures retain a historical model-unavailable message followed by a successful retry; it is not a PR failure. Judgment spots: attempt ownership, account resolution and stale-publication fencing, batched folder matching and process grace, permit lifetime and failed-identity eviction. |
Problem
Resting Codex chats could keep roughly 100 hidden app-server processes alive and start more than one per second on Windows, pinning CPU for hours (tracked in #34141). Each folder got a separate process, queue waits triggered duplicate checks, and the concurrency limit stopped counting children before they shut down.
Scope: native chat only
This only changes how resting native chat tabs (and native chat orchestration workers) get their
/and$command list while no agent is running for that chat. Terminal agent tabs never use this list and are unaffected. Running native chats still get their list from the live agent, as before.The process storm was Codex-specific: each Codex check starts an app-server that stays up about 70 seconds for plugin sync, and before this PR every folder got its own. Claude's resting checks exit as soon as they answer and were already capped at two at a time; Claude only picks up the shared scheduling rules here (one check per folder at a time, bounded retries). The separate fix that stops settled orchestration workers from asking for these lists at all is #34342.
Change
A folder has one in-flight catalog check. Successful lists age from actual provider-work admission; failed checks retry after ten seconds up to three times, then use the five-minute refresh interval. A five-minute deadline also bounds stuck preparation and discovery. Retirement cancels obsolete work, and unread catalog entries expire after an hour. The shared scheduler applies to Claude lists too.
Folders sharing the same resolved Codex executable, account/home, arguments and environment use one app-server, with at most eight folders per request. Replies remain matched to their exact folders. At most two probe processes are counted through actual teardown. Each healthy process retains its fixed 70-second plugin-sync grace, including when its final folder cancels. Existing live lists, fallback retention and sending behavior remain intact.
Refresh synchronizes the current runtime home before a reused process reloads its list. Failed starts retain 10/30/90/300-second backoff across Refresh and launch-generation changes; idle failure records expire an hour after their last demand. Cancellation reaches preparation and fallback continuations and holds process permits through close.
A virtual replay of the initial implementation reduced the 160-folder workload from about 93 concurrent probes to a peak of one, with three starts across 15 minutes. That replay is scheduler evidence, not Windows QA. The separate demand reduction for settled orchestration workers is tracked in #34342.
Validation
Full review path: shared catalog scheduling, retries and process ownership. Three production review rounds completed cleanly, with final reference and performance checks. The review fixed stuck preparation/cancellation, current-home synchronization for reused probes, quadratic queue work and retained failed identities. A permanent regression covers a directory handle acquired after cancellation.
QA ran on
4e9358b5ce23ca5413533fd22159a2bb442c594d; all test copies are stopped and real config hashes are unchanged. The completed QA evidence carries across conflict-driven main mergec298dc9ea49a85c93df71f8f6da790dd61fec255. The selected-account integration passed six focused Claude tests and two private actual-catalog timeout/retirement proofs. Independent carry review agreed with the integration. Exact-head CI is green, including all five unit shards, static analysis/typecheck, mobile verification, packaging, persistence, cross-version and WSL checks.