fix(pi): stream stdout to disk + NDJSON parsing to bound memory - #94
Open
Zlatanwic wants to merge 4 commits into
Open
fix(pi): stream stdout to disk + NDJSON parsing to bound memory#94Zlatanwic wants to merge 4 commits into
Zlatanwic wants to merge 4 commits into
Conversation
The subprocess pi adapter used renderPiBaseUrlOverride, which writes only
the baseUrl override. pi's openai provider then defaults custom models to
openai-responses (POST {baseUrl}/responses); non-OpenAI backends
(DeepSeek, vLLM, any OpenAI proxy) don't implement Responses and return
404. Switch managed mode to renderPiModelRegistration, which registers the
model with api: 'openai-completions' so pi uses /chat/completions —
matching the headless library driver that already did this correctly.
Discovered running the pi adapter against DeepSeek via skvm run.
Long TB tasks (tb-circuit-fibsqrt, tb-feal-*-cryptanalysis) crashed with
RangeError: Out of memory. Root cause: piEventsToRunRecord received the
full PiEvent[] from parsePiNDJSON, which materialized every one of pi's
~30k emitted events (message_update / thinking / *_delta streaming
deltas make up ~99.9% of a 0.3-1.7 GB transcript). The builder only
consumes agent_end + message_end (~25 events); the other 30k parsed
objects sat in a retained array alongside the buffered stdout string,
driving peak heap to 10-32 GB.
Add piBuildRunRecordFromNDJSON: scan the stdout string with charCodeAt
(no split() array), pre-filter each line with .includes('"type":"agent_end"')
or '"type":"message_end"' to skip JSON.parse on 99.9% of lines, and
retain only the two event types the builder actually reads. Behaviorally
equivalent to the old path — a dedicated equivalence test compares
parsePiNDJSON+piEventsToRunRecord vs the new function on clean and
message_end-fallback transcripts.
Extracts piMessagesToRunRecord as a shared helper so streaming and
full-events paths cannot drift. piEventsToRunRecord retained for the
headless driver which already holds events in memory.
Tests: 29/29 pass in test/adapters/pi.test.ts (adds equivalence,
noise-skipping, and message_end-fallback cases). Verified against the
tb-circuit-fibsqrt log that previously OOM'd.
Follow-up to f8603a9: parser-side fix bounded RunRecord memory, but the
raw stdout string itself (0.3-1.7 GB for a long agentic transcript) was
still fully buffered by runSubprocess before pi.ts even saw it. Bench
resumed on f8603a9 crashed at RSS=12.16 GB / peak=18.61 GB with a Bun
segfault (SIGSEGV at a 2.29 TB virtual address, characteristic of a
very large string allocation) after 7 tasks. The Layer-1 fix cut
retention of parsed events; it did nothing about the raw string.
Layer 2 fix: give runSubprocess an optional stdoutSink option that
streams stdout bytes verbatim to a file lazily (no empty file if the
child produces nothing) with the same reader-cancellation semantics as
the existing string path. Pi adapter wires task.convLog.filePath in as
the sink — pi never populates convLog via logRequest/logResponse, so
finalize() early-returns on an empty entries[] and the streamed file
survives. Then piBuildRunRecordFromFile streams the same file back
line-by-line into the shared PiEventCollector (extracted from the two
existing paths so they cannot drift), retaining O(longest-line) bytes
instead of O(transcript).
Wall of writes:
before: pi -> stdout pipe -> stdout string (1 GB) -> parser
-> convLog file
after: pi -> stdout pipe -> convLog file
-> parser (stream reads back)
Peak heap projection: from 18.61 GB observed -> a few MB per task.
Also collapses the previous dual-write (buffer + convLog) into one.
Test files (test/adapters/pi.test.ts, test/core/subprocess.test.ts) are
scoring-adjacent under the PUA integrity policy and are intentionally
left uncommitted here for separate verifier ownership.
This was referenced Jul 7, 2026
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds memory-safe handling of large pi NDJSON transcripts by streaming subprocess stdout to disk and parsing only relevant NDJSON events, preventing OOM on long/verbose runs.
Changes:
- Add
stdoutSinksupport inrunSubprocessto stream stdout directly to a file (lazy-created). - Introduce
piBuildRunRecordFromNDJSON/piBuildRunRecordFromFileto build RunRecords without materializing all NDJSON events in memory. - Update
PiAdapterto use stdout streaming + file-based parsing, and register openai-compatible models to force/chat/completions.
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 |
|---|---|
| test/core/subprocess.test.ts | Adds coverage for stdoutSink behavior (verbatim streaming, large output, timeout flush, lazy create). |
| test/adapters/pi.test.ts | Adds tests ensuring new NDJSON builders match old semantics and handle noisy/large transcripts. |
| src/core/subprocess.ts | Implements stdout streaming to disk and exposes stdoutFile in results. |
| src/core/pi-runtime.ts | Refactors message→RunRecord logic + adds memory-efficient NDJSON parsing (string + file variants). |
| src/adapters/pi.ts | Switches adapter to streaming stdout + file parsing and improves openai-compatible model registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Three memory-safety fixes to the pi adapter for handling giant LLM transcripts (0.3–1.7 GB) that previously drove heap to 10–32 GB and threw
RangeError: Out of memory.1. Use openai-completions API for openai-compatible routes
Pi's built-in
openaiprovider defaults custom-registered models to the Responses endpoint (/responses). Non-OpenAI openai-compatible backends (DeepSeek, vLLM, proxies) only speak/chat/completions, so the default routing yields 404. This pinsapi: "openai-completions"in model registration to force the correct endpoint.2. Stream NDJSON to bound RunRecord memory
Real pi transcripts emit ~99.9% noise (message_update/thinking deltas). The old path parsed the entire NDJSON string into a
PiEvent[]array (gigabytes), then built the RunRecord. The new path (piBuildRunRecordFromNDJSON) streams the NDJSON line-by-line and retains only the finalagent_endevent, collapsing memory from GB to KB.3. Stream stdout to disk to eliminate 1 GB heap-resident string
For subprocess-driven adapters (pi, opencode, claude-code), buffering the child's stdout into
result.stdoutas a single string hits the V8 string length limit (~1 GB) and crashes. This adds astdoutSinkoption torunSubprocessthat streams raw stdout bytes directly to a file, bypassing the in-memory accumulation. The pi adapter uses this to write the convLog file during the run instead of post-facto, cutting peak heap by 10x.Why it's standalone
These are pi adapter + pi-runtime internals plus a new optional parameter to
runSubprocess(stdoutSink). The option is purely additive — existing callers unchanged, no breakage. Thesubprocess.tschange here is independent of PR #93's process-tree/MSYS work (different regions, different concerns).Conflict note: If PR #93 merges first, this PR's
subprocess.tswill need a trivial rebase (the sink machinery and the reader-cancellation machinery touch overlapping lines but are orthogonal features).Test plan
bunx tsc --noEmitpassespiBuildRunRecordFromNDJSON(5000-line noise, agent_end fallback, tool-call preservation)piBuildRunRecordFromFile(file-based streaming path)runSubprocessstdoutSink option (lazy file creation, large output)Scope
Additive only. No breaking changes. The
stdoutFileresult field is new but optional.