Skip to content

[CI/Build] Bound OmniRunner teardown and clean up after a failed init - #6126

Closed
ShengleiFu wants to merge 1 commit into
vllm-project:mainfrom
ShengleiFu:fix/bounded-runner-teardown
Closed

ShengleiFu wants to merge 1 commit into
vllm-project:mainfrom
ShengleiFu:fix/bounded-runner-teardown

Conversation

@ShengleiFu

@ShengleiFu ShengleiFu commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

A test runner whose constructor fails skips its own teardown entirely, so the run stalls with no failure reported and dies on the CI step timeout instead.

iter_omni_runner builds the runner inside the with expression, and Python evaluates the constructor before __enter__. OmniRunner.__init__ builds the engine eagerly (self.omni = Omni(...)), so when it raises, __exit__ never runs: no close(), no _cleanup_process() sweep for residual enginecore processes, no cleanup_dist_env_and_memory(). The subprocesses the constructor already started are abandoned, and the interpreter joins those non-daemon children at exit. Observed on a Merge-tier build: the test errored at 3m30s on a model-asset load failure, then sat idle for 37 minutes until the 40-minute ceiling, recorded as timed_out with exit -1 — so the real exception is visible only by reading the log. Separately, __exit__ is unbounded even on the success path: destroy_process_group() waits on peer ranks a mid-init failure has already killed, and close() carries no timeout.

This PR:

  • Adds tests/helpers/deadline.py with run_with_deadline (runs on a daemon thread, stops waiting after a deadline, reports rather than raises so a teardown failure cannot replace the error that caused it) and build_with_cleanup (runs cleanup when a constructor raises, then re-raises the original error). Both are free of vllm imports, so they are unit-testable without an accelerator.
  • Runs OmniRunner._teardown under TEARDOWN_DEADLINE_S (180s — ample for a healthy teardown, whose psutil sweep alone allows 8s; it bounds only the pathological case).
  • Runs the orphan sweep and device cleanup when construction fails, via cleanup_after_failed_runner_init.
  • Makes _cleanup_process a staticmethod; it never used self, and the failed-init path has no instance.

Deliberately not included: raising Buildkite timeout_in_minutes, or wrapping pytest in a shell timeout. Both relabel the stall rather than fix it, and neither restores the cleanup that was skipped.

Backward compatibility: no test-facing API changes. __exit__ runs the same steps in the same order, now under a deadline; the only new behaviour is cleanup on a path that previously ran none.

Test Plan

vLLM Version: 0.26.0
vLLM-Omni Commit: 12f8168 (base)

  • pytest tests/helpers/tests/test_deadline.py -v — CPU only, no accelerator
  • python -c "import tests.helpers.runtime" — the changed module still imports
  • pytest tests/e2e/offline_inference/test_bagel.py --collect-only — the fixture path still collects
  • Mutation checks that the new tests are not vacuous, reverting each behaviour in turn:
    • thread.join() with no timeout
    • no cleanup when the constructor raises (current main behaviour)
    • cleanup error propagating instead of the build error
  • ruff check + ruff format --check (v0.14.10, as pinned)

Test Result

  • tests/helpers/tests/test_deadline.py — 6 passed
  • import tests.helpers.runtime — clean; TEARDOWN_DEADLINE_S = 180.0
  • test_bagel.py --collect-only — 2 tests collected
  • Mutation checks, each landing on its covering test:
    • thread.join() unbounded → pytest hangs, killed at a 30s ceiling (exit 124); restored, exit 0
    • no cleanup on failed construction → 2 tests fail
    • cleanup error replaces build error → 1 test fails
  • Run on 2× RTX PRO 6000 with vLLM 0.26.0 (unit tests, import and collection checks) and on CPU locally

The first mutation is the point of the change: it reproduces this failure mode in miniature — a fixture that never returns, so pytest reports nothing — in 30 seconds without a GPU.

Not reproduced end to end: the original 37-minute stall needs an H100, BAGEL-7B, and the model fetch to fail again. The claim this PR defends is narrower and provable by reading the code — the failed-init path skips teardown, and __exit__ has no upper bound.

Not a duplicate: no open PR touches iter_omni_runner or OmniRunner teardown, and no open issue covers a fixture stalling after a failed initialization. #5675 also edits tests/helpers/, but in media.py's Whisper device selection — no overlap.

AI assistance was used for this change (log analysis, drafting and the mutation checks). I have reviewed every changed line and run the tests reported here.

`iter_omni_runner` builds the runner in the `with` expression, so a constructor
that fails part-way never reaches `__exit__`: none of the teardown runs, and the
engine subprocesses it already started are abandoned for the interpreter to join
at exit. The run then stalls with no failure reported and dies on the CI step
timeout, which hides the error that started it.

`__exit__` was also unbounded. `destroy_process_group()` waits on peer ranks that
a mid-init failure has already killed, and `close()` carries no timeout, so a
fixture can block there indefinitely.

- Add `tests/helpers/deadline.py` with `run_with_deadline` (run on a daemon
  thread, stop waiting after a deadline, report rather than raise so a teardown
  failure cannot replace the error that caused it) and `build_with_cleanup`
  (run cleanup when a constructor raises, then re-raise the original error).
  Free of vllm imports so both are unit-testable without an accelerator.
- Run `OmniRunner._teardown` under `TEARDOWN_DEADLINE_S`, and run the orphan
  sweep plus device cleanup when construction fails.
- Make `_cleanup_process` a staticmethod; it never used `self`, and the
  failed-init path has no instance.

Co-authored-by: Claude
Signed-off-by: Shenglei Fu <sfu@confluent.io>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@ShengleiFu
ShengleiFu marked this pull request as draft August 12, 2026 21:34
@ShengleiFu ShengleiFu closed this Aug 12, 2026
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.

1 participant