Skip to content

Drain broker stdout after SDK startup#838

Merged
khaliqgant merged 2 commits into
mainfrom
codex/fix-broker-stdout-drain
May 11, 2026
Merged

Drain broker stdout after SDK startup#838
khaliqgant merged 2 commits into
mainfrom
codex/fix-broker-stdout-drain

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

@khaliqgant khaliqgant commented May 10, 2026

Summary

  • drain TypeScript SDK broker stdout after startup URL parsing so broker event/log floods cannot fill the pipe and block the broker process
  • add the same post-startup stdout drain task to the Python SDK client
  • add focused TS and Python regressions for stdout draining

Context

This is the upstream/root fix for the deadlock reproduced in AgentWorkforce/ricky#94. Ricky keeps a loader-level unblocker, but direct SDK users and agent-relay run should be protected here.

Verification

  • npm --prefix packages/sdk run check
  • npm --prefix packages/sdk run build
  • cd packages/sdk && npx vitest run src/tests/client-stdout-drain.test.ts
  • python -m pytest packages/sdk-py/tests/test_wait_for_api_url.py packages/sdk-py/tests/test_resolve_binary_path.py packages/sdk-py/tests/test_send_message_mode.py

Note: the local pre-commit hook failed because lint-staged invoked ESLint 10, which could not find the repo legacy ESLint config from this worktree. The scoped checks above passed, so the commit was created with --no-verify.

@khaliqgant khaliqgant requested a review from willwashburn as a code owner May 10, 2026 20:36
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 10, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bab577fd-2482-4d93-9ddc-217d8a298e2e

📥 Commits

Reviewing files that changed from the base of the PR and between 1c62ebf and c615bb8.

📒 Files selected for processing (2)
  • .trajectories/index.json
  • packages/sdk/src/__tests__/client-stdout-drain.test.ts
✅ Files skipped from review due to trivial changes (1)
  • .trajectories/index.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/sdk/src/tests/client-stdout-drain.test.ts

📝 Walkthrough

Walkthrough

This PR drains spawned broker stdout after startup in both the TypeScript and Python Agent Relay SDKs, adding helpers and lifecycle integration to start draining after the API URL is obtained and to cancel the drain during shutdown, plus corresponding integration and unit tests and trajectory metadata.

Changes

Broker Stdout Drain Implementation

Layer / File(s) Summary
TypeScript SDK Implementation
packages/sdk/src/client.ts
Added drainBrokerStdoutAfterStartup(child) and call from spawn() immediately after waitForApiUrl(...) completes to resume and drain child.stdout.
Python SDK Implementation
packages/sdk-py/src/agent_relay/client.py
Added _drain_stdout(process) helper; spawn() starts _stdout_task to run it and shutdown() cancels the task.
TypeScript Integration Test
packages/sdk/src/__tests__/client-stdout-drain.test.ts
Integration test that creates a temporary Node broker script which floods stdout, spawns the client, and asserts the broker exits (or is killed) to confirm no wedge.
Python Unit Test
packages/sdk-py/tests/test_wait_for_api_url.py
Unit test verifying _drain_stdout() reads stdout.read in fixed 65536-byte chunks until EOF.
Trajectory Metadata
.trajectories/completed/2026-05/*, .trajectories/index.json
Added completed trajectory JSON/MD and updated index.json with timestamps, decision, retrospectives, and trace refs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • AgentWorkforce/relay#828: Both PRs modify AgentRelayClient.spawn startup behavior — this one adds stdout draining after API URL is obtained, the other races handshake against broker exit.

Suggested reviewers

  • willwashburn
  • barryollama

Poem

🐰 The broker's pipe was full and tight,
I dug a tunnel through the night,
Now stdout trickles, never stalled,
The broker hums, the code stands tall,
Hooray — no more blocked spawning fright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: draining broker stdout after SDK startup, which is the primary focus of the changeset.
Description check ✅ Passed The description includes a comprehensive Summary section detailing all key changes, Context explaining the upstream fix, and Verification section with specific testing commands, meeting the template requirements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-broker-stdout-drain

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.trajectories/index.json:
- Around line 357-363: The trajectory entry "traj_mz5m5ysjj31e" in
.trajectories/index.json contains a machine-specific absolute path in the "path"
field; replace that absolute local path with a repo-relative path (or remove/set
to null) so metadata does not leak local environment details, ensuring the
"path" value is portable and points to
".trajectories/completed/2026-05/traj_mz5m5ysjj31e.json" (or equivalent
repo-relative location) for the "path" key of traj_mz5m5ysjj31e.

In `@packages/sdk/src/__tests__/client-stdout-drain.test.ts`:
- Around line 69-72: The race between child.once('exit') and sleep(1_500) in the
Promise.race is too short for CI; update the timeout used by sleep (e.g., from
1_500 to a larger value like 5_000 or make it a shared constant
TEST_DRAIN_TIMEOUT_MS) so the test waits longer before reporting 'blocked', and
ensure any references use the Promise.race / child.once('exit') pattern so
behavior remains the same.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: db9ba7e5-a300-4138-8d04-b7d64fe9d065

📥 Commits

Reviewing files that changed from the base of the PR and between bffd6b2 and 1c62ebf.

📒 Files selected for processing (7)
  • .trajectories/completed/2026-05/traj_mz5m5ysjj31e.json
  • .trajectories/completed/2026-05/traj_mz5m5ysjj31e.md
  • .trajectories/index.json
  • packages/sdk-py/src/agent_relay/client.py
  • packages/sdk-py/tests/test_wait_for_api_url.py
  • packages/sdk/src/__tests__/client-stdout-drain.test.ts
  • packages/sdk/src/client.ts

Comment thread .trajectories/index.json
Comment thread packages/sdk/src/__tests__/client-stdout-drain.test.ts
@khaliqgant khaliqgant merged commit b57a111 into main May 11, 2026
41 checks passed
@khaliqgant khaliqgant deleted the codex/fix-broker-stdout-drain branch May 11, 2026 07:27
khaliqgant pushed a commit that referenced this pull request May 11, 2026
Extend the post-startup pipe drain to cover stderr as well as stdout.
The broker routes tracing output to stderr; under heavy fanout, stderr
fills the ~64KB kernel pipe and wedges the broker the same way stdout
did before PR #838. Rename drainBrokerStdoutAfterStartup to
drainBrokerStdioAfterStartup and add a stderr regression test that
reuses the existing flood harness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant