diff --git a/Directory.Packages.props b/Directory.Packages.props index fa4c039cb..e2c815ea6 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -84,6 +84,10 @@ + + + + diff --git a/Netclaw.slnx b/Netclaw.slnx index 577604ed2..9263541f5 100644 --- a/Netclaw.slnx +++ b/Netclaw.slnx @@ -40,4 +40,7 @@ + + + diff --git a/benchmarks/Netclaw.Benchmarks/Netclaw.Benchmarks.csproj b/benchmarks/Netclaw.Benchmarks/Netclaw.Benchmarks.csproj new file mode 100644 index 000000000..e8d6e933f --- /dev/null +++ b/benchmarks/Netclaw.Benchmarks/Netclaw.Benchmarks.csproj @@ -0,0 +1,26 @@ + + + + net10.0 + Exe + enable + enable + Netclaw.Benchmarks + + false + + + + + + + + + + + + diff --git a/benchmarks/Netclaw.Benchmarks/Program.cs b/benchmarks/Netclaw.Benchmarks/Program.cs new file mode 100644 index 000000000..cc3e19c49 --- /dev/null +++ b/benchmarks/Netclaw.Benchmarks/Program.cs @@ -0,0 +1,14 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- + +using BenchmarkDotNet.Running; + +// Usage: +// dotnet run -c Release --project benchmarks/Netclaw.Benchmarks # pick interactively +// dotnet run -c Release --project benchmarks/Netclaw.Benchmarks -- --filter '*' # run everything +BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args); + +internal sealed partial class Program; diff --git a/benchmarks/Netclaw.Benchmarks/ShellDrainBenchmarks.cs b/benchmarks/Netclaw.Benchmarks/ShellDrainBenchmarks.cs new file mode 100644 index 000000000..4df1a3143 --- /dev/null +++ b/benchmarks/Netclaw.Benchmarks/ShellDrainBenchmarks.cs @@ -0,0 +1,59 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- + +using BenchmarkDotNet.Attributes; +using Netclaw.Actors.Tools; + +namespace Netclaw.Benchmarks; + +/// +/// Isolates the shell-output drain algorithm from the rest of ShellTool +/// (no process spawn, no real pipe I/O) so the numbers reflect only the cost of +/// turning a child's stdout stream into a bounded, redaction-ready string. +/// +/// The comparison is the regression fix for #1293: +/// (head+tail ring buffer, capped at +/// read time) versus the old path — +/// followed by , which materialised the +/// entire output before applying the cap. The story we want the numbers to tell +/// is allocation shape: the old path is O(total output) and lands on the LOH for +/// anything large; the new path is O(cap) regardless of how chatty the child is. +/// +[MemoryDiagnoser] +public class ShellDrainBenchmarks +{ + // Production default for ToolConfig.MaxOutputChars. Kept as a literal here so + // the benchmark exercises the real cap without taking a Configuration dependency + // at measurement time. + private const int Cap = 32_000; + + /// + /// Total characters the synthetic child "writes". Chosen to straddle the cap: + /// below it (no truncation), exactly at it, modestly over (LOH territory), and + /// far over (the pathological autonomous-log-pull case that triggered #1293). + /// + [Params(1_000, 32_000, 1_000_000, 50_000_000)] + public int TotalChars; + + [Benchmark(Baseline = true, Description = "ReadToEnd + TruncateOutput (pre-#1293)")] + public async Task ReadToEnd_ThenTruncate() + { + // Reader is constructed per-invocation because it is stateful (consumed as + // it is read). Its own allocation is a few dozen bytes and identical across + // both benchmarks, so it does not distort the head-to-head allocation story. + var reader = new SyntheticCharReader(TotalChars); + var all = await reader.ReadToEndAsync(); + return ShellTool.TruncateOutput(all, Cap).Length; + } + + [Benchmark(Description = "BoundedDrainAsync (post-#1293)")] + public async Task BoundedDrain() + { + var reader = new SyntheticCharReader(TotalChars); + var (text, _) = await ShellTool.BoundedDrainAsync(reader, Cap); + return text.Length; + } +} diff --git a/benchmarks/Netclaw.Benchmarks/SyntheticCharReader.cs b/benchmarks/Netclaw.Benchmarks/SyntheticCharReader.cs new file mode 100644 index 000000000..9990a78da --- /dev/null +++ b/benchmarks/Netclaw.Benchmarks/SyntheticCharReader.cs @@ -0,0 +1,56 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- + +namespace Netclaw.Benchmarks; + +/// +/// A that synthesises a fixed number of characters +/// without ever holding the full payload in memory. It stands in for a chatty +/// child process's stdout so the drain benchmark measures the algorithm's +/// allocations and nothing else — the input itself allocates O(1), so anything +/// the [MemoryDiagnoser] reports belongs to the code under test. +/// +/// All reads complete synchronously: the goal is to feed the drain loop as fast +/// as possible, not to model pipe latency. +/// +internal sealed class SyntheticCharReader(long total) : TextReader +{ + private const char Fill = 'x'; + private long _produced; + + public override int Read(char[] buffer, int index, int count) + { + var remaining = total - _produced; + if (remaining <= 0) + return 0; + + var n = (int)Math.Min(count, remaining); + Array.Fill(buffer, Fill, index, n); + _produced += n; + return n; + } + + public override int Read(Span buffer) + { + var remaining = total - _produced; + if (remaining <= 0) + return 0; + + var n = (int)Math.Min(buffer.Length, remaining); + buffer[..n].Fill(Fill); + _produced += n; + return n; + } + + // BoundedDrainAsync reads via the ValueTask Memory overload (below). + // The char[] Task overload is kept too so any reader path resolves to the + // synchronous fast path above and the benchmark measures the algorithm only. + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + => ValueTask.FromResult(Read(buffer.Span)); + + public override Task ReadAsync(char[] buffer, int index, int count) + => Task.FromResult(Read(buffer, index, count)); +} diff --git a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs index fbbf1569a..6be5ddee5 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs @@ -103,15 +103,16 @@ public async Task Caller_cancellation_kills_child_process_tree_and_returns_grace public async Task Output_truncation_applies() { var tool = new ShellTool(new ToolConfig { MaxOutputChars = 50 }); - // Generate output longer than 50 chars — use cross-platform command - var command = OperatingSystem.IsWindows() - ? "python -c \"print('x' * 200)\"" - : "printf 'x%.0s' {1..200}"; - var args = ToolInput.Create("Command", command); + // Write >50 chars to stdout with a command that needs no interpreter. + // `echo` is a builtin on both bash and cmd.exe; a long literal is + // deterministic. (python on the Windows runner resolves to the Store + // stub, which writes to stderr — so the truncation would land on stderr + // and the marker would read "[stderr truncated", not "[stdout truncated".) + var args = ToolInput.Create("Command", $"echo {new string('x', 200)}"); var result = await tool.ExecuteAsync(args, CancellationToken.None); - Assert.Contains("[output truncated]", result); + Assert.Contains("[stdout truncated", result); } [Fact] @@ -366,6 +367,111 @@ public void TruncateOutput_truncates_and_appends_indicator() Assert.EndsWith("[output truncated]", result); } + // ── BoundedDrainAsync ── + + [Fact] + public async Task BoundedDrain_short_output_returned_verbatim() + { + var input = "hello world"; + var reader = new StringReader(input); + var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100); + Assert.Equal(input, text); + Assert.False(truncated); + } + + [Fact] + public async Task BoundedDrain_empty_input_returns_empty() + { + var reader = new StringReader(""); + var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100); + Assert.Equal("", text); + Assert.False(truncated); + } + + [Fact] + public async Task BoundedDrain_output_exactly_at_cap_not_truncated() + { + var input = new string('a', 100); + var reader = new StringReader(input); + var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100); + Assert.Equal(input, text); + Assert.False(truncated); + } + + [Fact] + public async Task BoundedDrain_long_output_truncated_with_head_and_tail() + { + // 100-char head marker + separator + 100-char tail marker, with filler in the middle + var head = new string('H', 100); + var middle = new string('M', 5000); + var tail = new string('T', 100); + var input = head + middle + tail; + + var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 200); + + Assert.True(truncated); + Assert.StartsWith(new string('H', 100), text); // head preserved + Assert.EndsWith(new string('T', 100), text); // tail preserved + Assert.Contains("...", text); // separator present + Assert.DoesNotContain("M", text); // middle discarded + } + + [Fact] + public async Task BoundedDrain_head_and_tail_split_evenly() + { + // maxChars=10 → headCap=5, tailCap=5 + var input = "AAAAAXXXXXXBBBBB"; // 16 chars: 5 head, 6 overflow discard, 5 tail + var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 10); + + Assert.True(truncated); + Assert.StartsWith("AAAAA", text); + Assert.EndsWith("BBBBB", text); + } + + [Fact] + public async Task BoundedDrain_disabled_cap_returns_full_output() + { + var input = new string('x', 10_000); + var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 0); + Assert.Equal(input, text); + Assert.False(truncated); + } + + [Fact] + public async Task BoundedDrain_tail_ring_wraps_across_small_chunks() + { + // Drives the ring's wraparound + start-advance path that the StringReader + // tests skip: each read delivers a chunk smaller than tailCap, so the tail + // window is rebuilt incrementally and must wrap rather than reset wholesale. + // maxChars=10 → headCap=5 ("ABCDE"), tailCap=5; last 5 of "FGHIJKLMNO" = "KLMNO". + var reader = new ChunkedReader("ABCDEFGHIJKLMNO", chunkSize: 3); + + var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 10); + + Assert.True(truncated); + Assert.Equal("ABCDE\n...\nKLMNO", text); + } + + // Hands out at most chunkSize chars per read so tests can exercise the tail + // ring's incremental wrap path — real pipe reads arrive in arbitrary slices, + // not the single 4KB gulp a StringReader gives. + private sealed class ChunkedReader(string data, int chunkSize) : TextReader + { + private int _pos; + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + var remaining = data.Length - _pos; + if (remaining <= 0) + return ValueTask.FromResult(0); + + var n = Math.Min(Math.Min(chunkSize, buffer.Length), remaining); + data.AsSpan(_pos, n).CopyTo(buffer.Span); + _pos += n; + return ValueTask.FromResult(n); + } + } + [Fact] public async Task Command_referencing_denied_path_returns_access_denied() { diff --git a/src/Netclaw.Actors/Netclaw.Actors.csproj b/src/Netclaw.Actors/Netclaw.Actors.csproj index 2ba48752a..e50b6c8df 100644 --- a/src/Netclaw.Actors/Netclaw.Actors.csproj +++ b/src/Netclaw.Actors/Netclaw.Actors.csproj @@ -9,6 +9,7 @@ + diff --git a/src/Netclaw.Actors/Tools/ShellTool.cs b/src/Netclaw.Actors/Tools/ShellTool.cs index 49799aeff..355d49023 100644 --- a/src/Netclaw.Actors/Tools/ShellTool.cs +++ b/src/Netclaw.Actors/Tools/ShellTool.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Buffers; using System.ComponentModel; using System.Diagnostics; using System.Text; @@ -152,16 +153,17 @@ or UnauthorizedAccessException { process.StandardInput.Close(); - var outputBuilder = new StringBuilder(); - var errorBuilder = new StringBuilder(); - // Start draining both pipes up front: a chatty child can deadlock if // one pipe buffer fills while we wait on the other. The reads take // CancellationToken.None deliberately — a redirected child holds the // pipe write-ends open, so a blocked pipe read cannot be interrupted // by a token; killing the process is what closes the pipes. - var stdoutTask = process.StandardOutput.ReadToEndAsync(CancellationToken.None); - var stderrTask = process.StandardError.ReadToEndAsync(CancellationToken.None); + // + // BoundedDrainAsync reads into a head+tail window bounded by + // MaxOutputChars but continues draining after the cap is reached so + // the pipe never fills up and deadlocks a still-running child. + var stdoutTask = BoundedDrainAsync(process.StandardOutput, _config.MaxOutputChars); + var stderrTask = BoundedDrainAsync(process.StandardError, _config.MaxOutputChars); try { @@ -188,7 +190,7 @@ or UnauthorizedAccessException { // If the OS refuses the kill, the child may keep stdout/stderr // open forever. Close our read ends so cancellation still - // returns promptly instead of hanging in ReadToEndAsync. + // returns promptly instead of hanging in BoundedDrainAsync. killClosedPipes = false; Debug.WriteLine($"shell_execute: process kill skipped — {ex.Message}"); process.StandardOutput.Dispose(); @@ -209,25 +211,178 @@ or UnauthorizedAccessException : "Error: Command cancelled."; } - outputBuilder.Append(await stdoutTask); - errorBuilder.Append(await stderrTask); + var (stdoutText, stdoutTruncated) = await stdoutTask; + var (stderrText, stderrTruncated) = await stderrTask; + + // Redact secrets on the already-bounded strings, then assemble. + var stdout = SecretOutputRedactor.Redact(stdoutText); + var stderr = SecretOutputRedactor.Redact(stderrText); var result = new StringBuilder(); - if (outputBuilder.Length > 0) - result.Append(outputBuilder); - if (errorBuilder.Length > 0) + if (stdout.Length > 0) + { + result.Append(stdout); + if (stdoutTruncated) + result.Append($"\n[stdout truncated — output exceeded {_config.MaxOutputChars} chars; head and tail shown]"); + } + if (stderr.Length > 0) { if (result.Length > 0) result.AppendLine(); - result.Append(errorBuilder); + result.Append(stderr); + if (stderrTruncated) + result.Append($"\n[stderr truncated — output exceeded {_config.MaxOutputChars} chars; head and tail shown]"); + } + + return $"Exit code: {process.ExitCode}{Environment.NewLine}{result}"; + } + } + + /// + /// Drains into a head+tail window bounded by + /// . Chars beyond the cap are discarded but the + /// pipe continues to be read so a still-running child never deadlocks on a + /// full pipe buffer. Returns the captured text and whether it was truncated. + /// + /// + /// Allocation is O(), not O(total output): the + /// scratch read buffer is pooled, the tail ring is allocated only if the head + /// fills, and reads go through the overload so a + /// pipe that already has data buffered completes synchronously without a + /// per-chunk allocation. A child that prints hundreds of MB + /// is drained with a handful of KB of managed allocations — the property that + /// keeps a memory-limited daemon from being OOM-killed (see #1293). + /// + internal static async Task<(string Text, bool Truncated)> BoundedDrainAsync( + TextReader reader, int maxChars) + { + if (maxChars <= 0) + { + // Explicit opt-out: a non-positive cap disables truncation and reads + // the whole stream. The config schema enforces MaxOutputChars >= 1, so + // production never reaches this — it exists only for programmatic + // callers that deliberately pass 0/negative to capture full output. + var all = await reader.ReadToEndAsync(CancellationToken.None); + return (all, false); + } + + // Split the budget: first half for the head, second half for the tail. + // Odd maxChars gives the extra char to the head. Computed without (maxChars + // + 1) so a near-int.MaxValue cap can't overflow to a negative headCap. + var headCap = maxChars / 2 + maxChars % 2; + var tailCap = maxChars / 2; + + var head = new StringBuilder(Math.Min(headCap, 4096)); + + // Tail ring buffer, allocated lazily on first overflow past the head. The + // common case — output under the cap — never fills the head, so it never + // pays for the tail window at all. + char[]? tailBuf = null; + var tailStart = 0; // index of the oldest retained char in the ring + var tailLen = 0; // chars currently retained (<= tailCap) + + long totalChars = 0; // total chars seen across all reads; long so a multi-GB + // flood can't overflow the truncation check to a false negative + + // Transient scratch buffer for the read loop: pooled so a long drain + // doesn't allocate it per call and it never lands on the LOH. + var buf = ArrayPool.Shared.Rent(4096); + try + { + int read; + // ReadAsync(Memory) returns a non-allocating ValueTask when the + // read completes synchronously (data already buffered) — unlike the + // Task char[] overload, which allocates once per chunk. + while ((read = await reader.ReadAsync(buf.AsMemory(), CancellationToken.None)) > 0) + { + totalChars += read; + var span = buf.AsSpan(0, read); + + if (head.Length < headCap) + { + var headChunk = Math.Min(headCap - head.Length, span.Length); + head.Append(span[..headChunk]); + span = span[headChunk..]; + } + + if (span.IsEmpty || tailCap == 0) + continue; + + tailBuf ??= new char[tailCap]; + AppendToTailRing(tailBuf, span, ref tailStart, ref tailLen); } + } + finally + { + // clearArray: the scratch buffer held raw stdout/stderr (possibly + // secrets); wipe it before returning to the shared pool. + ArrayPool.Shared.Return(buf, clearArray: true); + } + + // Truncation only when total chars exceeded the full budget (head+tail), + // meaning some middle chars were discarded. + var truncated = totalChars > maxChars; + + // Reconstruct in place on `head`: when truncated, the discarded middle is + // marked with a separator; otherwise head + tail is the full output. Reusing + // `head` avoids a second StringBuilder and re-copying the head. + if (truncated) + head.Append("\n...\n"); + if (tailBuf is not null && tailLen > 0) + AppendRing(head, tailBuf, tailStart, tailLen); + return (head.ToString(), truncated); + } + + /// + /// Writes into a ring buffer that retains only the + /// most recent ring.Length chars. Uses block copies (at most two per + /// call) rather than a per-char loop, so draining a very chatty child stays + /// cheap regardless of how much it prints. + /// + private static void AppendToTailRing(char[] ring, ReadOnlySpan span, ref int start, ref int len) + { + var cap = ring.Length; - var sanitized = SecretOutputRedactor.Redact(result.ToString()); - var output = TruncateOutput(sanitized, _config.MaxOutputChars); - return $"Exit code: {process.ExitCode}{Environment.NewLine}{output}"; + if (span.Length >= cap) + { + // This span alone fills (or overfills) the window: only its last `cap` + // chars can survive. One contiguous copy, ring reset. + span[^cap..].CopyTo(ring); + start = 0; + len = cap; + return; + } + + var writePos = (start + len) % cap; + var first = Math.Min(span.Length, cap - writePos); + span[..first].CopyTo(ring.AsSpan(writePos)); + if (first < span.Length) + span[first..].CopyTo(ring); // remainder wraps to the front + + var newLen = len + span.Length; + if (newLen > cap) + { + // Overwrote the oldest chars: advance start past them. + start = (start + (newLen - cap)) % cap; + len = cap; } + else + { + len = newLen; + } + } + + /// Appends a ring buffer's retained chars, oldest-first, to . + private static void AppendRing(StringBuilder sb, char[] ring, int start, int len) + { + var first = Math.Min(len, ring.Length - start); + sb.Append(ring, start, first); + if (first < len) + sb.Append(ring, 0, len - first); } + // Retained for compatibility with tests that call it directly; the main + // execution path no longer uses this — BoundedDrainAsync caps at read time. internal static string TruncateOutput(string output, int maxChars) { if (output.Length <= maxChars)