diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 3d4990869..f1f1aec00 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 1e368f678..5953e16e1 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -6066,17 +6066,111 @@ fn serve(args: ServeArgs) -> Result<()> { } else { rocm_core::usable_amd_gpu_indices() }; + // 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 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 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; + if !cpu_only && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) + { + let selection = validate_engine_selection_runtime( + &paths, + resolve_engine_selection( + &config, + &selected_engine, + runtime_id.as_deref(), + env_id.as_deref(), + ), + )?; + 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 (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 - // 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: 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 — + // 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 + // (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) = visible_gpu_indices.as_deref() && usable.is_empty() { @@ -6118,13 +6212,28 @@ fn serve(args: ServeArgs) -> Result<()> { } else { None }; - 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)?; + // 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() @@ -6137,21 +6246,27 @@ 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, - }, - )?; + // Reuse the `ResolveModel` answer the reuse pre-gate already paid for, so a + // reusing invocation does not make the same round-trip twice. + 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, + }, + )?, + }; // Serialize GPU auto-selection with the managed-service claim: the busy-GPU // read and the claiming record write inside `spawn_managed_engine_child` must // be atomic, or two concurrent `rocm serve --gpu auto` can both read the same @@ -6325,6 +6440,25 @@ 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. + // + // 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, + report.already_running, + &report.status, + report.log_path.as_deref(), + ); let summary = serve_summary::DeploymentSummary { engine: selected_engine.clone(), requested_model: model, @@ -6410,6 +6544,159 @@ 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. +/// +/// 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 +/// 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, + 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; + } + // 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)); + 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, already_hinted) { + notes.push(note); + } + notes +} + fn validate_bind_host(host: &str, allow_public_bind: bool) -> Result<()> { if !is_loopback_host(host) && !allow_public_bind { bail!( @@ -6925,6 +7212,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, @@ -19213,6 +19516,41 @@ fn existing_live_managed_service( }) } +/// 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. +/// +/// 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 +/// 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 + && (service_model_names_match(&record.model_ref, model_ref) + || service_model_names_match(&record.canonical_model_id, model_ref)) + && managed_service_is_live(record) + }) + }) +} + fn managed_service_running_state(status: &str) -> &'static str { match status { "ready" | "running" => "running", @@ -29735,6 +30073,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) @@ -31286,6 +31754,209 @@ 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_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( + &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); + + // 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!( + 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}" + ); + } + + #[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 c4bfa0213..a3bc4d592 100644 --- a/apps/rocm/src/serve_summary.rs +++ b/apps/rocm/src/serve_summary.rs @@ -182,6 +182,83 @@ 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. 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. +/// +/// `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 + // 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. + // + // `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. {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}'`." + )) +} + /// 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 @@ -423,6 +500,240 @@ 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.", + false, + ) + .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}"); + // 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] + 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", 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", 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 + /// 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, 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 + // 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, 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:?}" + ); + // 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.", + false, + ) + .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(); + summary.status = "starting".to_owned(); + 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); + } + 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/e2e-report/src/lib.rs b/crates/e2e-report/src/lib.rs index 38b91d442..bce7ad6f4 100644 --- a/crates/e2e-report/src/lib.rs +++ b/crates/e2e-report/src/lib.rs @@ -16,3 +16,17 @@ mod single_report; pub use consolidated::{RunMeta, consolidated_summary_markdown, generate_consolidated}; pub use parse::{XfailReport, evaluate_xfail, scenario_results_by_id}; pub use single_report::generate; + +/// 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"; diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 34b1d6e14..5d0b17f35 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -1910,6 +1910,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 944fcec78..de1bb00bd 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -76,6 +76,7 @@ pub use runtime::{ runtime_python_env_bin_dir, runtime_python_executable_in_env, runtime_python_executable_name, runtime_rocm_library_filename, shell_command_for_host, user_runtime_dir, }; +pub use terminal::{quotable_in_single_quotes, strip_terminal_control_sequences}; pub use uv::{ DEFAULT_UV_TIMEOUT_SECS, DependencyViolation, UV_CACHE_DIR_ENV, UV_CACHE_DIR_OVERRIDE_ENV, UvCacheSource, ViolationSubject, check_dependencies, ensure_uv_binary, split_local_version, @@ -8096,9 +8097,86 @@ 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. +/// +/// 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. +/// +/// 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 { + log.lines().any(|line| { + let line = line.trim(); + !line.is_empty() && diagnose::vllm_oom_symptom_is_diagnosable(&format!("vllm: {line}")) + }) +} + +/// 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. +/// +/// `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. +/// +/// Scanning with `.rev()` is deliberate, not incidental: when several lines of +/// the tail mention running out of memory, the *last* one is the proximate +/// failure — the point where the process actually gave up — and the earlier ones +/// are usually the allocator's own retry chatter leading up to it. The cost is +/// real and worth stating: on a tail like "torch.OutOfMemoryError: ... Tried to +/// allocate 7.21 GiB. GPU 0 has a total capacity of 24.00 GiB." followed by a +/// bare "RuntimeError: ... killed: out of memory", the vaguer line wins even +/// though the first carries the allocation size and the capacity. Both are +/// diagnosable, so this picks which line is quoted, never whether one is. The +/// user still sees every line: callers print the whole log tail beside the hint. +/// +/// 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. +#[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(|line| format!("vllm: {line}")) +} /// 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 @@ -13238,6 +13316,84 @@ last_installed_runtime_id = "therock-release" ); } + #[test] + fn vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures() { + // 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. Tried to allocate 7.21 GiB." + )); + assert!(vllm_log_shows_oom( + "torch.cuda.OutOfMemoryError: CUDA out of memory" + )); + assert!(vllm_log_shows_oom("RuntimeError: hipErrorOutOfMemory")); + // Case-insensitive. + 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" + )); + assert!(!vllm_log_shows_oom("OSError: model weights not found")); + 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 combine_amd_gpu_counts_prefers_compute_authoritative_kfd() { // KFD is compute-authoritative: a nonzero KFD count wins, and DRM must not diff --git a/crates/rocm-core/src/terminal.rs b/crates/rocm-core/src/terminal.rs index 20eead507..d38abaa41 100644 --- a/crates/rocm-core/src/terminal.rs +++ b/crates/rocm-core/src/terminal.rs @@ -2,13 +2,13 @@ // // SPDX-License-Identifier: MIT -//! One ECMA-48 walk over untrusted terminal output, shared by the two callers -//! that need it. +//! What rocm-cli does with untrusted terminal output: one ECMA-48 walk, and the +//! decisions that rest on it. //! -//! Both callers are handling the same input — a raw terminal capture from a -//! subprocess, pasted or quoted back at the user — and both need the same -//! question answered: *what would a terminal have rendered here?* They use the -//! answer differently. +//! Every caller here is handling the same input — a raw terminal capture from a +//! subprocess, pasted or quoted back at the user — and needs the same question +//! answered: *what would a terminal have rendered here?* They use the answer +//! differently. //! //! * The vLLM engine strips the sequences out, so a colourised or bell-bearing //! log line cannot repaint the user's terminal from inside rocm-cli's own @@ -16,11 +16,17 @@ //! * The vLLM-OOM diagnostic needs the *line boundaries*, so that two things the //! terminal drew on separate rows are not scored as one line //! ([`rendered_lines`]) — with the single exception documented below. +//! * The surfaces that render such a line back inside a command the user is +//! invited to paste reject it rather than rewrite it +//! ([`quotable_in_single_quotes`]). //! //! A second, independent scan for the second caller would have been a second //! grammar to get wrong, and the first one took three rounds to get right. So //! the grammar lives here once, in one private stepping function, and each -//! caller interprets the classified tokens it yields. +//! caller interprets the classified tokens it yields. The third needs no walk +//! at all, only the character predicates ([`is_control_or_format`] and +//! [`is_control_or_line_separator`]) the other two end on — which is why it +//! lives here rather than beside either of its two callers. //! //! # Line breaks inside a string body: merging and losing rows //! @@ -159,18 +165,59 @@ pub fn is_control_or_format(c: char) -> bool { /// neither `char::is_control` nor [`is_control_or_format`]'s enumerated `Cf` /// set covers them, yet a terminal draws the text after either on the next row. /// The two places that must not let that happen — this module's classifier and -/// the vLLM engine's guard on what may be quoted into the `rocm diagnose -/// --symptom '...'` command it prints — therefore share this predicate instead -/// of each deciding for itself what breaks a line. The classifier was widened -/// to the pair when this walk moved out of the vLLM engine (the commit that -/// moved it says "behaviour is unchanged", which is true of everything except -/// this); the guard was not, and a line separator went on riding into the -/// printed command. +/// [`quotable_in_single_quotes`], the guard on what may be quoted into the +/// `rocm diagnose --symptom '...'` command the OOM surfaces print — therefore +/// share this predicate instead of each deciding for itself what breaks a line. +/// The classifier was widened to the pair when this walk moved out of the vLLM +/// engine (the commit that moved it says "behaviour is unchanged", which is +/// true of everything except this); the guard was not, and a line separator +/// went on riding into the printed command. #[must_use] pub fn is_control_or_line_separator(c: char) -> bool { c.is_control() || matches!(c, '\u{2028}' | '\u{2029}') } +/// 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. +/// +/// This is the counterpart of [`strip_terminal_control_sequences`], not a +/// duplicate of it: text rocm-cli merely *echoes* is stripped, while text it +/// renders into a command the user is told to run is rejected, because a +/// stripped line is a lookalike of the user's error that is also *runnable*. +/// +/// It is shared rather than per-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. +/// +/// The character test is [`is_control_or_format`] together with +/// [`is_control_or_line_separator`], because each covers scalars the other does +/// not and `symptom` is the *raw* log line, not the stripped one: a `Cf` bidi +/// override reordered how the printed command renders, and a `Zl` line +/// separator broke it across two rows. +#[must_use] +pub fn quotable_in_single_quotes(symptom: &str) -> bool { + !symptom.contains('\'') + && !symptom + .chars() + .any(|c| is_control_or_format(c) || is_control_or_line_separator(c)) +} + /// Consumes one glyph, control character or escape sequence and says which of /// the three it was. /// @@ -417,6 +464,175 @@ pub fn rendered_lines(text: &str) -> Vec { mod tests { use super::*; + #[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(crate::VLLM_OOM_CANONICAL_SYMPTOM)); + assert!(quotable_in_single_quotes( + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." + )); + // Every scalar the stripper removes must also be inadmissible in the + // quoted command, not merely stripped from the echoed sentence: the + // candidate this guard sees is built from the *raw* line, not the + // stripped one, so it needs its own pins. `Cf` is not `char::is_control` + // and `Zl`/`Zp` are in neither, which is why the test below is two + // predicates rather than one. + assert!( + !quotable_in_single_quotes("vllm: \u{202e}HIP out of memory"), + "a bidi override must make a line unquotable, not ride into the command" + ); + assert!( + !quotable_in_single_quotes("vllm: HIP\u{2028}out of memory"), + "a line separator must make a line unquotable, not ride into the command" + ); + assert!( + !quotable_in_single_quotes("vllm: HIP\u{2029}out of memory"), + "a paragraph separator must make a line unquotable, not ride into the command" + ); + assert!( + quotable_in_single_quotes("vllm: HIP out of memory"), + "the rejection must not be so broad that ordinary lines stop qualifying" + ); + } + + #[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(crate::VLLM_OOM_CANONICAL_SYMPTOM), + crate::VLLM_OOM_CANONICAL_SYMPTOM + ); + } + + #[test] + fn the_stripper_follows_the_escape_grammar_not_just_the_colour_case() { + // The `'m'`-terminated SGR case above is the *easy* one, and on its own + // it pins almost nothing: narrowing the CSI final-byte range from + // `0x40..=0x7E` to just `'m'` leaves it green. These cases pin the + // range, the parameter/intermediate classes, and the introducers. + // + // They are not academic. This input is a *killed* process's output, so + // truncated and interleaved sequences are the normal case on this code + // path, and every one of them used to corrupt the message the user + // reads -- the failures are quoted per case below. + for (raw, expected, defect) in [ + // Non-`m` CSI finals: `K` (erase-in-line) and `A` (cursor-up) are + // ordinary logger output, and narrowing the final-byte range to + // `'m'` leaves them in the message verbatim. + ( + "\u{1b}[2KRuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a non-`m` CSI final must terminate the sequence", + ), + ( + "\u{1b}[1ARuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a non-`m` CSI final must terminate the sequence", + ), + // Truncated CSI immediately followed by a well-formed one: the old + // scan consumed the second sequence's `ESC [` as the first one's + // parameters and stopped at `0`, yielding "0mKilled: out of memory". + ( + "\u{1b}[1;2\u{1b}[0mKilled: out of memory", + "Killed: out of memory", + "a truncated CSI must not swallow the next sequence's introducer", + ), + // A multi-byte scalar can never be in `0x40..=0x7E`, so the old scan + // ran past it and ate to the next byte that happened to land in + // range -- this yielded "ut of memory", losing the `o`. + ( + "\u{1b}[12\u{e9} out of memory", + "\u{e9} out of memory", + "an invalid CSI byte must end the sequence, not be scanned past", + ), + // `ESC ESC`: the old code dropped the second `ESC` as the first + // one's argument, then emitted `[0m` as literal text. + ( + "\u{1b}\u{1b}[0mRuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a stray ESC must not consume the next sequence's introducer", + ), + // OSC: the old code took the `else` branch on `]`, so the window + // title leaked into the message as "0;titleRuntimeError: ...". + ( + "\u{1b}]0;title\u{7}RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "an OSC body must not be emitted as text", + ), + // ...and with the ST terminator (`ESC \`) rather than BEL. + ( + "\u{1b}]0;title\u{1b}\\RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "an OSC terminated by ST must be consumed whole", + ), + // A two-character escape with no CSI at all. + ( + "\u{1b}7RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a simple escape must consume exactly its final byte", + ), + // Cf format characters: `char::is_control` is category Cc only, so + // U+202E survived and reversed how the rest of the line renders. + ( + "RuntimeError: \u{202e}HIP out of memory", + "RuntimeError: HIP out of memory", + "a bidi override must not survive into the message", + ), + // The `Zl`/`Zp` pair that `is_control_or_line_separator` adds to + // `char::is_control`: drop it from that predicate and both of these + // go red, as do the quoting assertions above. + ( + "RuntimeError: HIP\u{2028}out of memory", + "RuntimeError: HIPout of memory", + "a line separator must not survive into the message", + ), + ( + "RuntimeError: HIP\u{2029}out of memory", + "RuntimeError: HIPout of memory", + "a paragraph separator must not survive into the message", + ), + // The text-only control: nothing is removed from a clean line. + ( + "RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a clean line must pass through untouched", + ), + ] { + assert_eq!( + strip_terminal_control_sequences(raw), + expected, + "{defect}: {raw:?}" + ); + } + } + #[test] fn a_line_advance_of_any_form_is_a_boundary_and_sgr_is_not() { // The table is the reviewer's reproduction on PR #251, generalised: the diff --git a/docs/vllm.md b/docs/vllm.md index 7edbc634e..7f144545f 100644 --- a/docs/vllm.md +++ b/docs/vllm.md @@ -215,7 +215,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/process.rs b/engines/vllm/src/process.rs index ba1938a05..ad2e066a5 100644 --- a/engines/vllm/src/process.rs +++ b/engines/vllm/src/process.rs @@ -3,13 +3,11 @@ // SPDX-License-Identifier: MIT use anyhow::{Context, Result, anyhow, bail}; -use rocm_core::terminal::{ - is_control_or_format, is_control_or_line_separator, strip_terminal_control_sequences, -}; use rocm_core::{AppPaths, DEFAULT_LOCAL_PORT, require_nonempty}; use rocm_engine_protocol::{ - DevicePolicy, ENGINE_RECIPE_CONTRACT_VERSION, EngineRecipeHint, GpuSelection, LaunchRequest, - LaunchResponse, ResolveModelRequest, ResolveModelResponse, StopRequest, StopResponse, + DEFAULT_LOG_TAIL_LINES, DevicePolicy, ENGINE_RECIPE_CONTRACT_VERSION, EngineRecipeHint, + GpuSelection, LaunchRequest, LaunchResponse, ResolveModelRequest, ResolveModelResponse, + StopRequest, StopResponse, }; use serde_json::json; use std::ffi::OsString; @@ -21,7 +19,13 @@ use std::time::{Duration, Instant}; use crate::runtime::VllmRuntime; -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 @@ -735,7 +739,7 @@ fn startup_log_context(log_path: Option<&Path>) -> String { let hint = oom_utilization_hint(&summary); let summary = summary .lines() - .map(strip_terminal_control_sequences) + .map(rocm_core::strip_terminal_control_sequences) .collect::>() .join("\n"); format!("\n\nLast {STARTUP_FAILURE_LOG_TAIL_LINES} lines of startup log:\n{summary}{hint}") @@ -747,41 +751,29 @@ 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) { - return String::new(); - } // Route the user's *actual* failing line into the `--symptom` example when - // the diagnose checker would actually score it *and* it can be rendered as - // one intact single-quoted argument; 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. - // `.rev()` is deliberate, not incidental: when several lines of the tail - // mention running out of memory, the *last* one is the proximate failure — - // the point where the process actually gave up — and the earlier ones are - // usually the allocator's own retry chatter leading up to it. The cost is - // real and worth stating: on a tail like "torch.OutOfMemoryError: ... Tried - // to allocate 7.21 GiB. GPU 0 has a total capacity of 24.00 GiB." followed - // by a bare "RuntimeError: ... killed: out of memory", the vaguer line wins - // even though the first carries the allocation size and the capacity. Both - // are diagnosable, so this picks which line is quoted, never whether one is. - // The user still sees every line: the log tail is printed above this hint. - let raw_line = log_tail - .lines() - .rev() - .map(str::trim) - .find(|line| !line.is_empty() && log_tail_shows_oom(line)) - .unwrap_or("out of memory"); + // it can be rendered as one intact single-quoted argument; otherwise fall + // back to the canonical symptom so the printed command always reports a + // cause rather than reading as "the tool checked and there's no known + // cause". Selecting the line is the shared helper's job, so this surface and + // the `rocm` CLI serve summary route to `rocm diagnose` identically instead + // 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. + // + // 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 = strip_terminal_control_sequences(raw_line); - let candidate = format!("vllm: {raw_line}"); - let symptom = if quotable_in_single_quotes(&candidate) - && rocm_core::vllm_oom_symptom_is_diagnosable(&candidate) - { - candidate + 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() }; @@ -792,43 +784,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. -/// -/// The character test is [`is_control_or_format`] together with -/// [`is_control_or_line_separator`], because each covers scalars the other does -/// not and `symptom` is the *raw* log line, not the stripped one: a `Cf` bidi -/// override reordered how the printed command renders, and a `Zl` line -/// separator broke it across two rows. -fn quotable_in_single_quotes(symptom: &str) -> bool { - !symptom.contains('\'') - && !symptom - .chars() - .any(|c| is_control_or_format(c) || is_control_or_line_separator(c)) -} - -/// 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 @@ -1111,7 +1066,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"), @@ -1130,17 +1085,19 @@ 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] fn every_emitted_oom_symptom_is_diagnosable_and_names_the_branch_that_produced_it() { // 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. The detector now classifies + // each line with the *same* rule the checker uses, so an accepted line + // is diagnosable by construction -- this guards that invariant. // // Each case pins the *exact* symptom, not just that one is diagnosable. // Diagnosability alone cannot fail for the defect this test is named @@ -1150,9 +1107,17 @@ mod tests { // every such assertion. Naming the expected symptom per line is what // makes the routing branch and the fallback branch separately // falsifiable -- and it is why the table below deliberately mixes the - // two: three lines score on their own and must be quoted verbatim, - // three do not and must fall back. + // two. + // + // Since the detector was reconciled with the checker, the fallback is no + // longer reached by sub-threshold scoring (those lines now produce no + // hint at all -- see `sub_threshold_lines_carry_no_hint_at_all`); it is + // reached by a line that scores but cannot be rendered as one intact + // single-quoted argument. So the fallback rows here are the + // *unquotable* ones, which is also what keeps the admissibility guard + // falsifiable through the surface that actually prints the command. let accepted_lines = [ + // Score and are quotable: the user's own line must be quoted verbatim. ( "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", @@ -1161,49 +1126,102 @@ mod tests { "RuntimeError: hipErrorOutOfMemory", "vllm: RuntimeError: hipErrorOutOfMemory", ), + ( + "torch.cuda.OutOfMemoryError: CUDA out of memory", + "vllm: torch.cuda.OutOfMemoryError: CUDA out of memory", + ), ("CUDA out of memory", "vllm: CUDA out of memory"), - // Sub-threshold on their own: the class name with no allocator - // message, the allocator message with no class name, and a bare - // kill notice. These must reach the canonical fallback. + // Score but are inadmissible in a single-quoted word, so they must + // reach the canonical fallback: an apostrophe (which would close the + // quote), a colourised line (control bytes), and a bidi override + // (a `Cf` format character, which `char::is_control` does not catch + // and which reorders how the printed command renders). ( - "torch.cuda.OutOfMemoryError", + "torch.OutOfMemoryError: HIP out of memory. GPU 0 can't allocate the model's weights.", rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, ), ( - "HIP error: out of memory", + "\u{1b}[31mtorch.OutOfMemoryError: HIP out of memory\u{1b}[0m", rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, ), ( - "the process was killed: out of memory", + "torch.OutOfMemoryError: \u{202e}HIP out of memory. Tried to allocate 7.21 GiB.", rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, ), ]; let mut routed_verbatim = 0; + let mut fell_back = 0; for (line, expected) 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 = quoted_symptom_argument(&hint); assert_eq!( symptom, expected, "wrong branch for {line:?}: the engine must quote the user's own line when \ - the checker scores it and fall back to the canonical symptom only when it \ - does not" + it can be rendered as one intact single-quoted argument and fall back to \ + the canonical symptom only when it cannot" ); assert!( rocm_core::vllm_oom_symptom_is_diagnosable(symptom), "emitted symptom must be diagnosable, got {symptom:?} for line {line:?}" ); - if symptom != rocm_core::VLLM_OOM_CANONICAL_SYMPTOM { + if symptom == rocm_core::VLLM_OOM_CANONICAL_SYMPTOM { + fell_back += 1; + } else { routed_verbatim += 1; } } + // Both counters, not just the verbatim one: a table that drifted until + // every row took the same branch would still satisfy every per-row + // assertion above, which is the vacuity this test was written to close. assert_eq!( - routed_verbatim, 3, - "the table must keep exercising both branches; if a scoring change moved a line \ - across the threshold, re-pick the fixture rather than relaxing the expectation" + (routed_verbatim, fell_back), + (4, 3), + "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" ); } + #[test] + fn sub_threshold_lines_carry_no_hint_at_all() { + // 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"). + // + // This is a different property from the one above: there the question is + // *which* symptom an emitted hint carries, here it is whether a hint is + // emitted at all. Before the detector was reconciled these lines reached + // the canonical fallback instead; now they produce nothing, and printing + // a memory hint for them would be a regression the symptom table cannot + // see. + 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}" + ); + } + } + /// The `--symptom` value the hint actually hands the user, read back out of /// the rendered text exactly the way a shell would: everything between the /// opening quote that follows `--symptom ` and the next `'`. @@ -1224,7 +1242,7 @@ 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." ); - assert!(log_tail_shows_oom(log_tail)); + assert!(rocm_core::vllm_log_shows_oom(log_tail)); let hint = oom_utilization_hint(log_tail); // The rendered command must be one intact single-quoted argument: no @@ -1274,7 +1292,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" ); @@ -1293,129 +1311,6 @@ mod tests { ); } - #[test] - fn the_stripper_follows_the_escape_grammar_not_just_the_colour_case() { - // The `'m'`-terminated SGR case above is the *easy* one, and on its own - // it pins almost nothing: narrowing the CSI final-byte range from - // `0x40..=0x7E` to just `'m'` leaves it green. These cases pin the - // range, the parameter/intermediate classes, and the introducers. - // - // They are not academic. This input is a *killed* process's output, so - // truncated and interleaved sequences are the normal case on this code - // path, and every one of them used to corrupt the message the user - // reads -- the failures are quoted per case below. - for (raw, expected, defect) in [ - // Non-`m` CSI finals: `K` (erase-in-line) and `A` (cursor-up) are - // ordinary logger output, and narrowing the final-byte range to - // `'m'` leaves them in the message verbatim. - ( - "\u{1b}[2KRuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "a non-`m` CSI final must terminate the sequence", - ), - ( - "\u{1b}[1ARuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "a non-`m` CSI final must terminate the sequence", - ), - // Truncated CSI immediately followed by a well-formed one: the old - // scan consumed the second sequence's `ESC [` as the first one's - // parameters and stopped at `0`, yielding "0mKilled: out of memory". - ( - "\u{1b}[1;2\u{1b}[0mKilled: out of memory", - "Killed: out of memory", - "a truncated CSI must not swallow the next sequence's introducer", - ), - // A multi-byte scalar can never be in `0x40..=0x7E`, so the old scan - // ran past it and ate to the next byte that happened to land in - // range -- this yielded "ut of memory", losing the `o`. - ( - "\u{1b}[12\u{e9} out of memory", - "\u{e9} out of memory", - "an invalid CSI byte must end the sequence, not be scanned past", - ), - // `ESC ESC`: the old code dropped the second `ESC` as the first - // one's argument, then emitted `[0m` as literal text. - ( - "\u{1b}\u{1b}[0mRuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "a stray ESC must not consume the next sequence's introducer", - ), - // OSC: the old code took the `else` branch on `]`, so the window - // title leaked into the message as "0;titleRuntimeError: ...". - ( - "\u{1b}]0;title\u{7}RuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "an OSC body must not be emitted as text", - ), - // ...and with the ST terminator (`ESC \`) rather than BEL. - ( - "\u{1b}]0;title\u{1b}\\RuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "an OSC terminated by ST must be consumed whole", - ), - // A two-character escape with no CSI at all. - ( - "\u{1b}7RuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "a simple escape must consume exactly its final byte", - ), - // Cf format characters: `char::is_control` is category Cc only, so - // U+202E survived and reversed how the rest of the line renders. - ( - "RuntimeError: \u{202e}HIP out of memory", - "RuntimeError: HIP out of memory", - "a bidi override must not survive into the message", - ), - // The `Zl`/`Zp` pair that `is_control_or_line_separator` adds to - // `char::is_control`: drop it from that predicate and both of these - // go red, as do the quoting assertions below. - ( - "RuntimeError: HIP\u{2028}out of memory", - "RuntimeError: HIPout of memory", - "a line separator must not survive into the message", - ), - ( - "RuntimeError: HIP\u{2029}out of memory", - "RuntimeError: HIPout of memory", - "a paragraph separator must not survive into the message", - ), - // The text-only control: nothing is removed from a clean line. - ( - "RuntimeError: HIP out of memory", - "RuntimeError: HIP out of memory", - "a clean line must pass through untouched", - ), - ] { - assert_eq!( - strip_terminal_control_sequences(raw), - expected, - "{defect}: {raw:?}" - ); - } - - // Every one of those scalars must also be inadmissible in the quoted - // command, not merely stripped from the echoed sentence: the candidate - // the guard sees is built from the raw line, not the stripped one, so - // the second call site needs its own pins. - assert!( - !quotable_in_single_quotes("vllm: \u{202e}HIP out of memory"), - "a bidi override must make a line unquotable, not ride into the command" - ); - assert!( - !quotable_in_single_quotes("vllm: HIP\u{2028}out of memory"), - "a line separator must make a line unquotable, not ride into the command" - ); - assert!( - !quotable_in_single_quotes("vllm: HIP\u{2029}out of memory"), - "a paragraph separator must make a line unquotable, not ride into the command" - ); - assert!( - quotable_in_single_quotes("vllm: HIP out of memory"), - "the rejection must not be so broad that ordinary lines stop qualifying" - ); - } - #[test] fn the_printed_log_tail_is_sanitized_not_just_the_echoed_line() -> Result<()> { // The stripper's contract is that nothing in this message can repaint @@ -1457,11 +1352,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/README.md b/tests/e2e-cucumber/README.md index 23fb2c8f7..018aafb04 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -142,6 +142,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/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index 12bac602f..3a851d09a 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -295,3 +295,67 @@ 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 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 + # every PR and allocates no GPU memory. + @id:serve-oom-memory-guidance @requires-no-gpu @requires-os:linux + Scenario: serve-24 - 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 23 (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-25 - 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 + + # 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-26 - 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/capability.rs b/tests/e2e-cucumber/src/capability.rs index a7fe4dbfa..ad45fe382 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -123,8 +123,24 @@ 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. +/// +/// 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 /// "adapter present": vLLM's adapter is built-in everywhere but cannot run on @@ -372,6 +388,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, @@ -382,6 +404,7 @@ fn probe_host_capability() -> HostCapability { available_engines, effective_serve_engine, platform_slug, + oom_fault_injection, } } @@ -766,6 +789,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. @@ -780,6 +804,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 02f21c899..4b9c80403 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -31,6 +31,7 @@ const REQUIRES_GFX_TARGET_TAG: &str = "requires-gfx-target"; 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"; @@ -129,6 +130,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 — @@ -176,6 +187,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; @@ -209,6 +221,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 { @@ -225,6 +239,7 @@ impl ScenarioDecl { requires_no_gpu, requires_bare_metal, requires_wsl, + requires_oom_fault_injection, requires_engine, requires_os, serve_timeout_secs, @@ -563,6 +578,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) { @@ -649,6 +671,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(), @@ -659,6 +682,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(), @@ -669,6 +693,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 @@ -688,6 +713,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 @@ -701,6 +727,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. @@ -713,6 +740,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(), @@ -723,6 +751,7 @@ mod tests { available_engines: vec!["lemonade".into(), "vllm".into()], effective_serve_engine: "lemonade".into(), platform_slug: "mock".into(), + oom_fault_injection: false, }, } } @@ -1402,6 +1431,56 @@ 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, + Included { + nightly: false, + lifecycle: false, + docker: false, + merge_queue: 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, + Included { + nightly: false, + lifecycle: false, + docker: false, + merge_queue: 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/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 391d03c13..df30ee5b8 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -9,11 +9,39 @@ use std::time::{Duration, Instant}; use cucumber::{given, then, when}; use crate::E2eWorld; -use e2e_cucumber::mock_server::{MockServer, ServiceRecordOptions}; +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"; + +/// 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. +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 @@ -1122,6 +1150,106 @@ 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()), + ..ServiceRecordOptions::default() + }, + ); + // 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"); +} + +/// 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 + // 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 @@ -1223,6 +1351,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); @@ -1267,6 +1413,135 @@ 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(); + // 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_without_whitespace(&screen).contains("alreadyrunning"), + "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(); + // 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_without_whitespace(&screen).contains("ranoutofGPUmemory"), + "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. + // + // 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, + &[ + "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 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!( + 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 +/// 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 user is told the pinned GPU is unavailable")] async fn assert_masked_index_message(world: &mut E2eWorld) { let output = serve_output(world).to_lowercase(); diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index d03e6a489..0dec1d833 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -170,11 +170,18 @@ impl TuiSession { Self::spawn_binary(world, crate::rocm_binary(), args) } - /// Like [`spawn`](Self::spawn), but overlaying `extra_env` on top of the - /// scenario's isolation environment — for a step whose `Given` planted - /// scenario-owned state (e.g. a shell rc file) that only the piped - /// (`run_rocm_with_env`) path would otherwise pick up, since [`pty_env`]'s - /// `HOME`/lack of `SHELL` are the PTY's own isolation, not that state. + /// As [`spawn`](Self::spawn), but overlays `extra_env` onto the child only. + /// + /// Two kinds of step need this. One sets a variable the CLI reads at startup + /// (e.g. the test-only `ROCM_E2E_SIMULATE_OOM_LAUNCH` fault-injection + /// switch). The other has a `Given` that planted scenario-owned state (e.g. a + /// shell rc file) which only the piped `run_rocm_with_env` path would + /// otherwise pick up, since [`pty_env`]'s `HOME`/lack of `SHELL` are the + /// PTY's own isolation rather than that state. + /// + /// 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], @@ -196,6 +203,9 @@ impl TuiSession { 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. fn spawn_binary_with_env( world: &E2eWorld, binary: impl AsRef, @@ -231,7 +241,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); } diff --git a/xtask/src/e2e.rs b/xtask/src/e2e.rs index 05cf29bbb..bd0a5a00b 100644 --- a/xtask/src/e2e.rs +++ b/xtask/src/e2e.rs @@ -18,6 +18,17 @@ use anyhow::{Context, Result, bail}; use crate::paths::{binary_name, target_dir, workspace_root}; +/// 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 { rocm: PathBuf, @@ -62,6 +73,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 +105,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 +122,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 +199,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 +228,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] diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index d3bc2573e..15bc9d68a 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -267,8 +267,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 @@ -277,6 +276,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() @@ -292,9 +299,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}" ); } }