Skip to content

fix: add timeout to run_command() to prevent indefinite hangs - #3964

Open
Quratulain-bilal wants to merge 1 commit into
github:mainfrom
Quratulain-bilal:fix/subprocess-timeout-utils
Open

fix: add timeout to run_command() to prevent indefinite hangs#3964
Quratulain-bilal wants to merge 1 commit into
github:mainfrom
Quratulain-bilal:fix/subprocess-timeout-utils

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Problem

"run_command() utility called subprocess.run() without timeout. If a spawned process hangs, the CLI blocks forever.

Fix

  • Add imeout parameter (default 120s)
  • Handle subprocess.TimeoutExpired exception
  • Remove redundant hasattr(e, 'stderr') check

Add a timeout parameter (default 120s) to subprocess.run() calls in
run_command() utility. Previously, if a spawned process hung, the CLI
would block forever with no way to recover.

Also removes redundant hasattr(e, 'stderr') check on CalledProcessError
which always has a stderr attribute when capture_output=True.

Assisted-by: GitHub Copilot (model: mimo-v2-free, supervised)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a default timeout to prevent run_command() from hanging indefinitely.

Changes:

  • Adds configurable 120-second timeout.
  • Reports and re-raises timeout failures.
  • Simplifies stderr handling.
Show a summary per file
File Description
src/specify_cli/_utils.py Adds subprocess timeout support and error reporting.

Review details

馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/_utils.py
Comment on lines +109 to +111
except subprocess.TimeoutExpired:
console.print(f"[red]Command timed out after {timeout}s:[/red] {' '.join(cmd)}")
raise
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