fix(#664): the video registry's existence probes stop reaching Windows with POSIX stat - #736
Merged
Merged
Conversation
…s 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 cefacd2 carrying <sys/stat.h> 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 11cc1d5: $ 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 11cc1d5 - 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 11cc1d5 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]
FOLLOWING_AGENTS_PROTOCOL Brings the branch onto current main so the operator gate runs on the tree that will actually land. No conflicts; the only shared keyed record is the roadmap issue table, where main added four distinct keys in different regions and this branch adds the #664 row -- verified additive rather than accepted blind. Replaces two earlier merge commits that carried the DEFAULT `git merge` message and therefore no trailers at all, which reddened `agent-record`: d4e8dd4 [attribution] AI-Assisted must appear exactly once 72fb570 [trailers] FOLLOWING_AGENTS_PROTOCOL must appear exactly once ... The resulting TREE is byte-identical to 72fb570's, so the operator gate already run at that SHA still describes this content. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5[1m] [claude-code]
FOLLOWING_AGENTS_PROTOCOL Main moved again while this waited on CI. Merged so the operator gate runs on the tree that will actually land, and so the keyed records are reconciled by key rather than by an accepted auto-merge. This merge message carries the trailers deliberately: the predecessor PR (#677) was closed because two `git merge --no-edit` commits of mine wrote the DEFAULT message and therefore no trailers at all, reddening `agent-record`. A merge is a commit and owes the same attribution as any other. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5[1m] [claude-code] # Conflicts: # .agents/roadmap_v1.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #677 with identical content and clean commit trailers. Closes #664.
git write-treeon this branch equals72fb570dd191's tree byte-for-byte (ff87a88909431431746013a40be29e05153fa079), and the fix commit1f45c606fis unchanged. So the review and the operator gate already run at that SHA describe this content exactly.Why a new branch instead of a corrected #677
#677 failed
agent-record— on my commits, not the implementer's:I merged
origin/maintwice withgit merge --no-edit, whose default message carries no trailers, and the checker requires them on every commit. Repairing that in place would mean rewriting pushed history, and force-pushing is not something I will do — so the corrected history lands on a new branch and #677 closes with the reason recorded.Every commit in this range now carries the bare
FOLLOWING_AGENTS_PROTOCOLline and all three trailers.The fix
src/vllm/multimodal/video_engine.cppreached Windows with unguarded POSIXstat—:21#include <sys/stat.h>,:57-60IsDir,:62-65Exists— failingwindows-msvc-cpu/windows-msvc-vulkanon every open PR in the repository.IsDir/Existsnow route through thestd::error_code<filesystem>overloads via a file-local path helper, preserving the non-throwing contract::stathad.scripts/check-windows-portability.pywas not touched.The path helper adopts
fs_io.cpp:31's spelling byte-for-byte, deliberately notminimax_h3_sharded.cpp:55'sMultiByteToWideCharvariant, which throwsstd::invalid_argumenton malformed UTF-8 under_WIN32— that would reintroduce on Windows the escaping exception this fix removes on POSIX. (The wider divergence is #687.)Review verdict, carried forward
Reviewed PASS at
1f45c606f. The reviewer independently reproduced the premise from another lane's CI log, confirmed by sha256 that the checker was untouched, and built its ownerrnoprobe table showing ENOENT does not throw — so a missing-path test is vacuous and the real probe is a 300-character path component.On the one surviving mutation (
IsDirwith the throwing overload) it rejected the implementer's probe as insufficient — "fails to refute is not unreachability" — and established unreachability from the library mechanism instead:IsDiris TU-local in an anonymous namespace with one call site, gated byExists, and libstdc++ implements both viaf(status(p)), soexists()can only return true when thestat()already succeeded. The only distinguishing input is a TOCTOU race, which is not gateable.Operator gate, at the identical tree
What this does NOT do
It does not turn the Windows lane green on its own. Two independent defects were stacked. With this one removed the build gets much further and hits
M_PI, which MSVC does not define — #720, fixed by #732 (reviewed PASS). Both must land. Honest statement: this removes one of the two remainingerror Csets, not that Windows is green.Also related: #680 (
test_check_windows_portability.pyis run by no workflow), #584 (windows-msvc-*skipped onmain, so no baseline). Those two blind spots are why both defects landed unseen.🤖 Generated with Claude Code