fix(#664): the video registry's existence probes stop reaching Windows with POSIX stat - #677
fix(#664): the video registry's existence probes stop reaching Windows with POSIX stat#677localai-bot wants to merge 3 commits into
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]
|
Closing as superseded by #736, which carries byte-identical content with clean commit trailers. WhyThis PR failed As operator I merged Why a new PR rather than a fixed history hereRepairing it in place means rewriting commits that are already pushed, and I am not going to force-push. So the corrected history went to a new branch instead, and this one closes with the reason attached to the diff it excuses — which is what this project uses instead of a waiver registry. Nothing substantive was lost
The two Continue at #736. |
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
|
Closing as already landed — the change here is on Verified rather than assumed:
The three For anyone tracing this later: the Windows arm's remaining red is No work is lost by closing this; the fix is in the tree. |
Closes #664.
windows-msvc-cpuandwindows-msvc-vulkanare failing 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 causes all of it.src/vllm/multimodal/video_engine.cpplanded incefacd2d0carrying<sys/stat.h>,::statandS_ISDIR;scripts/check-windows-portability.py:1675-1688flags all three underfull_source_posix, which applies to every scanned source and not only theREQUIRED_CPPplatform-boundary set.mainwas never a denominator, because the Windows jobs are PR-only and areskippedon push runs (#584).Repaired at the source. The checker's semantics, allowlists and scope are untouched — the checker is right and the source was wrong.
The change
IsDir/Existsnow take thestd::error_codeoverloads of<filesystem>through a file-localNativePath, and<sys/stat.h>is gone.Two things preserved deliberately
1. The non-throwing contract.
::statreported an uninspectable path —ENAMETOOLONG,ELOOP,EACCESon a parent — by returning-1, which arrived here as a plainfalseand became the registry's ordinary"no such file or directory"refusal. The throwingstd::filesystem::exists(p)/is_directory(p)overloads raisefilesystem_errorfor exactly those cases. That exception escapesReadVideoCheckpointTensorNames, whose header contract (include/vllm/multimodal/video_engine.h:125-126) is to returnfalsewith*whyset, and throughDescribeCheckpointit escapesLoadVideoEnginein place of the refusal that names the registered families.2.
NativePath(). Nothing in the tree exports one to call —fs_io.cpp:31andminimax_h3_sharded.cpp:55both defineNativePathin their own anonymous namespace, andgguf_reader.cpp:22/safetensors_reader.cpp:29defineUtf8Paththe same way. The spelling adopted here isfs_io.cpp:31's, byte-for-byte, and deliberately notminimax_h3_sharded.cpp:55'sMultiByteToWideCharvariant: that one throwsstd::invalid_argumenton a malformed-UTF-8 path under_WIN32, which would reintroduce on Windows precisely the escaping exception this change removes on POSIX.RED first, twice
The portability red is the checker itself, on the unmodified tree at the base SHA:
After: exit 0,
Windows portability contract OK.The behavioural red is what a naive rewrite silently breaks. The throwing rewrite was written first and the new test run against it:
No token gate, golden or e2e render could have caught this — every checkpoint they hand over is perfectly stattable. That is also why the probe is a 300-character path component and not a missing file: a missing file is
ENOENT, and the throwing overloads do not throw forENOENT, so a missing-path test passes with and without the guarantee.Mutations
Both anchors asserted
count == 1, tree restored and verified bysha256sum -c.Exists→ throwing overloadIsDir→ throwing overloadThe survivor is reachability, and it is measured rather than asserted.
IsDirhas exactly one call site (video_engine.cpp:180), gated byExistsat: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 whichexists()succeeded whileis_directory()set an error code, because on this libstdc++ both resolve through the samestat(). Short of a TOCTOU race, no input distinguishes the twoIsDirspellings through the shipped surface. Theerror_codeoverload is kept there anyway: same contract, and a later caller probing a directory without a precedingExistswould otherwise inherit the throw.Gate
scripts/check-windows-portability.py: exit 0.ctest -N: 423, unchanged frommain, so the denominator did not drift.test_video_engine: 11 cases / 254 assertions → 12 cases / 260 assertions, exit 0 both times.BUILD_EXIT=0, zeroNo space left/BFD assertionlines in the build log.Two pre-existing reds, both verified against the base SHA and neither caused here:
test_cpu_x86_llamacpp_floor—NO_QUIET_WINDOW(exit 4,load=118) while this branch's own-j4build was running. Documented load artifact.tests/scripts/test_check_windows_portability.py— 2 of 71 fail on this tree and identically at the base SHA (verified by stash): theltx2.cpp/ltx2_video_vae.cpp/ltx2_audio_vae.cppallocation-and-math case, plus one fake-runner-scoped case. Not invoked by any workflow.Overlap with #524 — 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:std::stringstraight tostd::filesystem, with noNativePath— so on Windows the path is interpreted in the active code page and a non-ASCII checkpoint path silently resolves to the wrong file;ENOENT, which passes with the throwing overloads too, so it does not hold the contract it looks like it holds.The two hunks will conflict. Whichever lands second should take this file's version wholesale. Separately, #524 still carries the
tests/vt/test_backend_cross_device.cppsetenvrepair that11cc1d5896already landed viatests/support/test_env.h.Scope
src/vllm/multimodal/video_engine.cpp, its test, and the.agents/roadmap_v1.mdissue-table row for #664. No othersrc/file, and notscripts/check-windows-portability.py.