Repository navigation
fix(codex): say a full Stop wait can overrun quit's eviction budget, and unref its timer - #23859
Conversation
…and don't let its timer hold the process
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe wait timeout now calls Priority: ⬇️ Low Merge Risk: 🔵 Low · up to When owner status remains unresolved, restarting may leave the lease in place and report Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
CODEX_STOP_TURN_OPEN_WAIT_MScomment corrected — the JSDoc now records the real bound: a Stop queued ahead of a close/quit spends its 5s wait from the quit path's eviction budget, so a full wait plus a slow provider close can overrun it and next launch's recovery settles the lease.- Wait timer unref'd —
bound.unref?.()increateCodexTurnOpenWaitsso a pending Stop wait is never what keeps the Node process alive at quit, matchingcodex-structured-notification-retry.ts:73andcodex-structured-journal-generic-frames.ts:41.
Verified the arithmetic against structured-agent-session-host-teardown.ts:39 (1000 drain + 6000 supervised close + 1000 margin = 8000) and confirmed a Stop holds the session serialize across the wait while quit's eviction phase queued behind it is wrapped in withPhaseTimeout(..., CHILD_EVICTION_TIMEOUT_MS) — so a 5000 + 6000 sequence does exceed the budget. pnpm test on codex-structured-turn-open-wait.test.ts and structured-agent-session-codex-turn-end-settlement.test.ts passes 18/18.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
StatusReady. Head The comment on the Codex Stop's 5-second wait now says what really happens: a close or quit queued behind a waiting Stop spends the wait out of the 8-second eviction budget, so a full wait plus a slow close can overrun it, and the next launch's recovery settles the lease. The wait's timer is unref'd like the other Codex timers. No user-visible change. No new test, since neither is observable to one; the wait's tests pass, and |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: e3bbcded-cdf9-49ff-9068-020b52f5e5ed
📒 Files selected for processing (2)
src/main/codex/codex-structured-prompt-ownership.tssrc/main/codex/codex-structured-turn-open-wait.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| /** How long a Stop waits for Codex to open the turn it answered a send into. A close or quit queued | ||
| * behind the Stop spends this out of the eviction budget, so a full wait plus a slow provider | ||
| * close can overrun it; the next launch's recovery then settles the lease. */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the next-launch recovery outcome.
When restart recovery lacks sufficient owner-death evidence, it can retain the unresolved lease and report execution_owner_reconciling. The comment currently states that recovery settles the lease unconditionally.
Suggested fix
- * close can overrun it; the next launch's recovery then settles the lease. */
+ * close can overrun it; the next launch's recovery attempts to settle the lease, but may
+ * retain it as unresolved and report `execution_owner_reconciling`. */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** How long a Stop waits for Codex to open the turn it answered a send into. A close or quit queued | |
| * behind the Stop spends this out of the eviction budget, so a full wait plus a slow provider | |
| * close can overrun it; the next launch's recovery then settles the lease. */ | |
| /** How long a Stop waits for Codex to open the turn it answered a send into. A close or quit queued | |
| * behind the Stop spends this out of the eviction budget, so a full wait plus a slow provider | |
| * close can overrun it; the next launch's recovery attempts to settle the lease, but may | |
| * retain it as unresolved and report `execution_owner_reconciling`. */ |
|
Status update: merged current |
1 participant
ELI5
A code comment claimed that a Codex Stop's short wait always fits inside the time Orca allows for shutting Codex down when the app quits. It does not always fit, so the comment now says what really happens. The wait's timer also no longer counts as something that keeps the app running.
What Changed
Before
After
No behaviour a user sees changes: the 5-second limit and the order of close and quit are the same.
Why
The comment is the only record of why 5 seconds was safe, and it overstated that. The accurate version matters for whoever next changes the eviction budget or the wait. Unref'ing matches
codex-structured-notification-retry.tsandcodex-structured-journal-generic-frames.ts. Shortening the wait or releasing it at quit was not needed: overrunning needs a full wait and a slow close at the same moment, and recovery already handles it.Linked Issue
No issue. Follow-up from #23026's final review.
Visual Proof
N/A: a code comment and a timer flag; nothing a user sees changes.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
No new test: whether a timer is unref'd is not observable to a behaviour test, and the comment has no runtime effect. The Stop wait's existing tests pass (
codex-structured-turn-open-wait.test.ts, 10/10).pnpm tc:node, oxlint,check:code-quality:changedand the anti-slop audit pass.AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
N/A for SSH, mobile and mixed versions: a comment and a timer flag inside the local Codex adapter.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author: @BrennanKB5