Fix unbounded stderr_buffer leak; make Daemon stderr reliable#8
Merged
Conversation
In the default :consume stderr mode, every stderr chunk was prepended to state.stderr_buffer — a list that was never read anywhere in lib/. For long-running processes (especially NetRunner.Daemon) this grew the GenServer heap without bound, retaining data nothing could use. Replace it with a bounded tail: - Retain only the most-recent :stderr_tail_bytes bytes (default 8192) as a single binary; drain-and-drop the rest. Stats still count every byte. - Expose it via NetRunner.Process.stderr_tail/1 (the long-orphaned AbnormalExit.stderr feature; kept defined-but-unraised for a future opt-in). - Add and validate the :stderr_tail_bytes option (non-negative integer; 0 disables retention while still draining). Also fix a stderr-pipe race in NetRunner.Daemon: it ran its own stderr drain task while leaving the default :consume internal consumer on, so the two raced the same FD and on_output saw an arbitrary subset. Daemon now forces stderr: :disabled so its drain task is the sole reader and on_output receives the full stream in order. No C/shepherd/protocol changes. PTY mode unaffected.
The macOS test job ran OTP 27.2 / Elixir 1.17.3, which setup-beam does not reliably provide for the now-arm64 macos-latest runner — bump it to OTP 28.3.3 / Elixir 1.18.4 to match a supported build. Also bump actions/checkout from the aging v4 to v7 across all jobs.
dec3266 to
e2a6f4a
Compare
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
Fixes two findings from a performance review of the OTP I/O paths.
1. Unbounded
stderr_buffermemory leakIn the default
:consumestderr mode,NetRunner.Processdrained the child'sstderr pipe (correct — keeps the child from blocking on a full pipe) but
prepended every chunk to
state.stderr_buffer, a list that was never readanywhere in
lib/(verified write-only). For long-running processes —especially
NetRunner.Daemon— this grew the GenServer heap without bound,retaining data nothing could use.
NetRunner.Stream.AbnormalExitalready defined a:stderrfield and amessage/1that formats it, but was never raised with stderr populated — theorphaned consumer half of a half-built diagnostic feature.
Fix: retain only a bounded tail of stderr.
:stderr_tail_bytesbytes (default8192) as a singlebinary; drain-and-drop the rest. Stats still count every byte — only
retention is capped.
NetRunner.Process.stderr_tail/1surfaces it (useful fordiagnosing why a command failed).
:stderr_tail_bytesoption, validated as a non-negative integer (0disables retention while still draining).
2. Daemon stderr-pipe race
NetRunner.Daemonran its own stderr drain task and left the default:consumeinternal consumer on, so the two raced the same FD andon_outputreceived an arbitrary subset of stderr.
Daemon.init/1now forcesstderr: :disabledso its drain task is the sole reader andon_outputgetsthe full stream, in order.
Deliberately out of scope
stream!/2/run/2halt semantics are unchanged (no new raise onnonzero exit).
AbnormalExitis kept defined-but-unraised as a future opt-in,to avoid a breaking change.
deferred — low impact at current usage (0–2 parked reads).
Notes
diagnostic text, not guaranteed-valid UTF-8 (documented).
Test plan
mix format --check-formatted,mix compile --warnings-as-errors,mix credo --strict,mix dialyzer— all clean.mix test— 150 passed (addedtest/stderr_tail_test.exs: tail bound,custom cap, cap=0, leak guard (O(1) retention vs O(n) throughput), option
validation,
:disabledread path, and Daemonon_outputfull-stream).