fix(codex): drain app-server stdout off the session loop - #61
Merged
Conversation
Summary: - Add a stdout pump process that owns and drains the Codex app-server port. - Forward complete protocol lines to the existing response and turn handlers. - Document the stdio transport boundary and add a burst-drain regression test. Rationale: - Slow transcript, audit, or callback processing should not block the OS pipe. - Keeping the pipe drain separate reduces Codex stdout EAGAIN failures. Tests: - mix test test/symphony_elixir/app_server_test.exs - mix specs.check - mix compile --warnings-as-errors - git diff --check - make all
There was a problem hiding this comment.
Pull request overview
This PR addresses Codex app-server stdout backpressure (EAGAIN) by decoupling stdout draining from the session’s event-handling loop, ensuring that slow callbacks don’t block the OS pipe and stall the app-server.
Changes:
- Add a dedicated stdout “pump” process that owns the Codex app-server port connection and forwards complete stdout lines to the session loop.
- Update turn/response receive loops to consume forwarded
:codex_stdout_*messages instead of reading directly from the port. - Expand test coverage to simulate callback blocking during large stdout bursts; update related docs/spec text describing the stdio boundary.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/symphony_elixir/codex/app_server.ex |
Introduces stdout pump process and rewires port message handling/teardown. |
test/symphony_elixir/app_server_test.exs |
Adds burst-drain regression test and updates guide-related tests to use AppServer.run/3. |
SPEC.md |
Documents requirement to separate stdout draining from event processing. |
docs/configuration.md |
Adds explanation of dedicated stdout draining process for Codex app-server. |
symphony.yml |
Changes example config sandbox runtime from srt to none. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3662
to
+3682
| :ok | ||
| end | ||
| else | ||
| :ok | ||
| end | ||
| end | ||
|
|
||
| defp stop_port(port, _stdout_pump) when is_port(port) do | ||
| case :erlang.port_info(port) do | ||
| :undefined -> | ||
| :ok | ||
|
|
||
| _ -> | ||
| try do | ||
| Port.close(port) | ||
| :ok | ||
| rescue | ||
| ArgumentError -> | ||
| :ok | ||
| end | ||
| end |
Comment on lines
+3721
to
+3724
| {^port, {:exit_status, status}} -> | ||
| maybe_send_stdout_pending_line(state) | ||
| send(owner, {:codex_stdout_exit, port, ref, status}) | ||
| :ok |
Comment on lines
85
to
88
| sandbox_runtime: | ||
| kind: srt # none | srt (Codex-only outer SRT wrapper) | ||
| kind: none # none | srt (Codex-only outer SRT wrapper) | ||
| command: srt | ||
| enable_weaker_network_isolation: false |
Summary: - Wait for terminal markers before reading the persisted row. - Leave production lifecycle event ordering unchanged. Rationale: - PubSub broadcasts happen before the run-store update returns. - CI can receive the event while the row still has only the startup marker. Tests: - mix test test/symphony_elixir/orchestrator_status_test.exs:3404 - MIX_TEST_PARTITION=2 mix test --cover --partitions 4 - mix format --check-formatted - make all
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Codex app-server runs for RSM-3734 hit stdout EAGAIN when Symphony event handling fell behind the OS pipe.
TL;DR
Drain Codex stdout in a dedicated process before event handling.
Summary
Alternatives
Test Plan
make allmix test test/symphony_elixir/app_server_test.exsmix specs.checkmix compile --warnings-as-errorsgit diff --check