🐛 Make piped xmd syntax output complete, and report a closed sink (#715) - #718
Merged
Conversation
…715) `xmd syntax` waits for stdout to accept the whole catalog, so a pipe receives exactly what a regular-file redirect receives however slowly it is read. A sink that closes mid-write is now reported on stderr with exit status 1 rather than ending the process with an unhandled write failure: a broken pipe arrives twice — at the write callback, and again as an `error` event a tick later — so the listener outlives the write. Three rows in `packages/cli/tests/syntax-cli.test.ts` prove it through a real pipeline against an oversize catalog. `runShell()` and `cliShellCommand()` are the test-support seam that composes one.
PR #718: 🐛 Make piped
|
The listener `process.stdout.on("error", …)` installed was never removed,
so it outlived the operation that owned it: repeated invocations would
accumulate listeners on a process-global stream, and a completed delivery
could absorb a later, unrelated stdout failure.
Delivery moves to `stdout-delivery.ts` behind a narrow `DeliverySink`, and
its listener now lives for exactly one delivery. A try/finally detaches on
every completion path — success, either failure order, cancellation, and a
`write` that refuses outright. The one path that waits is a failed arrival
whose paired event is still owed, and `errored` holding this delivery's own
failure is what says so: Node and Deno destroy the stream with the very
error they are about to emit, while Bun reports the failure once and holds
nothing. Waiting unconditionally would hang there.
Tier SD covers the lifetime a real pipe cannot show. SX13–SX15 and the
test-support pipeline seam are unchanged.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| yield* writeStdoutWhole(rendered); | ||
| // Only the catalog goes through delivery today, because it is the one | ||
| // output this command writes in a single call and the only one already | ||
| // past a pipe buffer. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // past a pipe buffer. |
| }); | ||
| } catch (error) { | ||
| // A stream that refuses the call outright never calls back, so | ||
| // nothing else will settle this. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // nothing else will settle this. |
Tier SD is the CLI secret-detection opt-out's, and it holds SD1–SD13 (`packages/cli/tests/secret-detection-cli.test.ts`). The delivery evidence took the same prefix, so two unrelated suites answered to one name. The delivery tier and its eight rows become SDL. Nothing else moves: the secret-detection tier keeps SD, and the implementation and SX13–SX15 are untouched.
…715) `deliverWhole` settled through a `new Promise` handed to `until()`, which forced an `observe` placeholder declared outside the executor so the `finally` could reach it. `withResolvers()` is Effection's own synchronous bridge, so the listener, the write and the detach now sit in one generator. Not `action()`: a file or a terminal calls the write callback synchronously, inside `write`, and an `action()` resolved before its executor has returned never runs the cleanup that executor returns (effection 4.1.0) — five SDL rows fail that way. The detach stays in a `finally`, which is where Effection puts synchronous cleanup.
| }); | ||
| } catch (error) { | ||
| // A stream that refuses the call outright never calls back, so nothing | ||
| // else will settle this. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // else will settle this. |
The error listener's lifetime is now a resource() of the delivery's own scope, the way @effectionx/node's `on()` binds a listener — attached before the write, detached in the resource's synchronous finally however the scope ends — rather than a try/finally around the wait. `scoped()` is what makes that scope the delivery's rather than the caller's: without it the listener outlives the call and seven SDL rows fail. `settled` is gone: withResolvers() ignores a second resolve, so the guard duplicated it.
| yield* writeStdoutWhole(rendered); | ||
| // Only the catalog goes through delivery today, because it is the one | ||
| // output this command writes in a single call and the only one already | ||
| // past a pipe buffer. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // past a pipe buffer. |
| } | ||
| // Identity, not presence: an error the stream was already holding before | ||
| // this delivery has been emitted already, and waiting for it would wait | ||
| // forever. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // forever. |
| }); | ||
| } catch (error) { | ||
| // A stream that refuses the call outright never calls back, so nothing | ||
| // else would settle this. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // else would settle this. |
4 tasks
SX15 fails under Bun on Linux and nowhere else: the command exits 0 with a truncated catalog when the consumer closes the pipe. The captured stderr is `xmd-exit=0` and nothing more — the diagnostic never printed, so delivery returned Ok. Bun 1.3.14's `process.stdout.write` callback reports success on a broken pipe there, and no `error` event follows it, so there is nothing for the command to report. macOS reports correctly on both 1.3.14 and 1.4.0, which is why only CI ever saw this. 1.4.0 is Bun rewritten in Rust, with 1,517 new Node.js test-suite passes. This defect is exactly that class. The release notes do not name it, so this commit is the experiment that answers it. The whole Bun corpus passes under 1.4.0 locally: 4713 tests across 286 files, 0 failures. `test-weights.json` still records 1.3.14 as its measurement provenance and wants a remeasure on a runner.
| } | ||
| // Identity, not presence: an error the stream was already holding before | ||
| // this delivery has been emitted already, and waiting for it would wait | ||
| // forever. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // forever. |
| }); | ||
| } catch (error) { | ||
| // A stream that refuses the call outright never calls back, so nothing | ||
| // else would settle this. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // else would settle this. |
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.
Closes #715.
Why
A consumer of the component catalog could receive truncated JSON.
xmd syntaxhanded the rendered catalog to
process.stdout.write()and could finish beforean asynchronous pipe write drained, so
xmd syntax --json | jqsaw a catalogthat ended mid-token while
xmd syntax --json > filewas whole. And when theconsumer closed the pipe first, the command died on an unhandled stream
errorevent with a raw stack trace instead of saying which output was cut short.
What changes
Before:
xmd syntax --json | catdelivered 81,920 bytes under Node and 65,536 underBun, of a 109,211-byte catalog.
jqreported invalid JSON;xmdexited 0.node:events:487 throw er; // Unhandled 'error' event.After:
Markdown and JSON forms, however slowly the reader consumes it.
xmd syntax: stdout did not accept the whole catalog: write EPIPEon stderrand exits 1.
How it works
deliverWhole()(packages/cli/src/stdout-delivery.ts) waits for the writecallback, which is what makes the write finish before anything can exit. The
waiting half landed with #696; this change adds the failure half.
A broken pipe can arrive twice: at the write callback, and again as an
errorevent a tick later. Anerrorevent nobody is listening for ends theprocess with a stack trace, so delivery listens — the first arrival is the
verdict and the duplicate is absorbed.
That listener lives for one delivery and no longer. It survives a failed
first arrival only until the paired event lands, and
erroredholding thisdelivery's own failure is what says one is still owed. Node and Deno destroy
the stream with the very error they are about to emit; Bun reports the failure
once and holds nothing, so waiting unconditionally would hang there. Success,
cancellation and a
writethat refuses outright all detach without waiting.The observer is
useErrorObserver(), a privateresource()that attachesbefore
provide()and detaches in a synchronousfinally, anddeliverWhole()is a
scoped()that acquires it, writes, and awaits the outcome inside thescope. That is effectionx's scope-bound event registration — a listener's
lifetime depends on a scope, never on its event firing — and the same shape as
@effectionx/node'son(). Awaiting inside the scope is what holds theobserver open for a trailing event; every other path leaves the scope and takes
the observer with it.
The outcome is bridged with
withResolvers()rather thanaction(): a file ora terminal calls the write callback synchronously inside
write, and ineffection 4.1.0 an
action()resolved before its executor has returned neverruns the cleanup that executor returns — which is exactly the common case here.
Measured:
Review guide
Start with:
packages/cli/tests/syntax-cli.test.ts, theTier SX — the catalog a pipe receivesblock.Then review:
specs/executable-mdx-spec.md— the delivery contract, beside the command'sexisting inspection-failure contract.
deliverWhole()inpackages/cli/src/stdout-delivery.ts, and thesyntaxcase in
packages/cli/src/cli.ts.runShell()/cliShellCommand()inpackages/test-support/launch.ts.Look carefully at:
sink.errored === failure— identity, not presence — so an error the stream was already holding cannot
cause a wait that never ends.
Tier SDLinpackages/cli/tests/stdout-delivery.test.ts. Tier SD is thesecret-detection opt-out's (Reject secrets before journal persistence by default #199); this tier is SDL to stay out of its way.
What must stay true
SDL1–SDL8, which count listeners on the stream around every completion path.
which compare a real pipeline against a real redirect.
status out of band because a pipeline reports its last stage's.
because a catalog that fits arrives whole however it was written.
How to verify it
Deno 5 passed (27 steps) · Node 27 pass 0 fail · Bun 27 pass 0 fail.
Every break below was applied and observed, not reasoned about:
process.stdout.write(rendered): SX13, SX14 and SX15 all failunder Node. Under Deno only SX15 fails — Deno's
process.stdoutflushes apipe on the way out, so it never showed the truncation. Recorded in the
suite's own comment: verifying completeness under Deno alone proves nothing.
errorlistener when the write callback settles: SX15 failsand the process dies with
Unhandled 'error' event, the string SX15 forbids.finallyteardown: all eight SDL rows fail.alone fails — one listener at emit where two were required.
the Bun case.
scoped(), leaving a plain generator: SDL1, SDL2, SDL3, SDL4,SDL5, SDL7 and SDL8 fail. SDL6 still passes, because that row supplies a scope
of its own — which is exactly why it cannot be the only lifetime row.
The runtime facts these rest on were measured against the real command, not
assumed:
erroredat callbackScope
Included
xmd syntaxcatalog, in both forms.Intentionally unchanged
process.stdout.write()sites, includingxmd plan's. Issue Make pipedxmd syntaxoutput complete #715scopes this to
xmd syntaxand not to a general rewrite of CLI output;xmd planwrites a plan document rather than a catalog and has not been observedpast the buffer.
New abstractions
deliverWhole()andDeliverySinkinpackages/cli/src/stdout-delivery.tsexist because the listener's lifetime is the claim, and a real pipe cannot
show it. The narrow sink lets Tier SDL supply the arrival orders one runtime
produces and another does not, and watch the listener come and go.
useErrorObserver()is private to that module and exists to make thelistener's lifetime a scope's rather than an event's.
@effectionx/node'sonce()cannot serve here: it installs its listenereagerly at call time and removes it only when the event fires, so a delivery
that succeeds — the common case, where no error event ever arrives — would
leave it attached (SDL1, SDL6 and SDL8). Its
on()is correctly scope-bound,but yields a
Streamthat can only be read by suspending, and thefirst-arrival-wins rule has to consult state synchronously inside the write
callback.
runShell()andcliShellCommand()in@executablemd/test-support/launchexist because SX13–SX15's subject is the pipeline — a file redirect and a
reader that closes early cannot be expressed by capturing a stream this
process owns. They reuse
runCli's environment, timeout and reporting;bounded()now takes the launch and a label rather than the argv.Scope confirmation