Fix runaway os-open stderr logging loop - #126
Conversation
📝 WalkthroughWalkthroughAdds a reusable ChangesStderr line reading
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR fixes a runaway stderr-logging loop in
Confidence Score: 4/5Safe to merge — the core loop fix is correct and the child process is properly reaped via exe.wait(). The delimiter switch from takeDelimiterExclusive to takeDelimiter is the right fix for Zig 0.15.2 and the regression test directly covers the previously broken path. The only open question is whether a final stderr line lacking a trailing newline is silently dropped at EOF, which the new test does not exercise. src/os/open_reader.zig — the takeLine EOF-without-delimiter path deserves an additional test case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([openThread starts]) --> B{exe.stderr piped?}
B -- No --> W[exe.wait]
B -- Yes --> C[readerStreaming with 256-byte buffer]
C --> D[loop: call takeLine]
D --> E{takeDelimiter result}
E -- "null (EOF)" --> W
E -- "error.ReadFailed" --> W
E -- "[]u8 line" --> F[log.warn open stderr=line]
F --> D
E -- "error.StreamTooLong" --> G[take chunk_size bytes]
G -- "error.ReadFailed" --> W
G -- "error.EndOfStream" --> W
G -- "[]u8 chunk" --> F
W([exe.wait reaps child]) --> Z([thread exits])
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A([openThread starts]) --> B{exe.stderr piped?}
B -- No --> W[exe.wait]
B -- Yes --> C[readerStreaming with 256-byte buffer]
C --> D[loop: call takeLine]
D --> E{takeDelimiter result}
E -- "null (EOF)" --> W
E -- "error.ReadFailed" --> W
E -- "[]u8 line" --> F[log.warn open stderr=line]
F --> D
E -- "error.StreamTooLong" --> G[take chunk_size bytes]
G -- "error.ReadFailed" --> W
G -- "error.EndOfStream" --> W
G -- "[]u8 chunk" --> F
W([exe.wait reaps child]) --> Z([thread exits])
Reviews (1): Last reviewed commit: "fix: stop open stderr reader at EOF" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return reader.takeDelimiter('\n') catch |outer| switch (outer) { | ||
| error.ReadFailed => error.ReadFailed, | ||
| error.StreamTooLong => reader.take(chunk_size) catch |inner| switch (inner) { | ||
| error.ReadFailed => error.ReadFailed, | ||
| error.EndOfStream => null, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
If the child process writes a final stderr message that doesn't end with \n (e.g. the process exits mid-line or writes "error: bad url" without a newline), takeDelimiter('\n') will reach EOF before finding the delimiter. In Zig's std.Io.Reader, hitting EOF without a delimiter causes takeDelimiter to return null rather than the partial bytes it already read, so that last message is never logged. The same scenario in the StreamTooLong branch returns null via the EndOfStream case, also silently discarding content. The existing test only exercises fully-terminated lines; adding a case like "first\nno-newline-at-end" would clarify actual behavior and surface any silent data loss.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/os/open.zig (1)
81-82: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose
stderrto prevent file descriptor leaks.The
.Pipefile descriptors are not closed automatically byexe.wait(). Ensurestderris explicitly closed when the thread completes reading to avoid exhausting file descriptors.🔧 Proposed fix to close the pipe
if (exe.stderr) |stderr| { + defer stderr.close(); var buffer: [256]u8 = undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/os/open.zig` around lines 81 - 82, Update the stderr-reading branch around exe.stderr to explicitly close the pipe after the reader thread finishes consuming it, ensuring the descriptor is released while preserving the existing stderr capture behavior.
🧹 Nitpick comments (2)
src/os/open.zig (1)
72-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle thread spawn failure to prevent leaks.
If
std.Thread.spawnfails, the child process is never reaped, leaving a zombie process and leaking the stderr pipe. Consider handling the error by killing and reaping the process before returning.🛠️ Proposed refactor for error handling
- const thread = try std.Thread.spawn(.{}, openThread, .{exe}); + const thread = std.Thread.spawn(.{}, openThread, .{exe}) catch |err| { + if (exe.stderr) |stderr| stderr.close(); + _ = exe.kill() catch {}; + _ = exe.wait() catch {}; + return err; + }; thread.detach();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/os/open.zig` around lines 72 - 73, Handle errors from std.Thread.spawn in the openThread launch path by terminating and reaping the child process and closing the stderr pipe before propagating the failure. Preserve the existing detached-thread behavior when spawning succeeds, and reuse the process and pipe cleanup mechanisms already available in the surrounding open logic.src/os/open_reader.zig (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd test coverage for the fallback path.
Consider adding a regression test that exercises the
error.StreamTooLongfallback logic by reading a line longer than the chunk size (e.g., using a small buffer). This ensures the fallback chunking logic remains correct during future refactors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/os/open_reader.zig` around lines 13 - 19, Add regression coverage alongside the existing “open stderr reader consumes delimiters and reaches EOF” test that forces takeLine to hit error.StreamTooLong by using a line longer than the supplied small chunk size. Verify the fallback path returns the complete line, then confirm subsequent reads still consume delimiters and reach EOF correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/os/open.zig`:
- Around line 81-82: Update the stderr-reading branch around exe.stderr to
explicitly close the pipe after the reader thread finishes consuming it,
ensuring the descriptor is released while preserving the existing stderr capture
behavior.
---
Nitpick comments:
In `@src/os/open_reader.zig`:
- Around line 13-19: Add regression coverage alongside the existing “open stderr
reader consumes delimiters and reaches EOF” test that forces takeLine to hit
error.StreamTooLong by using a line longer than the supplied small chunk size.
Verify the fallback path returns the complete line, then confirm subsequent
reads still consume delimiters and reach EOF correctly.
In `@src/os/open.zig`:
- Around line 72-73: Handle errors from std.Thread.spawn in the openThread
launch path by terminating and reaping the child process and closing the stderr
pipe before propagating the failure. Preserve the existing detached-thread
behavior when spawning succeeds, and reuse the process and pipe cleanup
mechanisms already available in the surrounding open logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7bf3245-a7e0-4674-990e-0dcf71b9ad01
📒 Files selected for processing (2)
src/os/open.zigsrc/os/open_reader.zig
1 participant
Summary
openprocess readerexe.wait()reaps the child processRoot cause
Ghostty is pinned to Zig 0.15.2. Its
Reader.takeDelimiterExclusiveregression leaves the delimiter buffered, so after the first stderr line the loop repeatedly returns an empty slice. Each affectedopencall then creates a permanent logging thread and an unreaped child process, driving both cmux and macOSlogdCPU usage.Testing
takeDelimiter; the same Zig 0.15.2 test now passes.1/1targeted tests passed.A full local GhosttyKit build is blocked on macOS 26.5 because Zig 0.15.2 cannot link against that SDK; the isolated test was compiled with the pinned Zig toolchain to WASI and executed successfully.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a runaway stderr logging loop in
os.openby correctly consuming newline delimiters and stopping at EOF. Prevents permanent logging threads and unreaped child processes that could spike CPU usage.open_reader.takeLine(based ontakeDelimiter) instead oftakeDelimiterExclusiveto consume newline delimiters and handle long lines.exe.wait()can reap the child process.takeDelimiterExclusiveregression that left delimiters buffered, causing empty-line loops.Written for commit 71ef84e. Summary will update on new commits.
Summary by CodeRabbit