os/open: guard against tight loop in stderr streaming

[chyroc]

Summary

Fixes a runaway loop in openThread that spams tens of thousands of empty open stderr= log warnings per second and pins the parent process at 300%+ CPU indefinitely.

Reproducer (cmux 1.3.x, Ghostty 1.3.2-dev, macOS)

Observed on a running cmux instance:

• ~45,000 log lines/sec all reading [com.mitchellh.ghostty:os-open] os-open: open stderr= (empty payload)
• log stream --process cmux reports Messages dropped during live streaming — flood exceeds os_log throughput
• Parent process at 300–400 % CPU across 5 threads, each producing ~9,000 lines/sec
• Log-flood survives surface / workspace teardown; only killing cmux stops it

sample <pid> top-of-stack was dominated by os_log_impl_flatten_and_send, os_log_create, _os_log, and zig_os_log_with_type, confirming the hot path is the log.warn inside openThread.

Root cause

Introduced in bb375a2 ("deal with large outputs from xdg-open/rundll32/open"), which replaced the batched collectOutput with a streaming reader loop.

Under Zig 0.15.2's new std.Io.Reader, takeDelimiterExclusive('\n') can return zero-length slices without throwing EndOfStream when the underlying pipe has reached EOF but the reader has not yet fully drained its state. On macOS, once /usr/bin/open <url> exits, its stderr pipe enters this state and every subsequent call returns an empty slice — no error variant of the outer switch matches, so the loop never breaks and log.warn("open stderr={s}", .{line}) fires every iteration with an empty string.

Because openThread is .detach()'d, each leaked instance runs forever. The five leaks in the reproducer corresponded to five prior open() calls over the lifetime of the process.

A secondary issue: the pre-patch code calls _ = exe.wait() catch {}; after the stderr block, so any of the existing break paths that leave the loop early skip the reap.

Fix

1. Empty-read guard: count consecutive zero-length lines and break after 16. This tolerates legitimate blank lines in child output (unlikely from open/xdg-open/rundll32 but future-proof) while providing a hard ceiling that prevents the tight loop.
2. defer exe.wait(): unconditional reap regardless of which branch exits the loop.
3. Coalesce identical error arms in the switch for readability (no behavior change).

Verification

• Manual repro: cmux with 5 leaked openThreads was pinning 3–4 cores at 100 % and flooding os_log at ~45k lines/sec; after applying this patch and rebuilding, CPU is idle and no os-open messages are emitted after each child exits.
• No behavioral change on the happy path (child writes finite stderr, terminates, loop exits via EndOfStream).

Notes

• The upstream ghostty-org/ghostty main branch carries the same bb375a2, so this fix applies there as well; happy to open a parallel PR upstream if desired.
• takeDelimiterExclusive returning an empty slice at EOF instead of EndOfStream may itself be a std.Io.Reader bug worth reporting to Zig, but a defensive guard here is the right call regardless.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Prevents a tight loop in openThread stderr streaming that spammed empty logs and pegged CPU after /usr/bin/open exited. Uses peek() to detect EOF on blank reads and always reaps the child process.

• Bug Fixes
  • After zero-length reads, call reader.peek(1) and only break on EOF/read error; add a 4096 no-progress cap as a safety net.
  • Move exe.wait() to defer so the child is reaped on all exits.
  • Coalesce identical error arms for clearer control flow (no behavior change).

^{Written for commit fb2ea98. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when launching and monitoring external processes.
  • Prevented hangs during stderr reading, including EOF races and repeated empty reads.
  • Ensured child processes are properly reaped after execution.
  • Improved handling of oversized or failed output streams.

[coderabbitai]

📝 Walkthrough

Walkthrough

openThread now unconditionally waits for the child process and hardens stderr reading against terminal errors, oversized streams, and repeated empty reads.

Changes

openThread I/O lifecycle

Layer / File(s)	Summary
Unconditional reaping and bounded stderr reads src/os/open.zig	openThread defers child-process reaping, handles stderr stream termination errors, limits repeated empty reads, and logs only non-empty lines.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• manaflow-ai/ghostty#129: Both changes update openThread stderr reading and termination handling.

Suggested reviewers: jcollie

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main fix: preventing a tight loop in stderr streaming for os/open.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[greptile-apps]

Greptile Summary

This PR prevents runaway stderr logging in openThread.

• Bounds consecutive zero-length stderr reads.
• Reaps the child through a deferred wait on every loop exit.
• Coalesces identical reader error branches.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking edge case where valid blank stderr records can leave a detached worker and child deadlocked.

The runaway loop is bounded and child reaping is made unconditional, but the fixed record-count heuristic can stop draining a live child’s bounded stderr pipe before wait().

Files Needing Attention: src/os/open.zig

Important Files Changed

Filename	Overview
src/os/open.zig	Adds bounded empty-read handling and unconditional child reaping, but terminating on valid blank records can stop pipe draining before the child exits.

_{Reviews (1): Last reviewed commit: "os/open: guard against tight loop in std..." | Re-trigger Greptile}

[greptile-apps]

Blank records stop pipe draining

Sixteen consecutive blank stderr records terminate the drain loop without establishing EOF. A child that continues writing can then fill the bounded stderr pipe while exe.wait() waits for that child to exit, leaving the detached worker and child blocked; keep draining until EOF or close the pipe before waiting.

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@src/os/open.zig`:
- Around line 80-101: The stderr loop in the open/execution flow incorrectly
treats 16 delimiter-separated blank lines as end-of-stream, potentially leaving
the child blocked on a full pipe. Update the logic around
reader.takeDelimiterExclusive('\n') so empty lines do not increment the
EOF/no-progress streak; only genuine zero-progress reads or terminal reader
errors should end draining. Add a regression test covering 16 blank lines
followed by substantial stderr output and verify the process completes without
blocking.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dc25f25d-7a4b-4d3d-95ba-512130a8b9b3

📥 Commits

Reviewing files that changed from the base of the PR and between aeed68c and 9b934e5.

📒 Files selected for processing (1)

• src/os/open.zig

[chyroc]

Thanks for the review, @coderabbitai and @greptile-apps — the concern is valid.

Pushed fb2ea98d to address it:

• Removed the empty_streak >= 16 early exit. Legitimate blank stderr lines no longer bias termination.
• After any zero-length line, use reader.peek(1) to test whether the stream is truly done. peek fails only at real EOF (or read error), so blank records continue draining until the child closes stderr.
• Kept a large no_progress counter (4096) purely as a safety net against a pathological reader that neither errors nor advances via peek — should never trigger in practice, but caps the blast radius of any future std.Io regression.

The upstream mirror (ghostty-org/ghostty#13480) has been updated with the same fix on the chyroc:fix/os-open-tight-loop-upstream branch, though that PR was auto-closed by ghostty-vouch pending contributor vouching.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline src/os/open.zig --items all --match 'openThread'
rg -n -C 18 'no_progress|empty_streak|takeDelimiterExclusive|\.peek\s*\(|openThread' src/os/open.zig
git diff --find-renames HEAD^ HEAD -- src/os/open.zig

Length of output: 5436

---

@chyroc Thanks—reader.peek(1) correctly fixes the original premature-termination case for ordinary blank stderr lines.

One remaining edge case: no_progress currently increments after every successful peek, including when takeDelimiterExclusive('\n') has just consumed a legitimate blank line. Therefore, 4,097 consecutive blank stderr records still stop draining, despite progress being made.

Please reset/remove the counter after a successful peek (or otherwise tie it to actual lack of reader progress), so the safety mechanism does not cap valid blank-line output.

🐇

[greptile-apps]

The updated approach is solid. Using peek(1) as the EOF discriminator is the right call — it correctly distinguishes a legitimate blank line (peek succeeds, bytes remain in the pipe) from the EOF race condition (peek errors, stream is exhausted), without introducing any false termination threshold.

A few notes on the current implementation:

1. no_progress is never reset — it increments on every blank line where peek succeeds, including legitimate ones. At 4096 blank lines it would spuriously terminate. Given this is explicitly a "should never trigger" safety net for a pathological reader, that's fine, but worth a comment clarifying it accumulates across all blank lines (not just no-progress ones).

2. peek return value — _ = reader.peek(1) catch break discards the peeked slice on success. That's correct; you only care about error/no-error, not the content.

3. defer exe.wait() placement looks good — it's scoped to openThread so it fires on every exit path including the no_progress safety break.

Overall this is a meaningfully better fix than the original streak counter. The logic is sound.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.