From eb694a6d885688b70532c696855e9cbbc6531efc Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 3 Sep 2026 08:47:44 +0000 Subject: [PATCH 01/11] feat(vllm): OOM detection, serve-summary memory guidance, and diagnose routing (EAI-8059) Squashed the EAI-8059 series into one logical change: narrow vLLM OOM detection, gate the serve-summary memory guidance on a real launch failure (not a reused already-running service), tie the note's log-tail budget to the shared engine constant, and cover it with a feature-gated e2e scenario that reuses a planted managed service so it allocates no GPU memory. Original commits: - feat(vllm): enhance OOM diagnostics and guidance in serve summary - fix(vllm): address review feedback on OOM diagnostics - serve: narrow OOM detection and gate the note on real launch failure - serve: detect reusable running service before GPU pre-flight - e2e: cover positive OOM-launch memory guidance via fault injection - e2e: gate serve-oom scenario on the fault-injection build - serve: tie OOM note tail budget to the shared engine constant Signed-off-by: Roman Sirokov --- apps/rocm/Cargo.toml | 6 + apps/rocm/src/main.rs | 453 ++++++++++++++++-- apps/rocm/src/serve_summary.rs | 98 ++++ crates/rocm-core/src/lib.rs | 46 ++ engines/vllm/src/lib.rs | 32 +- .../features/model_serving.feature | 34 ++ tests/e2e-cucumber/src/capability.rs | 23 + tests/e2e-cucumber/src/expectation.rs | 59 +++ tests/e2e-cucumber/tests/e2e/serving_steps.rs | 168 ++++++- tests/e2e-cucumber/tests/e2e/tui_driver.rs | 33 ++ xtask/src/e2e.rs | 36 +- 11 files changed, 942 insertions(+), 46 deletions(-) diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 944e83e8b..7c462eaba 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -12,6 +12,12 @@ workspace = true [features] e2e-test-hooks = ["rocm-engine-lemonade/e2e-test-hooks"] +# Test-only fault injection for the e2e suite. Enabled ONLY by the `xtask e2e` +# mock-lane build (never by the release build), it compiles in a `rocm serve` +# hook that simulates a managed vLLM launch which OOM'd and never became ready, +# so the positive OOM-guidance summary can be verified on a GPU-less CI host. +# See `e2e_simulate_oom_launch` / `simulate_oom_managed_launch` in `main.rs`. +e2e-oom-fault-injection = [] [dependencies] anyhow.workspace = true diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 92380bf8b..b25c1a00b 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -5120,17 +5120,69 @@ fn serve(args: ServeArgs) -> Result<()> { // surface the explicit `--gpu` as ignored rather than printing a device the // server will not use. let cpu_only = matches!(device_policy, DevicePolicy::CpuOnly); + // `--gpu` selects by the amd-smi `gpu` ordinal but is exported via + // `HIP_VISIBLE_DEVICES`; those orderings can diverge when + // `ROCR_VISIBLE_DEVICES`/partitioning is in play, so warn at serve time. + let rocr_visible_devices_set = std::env::var_os("ROCR_VISIBLE_DEVICES").is_some(); + let resolved_selection = resolve_engine_selection( + &config, + &selected_engine, + runtime_id.as_deref(), + env_id.as_deref(), + ); + let resolved_selection = validate_engine_selection_runtime(&paths, resolved_selection)?; + // Reusing an already-running managed service launches nothing and pins no + // GPU, so it must bypass the GPU-required pre-flight below — the reused + // service was already vetted at its own launch, and this invocation does no + // GPU work. Detect that here, but only when a live managed service for this + // engine already exists (so the common launch path keeps failing fast before + // any engine work) and the model ref can be canonicalized (a runtime/env is + // available, or the engine manages its own). Without a runtime we cannot + // resolve, so we fall through and the pre-flight refuses the no-GPU / + // no-runtime case with its usual message. + let mut resolved_model: Option = None; + let mut reuse_existing = false; + let can_resolve_model = !cpu_only + && (resolved_selection.runtime_id.is_some() + || resolved_selection.env_id.is_some() + || engine_manages_own_runtime(&selected_engine)); + if can_resolve_model && any_live_managed_service_for_engine(&paths, &selected_engine) { + if engine_manages_own_runtime(&selected_engine) { + ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; + } + let probe = engine_request::<_, ResolveModelResponse>( + Some(&paths), + &selected_engine, + EngineMethod::ResolveModel, + &ResolveModelRequest { + model_ref: engine_model_ref.clone(), + runtime_id: resolved_selection.runtime_id.clone(), + device_policy: Some(device_policy.clone()), + recipe_override: None, + engine_recipe: engine_recipe.clone(), + }, + )?; + reuse_existing = + existing_live_managed_service(&paths, &selected_engine, &probe.canonical_model_id) + .is_some(); + resolved_model = Some(probe); + } // Fail fast under a GPU-required policy when the host has no usable AMD GPU, // BEFORE preparing or launching any engine (no wasted engine download, and an // actionable message instead of a late engine crash). The engine enforces the - // same rule as a backstop. Skipped for cpu_only; permissive when availability + // same rule as a backstop. Skipped for cpu_only and when reusing an + // already-running service (nothing is launched); permissive when availability // cannot be probed on this platform (probe returns `None`). The E2E-only // backend-failure scenario bypasses this host precondition so the black-box - // test reaches Lemonade's backend boundary without real GPU hardware. + // test reaches Lemonade's backend boundary without real GPU hardware, and the + // E2E-only OOM-launch fault injection likewise stands in for the GPU it does + // not have. let scripted_backend_failure = cfg!(feature = "e2e-test-hooks") && std::env::var_os("ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE").is_some(); if !cpu_only && !scripted_backend_failure + && !reuse_existing + && !e2e_simulate_oom_launch() && let Some(usable) = rocm_core::usable_amd_gpu_indices() && usable.is_empty() { @@ -5142,23 +5194,6 @@ fn serve(args: ServeArgs) -> Result<()> { policy = device_policy_name(&device_policy) ); } - // `--gpu` selects by the amd-smi `gpu` ordinal but is exported via - // `HIP_VISIBLE_DEVICES`; those orderings can diverge when - // `ROCR_VISIBLE_DEVICES`/partitioning is in play, so warn at serve time. - let rocr_visible_devices_set = std::env::var_os("ROCR_VISIBLE_DEVICES").is_some(); - let gpu_vram = if cpu_only { None } else { gpu_vram_usage() }; - let gpu_indices = if cpu_only { - Vec::new() - } else { - resolve_gpu_indices(&paths, &gpu_selection, gpu_vram.as_deref())? - }; - let resolved_selection = resolve_engine_selection( - &config, - &selected_engine, - runtime_id.as_deref(), - env_id.as_deref(), - ); - let resolved_selection = validate_engine_selection_runtime(&paths, resolved_selection)?; if !matches!(device_policy, DevicePolicy::CpuOnly) && resolved_selection.runtime_id.is_none() && resolved_selection.env_id.is_none() @@ -5171,21 +5206,31 @@ fn serve(args: ServeArgs) -> Result<()> { } if !matches!(device_policy, DevicePolicy::CpuOnly) && engine_manages_own_runtime(&selected_engine) + && resolved_model.is_none() { ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; } - let resolve = engine_request::<_, ResolveModelResponse>( - Some(&paths), - &selected_engine, - EngineMethod::ResolveModel, - &ResolveModelRequest { - model_ref: engine_model_ref, - runtime_id: resolved_selection.runtime_id.clone(), - device_policy: Some(device_policy), - recipe_override: None, - engine_recipe, - }, - )?; + let gpu_vram = if cpu_only { None } else { gpu_vram_usage() }; + let gpu_indices = if cpu_only { + Vec::new() + } else { + resolve_gpu_indices(&paths, &gpu_selection, gpu_vram.as_deref())? + }; + let resolve = match resolved_model { + Some(resolve) => resolve, + None => engine_request::<_, ResolveModelResponse>( + Some(&paths), + &selected_engine, + EngineMethod::ResolveModel, + &ResolveModelRequest { + model_ref: engine_model_ref, + runtime_id: resolved_selection.runtime_id.clone(), + device_policy: Some(device_policy), + recipe_override: None, + engine_recipe, + }, + )?, + }; let service_id = generate_service_id(&selected_engine, &resolve.canonical_model_id); // Attached foreground streaming is the debugging path, selected by `--verbose` @@ -5341,6 +5386,17 @@ fn serve(args: ServeArgs) -> Result<()> { host_gpu_summary.as_ref(), engine_serves_vllm, ); + // A managed serve that failed on VRAM lands here not-ready with the OOM + // traceback only in its own log. Read that log tail so the summary can + // name the memory knobs, since the pre-launch low-VRAM warning cannot + // fire without amd-smi/rocm-smi telemetry. + let notes = append_oom_serve_note( + notes, + engine_serves_vllm, + report.already_running, + &report.status, + report.log_path.as_deref(), + ); let summary = serve_summary::DeploymentSummary { engine: selected_engine.clone(), requested_model: model, @@ -5423,6 +5479,141 @@ fn collect_serve_notes( notes } +/// Test-only fault-injection switch, off (`const false`) in shipped binaries: +/// the `e2e-oom-fault-injection` feature is enabled only by the `xtask e2e` +/// mock-lane build, never by the release build. When enabled and +/// `ROCM_E2E_SIMULATE_OOM_LAUNCH=1` is set on the process, `rocm serve` fabricates +/// the on-disk state of a managed vLLM launch that spawned, wrote an +/// out-of-memory traceback to the log it owns, and never became ready — so the +/// positive OOM-guidance summary path can be verified on a GPU-less CI host, +/// where a real launch cannot run. See [`simulate_oom_managed_launch`]. +#[cfg(feature = "e2e-oom-fault-injection")] +fn e2e_simulate_oom_launch() -> bool { + std::env::var_os("ROCM_E2E_SIMULATE_OOM_LAUNCH").is_some_and(|value| value == "1") +} + +/// Release builds carry no fault-injection switch: the seam compiles out +/// entirely, so `rocm serve` behaves identically to a build without the feature. +#[cfg(not(feature = "e2e-oom-fault-injection"))] +const fn e2e_simulate_oom_launch() -> bool { + false +} + +/// Test-only (see [`e2e_simulate_oom_launch`]): fabricate the on-disk state of a +/// managed vLLM launch that spawned, OOM'd, and never became ready, then return +/// the launch report the summary path would see for it — `already_running: false` +/// (this invocation launched it), `status: "starting"` (it failed to become +/// ready), and a `log_path` whose contents carry a real allocator OOM signature. +/// [`append_oom_serve_note`] then renders the memory-knob guidance. No process is +/// spawned. Compiled out of shipped binaries. +#[cfg(feature = "e2e-oom-fault-injection")] +#[allow(clippy::too_many_arguments)] +fn simulate_oom_managed_launch( + paths: &AppPaths, + engine: &str, + service_id: &str, + requested_model: &str, + resolve: &ResolveModelResponse, + host: &str, + port: u16, + device_policy: &DevicePolicy, + gpu_indices: &[u32], + runtime_id: Option<&str>, + env_id: Option<&str>, +) -> Result { + paths.ensure()?; + fs::create_dir_all(paths.services_dir())?; + let mut record = ManagedServiceRecord::new( + paths, + service_id, + engine, + requested_model, + resolve.canonical_model_id.clone(), + host, + port, + "managed", + std::process::id(), + runtime_id.map(str::to_owned), + env_id.map(str::to_owned), + Some(device_policy_name(device_policy).to_owned()), + ); + record.gpu_indices = gpu_indices.to_vec(); + record.status = "starting".to_owned(); + record.write()?; + // A real allocator OOM signature (the same torch/HIP line + // `rocm_core::vllm_log_shows_oom` matches) in the log this invocation owns, so + // the summary renders memory guidance for a launch that actually OOM'd. + fs::write( + &record.log_path, + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .with_context(|| format!("failed to write {}", record.log_path.display()))?; + Ok(ManagedLaunchReport { + service_id: service_id.to_owned(), + endpoint_url: format!("{}/v1", format_http_base_url(host, port)), + status: "starting".to_owned(), + already_running: false, + child_pid: None, + log_path: Some(record.log_path), + manifest_path: Some(record.manifest_path), + }) +} + +/// Appends the actionable memory-knob note when a managed serve failed to become +/// ready and its engine log carries an out-of-memory signature. +/// +/// This is the post-failure companion to the pre-launch low-VRAM warning in +/// [`collect_serve_notes`]. The reported low-VRAM environment ships no +/// rocm-smi/amd-smi, so that telemetry-driven warning never fires there; reading +/// the failed serve's own log tail is the reliable signal that the launch died on +/// VRAM. Gated on vLLM (the only engine exposing `--gpu-memory-utilization`), on +/// the launch having actually *failed to become ready* +/// ([`serve_summary::serve_failed_to_become_ready`] — a healthy still-loading +/// `running` service is not a failure), and on this invocation actually having +/// launched the process (`already_running` is excluded) so healthy deployments, +/// unrelated failures, and an unrelated invocation that merely reused an +/// already-live service are never misattributed. The OOM-signature check in +/// [`serve_summary::oom_memory_note`] narrows it further. Also skips a note +/// whose hint text is already present in `notes` (the pre-launch low-VRAM +/// warning may have added it) so the same fix is never printed twice. +fn append_oom_serve_note( + mut notes: Vec, + engine_is_vllm: bool, + already_running: bool, + status: &str, + log_path: Option<&Path>, +) -> Vec { + if !engine_is_vllm || already_running || !serve_summary::serve_failed_to_become_ready(status) { + return notes; + } + // Cheap guard first: if the shared hint is already in `notes` (added by the + // pre-launch low-VRAM warning) there is nothing to add, so skip the log read + // entirely. + let already_hinted = notes + .iter() + .any(|note| note.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT)); + if already_hinted { + return notes; + } + let Some(log_path) = log_path else { + return notes; + }; + // Read the same tail budget the engine's own OOM surfaces use + // (`rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES`) so the CLI serve summary + // and the engine agree on what counts as "the tail" of a failed launch, + // rather than drifting apart on independent magic literals. + let log_tail = read_optional_tail_lines( + log_path, + rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES, + "service log", + ) + .join("\n"); + if let Some(note) = serve_summary::oom_memory_note(status, &log_tail) { + notes.push(note); + } + notes +} + fn validate_bind_host(host: &str, allow_public_bind: bool) -> Result<()> { if !is_loopback_host(host) && !allow_public_bind { bail!( @@ -5851,6 +6042,22 @@ fn start_managed_service( on_wait_tick: &mut dyn FnMut(Duration), ) -> Result { let paths = AppPaths::discover()?; + #[cfg(feature = "e2e-oom-fault-injection")] + if e2e_simulate_oom_launch() { + return simulate_oom_managed_launch( + &paths, + engine, + service_id, + requested_model, + resolve, + host, + port, + device_policy, + gpu_indices, + runtime_id, + env_id, + ); + } let (mut record, child_pid) = match spawn_managed_engine_child( &paths, engine, @@ -16394,6 +16601,19 @@ fn existing_live_managed_service( }) } +/// Whether any managed service for `engine` is currently live, without needing a +/// canonical model id. Used as a cheap pre-gate before resolving a model purely +/// to detect a reusable already-running service: when no live service for the +/// engine exists, the reuse check (and its `ResolveModel` round-trip) is skipped +/// and the normal launch pre-flight runs unchanged. +fn any_live_managed_service_for_engine(paths: &AppPaths, engine: &str) -> bool { + load_managed_services(paths).is_ok_and(|records| { + records + .iter() + .any(|record| record.engine == engine && managed_service_is_live(record)) + }) +} + fn managed_service_running_state(status: &str) -> &'static str { match status { "ready" | "running" => "running", @@ -25570,6 +25790,177 @@ install therock"; ); } + #[test] + fn append_oom_serve_note_reads_the_failed_serve_log_and_names_the_knobs() { + // A managed vLLM serve that died on VRAM lands not-ready with the OOM + // traceback only in its log; the note must be recovered from that log. + let log_path = + std::env::temp_dir().join(format!("rocm-oom-note-{}-hit.log", std::process::id())); + fs::write( + &log_path, + "loading weights\ntorch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .expect("write log"); + let notes = append_oom_serve_note(Vec::new(), true, false, "starting", Some(&log_path)); + let _ = fs::remove_file(&log_path); + assert!( + notes + .iter() + .any(|entry| entry.contains("--gpu-memory-utilization") + && entry.contains("--gpu ")), + "the OOM note must name both memory knobs: {notes:?}" + ); + } + + #[test] + fn append_oom_serve_note_is_withheld_when_it_does_not_apply() { + let log_path = + std::env::temp_dir().join(format!("rocm-oom-note-{}-miss.log", std::process::id())); + fs::write(&log_path, "torch.OutOfMemoryError: HIP out of memory\n").expect("write log"); + + // A ready serve is healthy even if the log mentions memory. + assert!( + append_oom_serve_note(Vec::new(), true, false, "ready", Some(&log_path)).is_empty() + ); + // A non-vLLM engine has no `--gpu-memory-utilization` to reach for. + assert!( + append_oom_serve_note(Vec::new(), false, false, "starting", Some(&log_path)).is_empty() + ); + // A missing log gives no signal to branch on. + assert!(append_oom_serve_note(Vec::new(), true, false, "starting", None).is_empty()); + + // A failure whose log carries no OOM signature is left alone. + fs::write(&log_path, "OSError: model weights not found\n").expect("rewrite log"); + assert!( + append_oom_serve_note(Vec::new(), true, false, "starting", Some(&log_path)).is_empty() + ); + let _ = fs::remove_file(&log_path); + } + + #[test] + fn append_oom_serve_note_is_withheld_for_a_healthy_still_loading_service() { + // `running` means the endpoint is up and the model is still loading + // normally — a healthy state deliberately kept distinct from `starting` + // so `rocmd` does not restart a slow-loading model. An OOM string left in + // its log tail (e.g. from an earlier probe) must NOT be reported as this + // serve having "run out of GPU memory". + let log_path = + std::env::temp_dir().join(format!("rocm-oom-note-{}-running.log", std::process::id())); + fs::write( + &log_path, + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .expect("write log"); + let notes = append_oom_serve_note(Vec::new(), true, false, "running", Some(&log_path)); + let _ = fs::remove_file(&log_path); + assert!( + notes.is_empty(), + "a healthy still-loading `running` service must not get the OOM note: {notes:?}" + ); + } + + #[test] + fn append_oom_serve_note_ignores_an_already_running_services_log() { + // Re-issuing `serve` against a service another invocation already + // launched must never blame *this* invocation for that other process's + // failure — even when its log carries a real OOM signature and the + // reused record's status is not yet "ready". + let log_path = std::env::temp_dir().join(format!( + "rocm-oom-note-{}-already-running.log", + std::process::id() + )); + fs::write( + &log_path, + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .expect("write log"); + let notes = append_oom_serve_note(Vec::new(), true, true, "starting", Some(&log_path)); + let _ = fs::remove_file(&log_path); + assert!( + notes.is_empty(), + "an already-running service's log must not be attributed to this invocation: {notes:?}" + ); + } + + #[test] + fn append_oom_serve_note_does_not_repeat_a_hint_already_in_the_notes() { + // When the pre-launch low-VRAM warning already carried the shared + // utilization hint, a post-failure OOM confirmation must not print the + // exact same hint text a second time. + let log_path = + std::env::temp_dir().join(format!("rocm-oom-note-{}-dedup.log", std::process::id())); + fs::write( + &log_path, + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .expect("write log"); + let pre_launch_notes = vec![ + "selected GPU 0 has low free VRAM".to_owned(), + rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT.to_owned(), + ]; + let notes = append_oom_serve_note( + pre_launch_notes.clone(), + true, + false, + "starting", + Some(&log_path), + ); + let _ = fs::remove_file(&log_path); + assert_eq!( + notes, pre_launch_notes, + "the hint must not be duplicated once it is already present: {notes:?}" + ); + } + + #[test] + fn append_oom_serve_note_reads_the_shared_engine_tail_budget() { + // The serve summary must read the same tail budget the engine's own OOM + // surfaces use (`rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES`) so the two + // surfaces never disagree on what counts as "the tail" of a failed + // launch. Plant the OOM signature on the oldest line still inside that + // window (it must fire), then push it one line past the window (it must + // fall silent) — pinning the read to the shared constant rather than a + // drifting literal. + let budget = rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES; + let oom_line = "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB."; + + // OOM on the oldest line that still fits in the last `budget` lines. + let in_window = std::iter::once(oom_line) + .chain(std::iter::repeat_n("shutting down worker", budget - 1)) + .collect::>() + .join("\n"); + let log_path = std::env::temp_dir().join(format!( + "rocm-oom-note-{}-in-window.log", + std::process::id() + )); + fs::write(&log_path, format!("{in_window}\n")).expect("write log"); + let notes = append_oom_serve_note(Vec::new(), true, false, "starting", Some(&log_path)); + let _ = fs::remove_file(&log_path); + assert!( + notes + .iter() + .any(|entry| entry.contains("--gpu-memory-utilization")), + "an OOM line inside the shared tail budget must still surface the note: {notes:?}" + ); + + // Push the OOM line one row past the window: it must scroll out of view. + let out_of_window = std::iter::once(oom_line) + .chain(std::iter::repeat_n("shutting down worker", budget)) + .collect::>() + .join("\n"); + let log_path = std::env::temp_dir().join(format!( + "rocm-oom-note-{}-out-window.log", + std::process::id() + )); + fs::write(&log_path, format!("{out_of_window}\n")).expect("write log"); + let notes = append_oom_serve_note(Vec::new(), true, false, "starting", Some(&log_path)); + let _ = fs::remove_file(&log_path); + assert!( + notes.is_empty(), + "an OOM line beyond the shared tail budget must not surface the note: {notes:?}" + ); + } + #[test] fn generation_defaults_inject_override_generation_config_for_vllm() { // vLLM has no raw sampling flags: all three controls collapse into a single diff --git a/apps/rocm/src/serve_summary.rs b/apps/rocm/src/serve_summary.rs index f12e8279f..4750fadb4 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -188,6 +188,39 @@ pub(crate) fn render_summary(summary: &DeploymentSummary) -> String { out } +/// Whether a managed service's recorded status means it *failed to become +/// ready* — the only state the post-failure OOM note should fire on. +/// +/// The status vocabulary from `status_for_readiness` is `ready` (serving), +/// `running` (endpoint up, model still loading — healthy, deliberately kept +/// distinct from `starting` so `rocmd` does not restart a slow-loading model), +/// and `starting` (endpoint never came up). Only `starting` is a failure; a +/// bare `!= "ready"` check would wrongly treat a healthy still-loading `running` +/// service as a failed launch. +pub(crate) fn serve_failed_to_become_ready(status: &str) -> bool { + status == "starting" +} + +/// Builds the actionable memory-knob note for a serve that failed to become +/// ready with an out-of-memory signature in its engine log. Returns `None` when +/// the serve became ready or the log carries no OOM signature, so healthy +/// deployments and unrelated failures are never cluttered with memory advice. +/// +/// The note names `--gpu-memory-utilization` and `--gpu` and is worded for the +/// shared-node case rather than as unconditional advice — vLLM reserves a +/// fraction of *total* VRAM, so a value good for a shared card would degrade a +/// dedicated one. It shares [`rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT`] with +/// the pre-launch low-VRAM note so both surfaces point at the same fix. +pub(crate) fn oom_memory_note(status: &str, log_tail: &str) -> Option { + if !serve_failed_to_become_ready(status) || !rocm_core::vllm_log_shows_oom(log_tail) { + return None; + } + Some(format!( + "the serve attempt ran out of GPU memory. {}", + rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT + )) +} + /// Run a single small chat completion against the just-started local server and /// measure time-to-first-token and generation throughput. Best-effort: any error /// (server not OpenAI-compatible, refused, timed out) yields empty metrics rather @@ -486,6 +519,71 @@ mod tests { assert!(render_summary(&summary).contains("note: selected GPU 0 has low free VRAM")); } + #[test] + fn oom_signatures_are_detected_case_insensitively() { + // The signatures the ticket calls out, plus casing variants the engine + // log can emit. + assert!(rocm_core::vllm_log_shows_oom( + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." + )); + assert!(rocm_core::vllm_log_shows_oom("HIP OUT OF MEMORY")); + assert!(rocm_core::vllm_log_shows_oom( + "RuntimeError: CUDA out of memory" + )); + } + + #[test] + fn unrelated_failures_are_not_flagged_as_oom() { + assert!(!rocm_core::vllm_log_shows_oom( + "OSError: model weights not found; check the model id" + )); + assert!(!rocm_core::vllm_log_shows_oom("")); + // vLLM's generic EngineCore wrapper is the terminal line for *any* + // startup crash, not just OOM; treating it as an OOM signature would + // misreport unrelated failures as memory exhaustion. + assert!(!rocm_core::vllm_log_shows_oom( + "ERROR Engine core initialization failed" + )); + } + + #[test] + fn oom_note_names_both_memory_knobs_on_a_failed_serve() { + let note = oom_memory_note( + "starting", + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ) + .expect("an OOM failure must produce a note"); + assert!(note.contains("--gpu-memory-utilization"), "{note}"); + assert!(note.contains("--gpu "), "{note}"); + assert!(note.contains("ran out of GPU memory"), "{note}"); + } + + #[test] + fn oom_note_is_withheld_for_a_ready_serve_or_a_clean_log() { + // A serve that became ready is healthy even if the log mentions memory. + assert_eq!( + oom_memory_note("ready", "torch.OutOfMemoryError: HIP out of memory"), + None + ); + // A failure with no OOM signature must not be given memory advice. + assert_eq!( + oom_memory_note("starting", "OSError: model weights not found"), + None + ); + } + + #[test] + fn oom_note_renders_in_the_summary_notes() { + let mut summary = base_summary(); + summary.status = "starting".to_owned(); + if let Some(note) = oom_memory_note(&summary.status, "torch.OutOfMemoryError") { + summary.notes.push(note); + } + let rendered = render_summary(&summary); + assert!(rendered.contains("note: the serve attempt ran out of GPU memory")); + assert!(rendered.contains("--gpu-memory-utilization")); + } + #[test] fn api_key_client_config_shown_only_when_present() { // Loopback / no key: the summary must not mention an api key at all. diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c80d4309a..f13800b32 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -7402,6 +7402,30 @@ pub const VLLM_GPU_MEMORY_UTILIZATION_HINT: &str = "vLLM reserves ~90% of the GP collide with memory already in use. Lower the reservation with `--gpu-memory-utilization \ <0-1>` (e.g. 0.1 for a small model), or target a less-busy GPU with `--gpu `."; +/// Whether a vLLM/PyTorch log excerpt carries a startup out-of-memory +/// signature. +/// +/// Deliberately excludes vLLM's generic "engine core initialization failed" +/// wrapper: vLLM emits that line as the terminal message for *any* EngineCore +/// startup crash (unsupported architecture, shm size, tensor-parallel +/// misconfiguration, missing weights, and OOM alike), and being the last line +/// it reliably lands in a truncated log tail — treating it as an OOM signature +/// would misreport unrelated startup failures as memory exhaustion. +/// +/// The two signatures are complementary, not redundant: `outofmemory` (the +/// space-free form, matched after lowercasing) catches the allocator error +/// types whichever module path they carry — `torch.OutOfMemoryError`, +/// `torch.cuda.OutOfMemoryError`, `hipErrorOutOfMemory` — while `out of memory` +/// catches the spaced runtime phrasing (`HIP out of memory`, `CUDA out of +/// memory`). An earlier `hip out of memory` entry was dead code, since any log +/// containing it already contains `out of memory`. +pub fn vllm_log_shows_oom(log: &str) -> bool { + const SIGNATURES: &[&str] = &["outofmemory", "out of memory"]; + + let lower = log.to_ascii_lowercase(); + SIGNATURES.iter().any(|signature| lower.contains(signature)) +} + /// Locate `amd-smi` inside the bin directories of the newest managed ROCm SDK /// runtime recorded in the registry. The binary ships with the TheRock wheel /// (under the SDK `bin_path` and/or the venv `install_root/bin`) and is not on @@ -11800,6 +11824,28 @@ last_installed_runtime_id = "therock-release" ); } + #[test] + fn vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures() { + // Space-free allocator error types, whatever module path they carry. + assert!(vllm_log_shows_oom( + "torch.OutOfMemoryError: HIP out of memory." + )); + assert!(vllm_log_shows_oom( + "raise torch.cuda.OutOfMemoryError(msg) # CUDA path" + )); + assert!(vllm_log_shows_oom("RuntimeError: hipErrorOutOfMemory")); + // Spaced runtime phrasing on its own (no `OutOfMemoryError` token). + assert!(vllm_log_shows_oom("HIP error: out of memory")); + // Case-insensitive. + assert!(vllm_log_shows_oom("TORCH.OUTOFMEMORYERROR")); + // The generic EngineCore wrapper is NOT an OOM signature. + assert!(!vllm_log_shows_oom( + "EngineCore failed: engine core initialization failed" + )); + assert!(!vllm_log_shows_oom("OSError: model weights not found")); + assert!(!vllm_log_shows_oom("")); + } + #[test] fn combine_amd_gpu_counts_prefers_compute_authoritative_kfd() { // KFD is compute-authoritative: a nonzero KFD count wins, and DRM must not diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index ae67a8dac..900c9fb09 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -2227,7 +2227,7 @@ fn startup_log_context(log_path: Option<&Path>) -> String { /// with the `rocm` CLI's pre-launch low-VRAM note so both surfaces point the /// user at the same fix rather than drifting into different phrasing. fn oom_utilization_hint(log_tail: &str) -> String { - if !log_tail_shows_oom(log_tail) { + if !rocm_core::vllm_log_shows_oom(log_tail) { return String::new(); } // Route the user's *actual* failing line into the `--symptom` example when @@ -2241,7 +2241,7 @@ fn oom_utilization_hint(log_tail: &str) -> String { .lines() .rev() .map(str::trim) - .find(|line| !line.is_empty() && log_tail_shows_oom(line)) + .find(|line| !line.is_empty() && rocm_core::vllm_log_shows_oom(line)) .unwrap_or("out of memory"); let candidate = format!("vllm: {symptom_line}"); let symptom = if rocm_core::vllm_oom_symptom_is_diagnosable(&candidate) { @@ -2256,13 +2256,6 @@ fn oom_utilization_hint(log_tail: &str) -> String { ) } -/// Case-insensitive scan for the out-of-memory signatures vLLM/PyTorch emit on a -/// HIP allocation failure (e.g. `torch.OutOfMemoryError: HIP out of memory`). -fn log_tail_shows_oom(log_tail: &str) -> bool { - let lower = log_tail.to_ascii_lowercase(); - lower.contains("out of memory") || lower.contains("outofmemory") -} - /// Polls the vLLM endpoint until it reports the model is loaded, or times out. /// Uses a monotonic clock (`Instant`) so wall-clock adjustments cannot corrupt the /// timeout, and surfaces an early process exit immediately instead of waiting out @@ -2728,7 +2721,7 @@ mod tests { fn oom_utilization_hint_fires_on_out_of_memory_log_tails() { // The exact PyTorch/HIP signature from the field report. let torch = "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB."; - assert!(log_tail_shows_oom(torch)); + assert!(rocm_core::vllm_log_shows_oom(torch)); let hint = oom_utilization_hint(torch); assert!( hint.contains("--gpu-memory-utilization"), @@ -2747,8 +2740,10 @@ mod tests { ); // Detection is case-insensitive and also matches the spaced phrasing. - assert!(log_tail_shows_oom("HIP OUT OF MEMORY")); - assert!(log_tail_shows_oom("RuntimeError: CUDA out of memory")); + assert!(rocm_core::vllm_log_shows_oom("HIP OUT OF MEMORY")); + assert!(rocm_core::vllm_log_shows_oom( + "RuntimeError: CUDA out of memory" + )); } #[test] @@ -2784,11 +2779,22 @@ mod tests { #[test] fn oom_utilization_hint_stays_quiet_for_unrelated_failures() { let unrelated = "ValueError: model architecture 'FooForCausalLM' is not supported"; - assert!(!log_tail_shows_oom(unrelated)); + assert!(!rocm_core::vllm_log_shows_oom(unrelated)); assert!( oom_utilization_hint(unrelated).is_empty(), "non-OOM failures must not carry a memory hint" ); + + // vLLM's generic EngineCore wrapper is the terminal line for *any* + // startup crash (unsupported arch, shm size, TP misconfig, missing + // weights, OOM, ...); treating it as an OOM signature would misreport + // those unrelated failures as memory exhaustion. + let wrapper_only = "ERROR Engine core initialization failed"; + assert!(!rocm_core::vllm_log_shows_oom(wrapper_only)); + assert!( + oom_utilization_hint(wrapper_only).is_empty(), + "the generic EngineCore wrapper alone must not be treated as OOM" + ); } #[test] diff --git a/tests/e2e-cucumber/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index e33ca52b3..2af6c1dd0 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -203,3 +203,37 @@ Feature: Model serving When the user previews a vLLM serve plan pinned to that GPU Then the serve plan warns the GPU is low on VRAM And the serve plan explains how to lower vLLM's memory reservation + + # Regression test (EAI-8059 review): re-issuing `serve` against an + # already-running managed service must never blame *this* invocation for + # whatever that other process's log contains, even when it carries a real + # OOM signature and the reused record is not yet "ready" — nothing was + # launched by this invocation, so there is nothing for it to have OOM'd on. + # Runs on the no-GPU mock host: reusing an already-running managed service + # launches nothing and pins no GPU, so `rocm serve` bypasses the GPU-required + # pre-flight and reaches the reuse short-circuit even here. It therefore gates + # every PR and allocates no GPU memory. + @id:serve-oom-memory-guidance @requires-no-gpu @requires-os:linux + Scenario: serve-20 - Reusing an already-running serve never blames it for another process's OOM + Given a live managed vLLM serve has an OOM startup log + When the user opens its interactive serve summary + Then the summary reflects the reused already-running service + And the deployment summary does not blame this invocation for GPU memory + + # Positive counterpart of Scenario 20 (EAI-8059 review asked for both): when + # THIS invocation launches a managed vLLM serve that OOMs and never becomes + # ready, the summary MUST name the memory knobs. A real launch cannot run on a + # GPU-less host, so the mock lane build compiles in a test-only fault-injection + # hook (feature `e2e-oom-fault-injection`, armed by `ROCM_E2E_SIMULATE_OOM_LAUNCH`) + # that fabricates exactly that failed-launch state — an owned log carrying a real + # allocator OOM signature, status "starting", nothing reused. The hook is absent + # from shipped binaries, so @requires-oom-fault-injection skips this on the + # self-hosted lanes (prebuilt release binary) and it runs on the mock lane, + # where xtask builds the binary with the feature. @requires-no-gpu because the + # fabricated launch stands in for the GPU the mock lane does not have. + @id:serve-oom-launch-memory-guidance @requires-no-gpu @requires-os:linux @requires-oom-fault-injection + Scenario: serve-21 - A launch that runs out of GPU memory names the memory knobs + Given a managed vLLM launch will run out of GPU memory + When the user opens the interactive serve summary for that launch + Then the deployment summary blames this launch for GPU memory + And the deployment summary names the GPU memory knobs diff --git a/tests/e2e-cucumber/src/capability.rs b/tests/e2e-cucumber/src/capability.rs index de2197cff..854dc0777 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -115,8 +115,22 @@ pub struct HostCapability { /// Stable platform identity derived from hardware, not from an artifact name: /// "mock" (no AMD GPU), else the family/target (e.g. "mi300x", "strix-halo"). pub platform_slug: String, + /// Whether the `rocm` binary under test carries the test-only + /// `e2e-oom-fault-injection` hook, so `@requires-oom-fault-injection` + /// scenarios can simulate a GPU-less OOM launch. It is compiled out of the + /// shipping binary, and the harness cannot probe for it (no product command + /// exposes it), so `xtask e2e` reports it via the [`OOM_FAULT_INJECTION_ENV`] + /// environment variable — set only when xtask built the binary itself with + /// the feature, and absent for a prebuilt `ROCM_CLI_BINARY` (the self-hosted + /// lanes' shipping release build). See [`probe_host_capability`]. + pub oom_fault_injection: bool, } +/// Environment variable `xtask e2e` sets to `1` when it compiled the binary +/// under test with the `rocm/e2e-oom-fault-injection` feature. Kept in sync with +/// the same name in `xtask::e2e`. +pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; + impl HostCapability { /// Whether a given engine can actually START on this host. Distinct from /// "adapter present": vLLM's adapter is built-in everywhere but cannot run on @@ -339,6 +353,12 @@ fn probe_host_capability() -> HostCapability { let effective_serve_engine = effective_serve_engine(gfx_target.as_deref(), &os_family); let platform_slug = derive_platform_slug(has_amd_gpu, gfx_target.as_deref(), &os_family, is_wsl); + // The fault-injection hook is a compile-time feature the harness can't probe + // for, so trust the signal `xtask e2e` sets only when it built the binary + // with that feature. Absent for a prebuilt `ROCM_CLI_BINARY` (shipping + // release build), so those runs skip `@requires-oom-fault-injection`. + let oom_fault_injection = + std::env::var_os(OOM_FAULT_INJECTION_ENV).is_some_and(|value| value == "1"); HostCapability { os_family, @@ -348,6 +368,7 @@ fn probe_host_capability() -> HostCapability { available_engines, effective_serve_engine, platform_slug, + oom_fault_injection, } } @@ -600,6 +621,7 @@ mod tests { available_engines: vec!["lemonade".to_owned(), "vllm".to_owned()], effective_serve_engine: "lemonade".to_owned(), platform_slug: "strix-halo".to_owned(), + oom_fault_injection: false, }; assert!(strix.engine_available("lemonade")); // vLLM adapter is "built-in" but cannot start on Windows / non-dcgpu. @@ -613,6 +635,7 @@ mod tests { available_engines: vec!["lemonade".to_owned(), "vllm".to_owned()], effective_serve_engine: "vllm".to_owned(), platform_slug: "mi300x".to_owned(), + oom_fault_injection: false, }; assert!(mi300x.engine_available("vllm")); assert!(mi300x.engine_available("lemonade")); diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 2eaa60090..6416acc53 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -28,6 +28,7 @@ const REQUIRES_GPU_TAG: &str = "requires-gpu"; const REQUIRES_NO_GPU_TAG: &str = "requires-no-gpu"; const REQUIRES_BARE_METAL_TAG: &str = "requires-bare-metal"; const REQUIRES_WSL_TAG: &str = "requires-wsl"; +const REQUIRES_OOM_FAULT_INJECTION_TAG: &str = "requires-oom-fault-injection"; const SERVE_TIMEOUT_PREFIX: &str = "serve-timeout:"; const NIGHTLY_TAG: &str = "nightly"; const LIFECYCLE_TAG: &str = "lifecycle"; @@ -85,6 +86,16 @@ pub struct ScenarioDecl { /// host, so it is skipped on native Linux, native Windows and everything /// else. Same reason `@requires-os:linux` cannot stand in for it. pub requires_wsl: bool, + /// `@requires-oom-fault-injection`: the scenario drives the test-only + /// `e2e-oom-fault-injection` hook (armed by `ROCM_E2E_SIMULATE_OOM_LAUNCH`) + /// to fabricate a GPU-less OOM launch, so it can only run against a binary + /// compiled with that feature. `xtask e2e` builds it in when it builds the + /// binary itself (the GitHub-hosted mock lane), but the self-hosted lanes + /// test a prebuilt shipping release binary that has the hook compiled out — + /// there the real GPU-required serve pre-flight would run instead. Skipped + /// when [`HostCapability::oom_fault_injection`] is false so those lanes do + /// not report the missing hook as a regression. + pub requires_oom_fault_injection: bool, /// Engine the scenario pins via `@requires-engine:` (if any). pub requires_engine: Option, /// OS the scenario requires via `@requires-os:` (e.g. "linux"), if any — @@ -123,6 +134,7 @@ impl ScenarioDecl { let mut requires_no_gpu = false; let mut requires_bare_metal = false; let mut requires_wsl = false; + let mut requires_oom_fault_injection = false; let mut requires_engine = None; let mut requires_os = None; let mut serve_timeout_secs = None; @@ -150,6 +162,8 @@ impl ScenarioDecl { requires_bare_metal = true; } else if tag == REQUIRES_WSL_TAG { requires_wsl = true; + } else if tag == REQUIRES_OOM_FAULT_INJECTION_TAG { + requires_oom_fault_injection = true; } else if tag == NIGHTLY_TAG { nightly = true; } else if tag == LIFECYCLE_TAG { @@ -164,6 +178,7 @@ impl ScenarioDecl { requires_no_gpu, requires_bare_metal, requires_wsl, + requires_oom_fault_injection, requires_engine, requires_os, serve_timeout_secs, @@ -444,6 +459,13 @@ pub fn resolve( reason: "requires WSL; this host is not running under WSL".to_owned(), }; } + if decl.requires_oom_fault_injection && !cap.oom_fault_injection { + return Expectation::Skip { + reason: "requires the e2e-oom-fault-injection build; this binary has \ + the hook compiled out" + .to_owned(), + }; + } if let Some(os) = &decl.requires_os && !os.eq_ignore_ascii_case(&cap.os_family) { @@ -529,6 +551,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "vllm".into(), platform_slug: "mi300x".into(), + oom_fault_injection: false, }, "strix-ubuntu" => HostCapability { os_family: "linux".into(), @@ -538,6 +561,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "strix-halo".into(), + oom_fault_injection: false, }, "strix-windows" => HostCapability { os_family: "windows".into(), @@ -547,6 +571,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "strix-halo".into(), + oom_fault_injection: false, }, // A WSL2 dev box with the ROCm passthrough in place: `os_family` is // `linux` and a GPU is usable, so nothing but `is_wsl` distinguishes @@ -560,6 +585,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "strix-halo-wsl".into(), + oom_fault_injection: false, }, // The same box without the passthrough: the gfx target is reported // by the Windows-side driver but ROCm cannot reach it, so the probe @@ -572,6 +598,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "wsl".into(), + oom_fault_injection: false, }, // The hosted WSL runner before the ROCm passthrough is complete: // Windows still reports the gfx target, but the CLI cannot use it. @@ -583,6 +610,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "strix-halo-wsl".into(), + oom_fault_injection: false, }, _ => HostCapability { os_family: "other".into(), @@ -592,6 +620,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "mock".into(), + oom_fault_injection: false, }, } } @@ -843,6 +872,36 @@ serve_timeout_secs = 90 } } + /// The fault-injection gate keys on the *binary's* build, not the host, so + /// the same mock lane runs it or skips it depending only on whether the hook + /// was compiled in. Proving both directions keeps a feature-less prebuilt + /// binary (the self-hosted lanes) from reporting the missing hook as a bug. + #[test] + fn requires_oom_fault_injection_runs_only_when_the_hook_is_built_in() { + let m = Expectations::default(); + let d = decl(&[ + "id:serve-oom-launch-memory-guidance", + "requires-oom-fault-injection", + ]); + assert!(d.requires_oom_fault_injection); + + // Binary built without the feature (default fixtures) — skip. + let without_hook = cap("mock"); + assert!(!without_hook.oom_fault_injection); + assert!(matches!( + resolve(&d, &without_hook, &m, false, false, false), + Expectation::Skip { .. } + )); + + // Same host, but the binary carries the hook — run. + let mut with_hook = cap("mock"); + with_hook.oom_fault_injection = true; + assert_eq!( + resolve(&d, &with_hook, &m, false, false, false), + Expectation::ExpectPass + ); + } + #[test] fn requires_os_linux_does_not_stand_in_for_requires_bare_metal() { // The reason the tag has to exist: WSL2 reports os_family "linux", so an diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index d03af837d..868fa56d4 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -9,11 +9,18 @@ use std::time::{Duration, Instant}; use cucumber::{given, then, when}; use crate::E2eWorld; -use e2e_cucumber::mock_server::MockServer; +use crate::e2e::tui_driver::TuiSession; +use e2e_cucumber::mock_server::{MockServer, ServiceRecordOptions, write_service_record_with}; use e2e_cucumber::serve_log::{ ServeAttempt, archive_service_log, serve_attempt_report, service_log_tail, }; +const OOM_GUIDANCE_MODEL: &str = "e2e/oom-model"; +const INTERACTIVE_SUMMARY_TIMEOUT: Duration = Duration::from_secs(30); +/// Model id for the positive OOM-launch scenario. A distinct id from +/// [`OOM_GUIDANCE_MODEL`] keeps the two OOM scenarios' service records from ever +/// colliding, and marks this one as the launch (not reuse) case. +const OOM_LAUNCH_MODEL: &str = "e2e/oom-launch-model"; /// How long to wait for a freshly served model's endpoint to become ready. /// /// On real GPU hardware the first serve of a model downloads its weights and @@ -1004,6 +1011,59 @@ async fn assert_selector_conflict_message(world: &mut E2eWorld) { ); } +#[given("a live managed vLLM serve has an OOM startup log")] +async fn plant_oom_managed_serve(world: &mut E2eWorld) { + let root = world.isolated_root.as_ref().expect("no isolated root"); + let services = root.path().join("data").join("services"); + write_service_record_with( + &services, + OOM_GUIDANCE_MODEL, + 65_534, + ServiceRecordOptions { + status: "starting", + startup_phase: Some("initializing"), + supervisor_pid: std::process::id(), + engine_pid: Some(std::process::id()), + }, + ); + // A real allocator OOM signature (not vLLM's generic EngineCore wrapper, + // which is deliberately excluded from detection — see EAI-8059 review). This + // log belongs to whatever process is already live, not to the invocation + // under test in this scenario. + std::fs::write( + services.join("e2e-mock.log"), + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + ) + .expect("failed to plant the OOM startup log"); +} + +#[when("the user opens its interactive serve summary")] +async fn open_oom_serve_summary(world: &mut E2eWorld) { + // The synthetic env selection satisfies resolution without installing a + // runtime. The planted live service is reused rather than launched, so + // `rocm serve` bypasses the GPU-required pre-flight (a reuse pins no GPU) and + // reaches the reuse short-circuit on the no-GPU mock host, allocating no GPU + // memory. + let mut session = TuiSession::spawn( + world, + &[ + "serve", + OOM_GUIDANCE_MODEL, + "--engine", + "vllm", + "--env-id", + "e2e-oom", + ], + ) + .unwrap_or_else(|error| panic!("failed to open interactive serve summary: {error}")); + session + .wait_for_exit(INTERACTIVE_SUMMARY_TIMEOUT) + .await + .unwrap_or_else(|error| panic!("interactive serve summary failed: {error}")); + world.tui = Some(session); +} +} + #[when("the CLI reports the service as ready")] async fn when_cli_reports_ready(world: &mut E2eWorld) { // Read readiness from the CLI's own view (`services list`), not a direct @@ -1130,6 +1190,112 @@ async fn assert_absent_index_message(world: &mut E2eWorld) { ); } +#[then("the summary reflects the reused already-running service")] +async fn assert_reused_running_service(world: &mut E2eWorld) { + let screen = world + .tui + .as_ref() + .expect("no interactive serve summary") + .screen_text(); + assert!( + screen.contains("already running"), + "expected the summary to reflect the reused live service:\n{screen}" + ); +} + +#[then("the deployment summary does not blame this invocation for GPU memory")] +async fn assert_no_oom_memory_guidance(world: &mut E2eWorld) { + let screen = world + .tui + .as_ref() + .expect("no interactive serve summary") + .screen_text(); + assert!( + !screen.contains("ran out of GPU memory"), + "reusing an already-running service must not blame this invocation for \ + another process's OOM:\n{screen}" + ); +} + +#[given("a managed vLLM launch will run out of GPU memory")] +async fn plant_oom_launch(_world: &mut E2eWorld) { + // Nothing to plant on disk: unlike the reuse case, this scenario drives a + // real `rocm serve` launch. The launch's failed-with-OOM state (an owned log + // carrying a real allocator signature, status "starting", nothing reused) is + // fabricated inside the CLI by the test-only `e2e-oom-fault-injection` hook, + // armed per-invocation by the env var set in the next step. This step exists + // to state the scenario's premise. +} + +#[when("the user opens the interactive serve summary for that launch")] +async fn open_oom_launch_summary(world: &mut E2eWorld) { + // Arm the CLI's test-only OOM-launch fault injection for this child only (the + // mock-lane binary is built with `e2e-oom-fault-injection`). The GPU-required + // pre-flight is bypassed for the simulated launch, so no GPU is touched; the + // launch resolves the model, "spawns", writes an OOM log it owns, and reports + // `starting` — exactly the state the memory-guidance note keys on. + let mut session = TuiSession::spawn_with_env( + world, + &[ + "serve", + OOM_LAUNCH_MODEL, + "--engine", + "vllm", + "--env-id", + "e2e-oom-launch", + ], + &[("ROCM_E2E_SIMULATE_OOM_LAUNCH", "1")], + ) + .unwrap_or_else(|error| panic!("failed to open interactive serve summary: {error}")); + session + .wait_for_exit(INTERACTIVE_SUMMARY_TIMEOUT) + .await + .unwrap_or_else(|error| panic!("interactive serve summary failed: {error}")); + world.tui = Some(session); +} + +#[then("the deployment summary blames this launch for GPU memory")] +async fn assert_oom_launch_memory_guidance(world: &mut E2eWorld) { + let screen = world + .tui + .as_ref() + .expect("no interactive serve summary") + .screen_text(); + // The note is a long line the 80-column PTY wraps across grid rows, so match + // whitespace-insensitively — otherwise a soft wrap between "GPU" and "memory" + // would break a literal `contains` and mask a rendered note. + assert!( + screen_without_whitespace(&screen).contains("ranoutofGPUmemory"), + "a managed launch that OOM'd must be blamed for GPU memory in its own \ + deployment summary:\n{screen}" + ); +} + +#[then("the deployment summary names the GPU memory knobs")] +async fn assert_oom_launch_names_knobs(world: &mut E2eWorld) { + let screen = world + .tui + .as_ref() + .expect("no interactive serve summary") + .screen_text(); + // The actionable fix: lower the reservation or move to a less-busy device. + // Assert on the flag name (whitespace-insensitive, since an 80-column wrap can + // split the token across rows) so a reworded preamble does not mask a dropped + // remediation. + assert!( + screen_without_whitespace(&screen).contains("--gpu-memory-utilization"), + "the OOM summary must name the memory knob `--gpu-memory-utilization`:\n{screen}" + ); +} + +/// Collapse a rendered PTY screen to its non-whitespace characters, so an +/// assertion survives the terminal soft-wrapping a long line across grid rows +/// (which inserts row breaks mid-phrase / mid-token). Only meaningful for +/// needles that themselves contain no whitespace once collapsed. +fn screen_without_whitespace(screen: &str) -> String { + screen.chars().filter(|c| !c.is_whitespace()).collect() +} + #[then("the output shows the full model name")] async fn assert_full_model_name(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no CLI output"); diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 3ec8d0d85..39535c365 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -126,6 +126,21 @@ impl TuiSession { Self::spawn_binary(world, crate::rocm_binary(), args) } + /// As [`spawn`](Self::spawn), but overlays `extra_env` onto the child only. + /// + /// Used by scenarios that must set a variable the CLI reads at startup (e.g. + /// the test-only `ROCM_E2E_SIMULATE_OOM_LAUNCH` fault-injection switch). The + /// vars are applied per-child on the `CommandBuilder`, never via the shared + /// process environment, so concurrent scenarios on the no-GPU lane cannot + /// observe each other's overrides. + pub fn spawn_with_env( + world: &E2eWorld, + args: &[&str], + extra_env: &[(&str, &str)], + ) -> Result { + Self::spawn_binary_with_env(world, crate::rocm_binary(), args, extra_env) + } + /// Spawn a specific `rocm` binary under a fresh PTY. /// /// Most scenarios use [`spawn`](Self::spawn) and exercise the harness-built @@ -135,6 +150,18 @@ impl TuiSession { world: &E2eWorld, binary: impl AsRef, args: &[&str], + ) -> Result { + Self::spawn_binary_with_env(world, binary, args, &[]) + } + + /// Backing implementation of [`spawn_binary`] / [`spawn_with_env`]: spawn + /// `binary ` under a PTY with the isolated environment, then overlay + /// `extra_env` on the child. + pub fn spawn_binary_with_env( + world: &E2eWorld, + binary: impl AsRef, + args: &[&str], + extra_env: &[(&str, &str)], ) -> Result { let pair = native_pty_system() .openpty(PtySize { @@ -177,6 +204,12 @@ impl TuiSession { cmd.env("COLUMNS", COLS.to_string()); cmd.env("LINES", ROWS.to_string()); + // Per-child overrides last, so a scenario's explicit variable wins over + // the inherited/isolation environment. + for (key, value) in extra_env { + cmd.env(key, value); + } + let mut child = pair .slave .spawn_command(cmd) diff --git a/xtask/src/e2e.rs b/xtask/src/e2e.rs index 05cf29bbb..971c8c00b 100644 --- a/xtask/src/e2e.rs +++ b/xtask/src/e2e.rs @@ -18,6 +18,13 @@ use anyhow::{Context, Result, bail}; use crate::paths::{binary_name, target_dir, workspace_root}; +/// Signal to the e2e-cucumber harness that the binary under test carries the +/// `rocm/e2e-oom-fault-injection` hook. Set to `1` only when this xtask built +/// the binary itself with that feature; a prebuilt `ROCM_CLI_BINARY` (the +/// self-hosted lanes' shipping release) has the hook compiled out. Kept in sync +/// with `e2e_cucumber::capability::OOM_FAULT_INJECTION_ENV`. +const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; + #[derive(Debug, PartialEq, Eq)] struct E2eBinaries { rocm: PathBuf, @@ -62,6 +69,15 @@ fn configure_harness_env(command: &mut Command, binaries: &E2eBinaries) { } else { command.env_remove("ROCM_CLI_ROCMD_BINARY"); } + // Only an xtask-built binary carries the fault-injection hook (see `run`); + // a prebuilt `ROCM_CLI_BINARY` does not, so clear any inherited value there + // rather than letting the harness run `@requires-oom-fault-injection` + // scenarios against a binary that has the hook compiled out. + if binaries.build_release { + command.env(OOM_FAULT_INJECTION_ENV, "1"); + } else { + command.env_remove(OOM_FAULT_INJECTION_ENV); + } } /// Build the release binaries and run the E2E suite, forwarding `args` to the @@ -85,6 +101,15 @@ pub fn run(args: &[String]) -> Result<()> { if binaries.build_release { let status = Command::new(&cargo) + // `--features rocm/e2e-oom-fault-injection` compiles in the test-only + // `rocm serve` OOM-launch fault injection used by the positive + // `serve-oom-launch-memory-guidance` scenario. It is scoped to the + // `rocm` package (rocmd has no such feature) and to this e2e build + // only — the release build never enables it, so shipped binaries + // carry no fault-injection hook. It joins `rocm/e2e-test-hooks` + // (the general e2e hook surface) in a single space-separated + // `--features` value; cargo takes one `--features` argument, so both + // must be listed together rather than in two overriding flags. .args([ "build", "--release", @@ -93,7 +118,7 @@ pub fn run(args: &[String]) -> Result<()> { "-p", "rocmd", "--features", - "rocm/e2e-test-hooks", + "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection", ]) .current_dir(&root) .status() @@ -170,6 +195,12 @@ mod tests { command_env(&command, "ROCM_CLI_ROCMD_BINARY"), binaries.rocmd.map(PathBuf::into_os_string) ); + // xtask built this binary with the fault-injection feature, so tell the + // harness the hook is present. + assert_eq!( + command_env(&command, "ROCM_E2E_OOM_FAULT_INJECTION"), + Some(OsString::from("1")) + ); } #[test] @@ -193,6 +224,9 @@ mod tests { command_env(&command, "ROCM_CLI_ROCMD_BINARY"), binaries.rocmd.map(PathBuf::into_os_string) ); + // A prebuilt binary has the fault-injection hook compiled out, so the + // signal must be cleared, not inherited from the ambient environment. + assert_eq!(command_env(&command, "ROCM_E2E_OOM_FAULT_INJECTION"), None); } #[test] From 7c091299960b8aaf5f74094ec5b0cc8b0d07565a Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 3 Sep 2026 09:37:59 +0000 Subject: [PATCH 02/11] =?UTF-8?q?serve:=20address=20EAI-8059=20re-review?= =?UTF-8?q?=20=E2=80=94=20validate=20--gpu=20before=20engine=20work,=20tig?= =?UTF-8?q?hten=20OOM=20negative=20assertion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review fixes on top of the squashed EAI-8059 change: - serve flow: move the pinned --gpu index validation (resolve_gpu_indices) back ahead of the no-runtime bail and ensure_self_managed_engine_ready, so a nonexistent --gpu index is rejected before any engine preparation begins. This restores the 'refused before any engine starts' contract that serve-absent-gpu-index-rejected asserts on the GPU lane; the reorder had pushed index validation after engine-ready work. It still sits after the no-usable-GPU pre-flight so a GPU-less host refuses first. - e2e: the negative OOM assertion used a literal contains("ran out of GPU memory") while the note wraps across the 80-column PTY grid, so it passed even if the note WERE wrongly rendered. Match whitespace-insensitively, exactly as the positive assertions do, so a mis-rendered note is caught. - vllm: fix a stale reference to the pre-rename log_tail_shows_oom in the emitted-symptom test; the detector now lives in rocm-core as vllm_log_shows_oom. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 20 +++++++++++++------ engines/vllm/src/lib.rs | 5 ++++- tests/e2e-cucumber/tests/e2e/serving_steps.rs | 8 ++++++-- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index b25c1a00b..365617357 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -5194,6 +5194,20 @@ fn serve(args: ServeArgs) -> Result<()> { policy = device_policy_name(&device_policy) ); } + // Validate the pinned `--gpu` index and resolve the GPU set BEFORE any + // runtime or engine work, so an index that does not exist on the host is + // rejected up front (`serve-absent-gpu-index-rejected`: "refused before any + // engine starts") rather than after `ensure_self_managed_engine_ready` has + // begun preparing the engine. It sits after the no-usable-GPU pre-flight so a + // GPU-less host still refuses with "no usable AMD GPU" before the index is + // ever inspected. A reuse pins no GPU, but validating the index here is + // side-effect-free and keeps a bad `--gpu` from being silently ignored. + let gpu_vram = if cpu_only { None } else { gpu_vram_usage() }; + let gpu_indices = if cpu_only { + Vec::new() + } else { + resolve_gpu_indices(&paths, &gpu_selection, gpu_vram.as_deref())? + }; if !matches!(device_policy, DevicePolicy::CpuOnly) && resolved_selection.runtime_id.is_none() && resolved_selection.env_id.is_none() @@ -5210,12 +5224,6 @@ fn serve(args: ServeArgs) -> Result<()> { { ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; } - let gpu_vram = if cpu_only { None } else { gpu_vram_usage() }; - let gpu_indices = if cpu_only { - Vec::new() - } else { - resolve_gpu_indices(&paths, &gpu_selection, gpu_vram.as_deref())? - }; let resolve = match resolved_model { Some(resolve) => resolve, None => engine_request::<_, ResolveModelResponse>( diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 900c9fb09..753eed08d 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -2762,7 +2762,10 @@ mod tests { "CUDA out of memory", ]; for line in accepted_lines { - assert!(log_tail_shows_oom(line), "detector must accept: {line}"); + assert!( + rocm_core::vllm_log_shows_oom(line), + "detector must accept: {line}" + ); let hint = oom_utilization_hint(line); let symptom = hint .split("--symptom '") diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index 868fa56d4..e87b801b8 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -1062,7 +1062,6 @@ async fn open_oom_serve_summary(world: &mut E2eWorld) { .unwrap_or_else(|error| panic!("interactive serve summary failed: {error}")); world.tui = Some(session); } -} #[when("the CLI reports the service as ready")] async fn when_cli_reports_ready(world: &mut E2eWorld) { @@ -1210,8 +1209,13 @@ async fn assert_no_oom_memory_guidance(world: &mut E2eWorld) { .as_ref() .expect("no interactive serve summary") .screen_text(); + // Match whitespace-insensitively, exactly as the positive assertions do: the + // note is a long line the 80-column PTY wraps across grid rows, so a literal + // `contains("ran out of GPU memory")` would never match the rendered note and + // this negative check would pass even if the note WERE wrongly shown. Collapse + // whitespace so a mis-rendered note is actually caught. assert!( - !screen.contains("ran out of GPU memory"), + !screen_without_whitespace(&screen).contains("ranoutofGPUmemory"), "reusing an already-running service must not blame this invocation for \ another process's OOM:\n{screen}" ); From 83ecfaad398245e4e073ac73510e184d1d248f18 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 8 Sep 2026 11:26:56 +0000 Subject: [PATCH 03/11] fix(vllm): reconcile OOM detector with diagnose threshold and de-dup env const (eai-8059) Address PR review on the OOM serve-summary work. rocm-core: classify a vLLM startup-log tail as OOM with the same rule as the `rocm diagnose` vLLM-OOM checker (anchored line clearing MIN_SCORE_FOR_MATCH) instead of a second, looser substring scan. A bare "out of memory" (kernel OOM-killer, a dependency's log) or a bare `torch.OutOfMemoryError` class with no allocator message no longer drives the memory note. Add `vllm_oom_diagnose_symptom` so the engine hint and the CLI serve summary select the `--symptom` line one way. Correct the utilization-hint bound (the parser rejects 0, so `<0-1>` was wrong). serve summary: the OOM note now carries the model-too-large caveat (lowering the reservation will not help; serve a smaller/quantized model) and routes to `rocm diagnose --symptom`, matching the diagnose entry's split remediation. e2e: hoist OOM_FAULT_INJECTION_ENV into the shared e2e-report crate so xtask (producer) and the e2e-cucumber harness (consumer) reference one literal and cannot drift. Signed-off-by: Roman Sirokov --- apps/rocm/src/serve_summary.rs | 31 ++++++++-- crates/e2e-report/src/lib.rs | 14 +++++ crates/rocm-core/src/lib.rs | 88 ++++++++++++++++++++-------- engines/vllm/src/lib.rs | 60 +++++++++++-------- tests/e2e-cucumber/src/capability.rs | 8 ++- xtask/src/e2e.rs | 16 +++-- 6 files changed, 154 insertions(+), 63 deletions(-) diff --git a/apps/rocm/src/serve_summary.rs b/apps/rocm/src/serve_summary.rs index 4750fadb4..eb7b8512c 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -209,14 +209,23 @@ pub(crate) fn serve_failed_to_become_ready(status: &str) -> bool { /// The note names `--gpu-memory-utilization` and `--gpu` and is worded for the /// shared-node case rather than as unconditional advice — vLLM reserves a /// fraction of *total* VRAM, so a value good for a shared card would degrade a -/// dedicated one. It shares [`rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT`] with -/// the pre-launch low-VRAM note so both surfaces point at the same fix. +/// dedicated one. When the model simply does not fit, it says so and points at a +/// smaller/quantized model instead of the knob (which would only trade an +/// earlier OOM for a later one), matching the `rocm diagnose` vLLM-OOM entry it +/// then routes the user to. It shares +/// [`rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT`] verbatim with the pre-launch +/// low-VRAM note so both surfaces point at the same fix and the pre-launch +/// warning can dedup against it. pub(crate) fn oom_memory_note(status: &str, log_tail: &str) -> Option { if !serve_failed_to_become_ready(status) || !rocm_core::vllm_log_shows_oom(log_tail) { return None; } + let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); Some(format!( - "the serve attempt ran out of GPU memory. {}", + "the serve attempt ran out of GPU memory. {} If the model simply does not fit in this \ + GPU's VRAM, lowering the reservation will not help — serve a smaller or quantized model \ + instead (rocm-cli serves one model on a single GPU). To have the tool pick the \ + case-appropriate fix, run `rocm diagnose --symptom '{symptom}'`.", rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT )) } @@ -556,6 +565,17 @@ mod tests { assert!(note.contains("--gpu-memory-utilization"), "{note}"); assert!(note.contains("--gpu "), "{note}"); assert!(note.contains("ran out of GPU memory"), "{note}"); + // The knob is not unconditional: when the model simply does not fit, the + // note must say lowering the reservation will not help and point at a + // smaller/quantized model, matching the `rocm diagnose` vLLM-OOM entry. + assert!( + note.contains("smaller or quantized model"), + "the note must carry the model-too-large caveat: {note}" + ); + assert!( + note.contains("rocm diagnose --symptom"), + "the note must route the user to the conditional diagnose entry: {note}" + ); } #[test] @@ -576,7 +596,10 @@ mod tests { fn oom_note_renders_in_the_summary_notes() { let mut summary = base_summary(); summary.status = "starting".to_owned(); - if let Some(note) = oom_memory_note(&summary.status, "torch.OutOfMemoryError") { + if let Some(note) = oom_memory_note( + &summary.status, + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ) { summary.notes.push(note); } let rendered = render_summary(&summary); diff --git a/crates/e2e-report/src/lib.rs b/crates/e2e-report/src/lib.rs index faa3229ce..bbca981a9 100644 --- a/crates/e2e-report/src/lib.rs +++ b/crates/e2e-report/src/lib.rs @@ -15,6 +15,20 @@ use std::time::SystemTime; use maud::{DOCTYPE, Markup, PreEscaped, html}; use serde::Deserialize; +/// Environment variable signalling the `e2e-oom-fault-injection` hook is present. +/// +/// `cargo xtask e2e` sets it to `1` when it built the binary under test with the +/// `rocm/e2e-oom-fault-injection` feature, so the `e2e-cucumber` harness can tell +/// whether `@requires-oom-fault-injection` scenarios can run against it. +/// +/// The single source of truth for this xtask ↔ harness contract. It lives in +/// this lean crate — the one both `xtask` and `e2e-cucumber` already depend on — +/// so the producer (`xtask::e2e`) and the consumer +/// (`e2e_cucumber::capability`) reference the same literal and cannot drift: a +/// typo previously would not fail to compile or fail a test, silently turning +/// `@requires-oom-fault-injection` into a skip (green suite, zero coverage). +pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; + #[derive(Deserialize)] struct Feature { name: String, diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index f13800b32..2b69de5a8 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -7400,30 +7400,52 @@ pub fn resolve_amd_smi_binary() -> OsString { /// collides with memory already in use and the engine OOMs even a tiny model. pub const VLLM_GPU_MEMORY_UTILIZATION_HINT: &str = "vLLM reserves ~90% of the GPU's total VRAM by default; on a shared or busy GPU this can \ collide with memory already in use. Lower the reservation with `--gpu-memory-utilization \ - <0-1>` (e.g. 0.1 for a small model), or target a less-busy GPU with `--gpu `."; + ` (e.g. 0.1 for a small model), or target a less-busy \ + GPU with `--gpu `."; -/// Whether a vLLM/PyTorch log excerpt carries a startup out-of-memory -/// signature. +/// Whether a vLLM startup-log tail carries a genuine out-of-memory failure. /// -/// Deliberately excludes vLLM's generic "engine core initialization failed" -/// wrapper: vLLM emits that line as the terminal message for *any* EngineCore -/// startup crash (unsupported architecture, shm size, tensor-parallel -/// misconfiguration, missing weights, and OOM alike), and being the last line -/// it reliably lands in a truncated log tail — treating it as an OOM signature -/// would misreport unrelated startup failures as memory exhaustion. +/// Classifies the tail with the *same* rule as the `rocm diagnose` vLLM-OOM +/// checker (`check_16_vllm_oom`, reached through +/// [`vllm_oom_symptom_is_diagnosable`]) so the pre-launch low-VRAM note, the +/// post-failure serve summary, and `rocm diagnose` never disagree on what counts +/// as a vLLM OOM. The tail is vLLM's own process output, so the checker's +/// required vLLM anchor is supplied by context: each line is scored as +/// `vllm: `, and the tail is an OOM iff some line clears the checker's +/// `MIN_SCORE_FOR_MATCH` threshold. /// -/// The two signatures are complementary, not redundant: `outofmemory` (the -/// space-free form, matched after lowercasing) catches the allocator error -/// types whichever module path they carry — `torch.OutOfMemoryError`, -/// `torch.cuda.OutOfMemoryError`, `hipErrorOutOfMemory` — while `out of memory` -/// catches the spaced runtime phrasing (`HIP out of memory`, `CUDA out of -/// memory`). An earlier `hip out of memory` entry was dead code, since any log -/// containing it already contains `out of memory`. +/// This deliberately rejects a bare `out of memory` with no allocator-shaped +/// signature — a kernel OOM-killer line, a dependency's log, or an unrelated +/// subprocess — the same false positive the checker's threshold guards against, +/// rather than driving the user-facing memory note off any stray OOM substring +/// in the tail. A real vLLM/HIP allocation failure (`HIP out of memory`, +/// `CUDA out of memory`, `hipErrorOutOfMemory`, or `torch.OutOfMemoryError` +/// corroborated by an allocator message) still clears it. pub fn vllm_log_shows_oom(log: &str) -> bool { - const SIGNATURES: &[&str] = &["outofmemory", "out of memory"]; + log.lines().any(|line| { + let line = line.trim(); + !line.is_empty() && diagnose::vllm_oom_symptom_is_diagnosable(&format!("vllm: {line}")) + }) +} - let lower = log.to_ascii_lowercase(); - SIGNATURES.iter().any(|signature| lower.contains(signature)) +/// The `rocm diagnose --symptom` string to route a vLLM OOM log tail to. +/// +/// Prefers the user's actual failing line (so `diagnose` echoes their real +/// error) and falls back to [`VLLM_OOM_CANONICAL_SYMPTOM`] when no single line +/// clears the checker's threshold, so the printed command always reports a +/// cause. Shared by the vLLM engine's post-failure hint and the `rocm` CLI serve +/// summary so both surfaces route to `diagnose` identically instead of +/// hand-rolling the line selection twice. +pub fn vllm_oom_diagnose_symptom(log_tail: &str) -> String { + log_tail + .lines() + .rev() + .map(str::trim) + .find(|line| !line.is_empty() && vllm_log_shows_oom(line)) + .map_or_else( + || VLLM_OOM_CANONICAL_SYMPTOM.to_owned(), + |line| format!("vllm: {line}"), + ) } /// Locate `amd-smi` inside the bin directories of the newest managed ROCm SDK @@ -11826,18 +11848,34 @@ last_installed_runtime_id = "therock-release" #[test] fn vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures() { - // Space-free allocator error types, whatever module path they carry. + // A real allocator message (the `MIN_SCORE_FOR_MATCH`-clearing shape the + // `rocm diagnose` vLLM-OOM checker recognises) is a genuine OOM, whichever + // module path the exception carries. assert!(vllm_log_shows_oom( - "torch.OutOfMemoryError: HIP out of memory." + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." )); assert!(vllm_log_shows_oom( - "raise torch.cuda.OutOfMemoryError(msg) # CUDA path" + "torch.cuda.OutOfMemoryError: CUDA out of memory" )); assert!(vllm_log_shows_oom("RuntimeError: hipErrorOutOfMemory")); - // Spaced runtime phrasing on its own (no `OutOfMemoryError` token). - assert!(vllm_log_shows_oom("HIP error: out of memory")); // Case-insensitive. - assert!(vllm_log_shows_oom("TORCH.OUTOFMEMORYERROR")); + assert!(vllm_log_shows_oom("HIP OUT OF MEMORY")); + + // Sub-threshold on their own — the same lines the diagnose checker keeps + // below `MIN_SCORE_FOR_MATCH` so a stray OOM phrase does not carry the + // verdict. Detecting these here would contradict that checker (they are + // the false positives the shared threshold exists to reject). + // + // The bare exception class every PyTorch OOM raises, with no allocator + // message to corroborate it. + assert!(!vllm_log_shows_oom("TORCH.OUTOFMEMORYERROR")); + // A spaced "out of memory" with no allocator anchor phrase. + assert!(!vllm_log_shows_oom("HIP error: out of memory")); + // A kernel OOM-killer line from an unrelated subprocess must NOT drive + // the vLLM memory note. + assert!(!vllm_log_shows_oom( + "Out of memory: Killed process 4242 (python)" + )); // The generic EngineCore wrapper is NOT an OOM signature. assert!(!vllm_log_shows_oom( "EngineCore failed: engine core initialization failed" diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 753eed08d..6e69f8baf 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -2230,25 +2230,13 @@ fn oom_utilization_hint(log_tail: &str) -> String { if !rocm_core::vllm_log_shows_oom(log_tail) { return String::new(); } - // Route the user's *actual* failing line into the `--symptom` example when - // the diagnose checker would actually score it; otherwise fall back to the - // canonical symptom so the printed command always reports a cause. The - // detector here is a coarse substring scan that accepts lines the scorer - // rates sub-threshold (e.g. a bare "... out of memory"), so without this - // fallback the diagnose command could report nothing -- which reads as "the - // tool checked and there's no known cause", worse than not printing it. - let symptom_line = log_tail - .lines() - .rev() - .map(str::trim) - .find(|line| !line.is_empty() && rocm_core::vllm_log_shows_oom(line)) - .unwrap_or("out of memory"); - let candidate = format!("vllm: {symptom_line}"); - let symptom = if rocm_core::vllm_oom_symptom_is_diagnosable(&candidate) { - candidate - } else { - rocm_core::VLLM_OOM_CANONICAL_SYMPTOM.to_owned() - }; + // Pick the `--symptom` line with the shared helper so this surface and the + // `rocm` CLI serve summary route to `rocm diagnose` identically instead of + // hand-rolling the selection twice. `vllm_log_shows_oom` now classifies each + // line with the *same* rule the diagnose checker uses, so the chosen line is + // guaranteed to score for that checker. + let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); + let symptom_line = symptom.strip_prefix("vllm: ").unwrap_or(symptom.as_str()); format!( "\n\nDetected an out-of-memory failure ({symptom_line}). {}\n\ For conditional remediation, run `rocm diagnose --symptom '{symptom}'`.", @@ -2750,15 +2738,13 @@ mod tests { fn every_emitted_oom_symptom_is_diagnosable() { // Closes the loop between the two layers: the engine prints // `rocm diagnose --symptom ''`, so whatever it emits must - // actually score for the diagnose checker -- including for lines the - // coarse substring detector accepts but the scorer rates sub-threshold - // on their own (those fall back to the canonical symptom). + // actually score for the diagnose checker. Since the detector now + // classifies each line with the *same* rule the checker uses, an accepted + // line is diagnosable by construction -- this guards that invariant. let accepted_lines = [ "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", "RuntimeError: hipErrorOutOfMemory", - "torch.cuda.OutOfMemoryError", - "HIP error: out of memory", - "the process was killed: out of memory", + "torch.cuda.OutOfMemoryError: CUDA out of memory", "CUDA out of memory", ]; for line in accepted_lines { @@ -2777,6 +2763,30 @@ mod tests { "emitted symptom must be diagnosable, got {symptom:?} for line {line:?}" ); } + + // The reconciled detector rejects sub-threshold lines the loose scan used + // to accept -- exactly the false positives the diagnose checker's + // threshold guards against. Rejecting them here keeps the two layers from + // disagreeing (the engine must not print a `--symptom` the checker would + // then score as "no known cause"). + let rejected_lines = [ + // The bare exception class every PyTorch OOM raises, uncorroborated. + "torch.cuda.OutOfMemoryError", + // A spaced "out of memory" with no allocator anchor phrase. + "HIP error: out of memory", + // A kernel OOM-killer line from an unrelated subprocess. + "the process was killed: out of memory", + ]; + for line in rejected_lines { + assert!( + !rocm_core::vllm_log_shows_oom(line), + "detector must reject sub-threshold line: {line}" + ); + assert!( + oom_utilization_hint(line).is_empty(), + "a sub-threshold line must not carry a memory hint: {line}" + ); + } } #[test] diff --git a/tests/e2e-cucumber/src/capability.rs b/tests/e2e-cucumber/src/capability.rs index 854dc0777..1dcb176e3 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -127,9 +127,11 @@ pub struct HostCapability { } /// Environment variable `xtask e2e` sets to `1` when it compiled the binary -/// under test with the `rocm/e2e-oom-fault-injection` feature. Kept in sync with -/// the same name in `xtask::e2e`. -pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; +/// under test with the `rocm/e2e-oom-fault-injection` feature. +/// +/// Re-exported from `e2e-report` — the single source of truth shared with the +/// producer (`xtask::e2e`) — so the two sides cannot drift out of sync. +pub use e2e_report::OOM_FAULT_INJECTION_ENV; impl HostCapability { /// Whether a given engine can actually START on this host. Distinct from diff --git a/xtask/src/e2e.rs b/xtask/src/e2e.rs index 971c8c00b..bd0a5a00b 100644 --- a/xtask/src/e2e.rs +++ b/xtask/src/e2e.rs @@ -18,12 +18,16 @@ use anyhow::{Context, Result, bail}; use crate::paths::{binary_name, target_dir, workspace_root}; -/// Signal to the e2e-cucumber harness that the binary under test carries the -/// `rocm/e2e-oom-fault-injection` hook. Set to `1` only when this xtask built -/// the binary itself with that feature; a prebuilt `ROCM_CLI_BINARY` (the -/// self-hosted lanes' shipping release) has the hook compiled out. Kept in sync -/// with `e2e_cucumber::capability::OOM_FAULT_INJECTION_ENV`. -const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; +/// Environment variable that signals to the e2e-cucumber harness that the binary +/// under test carries the `rocm/e2e-oom-fault-injection` hook. Set to `1` only +/// when this xtask built the binary itself with that feature; a prebuilt +/// `ROCM_CLI_BINARY` (the self-hosted lanes' shipping release) has the hook +/// compiled out. +/// +/// The literal is defined once in `e2e-report` and consumed there by the harness +/// (`e2e_cucumber::capability::OOM_FAULT_INJECTION_ENV`) so producer and consumer +/// cannot drift. +use e2e_report::OOM_FAULT_INJECTION_ENV; #[derive(Debug, PartialEq, Eq)] struct E2eBinaries { From 4a108b95ed409e9afb710b9124f7932304098681 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 9 Sep 2026 16:26:14 +0300 Subject: [PATCH 04/11] serve: make the GPU fail-fast contract true and share the vLLM tail budget (eai-8059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on three points that the earlier response commits did not actually close in the code: - The reuse pre-gate keyed on the engine alone, so a live managed service for an unrelated model pulled the reuse probe's `ResolveModel` round-trip — and, for a self-managing engine, an `ensure_self_managed_engine_ready` install that prints "Preparing for GPU serving..." — ahead of the no-usable-GPU bail. The comment above the bail claimed that work could not precede it. Gate the probe on the model as well (`any_live_managed_service_for_model`), matching with the same lenient relation the service surfaces already use so short-vs-canonical spellings still reach the probe, and state the real contract in the comment. - `engines/vllm`'s `STARTUP_FAILURE_LOG_TAIL_LINES` was an independent `80` literal while the serve summary's comment claimed the two surfaces were coupled to `rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES`. Derive it from that constant so the claim holds. - `assert_prebuilt_e2e_lanes_enable_test_hooks` said it asserted the exact feature list `cargo xtask e2e` builds; xtask now builds a superset. Describe it as the shared hook floor and note why the fault-injection hook is deliberately absent from prebuilt lanes. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 206 +++++++++++++++++++++++++++++---- engines/vllm/src/lib.rs | 8 +- xtask/src/workflow_contract.rs | 17 ++- 3 files changed, 204 insertions(+), 27 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 365617357..68a63d62c 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -5134,19 +5134,28 @@ fn serve(args: ServeArgs) -> Result<()> { // Reusing an already-running managed service launches nothing and pins no // GPU, so it must bypass the GPU-required pre-flight below — the reused // service was already vetted at its own launch, and this invocation does no - // GPU work. Detect that here, but only when a live managed service for this - // engine already exists (so the common launch path keeps failing fast before - // any engine work) and the model ref can be canonicalized (a runtime/env is - // available, or the engine manages its own). Without a runtime we cannot - // resolve, so we fall through and the pre-flight refuses the no-GPU / - // no-runtime case with its usual message. + // GPU work. Detect that here, but only when a live managed service that + // plausibly serves *this* model already exists, and when the model ref can be + // canonicalized (a runtime/env is available, or the engine manages its own). + // Without a runtime we cannot resolve, so we fall through and the pre-flight + // refuses the no-GPU / no-runtime case with its usual message. + // + // The pre-gate matches on the model, not just the engine: everything inside + // this block is real engine work (a `ResolveModel` round-trip, and for a + // self-managing engine an `ensure_self_managed_engine_ready` that can print + // "Preparing for GPU serving..." and install), so gating on the + // engine alone let a live service for an *unrelated* model — one this + // invocation can never reuse — drag that work ahead of the no-usable-GPU + // bail on a GPU-less host. let mut resolved_model: Option = None; let mut reuse_existing = false; let can_resolve_model = !cpu_only && (resolved_selection.runtime_id.is_some() || resolved_selection.env_id.is_some() || engine_manages_own_runtime(&selected_engine)); - if can_resolve_model && any_live_managed_service_for_engine(&paths, &selected_engine) { + if can_resolve_model + && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) + { if engine_manages_own_runtime(&selected_engine) { ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; } @@ -5168,11 +5177,20 @@ fn serve(args: ServeArgs) -> Result<()> { resolved_model = Some(probe); } // Fail fast under a GPU-required policy when the host has no usable AMD GPU, - // BEFORE preparing or launching any engine (no wasted engine download, and an - // actionable message instead of a late engine crash). The engine enforces the - // same rule as a backstop. Skipped for cpu_only and when reusing an - // already-running service (nothing is launched); permissive when availability - // cannot be probed on this platform (probe returns `None`). The E2E-only + // before preparing or launching any engine for *this* model (no wasted engine + // download, and an actionable message instead of a late engine crash). The + // engine enforces the same rule as a backstop. + // + // The precise contract, since the reuse detection above is the one thing that + // can precede this bail: engine work runs first only when a live managed + // service already matches this engine and model — i.e. only when this + // invocation is about to reuse it and legitimately skip the bail. When no such + // service exists (the ordinary launch, and every no-GPU refusal path) the + // block above is skipped entirely and this is still the first thing that runs. + // + // Skipped for cpu_only and when reusing an already-running service (nothing is + // launched); permissive when availability cannot be probed on this platform + // (probe returns `None`). The E2E-only // backend-failure scenario bypasses this host precondition so the black-box // test reaches Lemonade's backend boundary without real GPU hardware, and the // E2E-only OOM-launch fault injection likewise stands in for the GPU it does @@ -16609,16 +16627,32 @@ fn existing_live_managed_service( }) } -/// Whether any managed service for `engine` is currently live, without needing a -/// canonical model id. Used as a cheap pre-gate before resolving a model purely -/// to detect a reusable already-running service: when no live service for the -/// engine exists, the reuse check (and its `ResolveModel` round-trip) is skipped -/// and the normal launch pre-flight runs unchanged. -fn any_live_managed_service_for_engine(paths: &AppPaths, engine: &str) -> bool { +/// Whether a live managed service for `engine` plausibly already serves +/// `model_ref`, decided from the records alone — no canonical model id, and so +/// no engine round-trip, required. +/// +/// Cheap pre-gate for the reuse detection in `serve`. Everything that check does +/// is real engine work: a `ResolveModel` round-trip and, for a self-managing +/// engine, an [`ensure_self_managed_engine_ready`] that may print +/// "Preparing for GPU serving..." and install. That work runs ahead of +/// the no-usable-GPU pre-flight, so it must be reserved for invocations that can +/// actually reuse something: keying on the engine alone let a live service for an +/// unrelated model pull an install in front of the bail on a GPU-less host. +/// +/// Matching uses [`service_model_names_match`] — the same lenient relation the +/// service-listing surfaces already use to tie a user-typed name to a record — so +/// a short-vs-canonical spelling still reaches the probe. The probe then decides +/// reuse authoritatively on the canonical id via +/// [`existing_live_managed_service`]; this only decides whether asking is worth +/// the engine round-trip. +fn any_live_managed_service_for_model(paths: &AppPaths, engine: &str, model_ref: &str) -> bool { load_managed_services(paths).is_ok_and(|records| { - records - .iter() - .any(|record| record.engine == engine && managed_service_is_live(record)) + records.iter().any(|record| { + record.engine == engine + && (service_model_names_match(&record.model_ref, model_ref) + || service_model_names_match(&record.canonical_model_id, model_ref)) + && managed_service_is_live(record) + }) }) } @@ -24701,6 +24735,136 @@ install therock"; assert!(found.is_none()); } + /// Write one live managed record and report what the reuse pre-gate makes of + /// a serve for `queried_model_ref`. + fn reuse_pregate_for( + label: &str, + record_model_ref: &str, + record_canonical_model_id: &str, + queried_engine: &str, + queried_model_ref: &str, + ) -> Result { + let (root, paths) = test_paths(label); + paths.ensure()?; + let mut record = ManagedServiceRecord::new( + &paths, + format!("lemonade-{label}"), + "lemonade", + record_model_ref, + record_canonical_model_id, + "127.0.0.1", + 11_520, + "managed", + std::process::id(), + None, + None, + None, + ); + record.status = "starting".to_owned(); + record.engine_pid = Some(std::process::id()); + record.write()?; + + let gated = any_live_managed_service_for_model(&paths, queried_engine, queried_model_ref); + let _ = fs::remove_dir_all(root); + Ok(gated) + } + + #[test] + fn reuse_pregate_skips_engine_work_for_an_unrelated_live_model() -> Result<()> { + // EAI-8059 review: the reuse probe runs a `ResolveModel` round-trip and, + // for a self-managing engine, an install that prints "Preparing ... for + // GPU serving" — all of it ahead of the no-usable-GPU bail. Keying the + // pre-gate on the engine alone let a live service for a model this + // invocation can never reuse pull that work in front of the bail. Gating + // on the model keeps the fail-fast contract true. + assert!( + !reuse_pregate_for( + "reuse-pregate-other-model", + "other", + "other-canonical", + "lemonade", + "qwen", + )?, + "a live service for an unrelated model must not unlock the reuse probe" + ); + Ok(()) + } + + #[test] + fn reuse_pregate_admits_the_same_model_including_a_short_spelling() -> Result<()> { + // The probe must still be reached for anything that could genuinely be + // reused, so the gate must not narrow reuse detection: an exact ref, a + // canonical-id-only match, and a short-vs-canonical spelling all pass. + // The probe then decides reuse authoritatively on the canonical id. + assert!( + reuse_pregate_for( + "reuse-pregate-exact", + "qwen", + "qwen-canonical", + "lemonade", + "qwen", + )?, + "the same raw model ref must unlock the reuse probe" + ); + assert!( + reuse_pregate_for( + "reuse-pregate-canonical", + "some-alias", + "Qwen/Qwen3-8B", + "lemonade", + "Qwen/Qwen3-8B", + )?, + "a canonical-id match must unlock the reuse probe" + ); + assert!( + reuse_pregate_for( + "reuse-pregate-short", + "Qwen/Qwen3-8B", + "Qwen/Qwen3-8B", + "lemonade", + "qwen3-8b", + )?, + "a short spelling of the same model must unlock the reuse probe" + ); + Ok(()) + } + + #[test] + fn reuse_pregate_admits_the_model_id_the_oom_reuse_scenario_plants() -> Result<()> { + // `@id:serve-oom-memory-guidance` plants a live record for this exact id + // and then serves it, relying on the reuse short-circuit to reach the + // summary on a GPU-less host. That scenario only runs on the + // `@requires-no-gpu` mock lane, so pin the pre-gate's verdict for its + // literal model id here too: a matcher change that silently turned the + // scenario into a launch attempt would otherwise only show up there. + assert!( + reuse_pregate_for( + "reuse-pregate-e2e-oom", + "e2e/oom-model", + "e2e/oom-model", + "lemonade", + "e2e/oom-model", + )?, + "the OOM reuse scenario's planted service must unlock the reuse probe" + ); + Ok(()) + } + + #[test] + fn reuse_pregate_still_keys_on_the_engine() -> Result<()> { + assert!( + !reuse_pregate_for( + "reuse-pregate-other-engine", + "qwen", + "qwen-canonical", + "vllm", + "qwen", + )?, + "a live service for a different engine must not unlock the reuse probe" + ); + Ok(()) + } + #[test] fn spawn_managed_engine_child_blocks_reuse_with_mismatched_recipe() -> Result<()> { // A live service recorded with one recipe (e.g. a tool-call parser flag) diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 6e69f8baf..4e86bca4a 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -32,7 +32,13 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; const ENGINE_NAME: &str = "vllm"; const DEFAULT_HOST: &str = "127.0.0.1"; const HEALTHCHECK_TIMEOUT_MS: u64 = 700; -const STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80; +/// Tail budget for the startup-failure summary (and its OOM hint). +/// +/// Derived from [`DEFAULT_LOG_TAIL_LINES`] rather than spelled as its own +/// literal so this surface and the CLI's serve summary — which reads the same +/// protocol constant in `append_oom_serve_note` — cannot drift on what counts as +/// "the tail" of a failed launch. A change to the protocol budget moves both. +const STARTUP_FAILURE_LOG_TAIL_LINES: usize = DEFAULT_LOG_TAIL_LINES; const MAX_TAIL_READ: u64 = 4 * 1024 * 1024; /// How long a stop waits for the server to actually exit after each signal /// before reporting a timeout (or, under `force`, escalating to `SIGKILL`). diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 236a898fd..3fa96ea32 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -242,8 +242,7 @@ mod tests { } /// Every lane that pre-builds `rocm` and hands it to the suite via - /// `ROCM_CLI_BINARY` must enable the same test-hook feature `cargo xtask e2e` - /// enables when it builds for itself. + /// `ROCM_CLI_BINARY` must enable `rocm/e2e-test-hooks`. /// /// The suite's deterministic failure seams (e.g. the scripted Lemonade /// backend-install failure) are `#[cfg(feature = "e2e-test-hooks")]`. A lane @@ -252,6 +251,14 @@ mod tests { /// regressions — but only on whichever lane happens to select them, which is /// what made this divergence so hard to read the first time. Pin it here so a /// new lane copying an existing block cannot silently reintroduce it. + /// + /// `cargo xtask e2e` builds a *superset* for itself + /// (`rocm/e2e-test-hooks rocm/e2e-oom-fault-injection`), so this asserts the + /// shared floor rather than an exact match. The extra + /// `e2e-oom-fault-injection` hook is deliberately absent from the prebuilt + /// lanes: the scenarios needing it carry `@requires-oom-fault-injection` and + /// the harness skips them when `xtask` did not build the binary itself, so a + /// missing hook is a reported skip rather than a silent green. fn assert_prebuilt_e2e_lanes_enable_test_hooks(workflow: &str, text: &str) { let blocks: Vec<_> = multiline_run_blocks(text) .into_iter() @@ -267,9 +274,9 @@ mod tests { "cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks" ), "{workflow} prebuilt E2E lane must build with \ - `--features rocm/e2e-test-hooks`, matching what `cargo xtask e2e` \ - builds for itself; without it the suite's scripted failure seams \ - are compiled out:\n{block}" + `--features rocm/e2e-test-hooks`, the hook floor `cargo xtask e2e` \ + also builds for itself; without it the suite's scripted failure \ + seams are compiled out:\n{block}" ); } } From 0be2dde4f51c536ff94a9d653cfc717877f5cdd2 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 12:14:33 +0300 Subject: [PATCH 05/11] fix(serve): the serve summary's OOM note let log text break out of the command it prints `oom_memory_note` interpolated `rocm_core::vllm_oom_diagnose_symptom` straight into a single-quoted shell word: run `rocm diagnose --symptom '{symptom}'` That value is a line of vLLM's own subprocess output, and the sentence around it invites the user to paste the command. An apostrophe -- routine in Python error text (`GPU 0 can't allocate the model's weights`, `model 'foo'`) -- closes the quote, and the ANSI/BEL bytes vLLM's colourised logger emits reach the terminal verbatim. The vLLM engine's startup-failure hint had exactly this defect and was fixed on `gpu-out-of-memory`; this is the same defect on a second surface. Promote the two guards written for the engine into `rocm-core`, next to the helpers they guard, and use them from both command-building sites: - `quotable_in_single_quotes` -- used by the engine hint and now by the serve summary note. A quote- or control-bearing symptom is rejected in favour of `VLLM_OOM_CANONICAL_SYMPTOM`, which is guaranteed to report a cause. - `strip_terminal_control_sequences` -- used by the engine, which echoes the raw failing line. The serve summary echoes no raw line, so it needs only the quotability guard. Rejecting rather than escaping (`'` -> `'\''`) stays deliberate: a human pastes the result, an escaped form cannot be eye-checked, a subtly wrong escape is runnable-and-misleading rather than obviously broken, and control bytes would still reach the terminal. `vllm_oom_diagnose_symptom` keeps its contract -- it returns log text, not a shell token -- because some callers only display it; guaranteeing quotability there would withhold the user's real error from text that is never pasted anywhere. Its doc now says so, and says what a command-building caller owes. Regression tests on the serve-summary surface, both falsified against the pre-fix code: an apostrophe-bearing traceback tail produced four quotes in the rendered command, and a colourised line put raw escape bytes in it. Both assert the exact symptom that is printed rather than the absence of a fragment -- "contains no `'`" also holds if the apostrophes were silently deleted, which is the escaping behaviour the fallback exists to avoid. A third test pins that a clean line is still quoted verbatim, so the guard does not cost the common case its own error text. Signed-off-by: Roman Sirokov --- apps/rocm/src/serve_summary.rs | 109 +++++++++++++++++++++++++++++++ crates/rocm-core/src/lib.rs | 115 +++++++++++++++++++++++++++++++++ docs/vllm.md | 5 +- engines/vllm/src/lib.rs | 60 ++--------------- 4 files changed, 233 insertions(+), 56 deletions(-) diff --git a/apps/rocm/src/serve_summary.rs b/apps/rocm/src/serve_summary.rs index d38e92b91..a0784a09b 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -214,7 +214,19 @@ pub(crate) fn oom_memory_note(status: &str, log_tail: &str) -> Option { if !serve_failed_to_become_ready(status) || !rocm_core::vllm_log_shows_oom(log_tail) { return None; } + // The symptom is vLLM's own subprocess output and it lands inside a + // single-quoted shell word in a sentence that invites the user to paste the + // command, so it is only routed through verbatim when it can be rendered as + // one intact quoted argument; otherwise the canonical symptom stands in. See + // [`rocm_core::quotable_in_single_quotes`] for why this rejects rather than + // escapes. The engine's startup-failure hint guards the same text the same + // way. let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); + let symptom = if rocm_core::quotable_in_single_quotes(&symptom) { + symptom + } else { + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM.to_owned() + }; Some(format!( "the serve attempt ran out of GPU memory. {} If the model simply does not fit in this \ GPU's VRAM, lowering the reservation will not help — serve a smaller or quantized model \ @@ -529,6 +541,103 @@ mod tests { ); } + /// The `--symptom` value the note actually hands the user, read back out of + /// the rendered text exactly the way a shell would: the note's prose carries + /// its own apostrophes (`GPU's VRAM`), so the command is extracted from + /// between its backticks first, then the first `'...'` word inside it. + fn quoted_symptom_argument(note: &str) -> &str { + note.split("run `") + .nth(1) + .and_then(|rest| rest.split('`').next()) + .expect("the note must print a runnable command") + .split("--symptom '") + .nth(1) + .and_then(|rest| rest.split('\'').next()) + .expect("the command must carry a --symptom value") + } + + #[test] + fn an_apostrophe_in_the_failing_line_cannot_break_out_of_the_printed_command() { + // A realistic vLLM traceback tail: apostrophes are routine in Python + // error text, and this line scores well above MIN_SCORE_FOR_MATCH + // (torch.OutOfMemoryError + HIP out of memory), so the "route the user's + // real line" branch selects it. + let log_tail = concat!( + " File \"/opt/vllm/worker.py\", line 212, in load_model\n", + "ERROR 09-14 12:00:01 engine.py:389] torch.OutOfMemoryError: HIP out of memory. ", + "Tried to allocate 7.21 GiB. GPU 0 can't allocate the model's weights.\n" + ); + let note = oom_memory_note("starting", log_tail).expect("an OOM failure must carry a note"); + + // The rendered command must be one intact single-quoted argument: no + // byte the vLLM subprocess printed may close the quote and land outside + // it in a command the note invites the user to paste. + let command = note + .split("run `") + .nth(1) + .and_then(|rest| rest.split('`').next()) + .expect("the note must print a runnable command"); + assert_eq!( + command.matches('\'').count(), + 2, + "the --symptom argument must stay a single balanced quoted word: {command}" + ); + // Pin which branch ran, not just that no apostrophe survived: "contains + // no `'`" also holds if the apostrophes were silently deleted from the + // user's line, which is the escaping-style behaviour the fallback exists + // to avoid. Rejection means the canonical symptom, exactly. + let symptom = quoted_symptom_argument(¬e); + assert_eq!( + symptom, + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + "a quote-bearing line must be rejected in favour of the canonical symptom, \ + not silently rewritten: {symptom:?}" + ); + // ...and the command it does print must still report a cause. + assert!( + rocm_core::vllm_oom_symptom_is_diagnosable(symptom), + "the fallback symptom must still be diagnosable: {symptom:?}" + ); + } + + #[test] + fn control_bytes_from_the_log_never_reach_the_printed_command() { + // vLLM's logger colourises; an ANSI-coloured OOM line must not repaint + // the user's terminal from inside rocm-cli's own serve summary. + let log_tail = "\u{1b}[31mRuntimeError: HIP out of memory\u{1b}[0m\u{7}"; + let note = oom_memory_note("starting", log_tail).expect("an OOM failure must carry a note"); + assert!( + !note.chars().any(|c| c.is_control() && c != '\n'), + "no control byte may survive into the printed note: {note:?}" + ); + // Pin the exact value rather than the absence of a few fragments: an + // absence check cannot fail for the defect it names, since a stripper + // that drops only the escape byte and the `[` leaves `31m`/`0m` behind, + // which contains neither `[31m` nor `[0m` and carries no control byte. + let symptom = quoted_symptom_argument(¬e); + assert_eq!( + symptom, + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + "a control-byte-bearing line must be rejected in favour of the canonical \ + symptom, not stripped into a lookalike: {symptom:?}" + ); + } + + #[test] + fn oom_note_quotes_a_clean_failing_line_verbatim() { + // The guard must not cost the common case its own error text: a line + // with no quote and no control byte is still routed into the command. + let note = oom_memory_note( + "starting", + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ) + .expect("an OOM failure must carry a note"); + assert_eq!( + quoted_symptom_argument(¬e), + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." + ); + } + #[test] fn oom_note_renders_in_the_summary_notes() { let mut summary = base_summary(); diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index fd7469b8c..df0cabd2e 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -7657,6 +7657,14 @@ pub fn vllm_log_shows_oom(log: &str) -> bool { /// cause. Shared by the vLLM engine's post-failure hint and the `rocm` CLI serve /// summary so both surfaces route to `diagnose` identically instead of /// hand-rolling the line selection twice. +/// +/// The return value is *log text*, not a shell-safe token: some callers only +/// display it. A caller that renders it inside a quoted command must gate it on +/// [`quotable_in_single_quotes`] first and fall back to +/// [`VLLM_OOM_CANONICAL_SYMPTOM`] otherwise. Guaranteeing quotability here +/// instead would impose the command-builder's constraint on the display-only +/// callers, silently withholding the user's real error from text that is never +/// pasted anywhere. pub fn vllm_oom_diagnose_symptom(log_tail: &str) -> String { log_tail .lines() @@ -7669,6 +7677,68 @@ pub fn vllm_oom_diagnose_symptom(log_tail: &str) -> String { ) } +/// Whether `symptom` can be placed inside a `'...'` shell word verbatim. +/// +/// The value is untrusted subprocess output (the vLLM startup log tail) and the +/// messages it lands in invite the user to paste the command into a shell, so a +/// bare `'` would close the quote and let text nobody vetted become shell +/// syntax. An apostrophe is routine in Python error text (`can't allocate`, +/// `model 'foo'`), and a line only has to look like an allocation failure to be +/// selected, so this is an ordinary case rather than an exotic one. +/// +/// Rejecting instead of escaping (`'` -> `'\''`) is deliberate. Escaping keeps +/// the exact bytes but yields a command a reader cannot check by eye, and a +/// wrong escape is *runnable* and misleading rather than obviously broken; +/// control bytes would still reach the terminal. The canonical fallback is the +/// branch that already exists for "this line cannot be used", and it is +/// guaranteed to report a cause. The user's own line stays visible in the +/// human-readable sentence beside the command (and in the log tail printed with +/// it), so nothing is lost but the copy-paste convenience. +/// +/// Lives here rather than in one engine because two surfaces build that command +/// from the same untrusted text — the vLLM engine's startup-failure hint and the +/// `rocm serve` summary's OOM note — and a guard that protects only one of them +/// is the bug it was written to prevent. +#[must_use] +pub fn quotable_in_single_quotes(symptom: &str) -> bool { + !symptom.contains('\'') && !symptom.chars().any(char::is_control) +} + +/// Removes ANSI escape sequences and any remaining control characters, so a +/// colourised or bell-bearing log line cannot repaint the user's terminal from +/// inside rocm-cli's own error message. +/// +/// This is for text rocm-cli *echoes*; text rocm-cli renders into a command the +/// user is told to run is gated with [`quotable_in_single_quotes`] instead, so a +/// control-bearing line is rejected rather than rewritten into a lookalike. +#[must_use] +pub fn strip_terminal_control_sequences(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + let mut chars = line.chars().peekable(); + while let Some(c) = chars.next() { + if c == '\u{1b}' { + if chars.peek() == Some(&'[') { + // CSI (what a colourised logger emits): skip the parameter and + // intermediate bytes up to and including the final byte. + chars.next(); + for next in chars.by_ref() { + if ('\u{40}'..='\u{7e}').contains(&next) { + break; + } + } + } else { + // Any other escape: drop the byte it introduces too. + chars.next(); + } + continue; + } + if !c.is_control() { + out.push(c); + } + } + out +} + /// Locate `amd-smi` inside the bin directories of the newest managed ROCm SDK /// runtime recorded in the registry. The binary ships with the TheRock wheel /// (under the SDK `bin_path` and/or the venv `install_root/bin`) and is not on @@ -12337,6 +12407,51 @@ last_installed_runtime_id = "therock-release" assert!(!vllm_log_shows_oom("")); } + #[test] + fn quotable_in_single_quotes_rejects_quote_and_control_bearing_symptoms() { + // The ordinary case the guard exists for: Python error text with an + // apostrophe, which would close the shell quote in the printed command. + assert!(!quotable_in_single_quotes( + "vllm: torch.OutOfMemoryError: GPU 0 can't allocate the model's weights" + )); + assert!(!quotable_in_single_quotes( + "vllm: failed to load model 'foo/bar'" + )); + // Terminal control bytes from vLLM's colourised logger. + assert!(!quotable_in_single_quotes( + "vllm: \u{1b}[31mRuntimeError: HIP out of memory\u{1b}[0m" + )); + assert!(!quotable_in_single_quotes("vllm: out of memory\u{7}")); + assert!(!quotable_in_single_quotes("vllm: out of\nmemory")); + // The canonical fallback must always be usable, or the rejection branch + // would have nowhere to go. + assert!(quotable_in_single_quotes(VLLM_OOM_CANONICAL_SYMPTOM)); + assert!(quotable_in_single_quotes( + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." + )); + } + + #[test] + fn strip_terminal_control_sequences_removes_whole_escape_sequences() { + // Pin the exact rendering, not the absence of fragments: a stripper that + // dropped only the escape byte and the `[` it introduces would leave + // `31m`/`0m` behind, which carries no control byte and contains neither + // `[31m` nor `[0m`, so every absence check would still pass. + assert_eq!( + strip_terminal_control_sequences( + "\u{1b}[31mRuntimeError: HIP out of memory\u{1b}[0m\u{7}" + ), + "RuntimeError: HIP out of memory" + ); + // A non-CSI escape takes the byte it introduces with it. + assert_eq!(strip_terminal_control_sequences("a\u{1b}Bc"), "ac"); + // Text with nothing to strip is returned unchanged. + assert_eq!( + strip_terminal_control_sequences(VLLM_OOM_CANONICAL_SYMPTOM), + VLLM_OOM_CANONICAL_SYMPTOM + ); + } + #[test] fn combine_amd_gpu_counts_prefers_compute_authoritative_kfd() { // KFD is compute-authoritative: a nonzero KFD count wins, and DRM must not diff --git a/docs/vllm.md b/docs/vllm.md index 934a6d391..ae5fcf659 100644 --- a/docs/vllm.md +++ b/docs/vllm.md @@ -176,7 +176,10 @@ a tiny model. rocm-cli helps in three ways: The printed command quotes your actual failing line when it can be rendered as one intact single-quoted argument; a line carrying an apostrophe or terminal control bytes falls back to the canonical symptom below rather than handing you - a command whose quoting the log text broke. + a command whose quoting the log text broke. The same rule applies to the + `rocm serve` summary's out-of-memory note, which builds the same command from + the same engine log — both surfaces share one guard, so neither can hand you a + half-quoted command the other rejects. Explicitly, the workaround for an OOM on a shared card is: diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index dc785c896..58c9fb2d5 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -2248,9 +2248,10 @@ fn oom_utilization_hint(log_tail: &str) -> String { let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); // The line is subprocess output, so it is echoed only after the terminal // control bytes vLLM's colourised logger emits are removed. - let symptom_line = - strip_terminal_control_sequences(symptom.strip_prefix("vllm: ").unwrap_or(&symptom)); - let symptom = if quotable_in_single_quotes(&symptom) { + let symptom_line = rocm_core::strip_terminal_control_sequences( + symptom.strip_prefix("vllm: ").unwrap_or(&symptom), + ); + let symptom = if rocm_core::quotable_in_single_quotes(&symptom) { symptom } else { rocm_core::VLLM_OOM_CANONICAL_SYMPTOM.to_owned() @@ -2262,57 +2263,6 @@ fn oom_utilization_hint(log_tail: &str) -> String { ) } -/// Whether `symptom` can be placed inside a `'...'` shell word verbatim. -/// -/// The value is untrusted subprocess output (the vLLM startup log tail) and the -/// message it lands in invites the user to paste the command into a shell, so a -/// bare `'` would close the quote and let text nobody vetted become shell -/// syntax. An apostrophe is routine in Python error text (`can't allocate`, -/// `model 'foo'`), and a line only has to mention running out of memory to be -/// selected, so this is an ordinary case rather than an exotic one. -/// -/// Rejecting instead of escaping (`'` -> `'\''`) is deliberate. Escaping keeps -/// the exact bytes but yields a command a reader cannot check by eye, and a -/// wrong escape is *runnable* and misleading rather than obviously broken; -/// control bytes would still reach the terminal. The canonical fallback is the -/// branch that already exists for "this line cannot be used", and it is -/// guaranteed to report a cause. The user's own line stays visible in the -/// human-readable sentence above the command (and in the log tail printed with -/// it), so nothing is lost but the copy-paste convenience. -fn quotable_in_single_quotes(symptom: &str) -> bool { - !symptom.contains('\'') && !symptom.chars().any(char::is_control) -} - -/// Removes ANSI escape sequences and any remaining control characters, so a -/// colourised or bell-bearing log line cannot repaint the user's terminal from -/// inside rocm-cli's own error message. -fn strip_terminal_control_sequences(line: &str) -> String { - let mut out = String::with_capacity(line.len()); - let mut chars = line.chars().peekable(); - while let Some(c) = chars.next() { - if c == '\u{1b}' { - if chars.peek() == Some(&'[') { - // CSI (what a colourised logger emits): skip the parameter and - // intermediate bytes up to and including the final byte. - chars.next(); - for next in chars.by_ref() { - if ('\u{40}'..='\u{7e}').contains(&next) { - break; - } - } - } else { - // Any other escape: drop the byte it introduces too. - chars.next(); - } - continue; - } - if !c.is_control() { - out.push(c); - } - } - out -} - /// Polls the vLLM endpoint until it reports the model is loaded, or times out. /// Uses a monotonic clock (`Instant`) so wall-clock adjustments cannot corrupt the /// timeout, and surfaces an early process exit immediately instead of waiting out @@ -2928,7 +2878,7 @@ mod tests { // leaves `31m`/`0m` behind, which contains neither `[31m` nor `[0m` and // carries no control byte, so every absence check still passes. assert_eq!( - strip_terminal_control_sequences(log_tail), + rocm_core::strip_terminal_control_sequences(log_tail), "RuntimeError: HIP out of memory", "the ANSI sequence must be removed whole, not just its escape byte" ); From 40c3567ec334b6446f7544601fa53419ff281dbc Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 13:57:47 +0300 Subject: [PATCH 06/11] fix(serve): stop the pre-launch hint suppressing the whole OOM note (eai-8059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the review round on PR #284. serve summary: `append_oom_serve_note` returned early whenever the shared `--gpu-memory-utilization` hint was already in the notes, which discarded the entire OOM note on the feature's most likely trigger — low VRAM warned about pre-launch, user proceeds, vLLM OOMs. What was lost is not a duplicated fragment: the confirmation that this attempt really did run out of GPU memory, the "if the model does not fit, lowering the reservation will not help" branch, and `rocm diagnose --symptom` with the user's own failing line. De-duplication now happens at composition time in `oom_memory_note`, which omits the hint sentence only. The unit test that asserted `notes == pre_launch_notes` cemented the suppression as the specification; it now pins "the hint appears exactly once, and the rest of the note is still appended". rocm-core: restore the `--gpu-memory-utilization` range wording a merge from a pre-fix tree silently reverted. `parse_gpu_memory_utilization` rejects `<= 0` and accepts `1`, so `<0-1>` advertised a value the same tool refuses. The existing pin only checked the worked example (`0.5`), so extend it to the range text. rocm-core: drop the unreachable `VLLM_OOM_CANONICAL_SYMPTOM` fallback in `vllm_oom_diagnose_symptom`. Both callers pre-gated on `vllm_log_shows_oom` and the `.find()` re-evaluated the byte-identical predicate, so it could never fire. The function now returns `Option` and its `None` *is* the "no OOM in this tail" answer, so the callers use it as the gate instead of running the same predicate over the same string twice. The quotability fallback beside it is genuinely reachable and is unchanged. e2e: delete the duplicated `extra_env` loop in `tui_driver`. The surviving loop runs before the provider-credential `env_remove` calls; the duplicate ran after them, so an `extra_env` entry naming a credential key could have defeated the strip and selected a cloud backend for a journey meant to be deterministic local chat. Both current callers pass non-credential keys, so behaviour is unchanged. Also non-blocking review items: correct the two comments that called the `any_live_managed_service_for_model` pre-gate cheap (its condition goes through `load_managed_services`, which probes and rewrites every record before the model filter); document that the OOM guidance is interactive- only; route the interactive-summary wait through `tui_driver::default_timeout()` so `E2E_TUI_TIMEOUT_SECS` applies; assert both knobs in the serve-24 step that promises "knobs"; document `@requires-oom-fault-injection` in the e2e README; and cross-reference the `ROCM_E2E_SIMULATE_OOM_LAUNCH` producer and consumer. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 106 ++++++++++++++---- apps/rocm/src/serve_summary.rs | 105 +++++++++++++++-- crates/rocm-core/src/fix.rs | 24 ++++ crates/rocm-core/src/lib.rs | 79 ++++++++++--- engines/vllm/src/lib.rs | 11 +- tests/e2e-cucumber/README.md | 1 + tests/e2e-cucumber/tests/e2e/serving_steps.rs | 40 +++++-- tests/e2e-cucumber/tests/e2e/tui_driver.rs | 11 +- 8 files changed, 310 insertions(+), 67 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 990eddd1b..95e7e92b3 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -5378,11 +5378,20 @@ fn serve(args: ServeArgs) -> Result<()> { // engine enforces the same rule as a backstop. // // The precise contract, since the reuse detection above is the one thing that - // can precede this bail: engine work runs first only when a live managed - // service already matches this engine and model — i.e. only when this - // invocation is about to reuse it and legitimately skip the bail. When no such - // service exists (the ordinary launch, and every no-GPU refusal path) the - // block above is skipped entirely and this is still the first thing that runs. + // can precede this bail: *engine* work — the `ResolveModel` round-trip and any + // self-managed engine install — runs first only when a live managed service + // already matches this engine and model, i.e. only when this invocation is + // about to reuse it and legitimately skip the bail. When no such service + // exists (the ordinary launch, and every no-GPU refusal path) that block's + // body is skipped. + // + // Its *condition* is not free, though: `any_live_managed_service_for_model` + // goes through `load_managed_services`, which refreshes every service record — + // including records for unrelated models, since the model filter is applied + // afterwards — so a stale `ready`/`running` record costs an endpoint listing + // probe and possibly an inference probe, plus a `record.write()`, before this + // bail is reached. That is bounded and paid only when such records exist, but + // it is not "nothing runs before the pre-flight". // // Skipped for cpu_only and when reusing an already-running service (nothing is // launched); permissive when availability cannot be probed on this platform @@ -5647,6 +5656,14 @@ fn serve(args: ServeArgs) -> Result<()> { // traceback only in its own log. Read that log tail so the summary can // name the memory knobs, since the pre-launch low-VRAM warning cannot // fire without amd-smi/rocm-smi telemetry. + // + // Note the asymmetry this sits inside: `summary_mode` is + // `background && stdout().is_terminal()`, so the guidance reaches an + // interactive user only. A scripted / CI / assistant-driven run takes + // `print_managed_launch_plain`, which prints `readiness: {status}` and + // no notes at all — that path is machine-readable by design and is not + // the place to grow prose, but it does mean the same failed launch is + // explained in one invocation and not the other. let notes = append_oom_serve_note( notes, engine_serves_vllm, @@ -5832,9 +5849,16 @@ fn simulate_oom_managed_launch( /// launched the process (`already_running` is excluded) so healthy deployments, /// unrelated failures, and an unrelated invocation that merely reused an /// already-live service are never misattributed. The OOM-signature check in -/// [`serve_summary::oom_memory_note`] narrows it further. Also skips a note -/// whose hint text is already present in `notes` (the pre-launch low-VRAM -/// warning may have added it) so the same fix is never printed twice. +/// [`serve_summary::oom_memory_note`] narrows it further. +/// +/// When the pre-launch low-VRAM warning already put the shared +/// `--gpu-memory-utilization` hint in `notes`, the *fragment* is dropped from +/// the new note rather than the note being suppressed: low VRAM leading to an +/// OOM is exactly the case this note exists for, and everything else it carries +/// — the confirmation that this attempt really did run out of GPU memory, the +/// "if the model doesn't fit, lowering the reservation won't help" branch, and +/// the `rocm diagnose --symptom` command with the user's real failing line — is +/// absent from the pre-launch guess. fn append_oom_serve_note( mut notes: Vec, engine_is_vllm: bool, @@ -5845,15 +5869,13 @@ fn append_oom_serve_note( if !engine_is_vllm || already_running || !serve_summary::serve_failed_to_become_ready(status) { return notes; } - // Cheap guard first: if the shared hint is already in `notes` (added by the - // pre-launch low-VRAM warning) there is nothing to add, so skip the log read - // entirely. + // Whether the pre-launch low-VRAM warning already printed the shared hint. + // This de-duplicates that one fragment at composition time; it is not a + // reason to skip the note, which carries the OOM confirmation and the + // diagnose command the pre-launch warning has no way to know about. let already_hinted = notes .iter() .any(|note| note.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT)); - if already_hinted { - return notes; - } let Some(log_path) = log_path else { return notes; }; @@ -5867,7 +5889,7 @@ fn append_oom_serve_note( "service log", ) .join("\n"); - if let Some(note) = serve_summary::oom_memory_note(status, &log_tail) { + if let Some(note) = serve_summary::oom_memory_note(status, &log_tail, already_hinted) { notes.push(note); } notes @@ -17129,14 +17151,20 @@ fn existing_live_managed_service( /// `model_ref`, decided from the records alone — no canonical model id, and so /// no engine round-trip, required. /// -/// Cheap pre-gate for the reuse detection in `serve`. Everything that check does -/// is real engine work: a `ResolveModel` round-trip and, for a self-managing +/// Pre-gate for the reuse detection in `serve`. Everything that check does is +/// real engine work: a `ResolveModel` round-trip and, for a self-managing /// engine, an [`ensure_self_managed_engine_ready`] that may print /// "Preparing for GPU serving..." and install. That work runs ahead of /// the no-usable-GPU pre-flight, so it must be reserved for invocations that can /// actually reuse something: keying on the engine alone let a live service for an /// unrelated model pull an install in front of the bail on a GPU-less host. /// +/// Cheap only *relative* to what it guards — it is not free. `load_managed_services` +/// refreshes liveness for every record before this function's model filter is +/// applied, so each live record can cost an endpoint listing probe, an inference +/// probe, and a `record.write()`. It buys no engine round-trip and no install; +/// it does not buy "no I/O". +/// /// Matching uses [`service_model_names_match`] — the same lenient relation the /// service-listing surfaces already use to tie a user-typed name to a record — so /// a short-vs-canonical spelling still reaches the probe. The probe then decides @@ -26891,10 +26919,13 @@ install therock"; } #[test] - fn append_oom_serve_note_does_not_repeat_a_hint_already_in_the_notes() { - // When the pre-launch low-VRAM warning already carried the shared - // utilization hint, a post-failure OOM confirmation must not print the - // exact same hint text a second time. + fn append_oom_serve_note_prints_the_shared_hint_exactly_once() { + // The pre-launch low-VRAM warning fired, the user proceeded, and the + // launch then OOMed -- the exact causal chain this note exists for. The + // shared hint must not be printed twice, but the note itself must still + // appear: it is the only place that confirms the attempt *did* run out + // of GPU memory, carries the model-too-large branch, and hands over the + // diagnose command with the user's own failing line. let log_path = std::env::temp_dir().join(format!("rocm-oom-note-{}-dedup.log", std::process::id())); fs::write( @@ -26914,9 +26945,38 @@ install therock"; Some(&log_path), ); let _ = fs::remove_file(&log_path); + + // The hint text appears exactly once across the whole summary. + let hinting = notes + .iter() + .filter(|note| note.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT)) + .count(); assert_eq!( - notes, pre_launch_notes, - "the hint must not be duplicated once it is already present: {notes:?}" + hinting, 1, + "the shared hint must be printed exactly once: {notes:?}" + ); + + // ...and the OOM note is still added, with the content the pre-launch + // warning cannot carry. + assert_eq!( + notes.len(), + pre_launch_notes.len() + 1, + "the OOM note must still be appended, not suppressed: {notes:?}" + ); + let oom_note = notes.last().expect("the OOM note is the appended note"); + assert!( + oom_note.contains("ran out of GPU memory"), + "the note must confirm this attempt really did OOM: {oom_note}" + ); + assert!( + oom_note.contains("smaller or quantized model"), + "the model-too-large branch must survive de-duplication: {oom_note}" + ); + assert!( + oom_note.contains( + "rocm diagnose --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory." + ), + "the note must route the user's real failing line to diagnose: {oom_note}" ); } diff --git a/apps/rocm/src/serve_summary.rs b/apps/rocm/src/serve_summary.rs index a0784a09b..a3bc4d592 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -208,10 +208,23 @@ pub(crate) fn serve_failed_to_become_ready(status: &str) -> bool { /// earlier OOM for a later one), matching the `rocm diagnose` vLLM-OOM entry it /// then routes the user to. It shares /// [`rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT`] verbatim with the pre-launch -/// low-VRAM note so both surfaces point at the same fix and the pre-launch -/// warning can dedup against it. -pub(crate) fn oom_memory_note(status: &str, log_tail: &str) -> Option { - if !serve_failed_to_become_ready(status) || !rocm_core::vllm_log_shows_oom(log_tail) { +/// low-VRAM note so both surfaces point at the same fix. +/// +/// `hint_already_present` says that shared hint is already in the summary's +/// notes (the pre-launch low-VRAM warning added it), and de-duplicates *that +/// fragment only*: the rest of the note is new information the pre-launch guess +/// does not carry — that this attempt really did run out of GPU memory rather +/// than might, the "if the model doesn't fit, lowering the reservation won't +/// help" branch, and the `rocm diagnose --symptom` command with the user's own +/// failing line. Low VRAM leading to an OOM is the causal chain this note exists +/// for, so suppressing the whole note there would silence it on its most likely +/// trigger. +pub(crate) fn oom_memory_note( + status: &str, + log_tail: &str, + hint_already_present: bool, +) -> Option { + if !serve_failed_to_become_ready(status) { return None; } // The symptom is vLLM's own subprocess output and it lands inside a @@ -221,18 +234,28 @@ pub(crate) fn oom_memory_note(status: &str, log_tail: &str) -> Option { // [`rocm_core::quotable_in_single_quotes`] for why this rejects rather than // escapes. The engine's startup-failure hint guards the same text the same // way. - let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); + // + // `None` is also the "this tail shows no OOM" answer, so it doubles as the + // OOM gate: asking `vllm_log_shows_oom` first would evaluate the same + // predicate over the same string twice. + let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail)?; let symptom = if rocm_core::quotable_in_single_quotes(&symptom) { symptom } else { rocm_core::VLLM_OOM_CANONICAL_SYMPTOM.to_owned() }; + // Printed as its own sentence, or omitted when the pre-launch warning + // already printed the identical text. + let hint = if hint_already_present { + String::new() + } else { + format!("{} ", rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT) + }; Some(format!( - "the serve attempt ran out of GPU memory. {} If the model simply does not fit in this \ + "the serve attempt ran out of GPU memory. {hint}If the model simply does not fit in this \ GPU's VRAM, lowering the reservation will not help — serve a smaller or quantized model \ instead (rocm-cli serves one model on a single GPU). To have the tool pick the \ - case-appropriate fix, run `rocm diagnose --symptom '{symptom}'`.", - rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT + case-appropriate fix, run `rocm diagnose --symptom '{symptom}'`." )) } @@ -509,6 +532,7 @@ mod tests { let note = oom_memory_note( "starting", "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + false, ) .expect("an OOM failure must produce a note"); assert!(note.contains("--gpu-memory-utilization"), "{note}"); @@ -531,14 +555,67 @@ mod tests { fn oom_note_is_withheld_for_a_ready_serve_or_a_clean_log() { // A serve that became ready is healthy even if the log mentions memory. assert_eq!( - oom_memory_note("ready", "torch.OutOfMemoryError: HIP out of memory"), + oom_memory_note("ready", "torch.OutOfMemoryError: HIP out of memory", false), None ); // A failure with no OOM signature must not be given memory advice. assert_eq!( - oom_memory_note("starting", "OSError: model weights not found"), + oom_memory_note("starting", "OSError: model weights not found", false), + None + ); + // ...and neither case becomes advisable just because the pre-launch + // low-VRAM warning already fired. + assert_eq!( + oom_memory_note("ready", "torch.OutOfMemoryError: HIP out of memory", true), None ); + assert_eq!( + oom_memory_note("starting", "OSError: model weights not found", true), + None + ); + } + + #[test] + fn the_shared_hint_is_de_duplicated_without_losing_the_rest_of_the_oom_note() { + // The pre-launch low-VRAM warning already printed the shared hint + // verbatim, and low VRAM leading to an OOM is the causal chain this note + // exists for -- so what must be dropped is that one fragment, not the + // note. Everything else it carries is information the pre-launch guess + // does not have: that this attempt actually ran out of GPU memory, the + // "if the model doesn't fit, lowering the reservation won't help" + // branch, and the diagnose command with the user's real failing line. + let log_tail = "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB."; + let note = oom_memory_note("starting", log_tail, true) + .expect("an OOM failure must still carry a note when the hint was already printed"); + + // The hint the pre-launch warning printed appears zero further times... + assert!( + !note.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT), + "the shared hint must not be printed a second time: {note}" + ); + // ...and the content only this note has must all survive. + assert!( + note.contains("ran out of GPU memory"), + "the note must confirm this attempt really did OOM: {note}" + ); + assert!( + note.contains("smaller or quantized model"), + "the model-too-large branch is absent from the pre-launch hint: {note}" + ); + assert_eq!( + quoted_symptom_argument(¬e), + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + "the diagnose command must carry the user's own failing line: {note}" + ); + + // Counted across the whole summary, the hint is printed exactly once: + // the pre-launch note keeps it, the OOM note does not repeat it. + let pre_launch = rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT.to_owned(); + let printed = [pre_launch, note] + .iter() + .filter(|line| line.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT)) + .count(); + assert_eq!(printed, 1, "the shared hint must appear exactly once"); } /// The `--symptom` value the note actually hands the user, read back out of @@ -567,7 +644,8 @@ mod tests { "ERROR 09-14 12:00:01 engine.py:389] torch.OutOfMemoryError: HIP out of memory. ", "Tried to allocate 7.21 GiB. GPU 0 can't allocate the model's weights.\n" ); - let note = oom_memory_note("starting", log_tail).expect("an OOM failure must carry a note"); + let note = + oom_memory_note("starting", log_tail, false).expect("an OOM failure must carry a note"); // The rendered command must be one intact single-quoted argument: no // byte the vLLM subprocess printed may close the quote and land outside @@ -605,7 +683,8 @@ mod tests { // vLLM's logger colourises; an ANSI-coloured OOM line must not repaint // the user's terminal from inside rocm-cli's own serve summary. let log_tail = "\u{1b}[31mRuntimeError: HIP out of memory\u{1b}[0m\u{7}"; - let note = oom_memory_note("starting", log_tail).expect("an OOM failure must carry a note"); + let note = + oom_memory_note("starting", log_tail, false).expect("an OOM failure must carry a note"); assert!( !note.chars().any(|c| c.is_control() && c != '\n'), "no control byte may survive into the printed note: {note:?}" @@ -630,6 +709,7 @@ mod tests { let note = oom_memory_note( "starting", "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + false, ) .expect("an OOM failure must carry a note"); assert_eq!( @@ -645,6 +725,7 @@ mod tests { if let Some(note) = oom_memory_note( &summary.status, "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + false, ) { summary.notes.push(note); } diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 5a86ba5f4..ede3b8280 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -1589,6 +1589,30 @@ mod tests { ); } + #[test] + fn the_utilization_hint_states_the_bound_the_parser_accepts() { + // `rocm serve`'s `parse_gpu_memory_utilization` rejects `<= 0` and + // accepts `1`, so the domain is (0, 1]. `<0-1>` advertises `0`, and this + // hint prints in three places (pre-launch note, engine OOM hint, and the + // `fix-16-vllm-oom` summary), so a user who OOMs, reads the tool's own + // advice and passes `0` is rejected by the same tool. + // + // Pinned because the wording was silently reverted once: it was fixed on + // this branch, then a merge resolved the same line from a pre-fix tree. + // Nothing went red, because the only pin on this const checked the + // worked example (`0.5`) and not the range text. + const FLAG: &str = "--gpu-memory-utilization"; + let hint = crate::VLLM_GPU_MEMORY_UTILIZATION_HINT; + assert!( + hint.contains(&format!("{FLAG} ")), + "the hint must state the bound `{FLAG}` actually accepts:\n{hint}" + ); + assert!( + !hint.contains("<0-1>"), + "`<0-1>` wrongly advertises 0, which `{FLAG}` rejects:\n{hint}" + ); + } + #[test] fn auto_applicable_recipes_have_a_runner() { for r in RECIPES { diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index ace8e30bd..8199afb0c 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -7768,9 +7768,17 @@ pub fn resolve_amd_smi_binary() -> OsString { /// weights of most models people actually serve, so it trades one startup /// failure for another. Pinned by /// `the_utilization_hint_example_matches_the_recipe_command`. +/// +/// The stated bound must be the one `rocm serve` actually accepts: +/// `parse_gpu_memory_utilization` rejects `<= 0` and accepts `1`, so the domain +/// is `(0, 1]` and the older `<0-1>` wording wrongly advertised `0`. Pinned by +/// `the_utilization_hint_states_the_bound_the_parser_accepts`, because this +/// wording has already been reverted once by a merge that resolved the line from +/// a pre-fix tree. pub const VLLM_GPU_MEMORY_UTILIZATION_HINT: &str = "vLLM reserves ~90% of the GPU's total VRAM by default; on a shared or busy GPU this can \ collide with memory already in use. Lower the reservation with `--gpu-memory-utilization \ - <0-1>` (e.g. 0.5 for a small model), or target a less-busy GPU with `--gpu `."; + ` (e.g. 0.5 for a small model), or target a less-busy \ + GPU with `--gpu `."; /// Whether a vLLM startup-log tail carries a genuine out-of-memory failure. /// @@ -7797,14 +7805,21 @@ pub fn vllm_log_shows_oom(log: &str) -> bool { }) } -/// The `rocm diagnose --symptom` string to route a vLLM OOM log tail to. +/// The `rocm diagnose --symptom` string to route a vLLM OOM log tail to, or +/// `None` when the tail carries no OOM line to route. +/// +/// Returns the user's actual failing line (so `diagnose` echoes their real +/// error), selected with the *same* rule [`vllm_log_shows_oom`] classifies the +/// tail with, so whatever comes back is diagnosable by construction. Shared by +/// the vLLM engine's post-failure hint and the `rocm` CLI serve summary so both +/// surfaces route to `diagnose` identically instead of hand-rolling the line +/// selection twice. /// -/// Prefers the user's actual failing line (so `diagnose` echoes their real -/// error) and falls back to [`VLLM_OOM_CANONICAL_SYMPTOM`] when no single line -/// clears the checker's threshold, so the printed command always reports a -/// cause. Shared by the vLLM engine's post-failure hint and the `rocm` CLI serve -/// summary so both surfaces route to `diagnose` identically instead of -/// hand-rolling the line selection twice. +/// `None` *is* the "this tail is not an OOM" answer, so callers use it as the +/// OOM gate directly rather than testing [`vllm_log_shows_oom`] first and then +/// asking for the line: the two are the same predicate over the same string, so +/// a caller that did both would evaluate it twice and carry a branch that can +/// never be taken. /// /// The return value is *log text*, not a shell-safe token: some callers only /// display it. A caller that renders it inside a quoted command must gate it on @@ -7813,16 +7828,14 @@ pub fn vllm_log_shows_oom(log: &str) -> bool { /// instead would impose the command-builder's constraint on the display-only /// callers, silently withholding the user's real error from text that is never /// pasted anywhere. -pub fn vllm_oom_diagnose_symptom(log_tail: &str) -> String { +#[must_use] +pub fn vllm_oom_diagnose_symptom(log_tail: &str) -> Option { log_tail .lines() .rev() .map(str::trim) .find(|line| !line.is_empty() && vllm_log_shows_oom(line)) - .map_or_else( - || VLLM_OOM_CANONICAL_SYMPTOM.to_owned(), - |line| format!("vllm: {line}"), - ) + .map(|line| format!("vllm: {line}")) } /// Whether `symptom` can be placed inside a `'...'` shell word verbatim. @@ -12635,6 +12648,46 @@ last_installed_runtime_id = "therock-release" assert!(!vllm_log_shows_oom("")); } + #[test] + fn vllm_oom_diagnose_symptom_selects_the_failing_line_or_reports_no_oom() { + // The selector and the classifier are one predicate: a tail + // `vllm_log_shows_oom` accepts always yields a line, and a tail it + // rejects always yields `None` — which is what lets callers use this as + // the OOM gate instead of asking the same question twice. + let tail = concat!( + " File \"/opt/vllm/worker.py\", line 212, in load_model\n", + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.\n", + "INFO shutting down worker\n" + ); + assert_eq!( + vllm_oom_diagnose_symptom(tail).as_deref(), + Some("vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB."), + "the user's own failing line must be routed into `rocm diagnose --symptom`" + ); + // Whatever comes back must be diagnosable, or the printed command would + // report no known cause. + let symptom = vllm_oom_diagnose_symptom(tail).expect("an OOM tail selects a line"); + assert!(diagnose::vllm_oom_symptom_is_diagnosable(&symptom)); + // The *last* OOM line wins: a tail ends with the failure that killed the + // launch, and an earlier retry's line would misreport it. + assert_eq!( + vllm_oom_diagnose_symptom("RuntimeError: hipErrorOutOfMemory\nHIP OUT OF MEMORY\n") + .as_deref(), + Some("vllm: HIP OUT OF MEMORY") + ); + // No OOM line, no symptom — including the sub-threshold shapes the + // shared classifier rejects, which must not be reported as a cause. + assert_eq!( + vllm_oom_diagnose_symptom("OSError: model weights not found"), + None + ); + assert_eq!( + vllm_oom_diagnose_symptom("Out of memory: Killed process 4242 (python)"), + None + ); + assert_eq!(vllm_oom_diagnose_symptom(""), None); + } + #[test] fn quotable_in_single_quotes_rejects_quote_and_control_bearing_symptoms() { // The ordinary case the guard exists for: Python error text with an diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 58c9fb2d5..1c7ecb506 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -2233,9 +2233,6 @@ fn startup_log_context(log_path: Option<&Path>) -> String { /// with the `rocm` CLI's pre-launch low-VRAM note so both surfaces point the /// user at the same fix rather than drifting into different phrasing. fn oom_utilization_hint(log_tail: &str) -> String { - if !rocm_core::vllm_log_shows_oom(log_tail) { - return String::new(); - } // Route the user's *actual* failing line into the `--symptom` example when // it can be rendered as one intact single-quoted argument; otherwise fall // back to the canonical symptom so the printed command always reports a @@ -2245,7 +2242,13 @@ fn oom_utilization_hint(log_tail: &str) -> String { // of hand-rolling the selection twice; it classifies each line with the // *same* rule the diagnose checker uses, so whatever it returns is // diagnosable by construction. - let symptom = rocm_core::vllm_oom_diagnose_symptom(log_tail); + // + // No line means the tail carries no OOM, so the helper's `None` is the OOM + // gate too: asking `vllm_log_shows_oom` first would evaluate the same + // predicate over the same string a second time. + let Some(symptom) = rocm_core::vllm_oom_diagnose_symptom(log_tail) else { + return String::new(); + }; // The line is subprocess output, so it is echoed only after the terminal // control bytes vLLM's colourised logger emits are removed. let symptom_line = rocm_core::strip_terminal_control_sequences( diff --git a/tests/e2e-cucumber/README.md b/tests/e2e-cucumber/README.md index 751f7b0cf..88570fa89 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -140,6 +140,7 @@ Scenarios carry stable-id and capability tags: | `@requires-wsl` | The inverse: premise **is** a WSL2 host. Resolves to **skip** on native Linux, native Windows, and everything else. | | `@requires-engine:` | Pins the serve engine. Resolves to skip where that engine can't start (e.g. vLLM on a lemonade-only Strix host). | | `@requires-os:` | Premise is OS-specific; skip on other OSes. | +| `@requires-oom-fault-injection` | Needs the `rocm` binary under test to carry the test-only `e2e-oom-fault-injection` hook, which fabricates a managed launch that runs out of GPU memory so the OOM guidance can be verified without a GPU. Unlike every other capability this one **cannot be probed** — no product command exposes it — so `xtask e2e` reports it via `ROCM_E2E_OOM_FAULT_INJECTION=1`, set only when xtask compiled the binary itself with the feature. A prebuilt `ROCM_CLI_BINARY` (the self-hosted lanes' shipping release build) has the hook compiled out, so these scenarios resolve to **skip** there and run on the mock lane. | | `@serve-timeout:` | Lengthen the serve-readiness wait for a genuinely slow serve (e.g. a large model). | | `@nightly` | Expensive scenario skipped by default; included when `E2E_INCLUDE_NIGHTLY=1`. | | `@lifecycle` | Expensive, OS-mutating release-lifecycle scenario (packaging + real installer + install/uninstall). Skipped by default; included when `E2E_INCLUDE_LIFECYCLE=1`. `E2E_ONLY_LIFECYCLE=1` selects only this set without bypassing expectation resolution. | diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index c2945f169..c466c3914 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -9,14 +9,23 @@ use std::time::{Duration, Instant}; use cucumber::{given, then, when}; use crate::E2eWorld; -use crate::e2e::tui_driver::TuiSession; +use crate::e2e::tui_driver::{TuiSession, default_timeout}; use e2e_cucumber::mock_server::{MockServer, ServiceRecordOptions, write_service_record_with}; use e2e_cucumber::serve_log::{ ServeAttempt, archive_service_log, serve_attempt_report, service_log_tail, }; const OOM_GUIDANCE_MODEL: &str = "e2e/oom-model"; -const INTERACTIVE_SUMMARY_TIMEOUT: Duration = Duration::from_secs(30); + +/// How long to wait for an interactive (PTY) serve summary to render and exit. +/// +/// Routed through the suite-wide [`default_timeout`] rather than a fixed literal +/// so an operator can raise it with `E2E_TUI_TIMEOUT_SECS` on a contended +/// self-hosted lane without a code change — the same reason every other PTY wait +/// uses it. +fn interactive_summary_timeout() -> Duration { + default_timeout() +} /// Model id for the positive OOM-launch scenario. A distinct id from /// [`OOM_GUIDANCE_MODEL`] keeps the two OOM scenarios' service records from ever /// colliding, and marks this one as the launch (not reuse) case. @@ -1142,7 +1151,7 @@ async fn open_oom_serve_summary(world: &mut E2eWorld) { ) .unwrap_or_else(|error| panic!("failed to open interactive serve summary: {error}")); session - .wait_for_exit(INTERACTIVE_SUMMARY_TIMEOUT) + .wait_for_exit(interactive_summary_timeout()) .await .unwrap_or_else(|error| panic!("interactive serve summary failed: {error}")); world.tui = Some(session); @@ -1342,6 +1351,14 @@ async fn open_oom_launch_summary(world: &mut E2eWorld) { // pre-flight is bypassed for the simulated launch, so no GPU is touched; the // launch resolves the model, "spawns", writes an OOM log it owns, and reports // `starting` — exactly the state the memory-guidance note keys on. + // + // The consumer of this variable is `e2e_simulate_oom_launch` in + // `apps/rocm/src/main.rs`. Unlike `OOM_FAULT_INJECTION_ENV` (shared through + // `e2e-report`, whose producer and consumer are both test-side), the + // consumer here is the shipped binary's crate, which must not depend on the + // e2e harness — so the name is spelled out on both sides. A typo fails + // loudly rather than silently skipping: the fault injection would not arm, + // the real GPU pre-flight would bail, and the step below would fail. let mut session = TuiSession::spawn_with_env( world, &[ @@ -1356,7 +1373,7 @@ async fn open_oom_launch_summary(world: &mut E2eWorld) { ) .unwrap_or_else(|error| panic!("failed to open interactive serve summary: {error}")); session - .wait_for_exit(INTERACTIVE_SUMMARY_TIMEOUT) + .wait_for_exit(interactive_summary_timeout()) .await .unwrap_or_else(|error| panic!("interactive serve summary failed: {error}")); world.tui = Some(session); @@ -1387,13 +1404,20 @@ async fn assert_oom_launch_names_knobs(world: &mut E2eWorld) { .expect("no interactive serve summary") .screen_text(); // The actionable fix: lower the reservation or move to a less-busy device. - // Assert on the flag name (whitespace-insensitive, since an 80-column wrap can - // split the token across rows) so a reworded preamble does not mask a dropped - // remediation. + // Assert on the flag names (whitespace-insensitive, since an 80-column wrap + // can split a token across rows) so a reworded preamble does not mask a + // dropped remediation. Both knobs the step promises are checked — the step + // passed while only `--gpu-memory-utilization` was asserted, so dropping + // `--gpu ` from the guidance would not have failed anything. + let flattened = screen_without_whitespace(&screen); assert!( - screen_without_whitespace(&screen).contains("--gpu-memory-utilization"), + flattened.contains("--gpu-memory-utilization"), "the OOM summary must name the memory knob `--gpu-memory-utilization`:\n{screen}" ); + assert!( + flattened.contains("--gpu"), + "the OOM summary must also name the other knob it advertises, `--gpu `:\n{screen}" + ); } /// Collapse a rendered PTY screen to its non-whitespace characters, so an diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 9298ad361..1bb5967c1 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -197,7 +197,10 @@ impl TuiSession { cmd.env(key, value); } // Caller-supplied overrides win over the scenario's own isolation - // (e.g. a `Given` step's HOME/SHELL for state it planted itself). + // (e.g. a `Given` step's HOME/SHELL for state it planted itself), but + // deliberately stay *above* the provider-credential strip below, so no + // `extra_env` entry can reinstate a credential and select a cloud + // backend for a journey that is meant to be deterministic local chat. for (key, value) in extra_env { cmd.env(key, value); } @@ -219,12 +222,6 @@ impl TuiSession { cmd.env("COLUMNS", COLS.to_string()); cmd.env("LINES", ROWS.to_string()); - // Per-child overrides last, so a scenario's explicit variable wins over - // the inherited/isolation environment. - for (key, value) in extra_env { - cmd.env(key, value); - } - let mut child = pair .slave .spawn_command(cmd) From cafc8b0a5a3c94b45650b56f02e93bf1408d8b92 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 17 Sep 2026 11:40:40 +0300 Subject: [PATCH 07/11] chore(rocm-core): give the new terminal module the license header CI requires `crates/rocm-core/src/terminal.rs` arrived from the base branch (fcf99156) without the `Copyright`/`SPDX-License-Identifier: MIT` header that `licenserc.toml` requires of every `**/*.rs`, so `License header check (hawkeye)` is red. It is red on PR #251 at the same commit, for the same file, so this is the base's omission surfacing here rather than anything the merge introduced -- fixing it at the source would be better, and this can be dropped from the merge once it lands there. The header is the exact text `licenserc.toml` declares, copied from the sibling modules, and it goes above the `//!` module doc the way `diagnose.rs`, `runtime.rs` and `uv.rs` place theirs. EAI-8059 Signed-off-by: Roman Sirokov --- crates/rocm-core/src/terminal.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/rocm-core/src/terminal.rs b/crates/rocm-core/src/terminal.rs index 26ef0e350..69eff3420 100644 --- a/crates/rocm-core/src/terminal.rs +++ b/crates/rocm-core/src/terminal.rs @@ -1,3 +1,7 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + //! What rocm-cli does with untrusted terminal output: one ECMA-48 walk, and the //! decisions that rest on it. //! From 653d5dbe686ac29952041e9d1b202944ca03746d Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 29 Sep 2026 11:56:45 +0300 Subject: [PATCH 08/11] EAI-8059: adapt the OOM fault-injection expectation test to Included The base modularized `resolve` to take an `Included` struct instead of three positional bools. Git merged both sides textually without a conflict, leaving 284's new call sites on the old arity, so the e2e-cucumber lib test stopped compiling. Both sites meant "no opt-in sets included", which is the all-false `Included` value the surrounding tests already spell out field by field. Signed-off-by: Roman Sirokov --- tests/e2e-cucumber/src/expectation.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index b89a5c60f..2678d48c3 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -1441,7 +1441,17 @@ serve_timeout_secs = 90 let without_hook = cap("mock"); assert!(!without_hook.oom_fault_injection); assert!(matches!( - resolve(&d, &without_hook, &m, false, false, false), + resolve( + &d, + &without_hook, + &m, + Included { + nightly: false, + lifecycle: false, + docker: false, + merge_queue: false + } + ), Expectation::Skip { .. } )); @@ -1449,7 +1459,17 @@ serve_timeout_secs = 90 let mut with_hook = cap("mock"); with_hook.oom_fault_injection = true; assert_eq!( - resolve(&d, &with_hook, &m, false, false, false), + resolve( + &d, + &with_hook, + &m, + Included { + nightly: false, + lifecycle: false, + docker: false, + merge_queue: false + } + ), Expectation::ExpectPass ); } From 90ce2a60a8a1d477c274ac493013eaf71266b20d Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 30 Sep 2026 16:26:39 +0300 Subject: [PATCH 09/11] serve: keep runtime resolution behind the no-usable-GPU bail (eai-8059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `4a108b95` set out to make the GPU fail-fast contract true, but the contract was still false on the ordinary path: this PR had also moved `resolve_engine_selection` / `validate_engine_selection_runtime` from below the bail to above it, and that chain is not side-effect-free. With neither `--runtime-id` nor `--env-id` given — the default — the second call takes `single_ready_runtime_key`, which calls `recover_setup_runtime_registration`, which can `create_dir_all` + `write` a runtime registry manifest and can fail with an unrelated runtime-manifest error. So a plain `rocm serve ` on a GPU-less host could mutate disk, or report a runtime-manifest problem, before reaching "no usable AMD GPU detected". Rather than soften the comment, move the code back. The reuse pre-gate is the only thing that legitimately precedes the bail, so make its record-only check (`any_live_managed_service_for_model`) the outermost condition and resolve the runtime selection *inside* it; every other path — including every no-GPU refusal — now resolves below the bail and reuses nothing. Reproduced with a `.rocm-cli-runtime.json` the setup root cannot parse: before, a masked-GPU `rocm serve` died with "failed to parse .../.rocm-cli-runtime.json"; after, it refuses with "no usable AMD GPU detected". Also records, on `append_oom_serve_note`, why its `already_running` clause stays even though the only producer of `already_running: true` reports `log_path: None`: the two guards assert different things, and the unit test discriminates the clause where the E2E scenario cannot. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 142 +++++++++++++++++++++++++++++------------- 1 file changed, 98 insertions(+), 44 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 442a043cb..e373e4f26 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -6040,13 +6040,6 @@ fn serve(args: ServeArgs) -> Result<()> { } else { rocm_core::usable_amd_gpu_indices() }; - let resolved_selection = resolve_engine_selection( - &config, - &selected_engine, - runtime_id.as_deref(), - env_id.as_deref(), - ); - let resolved_selection = validate_engine_selection_runtime(&paths, resolved_selection)?; // Reusing an already-running managed service launches nothing and pins no // GPU, so it must bypass the GPU-required pre-flight below — the reused // service was already vetted at its own launch, and this invocation does no @@ -6056,41 +6049,66 @@ fn serve(args: ServeArgs) -> Result<()> { // Without a runtime we cannot resolve, so we fall through and the pre-flight // refuses the no-GPU / no-runtime case with its usual message. // - // The pre-gate matches on the model, not just the engine: everything inside - // this block is real engine work (a `ResolveModel` round-trip, and for a - // self-managing engine an `ensure_self_managed_engine_ready` that can print - // "Preparing for GPU serving..." and install), so gating on the - // engine alone let a live service for an *unrelated* model — one this - // invocation can never reuse — drag that work ahead of the no-usable-GPU - // bail on a GPU-less host. + // The outermost condition is `any_live_managed_service_for_model`, and it is + // first for a reason: it reads the managed-service records and nothing else, + // so it is the only thing this block costs on the ordinary launch. Everything + // it guards has side effects that must not precede the bail — + // + // - the pre-gate matches on the model, not just the engine, because the + // probe body is real engine work (a `ResolveModel` round-trip, and for a + // self-managing engine an `ensure_self_managed_engine_ready` that can + // print "Preparing for GPU serving..." and install). Gating on + // the engine alone let a live service for an *unrelated* model — one this + // invocation can never reuse — drag that work ahead of the no-usable-GPU + // bail on a GPU-less host. + // + // - resolving the engine/runtime selection is not free either. + // `validate_engine_selection_runtime` takes `single_ready_runtime_key` + // whenever neither `--runtime-id` nor `--env-id` is given (the default + // path), and that calls `recover_setup_runtime_registration`, which can + // `fs::create_dir_all` + `fs::write` a runtime registry manifest and can + // fail with an unrelated runtime-manifest error. So it stays *inside* + // this block, behind the record-only check, and is computed below the + // bail on every other path. + let mut resolved_selection: Option = None; let mut resolved_model: Option = None; let mut reuse_existing = false; - let can_resolve_model = !cpu_only - && (resolved_selection.runtime_id.is_some() - || resolved_selection.env_id.is_some() - || engine_manages_own_runtime(&selected_engine)); - if can_resolve_model - && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) + if !cpu_only && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) { - if engine_manages_own_runtime(&selected_engine) { - ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; - } - let probe = engine_request::<_, ResolveModelResponse>( - Some(&paths), - &selected_engine, - EngineMethod::ResolveModel, - &ResolveModelRequest { - model_ref: engine_model_ref.clone(), - runtime_id: resolved_selection.runtime_id.clone(), - device_policy: Some(device_policy.clone()), - recipe_override: None, - engine_recipe: engine_recipe.clone(), - }, + let selection = validate_engine_selection_runtime( + &paths, + resolve_engine_selection( + &config, + &selected_engine, + runtime_id.as_deref(), + env_id.as_deref(), + ), )?; - reuse_existing = - existing_live_managed_service(&paths, &selected_engine, &probe.canonical_model_id) - .is_some(); - resolved_model = Some(probe); + let can_resolve_model = selection.runtime_id.is_some() + || selection.env_id.is_some() + || engine_manages_own_runtime(&selected_engine); + if can_resolve_model { + if engine_manages_own_runtime(&selected_engine) { + ensure_self_managed_engine_ready(&paths, &mut config, &selected_engine)?; + } + let probe = engine_request::<_, ResolveModelResponse>( + Some(&paths), + &selected_engine, + EngineMethod::ResolveModel, + &ResolveModelRequest { + model_ref: engine_model_ref.clone(), + runtime_id: selection.runtime_id.clone(), + device_policy: Some(device_policy.clone()), + recipe_override: None, + engine_recipe: engine_recipe.clone(), + }, + )?; + reuse_existing = + existing_live_managed_service(&paths, &selected_engine, &probe.canonical_model_id) + .is_some(); + resolved_model = Some(probe); + } + resolved_selection = Some(selection); } // Fail fast under a GPU-required policy when the host has no usable AMD GPU, // before preparing or launching any engine for *this* model (no wasted engine @@ -6098,12 +6116,13 @@ fn serve(args: ServeArgs) -> Result<()> { // engine enforces the same rule as a backstop. // // The precise contract, since the reuse detection above is the one thing that - // can precede this bail: *engine* work — the `ResolveModel` round-trip and any - // self-managed engine install — runs first only when a live managed service - // already matches this engine and model, i.e. only when this invocation is - // about to reuse it and legitimately skip the bail. When no such service - // exists (the ordinary launch, and every no-GPU refusal path) that block's - // body is skipped. + // can precede this bail: everything with a side effect — the runtime-selection + // resolution (which can write a runtime registry manifest), the `ResolveModel` + // round-trip, and any self-managed engine install — runs first only when a + // live managed service already matches this engine and model, i.e. only when + // this invocation is about to reuse it and legitimately skip the bail. When no + // such service exists (the ordinary launch, and every no-GPU refusal path) + // that block's body is skipped and this is the first thing that runs. // // Its *condition* is not free, though: `any_live_managed_service_for_model` // goes through `load_managed_services`, which refreshes every service record — @@ -6167,6 +6186,28 @@ fn serve(args: ServeArgs) -> Result<()> { } else { None }; + // Resolve which runtime/env this serve will use, now that the host GPU + // pre-condition and `--gpu` have both been checked. This must not move back + // above the bail: `validate_engine_selection_runtime` reaches + // `recover_setup_runtime_registration` on the default (no `--runtime-id` / + // `--env-id`) path, which writes a runtime registry manifest and can fail + // with a runtime-manifest error that has nothing to do with the GPU — so a + // GPU-less `rocm serve ` would mutate disk, or report the wrong + // problem, instead of failing fast. The reuse pre-gate above needs the answer + // early and already paid for it when a reusable service exists; take that + // answer rather than repeating the registry recovery. + let resolved_selection = match resolved_selection { + Some(selection) => selection, + None => validate_engine_selection_runtime( + &paths, + resolve_engine_selection( + &config, + &selected_engine, + runtime_id.as_deref(), + env_id.as_deref(), + ), + )?, + }; if !matches!(device_policy, DevicePolicy::CpuOnly) && resolved_selection.runtime_id.is_none() && resolved_selection.env_id.is_none() @@ -6573,6 +6614,19 @@ fn simulate_oom_managed_launch( /// already-live service are never misattributed. The OOM-signature check in /// [`serve_summary::oom_memory_note`] narrows it further. /// +/// The `already_running` exclusion is deliberately kept even though today's only +/// producer of `already_running: true` — `spawn_managed_engine_child`'s reuse +/// short-circuit — also reports `log_path: None`, so the `log_path` guard below +/// would already catch that one case. The two say different things: `log_path` +/// is a local "there is no log to read", while `already_running` is the +/// attribution rule this function exists to enforce, and it must survive any +/// future reuse path that does carry the reused service's log. The unit test +/// `append_oom_serve_note_ignores_an_already_running_services_log` discriminates +/// it directly — it passes a log path that *does* contain an OOM signature +/// together with `already_running: true` — so dropping the clause turns that +/// test red. The E2E scenario `@id:serve-oom-memory-guidance` cannot: it goes +/// through the real reuse path, where `log_path` is `None`. +/// /// When the pre-launch low-VRAM warning already put the shared /// `--gpu-memory-utilization` hint in `notes`, the *fragment* is dropped from /// the new note rather than the note being suppressed: low VRAM leading to an From d0fee30c1a6c814056619431d35315b7680a5967 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 30 Sep 2026 16:26:52 +0300 Subject: [PATCH 10/11] test(e2e): cover the behaviour the model-keyed reuse pre-gate introduces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `4a108b95` changed what a user sees — on a GPU-less host with a live managed service for an *unrelated* model, `rocm serve` must refuse with "no usable AMD GPU" and must not print "Preparing for GPU serving..." or install an engine — and shipped only unit tests on `any_live_managed_service_for_model`. AGENTS.md is explicit that those prove the function, not the behaviour. serve-13 plants no service so it passes either way, and serve-23 plants the same model. serve-25 plants a live *Lemonade* service for a different model, then serves a Lemonade model: same engine, so the model is the only thing holding the pre-gate shut, and Lemonade because it is the self-managing engine whose preparation is user-visible. Falsified by reverting the model keying — the scenario goes red on "Preparing lemonade for GPU serving..." followed by an embeddable download. `ServiceRecordOptions` grows an `engine` field (default `"vllm"`) so the planted record can name Lemonade without duplicating the on-disk schema. Two review fixes alongside: - `the summary reflects the reused already-running service` matched "already running" against a raw 80-column PTY screen, where a soft wrap between the words breaks it. Route it through `screen_without_whitespace` like its two siblings. - serve-23's comment claimed the planted OOM content was load-bearing. It is not: the only site producing `already_running: true` also sets `log_path: None`, so `append_oom_serve_note` returns at that guard regardless. Say what the scenario actually covers and name the unit test that does discriminate the `already_running` clause. Signed-off-by: Roman Sirokov --- .../features/model_serving.feature | 38 ++++++++- tests/e2e-cucumber/src/mock_server.rs | 8 +- tests/e2e-cucumber/tests/e2e/serving_steps.rs | 83 ++++++++++++++++++- 3 files changed, 123 insertions(+), 6 deletions(-) diff --git a/tests/e2e-cucumber/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index c680f92e6..ddb36f59c 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -284,10 +284,17 @@ Feature: Model serving And the serve plan explains how to lower vLLM's memory reservation # Regression test (EAI-8059 review): re-issuing `serve` against an - # already-running managed service must never blame *this* invocation for - # whatever that other process's log contains, even when it carries a real - # OOM signature and the reused record is not yet "ready" — nothing was - # launched by this invocation, so there is nothing for it to have OOM'd on. + # already-running managed service must never blame *this* invocation for the + # other process's failure — nothing was launched here, so there is nothing for + # it to have OOM'd on. What this scenario covers end-to-end is that the reuse + # summary carries no memory guidance; it does NOT discriminate the + # `already_running` clause of `append_oom_serve_note`'s guard, because the real + # reuse path reports `log_path: None` and the note is withheld on that ground + # alone. The planted OOM log therefore states the premise (a live service whose + # log does carry a real allocator signature) rather than driving the assertion. + # The `already_running` clause itself is pinned by the unit test + # `append_oom_serve_note_ignores_an_already_running_services_log`, which feeds + # it an OOM-bearing log path directly. # Runs on the no-GPU mock host: reusing an already-running managed service # launches nothing and pins no GPU, so `rocm serve` bypasses the GPU-required # pre-flight and reaches the reuse short-circuit even here. It therefore gates @@ -316,3 +323,26 @@ Feature: Model serving When the user opens the interactive serve summary for that launch Then the deployment summary blames this launch for GPU memory And the deployment summary names the GPU memory knobs + + # Regression test for the GPU fail-fast contract (EAI-8059 review). The reuse + # pre-gate that lets an already-running service skip the no-usable-GPU bail used + # to key on the ENGINE alone, so any live managed service was enough to pull the + # reuse probe's engine work — for a self-managing engine, an + # `ensure_self_managed_engine_ready` that prints "Preparing for GPU + # serving..." and downloads an install — in front of the bail. On a GPU-less + # host that is exactly backwards: the invocation can never reuse a service for a + # DIFFERENT model, so it must refuse first and prepare nothing. + # + # Lemonade specifically, because it is the engine that manages its own runtime + # and therefore the one whose preparation is user-visible; the planted record + # names lemonade too, so the engine matches and the MODEL is the only thing + # keeping the pre-gate shut. Scenario 13 already covers the plain refusal with + # no service planted at all, and scenario 23 plants the SAME model, so neither + # discriminates the model keying — this one does. + @id:serve-unrelated-live-service-still-fails-fast @requires-no-gpu + Scenario: serve-25 - A live service for another model does not soften the no-GPU refusal + Given a live managed Lemonade serve for an unrelated model + When the user serves a different model with Lemonade under the GPU-required default + Then serving is refused before any engine starts + And the user is told no AMD GPU was detected + And no engine was prepared for GPU serving diff --git a/tests/e2e-cucumber/src/mock_server.rs b/tests/e2e-cucumber/src/mock_server.rs index 78d8411a5..d3b76380e 100644 --- a/tests/e2e-cucumber/src/mock_server.rs +++ b/tests/e2e-cucumber/src/mock_server.rs @@ -676,6 +676,11 @@ impl MockServer { /// record shape. #[derive(Debug, Clone, Copy)] pub struct ServiceRecordOptions { + /// Engine name recorded for the service. Defaults to `"vllm"`, the engine + /// the mock server stands in for. A scenario that needs the CLI to match + /// this record against a *different* engine — e.g. the self-managing + /// `lemonade` path — overrides it. + pub engine: &'static str, pub status: &'static str, pub startup_phase: Option<&'static str>, pub supervisor_pid: u32, @@ -685,6 +690,7 @@ pub struct ServiceRecordOptions { impl Default for ServiceRecordOptions { fn default() -> Self { Self { + engine: "vllm", status: "ready", startup_phase: None, supervisor_pid: 0, @@ -722,7 +728,7 @@ pub fn write_service_record_with( // `/v1/models` for its readiness probe, which the mock serves. let record = json!({ "service_id": "e2e-mock", - "engine": "vllm", + "engine": options.engine, "model_ref": model, "canonical_model_id": model, "host": "127.0.0.1", diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index 2f659003f..29eab481a 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -30,6 +30,18 @@ fn interactive_summary_timeout() -> Duration { /// [`OOM_GUIDANCE_MODEL`] keeps the two OOM scenarios' service records from ever /// colliding, and marks this one as the launch (not reuse) case. const OOM_LAUNCH_MODEL: &str = "e2e/oom-launch-model"; +/// Model id of the live managed service planted by +/// `@id:serve-unrelated-live-service-still-fails-fast`. +/// +/// Must not be relatable to [`LEMONADE_SERVE_TARGET`] by the CLI's lenient +/// service-name matcher (case-insensitive equality, or either name containing +/// the other once extensions are stripped) — otherwise the pre-gate would open +/// on a legitimate match and the scenario would stop testing the model keying. +const UNRELATED_LIVE_MODEL: &str = "e2e/unrelated-live-model"; +/// The GGUF model the Lemonade-pinned serve steps request. Same checkpoint +/// `serve-lemonade-preparation-recovery` uses, so no new download is implied on +/// any lane that ever does reach a real install. +const LEMONADE_SERVE_TARGET: &str = "Qwen3-0.6B-GGUF"; /// How long to wait for a freshly served model's endpoint to become ready. /// /// On real GPU hardware the first serve of a model downloads its weights and @@ -1131,6 +1143,7 @@ async fn plant_oom_managed_serve(world: &mut E2eWorld) { startup_phase: Some("initializing"), supervisor_pid: std::process::id(), engine_pid: Some(std::process::id()), + ..ServiceRecordOptions::default() }, ); // A real allocator OOM signature (not vLLM's generic EngineCore wrapper, @@ -1144,6 +1157,53 @@ async fn plant_oom_managed_serve(world: &mut E2eWorld) { .expect("failed to plant the OOM startup log"); } +/// Plant a live managed **Lemonade** service for a model the serve under test +/// will not ask for. +/// +/// Lemonade because it is the engine that manages its own runtime: the reuse +/// pre-gate's body calls `ensure_self_managed_engine_ready`, which prints +/// "Preparing lemonade for GPU serving..." and installs. That print is the +/// user-visible evidence that engine work ran, and it is what +/// `assert_no_engine_preparation` looks for. +/// +/// The record names the same engine the serve will pass, so the ENGINE is not +/// what holds the pre-gate shut — only [`UNRELATED_LIVE_MODEL`] differing from +/// the served model is. `starting` plus this test process's pid keeps the record +/// live through the CLI's liveness overlay, exactly as the reuse scenario's +/// record does. +#[given("a live managed Lemonade serve for an unrelated model")] +async fn plant_unrelated_live_lemonade_serve(world: &mut E2eWorld) { + let root = world.isolated_root.as_ref().expect("no isolated root"); + let services = root.path().join("data").join("services"); + write_service_record_with( + &services, + UNRELATED_LIVE_MODEL, + 65_533, + ServiceRecordOptions { + engine: "lemonade", + status: "starting", + startup_phase: Some("initializing"), + supervisor_pid: std::process::id(), + engine_pid: Some(std::process::id()), + }, + ); +} + +/// Serve a Lemonade model under the GPU-required default while the unrelated +/// live service planted above exists. No fault-injection env var: the scripted +/// Lemonade backend failure would waive the very GPU pre-flight this scenario +/// asserts. +#[when("the user serves a different model with Lemonade under the GPU-required default")] +async fn user_serves_other_model_with_unrelated_service_live(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm( + world, + &["serve", LEMONADE_SERVE_TARGET, "--engine", "lemonade"], + ); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + #[when("the user opens its interactive serve summary")] async fn open_oom_serve_summary(world: &mut E2eWorld) { // The synthetic env selection satisfies resolution without installing a @@ -1271,6 +1331,24 @@ async fn assert_no_gpu_message(world: &mut E2eWorld) { ); } +#[then("no engine was prepared for GPU serving")] +async fn assert_no_engine_preparation(world: &mut E2eWorld) { + let output = serve_output(world); + // `ensure_self_managed_engine_ready` announces itself with exactly this line + // (`apps/rocm/src/main.rs`) immediately before it downloads and installs, so + // its absence is the observable "no engine work ran". Matched on the + // "Preparing " prefix rather than the whole sentence: that is the token the + // CLI owns, and it stays true if the rest of the sentence is reworded. + // + // Nothing here wraps — this is captured pipe output, not a PTY grid — so a + // plain `contains` is the right check. + assert!( + !output.contains("Preparing "), + "a live managed service for an unrelated model must not drag engine \ + preparation ahead of the no-usable-GPU refusal:\n{output}" + ); +} + #[then("the CLI explains that temperature cannot be negative")] async fn assert_negative_temperature_message(world: &mut E2eWorld) { let output = serve_output(world); @@ -1322,8 +1400,11 @@ async fn assert_reused_running_service(world: &mut E2eWorld) { .as_ref() .expect("no interactive serve summary") .screen_text(); + // Whitespace-insensitive for the same reason as the two assertions below: the + // heading is rendered onto an 80-column PTY grid, so a soft wrap falling + // between "already" and "running" would break a literal `contains`. assert!( - screen.contains("already running"), + screen_without_whitespace(&screen).contains("alreadyrunning"), "expected the summary to reflect the reused live service:\n{screen}" ); } From 37247be0df577271d8a039728d05cf3a7c847fa4 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 30 Sep 2026 16:27:00 +0300 Subject: [PATCH 11/11] test(vllm): narrow the OOM fixture-table failure message to admissibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The message blamed "a scoring or admissibility change" for a row crossing a branch boundary, but no fixture row sits near the scorer's threshold — all seven are comfortably accepted by the detector, and the three fallback rows fall back purely because they cannot be rendered as one single-quoted word (apostrophe, control bytes, bidi override). Pointing a future maintainer at scoring would send them to the wrong knob. The descriptive comment above the table already says this correctly and is unchanged. Signed-off-by: Roman Sirokov --- engines/vllm/src/process.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/engines/vllm/src/process.rs b/engines/vllm/src/process.rs index c929e6ddd..ad2e066a5 100644 --- a/engines/vllm/src/process.rs +++ b/engines/vllm/src/process.rs @@ -1180,9 +1180,11 @@ mod tests { assert_eq!( (routed_verbatim, fell_back), (4, 3), - "the table must keep exercising both branches; if a scoring or admissibility \ - change moved a line across a boundary, re-pick the fixture rather than relaxing \ - the expectation" + "the table must keep exercising both branches; if an admissibility change \ + moved a line across the quotability boundary, re-pick the fixture rather than \ + relaxing the expectation. Scoring is not what separates these rows — every row \ + here is comfortably accepted by the detector, and the three fallbacks are \ + fallbacks purely because they cannot be rendered as one single-quoted word" ); }