feat(shell): job console for streaming background output - #611
Conversation
Background jobs stored their output in two byte fields that were only readable once the job ended, so nothing could watch a running job and a reader had no way to say where it had got to. Replace them with a JobConsole: an append-only log of timestamped chunks addressed by sequence number, where a reader's whole state is one integer. Readers can join late, read from any position, and disappear without the console noticing. The job's ending is an in-band CONTROL chunk, so "is it done" and "what did it print" are the same question. - mirage/shell/console/: config, store protocol, RAM store, JobConsole. The RAM store keeps a waiter registry keyed by event loop, so a reader parked on another thread and loop is woken safely. - Job drops stdout/stderr for a console. All writes to a job's status, exit code, and ending happen in one place, so the table has a single writer and the polling _refresh is gone. - kill and kill_all are async and join on the console, so a killed job is settled before kill returns. - Thread the console down the executor as an output sink. Sequencing constructs (loops, lists, groups, subshells, conditionals) pass it to their children, so each statement lands as it finishes instead of the whole construct arriving at the end. Capture sites (command substitution, pipe stages, redirects) do not inherit it and keep receiving their output as a value. - TypeScript: connect the job's AbortController to the executor, which already checked the signal but was never given one. kill now stops a running job, so it joins like Python instead of settling the job itself. - Drop the unreachable "no job table" branch in handle_background: the workspace always has one, so the console is never optional. Snapshots keep the existing stdout/stderr byte format; a restored job rebuilds a finished console from it.
…overage The battery had no case that backgrounded a compound construct: all eight existing job cases background a simple command, so the path this branch rewrote was untested. Adding those cases surfaced a real bug. `wait` with no operand waited for every job and then discarded what they printed, while `wait <id>` returned it. A real shell has nothing to adopt because its jobs share the terminal and have already printed; mirage jobs print to their console, so the shell has to surface it or the output is stranded. This was pre-existing, not introduced here. - Bare `wait` now concatenates every unreaped job's console in job-id order, then reaps. Not just the running ones: a job that finished before the line was reached still has output nobody has read, and whether it finished in time is a scheduling accident. Reaping keeps a second `wait` from reprinting, and matches GNU, where `jobs` prints nothing once `wait` has returned. - 18 integ cases in bash/jobs/compound.json covering backgrounded for, while, if, &&, subshell and brace groups, plus the capture sites that must not leak (command substitution, pipe stages, redirects) and stderr routing. Both hosts: 1629 passed, 0 failed. - Unit coverage for bare `wait` in both languages. The existing "completed jobs cleared after listing" test now uses `wait %1`, which waits without reaping, so it still exercises what it was written for; bare `wait` gets its own case.
`((echo a); echo b)` failed with a syntax error. tree-sitter-bash lexes `((` as the arithmetic opener and the lexer cannot back out, so a subshell that immediately opens another subshell never parses. Bash resolves the same ambiguity by trying the arithmetic command first and reparsing as nested subshells when that fails. Do the same: on a tree that already has an error, split the `((` openers sitting inside the error and keep the retry only if it parses cleanly. Commands that parse today are untouched, so no working command's offsets move, and the original error is preserved when no reparse helps. The guard is the subtle part. Splitting only openers inside an ERROR subtree is NOT sufficient: tree-sitter's error region swallows neighbouring tokens, so a valid `((i++))` next to a bad opener also reports as errored, and splitting it silently turns arithmetic into a subshell running `i++` as a command. That is a wrong parse rather than a rejected one, which is worse. An arithmetic construct has to close with `))`, so a line containing no `))` anywhere cannot hold a real arithmetic opener and splitting it is provably safe; lines that mix both keep their error. Covered by unit tests in both languages, including the mixed line that must stay rejected, plus four integ cases in bash/subshell/nested.json. Both hosts: 1633 passed, 0 failed.
…region The first cut bailed out whenever the line contained `))` anywhere, which left `i=1; ((i++)); ((echo x); echo $i)` rejected even though bash runs it. That guard was sound but far too broad. Judge each opener separately instead, by parsing its balanced span on its own: `((i++))` stands alone cleanly and is left untouched, while `((echo x); echo $i)` does not and gets split. This is what makes a valid arithmetic command safe when it shares a line with a broken opener, which scope alone cannot do because tree-sitter's error region covers both. The span scan skips parens inside quotes and backslash escapes, so a literal `")"` cannot throw off the depth, and an unbalanced span is assumed arithmetic and left alone. Newly working, all matching GNU: i=1; ((i++)); ((echo x); echo $i) -> x 2 ((echo ")"); echo b) -> ) b ((echo a); echo b); ((echo c); echo d) -> a b c d ((echo a) && (echo b)) -> a b `(((echo d)))` still errors, which matches GNU: bash rejects it too. Four more integ cases in bash/subshell/nested.json, both hosts 1637 passed 0 failed, plus unit tests in both languages.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9230bca3d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rse byte offsets - Targeted `wait %N`/`fg` now reap the job after adopting its output, so a later bare `wait` cannot replay the same console (GNU bash deletes a job waited on by id; pinned with docker bash 5.2). Job numbering restarts at 1 once the table empties, matching GNU, so repeated `cmd & wait %1` keeps working after reaps. - RAM console stores never trim the terminal CONTROL chunk: evicting it left wait_finished()/follow() blocked forever under byte budgets smaller than the outcome payload. - Python parse: _balanced_end/_is_arithmetic now scan the encoded bytes, so tree-sitter's byte offsets index correctly when multibyte text precedes an ambiguous `((` opener (e.g. `echo é; ((echo a); echo b)`). - TS parse: hoist isArithmetic to module scope (no nested functions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c3181909
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… never notice the abort `kill` aborted the job then joined on the console's finish. But the abort is only observed where something checks it (the executor between nodes, and commands that take the signal), so a job sitting inside one long command never sees it and `kill` waits forever on exactly the runaway job it is trying to stop. Every existing kill test used a runner that does observe the abort, so the join looked safe. kill now settles the job itself — abort, mark KILLED/137, emit `Killed`, finish the console — and does not join. The console drops emits after the ending chunk, so a runner still unwinding cannot append past its own death. Python also gains the `status != RUNNING` guards in `_settle` that TypeScript already had, so a runner completing in the window after `kill` (cancel is deferred via call_soon_threadsafe) cannot relabel a killed job. Regression test added in both languages with a runner that ignores the abort; it times out on the pre-fix code and passes after.
|
Pushed
Regression test added in both languages with an abort-ignoring runner — times out on the pre-fix code, passes after. TS core 4935 green, py tests/shell+tests/workspace green, pre-commit clean. |
|
@claude review |
There was a problem hiding this comment.
Beyond the inline nit on subRecurse dropping opts in the subshell branch, I also checked whether arithmetic (( )) inside a backgrounded compound construct could fail to stream its output or lose it on kill — it doesn't exhibit that issue.
Extended reasoning...
This run's only new item beyond the inline finding is a ruled-out candidate: a verifier checked whether arithmetic (( )) commands running inside a backgrounded compound construct (e.g. ( ((i++)) ) &) fail to stream output incrementally or lose output when killed, similar to the subRecurse opts-forwarding nit. That candidate was investigated and refuted — arithmetic commands don't hold long-lived streams the way pump()/console emission paths do, so there's no equivalent gap there. Recording this so it isn't re-explored from scratch in a future pass; it's not a guarantee of correctness elsewhere in this large PR.
Resolve the job-console work against main's executor and workspace rewrites, and fix the one open review finding. Conflicts: - shell/parse: strip the line continuation, then run the (( reparse. TypeScript now scans the stripped source throughout, like Python. - executor/jobs: keep the console runner, drop the [N] launch line that main removed. - node/execute_node: thread the sink through main's finish_statement, apply_barrier and pre-expanded case patterns. C-style for is a sequencing construct, so it streams too. - snapshot/state: async per-channel job snapshot beside main's CLI snapshot. - executor/command: take main's JOB_HANDLERS table. - workspace: main split workspace.py into a package, so the job teardown moves into workspace/lifecycle, still requesting the cancel without joining. The merge also dropped jobs.ts's concat helper, which left two call sites on an undefined name; use the existing io/cachable_iterator one. subRecurse was a four-parameter closure, so the nested job's console and abort signal were silently dropped (TypeScript accepts this because function parameters are bivariant). It now forwards opts like recurse and stream, with a regression test in both languages. cfor_readonly_aborts made the bare name i readonly, and the battery shares one session, so it broke the later ((i++)) cases. Rename it to zcfi, matching the scoped names the other readonly cases use.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a1f07a8d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two of the four codex findings on the merge were real. Teardown settled jobs on main, where kill was sync. This branch made kill async and teardown dropped to a bare cancel, so a job stayed RUNNING with no ending chunk and anyone parked on wait_finished waited forever on a workspace that was already gone. Both close paths are async, and kill no longer joins the runner, so they await kill_all instead. It runs before any resource closes, so a job cannot keep touching one that is already gone. The sync half keeps the bare cancel as a last resort for a caller with no loop at all. ConsoleStore.close promised to wake blocked readers, but follow() and wait_finished() loop: woken, they re-read, find no CONTROL chunk, and wait again on a console nobody will write to. Stores now carry closed state, wait() returns immediately once set, and both loops end on it. Regression tests in both languages, each verified to fail without its fix. They use an abort-ignoring runner rather than sleep, which is the one command that consumes the signal and would pass either way, and they release that runner in a finally so a failure cannot turn into a hang at loop teardown.
A streaming command's status can depend on its content (grep's exit_on_empty settles the origin only at drain time), so merge() no longer copies the number: the merged result links to its right-hand original and reads through the link, always fresh. An explicit write stores locally and severs the link, so aggregated or overridden statuses still win (issue #43). Deletes sync_exit_code/syncExitCode and every call site in both languages; the old one-shot sync also detached on first use, so a sync before the drain froze a stale 0 forever, a hazard class the delegating read removes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d384242f11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
KILLED_OUTCOME moves to constants, Channel/ConsoleChunk/ReadResult to types (ReadResult joins from store.py, mirroring where TS already kept it), exit_outcome to utils, per the module-layout convention; config.py held no configuration. Same split in TypeScript.
kill and settle both flip the status before their final appends, so a status-based return let a waiter snapshot and reap a killed job before the Killed marker or ending chunk was persisted: a waiter on another loop today, any store that suspends tomorrow, and in TypeScript any concurrent waiter, since every await yields a microtask. wait now joins wait_finished for live jobs (restored jobs keep the no-task fast path), and wait_all joins every job rather than only the running ones, since bare wait snapshots killed jobs too. Gated-store regression tests in both languages, verified failing against the old wait.
Background jobs become observable: output moves from two after-the-fact byte fields into a live, seekable console, compound bodies stream into it as they run,
wait/fg/jobs/killadopt bash's job-control semantics over it, and teardown settles jobs instead of stranding their readers. Plus two shell fixes the new coverage uncovered (((reparse, nested-subshell console ownership) and one general cleanup the console made worth finishing (delegating exit-code reads).1. Job output becomes a console
A background job kept its output in two byte fields readable only once the job ended. Nothing could watch a running job, a killed job's partial output was discarded outright, and a reader had no way to say where it had got to.
Output moves into a
JobConsole: an append-only log of timestamped chunks addressed by sequence number, over a swappableConsoleStore(append/read_from/wait/close; RAM today, shaped so a disk or Redis-stream store slots in the way history'sObserverStorevariants do). A reader's whole state is one integer, so readers cost nothing, join late, read from any position, and can disappear unnoticed. The job's ending is an in-bandCONTROLchunk, which makes "is it done" and "what did it print" the same question:Single writer, settle once.
Jobdrops its byte fields; status, exit code and the ending chunk are written in one place (_settle), status before finish, so a reader released by the ending chunk always sees final fields. The polling_refreshis gone.The table creates the job with its console first, then starts the runner, so there is no window where output has nowhere to go.
killsettles without joining. Cancellation is only observed where something checks it, so a job stuck inside one long command would never notice; joining would hang the shell on exactly the runaway job being killed.killrecords KILLED / exit 137, emitsKilled, finishes the console itself, and the console's guards drop any late emits from the still-unwinding runner. Partial output survives:Teardown settles jobs.
workspace.close()runskill_all()before any resource closes, so a job cannot touch a closing resource and a reader parked onwait_finished()is released instead of waiting forever (a bare abort left the job RUNNING with no ending chunk).JobConsole.close()likewise wakes blocked waiters and ends reader loops, so shutting a store down cannot strand or spin a reader.Snapshots capture history as events and restore jobs with their consoles.
2. The sink: compound bodies stream, capture sites never see it
The console alone is not streaming: the executor used to return a compound construct's output as one value, which would land in a single emit at the end.
execute_nodenow takes an optionalsink. Sequencing kinds (PROGRAM, LIST, SUBSHELL, IF, FOR, CFOR, SELECT, WHILE, UNTIL, CASE, NEGATED) pass it to their children so each statement lands as it finishes; every other node runs unchanged and has its result pumped at the boundary.b''b'1\n2\n3\n'b'1\n2\n'b'1\n2\n3\n'The other half is deliberate: capture sites never inherit the sink. The recursion used for capture is built without it, so a path only streams if its kind opted in:
Two fixes fell out of wiring this in TypeScript:
killgenuinely stops a TS job.executeNodealready honoreddeps.signal, buthandleBackgroundnever wired the job'sAbortControllerinto the walk. The signal now rides the forked session and the per-call opts, merged with any enclosing job's channel.subRecurseclosure declared four parameters; TS function parameters are bivariant, so it still satisfied the five-parameterExecuteNodeFn, and JavaScript silently dropped the fifth argument, which is exactly the bag carrying a nested job's own console and abort signal. In( (sleep 0.15; echo a) & echo b & wait ) &the nested jobs wrote to the outer job's console in completion order (b\na). Withoptsdeclared and threaded, the innerwaitadopts them in job-id order (a\nb). Python was never exposed (itssub_recurseis afunctools.partial, and a call-timesink=always overrides a bound default); both languages now carry the regression test.3. Bare
waitno longer discards job outputwaitwith no operand waited for every job then threw away what they printed, whilewait <id>returned it. A real shell has nothing to adopt because its jobs share the terminal; mirage jobs print to their console, so the shell has to surface it or the output is stranded.It now concatenates every unreaped job's console in job-id order, then reaps. Not just the running ones: a job that finished before the line was reached still has output nobody has read, and whether it finished in time is a scheduling accident. Id order because completion order is not reproducible. Reaping matches GNU, where a second
waitprints nothing andjobsis empty afterwards.fgechoes the command line, then adopts the job's output and exit code the same way.4.
((reparsed as nested subshells when it is not arithmetic((echo a); echo b)failed with a syntax error. tree-sitter-bash lexes((as the arithmetic opener and cannot back out; bash resolves the ambiguity by trying arithmetic first and reparsing as nested subshells when that fails.The subtlety is which openers to split. Being inside an ERROR is not evidence an opener is broken: tree-sitter's error region swallows neighbouring tokens, so a valid
((i++))beside a bad opener also reports as errored, and splitting it silently turns arithmetic into a subshell runningi++as a command. A wrong parse is worse than a rejected one.So each opener is judged on its own balanced span:
((i++))parses cleanly standalone and is left alone;((echo x); echo $i)does not and gets split. The scan skips parens inside quotes and escapes, splices right to left so earlier offsets stay valid, and keeps the retry only if it parses clean, returning the original error otherwise. Commands that parse today never reach the retry, so no working command's offsets move.All matching GNU:
5. Exit-code reads delegate to the stream source
A streaming command's status can depend on its content: grep returns
(exit_on_empty(stream, io_A), io_A)with a provisionalexit_code = 0, and the wrapper settles the real value onio_Aonly when the stream drains.merge()used to copy the number into the merged result, then patch the staleness with a back-pointer plus a manualsync_exit_code()that every reader had to remember to call; forgetting one call site meant a silently wrong$?. Worse, the sync assigned through the setter, which severed the link, so one sync before the drain froze the provisional 0 forever.Now the merged result does not copy the number at all: reads follow the link (
exit_codeis a property in Python, the getter delegates in TS), so the value is exactly as fresh as the origin whenever it is read, through any depth of merges. An explicit write (io.exit_code = 124) stores locally and severs the link, so an aggregated or overridden status still wins (issue #43).sync_exit_code/syncExitCodeand every call site are deleted in both languages; the drain-before-read ordering they papered over is already guaranteed by the statement barriers (finish_statement/apply_barrier).Merged with main, twice
The branch absorbed main through
#791(CLI doors, session write gate, ambient sessions). The interesting intersection: the background runner now rebinds the ambient session around its whole body (contextvar token in Python;runWithSessiongated onasyncContextIsolatesTasksin TS), with the console pump inside the rebind, since draining can still run ops that read the ambient session. The loop and select handlers thread main's policy view while keeping the streaming walker, and main's new handler unit suite forwait/kill/jobs/ps/fgwas adapted to the console-backed job table.Why the integ coverage grew
The battery had no case that backgrounded a compound construct: all eight existing job cases background a simple command, so the path this branch rewrote was untested. Adding those cases is what surfaced the bare-
waitbug. New cases live inbash/jobs/compound.jsonandbash/subshell/nested.json.Tests
build+ dts cleanpre-commit run --all-filesgreen with a clean tree