Skip to content

Arrange for subcommands to time out#43

Open
ctz wants to merge 1 commit into
mainfrom
jbp-command-timeout
Open

Arrange for subcommands to time out#43
ctz wants to merge 1 commit into
mainfrom
jbp-command-timeout

Conversation

@ctz

@ctz ctz commented Jul 6, 2026

Copy link
Copy Markdown
Member

No description provided.

Copilot AI 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.

Pull request overview

This PR adds explicit timeouts to the subprocesses executed by ci-bench-runner so that CI benchmark jobs cannot hang indefinitely when external commands (git/cargo/bench runs) stall.

Changes:

  • Refactors run_command to spawn the subprocess, drain stdout/stderr concurrently, and enforce a timeout via polling + kill.
  • Threads timeout values through each run_command call site (quick git ops vs cargo build vs benchmark/test runs).
  • Introduces timeout constants for the different command categories.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ci-bench-runner/src/runner.rs Outdated
Comment thread ci-bench-runner/src/runner.rs
@ctz ctz force-pushed the jbp-command-timeout branch from 3929b64 to 41d2ae1 Compare July 6, 2026 17:18

@cpu cpu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@ctz ctz force-pushed the jbp-command-timeout branch 4 times, most recently from 9983ab3 to 071d0a2 Compare July 7, 2026 13:53
@ctz ctz force-pushed the jbp-command-timeout branch from 071d0a2 to 02ad654 Compare July 7, 2026 13:53
@ctz

ctz commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

since review:

  • put subprocesses in a new pgroup, so they are killed all together and their shared stdout/stderr are closed properly
  • adjust build timeout, as it was currently quite close

currently deployed version is 02ad654 -- see https://bench.rustls.dev/info

Comment on lines +204 to +206
unsafe {
libc::killpg(child.id() as libc::pid_t, libc::SIGKILL);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

huh, I'm a little surprised we have to use libc/unsafe for this. Bummer.

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.

3 participants