fix(modelartifacts): stage each writer's artifact in its own partial tree

[localai-bot]

Problem

.artifacts/.partial/<cacheKey> was shared mutable state with no second line of defence — safe only because the artifact lock held.

Two writers that both believed they held it opened the same blob with O_APPEND (pkg/downloader/uri.go:706) and interleaved into one file, while the resume probe (uri.go:634) read the other writer's in-flight size. SHA verification caught it only after both had burned the entire download — 57 GB, in the incident that surfaced this.

#10986 restored the lock's precondition on CIFS. It did not remove the dependency on it.

Change

Stage into .partial/<cacheKey>.<writerID>, where writerID is 64 bits of crypto/rand drawn once per process run.

Concurrent writers now cannot corrupt each other regardless of what the lock does. The lock stops being a correctness dependency and becomes a pure efficiency optimisation — a lock failure costs a duplicated download, not a corrupted one.

The lock is retained and still has that job: it stops N replicas each pulling the same 57 GB, and it is what proves a peer is not writing (see adoption below).

Commit stays an atomic rename. The loser of a commit race now reconciles onto committedResult and discards its own tree, rather than surfacing a bare ENOTEMPTY for work that actually succeeded.

writerID is crypto/rand rather than PID or hostname: PIDs repeat freely across containers sharing a NAS mount, and a restarted pod can inherit a hostname while the dead pod's tree is still on disk. A collision needs two replicas to draw the same 64 bits within the same window, and even then degrades to the previous shared-tree behaviour rather than anything worse.

Consequences handled

Disk leak. A crashed writer's tree is no longer overwritten by its successor, so it would leak on a shared NAS forever. A 24h sweep (matching the existing startup reaper's window) runs at startup and on the materialization path under the lock. Three independent guards make deleting a live tree impossible:

1. the name must parse as <64-hex cacheKey>.<16-hex writerID> — a shape only this package writes
2. it must not be the caller's own tree, passed explicitly
3. the newest mtime anywhere inside the subtree must be older than 24h

That third guard matters: the directory's own mtime is useless, because writing .downloads/<blob>.partial never touches an ancestor. A running download writes continuously, so its newest mtime is seconds old; a silently stalled one is killed by the downloader's DownloadStallTimeout watchdog after 60s. An unreadable entry counts as "now", so a tree that cannot be fully inspected is never judged stale.

Resume. A new process would not find its predecessor's partial and would restart from zero — a real regression on a 57 GB repo. Adoption lets it claim an idle same-cacheKey tree by os.Rename into its own writer path. The rename is atomic, so racing adopters cannot both win and the loser simply starts fresh. Adoption runs only while holding the artifact lock — flock releases exactly when the owning process dies, which is the liveness signal — and is additionally gated on 5 minutes of idleness.

The cost, stated deliberately: a restart that retries within 5 minutes re-downloads from zero. A longer window means more wasted re-downloads after a crash; a shorter one means a broken lock could let us claim a live peer's tree. Both directions cost exactly one wasted download, because every adopted blob is still SHA-verified before promotion — a wrongly-adopted tree fails verification rather than committing corruption. Symmetric costs, so 5 minutes is a middle, not a derived optimum, and is easy to revisit.

Tests

The headline spec runs two writers concurrently with the lock bypassed via WithLocker, using a rendezvous server to force overlap. On master it fails with interleaved bytes:

[FAILED] writer 0 must not be poisoned by its peer
  SHA mismatch for file ".../.partial/b79e23e0.../.downloads/9d75c109..."
  ( calculated: 4695dbc7... != metadata: feb1e440... )
Ran 38 of 38 Specs — 37 Passed | 1 Failed

That is the corruption, reproduced — not a missing symbol.

Plus specs for sweep safety (live tree, own tree, foreign directory, missing root, and the "old directory, fresh blob inside" case that a naive directory-mtime check would get wrong), adoption with a real Range-resume assertion, and writer-identity validation.

go build ./core/... ./pkg/... OK; go test ./pkg/modelartifacts/ ./pkg/downloader/ ./core/gallery/ ./core/config/ all ok; -race clean over 5 consecutive runs; make lint 0 issues. Repo-wide grep including tests/e2e/ and docs: nothing outside the package asserts the .partial layout. Dropped Layout.PartialSnapshot, which was set but never read.

Follow-up, not done here

Writer-unique staging makes the previously-sketched design cheap to add: a brief advisory-lock election plus an expiring on-disk claim, with no lock held across the download. The writer identity and the atomic-rename claim are exactly the pieces that needed. Deliberately out of scope.

---

🤖 Generated with Claude Code