Serialize and atomically publish OCI cache writes

[sjmiller609]

Summary

layout.AppendImage does an unsynchronized read-modify-write of index.json. Registry pushes, remote image pulls, and builder imports could all append at the same time, which lost descriptors or left the index malformed.

• Add ocicache.AppendImage and point the three callers at it. Blob writes run concurrently; the index append holds a package mutex.

• Every cache file (layers, config, manifest, oci-layout, index.json) is written to a temp file, fsynced, and renamed into place. A crash or ENOSPC mid-write never replaces a good file, a renamed file survives power loss on XFS, and readers of index.json, which take no lock, never see a partial file.

• Blobs are skipped only when the existing file has the expected size. The library wrote config and manifest straight to their final path and skipped any existing file regardless of size, so a blob truncated by a killed writer was trusted forever.

• The registry's Docker manifest blob goes through the same writer. It was an os.WriteFile on the final path, which a second PUT of the same digest could truncate while the first PUT's conversion was reading it.

• Initialize the layout only when it does not exist. The old call sites fell through to layout.Write on any FromPath error, which replaces the index with an empty one.

• Copy the descriptor before annotating it. partial.Descriptor on a remote image returns the image's own descriptor.

• lib/instances/qemu_test.go: capture the instance ID before registering cleanup. The tests reassign inst from waitForInstanceState, which returns nil on timeout, so one boot timeout panicked the whole package. Surfaced by a CI run on an sfo runner where the QEMU microvm tests time out; unrelated to this change.

Tests: 64 distinct images appended concurrently, one image appended under 16 tags concurrently, a truncated config blob rewritten on the next append, and a reader that never observes an invalid index.json during appends.

The same 64-writer scenario run against layout.AppendImage directly fails every time, either with lost descriptors (22 or 23 of 64) or with invalid character '}' after top-level value, the error seen in production.

One hypeman process owns a cache directory, so an in-process mutex is sufficient. Temp file names (.<name>.tmp-*) never match a blob digest, so the cache GC ignores them. A killed process can leave them behind; they are small and only accumulate on crashes.

Existing malformed indexes are not repaired by this change.

Validation

• Passed: go test -race -count=5 ./lib/ocicache, go test -race ./lib/registry ./lib/ocicachegc, go test ./lib/builds.
• Passed: go vet on lib/ocicache, lib/registry, lib/images, lib/builds, lib/ocicachegc.
• lib/images: the OCI cache import tests exercise the new append path through to a cache hit. Three tests fail in my environment for unrelated reasons (mkfs.erofs not installed, a 60s registry timeout) and fail identically on main.
• Not verified against a live host.

---

Note

Medium Risk
Changes how the shared OCI image cache is written under concurrency; incorrect behavior could break pulls, pushes, or builder imports, but the change targets known index corruption and non-atomic blob writes.

Overview
Introduces lib/ocicache so registry pushes, remote pulls, and builder imports no longer call unsynchronized layout.AppendImage on the shared OCI cache. AppendImage writes layers/config/manifest concurrently, then appends to index.json under a package mutex; WriteBlob is used for registry manifest blobs. All cache files are written via temp file, fsync, and rename, and existing blobs are reused only when on-disk size matches (so truncated writes get fixed instead of trusted forever). Layout initialization no longer overwrites the index on arbitrary FromPath errors.

Call sites in lib/builds, lib/images, and lib/registry switch to these helpers. QEMU integration tests capture instanceID in cleanup closures to avoid racing on a reassigned inst.

^{Reviewed by Cursor Bugbot for commit 1679ab7. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chruffins]

reviewed — approving with findings below. the two bugs are worth fixing, ideally in this PR or a fast follow-up.

Bugs

• lib/ocicache/layout.go:38-41 — the init fallback runs on any layout.FromPath error, not just not-exist. FromPath only stats index.json, so a non-ENOENT stat error (EACCES, EIO) falls into layout.Write(cacheDir, empty.Index), which rewrites index.json as empty and drops every existing descriptor. consider only initializing on fs.ErrNotExist and returning other errors.
• lib/ocicache/layout.go:43 — AppendDescriptor writes index.json in place (WriteFile truncates, then writes). the mutex serializes writers, but an interrupted or failed write (ENOSPC, crash) leaves a truncated index. consider marshaling under the lock, writing a sibling temp file, and os.Rename-ing it over index.json. rename is atomic, so this also closes the unlocked-reader partial-read gap the PR defers.

Structural / Maintainability

• lib/ocicache/layout.go:35-43 — the init branch and the append are two separate index operations under one lock. a single read-modify-write (missing index = empty, append, temp+rename) would drop the FromPath probe and the layout.Write fallback, and would also fix the first bug.

Questions

• lib/ocicache/layout.go:16 — is one process per cache directory an enforced invariant? the mutex only covers in-process callers.
• lib/ocicache/layout_test.go:14 — the PR body says this test fails on main, but it calls ocicache.AppendImage, which doesn't exist on main. what was actually run to show that?

Nits

• lib/ocicache/layout.go:33 — nit: partial.Descriptor can return the image's retained descriptor pointer (remote images do), so setting Annotations mutates the caller's image. not reachable from the three current callers, but copying the struct before annotating avoids it.

coverage — all 5 changed files inspected statically; no generated files in the diff. dimensions checked: behavior parity with the old caller code, boundary inputs, errors/cleanup, concurrency, API compatibility, performance, test adequacy. unverified: runtime interleavings, cross-process behavior, and the test outcome on main. no tests were run during review.
status: review complete; approved at 41790f7

[sjmiller609]

thanks for the review. addressed in 4da048d:

• init fallback: now only initializes on fs.ErrNotExist and returns any other FromPath error. the three original call sites all had the fallthrough; this was the right place to fix it.
• descriptor nit: copying before annotating.
• test on main: fair, the wording was wrong. i ran the same 64-writer scenario against layout.AppendImage directly. it fails 3/3: twice with lost descriptors (22 and 23 of 64), once with invalid character '}' after top-level value, which is the exact error the host logged. PR body updated to say that.
• one process per cache dir: yes. only cmd/api touches the data dir and systemd stops the old daemon before starting the new one. noted in the body.
• temp + rename: deferring to a follow-up on purpose to keep this PR to the writer race. the production error was trailing garbage from overlapping writes, not a truncated file. agree it's the right next change for the unlocked readers, especially with an 850KB index being rewritten in place on every append.

[chruffins]

reviewed — thanks for the follow-up on the init guard, descriptor copy, and test wording. two items left:

• lib/ocicache/layout.go:23 — config and manifest blobs can be corrupted by concurrent writes of the same image. WriteImage sends them through WriteBlob, which writes straight to the final digest path and skips any existing file regardless of size. a second writer can publish a descriptor while the first is still writing, and a blob truncated by a killed writer is never rewritten. concurrent pushes of the same digest hit this (lib/registry/registry.go:153 spawns a conversion per manifest PUT). the 64-writer test uses random images, so it never shares a config blob. consider writing config and manifest via temp+rename in the helper, skipping only on a size match, and adding a case with one image under two tags. the "blob writes run concurrently" comment at layout.go:20-22 should also be narrowed to layers.
• lib/ocicache/layout.go:51 — index.json is still written in place. agreed that temp+rename is the right follow-up for the unlocked-reader gap. the lock does not cover crash or ENOSPC mid-write, though: that leaves an empty index that never self-repairs. either do the rename here (it's a few lines) or note in the PR body that crash and ENOSPC during an append are not covered.

status: review complete; read-only; no tests run

[chruffins]

approving at 4da048d, with the two follow-up items from my last comment:

• lib/ocicache/layout.go:23 — config and manifest blobs go through WriteBlob, which writes to the final digest path and skips any existing file regardless of size. concurrent pushes of the same image can publish a descriptor for a blob still being written. consider temp+rename for config and manifest in the helper, skipping only on size match, and a test with one image under two tags. also narrow the "blob writes run concurrently" comment at layout.go:20-22 to layers.
• lib/ocicache/layout.go:51 — index.json is still written in place. the lock doesn't cover crash or ENOSPC mid-write. either do the rename here or note in the PR body that those cases aren't covered.

status: review complete; read-only; no tests run

[sjmiller609]

both taken in 5767a6e rather than deferred:

• index.json: marshaled under the lock, written to a temp file, renamed into place. covers crash and ENOSPC mid-append and the unlocked readers.
• config and manifest blobs: the helper now writes all blobs itself via temp and rename, skipping only on a size match. added a test with one image under 16 concurrent tags and one that truncates a config blob and checks the next append rewrites it.

temp names are .<name>.tmp-*, which never match the 64-hex blob pattern the GC sweeps. left the comment wording as is since it describes what runs outside the lock.

[chruffins]

approving at 5767a6e. remaining items, all nits:

• nit: lib/ocicache/layout.go:184-187 — writeFile renames without Sync(). rename is safe against process crash, but after power loss or a host reset the renamed index.json or blob can come back empty. consider tmp.Sync() before close, and a directory sync after the rename if you want the same guarantee for the rename itself.
• nit: lib/registry/registry.go:174 — storeManifestBlob still writes the manifest blob with os.WriteFile on the final path, outside the helper. a second PUT of the same digest can truncate the manifest while the conversion from the first PUT (registry.go:153) is reading it. consider routing it through the same atomic writer.
• nit: lib/ocicache/layout.go:27-28 — the doc comment says an interrupted write "leaves nothing behind." a killed process does leave .<name>.tmp-* files, and the GC only sweeps 64-hex names, so they accumulate. either reword the comment or have GC reap stale temp files.

status: review complete; read-only; no tests run

[sjmiller609]

all three in 0c19f67:

• fsync: tmp.Sync() before close in the shared writer. the cache lives on XFS, so this one matters. skipped the directory fsync: losing the rename itself just leaves the previous good file in place.
• registry manifest blob: storeManifestBlob now calls an exported ocicache.WriteBlob, same temp + rename + size-match skip as everything else.
• comment: reworded to "never replaces a good file" and noted in the PR body that a killed process leaves temp files behind.