Repository navigation
fix(actions): bound, batch and evict action output to close the output leak - #1067
Merged
Merged
Conversation
The executor kept every read of an action's output for the life of the app: an unbounded Vec of per-read chunks while the action ran, cloned in full at exit, then retained until the frontend cleared it. This implements items 1, 2 and 3 (backend half) of the recommended fix in the action output leak note, and the two side effects its root-cause sections call out for this code. Move at exit (item 1) - The completion thread moves the execution's shared buffer into the finished set instead of cloning it, so the running and the finished log are the same allocation. - It then waits up to 2 s for the reader threads before it reports the terminal status. Output written just before exit is now in the finished log and is streamed before the status. The wait is bounded because a background process can keep the pipes open; what such a process writes later still reaches the same buffer. Cap by bytes (item 2) - New OutputBuffer keeps the most recent 4 MiB as a ring of blocks. Adjacent reads of one stream are coalesced into blocks of up to 64 KiB. - OutputChunk is unchanged. Once output has been dropped, the snapshot starts with a synthetic chunk on the stderr stream that says how much, and the retained text starts on a line boundary. - Streaming to the listener is not capped. Evict finished buffers (item 3, backend only) - Only the 8 most recently finished executions keep their output. - New ActionExecutor::discard_output drops an execution's output: at exit if it is still running, at once if it has finished. Branch teardown (delete branch, remove project repo, delete project) calls it through stop_and_discard_actions_for_branches. Moving a branch and transferring a project keep the output. - Clearing from the frontend when a pill is dropped is a later commit. Side effects - Reads are decoded by a streaming UTF-8 decoder, so a character split across two reads is no longer turned into replacement characters. - The tail lost after the exit snapshot is fixed by the reader wait. Tests - Unit tests for the decoder, the buffer and the finished set, and executor tests for all three items that run real /bin/sh scripts. - A Tauri test that teardown discards a deleted branch's output and a move keeps it. Follow-up, not caused by this change: children killed at birth - On a loaded macOS machine (load average 40 to 50, four copies of the test binary at once) a freshly spawned child is now and then killed with SIGKILL (wait status 9) about a millisecond after spawn, before it runs any of its script. The executor reports it as failed with no exit code and no output. - The executor at the parent commit does the same. Interleaved runs of the same spawn mix, 44 runs of each binary: 10 of 1012 child exits killed before this change, 4 of 968 with it. - It did not reproduce with --test-threads=1, with only some of the tests selected, or in standalone spawners outside the test binary. There was no memory pressure. The cause is unknown. - The new executor tests start a script again, at most 5 times, when its child dies that way. Their assertions are unchanged. Under the same stress 48 of 48 runs of them passed. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Every read of an action's output, about 1 KB, cost one listener call, one event to every client and a line-buffer append, whether or not anything consumed it. This implements item 4 of the recommended fix in the action output leak note, all three parts, and closes the hole in root cause section 3: a registry entry recreated by output that arrives after `unregister`. Batch the output (item 4, part 1) - New output_emitter module. The reader threads queue their reads for one emitter thread per execution. It merges them and delivers a batch once it is 50 ms old or 64 KiB large, one event per run of a stream. - A batch is appended to the buffer and then handed to the listener, as each read was. Output waiting in a batch is in neither, so the buffer and the events stay in step for a viewer that reads one and then follows the other. - Completion waits for the readers, then has the emitter deliver what it holds, then reports the terminal status. The tail still precedes the status. The flush has its own 2 s bound, so what was read is delivered before the status even when the readers used up theirs. - The queue holds 64 reads. A slow listener blocks the readers, as it did when each read was delivered directly, so no queue builds up. - ExecutionEvent and the `action_output` payload are unchanged. A chunk is larger, and a size-triggered batch can exceed 64 KiB by one read. - OutputBuffer::push splits text larger than a block into several blocks. Its bound assumed pushes no larger than a block, which batches are not. Skip the web serialization (item 4, part 2) - emit_to_all serializes for the WebSocket channel only while a client is subscribed. This applies to every event, not just action output. - It also passes the payload to Tauri by reference instead of cloning it. Gate and cap the registry buffer (item 4, part 3) - Only Run actions have a detection buffer. `register` creates it when the action starts, before any output can arrive. Creating it in `wire_run_detection`, which runs after `execute` returns, would lose the output written in between. `wire_run_detection` still asks for the buffer, which covers any other execution it is ever called for. - Output of build, test, check, format, prerun and clean-up actions is no longer normalized or stored there. - The buffer keeps the most recent 20,000 lines and 2 MiB of text. An unterminated line stops growing at 2 MiB. - Lines are numbered from the start of the output, so the regex matcher keeps its place once old lines are dropped. It now copies only the new lines on each poll instead of the whole buffer, and the autodetect poller copies only the last 200. - Limit: a line is missed by the regex matcher if more than the cap is written after it within one 2 s poll. Before, nothing was dropped and the buffer grew without bound. Entries recreated after unregister (root cause section 3) - `append_output_chunk` no longer creates a buffer, so late output from a process that outlived its action is dropped. - The run phase, the output buffer and the detector's cancel sender are now stored only for a registered execution, under the lock that `unregister` holds. Wiring detection for an action that has already ended stored all three for good before, and left its detector task polling. That is the same hole reached another way, so it is closed here too. A cancel sender that is not stored is dropped, which stops the task. Tests - Emitter unit tests: merging, stream order, the interval, the size limit, buffer-before-listener order, flush deadlines, shutdown. - Executor tests on real /bin/sh scripts: one event for many reads, size-triggered batches, delivery while the action runs, and a pending batch delivered before the status when the readers time out. - Registry, run detector and terminal output tests for the gating, the cap, line numbering and the unregister hole. - web_server tests that count serializations with and without a connected client. Not changed - The frontend. The modal still fetches the buffer before it registers its listener, which is item 5. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Requested: item 5 of the action-output leak note's recommended fix (make get_action_output_buffer asynchronous and tail-only, register the listener before fetching the snapshot, cap displayed lines, stop copying the line array each frame, release lines and the render cache on close), the frontend half of item 3 (clear a finished execution when its pill is dropped, unless the modal is showing it), and what root cause section 4 lists for the open view. Builds on cb75cc2 and bd03eba, which bounded, batched and evicted the output in the backend. NEEDS HUMAN REVIEW: backend shape changes The constraint was no missing or duplicated chunk where the snapshot meets the live events. That cannot be met from the existing shapes: the snapshot and the events travel on separate channels, so an event can land on either side of a snapshot that already holds its text, and matching on content is ambiguous for output that repeats itself (a progress bar). The smallest change that makes the join exact is one byte offset on each side: - builderbot_actions::ExecutionEvent::Output gains `offset: u64`, the bytes of output delivered before the chunk. - The `action_output` event payload gains `offset`. - `get_action_output_buffer` now returns `{ chunks, endOffset }`, the new builderbot_actions::BufferedOutput, instead of a bare chunk array. `OutputExecutor::get_buffered_output` returns the same type. - `get_action_output_buffer` is now an async command, so it no longer serialises up to 4 MiB on the main thread. Its arguments are unchanged. Both additions are additive for a reader of the events. The return shape is not: a web client still running a cached older shell would fail to load output until it reloads. "Tail-only" adds no parameter: the tail is the 4 MiB ring cb75cc2 introduced, which the command already returned. The offset is counted in OutputBuffer under the lock that appends a batch, and a batch is appended whole before it is emitted, so a snapshot always ends on the boundary between two events. Frontend - outputFollower (new) joins the two sources. It shows a snapshot, skips events that end at or before the snapshot's end, shows the event that starts exactly there, and takes a new snapshot on any gap or overlap instead of showing output with a hole in it. Events that arrive while a snapshot is in flight are held, up to 4 Mi characters. - ActionOutputModal registers its output listener first and reads the buffer from the listener's onEstablished, which also fires after a reconnect or an event gap. The old order lost what was written before the listener went live. - createIncrementalProcessor keeps a bounded tail: 5,000 lines, 4 Mi characters, 64 Ki characters per line. It updates one array in place and replaces a line object only when its text changes. The modal shows a count of the earlier lines it dropped. - The modal publishes that same array once per frame through a keyed each, so a frame costs the lines that changed, not the lines shown. - The render cache is gone rather than bounded. With the keyed list a line is rendered once per line object, so there was nothing left for a cache to save. - The modal releases its lines once the dialog has finished closing (onOpenChangeComplete), so the instance a branch card keeps mounted holds nothing between uses. It shows "Output is no longer available." for an execution whose output was cleared or evicted. - ActionRunner clears a finished execution in the backend when its pill is dropped. If the output modal is showing that execution, the clear waits until the modal closes or moves to another execution. Tests - Crate: offsets in the buffer and emitter, and events against the snapshot under eviction. - Tauri: the command's JSON shape and its agreement with the events. - Frontend: the follower (boundary cases and a randomised delivery and loss simulation), the processor (parity with processChunksToLines, limits, object identity), the runner's clearing, and the modal under the real dialog primitive. Checks run: cargo fmt --check and clippy for the crate and the Tauri app, cargo test -p builderbot-actions, the Tauri actions and web dispatch tests, prettier, svelte-check and tsc, and vitest for the actions feature, BranchCard and projectRunActions. Not run, because of machine load and disk: the full Tauri and frontend suites, and `just ci`. Not exercised in a real webview: the release after the close animation. Known limits: the Save log button saves the lines the modal holds, not the whole run. A reader scrolled up at the line cap sees the content shift as lines are dropped. Clearing on pill drop is per runner, so another window that opens the same finished execution afterwards sees "Output is no longer available." Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Requested: apply the review of 21f8e6f as one commit. Fix every warning and suggestion except the one on branch teardown, make sure a Stop between an action's exit and its terminal status cannot leave its id in `stopped` for good, and give each behavioural fix a test that fails without it. No backend model type, event payload or command signature is added or changed. Left out: output of a finished action on a deleted branch - Teardown still discards output only for executions the registry lists as running (stop_branch_actions, commands.rs). An action that finished with no card mounted keeps its output, up to 4 MiB, until eight later executions finish. - Closing that needs the executor to know which branch a finished execution belonged to. That is a new backend field, so it is left for a human decision. An execution is running until its terminal status is due (warning) - The completion thread took the execution out of `running` when the process exited, then waited up to 4 s for the readers and the flush. In that window the liveness queries left it out, so a card that hydrated then never showed it. - The entry now stays in `running` until the flush is done, and moves to the finished set just before the status is reported. `is_running`, `get_running_ids` and `wait_for_executions` count it as running until then. In the usual case the window is still microseconds. - App quit uses `wait_for_executions`, so it can wait for that delivery too. It is bounded by the quit's own budget, as before. Stop in that window, and Stop of anything without a process - The completion thread clears the execution's pid and consumes its `stopped` mark at exit, under the `running` lock. Stop now adds the mark under that same lock, and only when there is a pid to signal. - A Stop in the window therefore does nothing: no signal to a pid that may be reused, no mark, and the action reports the status it earned. - The same rule ends an older leak: stopping a finished or unknown execution used to leave its id in `stopped` for good. Partial-line flag across streams (output_buffer.rs) - The buffer skipped the first line of the oldest retained block whenever the evicted block did not end on a line break. With separate stdout and stderr pipes that dropped a whole stderr line after a cut in stdout. The flag is now set only when both blocks are one stream. Newest detection line always stays (terminal_output.rs) - A line that a multi-byte character took a few bytes past the byte bound evicted itself when it ended, so the matcher never saw it. The newest finalized line now stays whatever its size, as the newest block does in OutputBuffer. Retry after a failed first snapshot (outputFollower.ts) - After the first snapshot failed nothing asked again until a reconnect, so the modal stayed on its load error while the action kept writing. The next output event now takes the snapshot again. Test-only changes - commands.rs: the buffer and events test retried whenever the buffer was empty, which is also what lost output looks like. It now records the status and exit code, retries only on failed with no exit code and no output, and asserts a clean exit. - executor/tests.rs: the pending-batch test has to meet its drain timeout, not outlast it. The timeout is 10 s instead of 2 s, so a starved machine still reads and delivers in time. Tests that fail without the fix - Executor: running until the status is due, and Stop after the process has exited. With the old exit sequence both fail. With only Stop's marking reverted the Stop test fails on the leftover mark. - OutputBuffer: a line of another stream at the cut is kept whole. - Terminal output: a line a wide character took past the bound is kept. - Follower: two tests for the retry on the next event. - Each was run against the reverted fix and failed. A further executor test, that a real Stop still reports Stopped and clears its mark, is a guard and passes either way. Checks run, one at a time, on this tree - Disk before starting: 54 GiB free. Load average 13 to 68 throughout, from other sessions. - Crates workspace: cargo fmt --all --check, cargo clippy --workspace -- -D warnings, cargo test --workspace. All pass: 694 tests, 9 ignored, plus 1 doc test. - Tauri app: just app staged ci-rust-lint (fmt check, clippy) and ci-rust-test (cargo test --workspace --lib --bins --tests). All pass: 2112 app tests with 2 ignored, including all 136 project_transfer tests, 106 stagedctl tests, the help-pin test, and both bin targets, which hold no tests. - Frontend: prettier --check, svelte-check and tsc (0 errors, 0 warnings), and the full vitest suite. - Limits used to hold the load down: RUST_TEST_THREADS=4 for the Rust tests and --maxWorkers=3 for vitest. One failure, not repeated - The first full vitest run had 1 failure in 3590 tests: a 5 s timeout in MarkdownWysiwygEditor.component.test.ts, "loads embedded images from the media server", at a load average of 68. - It passed alone 3 times and in 2 further full runs on this tree. - On the merge base 4cc7a04 it passes alone and in the full suite, 3521 of 3521. This branch changes nothing under features/notes. Not run - `just ci` as one command. Its crates and Staged parts are the checks above. The Differ and Penpal apps, which this branch does not touch, were not checked. - Nothing was exercised in a running app. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
The action executor kept every read of an action's output for the life of the app, cloned it in full at exit, emitted one event per ~1 KB read to every client, and the output modal fetched its snapshot after registering nothing, so it both leaked memory and could lose or duplicate output. This PR implements the recommended fix from the action-output leak note across four commits.
Backend (
crates/builderbot-actions, Tauriactions/)OutputBufferring keeps the most recent 4 MiB per execution; the buffer is moved, not cloned, at exit; only the 8 most recently finished executions keep output; branch teardown discards output for deleted branches; reads are decoded by a streaming UTF-8 decoder.output_emittermerges reads into one batch per 50 ms / 64 KiB; WebSocket serialization only happens while a client is subscribed; the run-detection buffer exists only for Run actions and is capped (20,000 lines / 2 MiB) with stable line numbering; late output can no longer recreate a registry entry afterunregister.ExecutionEvent::Outputand theaction_outputpayload gain a byteoffset;get_action_output_bufferis now async and returns{ chunks, endOffset }(BufferedOutput).runninguntil its terminal status is due, so liveness queries never miss it; Stop in the exit window is a no-op and no longer leaks ids intostopped; partial-line eviction flag is per-stream; the newest detection line always survives the byte bound.Frontend (
features/actions)outputFollowerjoins the snapshot and live events by offset, holds events while a snapshot is in flight, and re-snapshots on any gap or overlap (including after a failed first snapshot).ActionOutputModalregisters its listener before fetching, keeps a bounded tail (5,000 lines / 4 Mi chars) updated in place with a keyed each, drops the render cache, and releases its lines once the dialog has closed.ActionRunnerclears a finished execution's backend output when its pill is dropped, deferred while the modal is showing it.Needs human review: backend shape changes
Per AGENTS.md, these additions were flagged in the commits and need a human call:
ExecutionEvent::Output.offset: u64andoffseton theaction_outputevent payload (additive).get_action_output_bufferreturn shape changed from a chunk array toBufferedOutput { chunks, endOffset }, and the command is now async. A web client on a cached older shell would fail to load output until reload.Deliberately left open: output of an action that finished with no card mounted on a since-deleted branch is kept until eight later executions finish. Closing that needs the executor to know a finished execution's branch, which is a new backend field.
Known limits
Test plan
cargo fmt --check,cargo clippy --workspace -D warnings,cargo test --workspaceon the crates workspace (694 tests + 1 doc test)just app staged ci-rust-lintandci-rust-test(2112 app tests)OutputBuffer, finished set, emitter, executor against real/bin/shscripts, registry/run-detector gating, web_server serialization counts, follower (incl. randomised delivery/loss), processor, runner clearing, modal under the real dialog primitive🤖 Generated with Claude Code