Skip to content

[mem] Backpressure worker subprocess output without dropping bytes - #30698

Merged
nwparker merged 5 commits into
mainfrom
OrcaWin/np-OOMs-10-19-more
Oct 22, 2026
Merged

nwparker merged 5 commits into
mainfrom
OrcaWin/np-OOMs-10-19-more

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​265 $\color{#cf222e}{\Huge{\mathbf{−}}}$​14 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​251
Prod 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​131 $\color{#cf222e}{\Huge{\mathbf{−}}}$​17 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​114

ELI5

A disk-space scan or Codex usage probe can produce output while Orca's main process is busy. Pause its output when too much is waiting, so the worker cannot queue the entire response in memory. Output-heavy commands may take longer while their reader catches up.

What Changed

The main process acknowledges each stdout or stderr chunk after its synchronous listeners return. The worker counts outstanding bytes per child, pauses both pipes at 256 KiB, and resumes as acknowledgements arrive. Each chunk owns its transferred backing buffer, avoiding a second payload copy.

Real-worker tests exercise the actual Codex usage reader with noisy stderr, initialization and RPC deadlines, aborts, and graceful stdin-EOF shutdown after output listeners detach. Sub-window replies are checked to continue without pausing.

Why

Acknowledgement-based flow control preserves complete output and its order. Dropping bytes can corrupt RPC messages; terminating commands changes their results; disk spooling would add storage and cleanup machinery. The existing caller limits run downstream and cannot prevent transport queues growing while the main process is stalled.

Linked Issue

Follow-up to the OOM audit in #30650. This PR protects worker subprocess transport; it does not establish the cause of a production OOM.

Visual Proof

N/A — internal subprocess transport and tests; no visual or interaction change.

Testing

  • Real subprocesses and worker threads tested locally on macOS with background launch enabled.
  • Automated tests cover bounded queue growth, complete stdout/stderr delivery, noisy RPC startup, both RPC deadlines, aborts, detached listeners, and graceful EOF shutdown. The EOF path used on Windows is exercised through the existing platform injection; native Windows was not exercised locally.
  • Worker and focused caller suites: 47 tests passed on the current base, with test concurrency limited to two workers on this shared host.
  • pnpm tc:node, changed-code quality, full formatting check, and commit hooks passed for the updated branch.
  • A real-worker stress experiment blocked the main thread for 500 ms: the old bridge queued up to all 32 MiB, while the flow-controlled bridge queued 256 KiB and subsequently delivered matching hashes.
  • Large-output throughput has a cost: the existing five-warm-run sample measured median 58 ms before / 75 ms after for 32 MiB stdout. Small-message latency measurements on this shared host were too variable to support a performance claim.
  • Native Linux, native Windows, and live SSH were not exercised locally. Full PR CI passed for bdfda9ca10a7514bba948590c7bc8b5a7617586b, including all five test shards, relay integration, static analysis/typechecking, Linux and Windows packaging, and the required aggregate gate: https://github.com/stablyai/orca/actions/runs/39723529707.

Review

The 256 KiB value is a pause threshold, not a strict memory cap: chunks and closing pipes can overshoot it. The window is per child and bounds transport payload only; callers can still retain output, and producers that ignore backpressure can buffer in their own process. Draft #31408 evaluates a separate 16 MiB pending-line budget. Its producer investigation reproduced complete HTTP response bodies being copied into oversized usage errors. That receiver guard is defensive hardening, not a producer or production-OOM root-cause fix; its response-size policy remains under review.

Existing deadlines remain unchanged. Waiting for output can make a child or caller miss a deadline it previously met; a synthetic child with its own deadline reproduced that tradeoff. A noisy stderr stream can pause stdout because both pipes share the window. The real caller tests cover recovery, deadline expiry, and shutdown without extending deadlines or silently discarding output.

The acknowledgement protocol is internal to a worker and its owning process. Paired SSH, relay, and mobile protocols are unchanged. No changes to interactive terminal output are included.

Agent skill upstream boundary

  • Not applicable: no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation are copied or mechanically translated.

Notes

This change uses Node pipe pause/resume and worker buffer transfer. It adds no output-size cap or new timeout to the bridge.

Checklist

  • The PR is focused on worker output transport and its callers' regression coverage.
  • The before/after, mechanism, alternatives, and limitations are explained.
  • Visual proof is N/A with the reason above.
  • Self-reviewed for correctness, security, and performance.
  • Cross-platform, SSH, and compatibility impact considered.
  • Full repository checks and packaging pass in CI for the exact PR commit.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fe9322b1-d184-4799-9671-571ca0f1d039
📥 Commits

Reviewing files that changed from the base of the PR and between 5750139 and bdfda9c.

📒 Files selected for processing: 1
  • src/main/rate-limits/codex-rpc-worker-backpressure.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The worker child-process protocol now acknowledges consumed stdout and stderr bytes. The server tracks pending output, transfers owned buffers, and pauses or resumes both streams based on a 256 KiB threshold. New tests cover output ordering, queue limits, and stream integrity. Additional tests exercise Codex RPC replies, timeouts, and aborts while output is backlogged. Shared worker test fixtures provide entry bundling, host setup, and cleanup.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to bdfda

The fixture now starts noisy output after initialization, and no unresolved concrete merge risk is established. The change appears ready for normal CI.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 11 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: adding backpressure to worker subprocess output without dropping bytes.
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.
Description check Passed The pull request description is complete and follows the repository template. It explains the change, motivation, linked issue, testing, limitations, compatibility impact, and checklist status. Visual…
Full details: Description check

Explanation

The pull request description is complete and follows the repository template. It explains the change, motivation, linked issue, testing, limitations, compatibility impact, and checklist status. Visual proof is correctly marked N/A for an internal transport change. The linked issue is referenced as #30650, and the reported CI and local test results provide clear verification details.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@nwparker
nwparker force-pushed the OrcaWin/np-OOMs-10-19-more branch from 57dcebb to 2413433 Compare October 20, 2026 01:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2b642abc-8fcf-473b-8c94-23b600bf08e3
📥 Commits

Reviewing files that changed from the base of the PR and between 57dcebb and 3a170c9.

📒 Files selected for processing: 8
  • config/scripts/vitest-node-runtime-files.mjs
  • src/main/managed-server/lifecycle/orcad-managed-tunnel-identity-restart.test.ts
  • src/main/rate-limits/codex-rpc-worker-backpressure.test.ts
  • src/main/worker-thread-child-process/worker-thread-child-output-flow.test.ts
  • src/main/worker-thread-child-process/worker-thread-child-process-worker.test-fixtures.ts
  • src/main/worker-thread-child-process/worker-thread-child-process.test.ts
  • src/main/wsl/wsl-orcad-connection.ts
  • src/main/wsl/wsl-orcad-pairing-identity.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@nwparker
nwparker merged commit eba0124 into main Oct 22, 2026
36 checks passed
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.

2 participants