Skip to content

fix: respect output stream backpressure - #122

Closed
gnapse wants to merge 1 commit into
mainfrom
ernesto/fix-output-backpressure
Closed

fix: respect output stream backpressure#122
gnapse wants to merge 1 commit into
mainfrom
ernesto/fix-output-backpressure

Conversation

@gnapse

@gnapse gnapse commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Large IDs-only and NDJSON results can exceed stdout's writable buffer. The existing emitters could keep producing output after write would have returned false, which allows memory usage to grow when the downstream consumer is slow.

This change:

  • adds a shared buffered line writer that waits for stdout to drain
  • makes outputIds asynchronous and backpressure-aware
  • adds the backpressure-aware outputNdjson public helper
  • migrates cli-core's own NDJSON emitters to await the new writer
  • keeps the synchronous formatting helpers for callers that need strings

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR makes stdout emission backpressure-aware via a shared buffered line writer, migrating outputIds and cli-core's NDJSON emitters to it while keeping the synchronous formatting helpers intact.

Few things worth tightening:

  • Attach error handling around every in-flight write, not just when write() returns false — a small write can later emit EPIPE (e.g. --ndjson | head -n 0) with no listener, raising an unhandled error event.
  • In the accounts emitter, generate payloads lazily (e.g. pass an iterable yielding one payload at a time) instead of accounts.map(toPayload), which materializes the full result set and defeats backpressure for large lists.
  • Minor: the new outputNdjson helper changes the error contract that formatNdjson explicitly guarantees — worth documenting the difference.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (4)
  • P3 src/json.ts:43: Unlike formatNdjson, which validates every item before any byte is written, outputNdjson flushes chunks as it goes — so a non-serializable item late in a large stream (e.g. a function/symbol at index 50,000) leaves the earlier chunks already on stdout before the TypeError rejects. That's an inherent tradeoff of streaming, but it's a behavioral change from the all-or-nothing contract the sibling helper documents. Worth a sentence in this JSDoc noting that a mid-stream serialization failure can leave partial output behind.
  • P3 src/commands/update.test.ts:87: This hand-rolled process.stdout.write spy duplicates captureStream from ../testing/console.js (or the installCapturedStream wrapper from ../test-support/cli-harness.js), which the sibling suites in this same PR (account.test.ts, status.test.ts) already use. Reuse it instead: import { captureStream } from '../testing/console.js' and assign stdoutSpy = captureStream(). Unlike () => true, captureStream's silentWrite also invokes the optional write callback on a microtask, so it stays correct if a write path ever passes write(chunk, cb).
  • P3 src/stream.test.ts:14: This test only exercises the oversized-single-line flush path (chunkBytes >= chunkSize) — one line of exactly writableHighWaterMark bytes forces an immediate flush. The realistic scenario motivating this PR is many small ID/NDJSON lines whose accumulated bytes cross the buffer, which goes through the separate chunk && chunkBytes + lineBytes > chunkSize branch and is never covered. If that accumulation branch regresses (e.g., the pre-flush check is dropped or its > comparison breaks), the test still passes while real large result sets lose backpressure. Add a case with enough small items to exceed the high water mark (asserting the first write waits on drain before subsequent writes proceed) so the accumulation path is actually protected.
  • P3 src/json.test.ts:73: Two of these three tests re-test behavior already covered elsewhere: "writes NDJSON to stdout in one block" and "writes nothing for an empty iterable" mirror the writeLines tests in stream.test.ts (and the identical ids.test.ts outputIds cases), and the non-serializable-index test re-exercises stringifyNdjsonItem, which formatNdjson's "throws with the bad index" test covers with the same [1, undefined, 2] input and regex. Since outputNdjson is a thin wrapper, keep a single wiring test (the happy-path one) and delete the other two.

Share FeedbackReview Logs

Comment thread src/stream.ts
const output = chunk
chunk = ''
chunkBytes = 0
if (!process.stdout.write(output)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P2 Handle asynchronous write errors for every chunk, not only after write() returns false. A small write can return true and later emit EPIPE when a downstream pipe closes (for example, --ndjson | head -n 0); this direct write has no error listener and Node raises an unhandled error event. Install temporary error handling around each in-flight write and propagate or explicitly handle the failure.

Comment thread src/auth/account.ts
// both flags are set. Empty list → no lines (EOF-as-end-of-stream).
if (view.ndjson && !view.json) {
for (const entry of accounts) console.log(formatNdjson([toPayload(entry)]))
await outputNdjson(accounts.map(toPayload))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P2 Keep payload generation lazy here. accounts.map(toPayload) invokes renderJson and retains every generated payload before writeLines can apply backpressure, so a large account list or large custom payloads can still grow memory with the full result set. Pass a generator/iterable that yields toPayload(entry) one entry at a time.

@gnapse

gnapse commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Closing without merging. The backpressure concern is technically valid, but we have no reports of it causing problems in practice. Addressing it makes the output helpers asynchronous and requires broad changes across callers and tests. That cost is not justified for now. We can revisit this if we get a real-world report or start streaming result sets that are large enough to make backpressure material.

@gnapse gnapse closed this Aug 27, 2026
@gnapse
gnapse deleted the ernesto/fix-output-backpressure branch August 27, 2026 16:41
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