[PeaceMaker-best]

Purpose

Stage milestones and aggregate transfer durations cannot represent the four
first-payload boundaries independently. This core candidate for #6472 adds
typed, once-only event recording owned by the existing ClientRequestState.
Directed edges and segment generations are isolated; retired segment handles
and closed request owners cannot accept late events or attach to a reused ID.

Detached snapshots use elapsed milliseconds, preserve missing events as null,
declare each event's clock domain and omit request IDs. Deltas are available
only for a shared clock and origin. The existing entrypoint response builder
can carry enabled snapshots, and canonical request cleanup closes their owner.
Collection is absent by default.

Related to #6472. This is the generic core and synthetic proof path, rather than
a completed four-event production deployment. Worker/connector instrumentation,
canonical benchmark serialization and real-GPU reconciliation remain separate
work. The RFC's event/schema/clock decisions still need maintainer alignment.
Proposed publication: Draft; do not close the RFC.

Test Plan

vLLM Version: official 0.31.0+cpu wheel, CPU PyTorch 2.13.0,
Transformers 5.14.1, Python 3.12.3, Ubuntu 24.04 Docker environment.

vLLM-Omni Commit: base 9146284c16abbe7309f3f79dfdacfb48cdf2499a;
implementation 9323594.

Upstream CPU command:

python -m pytest tests/metrics/test_stream_edge_events.py -m 'core_model and cpu' -q -p no:cacheprovider --basetemp=/tmp/pytest

Test Result

• 17 passed in a hardened network-disabled CPU Docker container using the
  real vLLM runtime and normal repository pytest entrypoint. Production modules
  were imported normally. The response factory is mocked by the unit test;
  no production method was replaced. Warnings: TorchScript deprecation and the
  development checkout's version fallback. No GPU models or weights were loaded.
• Synthetic two-stage coverage includes successful transfer, send/admission
  failures, empty terminal payloads, consumer buffering, duplicates, interleaved
  requests/edges/segments, stale handles, zero versus missing values, detached
  incremental response snapshots, reused external IDs and canonical cleanup.
• Ruff 0.14.10 lint/format, Typos 1.49.0, SPDX, forbidden imports, CUDA-call policy, test
  marks and Markdown checks passed. The new core, request-state additions and
  test file pass mypy 1.11.1 for Python 3.10.
• Native changed-file mypy passes the stream-edge module, client request state
  and test file. The same-environment upstream omni_base.py baseline has the
  same two diagnostics: OmegaConf conversion typing and EngineQueueMessage.type.
  Source-line comparison confirms no new diagnostics. Full-file mypy is not
  green; no full pre-commit or upstream CI success is claimed.
• No real worker event collection or performance claim.

[vllm-omni-review-bot]

This PR appears to belong to: docs/design/module/observability.md.

Module owners: @lishunyang12 @vraiti @hsliuustc0106

Routing: @lishunyang12 via module of the changed files, semantic router, CODEOWNERS; @vraiti via module of the changed files, semantic router; @hsliuustc0106 via CODEOWNERS

@PeaceMaker-best, 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.

[vllm-omni-review-bot]

Omni ReviewBot routing record

Assigned Strict on cursor (cursor-grok-4.6-high) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.

[MinhaoLi0318]

Thanks for limiting this PR to the core and a synthetic caller, and for stating clearly what the description does and does not prove. I read the full diff and the touched code on main at the PR base (9146284). The first-write-wins and segment-fencing logic in stream_edge.py matches what the tests assert. My comments are about the contract around it:

1. The sync abort path skips the release. Omni.abort() (vllm_omni/entrypoints/omni.py:222-228) pops request_states directly, and Omni._run_generation calls it on GeneratorExit and on exceptions (omni.py:208-216). So the doc's "including cancellation and errors" guarantee holds for AsyncOmni, which always goes through _log_summary_and_cleanup in the finally of generate(), but not for Omni. Details inline.
2. The transport is not described yet. The owner lives on ClientRequestState in the entrypoint process, but three of the four events happen in stage or connector code. Please state in the design doc which existing channel carries observations to the owner (for example, a field on StageMetricsMessage applied in OmniBase._process_stage_metrics_message), who calls start_segment for non-segmented edges, and which component sets the origin for each clock_domain. That choice decides whether the handle API and its fencing have the right shape (see inline at stream_edge.py:100).
3. Telemetry errors can fail a request. record(), start_segment() and enable_stream_edge_metrics() raise on data or lifecycle conditions. The existing per-request transfer metrics swallow errors instead (vllm_omni/metrics/stats.py:335, :369). I suggest returning False for runtime conditions and raising only for programmer errors.
4. Smaller test and docs points are inline.

Process note: the description proposes Draft, and #6472 asks for maintainer alignment on ownership, clock domain, identity and cleanup before the core implementation starts. I could not find a maintainer reply on the RFC yet. It may be worth asking an observability owner to confirm points 2 and 3 before merge, so that the first adopter PR does not have to change this API.

[MinhaoLi0318]

This covers every path that reaches _log_summary_and_cleanup, but the sync entrypoint has a second removal path. Omni.abort() in vllm_omni/entrypoints/omni.py:222-228 does self.request_states.pop(req_id, None) without releasing, and _run_generation calls it on GeneratorExit and on exceptions (omni.py:208-216). As a result, a recorder kept past an aborted sync request still returns True, which contradicts stream_edge_first_events.md:28-29. Suggestion: move release + pop into a small helper (for example _drop_request_state(request_id)) and use it from both _log_summary_and_cleanup and Omni.abort. Then add a CPU test that builds object.__new__(Omni) with log_stats = False and a mocked engine, calls abort("internal"), and asserts that a retained recorder returns False.

[MinhaoLi0318]

Raising here turns a bad clock reading at an instrumented boundary (NaN, or a negative value when the origin was set after the event) into an exception in a connector or output path. Existing per-request transfer telemetry returns None on any error instead (vllm_omni/metrics/stats.py:335, :369). Validation also depends on the guard at line 76: the same NaN is silently ignored (record() returns False) with meaningful=False or on a stale handle, but raises with meaningful=True. Suggestion: keep ValueError for programmer errors (a value that is not a StreamEdgeEvent, an invalid StreamEdgeKey). For an invalid elapsed_ms, chunk_seq or empty clock_domain, return False and log at logger.debug. Then change test_invalid_elapsed_time_is_rejected_without_state to assert False and an empty snapshot.

[MinhaoLi0318]

Could the design doc name the intended channel? Producer emit, send complete and consumer accept are observed in worker processes (model-runner connector code) or on the orchestrator thread, and this docstring says that code must not mutate the owner directly. Suppose observations travel on StageMetricsMessage (vllm_omni/engine/messages.py:119) and are applied in OmniBase._process_stage_metrics_message (omni_base.py:478 at this head). Two consequences follow. (a) Late messages after cleanup are already dropped by the request_states.get() lookup there (omni_base.py:480-482). (b) The handler only has a key, so it would call start_segment(key) per message. That is idempotent for the current segment, but a late observation for an older segment raises ValueError (stream_edge.py:113-114). If the first observation for segment 1 arrives before the last one for segment 0, the segment-0 observation either raises (through start_segment) or is dropped (through a retained handle). So it matters who advances segments, and who calls start_segment for non-segmented edges. A key-based RequestStreamEdgeEvents.record(key, event, ...) that returns False for a retired segment may be a simpler entry point for that handler. Either way, one sentence about this in the doc would help whoever reviews the first adopter.

[vllm-omni-review-bot]

Omni ReviewBot review

1 actionable finding(s).

CI at 9323594bf83d (2026-10-10T12:06:10.238554+00:00): required check(s) blocking: buildkite/vllm-omni (missing).

Full review analysis

Scan:

Category	Result
Tests / verification	1 finding(s) below
Security	no finding reported
Docs / comments	no finding reported
Behavior / compatibility	no finding reported
Correctness	no finding reported

Validated:

• [resolved] process_stage_metrics in stats.py:514 overwrites metrics but has no callers at this head — omni_base builder is the only live attacher
• [resolved] first-write-wins / segment fence / detached snapshot match tests; residual: start_segment(same index) after a later generation is the retired ValueError, which would crash an event-loop apply path that replays old keys (channel thread already posted)
• [resolved] first-write-wins + later-segment fence match tests; residual: equal segment_index rebinds without fencing (intentional reconnect)
• [resolved] _process_single_result duck-type: test_emit_calls.py:276-277 sets final_output=False so line 676-677 returns before 735; production ClientRequestState always defines stream_edge_events.
• [claim-verified] Snapshot omits request ids as structure; synthetic test forbids 'internal'/'external' strings.
• [claim-verified] no worker/connector record() calls; design L66-67 no perf claim

1 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!}

[vllm-omni-review-bot]

[P1] This diff calls req_state.release_stream_edge_metrics() in `log_summary_and…

Evidence and suggested fix

This diff calls req_state.release_stream_edge_metrics() in _log_summary_and_cleanup's finally (vllm_omni/entrypoints/omni_base.py:453) immediately before request_states.pop and _publish_request_gauges. Existing L1 CPU stubs in tests/metrics/test_prometheus.py (SimpleNamespace in request_states) and tests/metrics/test_emit_calls.py (_log_summary_and_cleanup("req-1", reason="stage_error")) have no such method, so the new call raises AttributeError and the rest of that finally never runs — the request leaks in request_states and running/waiting gauges are not republished. Production ClientRequestState has the method; victims are those cleanup tests and any duck-typed request_states entry. Guard the new call and keep pop/gauges in an inner finally. The PR test plan only ran test_stream_edge_events.py, which uses a real ClientRequestState and cannot catch this.

Evidence: vllm_omni/entrypoints/omni_base.py:452-458 finally: / if req_state is not None: / req_state.release_stream_edge_metrics() then self.request_states.pop(request_id, None) and self._publish_request_gauges(len(self.request_states)) — an AttributeError on the new call aborts the rest of this finally. Unchanged by this diff, present in the PR-time tree: tests/metrics/test_prometheus.py:10 pytestmark = [pytest.mark.core_model, pytest.mark.cpu]; tests/metrics/test_prometheus.py:164 obj.request_states["req-1"] = SimpleNamespace(metrics=SimpleNamespace(e2e_done={"req-1"})) then :167 obj._log_summary_and_cleanup("req-1") — SimpleNamespace has no release_stream_edge_metrics, so this L1 gauge-republish test raises before pop. Same trigger, also unchanged: tests/metrics/test_emit_calls.py:111-120 obj.request_states["req-1"] = SimpleNamespace(...) then obj._log_summary_and_cleanup("req-1", reason="stage_error"). Production ClientRequestState (this diff) defines the method at client_request_state.py:66 def release_stream_edge_metrics(self) -> None:; the new PR test uses a real ClientRequestState and cannot catch these stubs.

Suggestion: try:
if req_state is not None:
release = getattr(req_state, "release_stream_edge_metrics", None)
if release is not None:
release()
finally:
self.request_states.pop(request_id, None)

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

**[p1] This diff calls req_state.release_stream_edge_metrics() in _log_summary_and_…** — vllm_omni/entrypoints/omni_base.py:452`

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[PeaceMaker-best]

@MinhaoLi0318 Thanks for the detailed contract review. Author self-review at 9323594: I confirmed the sync Omni.abort() path removes the request without closing its recorder. I also reproduced the cleanup compatibility regression noted in the automated review.

Using the same real-vLLM, non-root, network-disabled CPU Docker environment:

• PR base 9146284: existing test_prometheus.py + test_emit_calls.py — 42 passed.
• PR head 9323594: those files + test_stream_edge_events.py — 4 failed, 55 passed. All four failures are AttributeError at the new release_stream_edge_metrics() call on existing SimpleNamespace fixtures. Both runs reported 15 warnings (TorchScript deprecation and development-version fallback).

Head command:

python -m pytest tests/metrics/test_stream_edge_events.py tests/metrics/test_prometheus.py tests/metrics/test_emit_calls.py -m 'core_model and cpu' -q -p no:cacheprovider --basetemp=/tmp/pytest

The base command omits the new test_stream_edge_events.py file. This is an introduced regression; the original 17-test result was too narrow.

Required revisions:

1. Share recorder release/removal between canonical cleanup and sync abort, preserve pop/gauge publication if telemetry release fails, and cover retained recorders after sync cancellation and errors.
2. Make runtime telemetry/lifecycle conditions fail open: bad elapsed values, missing clock information, late observations, and released/retired state should not fail inference. Keep exceptions for programmer contract errors and test that rejected observations leave no state.
3. Document a candidate observation channel, such as an identity-bearing envelope on StageMetricsMessage applied by _process_stage_metrics_message, for metrics-owner alignment. The channel must distinguish request instances across ID reuse; the current in-process handles alone do not prove that.

This should follow the RFC clarification: non-segmented edges use segment 0; runtime establishes identity/origin before dispatch; arriving telemetry never advances segments. Comparable clocks and identical origins need separate verification. Resumable lifecycle transitions and production clock/transport validation remain dependencies. Contributor discussion is not maintainer approval of that contract.

The current head remains an unready recording-core proposal. No fixes or production event collection are claimed in this response.

AI assistance: Codex helped inspect the paths, run the CPU comparisons, and draft this response.

[vllm-omni-review-bot]

Omni ReviewBot: no human activity for 7 days

@PeaceMaker-best 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.

[vllm-omni-review-bot]

Omni ReviewBot: no human activity for 14 days

@PeaceMaker-best this pull request has had no human commit, comment or review since 2026-10-10. Please consider marking this PR as draft until work can resume. 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.