Skip to content

fix(doctor): bound subprocess probes#764

Merged
matt2e merged 4 commits into
mainfrom
kalvin/doctor-timeouts
Jun 5, 2026
Merged

fix(doctor): bound subprocess probes#764
matt2e merged 4 commits into
mainfrom
kalvin/doctor-timeouts

Conversation

@kalvinnchau

Copy link
Copy Markdown
Contributor

summary

  • route doctor probe subprocesses through a bounded runner with timeout diagnostics
  • return promptly after timeout cleanup even when escaped descendants keep pipes open
  • share timeout row construction and aggregate freshness timeout output once per check
  • keep fix execution intentionally unbounded for user-triggered install/auth/update commands

tests

  • cargo test -p doctor
  • cargo clippy -p doctor --all-targets -- -D warnings

add a shared timeout runner for local doctor probes with 10s defaults and 15s freshness subprocess limits.

kill timed-out process groups on unix and return specific doctor timeout rows or freshness details instead of hanging the report.
kill and reap timed-out probes without joining reader threads so escaped
descendants cannot block doctor callers.

share timeout check construction and aggregate freshness timeout output while
keeping public report fields unchanged.
remove duplicate descendant cleanup coverage and keep the timeout runner tests
focused on caller bounds, large output capture, and basic timeout behavior.
collapse redundant runner assertions into the remaining regression tests and
shorten timeout diagnostic setup while preserving deadlock and aggregation
coverage.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35494ab6ef

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +161 to +162
let stdout = stdout_thread.map(join_reader).unwrap_or_default();
let stderr = stderr_thread.map(join_reader).unwrap_or_default();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep successful probes bounded while collecting output

When the direct child exits before timeout but leaves a descendant running with inherited stdout/stderr (for example a CLI probe that daemonizes or starts a background helper), wait_timeout returns Ok(Some(_)) and this code immediately joins the reader threads. Those threads are blocked in read_to_end until every inherited pipe handle closes, so the doctor can still hang indefinitely even though the probe child already finished and no timeout cleanup runs. This undermines the new bounded-probe behavior for any probed command with lingering children; the success path needs an output collection strategy that is also bounded or cleans up the process group before joining.

Useful? React with 👍 / 👎.

@matt2e matt2e merged commit 31d08d9 into main Jun 5, 2026
7 checks passed
@matt2e matt2e deleted the kalvin/doctor-timeouts branch June 5, 2026 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants