From 1f45c606f4c2d7136f8821ea0ae62abfb13a0485 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 21:14:30 +0000 Subject: [PATCH] fix(#664): the video registry's existence probes stop reaching Windows with POSIX stat windows-msvc-cpu and windows-msvc-vulkan were RED on EVERY open pull request in this repository - nine sampled across five unrelated lanes (#661, #662, #663, #601, #596, #592, #578, #638) - and one file caused all of it. src/vllm/multimodal/video_engine.cpp landed in cefacd2d0 carrying at :21, ::stat + S_ISDIR at :57-60, and ::stat at :62-65. check-windows-portability.py:1675-1688 flags all three under full_source_posix, which applies to EVERY scanned source and not only to the REQUIRED_CPP platform-boundary set, so no exemption applied and none is added here. main was never a denominator: the Windows jobs are PR-only and are `skipped` on push runs (#584), so the breakage was invisible on the branch it landed on and visible on every branch cut from it afterwards. Repaired at the SOURCE. The checker's semantics, its allowlists and its scope are untouched - the checker is right and the source was wrong. RED FIRST, twice, because there are two separate claims here. 1. THE PORTABILITY RED is the checker itself. On the unmodified tree at 11cc1d5896: $ python3 scripts/check-windows-portability.py # exit 1 ERROR: src/vllm/multimodal/video_engine.cpp:21: unguarded POSIX include/call reaches Windows ERROR: src/vllm/multimodal/video_engine.cpp:59: unguarded POSIX include/call reaches Windows ERROR: src/vllm/multimodal/video_engine.cpp:64: unguarded POSIX include/call reaches Windows After: exit 0, "Windows portability contract OK". 2. THE BEHAVIOURAL RED is what a naive rewrite silently breaks, and it is the reason this is 42 lines and not 6. ::stat reported an UNINSPECTABLE path - ENAMETOOLONG, ELOOP, EACCES on a parent - by returning -1, which arrived here as a plain false and became the registry's ordinary "no such file or directory" refusal. The THROWING std::filesystem::exists(p) / is_directory(p) overloads raise filesystem_error for exactly those cases instead. That exception escapes ReadVideoCheckpointTensorNames, whose header contract (include/vllm/multimodal/video_engine.h:125-126) is to return false with *why set, and through DescribeCheckpoint it escapes LoadVideoEngine in place of the refusal that names the registered families. So the throwing rewrite was written FIRST and the new test run against it: test_video_engine.cpp:281: FATAL ERROR: REQUIRE_NOTHROW( ... ) THREW exception: "filesystem error: status: File name too long [...]" [doctest] test cases: 1 | 0 passed | 1 failed | assertions: 1 | 0 passed | 1 failed Then the std::error_code overloads: 1 passed, 6 assertions, exit 0. No token gate, golden or e2e render could ever have caught this - every checkpoint they hand over is perfectly stattable, which is why the probe is a 300-character path component rather than a missing file. A MISSING file is ENOENT, and the throwing overloads do not throw for ENOENT, so a missing-path test passes with and without the guarantee. NativePath() is file-local, and deliberately so. Nothing in the tree exports one to call: fs_io.cpp:31 and minimax_h3_sharded.cpp:55 both define NativePath in their own anonymous namespace, and gguf_reader.cpp:22 / safetensors_reader.cpp:29 define Utf8Path the same way. The spelling adopted here is fs_io.cpp:31's, byte-for-byte, and NOT minimax_h3_sharded.cpp:55's MultiByteToWideChar variant - that one THROWS std::invalid_argument on a malformed-UTF-8 path under _WIN32, which would reintroduce on Windows precisely the escaping exception this change removes on POSIX. The u8string step is not decoration either: on Windows a narrow std::string handed to std::filesystem::path is interpreted in the ACTIVE CODE PAGE, so a non-ASCII checkpoint path silently resolves to the wrong file. MUTATIONS, both anchors asserted count == 1 and the tree restored and verified by sha256: * Exists -> throwing overload -> RED, 1 case failed, exit 1. The guard bites. * IsDir -> throwing overload -> SURVIVES, 12/12, 260 assertions, exit 0. The survivor is REACHABILITY, and it is measured rather than asserted. IsDir has exactly one call site (video_engine.cpp:180) and it is gated by Exists at :176. A probe over nine pathological path shapes - ENAMETOOLONG (36), ELOOP (40), EACCES (13), ENOTDIR (20), ENOENT (2), dangling symlink, empty path, real file, real directory - produced NO row in which exists() succeeded while is_directory() set an error code, because on this libstdc++ both resolve through the same stat() call. Short of a TOCTOU race, no input can distinguish the two IsDir spellings through the shipped surface. The error_code overload is kept there anyway: it is the same contract, and a later caller that probes a directory WITHOUT a preceding Exists would otherwise inherit the throw. Gate: ctest -N registers 423, unchanged from main, so the denominator did not drift. Focused before/after on the one suite touched, test_video_engine: 11 cases / 254 assertions -> 12 cases / 260 assertions, exit 0 both times. BUILD_EXIT=0 with zero ENOSPC or BFD-assertion lines in the build log. agent-preflight.sh: one gate failed, test_cpu_x86_llamacpp_floor, with NO_QUIET_WINDOW (exit 4, load=118) while this branch's own -j4 build was running; it is the documented load artifact and it fails the same way on unmodified main. tests/scripts/test_check_windows_portability.py fails 2 of 71 on this tree AND, verified by stash, identically at the base SHA 11cc1d5896 - pre-existing, unrelated (ltx2.cpp / ltx2_video_vae.cpp / ltx2_audio_vae.cpp allocation-and-math, plus one fake-runner-scoped case), and not run by any workflow. OVERLAP, stated rather than discovered later: open PR #524 (row/ENG-RELEASE-WINDOWS-512, "fix(release): run no-argv tests on Windows") already carries a version of this same repair. It is weaker in two exact ways - it passes the narrow std::string straight to std::filesystem, with no NativePath and therefore the active-code-page defect above, and its added test covers directory / regular file / MISSING path, which is ENOENT and so passes with the throwing overloads too. Its hunk and this one will conflict; whichever lands second should take this file's version wholesale. #524 also still carries the tests/vt/test_backend_cross_device.cpp setenv repair that 11cc1d5896 already landed via tests/support/test_env.h. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode] --- .agents/roadmap_v1.md | 1 + src/vllm/multimodal/video_engine.cpp | 42 ++++++++++++++++--- tests/vllm/multimodal/test_video_engine.cpp | 46 +++++++++++++++++++++ 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index c335bd797..426a37d40 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -188,6 +188,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | | [#659](https://github.com/mudler/vllm.cpp/issues/659) | — | LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing) | bug | | [#660](https://github.com/mudler/vllm.cpp/issues/660) | — | `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553) | bug | +| [#664](https://github.com/mudler/vllm.cpp/issues/664) | — | **FIXED 2026-08-13, `row/FIX-WINDOWS-POSIX-VIDEO-ENGINE`.** `windows-msvc-cpu` / `windows-msvc-vulkan` were RED on EVERY open PR (9 sampled across 5 unrelated lanes) from ONE file: `video_engine.cpp` reached Windows with ``, `::stat` and `S_ISDIR` (landed `cefacd2d0`), which `check-windows-portability.py:1675-1688` flags under `full_source_posix` — every scanned source, not only the platform-boundary set. `main` was never a denominator because the Windows jobs are PR-only and `skipped` on push (#584). Repaired at the SOURCE, not the checker: `IsDir`/`Exists` now take the `std::error_code` overloads through a file-local `NativePath`, which preserves `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`, 41 shipped names / 37 families recorded.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` is PORTABLE from vLLM source**: `minimax_m3` is backed by the Rust crate, `openai` is a declared Harmony stub that raises on both methods, and both Cohere names are shims over the out-of-tree `cohere_melody` package, so W1/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | | [#647](https://github.com/mudler/vllm.cpp/issues/647) | — | Oracle policy had no fallback and no pin concept: five upstreams beyond vLLM are already compared against (vLLM-Omni, SGLang, llama.cpp, `transformers`, tt-forge) with their pins scattered across individual specs or absent entirely. AGENTS.md now admits a named secondary oracle where vLLM implements nothing, `.agents/oracles/.md` pins each one file-per-oracle, and `check-oracle-pins.py` enforces both directions. The gateability debts for `sglang`, `diffusers` and `tt-forge` stay open on this issue | feature | | [#649](https://github.com/mudler/vllm.cpp/issues/649) | `TOOLS-CALLING-CORE` | That row's prose still records `tool_parser_names()` 40 / `reasoning_parser_names()` 7; both enumerations have grown since 2026-07-24 and are now **41** (`tool_parsers/abstract.cpp:269`) and **12** (`reasoning_parsers/abstract.cpp:72`). Code and tests are correct — `test_detect.cpp:221` already pins 41 — only the record drifted. Halves belong to two other rows (#608, #605), so it is filed rather than repaired inside #643 (found while implementing #643's review findings) | bug | diff --git a/src/vllm/multimodal/video_engine.cpp b/src/vllm/multimodal/video_engine.cpp index 656a312f1..62604274a 100644 --- a/src/vllm/multimodal/video_engine.cpp +++ b/src/vllm/multimodal/video_engine.cpp @@ -11,15 +11,15 @@ #include #include #include +#include #include #include #include #include +#include #include #include -#include - #include #include "vllm/entrypoints/openai/video_api.h" @@ -54,14 +54,44 @@ std::vector& RegistryStorage() { // detector may claim a checkpoint and exactly one entry may carry a name. const std::vector& OrderedRegistry() { return RegistryStorage(); } +// A caller's UTF-8 path as a native filesystem path. Byte-for-byte the spelling +// in v1/kv_offload/fs_io.cpp:31, and the same job the loader lane's Utf8Path +// does (gguf_reader.cpp:22, safetensors_reader.cpp:29) — no shared helper +// exists to call, because each is file-local to its own TU. The u8string step +// is not decoration: on Windows a narrow std::string handed to +// std::filesystem::path is interpreted in the ACTIVE CODE PAGE, so every +// non-ASCII checkpoint path silently resolves to the wrong file (or to none). +// Saying char8_t makes the UTF-8 explicit and the conversion to UTF-16 exact. +std::filesystem::path NativePath(const std::string& utf8) { +#if defined(_WIN32) + const std::u8string value(reinterpret_cast(utf8.data()), utf8.size()); + return std::filesystem::path(value); +#else + return std::filesystem::path(utf8); +#endif +} + +// BOTH probes take the std::error_code overloads, and that is load-bearing +// rather than stylistic. The `::stat` calls these replaced reported an +// uninspectable path — ENAMETOOLONG, ELOOP, EACCES on a parent — by returning +// -1, which arrived here as a plain `false` and became the registry's ordinary +// "no such file or directory" refusal. The THROWING +// std::filesystem::exists(p) / is_directory(p) overloads raise +// filesystem_error for exactly those cases instead, which would escape +// ReadVideoCheckpointTensorNames — whose header contract is to return false +// with *why set — and, through DescribeCheckpoint, escape LoadVideoEngine in +// place of the refusal that names the registered families. Returning false on +// error keeps the POSIX behaviour these calls had. Issue #664. bool IsDir(const std::string& path) { - struct stat st {}; - return ::stat(path.c_str(), &st) == 0 && S_ISDIR(st.st_mode); + std::error_code error; + const bool result = std::filesystem::is_directory(NativePath(path), error); + return !error && result; } bool Exists(const std::string& path) { - struct stat st {}; - return ::stat(path.c_str(), &st) == 0; + std::error_code error; + const bool result = std::filesystem::exists(NativePath(path), error); + return !error && result; } std::string StripTrailingSlash(const std::string& dir) { diff --git a/tests/vllm/multimodal/test_video_engine.cpp b/tests/vllm/multimodal/test_video_engine.cpp index 8854fd3a8..6df8af57f 100644 --- a/tests/vllm/multimodal/test_video_engine.cpp +++ b/tests/vllm/multimodal/test_video_engine.cpp @@ -256,6 +256,52 @@ TEST_CASE("video engine registry: an unrecognizable checkpoint refuses instead o } } +// A path the OS cannot inspect AT ALL — one 300-character component, which is +// ENAMETOOLONG rather than ENOENT. This is the case that separates a REFUSAL +// from a THROW, and it is why the registry's existence probes must use the +// std::error_code overloads of : +// +// ::stat(path, &st) -> -1, so the probe returned false +// std::filesystem::exists(p, ec) -> false, ec = "File name too long" +// std::filesystem::exists(p) [throwing] -> THROWS filesystem_error +// +// The header's contract for ReadVideoCheckpointTensorNames is "returns false +// with *why holding the reason when the artifact cannot be enumerated" +// (include/vllm/multimodal/video_engine.h). A throwing overload silently trades +// that documented refusal for an exception escaping a registry query, and no +// token- or golden-based gate would ever see it, because every checkpoint those +// gates hand over is perfectly stattable. Issue #664. +TEST_CASE("video engine registry: an UNINSPECTABLE path refuses, and never throws") { + SeamWorkspace ws; + const std::string unstattable = ws.root + "/" + std::string(300, 'x'); + + std::vector names{"stale"}; + std::string why; + bool enumerated = true; + REQUIRE_NOTHROW(enumerated = vllm::multimodal::ReadVideoCheckpointTensorNames( + unstattable, &names, &why)); + CHECK_FALSE(enumerated); + CHECK(names.empty()); + CHECK(why == "no such file or directory"); + + // ...and the same path through the seam ends in the registry's OWN refusal, + // which names the registered families. std::filesystem::filesystem_error + // derives from std::runtime_error, so CHECK_THROWS_AS(std::runtime_error) + // would NOT tell the two apart — the message is what discriminates them, and + // a filesystem_error's what() knows nothing about video families. + vllm::multimodal::VideoModelParams mp = FixtureParams(ws.fixture); + mp.dit_path = unstattable; + try { + (void)vllm::multimodal::LoadVideoEngine(mp); + FAIL("an uninspectable dit_path must be refused, not detected around"); + } catch (const std::exception& e) { + const std::string msg = e.what(); + INFO(msg); + CHECK(msg.find("minimax-h3") != std::string::npos); + CHECK(msg.find("no such file or directory") != std::string::npos); + } +} + // ─── extras carry the family-specific knob, in BOTH directions ────────────── TEST_CASE("video engine seam: H3's partition rides in extras and still guards") { SeamWorkspace ws;