Skip to content

fix(agent): close codex app-server sessions - #55450

Open
changeroa wants to merge 2 commits into
NousResearch:mainfrom
changeroa:fix/codex-app-server-lifecycle
Open

fix(agent): close codex app-server sessions#55450
changeroa wants to merge 2 commits into
NousResearch:mainfrom
changeroa:fix/codex-app-server-lifecycle

Conversation

@changeroa

Copy link
Copy Markdown

Summary

  • close attached Codex app-server sessions when cached AIAgent instances release clients or close
  • treat closed app-server stdin during turn/interrupt as non-fatal so timeout retirement can complete
  • add regressions for release/close cleanup and broken-pipe interrupt retirement

Tests

  • /Users/victor/.hermes/hermes-agent/venv/bin/python -m pytest -o addopts='' tests/run_agent/test_codex_app_server_integration.py::TestApiModeAccepted::test_agent_close_closes_attached_codex_app_server_session tests/run_agent/test_codex_app_server_integration.py::TestApiModeAccepted::test_agent_release_clients_closes_attached_codex_app_server_session tests/agent/transports/test_codex_app_server_session.py::TestRunTurn::test_deadline_interrupt_broken_pipe_still_retires_turn -q
  • /Users/victor/.hermes/hermes-agent/venv/bin/python -m pytest -o addopts='' tests/run_agent/test_codex_app_server_integration.py tests/agent/transports/test_codex_app_server_session.py -q
  • /Users/victor/.hermes/hermes-agent/venv/bin/python -m py_compile run_agent.py agent/transports/codex_app_server_session.py tests/run_agent/test_codex_app_server_integration.py tests/agent/transports/test_codex_app_server_session.py

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint codex P3 Low — cosmetic, nice to have labels Jun 30, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary\n\nVerdict: LGTM\n\nFixes Codex app-server session lifecycle: closes attached sessions on agent release/close, treats broken-pipe during turn/interrupt as non-fatal. 4-file change with dedicated regression tests.\n\n### Looks Good\n- Well-scoped fix with clear root cause\n- Regression tests cover both cleanup paths and broken-pipe retirement\n- Minimal source changes (+18 lines across 2 files)\n---\nReviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: LGTM

Ensures codex app-server sessions are properly closed when the agent closes or releases clients. Extracts self._client and self._thread_id into local variables to avoid stale references during the turn loop. Adds RuntimeError handling for turn/interrupt when the transport is closed. Good test coverage (3 new tests: deadline interrupt broken pipe, close during turn, agent close/release integration).

Verdict: LGTM -- thorough resource lifecycle fix with tests.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused lifecycle fix. The premise still holds on current main: run_agent.py:3466-3473 and run_agent.py:3522-3529 close only the OpenAI/httpx client and leave _codex_session attached, while agent/transports/codex_app_server_session.py:300-310 can clear the session fields concurrently with run_turn() dereferences at :400-415 and :455-465.

The broken-pipe handling is also warranted: _issue_interrupt() currently catches only CodexAppServerError and TimeoutError (agent/transports/codex_app_server_session.py:788-801), whereas the transport raises RuntimeError for closed stdin/client states (agent/transports/codex_app_server.py:296-307). The added stable references and cleanup coverage match those current paths.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@jeff-mettel

Copy link
Copy Markdown
Contributor

Link for discoverability: this PR is the closest match for #72548, which currently shows no linked PR in its timeline.

That issue asks specifically for _codex_session to be closed and cleared from both release_clients() and close(), with the reference cleared even when transport cleanup raises. This is the AIAgent-layer PR of the four open ones (#62105 / #62112 are the cron layer, #73429 the gateway/webhook layer), so it is the one that lines up.


Filed by an AI agent (Claude Opus 5) operating autonomously on @jeff-mettel's behalf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history codex comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants