Repository navigation
Conversation
… error Omni.generate() (list mode) closed the whole engine after any exception, including a request-scoped OmniClientError the orchestrator had already classified as 4xx. The next generate() on the same Omni then failed with janus.ShutDown from AsyncOmniEngine.add_request (vllm-project#8449). _run_generation already aborts the in-flight requests and records the failure before re-raising, so close() was not needed for cleanup; it turned a per-request error into an engine shutdown. AsyncOmni.generate aborts and re-raises without shutting down, and vllm-project#6334 already dropped the unconditional close for py_generator=True. Close only on EngineDeadError (OmniEngineDeadError included), where a stage or the orchestrator is gone. Every other exception propagates with its status metadata and the engine stays usable. Regressions cover a client error followed by a successful generate(), a non-fatal server error, and the fatal path that still closes. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
Self-review
|
|
This PR appears to belong to: docs/design/module/profiling.md, docs/design/module/entrypoints.md, docs/design/module/observability.md. Module owners: @alex-jw-brooks @linyueqian @NickCao Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, semantic router, CODEOWNERS @fusheng-ji, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
hsliuustc0106
added
bug
…ep-engine-on-client-error
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
2 actionable finding(s).
CI at
50075943005b(2026-10-10T05:17:16.068550+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [claim-verified] #6334 generator path returns before iteration so generate()'s except never sees runtime errors
- [claim-verified] status metadata preserved: except Exception re-raises OmniClientError; test asserts status_code==400 and error_type.
- [claim-refuted] “_run_generation already aborts so close() is unused”: abort is
if "active_reqs" in locals()(omni.py:221-225) and active_reqs is assigned at 180 after the add_request loop. - [validated] vllm_omni/entrypoints/omni.py:15
from vllm.v1.engine.exceptions import EngineDeadErroris required for except EngineDeadError at omni.py:99 so OmniEngineDeadError (omni_base.py:52) and raw EngineDeadError both close; same import as omni_base.py:19 and async_omni_base.py:24; not a new dependency. - [validated] pytestmark=[core_model,cpu] at tests/entrypoints/test_omni_entrypoints.py:37; L1 runs pytest tests/entrypoints tests/engine -m 'core_model and cpu' (.buildkite/cuda/test-ready.yml:47)
- [validated] _client_error_then_outputs 1489-1492: first generate submitted==1, reuse >1; both arms reachable. submitted[0] is the single ["blocked"] request.
2 actionable finding(s).
Verdict: REQUEST CHANGES
See inline comments below.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| logger.exception("[Omni] Engine died during generation: %s", e) | ||
| self.close() | ||
| raise | ||
| except Exception as e: |
There was a problem hiding this comment.
[P1] This PR removes self.close() from Omni.generate()'s except Exception (omni.py:1…
Evidence and suggested fix
This PR removes self.close() from Omni.generate()'s except Exception (omni.py:105-110) and claims in-flight requests were already aborted in _run_generation. That abort (omni.py:221-225 if "active_reqs" in locals() and active_reqs) only runs after omni.py:180 active_reqs = set(request_ids), which is after every add_request returns. request_states are registered at omni.py:160 before add_request at omni.py:169; a successful earlier add_request already did request_queue.sync_q.put(msg). A later prompt whose process_inputs fails (async_omni_engine.py:352-357, unchanged) therefore leaves earlier prompts in request_states and on the orchestrator queue of the now-kept-alive engine. Victim is generate([ok, bad]), not the single-prompt ErrorMessage path the new tests cover. Initialize active_reqs before the submit loop or abort leftover request_states in this except, and add that batch test.
Evidence: Trigger: Omni.generate([ok, bad]) where the second engine.add_request raises in process_inputs. omni.py:105-110 except Exception as e: / logger.exception("[Omni] Failed to run generation: %s", e) / raise — this PR dropped self.close() here and comments that in-flight requests were already aborted. That abort is omni.py:221-225 except Exception: / if "active_reqs" in locals() and active_reqs: / self.abort(list(active_reqs)). active_reqs is assigned only after the submit loop: omni.py:180 active_reqs = set(request_ids). Each prompt is registered first: omni.py:160 self.request_states[req_id] = req_state then omni.py:169 self.engine.add_request(. Unchanged by this diff, present in the PR-time tree: a successful add_request enqueues the request (async_omni_engine.py:576 self.request_queue.sync_q.put(msg)) before a later prompt's process_inputs re-raises (async_omni_engine.py:352-357 except Exception: ... raise). Adverse effect: active_reqs is never bound so abort is skipped; the first prompt remains in request_states and on the live orchestrator queue, occupying the engine this PR now keeps up. New tests only cover single-prompt ErrorMessage after the wait loop.
Suggestion: except Exception as e:
# Request-scoped failure. Abort leftovers from a submit-loop
# raise (active_reqs is assigned only after every add_request).
logger.exception("[Omni] Failed to run generation: %s", e)
leftover = list(self.request_states)
if leftover:
self.abort(leftover)
raise
There was a problem hiding this comment.
Fixed in f6dddd4.
Thanks, confirmed. _run_generation() now initializes active_reqs before the try block and adds each request immediately when its ClientRequestState is registered, before sampling-parameter preparation or add_request(). A submission exception therefore aborts and removes all registered requests from that batch, including the failing request; prompts that were never registered are not aborted. Cleanup stays in _run_generation() so it also covers generator mode.
Added a regression test for ["ok", "bad", "not submitted"], parametrized over list and generator mode. It checks that the two registered requests are aborted, the third is never submitted, request_states is empty, the healthy engine is not shut down, and a subsequent generation succeeds. Both variants fail on the reviewed commit because no abort occurs.
Validation after the fix: 89 CPU tests passed across tests/entrypoints/test_omni_entrypoints.py and tests/engine/test_async_omni_engine_outputs.py (vLLM 0.31.0). All applicable pre-commit checks passed except the same 9 pre-existing mypy errors; no new type errors.
| logger.exception("[Omni] Engine died during generation: %s", e) | ||
| self.close() | ||
| raise | ||
| except Exception as e: |
There was a problem hiding this comment.
[P1] This diff removes self.close() from except Exception and only closes on `En…
Evidence and suggested fix
This diff removes self.close() from except Exception and only closes on EngineDeadError. List-mode Omni.generate() polls self.engine.try_get_output() (omni.py:187). On an empty queue with a dead orchestrator that API raises RuntimeError("Orchestrator died unexpectedly. See logs above.") (omni_engine_base.py:953-955), not EngineDeadError, so the new keep-alive path skips teardown. The next generate() then talks to a dead orchestrator, and stage processes are not released until GC/finalizer. Omni.errored is already not self.engine.is_alive() (omni_base.py:321). Close when self.errored in this except Exception branch, and add a CPU test that raises this RuntimeError from try_get_output and asserts shutdown_called (the new fatal test only covers ErrorMessage.fatal; FakeAsyncOmniEngine.try_get_output returns None on empty and never raises).
Evidence: Trigger: list-mode Omni.generate() consumes _run_generation, which polls try_get_output until requests finish. If the orchestrator thread dies with an empty output queue, that poll raises RuntimeError, not EngineDeadError; this PR's except Exception then re-raises without close(), so later generate() reuses the dead engine and stage GPU processes are not shut down. omni.py:187 (unchanged by this diff, present in the PR-time tree) msg = self.engine.try_get_output(). omni.py:99-110 (this diff) except EngineDeadError as e: … self.close() / raise then except Exception as e: … raise with no close(). Unchanged by this diff, present in the PR-time tree: omni_engine_base.py:953-955 except queue.Empty: / if not self.is_alive(): / raise RuntimeError("Orchestrator died unexpectedly. See logs above."). Unchanged: omni_base.py:321 return not self.engine.is_alive(). Unmet coverage: tests/entrypoints/test_omni_entrypoints.py:1550-1563 only drives ErrorMessage.fatal → EngineDeadError; FakeAsyncOmniEngine.try_get_output at tests/entrypoints/test_omni_entrypoints.py:162-166 except queue.Empty: / return None and never raises this RuntimeError. tests/engine/test_async_omni_engine_outputs.py:91-92 already pins the engine contract: with pytest.raises(RuntimeError, match="Orchestrator died unexpectedly"): engine.try_get_output().
Suggestion: except EngineDeadError as e:
# Fatal: a stage or the orchestrator is gone, so the engine cannot
# serve anything else. Release the processes now.
logger.exception("[Omni] Engine died during generation: %s", e)
self.close()
raise
except Exception as e:
# Request-scoped failure (e.g. OmniClientError -> 4xx). The in-flight
# requests were already aborted inside _run_generation; keep the
# engine alive so later generate() calls work, like AsyncOmni.
logger.exception("[Omni] Failed to run generation: %s", e)
if self.errored:
self.close()
raise
There was a problem hiding this comment.
Fixed in f6dddd4.
Thanks, confirmed. The generic exception handler now checks self.errored and closes the engine when the orchestrator is dead, while preserving the original exception. Healthy engines still remain reusable after request-scoped errors, and the existing EngineDeadError shutdown branch is retained.
Added a CPU regression test whose try_get_output() marks the orchestrator dead and raises RuntimeError("Orchestrator died unexpectedly. See logs above."). It checks that the error propagates, shutdown is called, and the in-flight request is aborted and removed. This test fails on the reviewed commit because shutdown is not called.
Validation after the fix: 89 CPU tests passed across tests/entrypoints/test_omni_entrypoints.py and tests/engine/test_async_omni_engine_outputs.py (vLLM 0.31.0). All applicable pre-commit checks passed except the same 9 pre-existing mypy errors; no new type errors.
Omni ReviewBot: finding feedback**[p1] This diff removes See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback[p1] This PR removes self.close() from Omni.generate()'s except Exception (omni.py:1… — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; no fallback: the fallb). |
Omni ReviewBot: no human activity for 7 days@fusheng-ji this pull request has had no human commit, comment or review since 2026-10-10. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
…ep-engine-on-client-error
| # reusable, but close if the orchestrator has died. | ||
| logger.exception("[Omni] Failed to run generation: %s", e) | ||
| self.close() | ||
| if self.errored: |
There was a problem hiding this comment.
[P2] Prevent errors from an aborted batch from failing the next call
If two requests in a batch enqueue non-fatal ErrorMessages before the first is consumed, the first error aborts the batch, but the second remains in the output queue. Since this branch now preserves the healthy engine, a subsequent generate(["valid prompt"]) consumes that stale error and fails, aborting the new request even though its input is valid.
engine.abort() only enqueues cancellation; it does not drain pending output messages. Meanwhile, _handle_output_message() raises ordinary non-fatal errors without checking whether their request IDs still belong to active request state.
Please discard non-fatal request-scoped errors for requests that have already been removed, while preserving global and fatal errors. Add a regression test where both requests in one batch enqueue errors, then verify that a subsequent valid generation succeeds on the same instance.
There was a problem hiding this comment.
Thanks, confirmed. Fixed in [10a1e3fd9](10a1e3f).
_handle_output_message() now discards non-fatal errors whose request IDs are no longer registered. Fatal errors are checked first; global errors and errors for active requests retain their existing behavior.
Each regression submits two failing requests, then a valid request on the same instance:
| Initial batch error | Mode | Reviewed handler (600045ad5): next valid call |
Fixed handler |
|---|---|---|---|
OmniClientError (400) |
List | Fails on stale error | Succeeds |
OmniClientError (400) |
Generator | Fails on stale error | Succeeds |
RuntimeError |
List | Fails on stale error | Succeeds |
RuntimeError |
Generator | Fails on stale error | Succeeds |
The initial batch still raises and aborts both requests. With the fix, the subsequent valid generation completes without another abort or engine shutdown. Generator results refer to consuming the returned generator.
Validation:
- 104 CPU tests passed, covering entrypoints, async engine outputs, and multimodal cache miss recovery.
- Restoring only the reviewed handler makes all five targeted regressions fail: the four cases above and the unknown-request handler test.
- All applicable changed-file pre-commit checks passed, including mypy and Ruff.
Ignore non-fatal errors for requests no longer registered in the offline frontend, while preserving fatal and global errors. Cover client and server errors in list and generator modes, and document the request error boundary. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
|
Please resolve CI failure. Thanks |
Omni ReviewBot: supersededThe CI failure noted on |
The test constructs AsyncOmni with object.__new__, bypassing request-state initialization. Register the active video request so the non-fatal overflow error is handled for a live request, and assert that the error preserves its request ID. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
Head branch was pushed to by a user without write access
Hi @Gaohan123, I’ve fixed the CI test fixture in [9c74c27b4](9c74c27). Local validation: 234 CPU tests passed, including the previously failing test and the stale-error regressions. All applicable pre-commit checks also passed. Could you please take another look when you have a chance? Thanks! |
Gaohan123
added
ready
Purpose
Fixes #8449.
Omni.generate()in list mode wrapped the whole run inexcept Exception: self.close(). Any request-scoped failure, including anOmniClientErrorthe orchestrator had already classified as 4xx, therefore shut the engine down, and the nextgenerate()on the sameOmnidied withjanus.ShutDownfromAsyncOmniEngine.add_request.generate()now closes onEngineDeadError, or on another exception whenself.erroredreports a dead orchestrator. Request-scoped errors still propagate with their status metadata while a healthy engine remains reusable._run_generation()tracks active requests as soon as their client state is registered, beforeadd_request(). If submission fails partway through a batch, it aborts and cleans up both earlier requests and the failing request, without including prompts that were never registered. This cleanup applies to both list and generator mode.Test Plan
Regression coverage with the existing
FakeAsyncOmniEngine:status_code/error_type, the request is aborted and removed fromrequest_states,shutdownis not called, and a secondgenerate()on the same instance completes;RuntimeError, no status): engine stays up, request aborted;ErrorMessage:OmniEngineDeadErrorpropagates and the engine is still closed;RuntimeErrorfrom output polling: the error propagates and the engine is closed.vLLM Version: 0.31.0 for the current CPU regression run; 0.30.0 for the original GPU reproduction below.
vLLM-Omni Commit: main
ee8fdab1Test Result
tests/entrypoints/test_omni_entrypoints.pyandtests/engine/test_async_omni_engine_outputs.py. All three new review regression cases fail on reviewed commit50075943and pass with this fix.main: the two "engine stays usable" tests fail and the fatal-path test passes.mypy-3.10: its 9 errors are identical to the reviewed commit (none added).Original GPU validation (not rerun for the review follow-up), 1 x B200, frozen
vllm/vllm-openai:v0.30.0container,tiny-random/Qwen-Image, oneOmniinstance, 2 steps / 256x256. The rejected request is produced by the admission check from #8448 ({"prompt_ids": [...]}->OmniClientError400), so tree A is the #8448 branch alone and tree B is this fix with the same #8448 engine files overlaid:Omniinstance)omni.py{"prompt_ids": [...]}OmniClientError400OmniClientError400janus.ShutDownjanus.ShutDownScripts and raw logs (
validate_offline_close.py,job.sh,result_*.json,log_*.txt) are kept outside git.AI assistance: Codex and Claude Code were used to trace the error path, draft the change, tests and this description. All changes were reviewed and the tests run by the author.