Skip to content

fix(codex): ungate app-server terminal liveness and fan out poisoned-client retirement#102097

Closed
obviyus wants to merge 3 commits into
mainfrom
fix/app-server-liveness-fanout
Closed

fix(codex): ungate app-server terminal liveness and fan out poisoned-client retirement#102097
obviyus wants to merge 3 commits into
mainfrom
fix/app-server-liveness-fanout

Conversation

@obviyus

@obviyus obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Track B2 of #101863 — the Codex app-server half of the liveness redesign (#89742, regression of #82681; also #85251, #90673).

One shared app-server client serves every session with the same auth profile + start options (shared-client.ts keyed global map). Two verified defects:

  1. Every fast watch is gated by in-flight server→client requests. activeAppServerTurnRequests > 0 suppresses the completion-idle (60s), assistant-completion (10s), and terminal-idle (30 min) watches. A client that goes silent with a request outstanding (the reported 429-burst shape: turn/started, then nothing) is exactly the state no watch can see; the only remaining bound is the whole run budget.
  2. Retiring a poisoned client doesn't fan out. After a timed-out turn, the shared entry is detached so future acquisitions get a fresh client — but sessions already leased on the wedged process keep waiting with nothing to abort them.

Why This Change Was Made

Protocol proof (Codex gate). Inspected sibling openai/codex source directly: server→client requests register a oneshot callback and await the client's response (codex-rs/app-server/src/outgoing_message.rs:272-350), completely separate from fire-and-forget server notifications (outgoing_message.rs:589-629; envelope split in app-server-transport/src/outgoing_message.rs:21-31; types in app-server-protocol/src/protocol/common.rs:1199-1252, 1630-1658). An in-flight request therefore proves nothing about notification liveness in either direction — gating a notification-silence watchdog on the request counter is unsound.

Changes (all in extensions/codex/src/app-server/):

  1. Terminal-idle watch ungated (attempt-turn-watches.ts): the 30-minute zero-notification backstop now fires regardless of in-flight requests. Its clock still resets on every real notification, so healthy turns — including ones legitimately waiting on approvals/elicitation — are untouched as long as the server emits anything within the window. The completion-idle and assistant-completion watches keep their request gates: they express turn-completion semantics, which are genuinely indeterminate while a request is being served. No timeout constants changed.
  2. Suspect-client retirement fans out (shared-client.ts, attempt-client-cleanup.ts): retireSharedCodexAppServerClientIfCurrent gains an explicit failActiveLeases mode used only by the timed-out-turn path — it closes the physical client immediately (transport close SIGKILLs the child after 1s), so co-leased attempts hit the existing client-closed retry path and re-run on a fresh client, and pending acquires reject instead of leasing the poisoned process. Default retirement stays graceful (detach + close-when-idle): routine one-shot bundle-MCP cleanup and startup-failure paths keep main's behavior — the initial implementation applied immediate-close to all callers, which would have yanked healthy clients from co-leased sessions at every one-shot run end; review caught it and scoped it, restoring main's two graceful-retirement tests verbatim and adding a pin test for the default.

Stated tradeoff: a healthy sibling turn co-leased on a client that another turn timed out gets aborted-and-retried. Intentional — a client suspect enough to deny future sessions should not keep serving current ones — and the failure is retryable, not user-terminal.

User Impact

  • A wedged shared app-server client is now bounded at 30 minutes of true silence instead of the full run budget (up to 48h), even mid-request.
  • One poisoned client no longer wedges every co-leased session: siblings fail over to a fresh client automatically.
  • No behavior change for healthy clients, isolated-client mode, or routine cleanup paths.

Evidence

  • Protocol citations above from direct ../codex inspection (f1affbac), verified independently by implementer and reviewer.
  • Tests: terminal watch fires with requests in flight + zero notifications (previously structurally impossible); does not fire while notifications arrive; completion/assistant gates regression-pinned; fan-out (co-lease sees retryable client-closed failure, pending acquires reject, fresh acquisition gets a new client); graceful-default retirement pinned (leased client survives routine retire, closes on release); isolated-client path unchanged.
  • Validation: full extensions/codex suite green (96 files / 2005 tests on Testbox; app-server subset 78 files / 1681 tests re-run green after review edits), extensions tsgo + extensions-test tsgo lanes exit 0, oxlint/oxfmt clean.

Part of #101863 (Track B2). Fixes #89742; addresses #85251 / #90673 (same wedge class — will verify against each before closing).

@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 8:22 AM ET / 12:22 UTC.

Summary
The PR removes the in-flight-request gate from the Codex app-server terminal-idle watchdog and adds opt-in suspect-client retirement that force-closes shared clients for active and pending leases.

PR surface: Source +77, Tests +204. Total +281 across 10 files.

Reproducibility: yes. source-reproducible: current main gates terminal-idle on activeAppServerTurnRequests, and the PR adds focused tests proving terminal-idle now fires with an in-flight request and resets on notifications. I did not run live or unit tests in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Runtime liveness gates: 1 terminal-idle request gate removed; completion request gates preserved. This separates physical-client liveness from per-turn completion semantics, which is the core behavior maintainers must accept.
  • Shared-client retirement modes: 1 opt-in forced-retirement path added; graceful default retained. The new mode can abort co-leased Codex turns only for suspect terminal-idle clients while routine cleanup stays compatible.

Root-cause cluster
Relationship: superseded
Canonical: #102160
Summary: This PR is a B2 track review record; the open integration PR is the intended atomic landing path, while the linked issues remain candidate-fixed only after merge and verification.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Risk before merge

  • [P1] Merging this branch standalone changes Codex runtime recovery semantics: terminal-idle can now close the physical shared client and force co-leased turns onto retry paths.
  • [P1] A healthy but completely silent in-flight server request longer than the terminal liveness window would now be interrupted where current main would keep waiting.
  • [P1] The author states this track should land atomically through fix(agents): run-lifecycle reliability — bounded release, evidence-based liveness, watchdog semantics #102160, so standalone landing would bypass the intended cross-track validation boundary.

Maintainer options:

  1. Land through the integration PR (recommended)
    Keep this PR as a draft review record and let the integration PR carry the accepted Codex liveness behavior after stack-level review.
  2. Require standalone upgrade proof
    If maintainers want this branch merged by itself, require explicit proof that forced suspect-client close retries co-leased sessions without losing user-visible replies.
  3. Do not land this track separately
    Close or leave the branch parked if maintainers decide the standalone retry and timeout semantics are not worth a separate merge path.

Next step before merge

  • [P1] No automated repair is needed on this branch; maintainers need to keep or land it according to the protected integration-plan decision.

Maintainer decision needed

  • Question: Should this B2 Codex app-server liveness track remain review-only and land only through fix(agents): run-lifecycle reliability — bounded release, evidence-based liveness, watchdog semantics #102160, or should maintainers allow standalone landing with the new shared-client recovery behavior?
  • Rationale: The code defect from the prior review appears fixed, but the remaining choice is compatibility-sensitive merge policy for shared Codex clients and the broader run-lifecycle stack.
  • Likely owner: obviyus — They own this branch and the linked integration PR that is meant to land the track.
  • Options:
    • Keep atomic integration path (recommended): Keep this draft as the review record and merge the behavior through the integration PR after its stack-level risks are accepted.
    • Allow standalone landing: Maintainers can accept the Codex-only recovery semantics here, but that intentionally departs from the author’s stated atomic landing plan.
    • Pause both paths: Hold this track and the integration stack until maintainers choose different timeout or client-retirement policy.

Security
Cleared: No concrete security or supply-chain concern was found; the diff stays inside Codex app-server runtime/tests and does not change dependencies, workflows, lockfiles, secrets, permissions, or package execution.

Review details

Best possible solution:

Keep this PR open as the B2 review record and land the behavior through #102160 after maintainers accept the Codex shared-client recovery tradeoff.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main gates terminal-idle on activeAppServerTurnRequests, and the PR adds focused tests proving terminal-idle now fires with an in-flight request and resets on notifications. I did not run live or unit tests in this read-only review.

Is this the best way to solve the issue?

Yes for the B2 Codex liveness slice: removing only the terminal-idle request gate and making forced retirement opt-in is narrower than changing completion semantics or default cleanup. The best merge path is still the linked atomic integration PR, not standalone cleanup landing.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4bf70be01a21.

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The branch changes existing timeout and shared-client retirement semantics that current Codex users may observe during upgrade.
  • add merge-risk: 🚨 message-delivery: Co-leased sessions on a suspect shared client can now be aborted into retry paths, so reply delivery behavior changes if the retry path fails.
  • add merge-risk: 🚨 session-state: Terminal-idle cleanup now clears the thread binding and force-closes suspect shared clients, affecting active session recovery state.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR is protected as maintainer-labeled, so the external-contributor proof gate is not applied; the body provides Testbox and focused test validation, but I did not treat tests as standalone real behavior proof.

Label justifications:

  • P1: The PR addresses real Codex app-server wedges that can stall active agent/channel workflows and queued replies.
  • merge-risk: 🚨 compatibility: The branch changes existing timeout and shared-client retirement semantics that current Codex users may observe during upgrade.
  • merge-risk: 🚨 session-state: Terminal-idle cleanup now clears the thread binding and force-closes suspect shared clients, affecting active session recovery state.
  • merge-risk: 🚨 message-delivery: Co-leased sessions on a suspect shared client can now be aborted into retry paths, so reply delivery behavior changes if the retry path fails.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR is protected as maintainer-labeled, so the external-contributor proof gate is not applied; the body provides Testbox and focused test validation, but I did not treat tests as standalone real behavior proof.
Evidence reviewed

PR surface:

Source +77, Tests +204. Total +281 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 5 96 19 +77
Tests 5 215 11 +204
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 10 311 30 +281

What I checked:

Likely related people:

  • vincentkoc: Recent current-main history and blame for the extracted Codex app-server watch/shared-client files point to the plugin localization refactor and adjacent Codex auth/startup work. (role: recent area contributor; confidence: high; commits: cc083e5f4fe0, e085fa1a3ffd, f1cc8f0cfc; files: extensions/codex/src/app-server/attempt-turn-watches.ts, extensions/codex/src/app-server/shared-client.ts, extensions/codex/src/app-server/run-attempt.ts)
  • steipete: Older history shows the Codex app-server harness, lifecycle seams, interrupt shutdown, and hardening were introduced and iterated through April commits by Peter Steinberger. (role: feature history owner; confidence: high; commits: dd26e8c44d, d5698038d7, 8d72aafdbb; files: extensions/codex/app-server/run-attempt.ts, extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/client.ts)
  • obviyus: They authored the current B2 branch, the linked run-lifecycle design, and the open integration PR that is intended to land this track atomically. (role: integration branch owner; confidence: high; commits: 9232ebf314cf, d65ee9b354a2, c66a2e58956f; files: extensions/codex/src/app-server/attempt-turn-watches.ts, extensions/codex/src/app-server/shared-client.ts, extensions/codex/src/app-server/attempt-client-cleanup.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (3 earlier review cycles)
  • reviewed 2026-07-08T07:33:30.088Z sha 9232ebf :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T10:08:56.701Z sha d65ee9b :: needs real behavior proof before merge. :: [P2] Honor failActiveLeases for detached leases
  • reviewed 2026-07-08T10:18:30.163Z sha d65ee9b :: needs real behavior proof before merge. :: [P2] Honor failActiveLeases for detached leases

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 8, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 8, 2026
@obviyus obviyus mentioned this pull request Jul 8, 2026
Merged
@obviyus

obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Landing plan update: this track lands atomically with its siblings via the integration PR #102160 (all commits preserved, rebase merge) — sequential landing would put untested intermediate states on main (e.g. the stale takeover without the quiet-tool floor). This PR stays open as the per-track review record and closes when #102160 lands.

@obviyus
obviyus force-pushed the fix/app-server-liveness-fanout branch from a43547d to c66a2e5 Compare July 8, 2026 12:11
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 8, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

1 participant

@obviyus