Cap SandboxLogs per-line buffer to bound control-process memory

[jcasimir]

Problem

A sandbox that emits a large, newline-poor blob to stdout grows
(*SandboxLogs).buf without bound. A log line is only flushed on \n, so
containerd streaming a multi-GB payload across many Write calls accumulates
the whole thing in one bytes.Buffer, which is then gjson-parsed and
JSON-marshaled into a single log entry.

On our self-hosted cluster this manifested as a recurring control-process memory
balloon (~every 4h) that tripped the host OOM and took down every app in the
shared miren.service cgroup. A live heap profile of the control process during
a balloon attributed 97.8% of a 68 GB heap to this path:

containerd cio.copyIO  →  (*SandboxLogs).Write
  bytes.growSlice            20.6 GB   (one unbounded bytes.Buffer)
  encoding/json.Marshal      18.4 GB   (marshaling the giant line)
HeapInuse 53.7 GB, 54M objects

The trigger was an app's 4-hourly job occasionally logging a huge raw payload.

Fix

Cap the partial-line buffer at 1 MiB (maxLineBytes): bytes past the cap are
dropped until the next newline flushes the truncated line, bounding memory to
roughly maxLineBytes per concurrent sandbox regardless of what an app emits.

Also fixes a latent bug where buf was never Reset after completing a
buffered line, so a partial line could be re-appended to itself.

Existing TestSandboxLogs cases (line handling, partial-line buffering, USER/
ERROR prefixes) are unchanged; a new case asserts a ~12 MiB newline-poor stream
is capped to maxLineBytes and flushes truncated on the next newline.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d6b5d820-c98f-429e-9807-f59bb36b904e

📥 Commits

Reviewing files that changed from the base of the PR and between 64b9043 and efb45c7.

📒 Files selected for processing (2)

• controllers/sandbox/log.go
• controllers/sandbox/log_test.go

---

📝 Walkthrough

Walkthrough

SandboxLogs.Write is rewritten to bound the size of its internal partial-line buffer (s.buf) using a new maxLineBytes constant (1 MiB). A new bufferCapped helper appends bytes to s.buf and silently discards any data that would exceed the cap. Completed lines (including those preceded by capped fragments) are forwarded to processLine. A new subtest stresses the path with many large newline-free chunks and verifies that the buffer stays within the cap and that a final newline produces exactly one truncated log entry.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[miren-code-agent]

Review: Cap SandboxLogs per-line buffer to bound control-process memory

This is a focused, well-motivated fix for a real memory safety hazard. I read through the diff carefully, traced the control flow in both the old and new implementations, and checked the tests. Here's my assessment.

What the bug was

The original Write had a subtle but serious problem. When s.buf held a partial line and a new chunk arrived, the code did:

if s.buf.Len() > 0 {
    s.buf.Write(p)
    p = s.buf.Bytes()
}

…but it never called s.buf.Reset(). After the inner loop finished (having processed all newlines from the combined slice), whatever was in s.buf was never cleared. On the next Write call with s.buf.Len() > 0, stale processed data was prepended again — growing the buffer without any upper bound, and growing it in a way that was visually easy to miss. A sandbox emitting a long line in many small chunks (e.g. a scraper dumping a raw HTTP response body with no newlines) could, over time, OOM the control process.

What the fix does

The new code restructures Write cleanly and correctly:

1. No-newline path → calls bufferCapped(p), which will only accept up to maxLineBytes - s.buf.Len() bytes, silently dropping the rest. The buffer can never exceed 1 MiB. ✅
2. Newline with buffered prefix → calls bufferCapped(p[:nl]) to complete the line (still bounded), calls s.processLine(s.buf.String()), then critically s.buf.Reset(). Stale state is gone. ✅
3. Newline, no buffered prefix → slices p[:nl] directly to at most maxLineBytes and processes it without touching the buffer at all. ✅

The bufferCapped helper is a clean abstraction for the cap-and-write logic, and it's easy to reason about in isolation.

On the choice of 1 MiB

This is a practical call: big enough to accommodate any reasonable log line (structured JSON with long stack traces, etc.), small enough that even hundreds of concurrent sandboxes can't blow out the heap. The comment explains the tradeoff well.

Tests

The new "caps an unterminated firehose line" test is exactly right — it simulates the real failure mode (200 × 64 KiB chunks, no newline), asserts the buffer never exceeded the cap, then fires a final \n and asserts the flushed line is exactly maxLineBytes long. The existing "buffers partial lines" test continues to validate that cross-Write line assembly works correctly, which provides good confidence that the refactor didn't break the normal path.

Minor observation (non-blocking)

One small thing worth being aware of: when a line is assembled across multiple Write calls and the buffer hits the cap mid-assembly, the content after the cap is silently dropped. That's the right call for a memory safety fix, but there's no log-level signal to the operator that truncation occurred. If a truncated-but-flushed log line would ever be difficult to diagnose in production, a s.log.Warn("log line truncated", "cap", maxLineBytes) on the flush path might help in the future. Not a blocker at all — the current behavior is correct and the comment documents it well.

Summary

The fix is correct, the new logic is easier to follow than the original, the tests are meaningful and cover the bug scenario directly, and the comments explain the "why" clearly. I'm happy to see this merged.

[miren-code-agent]

When the buffer was already at the cap before this bufferCapped call (i.e. room <= 0), s.buf.String() will return only the first maxLineBytes of the assembled line — the rest of p[:nl] is silently dropped. That's the correct memory-safe behaviour, but it means the flushed line is truncated without any indication in the log entry itself. Something like a "[truncated]" sentinel appended to the body, or even a s.log.Warn("sandbox log line truncated", "cap", maxLineBytes, "entity", s.entity), would help operators diagnose confusing output in production. Non-blocking, just worth considering.

[evanphx]

@jcasimir great find and sorry about the outage!