[aditya-rai-5]

What

Validate unpinned cached files in multi-file models against their recorded staged size, keeping entries without a record and backfilling their local size where safe, and check the published file size after staging. Prevent serving pods from staging into FUSE-backed caches where cross-node locking cannot be trusted.

Why

Concurrent staging into a shared ReadWriteMany cache can leave a truncated weight file that later pods accept based only on its existence.

Fixes #2158

How

• Use recorded staged sizes to validate IfNotPresent cache hits locally, while keeping entries without a record without contacting the origin and backfilling their local size where safe. A file that mismatches a valid record is downloaded again. Checksum-pinned files retain local verification.
• For OnChange, check the recorded staged size when revalidation fails and the origin size is unavailable, so a known truncated file is not accepted.
• Detect FUSE using its numeric filesystem identifier, which works with the downloader image’s BusyBox stat.
• Behavior change: serving pods refuse actual multi-file staging on FUSE mounts, including virtiofs/Kata volumes. Warm caches remain usable.
• Allow the prefetch Job to stage on FUSE. Its pod retries wait for failed pods, and Job replacement uses foreground deletion so an old writer finishes before the new Job starts.
• Add regression coverage for cache sizes, FUSE detection, and Job replacement; update generated command goldens and cache documentation.
• For OnChange, a failed refresh compares the cached file with its own size record, even if the origin now reports a different size. A file without a valid record is kept with a warning.
• Write staged size records using a temporary file and rename so concurrent readers cannot observe a partially written number.

Focused regression and golden tests, make test-affected, and make gate passed locally. Azure Blob CSI validation remains pending.

Assisted-by: Codex (some part of implementation).

Checklist

• Tests added/updated
• make test passes locally
• make lint passes locally
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated (if user-facing change)

[Defilan]

Thanks for picking up #2158. This is a tricky corner of the downloader, and the PR shows real care: the post-publish size check is a good idea, the HF probe correctly follows the CDN redirect and reads the final Content-Length, the token stays out of the logs, and the FUSE magic check is right for BusyBox (I confirmed stat -f -c %t prints 65735546 in curlimages/curl:8.18.0). Lint passes on darwin and linux.

I ran the golden scripts in the real init image (curlimages/curl:8.18.0) against a local origin, comparing this branch with its base. A few behavior changes need addressing before this can merge.

1. A warm cache can no longer start offline. With IfNotPresent (the default), a complete, correct cached file now needs a successful HEAD on every pod start. If the origin is unreachable, returns an error, or is rate limited, the script treats the file as unverified, tries to re-download it, fails, and exits 1. Before this change, the same pod started from cache without touching the network.

complete cached file, origin...	before	this PR
unreachable (http, s3)	starts from cache	init fails
503 (http, hf with token)	starts from cache	init fails

That breaks the air-gapped workflow in docs/MODEL-CACHE.md and the IfNotPresent contract in api/v1alpha1/model_types.go ("download only if the cached file is missing ... an operator upgrade triggers no surprise re-pulls"). TestModelDownloadLock_ReadOnlyWarmCache pointed at an unreachable origin on purpose to guard this case. Please keep that test as it was rather than giving it a live origin.

GPU Deployments use the Recreate strategy, and this PR changes the init command, so every multi-file cached Deployment rolls on upgrade. The old pods would stop first, and the new ones could not start.

Suggested direction: re-stage only on a definite mismatch (the origin answered with a numeric Content-Length that differs from the local size). Keep the cached file, with a warning, when no size is available. Even better would be a comparison that needs no network at start: the controller already gets per-file sizes from the HF tree listing (hf_listing.go), and those could be passed in as env the same way MODEL_FILE_SHA256 is. A local size record written after a verified publish would also work.

2. OnChange lost its keep-cached fallback. An unreachable origin used to log "Revalidation unreachable ... kept cached copy" and start. Now it fails (http and s3). That also diverges from the single-file revalidation path, which still keeps the cached copy.

3. No Content-Length means a re-download on every start. If the origin, or a mirror or proxy in front of it, answers HEAD without Content-Length, every pod start re-downloads the full file (I saw 2 starts produce 2 full GETs). A 429 from Hugging Face does the same. HF counts each /resolve/ request against the Resolvers limit (3,000 per 5 minutes per IP for anonymous users, https://huggingface.co/docs/hub/rate-limits), so a large rollout could hit that limit and turn warm starts into full re-pulls. Fixing item 1 fixes this too.

4. The FUSE gate. Detection works, but a few things need changing:

• It runs before anything else, so it also blocks a FUSE cache that is already fully staged and needs no writes. The issue asks that staging not proceed without a working lock. Could the check run only when a file actually needs downloading? As written, the reporter's existing blobfuse deployment would go down on upgrade unless they set prefetch first.
• The error tells users to set spec.modelCache.prefetch: true, but s3:// sources are not prefetchable (prefetchableSource in prefetch.go). An s3:// multi-file Model on a FUSE cache would have no way to start.
• The message does not match any of the transfer-failure signatures in model_transfer_diag.go, so the InferenceService gets no condition. Users would only see Init:CrashLoopBackOff. A signature with a clear reason and remedy would help a lot.
• 0x65735546 also matches virtiofs, so volumes under Kata Containers (and virtiofs bind mounts generally) would be refused too. Worth a line in the docs.
• Please call this out as a behavior change in the PR description so it lands in the release notes.

5. The controller suite fails. With envtest, go test ./internal/controller/... has 4 failing specs in inferenceservice_storage_test.go: the three "uses/generates ... HEAD revalidation for OnChange" specs (they assert kept cached copy) and "generates download loop for IfNotPresent policy" (its expected string predates the publish wrapper). Also, the new shell tests skip on macOS, because requireInitShellEnvironment needs a stat with -c. If you ran them on a Mac, they were skipped rather than run. Running them in a Linux container (for example golang:1.26) exercises them for real. They do pass there on this branch.

Smaller things:

• TestMultiFileOnChangeRejectsUnverifiedCache lists config.json but never caches it, so the script fails on the missing config.json, not on the truncated shard. Restoring the old keep-cached line still passes the test. Caching config.json as well would make it test what it says.
• When the post-publish size check fails, $dest is left in place and only the .sha256 stamp is removed. Removing $dest too would stop an existence-only reader (for example, the prefetched-pod verify step) from picking up the short file.
• For an unpinned revision, an upstream re-upload that changes one shard's size now re-fetches only that shard, so the cache can end up mixing revisions. Comparing against the size recorded at staging time avoids that.
• The foreground deletion change looks reasonable. One note: if the old Job's pod is on a node that went NotReady, the replacement waits until that pod is force-deleted. A hint in the status message or docs would help users unblock it.
• TestMultiFileCachedSize (offline, head-failed) and TestMultiFileOnChangeRejectsUnverifiedCache currently assert the fail-closed behavior, so they will need to flip along with items 1 and 2. An offline warm-cache case that must pass would be a good addition.
• llmkube_cached_size_matches is redefined on every loop iteration. Defining it once in the prefix is enough.

The post-publish size check and "re-stage when the origin definitely reports a different size" are worth keeping. If the fail-closed parts are dropped and the FUSE gate is limited to actual staging, I think this gets close. Happy to talk through the size-from-listing approach if you want to go that way.

[Defilan]

Thanks for the second round. This addresses almost everything from the first review, and the size record is a better design than what I suggested: a warm cache with a record now validates with no network at all. I re-ran the same harness as last time, with the generated scripts in the real init image (curlimages/curl:8.18.0) against a local origin, plus a Docker Desktop bind mount, which reports the FUSE magic (virtiofs).

What's fixed, confirmed in the image:

• Warm caches start offline again (items 1 and 3). With IfNotPresent, a complete cached file starts when the origin is unreachable, returns 503, or sends no Content-Length. It logs a warning and makes no re-download, for http, s3 and HF with a token. A truncated file is re-downloaded when the origin answers, and the size record is written. With a record, the check is local: a complete file starts offline with zero requests, and a second start after a fresh download makes no request.
• OnChange keeps the cached copy when revalidation is unreachable (item 2), for http, s3 and HF.
• The FUSE gate (item 4). A warm FUSE cache starts, with no lock and no writes. Staging is refused with ModelCacheLockUnsupported, the prefetch stager may stage, s3:// gets its own remedy, and the new transfer signature turns the refusal into a condition. The docs cover the behavior change, virtiofs and Kata, and the NotReady recovery.
• The smaller items: $dest is removed on a failed size check, config.json is now cached in the test, the size helper is defined once, and the tests that asserted the old fail-closed behavior have flipped.

Two things before merge:

1. The branch no longer builds against main. #2206 added a prep parameter to buildModelStorageConfig, so the new model_storage_fuse_test.go:59 call is one argument short. CI tests the merge with main, and this one error is all six failures (Lint, Tests and four e2e jobs): not enough arguments in call to buildModelStorageConfig. The fix is to pass ModelCachePrepAlways, as the other test call sites do:

storage := buildModelStorageConfig(model, nil, "default", true, ModelCacheModeShared, "", defaultPrefetchImage, 0, ModelCachePrepAlways, nil, "")

With that one change, rebased onto main: make test passes (53 packages), golangci-lint reports 0 issues on darwin and linux, and the shell-behavior tests pass in a Linux container (golang:1.27.3), where they run rather than skip.

2. OnChange can still serve a file that its own size record proves is truncated. With the origin unreachable, a 5-byte file whose .llmkube-size says 23 is kept: "Revalidation unreachable for model.safetensors; kept cached copy". IfNotPresent correctly fails in the same case. The cause is that keepCachedTail calls llmkube_cached_size_matches "$remote_size" with an empty argument, and any argument skips the record. This is #2158's failure, a truncated shard accepted on existence, and the record you added can catch it. Passing the argument only when it's set fixes it:

llmkube_cached_size_matches ${remote_size:+"$remote_size"}

I ran that change in the image. Offline, a truncated file with a record now fails, a complete file with a record is kept with no warning, and a file without a record is still kept with the warning. With the origin up, the truncated file is repaired. A test for "OnChange, origin unreachable, file shorter than its record, init fails" would pin it.

Optional, but worth considering:

• Pre-existing caches send a HEAD on every start, indefinitely. The record is only written when a file is published, so a cache staged before this change (every current user's) never gets one and probes the origin on each pod start. Writing the record after a successful size match would let those caches go offline-clean after one start, and keeps HF's Resolvers limit out of large rollouts.
• The PR description still describes the first commit, for example "A mismatched or unverified unpinned file is downloaded again." Updating it to the current behavior, with the behavior-change note that's already in the docs, would keep the release notes accurate.

Once 1 and 2 are in, I think this is ready.

[aditya-rai-5]

Addressed both: merged upstream main and updated the storage test call with ModelCachePrepAlways; fixed both OnChange fallback paths to consult the staged size record when the remote size is unavailable. Added regression coverage across HTTP, HF, S3, and partially pinned manifests, and updated the command goldens. make test-affected and make gate pass locally.

[Defilan]

Thanks for the quick turnaround. Both items from the last round are in: the branch builds against main, and the OnChange fallback now consults the size record.

Another pass on fff210a4 found two problems in how caches staged before this change behave after the upgrade. Sorry these are landing after I said it was close; they're in code the earlier rounds didn't catch. These two need fixing before merge:

1. IfNotPresent can mix revisions in a pre-existing cache (inline at model_storage.go:1224).
2. OnChange fails, instead of keeping the cached copy, when upstream changed size and the download fails (inline at model_storage.go:1180).

Both come from treating a cache without a size record as untrusted. One shape that fixes both, and the HEAD-every-start note from last round: under IfNotPresent, trust an existing file that has no record (as main does) and backfill the record, so the next start is offline-clean. Under OnChange, keep the pre-PR "kept cached copy" behavior when the download fails and the only reason for the mismatch is that upstream changed size.

Everything else below is non-blocking. Fix it here if it's cheap, or I'm happy to take follow-up issues:

• Inline: FUSE prefetch stagers of two Models sharing a source and claim (:1157), Content-Length: 0 treated as authoritative (:1220), HEAD with no timeout and no backfill (:1219), the size record written non-atomically (:1206), duplicated remedy text (:1151), the foreground-delete decision depending on a feature-gated field (prefetch.go:235), and the stat -f requirement labelled as a lock problem (model_transfer_diag.go:125).
• Not in the diff: the single-file GGUF cached path (mkcache around model_storage.go:504) still takes cacheDirLock on FUSE, where flock doesn't exclude across nodes, so #2158's truncated-partial race is still reachable for single-file models. Detection next to cacheDirLock would cover both builders. A follow-up issue is fine.

[Defilan]

Needs fixing before merge. For a file with no .llmkube-size (every cache staged before this PR), llmkube_cached_size_matches HEADs the origin and re-downloads the file if the size differs, per file.

Scenario: a Model on unpinned hf://org/repo (tracking main), cache staged by the previous operator. Upstream pushes a new model-00002-of-00004.safetensors or tokenizer.json with a different size. After the upgrade, the next pod start re-downloads only that file. The cache now holds shards and tokenizer from two revisions, and the server loads mismatched weights or crashes. On main, [ ! -f "$dest" ] never touched warm files.

The comment above says the record exists to avoid "refreshing just one shard when a mutable upstream changes", but the no-record fallback does exactly that for every existing cache. Suggest: under IfNotPresent, accept an existing file with no record (as before) and write the record from its local size, or fail closed for the whole manifest rather than refreshing a subset. A regression test for "no record, origin size differs, IfNotPresent: file not re-downloaded" would pin it.

[Defilan]

Needs fixing before merge. When the origin is reachable and upstream changed a file's size, remote_size holds the new size. If the download then fails (network blip, 5xx, disk pressure), this fallback compares the cached file to the new size, which can never match, so the init exit 1s. Before this PR it logged "kept cached copy" and served.

On FUSE it's worse: llmkube_prepare_download always refuses for non-stager pods, so any upstream change to an OnChange model makes every serving pod's init fail. That contradicts the docs' "Warm caches remain usable".

Suggest: when the download fails, judge the cached copy against its own record (or keep it with the warning when there is none), not against the new origin size. The new origin size proves the cache is stale, not that it's corrupt. A test for "OnChange, origin reachable with a changed size, download fails: cached copy kept" would pin it.

[Defilan]

Non-blocking. On FUSE every prefetch stager skips cacheDirLock. The cache dir is keyed on source alone, so two Models on the same hf://org/repo with prefetch: true on one BlobFuse RWX claim run two prefetch Jobs that curl -C - into the same $MODEL_PATH.<key>.tmp partials with no lock. Foreground deletion and PodReplacementPolicy serialize pods within one Job, not across Jobs. The docs say to "avoid" this setup, but nothing enforces it. A follow-up could have the controller refuse, or serialize, a second stager for the same cache key.

[Defilan]

Non-blocking. An origin answering HEAD with Content-Length: 0 passes the numeric case, so a non-empty cached file never matches. IfNotPresent then re-downloads every warm file on every start, and a FUSE serving pod fails with ModelCacheLockUnsupported even though the cache is good. The OnChange probe at the same site already guards with [ "$remote_size" != "0" ]; this helper should treat 0 as "unavailable" too.

[Defilan]

Non-blocking (the follow-up to last round's optional note). The HEAD here has no --connect-timeout/--max-time, and a matching probe never writes .llmkube-size, so a pre-existing cache probes on every start indefinitely. On an air-gapped cluster where the origin is blackholed rather than refused, each probe waits for the TCP connect timeout (about 2 minutes), so a 40-shard model can stall startup for over an hour. A short timeout plus backfilling the record after a successful match would fix both.

[Defilan]

Non-blocking. The record is written with printf > "$dest.llmkube-size" while serving pods on a shared cache may read it concurrently. A reader can see an empty or truncated number (123 of 12345678). The empty case is handled, but a truncated digit string passes the numeric case, mismatches, and triggers a download, or a ModelCacheLockUnsupported failure on FUSE, for a correct file. llmkube_atomic_write (mktemp + rename) exists for this, but only in the withSHA256 helpers; the unpinned path could use it too.

[Defilan]

Nit. The ModelCacheLockUnsupported remedy text lives here and again, with different wording, in the transfer-diag signature. When prefetch support changes (e.g. s3 prefetch), the pod log and the ModelTransferHealthy condition will give conflicting instructions unless both are updated. A shared constant, or letting the diag pass through the script's message, would avoid the drift.

[Defilan]

Non-blocking. This picks foreground deletion only when the Job read back from the API server has PodReplacementPolicy=Failed. With the JobPodReplacementPolicy gate off (alpha and off by default on 1.28, or disabled on 1.29+), the API server drops the field, so FUSE stager Jobs get background deletion and immediate pod replacement, and old and new stagers overlap with no flock. Keying the decision off something the operator owns (a label or annotation set in buildPrefetchJob) and documenting the gate requirement would make the protection hold on any cluster.

[Defilan]

Non-blocking. The script's stat -f failure ("cannot determine cache filesystem") also carries the ModelCacheLockUnsupported token, so it matches this signature with the remedy "set spec.modelCache.prefetch: true", but prefetch would hit the same stat failure. It also fails on every filesystem, not just FUSE, for a custom init image whose stat lacks -f (which worked before on NFSv4/CephFS). Suggest a distinct token for that error, and falling back to cacheDirLock (the previous behavior) when the filesystem type can't be determined.

[aditya-rai-5]

Thanks for the detailed review. This update addresses both blocking findings and three related non-blocking comments:

1. IfNotPresent preserves existing files without a size record without contacting the origin, preventing mixed revisions after an upgrade. It backfills the local size where safe; warm FUSE caches remain write-free.
2. After a failed OnChange refresh, cached files are checked against their own size record, not the changed origin size. Files without a valid record retain the keep-cached fallback; known truncated files still fail.
3. Content-Length: 0 no longer triggers warm-cache downloads under IfNotPresent because that origin probe has been removed.
4. Removing the same probe also eliminates repeated HEAD requests and their startup delays for legacy IfNotPresent caches.
5. Size records now use temporary-file-and-rename writes, reusing the existing atomic writer.

Regression tests reproduced the blocking failures before the fix and pass afterward. Coverage includes HTTP, HF, S3, partially pinned manifests, failed refreshes, known truncation, atomic records, and simulated warm FUSE caches.

I have left these reviewer-designated non-blocking items for follow-up:

6. Cross-Model FUSE prefetch coordination.
7. Consolidating the duplicated remedy text.
8. Job replacement safety when PodReplacementPolicy is dropped.
9. Filesystem-probe failure diagnostics and fallback.

The separate single-file GGUF/FUSE staging observation also remains for follow-up.

Validation:

• Focused behavioral, checksum, wiring, and golden tests passed.
• Controller-package lint: 0 issues.
• GOTOOLCHAIN=go1.27.3 make test-affected: passed.
• GOTOOLCHAIN=go1.27.3 make gate: all steps passed.

Real Azure Blob CSI/BlobFuse validation remains pending; the FUSE tests simulate filesystem reporting.

[Defilan]

Thanks, this round fixes both blockers, and the approach (no origin probe for a file with no record, backfill from the local size) is simpler than what I suggested. I re-ran the generated scripts in the real init image (curlimages/curl:8.18.0) against a local origin, and on a real FUSE mount (fuse-overlayfs, magic 65735546) in Alpine's BusyBox.

Both blockers, confirmed:

• IfNotPresent, cache with no record, upstream changed size: the file is kept, its record is backfilled, and the origin gets zero requests. The next start runs with no warning. No more mixed revisions.
• OnChange, origin reachable with a changed size, download fails (500): the cached copy is kept, with or without a record. A file shorter than its own record still fails, both offline and when the download fails.

Also checked:

• IfNotPresent with a record and a short file: repaired when the origin is up, init fails when it is down.
• FUSE: a warm cache with no record starts and nothing is written. A serving pod refuses to stage a missing file, and the prefetch stager stages it.
• The size record is written atomically.
• Shell tests run (not skipped) in golang:1.27.3 as a non-root user, including TestModelDownloadLock_ReadOnlyWarmCache. The full internal/controller suite passes, and golangci-lint reports 0 issues.

Non-blocking, fine as follow-ups:

1. OnChange on FUSE, serving pod, upstream changed: it logs ERROR: ModelCacheLockUnsupported: ... and then Revalidation unreachable for model.safetensors; kept cached copy, then exits 0. The behavior is right: the init exits 0, so the transfer diagnostics stay quiet. But the log reads as two failures, and "unreachable" is wrong, because the origin answered. The pod also keeps serving the old revision until a stager refreshes it. A message like "upstream changed; a serving pod does not stage on FUSE; kept the cached copy" would be clearer.
2. The FUSE prefetch stager does not backfill the record either, so a FUSE cache staged before this change warns on every start, forever. The stager is the one pod allowed to write there, so it could backfill.
3. The backfill records a legacy file's size as it is, including a file #2158 truncated before the upgrade. The docs say so ("cannot prove that a legacy file was complete"). A line in the release notes would help.
4. The temp files from the record writer (model.safetensors.llmkube-size.XXXXXX) are not swept by the *.tmp cleanup if the pod dies between mktemp and mv. The sha256 stamps have the same gap today, so this is not new.
5. The PR description's second "How" bullet ("check the recorded staged size when revalidation fails and the origin size is unavailable") contradicts the later bullet and the code: a failed refresh now checks the record whatever the origin says. Dropping it would keep the release notes accurate.

The deferred items (cross-Model FUSE stagers, the shared remedy text, the PodReplacementPolicy gate, the stat -f fallback, single-file GGUF on FUSE) are fine as follow-up issues; I can open them.

CI has not run on this push yet, so this merges once it's green.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!