fix(windows): 46 → 36 failing packages on the experimental CI lane - #25
Merged
Conversation
The windows CI lane fails 46 of the module suite's packages. This lands the
six whose cause is ours rather than a dependency's, each verified against the
lane's own log (run 31109255741, job 92642423611).
- serving/admin: pathWithinDir's escape arm tested only "../", but PathRel
hands back an OS-native relative path, so an escaping result reads "..\evil"
on Windows and was waved through as contained — a containment bypass, not
just a red test. Split the check into hasParentPrefix, which tests the
native separator too. Fixes TestPathWithinDir_Containment_Good and
TestResolveModelNameToPath_SymlinkEscape_Bad.
- serving/admin tests: two reload bodies pasted a filesystem path raw into a
quoted JSON literal; "C:\Users\..." decodes as the invalid escape "\U".
Route both through a jsonString helper.
- serving/scheduler: the BeginPrepare span was recorded with a bare
time.Since, and Windows' monotonic tick returns exactly 0 for a
sub-microsecond prepare — blanking PrefillDuration and, via its positive-
span guard, PrefillTokensPerSec as well. Floor it in measuredSpan.
- inference (root): TestGGUF_DiscoverModels_Ugly built its expectation with
core.JoinPath (always '/') and compared it against a path DiscoverModels
produced with native separators — PathJoin is the one that matches.
TestDiscover_Good_RelativeBaseDir asked PathRel to relativise t.TempDir()
against the cwd, which cannot be expressed when CI puts the checkout on D:
and TEMP on C:; the base now sits under the cwd.
- kv/blockcache, model/bundle: five tests inject faults through POSIX-only
filesystem semantics — chmod 0o500 on a directory (Windows os.Chmod only
toggles FILE_ATTRIBUTE_READONLY and still permits creates and unlinks) and
a directory's non-zero Stat size (0 on Windows, so ReadFull is handed an
empty buffer and cannot fail). Skipped there with the reason recorded, so
the arms keep their POSIX coverage instead of asserting a false negative.
Receipt — macOS, the required lane's platform:
go test -count=1 ./serving/admin/... ./serving/scheduler/... \
./kv/blockcache/... ./model/bundle/... ./lab/... .
ok serving/admin 0.870s · serving/scheduler 0.549s · kv/blockcache 1.501s
ok model/bundle 1.222s · lab 0.624s · inference 0.316s
gofmt -l: clean · go vet: clean
lab's TestCmd_RunServe_Bad_ListenAddrInUse asserted the POSIX errno text
("in use"); Windows says "Only one usage of each socket address ... is
normally permitted". It now asserts the syscall stage ("bind:"), which both
report and which is what the test is actually pinning.
The remaining 40 packages are dependency-side and ledgered separately.
Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughThe changes improve cross-platform test behaviour, strengthen model-path traversal checks, encode filesystem paths correctly in reload tests, and ensure scheduler prefill measurements are at least one nanosecond. ChangesCross-platform and runtime behaviour
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…r separator
core.PathBase and core.PathDir match only the platform's own separator
(core.Env("DS")), so on Windows a '/'-separated path reads as having no
separator at all: PathBase("/models/gemma3-1b") returns the whole string,
PathDir("/models/x/adapter.safetensors") returns ".". Go accepts '/' on every
platform and it arrives from config files, CLI flags, HTTP model ids and hub
refs, so four call sites deriving a name or a parent from a caller-supplied
path were wrong there — not merely red under test.
internal/pathx carries the both-separator Base/Dir/Join. '\' is a boundary
only where the platform says so; on POSIX it stays an ordinary filename
character, which the _Ugly tests pin on both hosts.
- eval/bench: run.Model is a hub ref ("org/model-a") as often as a path, and
those are '/'-separated everywhere — the matrix row was named for the whole
ref instead of its base.
- train/lora: adapterConfigPath resolved the sidecar of
"/models/my-lora/adapter.safetensors" to "./adapter_config.json" — the wrong
directory, not just wrong-looking. Its join also hardcoded '/' onto a
PathDir result; pathx.Join keeps whichever separator the input used, so a
'/'-spelled Windows path no longer comes back mixed. The filepath.Clean-free
fast path the comment defends is preserved.
- serving/compat: resolverModelNames published the entire ModelPath as the
OpenAI model id.
- model/arch/google/gemma3/gguf: gemma3ModelName wrote the whole checkpoint
path into general.name.
Receipt — macOS:
go test -count=1 ./internal/pathx/... ./train/lora/... ./eval/bench/... \
./serving/compat/... ./model/arch/google/gemma3/...
ok internal/pathx 0.260s · train/lora 0.718s · eval/bench 0.839s
ok serving/compat 0.300s · gemma3 0.771s · gemma3/gguf 1.026s
gofmt -l: clean · go vet: clean · go build ./...: clean
Co-Authored-By: Virgil <virgil@lethean.io>
The windows lane's first run past the prefill fix moved the failure to the next assertion in the same test: "final durations Total=0s Decode=0s". Same cause — Windows' monotonic tick cannot resolve the simulated lane's spans, so time.Since returns exactly 0 — and the same consequence, since DecodeTokensPerSec's divide is guarded on a positive DecodeDuration. Route both through measuredSpan, as PrefillDuration already was. Receipt: 46 → 41 failing packages on the windows lane from the previous commit (run 31247873871, job 93079323014); serving/scheduler was the one package whose failure moved rather than cleared. go test -count=1 ./serving/scheduler/ → ok 0.551s Co-Authored-By: Virgil <virgil@lethean.io>
The pathx switch made TestInspect_Inspect_Good fail on the windows lane — a test that passed before, so a regression I introduced. It asserted info.Name != core.PathBase(identityPath), recomputing the expectation with the same DS-literal helper the production code had just moved off: with identityPath spelled "/adapters/original/support-tone", Inspect now correctly answers "support-tone" while core.PathBase on Windows answers the whole path. Spell the expected name out instead, so the assertion pins the behaviour rather than a helper that shares the bug. Receipt: go test -count=1 ./train/lora/ → ok 0.296s The other three lora failures this lane started with are already cleared — run 31248300740 shows Name resolving to "support-tone" on Windows. Co-Authored-By: Virgil <virgil@lethean.io>
…dent
pathx called core.Env("DS") up to three times per Base/Dir/Join. Env walks
systemInfo's map and falls back to os.Getenv when the key is unset — the
common case for "DS" — and lora's adapterConfigPathPrecomputed, which the
surrounding comment documents as an Inspect hot path, now routes through here.
Cache it behind sync.Once exactly as discover.go's pathSeparator already does
for joinPath/cleanPath, and for the same reason: the override is set once at
process start and never mutates.
Receipt:
go test -count=1 ./internal/pathx/ ./train/lora/ ./eval/bench/ \
./serving/compat/ ./model/arch/google/gemma3/gguf/
ok internal/pathx 0.289s · train/lora 0.335s · eval/bench 0.570s
ok serving/compat 0.699s · gemma3/gguf 0.947s
Co-Authored-By: Virgil <virgil@lethean.io>
Snider
changed the title
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
1 participant
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.
Portability fixes for the experimental windows CI lane, each traced to that lane's own log rather than guessed at. 46 → 36 failing packages, with linux + macos (the required checks) green throughout and nothing newly broken.
Measurement
0070ed18Packages that went green:
inference(root),eval/bench,kv/blockcache,lab,model/arch/google/gemma3/gguf,model/bundle,serving/admin,serving/compat,serving/scheduler,train/lora.Two bugs that were wrong on Windows, not merely red
serving/admin— containment bypass.pathWithinDir's escape arm tested only"../".core.PathRelreturns an OS-native relative path, so an escaping result reads"..\evil"on Windows and was waved through as contained. Split intohasParentPrefix, which tests the native separator too.train/lora— sidecar read from the wrong directory.adapterConfigPath("/models/my-lora/adapter.safetensors")resolved to"./adapter_config.json".internal/pathx— splitting on either separatorcore.PathBaseandcore.PathDirmatch only the platform's own separator (core.Env("DS")), so on Windows a/-separated path reads as having no separator at all:PathBase("/models/gemma3-1b")returns the whole string,PathDir("/models/x/adapter.safetensors")returns".". Go accepts/on every platform and it arrives from config files, CLI flags, HTTP model ids and hub refs.internal/pathxcarries the both-separatorBase/Dir/Join.\is a boundary only where the platform says so — on POSIX it stays an ordinary filename character, which the_Uglytests pin on both hosts. Four call sites moved across:eval/bench—run.Modelis a hub ref (org/model-a) as often as a path; the matrix row was named for the whole ref.train/lora— pluspathx.Join, which keeps whichever separator the input used, so a/-spelled Windows path no longer comes back mixed. Thefilepath.Clean-free fast path the comment defends is preserved.serving/compat—resolverModelNamespublished the entireModelPathas the OpenAI model id.model/arch/google/gemma3/gguf—gemma3ModelNamewrote the whole checkpoint path intogeneral.name.Clock, encoding, and POSIX-only semantics
serving/scheduler— the prefill, total and decode spans used a baretime.Since; Windows' monotonic tick returns exactly0for a sub-microsecond span, blanking the durations and, via their positive-span guards, the throughput rates too.measuredSpanfloors them.serving/admin(tests) — two reload bodies pasted a path raw into a quoted JSON literal;C:\Users\…decodes as the invalid escape\U.inference(root) —TestGGUF_DiscoverModels_Uglybuilt its expectation withcore.JoinPath(always/) and compared it against a native-separator path;TestDiscover_Good_RelativeBaseDiraskedPathRelto cross volumes (CI puts the checkout onD:, TEMP onC:).lab— asserted the POSIX errno text ("in use"); Windows says "Only one usage of each socket address … is normally permitted". Now asserts the syscall stage ("bind:"), which is what the test actually pins.kv/blockcache,model/bundle— five tests inject faults through POSIX-only semantics:chmod 0o500on a directory (Windowsos.Chmodonly togglesFILE_ATTRIBUTE_READONLYand still permits creates and unlinks) and a directory's non-zeroStatsize (0on Windows, soReadFullgets an empty buffer and cannot fail). Skipped there with the reason recorded, so the arms keep their POSIX coverage instead of asserting a false negative.Ledger — the remaining 36, none fixable here
No
replaceingo.mod, so all three causes are dependency-side.30 packages — core/go #21 (open, awaiting review).
io.Localislocal.New("/"), and"/"is not an absolute root on Windows — it names the current drive. Both failure shapes follow: an absoluteC:\Users\…under aD:-rooted medium givespath escapes from parent, and a relativetestdata/x.jsonresolves against the drive root instead of the cwd, givingThe system cannot find the path specified. #21'sFs.pathfix covers both.5 packages —
dappco.re/go/processv0.16.1.lookPath(exec/exec.go:301) joinsdir + fileand stats it with noPATHEXTexpansion, sogit.exeis never found by the bare namegit.commandContext(:288) then hand-rolls&exec.Cmd{Path: name}instead of callingexec.CommandContext, so that failure is swallowed rather than surfaced asCmd.Err— and Go's WindowsCmd.Startresolves the surviving bare name relative toCmd.Dir, producingexec: "C:\…\acceptance source\git".1 package —
serving/chathistory. DuckDB holds its database file withoutFILE_SHARE_READon Windows, soCopyTo's second descriptor onto the live file cannot open. Attempted the DuckDB-nativeATTACH+COPY FROM DATABASEand reverted it: the copy does not order tables by foreign key (Violates foreign key constraint because key … does not exist in the referenced table) and it changes the error surface of two further tests. Needs a real fix, not a quick one.Also parked:
train/{,distill/,grpo/}checkpointMetadataPathshare thecore.PathDirbug fixed elsewhere here, but those packages stay red on #21 regardless, so the change could not be verified green — better done when the lane can prove it.Receipts
CI linux + macos green on every commit in this branch.