Concurrent pulls corrupt local storage: shared ingest files and non-atomic, lost-update index writes

[gorkem]

Describe the bug

Two PullModel calls running at the same time — two kit pull processes, or two goroutines in a process embedding the library — are not isolated from each other, even when they pull completely different refs. Local storage is a single shared root, so concurrent pulls collide on the ingest directory and on index.json. The worst outcome is silent: a model that pulled successfully disappears from the shared index and becomes eligible for garbage collection.

There is no cross-process locking anywhere in pkg/ (no flock, no lockfile), so nothing prevents this today.

Root cause: refs do not partition storage

pkg/cmd/pull/pull.go:41-42 builds every repo on one root, constants.StoragePath(configHome), and newLocalRepoForName (pkg/lib/repo/local/registry.go:68-87) opens the oci.Store at that root directly. The ref only selects a per-repo index filename (<base64(repo)>-index.json), never a subdirectory. All refs therefore share one blobs/sha256/ and one ingest/. Because storage is content-addressed, two different refs that share a layer digest — same base model under two tags, a common case — resolve to the same physical paths.

Four concrete failures

1. Ingest file collision on the resumable download path. resumeAndDownloadFile (pkg/lib/repo/local/pull.go:146-147) opens a fixed filename ingest/<digest> with O_CREATE|O_RDWR and no exclusivity. The pulledDigests map at pull.go:77 only dedupes within a single PullModel call. Two concurrent pulls of the same digest interleave writes into one file, and the resume logic (pull.go:159-169) reads the other writer's partial bytes as its own resume offset. After the first pull renames the file into blobs/, the second writer's still-open fd points at the now-live blob and keeps writing into it. The non-seekable downloadFile path is safe here — it uses os.CreateTemp with a random suffix (pull.go:198).

2. cleanupIngestDir deletes other pulls' in-flight files. A completing pull calls cleanupIngestDir (pull.go:118), which removes every file under the shared ingest/ dir (pull.go:248-266) — including another pull's active ingest file. That pull's final os.Rename then fails with ENOENT and its resume state is gone.

3. Lost updates on the shared index.json. PullModel calls l.Store.Tag(ctx, desc, desc.Digest.String()) at pull.go:107. In oras-go v2.6.x, oci.Store.saveIndex rewrites the whole index.json from that instance's in-memory tag map via os.WriteFile. Its mutexes are per-Store-instance and protect nothing across two localRepo instances or two processes. Concurrent pulls of any two refs — including different registries and repositories — race on this one file, and the loser's manifest entry vanishes. Per the comment at pull.go:106, that entry is exactly what keeps the manifest safe from garbage collection, so a successful pull silently becomes GC bait.

4. Non-atomic index writes. Both localIndex.save (pkg/lib/repo/local/repository.go:104) and tagsIndex.save (repository.go:224) write in place with os.WriteFile — truncate, then write. A crash or a full disk mid-write leaves truncated JSON, and a concurrent reader can observe a partial file. Separately, each localRepo loads its index at construction, so two pulls of different tags in the same repo perform an unlocked read-modify-write and the last writer silently drops the other's manifest and tag.

To Reproduce

Cross-repo, hits failure 3 — the two refs share no content at all:

kit pull registry-a.example.com/org/model-a:latest &
kit pull registry-b.example.com/org/model-b:latest &
wait
kit list   # one of the two models is missing from the shared index

Same-repo with shared layers, hits 1, 2 and 4 as well:

kit pull registry.example.com/org/model:v1 &
kit pull registry.example.com/org/model:v2 &
wait

Failure 1 needs a registry that supports range requests, since it only affects the resumable path.

Expected behavior

Concurrent pulls either serialize safely or fail loudly. Neither should ever leave storage in a state where a completed pull is missing from an index, an index file is truncated, or a blob contains interleaved bytes from two writers.

Additional context

Found while reviewing serialization in the local repo layer. Filing separately: the existing work is about making in-process serialization correct, whereas this is a distinct set of failure modes across processes and storage layout, with its own design decisions.

[amisevsk]

To be clear, this issue mainly/only impacts usage of KitOps as a library, and not the CLI, correct?

[amisevsk]

Some of these issues are by design and generally required for CLI usage:

1. Ingest file collision on the resumable download path.

This is what makes resumable downloads work; between entirely separate runs of Kit, unless we use a deterministic filename to resume the download, Kit will be unable to find the in-progress download file. See PR #408

2. cleanupIngestDir deletes other pulls' in-flight files.

This is also necessary; if a pull is aborted, the owner of those ingest files no longer exists, and if we don't delete them somewhere the ingest directory will grow in size over time. This was also required to support resumable downloads in #408

3. Lost updates on the shared index.json

This is genuine but hard to address -- it would require creating a lockfile, which in turns means we have to consider what happens when KitOps dies during an operation (a leftover lockfile means no changes can happen). This is why we've generally not supported parallel pulls. As a workaround, since blobs are already present in the missing manifest case, a simple retry of the pull should add it without downloading any significant data.

4. Non-atomic index writes.

This issue is not stated very cleanly -- non-atomic writes can mean a corrupted index, which is bad, though I'm not sure there's much we can do if e.g. the disk is genuinely full. The solution would likely be to write a new index and swap it with the old one on success, but the risk of a corrupted index in this way seems minimal to me.

The second part (Separately, each localRepo loads its index at construction, so two pulls of different tags in the same repo perform an unlocked read-modify-write...) appears to be a restatement of issue 3.