Drain broker stdout after SDK startup#838
Merged
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis 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. ChangesBroker Stdout Drain Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (7)
.trajectories/completed/2026-05/traj_mz5m5ysjj31e.json.trajectories/completed/2026-05/traj_mz5m5ysjj31e.md.trajectories/index.jsonpackages/sdk-py/src/agent_relay/client.pypackages/sdk-py/tests/test_wait_for_api_url.pypackages/sdk/src/__tests__/client-stdout-drain.test.tspackages/sdk/src/client.ts
3 tasks
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.
khaliqgant
added a commit
that referenced
this pull request
May 11, 2026
* fix(sdk): drain broker stderr alongside stdout after startup PR #838 added `drainBrokerStdoutAfterStartup` to keep the broker's stdout pipe drained after the SDK finishes parsing the startup URL, preventing the broker from blocking on `write()` when the parent stops reading. **Stderr has the same failure mode** but was never covered. The Rust broker routes `tracing` output to stderr (`.claude/rules/rust.md`). Under heavy fanout (e.g. 9 PTY workers in proactive-runtime M1), tracing volume is large enough to fill stderr's ~64KB kernel pipe and block the broker process exactly the way stdout did before #838. Today the existing `createInterface({ input: child.stderr })` (around client.ts:248) keeps stderr in flowing mode during normal operation by attaching a `'line'` listener that pushes into `stderrLines`. That works — except when the readline parser falls behind, or under buffered-flush boundary conditions where the stream momentarily falls back to paused mode. A no-op `'data'` listener + explicit `.resume()` gives a belt-and-suspenders guarantee that stderr is always drained. Changes - `packages/sdk/src/client.ts` - Rename `drainBrokerStdoutAfterStartup` → `drainBrokerStdioAfterStartup` (it's now accurate — covers both streams). - Iterate over `[child.stdout, child.stderr]` and attach the no-op drain handler + `.resume()` to each. - Update the single call site at `client.ts:264` to the new name. - `packages/sdk/src/__tests__/client-stdout-drain.test.ts` - Extract the fake-broker generator + the drain-or-block harness into helpers parameterized by stream name. - Existing stdout test now floods the named stream; add a parallel test that floods stderr and asserts the broker exits within the same 5s window. - Bump per-test timeout to 30s so spawn + drain + child-exit has headroom past vitest's 5s default (the inner BROKER_STDIO_DRAIN_TIMEOUT_MS is still 5s — what's measured is the broker's exit timing, not the test wall-clock). Verification - `npx vitest run src/__tests__/client-stdout-drain.test.ts` — **2/2 pass** Related - relay#838 — original stdout-only drain - ricky#94 / #98 — Node-side child_process.spawn monkeypatch (backstop for consumers still on older SDKs) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drain broker stderr after SDK startup 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. * test(sdk): reduce drain test chunk count to avoid timing races Drop the broker-flood chunk count from 20000 (~20MB) to 5000 (~5MB) so the line-by-line readline parsing of stderr in the SDK startup path never races the drain assertion timeout. Still forces ~80 backpressure cycles against the ~64KB macOS pipe buffer — more than enough to wedge an undrained broker. * test(sdk): assert broker stdio drain attachment --------- Co-authored-by: Proactive Runtime Bot <agent@agent-relay.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
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
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.