Skip to content

fix(install): enforce smoke test timeout#3053

Open
jmnie wants to merge 4 commits into
dimensionalOS:mainfrom
jmnie:fix/jiaming/install-smoke-timeout-3017
Open

fix(install): enforce smoke test timeout#3053
jmnie wants to merge 4 commits into
dimensionalOS:mainfrom
jmnie:fix/jiaming/install-smoke-timeout-3017

Conversation

@jmnie

@jmnie jmnie commented Jul 19, 2026

Copy link
Copy Markdown

Contribution path

Problem

The post-install smoke test promises to stop after 60 seconds but launches DimOS without a timeout. Ctrl+C can also terminate the installer before it prints the successful-install quickstart.

Solution

  • Run the smoke command as an argument-preserving array under a Bash 3.2-compatible deadline loop.
  • Send SIGTERM at the deadline, then SIGKILL after a 5-second shutdown grace period so the timeout remains bounded.
  • Handle Ctrl+C inside the smoke test and return 130, allowing the completed installer to print its quickstart.

This does not depend on GNU timeout, which is absent from stock macOS.

How to Test

bash -n scripts/install.sh
pre-commit run --files scripts/install.sh
uv run mypy
pre-commit run --all-files --show-diff-on-failure

Direct checks on macOS with Bash 3.2:

  • exit 0 remains 0; exit 7 remains 7
  • a spaced argument remains one argument
  • a 1-second deadline returns 124 after 1 second
  • the production 60-second deadline returns 124 after 60 seconds
  • Ctrl+C returns 130 in about 0.4 seconds
  • a TERM-ignoring process is killed after the 5-second grace period and returns 124

The complete MuJoCo smoke command was not run locally; the timeout and signal paths were exercised against real shell processes. No robot hardware was used.

AI assistance

OpenAI Codex with GPT-5 was heavily involved in repository analysis, implementation, adversarial signal testing, and PR preparation. The contributor reviewed the complete diff, understands its timeout, signal, exit-code behavior and limitations, and stands behind the submitted changes.

Checklist

  • This PR is scoped to one issue or clearly stated problem.
  • I ran the relevant checks (uv run pytest, pre-commit) for the files I changed.
  • I have reviewed and understood every line in this PR.
  • I disclosed AI assistance above.
  • I have read and approved the CLA.

@jmnie
jmnie marked this pull request as ready for review July 19, 2026 16:15
@greptile-apps

greptile-apps Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a bounded timeout to the post-install smoke test. The main changes are:

  • Runs the smoke command with an argument-preserving array.
  • Sends SIGTERM at the deadline and SIGKILL after a grace period.
  • Handles Ctrl+C while allowing the installer to print its quickstart.
  • Restores the normal interrupt handler after the smoke test.

Confidence Score: 5/5

This looks safe to merge.

  • The temporary interrupt handler is installed before the smoke-test process starts.
  • No blocking issues remain in the updated interrupt path.

Important Files Changed

Filename Overview
scripts/install.sh Adds bounded smoke-test execution and moves temporary interrupt handling before the child process starts.

Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/jiaming/ins..." | Re-trigger Greptile

Comment thread scripts/install.sh Outdated
@TomCC7 TomCC7 added the first-time-contributor PR opened by an author who had not previously committed to this repository label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-time-contributor PR opened by an author who had not previously committed to this repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install.sh: smoke test never times out, and interrupting it reports "installation failed"

2 participants