Repository navigation
fix(store): Lock a pull's staging directories instead of guessing from the pid - #115
Conversation
94cd41c to
9198b54
Compare
|
@claude-fleet review |
|
Review task queued. Task: 6a462074-084e-4d63-9777-9917e6702e61 |
|
@claude-fleet review |
|
Review task |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: one should_fix finding, three nits. The core fix is sound and the tests prove it.
Verified:
- The sweep replaces the blind glob and keeps a live pid's
.tmp-and.old-:pkg/store/staging.go:61-79,192-210, called atpkg/ociclient/client.go:239. StagingInFlightis the same two-part check prune had (live pid and touched withinPullStagingGrace,.old-dated by ctime); prune now calls it rather than its own copy —cmd/hull/prune.go:167, old copy deleted atprune.go:303-429. One implementation, no drift.- The heartbeat reaches the staging directory and nothing a tar carries can:
TouchStagingchtimestmpDiritself (staging.go:84-87), the layers unpack intotmpDir/rootfs(client.go:259). Tree mtime stays as the fallback (staging.go:77). .old-is dated by ctime on both platforms, with a realStat_tfield on each:staging_linux.go:28-34(Ctim),staging_darwin.go:28-34(Ctimespec).- The entry count is written into the staging directory before the publishing rename, so a published image always carries its own count:
client.go:280beforeclient.go:306. - Only a lower count is damage, and an image with no count passes:
integrity.go:133-143. This is what keeps an upgrade from re-pulling every existing store. - The description's claim that
hull runpays for the count only under--pull=always:cmd/hull/run.go:2761-2766returns the cached digest without enteringPullPlatformformissingandnever. integrity.go:38-41's claim that nothing writes into a published rootfs: the generic-container virtiofs share is read-only (run.go:1366,ReadOnly: !containerRootfsDirect), the HVI and 9pfs/virtiofs paths clone the tree into the bundle first (run.go:829,run.go:918-953) and only write into the clone.- The description mentions every package the diff touches.
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok (3.2s, 13.2s)- Reverted
pkg/store/staging.go:200to sweep unconditionally →TestSweepStagingLeavesALivePullAlonefails (removes all four, including both live-pid directories) andTestPullLeavesAConcurrentPullsStagingAlonefails on both the.tmp-and the.old-. - Replaced
damaged := c.store.VerifyImage(digestStr)withvar damaged error→TestPullRepairsATruncatedImagefails: "the second pull reused the truncated image". - Removed
_ = os.RemoveAll(oldDir)atclient.go:298→TestPullCommitsOverItsOwnOldLeftoverfails:failed to displace the previous image: ... file exists. - Working tree restored after each (
git status --porcelainclean).
Smaller things:
UnpackLayers's new parameter has only two call sites outside tests, both updated (client.go:259,unpack_hardlink_test.go:161); no other copy of the blind*.tmp-*/*.old-*glob is left in the tree.- The pull refuses to repair for a stopped holder too, while
rmiallows that under--force. The warning namesrmi --forceas the way out, so the advice is consistent; worth a line indocs/storage.mdthat stopped instances also block the repair — the doc currently says "running or stopped" only in passing.
Not checked:
- The PR's end-to-end repro (two concurrent pulls of claude-code-stock:root and the 20601-entry comparison): no registry access from this workspace, so this rests on the unit tests and the revert checks.
- golangci-lint and the GOOS=darwin build; staging_darwin.go was read but not compiled here (linux host).
- Whether a long-running guest can modify a shared image rootfs on a backend other than the generic-container virtiofs path and so change the entry count after publication.
Task 6a462074-084e-4d63-9777-9917e6702e61 on rp1, model claude-opus-5, effort medium.
| oldDir := filepath.Join(imagesRoot, fmt.Sprintf("%s.old-%d", digestStr, os.Getpid())) | ||
| _ = os.RemoveAll(oldDir) | ||
| swapped := false | ||
| if _, err := os.Stat(finalDir); err == nil { |
There was a problem hiding this comment.
should_fix: The repair path checks ImageHolders before the unpack and never again before the commit, so an instance created during the pull loses its root filesystem.
Failure: hull pull ref finds the digest cached but damaged (client.go:196-211) and ImageHolders returns empty, so it falls through to a full download and unpack — minutes for a real image. While that runs, the user starts hull run on the same digest; the vz generic-container path shares the image's own rootfs directory into the guest (cmd/hull/run.go:837, rootfsDir = resolved, with EvalSymlinks at run.go:816 resolving into images//rootfs). The pull then reaches client.go:300-313, renames images/ to .old-<pid> and os.RemoveAlls it, and the running guest's shared tree is deleted under it.
Evidence: the holders check is at client.go:199-211, the displacing rename at client.go:300-305 and the delete at client.go:313, with nothing in between. hull rmi makes the same check milliseconds before DeleteImage (cmd/hull/rmi.go:134-150); here the window is the whole download.
Fix: re-run c.store.ImageHolders(digestStr) immediately before the swap and skip the displace (keeping the staging directory out of the way, or removing it) when it is non-empty.
| // dates it by its own mtime, which the layers' tars cannot reach, and a | ||
| // pull that takes longer than the grace would otherwise be swept under | ||
| // its live pid by the next pull into the store. | ||
| heartbeat := func() { |
There was a problem hiding this comment.
nit: The heartbeat fires between layers, so a single layer that takes longer than PullStagingGrace still reads as abandoned.
Failure: a 2 GB layer on a slow link takes over an hour inside one unpackLayerWithRetry call. tmpDir's mtime was last set when the previous layer finished, and the tree fallback does not help because the tar restores the mtimes of what it unpacks (the reasoning in pkg/store/staging.go:53-60). A concurrent hull pull then sweeps the directory under the live pid, which is the #114 failure again, just needing a bigger layer.
Evidence: layerDone is called once per layer at pkg/ociclient/unpack.go:56-58; StagingInFlight compares fi.ModTime() against the one-hour grace at pkg/store/staging.go:74.
Fix: drive the heartbeat from the progress reporter (which ticks within a layer) as well as from layerDone, or touch on an interval timer for the life of the pull.
| // ImageComplete says the pieces are there; this says the rootfs is whole. A | ||
| // pull whose staging directory was deleted under it part-way through could | ||
| // finish unpacking the remaining layers, publish the result and exit 0, and | ||
| // the store then held an image with hundreds of entries missing that every |
There was a problem hiding this comment.
nit: The comment (and the matching paragraph in the PR description) claims the count repairs the store #114 left behind, but it only catches entries lost after the count was written.
Failure: in the interleaving where the sweep removes the staging directory and the pull keeps unpacking the remaining layers into recreated ones, WriteRootfsEntries runs afterwards (pkg/ociclient/client.go:280, after the unpack at :259) and records the truncated tree. VerifyImage then finds found == recorded and reports the image whole, so hull pull does not repair it and hull rmi is still the only way out — exactly the case the comment names. What the count does catch is truncation after the record, such as a concurrent RemoveAll that keeps deleting through its open directory fds past the publishing rename, which is the likelier origin of the reported damage.
Evidence: write order at client.go:259-282; the comparison at integrity.go:140.
Fix: narrow the sentence to the damage it actually detects (entries lost after publication), so a later change is not judged against a guarantee that is not there.
| if !ok { | ||
| return nil | ||
| } | ||
| found, err := CountRootfsEntries(imageDir) |
There was a problem hiding this comment.
nit: VerifyImage walks the rootfs with no lock held, so a commit of the same digest in another process can make a whole image read as damaged.
Failure: process A is committing the digest (rename images/ aside, then rename its staging directory in — client.go:300-311) while process B walks the same path. B's filepath.WalkDir sees the root vanish or a partial tree, and either the root error or the undercount produces ErrImageDamaged, costing a full re-download and re-unpack of an image that is fine.
Evidence: the lock is taken only around readRootfsEntries (integrity.go:127-129); the walk at integrity.go:136 runs unlocked, and the comment at integrity.go:123-124 justifies that with "a published rootfs is never written to", which does not cover the directory being renamed out from under the walk.
Fix: treat a walk error on the root as "cannot tell" rather than damage, or take the store lock for the walk the way RemoveStagingDir does.
9198b54 to
b291f31
Compare
|
@claude-fleet review |
|
Review task queued. Task: 06024223-34bd-41e3-8fbc-2e8ed7072946 |
There was a problem hiding this comment.
Approve: the fix is sound and all four earlier findings are addressed in the code, not just in the commit message. Only nits remain.
Verified
- The sweep no longer deletes a live pull's staging.
SweepStagingfilters throughStagingInFlight(pkg/store/staging.go:177-199, :62-79) and removes each survivor under the store lock viaRemoveStagingDir(pkg/store/image.go:188-210). Both staging kinds are covered, and.old-is dated by ctime rather than the displaced image's mtimes. - No other blind glob survives: the only
*.tmp-*/*.old-*producers/consumers left areclient.go:253,image.go:85,121andisStagingDir(store.go:275). The old prune-local copies ofpullInFlight,stagingPID,processAlive,dirUsageare gone, so prune and the pull sweep now read the same function. - Earlier finding "re-check holders before the swap" is implemented:
c.imageHeld(digestStr)at client.go:304-313, gated onImageCompleteso a fresh pull pays nothing. Proven by revert below. - Earlier finding "heartbeat is per-layer" is implemented as a 15-minute ticker over the whole pull (client.go:36-38, 258-275), which covers a single layer outlasting the grace.
- Earlier finding "
VerifyImagewalks unlocked" is implemented: the flock now wraps both the read and the walk (integrity.go:135-152). - Earlier finding "the count does not repair what #114 left behind" is addressed in the prose: integrity.go:113-118 now states the count is taken after the last layer and cannot see a tree that was already short.
- Description claim "
hull runwith the default pull policy does not count": confirmed —resolveImageDigestreturns the cached digest beforePullPlatformunless the policy isalways(cmd/hull/run.go:2761-2768). - Description claim "an image an instance refers to is never displaced, running or stopped":
ImageHoldersfilters on digest only, ignoring status (integrity.go:166-180). CountRootfsEntries'fs.SkipDiron error is safe:WalkDironly reports errors for the root lstat and for aReadDirfailure on a directory, so the "skips the rest of the containing directory" trap for non-directories is not reachable (integrity.go:51-59).
Checks run
go test ./pkg/store/ ./pkg/ociclient/→ both ok../cmd/hull/does not build on linux (undefined: supervisor,composeExec, …); it fails the same way on159772e, so it is unrelated to this PR.- Reverted
StagingInFlightto always return false →TestPullLeavesAConcurrentPullsStagingAlonefails on both the.tmp-and the.old-assertion. - Reverted
found < recordedto never fire →TestPullRepairsATruncatedImagefails with "the second pull reused the truncated image". - Removed
_ = os.RemoveAll(oldDir)fromswapImage→TestPullCommitsOverItsOwnOldLeftoverfails withfailed to displace the previous image: … file exists. - Disabled the commit-time
len(holders) > 0branch →TestPullKeepsAnImageAnInstanceStartedOnDuringThePullfails with "the pull displaced an image an instance started on during the download". - With the
TestPullSkipsUnchangedDigestsentinel moved back insiderootfs/, the test still passes — which is the basis of the first finding below.
Smaller things
cmd/hull/prune_test.go:209still says "a.tmp-is dated by the newest mtime under it"; it is now dated by its own mtime first, with the tree as fallback. The test ages both, so it still proves what it claims.- The pull's warning offers
hull rmi --forceas the way out, butremoveOneImagerefuses a running holder even with--force(cmd/hull/rmi.go:135-139). For a running instance only the "remove them" half of the advice applies. swapImagedoesos.RemoveAll(oldDir)inside the store lock. That leftover is normally absent, but when it is a full image the lock is held for the delete rather than for two renames, contrary to the doc comment at image.go:116-119.
Not checked:
- cmd/hull package tests: the package does not build on linux (undefined supervisor/composeExec/restartPolicy), and fails identically on the base commit 159772e, so the prune_test.go changes were read but not run.
- pkg/store/staging_darwin.go: read, but statusChangeTime cannot be compiled or exercised on this linux host; only the linux twin was built.
- The manual repro in the PR description (raced pulls of claude-code-stock:root, 20601 entries) — no registry or macOS host here.
Findings outside the diff:
- docs/storage.md:192 (nit): The layout tree is the canonical list of what an image directory holds, and the new
rootfs-entriesfile is missing from it.
Failure: a reader auditing an image directory against docs/storage.md:186-192 seesimage.json,oci-config.json,rootfs/and the unpack-schema stamp, finds a fifth file on disk, and has no way to tell it from something a pull left by mistake — exactly the judgement prune and rmi callers make. The permissions sentence at docs/storage.md:208-210 enumerates the 0600 files and omits it too, though integrity.go:80 writes it 0600.
Evidence: the tree at docs/storage.md:186-192; the write at pkg/store/integrity.go:80. The file is described in prose further down (docs/storage.md:459-473), so only the layout block is stale.
Fix: add a(rootfs-entries count)line beside the unpack-schema stamp and name it in the 0600 list.
Task 06024223-34bd-41e3-8fbc-2e8ed7072946 on rp1, model claude-opus-5, effort medium.
| } | ||
|
|
||
| sentinel := filepath.Join(s.RootDir(), "images", first.Digest, "rootfs", ".sentinel") | ||
| // Beside the rootfs, not inside it: an entry added to the rootfs reads as |
There was a problem hiding this comment.
nit: The comment states a rule the code does not have, and the move it justifies drops coverage. VerifyImage flags only found < recorded (pkg/store/integrity.go:149-153), and pkg/store/integrity.go:120-124 plus TestVerifyImageToleratesAnExtraEntry say in as many words that an entry added to the rootfs is not damage.
Failure: I moved the sentinel back to images/<digest>/rootfs/.sentinel and ran go test ./pkg/ociclient/ -run TestPullSkipsUnchangedDigest — it passes, so the count never objected. With the sentinel beside the rootfs the test no longer proves that the skipped pull left rootfs/ itself untouched, which is the thing the test exists to show.
Evidence: the comparison at pkg/store/integrity.go:149-153; the tolerance paragraph at pkg/store/integrity.go:120-124; test run above.
Fix: put the sentinel back under rootfs/ and drop the comment.
|
|
||
| // The tar of a finished layer restores the mtimes of everything it unpacked, | ||
| // so an hour into a slow pull nothing under rootfs/ is fresh. The pull marks | ||
| // the staging directory itself after every layer, and that alone must keep |
There was a problem hiding this comment.
nit: The comment describes the per-layer heartbeat that the review round replaced with a timer.
Failure: a reader takes "The pull marks the staging directory itself after every layer" as the contract this test guards, and a later change that drops the ticker in favour of a per-layer call reads as a no-op against it — while reintroducing the single-slow-layer hole the ticker exists to close.
Evidence: the comment at pkg/store/staging_test.go:143-147; the implementation is a time.NewTicker(stagingHeartbeat) over the whole pull at pkg/ociclient/client.go:258-275, with stagingHeartbeat = store.PullStagingGrace / 4 at client.go:38.
| if !ok { | ||
| return nil | ||
| } | ||
| found, err := CountRootfsEntries(imageDir) |
There was a problem hiding this comment.
nit: VerifyImage holds the exclusive store flock for the whole rootfs walk, which is on the --pull=always hot path.
Failure: hull run --pull=always on a cached image walks every entry under rootfs/ (20601 for the image in the PR description) with .lock held. For that walk, every other hull process blocks in CreateInstance, SaveInstance and DeleteInstanceRecord, which all take the same flock — so a hull ps refresh or an instance state transition elsewhere stalls on a cold or network-backed store.
Evidence: the lock is taken at pkg/store/integrity.go:135 and released by the deferred unlock at the end of VerifyImage, with the walk at :149; the competing takers are pkg/store/store.go:414, :442 and :521.
Fix: if the lock is only there to exclude a concurrent CommitImage swap (the reason given at integrity.go:124-128), walking first and re-reading the count under the lock, or re-walking once on a mismatch, would keep the exclusion without holding it for the tree.
b291f31 to
0598fa9
Compare
|
@claude-fleet review |
|
Review task queued. Task: 1cf77509-01d7-4d0e-bf2e-f2cb2b60f22f |
ananos
changed the title
There was a problem hiding this comment.
Request changes: one should_fix finding. The lock design itself holds up — both revert checks confirm the tests are tied to their fixes, and the claims the comments make about code outside the diff check out.
Verified:
flockfollows the inode through rename and dies with the process, which is whatCreateStaging/swapImagerely on —pkg/store/staging.go:60-77,pkg/store/image.go:152-159; exercised byTestCreateStagingKeepsTheNameItLocked(staging_test.go:166-186).- "Nothing writes into a published rootfs:
hull runshares it read-only or clones it into the instance" (pkg/store/integrity.go:39-41) is true: the only writable share is the HVI path, and that one shares the instance bundle's APFS clone, not the image —cmd/hull/run.go:823-836setscontainerRootfsDirectonly there, andrun.go:1366passesReadOnly: !containerRootfsDirect. The vz path atrun.go:836sharesresolvedread-only. - "
hull runwith the default pull policy does not count;--pull=alwaysgoes through the pull path and does" —cmd/hull/run.go:2761-2769: anything butalwaysreturnscachedDigestwithout touchingPullPlatform, andVerifyImageis reached only frompkg/ociclient/client.go:197. - The previous round's
should_fix(holders re-checked before the swap) is in and works for the repair case:client.go:291-298runsimageHeldafter the unpack, andimageHeldreachesImageHoldersbecause the damaged image is stillImageComplete(client.go:324-327,pkg/store/store.go:286-302). CountRootfsEntries'fs.SkipDiris only ever returned for a directory whoseReadDirfailed, whereWalkDirturns it into "skip the children" rather than "skip the rest of the parent" —pkg/store/integrity.go:52-63, matching the comment andTestCountRootfsEntriesSkipsAnUnlistableDirectory.- Prune's
ErrStagingInUsepath is not counted as a failure and not counted as reclaimed —cmd/hull/prune.go:131-137.
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok (3.1s, 13.2s).- Reverted the lock check in
RemoveStagingDir(dropped thelockDirerror return atpkg/store/image.go:219-222) →TestPullLeavesAConcurrentPullsStagingAlonefails on both assertions:the pull swept a concurrent pull's ...tmp-1and...old-1. Restored. - Disabled the count comparison at
pkg/store/integrity.go:153→TestPullRepairsATruncatedImagefails:the second pull reused the truncated image. Restored;git status --porcelainclean apart from the untrackednode_modules. GOOS=darwin go build ./...→ clean.
Smaller things:
- Two earlier threads look unaddressed rather than resolved, so I am not refiling them: the comment at
pkg/ociclient/client_test.go:173-174still states that an entry added to the rootfs reads as a count mismatch, whichpkg/store/integrity.go:153andTestVerifyImageToleratesAnExtraEntrycontradict; andVerifyImagestill holds the exclusive store lock across the whole walk (pkg/store/integrity.go:135-149), now on the--pull=alwayspath. VerifyImageis the onlylockFilecaller that does not takes.mufirst (pkg/store/integrity.go:135vsimage.go:141-147,image.go:210-216), and the only image-path method that skipsValidateImageDigest. Harmless for the current callers, both digests coming from the registry, but it breaks the file's convention.
Not checked:
cmd/hulldoes not build on this host (supervisor.go,compose.goand friends are darwin-only:undefined: supervisor,undefined: composeExec), so the prune tests ran nowhere — I read them only.- The race detector is unavailable here (
ThreadSanitizer: unsupported VMA range), so the finding below is from reading the test, not from a-racerun.
Not checked:
- cmd/hull tests: the package does not build on linux (darwin-only supervisor.go/compose.go), so prune_test.go was read but not run
- go test -race: the race detector cannot start on this host (ThreadSanitizer: unsupported VMA range), so the client_test.go data-race finding is from reading the test only
- The PR's manual verification on macOS: the flock-on-a-directory-descriptor behaviour, the 20601-entry raced pull, and the golangci-lint v2.12.2 run
Task 1cf77509-01d7-4d0e-bf2e-f2cb2b60f22f on rp2, model claude-opus-5, effort medium.
| return err | ||
| } | ||
| if previous != nil { | ||
| _ = os.RemoveAll(oldDir) |
There was a problem hiding this comment.
should_fix: The pull's staging lock follows the inode into the published image and is only released after the displaced copy has been deleted, so a concurrent CommitImage of the same digest fails instead of waiting.
Failure: two hull pulls of the same digest into a store that already holds it (a re-unpack after an UnpackSchema bump, or the repair path), on two hosts' worth of parallel CI jobs. Pull A commits: swapImage renames tmp-<pidA> to images/<digest> and releases the store lock, and A's staging fd — opened on the staging directory at pkg/ociclient/client.go:252 — now locks the published image's inode. A then spends os.RemoveAll(oldDir) here deleting the previous rootfs, seconds on a 20k-entry tree, and only closes the lock afterwards at pkg/ociclient/client.go:306. If B reaches its own commit in that window, lockDir(finalDir, false) at pkg/store/image.go:152 gets EWOULDBLOCK and B's pull exits non-zero with "failed to displace the previous image: a pull is using the staging directory" — a message about a staging directory for what is a published image. Nothing is corrupted; the pull just fails, which is the outcome this PR is closing.
Evidence: the lock is taken in CreateStaging (pkg/store/staging.go:100-118) and closed at pkg/ociclient/client.go:306, after CommitImage returns; CommitImage does the slow os.RemoveAll(oldDir) at pkg/store/image.go:131 before returning; the non-blocking acquire that B fails on is pkg/store/image.go:152. A narrower version of the same window (microseconds) exists even when there is no previous image to delete, between the store unlock in swapImage and client.go:306.
Fix: release the staging lock inside the locked section, right after the os.Rename(stagingDir, finalDir) at pkg/store/image.go:161 succeeds — pass it into CommitImage — so the published image is never flock-held.
| if err != nil { | ||
| t.Fatalf("first Pull: %v", err) | ||
| } | ||
| digest = first.Digest |
There was a problem hiding this comment.
nit: digest is written by the test goroutine and read by the httptest handler goroutine with no synchronization, while the neighbouring started flag is an atomic.Bool.
Failure: go test -race ./pkg/ociclient/ can report a data race between the write at client_test.go:507 and the read at client_test.go:492, which the handler performs on the server's own goroutine during the first pull's layer fetch. A loopback socket round-trip is not a happens-before edge the race detector models, so this is a CI-only flake with no bearing on the production code.
Evidence: the hook closure reads digest at client_test.go:492 and is invoked from the http.HandlerFunc at client_test.go:101-107; the only write is at client_test.go:507, after the first c.Pull returns but with the keep-alive handler goroutine still live.
Fix: make it an atomic.Pointer[string] or a sync.Mutex-guarded field, as started already is.
| @@ -309,42 +339,6 @@ func TestPruneEmptyStore(t *testing.T) { | |||
|
|
|||
| // The figure printed is the space the host gets back, so it counts blocks and | |||
There was a problem hiding this comment.
nit: The comment that documented TestDirDiskUsageCountsAHardLinkOnce was left behind when the test moved to pkg/store, and now sits on top of TestPruneKeepsTheLastCompleteEntryOfATag.
Failure: prune_test.go:340-345 reads as one comment block saying both "the figure printed is the space the host gets back, so it counts blocks" and "the blocking case from review" about a test that measures neither. A reader looking for what guards the block accounting finds it here and not in pkg/store/staging_test.go:55, where the test actually lives now.
Evidence: the two sentences at cmd/hull/prune_test.go:340-341 and the test they described, now pkg/store/staging_test.go:55-72.
Fix: delete prune_test.go:340-341.
0598fa9 to
765c2d1
Compare
|
@claude-fleet review |
|
Review task queued. Task: c9f8374e-e2e2-4d40-bdd9-efe780eb8c23 |
There was a problem hiding this comment.
Request changes: one should_fix, one nit. The lock protocol itself holds up — I traced the mkdir/lock/sweep interleavings and could not break it.
Verified:
- The sweep a pull runs now skips a locked directory and deletes under the lock:
SweepStaging→RemoveStagingDirtakes the directory flock non-blocking and holds it throughos.RemoveAll— pkg/store/image.go:226-235, pkg/store/staging.go:160-181. CreateStagingcannot be left writing into a swept path: a sweeper mid-RemoveAllholds the inode lock, soMkdirAlleither adopts the doomed inode and then blocks on the lock (and failssameInodeon retry) or creates a fresh one after the final unlink — pkg/store/staging.go:95-118.- No lock-order inversion: every blocking wait is on one lock only.
CreateStagingwaits for a directory lock while holding no store lock;swapImageandRemoveStagingDirtake the store flock first and the directory lock non-blocking — pkg/store/image.go:144-170, :219-235, pkg/store/store.go:405-418. - The staging lock is released inside the locked section, right after the publishing rename, so the published image is never flock-held while the displaced copy is deleted — pkg/store/image.go:161-168, with
TestCommitImageSwapsAndRollsBackasserting the published directory is lockable (pkg/store/image_test.go:367-372). This closes the earlier image.go:139 finding. - The holders check is repeated immediately before the swap, closing the window the download opened — pkg/ociclient/client.go:284-301;
TestPullKeepsAnImageAnInstanceStartedOnDuringThePulldrives it from a blob hook. This closes the earlier client.go holders finding. VerifyImageno longer holds the store lock for the hot-path walk, and confirms a shortfall under the lock by re-reading and re-walking, so a concurrentCommitImageswap cannot read as damage — pkg/store/integrity.go:140-160.- Description claim "
hull runwith the default pull policy does not count;--pull=alwaysdoes": confirmed —resolveImageDigestreturnscachedDigestwithout callingPullPlatformunless the policy isalways— cmd/hull/run.go:2762-2771. - Description claim that the pid in the name now only separates two pulls' directories: no reader of it is left;
processAlive,stagingPID,PullStagingGraceand the heartbeat are gone from the tree (grep). - The earlier nits on the
digestdata race (nowatomic.Pointer[string], client_test.go:472), the stale hard-link comment in prune_test.go, the heartbeat comment in staging_test.go and the overstated repair claim in integrity.go are all addressed.
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok (3.1s, 13.2s).- Reverted the directory lock in
RemoveStagingDir(pkg/store/image.go:228-232) →TestSweepStagingLeavesALockedDirectoryAlonefails ("the sweep took a live pull's ...tmp-1"),TestRemoveStagingDirRefusesALockedDirectoryandTestCreateStagingIsHeldUntilClosedfail, andTestPullLeavesAConcurrentPullsStagingAlonefails on both the.tmp-and the.old-directory. Restored. - Replaced
damaged := c.store.VerifyImage(digestStr)withvar damaged error(client.go:197) →TestPullRepairsATruncatedImagefails ("the second pull reused the truncated image"). Restored;git status --porcelainclean apart from untrackednode_modules. go vet ./pkg/store/ ./pkg/ociclient/under GOOS=linux → no output.
Smaller things:
- The store layout tree in docs/storage.md:184 lists
(unpack-schema stamp)but notrootfs-entries, which the prose two sections later introduces as a sibling of it. swapImage's openingos.RemoveAll(oldDir)(pkg/store/image.go:154) is the one deletion underimages/that does not consult the directory lock. The argument that only this process can own that name holds on one host; it stops holding for a store shared between two pid namespaces, whereos.Getpid()collides.
Not checked:
cmd/hulldoes not build under GOOS=linux on this host (cmd/hull/compose.go:277:23: undefined: oneshotInitCommand, a darwin-only symbol), so the prune tests in cmd/hull/prune_test.go were not run; I read them instead.go test -raceaborts on this host before running any test (FATAL: Found 47 - Supported 48, a kernel VA-bits mismatch), so the race detector was not exercised.- The manual repro in the description (two concurrent pulls of
claude-code-stock:root, 20601 entries) and the macOSflock-on-a-directory behaviour.
Not checked:
- cmd/hull/prune_test.go and cmd/hull/prune.go behaviour at runtime: cmd/hull does not build under GOOS=linux on this host (darwin-only symbols), so those tests were read, not run
- go test -race: the race detector aborts on this host before running any test (kernel VA-bits mismatch)
- The manual two-pull repro and the macOS flock-on-a-directory observations from the description
- docs/storage.md prose beyond the changed hunks
Task c9f8374e-e2e2-4d40-bdd9-efe780eb8c23 on rp2, model claude-opus-5, effort medium.
| dir := filepath.Join(s.rootDir, "images", name) | ||
| l, err := lockDir(dir, false) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
should_fix: RemoveStagingDir now fails with ENOENT when the directory is gone by the time it runs, where it used to succeed, and hull prune turns that into a failed run.
Failure: hull prune lists <digest>.tmp-<pid> as a leftover, then a concurrent hull pull sweeps it (the pull sweep is new in this PR, so this is now a routine interleaving). Prune reaches RemoveStagingDir, lockDir calls os.Open on the missing path, and the raw *PathError is returned. removePruneTargets matches only store.ErrStagingInUse (cmd/hull/prune.go:133), so the error joins failures and hull prune exits non-zero with open .../images/<digest>.tmp-<pid>: no such file or directory although nothing is wrong.
Evidence: pkg/store/image.go:228-230 returns the open error unchanged; before this PR the body was os.RemoveAll, which is nil for a missing path. SweepStaging handles exactly this case explicitly (errors.Is(err, fs.ErrNotExist), pkg/store/staging.go:176), so the pull path tolerates it and prune does not.
Fix: return nil from RemoveStagingDir when the directory is already gone, or add fs.ErrNotExist alongside ErrStagingInUse in removePruneTargets.
| } | ||
| if swapped { | ||
| _ = os.RemoveAll(oldDir) | ||
| if len(holders) > 0 { |
There was a problem hiding this comment.
nit: When the pre-commit holders check fires, the pull returns success without refreshing the image metadata, so the resolution it just performed is not recorded anywhere.
Failure: the image is complete but damaged and unheld at client.go:196-211, so the pull downloads; an instance starts on the digest during the download; imageHeld is non-empty at client.go:291-297, the commit is skipped and the staging directory is discarded with its image.json. PullPlatform then returns a PullResult at client.go:306-311 without calling SaveImage, so Ref, PulledAt and any new tag that resolved to this digest are never written. The equivalent skip a few lines above does call SaveImage (client.go:217-219) for precisely this reason. A later hull run <newref> finds no cached entry for that reference and pulls again.
Evidence: the SaveImage in the early skip at client.go:217-219; its absence on the late skip at client.go:295-301.
Fix: call c.store.SaveImage(digestStr, metadata) on the holders branch too, since the image on disk is the one the instance holds and the metadata only records the resolution.
765c2d1 to
97f99f3
Compare
|
@claude-fleet review |
|
Review task queued. Task: f2a3c879-7071-4528-a76e-fa593179806d |
There was a problem hiding this comment.
Approve. The sweep race in #114 is closed the right way: a lock on the inode is evidence, the pid-plus-mtime heuristic it replaces was not. Both revert checks pass, and every finding from the earlier rounds is addressed in the code (not just the commit message) — the staging lock is now released inside swapImage at pkg/store/image.go:161, removeLockedDir returns nil for a directory already gone, the commit-time holders branch calls SaveImage, the VerifyImage walk runs unlocked with a confirm pass, the sentinel is back under rootfs/, and digest in the mid-pull test is an atomic.Pointer. Two nits below, neither holding anything up.
Verified:
- The pull sweep only takes unlocked directories, and
CreateStaginghands the pull a locked one — pkg/store/staging.go:100-118, pkg/ociclient/client.go:240-256. - The lock follows the inode through the publishing rename and is dropped there, so a concurrent commit of the same digest finds the image free — pkg/store/image.go:152-162, asserted at pkg/store/image_test.go:368-373.
VerifyImageis reached only from the pull path, sohull runwith the default policy pays nothing for the walk — the sole caller is pkg/ociclient/client.go:197;cmd/hull/run.go:2718usesImageCompleteonly. The description's claim holds.- An extra entry is not damage and an unrecorded image passes — pkg/store/integrity.go:180, pkg/store/integrity_test.go:77-89 and :155-164.
- A damaged image an instance refers to is served as is, both before the download and again at commit time — pkg/ociclient/client.go:203-210 and :291-302.
- Prune's removal re-takes the lock rather than trusting its own listing — cmd/hull/prune.go:128-138, pkg/store/image.go:236.
- The docs layout tree lists
rootfs-entriesand the prose matches the implementation — docs/storage.md:184, :452-465.
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok (3.1s, 13.3s).- Reverted pkg/ociclient/client.go:240-246 to the original
*.tmp-*/*.old-*glob sweep →TestPullLeavesAConcurrentPullsStagingAlonefails on both assertions (the pull swept a concurrent pull's ….tmp-1and….old-1). Restored. - Replaced
damaged := c.store.VerifyImage(digestStr)with a nil error →TestPullRepairsATruncatedImagefails:the second pull reused the truncated image. Restored;git status --porcelainclean apart from untrackednode_modules.
Smaller things:
CreateStagingmakes the staging directory 0700, whereUnpackLayers'MkdirAll(rootfsDir, 0755)(pkg/ociclient/unpack.go:44) used to create it 0755. Published image directories therefore go from 0755 to 0700. That matches the rest of the store (pkg/store/store.go:220, :242 are all 0700) and the store root is 0700 anyway, so nothing can reach it that could not before — worth knowing, not worth changing.SweepStagingtakes and releases the store flock once per leftover (pkg/store/staging.go:184 into pkg/store/image.go:231). Fine for the handful a store accumulates.
Not checked:
cmd/hulldoes not build on this host (undefined: supervisor,composeExec, and others in darwin-only files), so none of the prune tests ran here. I read the prune diff and its tests but cannot report a pass.-raceis unavailable on this host:FATAL: ThreadSanitizer: unsupported VMA range. Theatomic.Pointer[string]change reads correct, but I did not run the detector over it.- The macOS
flock-on-a-directory behaviour and the manualclaude-code-stock:rootrepro in the description — both are claims about a host I do not have.
Not checked:
- cmd/hull tests: the package does not build on linux/arm64 here (undefined: supervisor, composeExec, restartPolicy in darwin-only files), so no prune test was executed. The prune diff and its tests were read.
- go test -race: unavailable on this host (ThreadSanitizer: unsupported VMA range), so the atomic.Pointer fix in client_test.go was reviewed but not exercised under the detector.
- The description's macOS flock-on-a-directory observations and the manual claude-code-stock:root repro, neither reproducible on this host.
- pkg/ociclient/unpack.go beyond the MkdirAll permission at line 44; it is not in the diff.
Findings outside the diff:
- pkg/store/image.go:88 (nit):
DeleteImagedoes not clear a leftover.old-<pid>of its own pid before renaming onto that name, whileCommitImage— added in this PR for exactly that reason — does.
Failure: a commit's best-effort os.RemoveAll(oldDir) at pkg/store/image.go:131 fails (a held fd, a permission change, a full unlink), leaving images/<digest>.old-<pid> populated. A later DeleteImage of the same digest in the same process computes the same aside name at image.go:87, and os.Rename onto a non-empty directory returns ENOTEMPTY, so the removal fails with "failed to displace image".
This is narrow: hull runs one command per process, so it needs a pull and a delete of the same digest in one process, and the next pull's SweepStaging clears the leftover anyway. I could not construct a reachable caller, so this is a consistency note rather than a demonstrated bug.
Evidence: the guard CommitImage has at pkg/store/image.go:148-150 (removeLockedDir(oldDir), with the reason spelled out at image.go:106-110); the same rename with no guard at image.go:87-90.
Fix: call removeLockedDir(aside) before the rename, the way swapImage does.
Task f2a3c879-7071-4528-a76e-fa593179806d on rp1, model claude-opus-5, effort medium.
| } | ||
| imageDir := s.ImageDir(digest) | ||
| short, err := s.rootfsShort(imageDir) | ||
| if err != nil || short == nil { |
There was a problem hiding this comment.
nit: The confirm-under-the-lock pass is skipped when the first walk errors, so the one case the lock exists for is not covered on that path.
Failure: hull pull --pull=always ref passes ImageComplete at pkg/ociclient/client.go:196, then enters rootfsShort. readRootfsEntries succeeds, and between it and CountRootfsEntries another process's CommitImage renames images/<digest> aside (pkg/store/image.go:155) or hull rmi does the same at pkg/store/image.go:88 and spends seconds in RemoveAll with the directory gone. filepath.WalkDir's first lstat on <imageDir>/rootfs fails, path == root so the error is returned (pkg/store/integrity.go:52-55), and rootfsShort returns it in the second slot wrapped in ErrImageDamaged (integrity.go:178). VerifyImage then hits if err != nil || short == nil { return err } and returns damage without ever taking the store lock or walking again. The pull falls through to a full re-download and re-unpack.
The undercount path is handled: a walk that merely sees fewer entries returns via short, gets confirmed under the lock, and comes back clean. Only the root-lstat error escapes, so the window is narrow (an rmi widens it to the length of its RemoveAll) and the direction is safe — a wasted download, never corruption. But the doc comment at integrity.go:132-139 and the PR description both state the stronger guarantee that "a concurrent commit cannot read as damage", and a later change will be judged against that.
Evidence: integrity.go:145-148 (early return on err), integrity.go:168-179 (rootfsShort returns walk failures as err, not short), integrity.go:52-55 (root errors propagate).
Fix: treat a walk error on the root as "cannot tell" and fall through to the locked re-check, rather than returning it straight out.
3ca9214 to
3eac5cb
Compare
|
@claude-fleet review |
|
Review task queued. Task: b951e381-5973-45d0-8921-2ab38f7f25e0 |
There was a problem hiding this comment.
Request changes: one should_fix finding; the rest are nits.
The lock design holds up. CreateStaging → unpack → CommitImage is a clean chain: the lock is taken on the inode, re-checked against the name, follows the rename, and is released inside the locked section so a published image is never flock-held. Lock ordering is consistent everywhere (s.mu → store flock → directory flock, and the directory flock is non-blocking at every point where the store flock is already held), so the new locks cannot deadlock against the store lock.
All twelve findings from the earlier rounds are addressed in the code I read: RemoveStagingDir tolerates a missing directory (image.go:245-247), the commit-time holders check and its SaveImage are at client.go:291-302, the staging lock is released at image.go:182, VerifyImage walks unlocked and confirms both a shortfall and a walk error under the lock (integrity.go:141-153), the TestPullSkipsUnchangedDigest sentinel is back under rootfs/ (client_test.go:174), digest is an atomic.Pointer[string] (client_test.go:500), and the stray dirDiskUsage comment is gone from prune_test.go.
Verified:
- Nothing writes into a published rootfs, so the recorded count is stable. The vz generic-container path shares
images/<digest>/rootfsread-only (run.go:837,run.go:1366withReadOnly: !containerRootfsDirectandcontainerRootfsDirectfalse on that branch); HVI clones it (run.go:829-835); the unikernel virtiofs/9pfs path clones into the bundle beforeinjectHosts(run.go:918-955); block mode only reads (run.go:910). ThewriteIntoRootfscalls atrun.go:1114andrun.go:1211are on the cloned bundle rootfs, not the image. hull runwith the default policy never reads the count:resolveImageDigestshort-circuits oncachedDigestunless--pull=always(run.go:2761-2770). Matches the docs claim.CountRootfsEntries'sfs.SkipDiron a non-root error is safe:filepath.WalkDironly reports errors for a failed root lstat or a failedReadDirof a directory it has already called back for, andSkipDiron a directory is swallowed.WriteRootfsEntriesis called afterstoreImageConfig/WriteImageMetadata/WriteUnpackSchema, all of which write besiderootfs/, so the count it records is the one a later walk sees (client.go:264-283).- No other blind staging sweep remains:
grepfor.tmp-/.old-outside tests turns up only the name construction inclient.go:251,image.go:87,image.go:133andisStagingDirinstore.go:275.
Checks run:
go test ./pkg/store/→ ok (3.1s)go test ./pkg/ociclient/→ ok (13.2s)- Reverted
SweepStaging'ss.RemoveStagingDir(name)to a plainos.RemoveAll→TestPullLeavesAConcurrentPullsStagingAloneFAILs on both the.tmp-1and.old-1assertions. Restored. - Replaced
damaged := c.store.VerifyImage(digestStr)with a nil error →TestPullRepairsATruncatedImageFAILs on both assertions ("the second pull reused the truncated image", "rootfs holds 0 entries, 1 were recorded"). Restored;git status --porcelainclean. GOOS=darwin GOARCH=arm64 go vet ./cmd/hull/ ./pkg/store/ ./pkg/ociclient/→ no output.go vetfor linux on the two library packages → no output.
Smaller things:
CreateStaging's blockinglockDirhas no bound. On one host the only competing holder is a sweeper, as the comment says; across pid namespaces sharing a store, two pulls of the same digest with the same pid serialize for the length of a whole unpack. Acceptable, and better than the corruption it replaces, but the comment's "that is over in a moment" is only true for the single-host case.hull prunecould not be exercised here:cmd/hulldoes not build for linux (undefined: supervisor,composeExec— pre-existing, unrelated to this PR). Its tests type-check underGOOS=darwin.
Not checked: the race detector is unavailable on this host (ThreadSanitizer: unsupported VMA range), so the atomic.Pointer fix is confirmed by reading only; cmd/hull tests were not executed; the macOS flock-across-rename behaviour reported in the description.
Not checked:
- cmd/hull tests (including the prune tests) could not run: the package does not build for linux on this host for reasons unrelated to this PR (undefined: supervisor, composeExec, restartPolicy). It type-checks under GOOS=darwin.
- go test -race: the race detector aborts on this host (ThreadSanitizer: unsupported VMA range), so the client_test.go atomic.Pointer fix for the earlier data-race nit was confirmed by reading only.
- The macOS flock behaviour asserted in the description (EWOULDBLOCK on a second descriptor, lock surviving a directory rename, release on close) was not reproduced; only linux/arm64 was exercised.
- docs/storage.md prose beyond the two claims checked against run.go and resolveImageDigest.
Task b951e381-5973-45d0-8921-2ab38f7f25e0 on rp1, model claude-opus-5, effort medium.
| // pull has to respect: an instance cannot be running on an incomplete one, | ||
| // and a stopped record naming one is waiting for exactly this pull. | ||
| func (c *Client) imageHeld(digest string) ([]string, error) { | ||
| if !c.store.ImageComplete(digest) { |
There was a problem hiding this comment.
should_fix: imageHeld returns no holders whenever ImageComplete is false, so the new guard does not protect a running instance when the cached directory is incomplete rather than merely short.
Failure: an instance is running on images/<digest> under the vz generic-container path, which shares that very directory into the guest (run.go:837 sets rootfsDir = resolved, bundle.go:71 points the bundle symlink at images/<digest>/rootfs, run.go:1366 exports it over virtiofs). hull is then upgraded with a bumped UnpackSchema, so ImageComplete returns false purely on the schema check (store.go:301). hull pull ref or hull run --pull=always skips the early return, downloads, calls imageHeld at client.go:291, gets nil because of the gate at client.go:326, and CommitImage renames images/<digest> aside and os.RemoveAlls it (image.go:140) under the live guest. The same holds if the post-publication damage this PR is about happens to take image.json or unpack-schema — both sit directly under images/<digest>/, and a RemoveAll deletes in readdir order — since the entry-count check only fires for an image that is complete and short.
Evidence: the gate at client.go:325-328; ImageComplete's schema check at pkg/store/store.go:291-301; the displace and delete at pkg/store/image.go:165-174 and :140. hull rmi makes no such gate — removeOneImage refuses on running holders regardless of completeness (cmd/hull/rmi.go:134-141) — so the pull is weaker than the command the comment says it mirrors.
The justification in the doc comment ("an instance cannot be running on an incomplete one") is what does not hold: a schema-stale image is incomplete to the cache and perfectly live to a guest already booted on it.
Fix: drop the ImageComplete gate and refuse to displace when any holder is running, keeping the gate only for the stopped-holder case the comment is really about (a stopped record waiting for exactly this pull). InstanceState.Status is already what runningHolders reads in rmi.
| // The leftover's lock is consulted like any other staging directory's. | ||
| // On one host only this process can own the name; a store shared | ||
| // between two pid namespaces can have another's pull behind it. | ||
| if err := removeLockedDir(oldDir); err != nil { |
There was a problem hiding this comment.
nit: swapImage does a full os.RemoveAll of a leftover .old-<pid> inside the store lock, which breaks the invariant stated six lines above it.
Failure: an earlier commit of the same digest in this process left a complete previous image behind because its best-effort os.RemoveAll(oldDir) at image.go:140 did not finish. The next commit calls removeLockedDir(oldDir) at image.go:161, which is os.RemoveAll on a 20k-entry tree (image.go:251) — seconds — with s.mu and the store flock both held. Every other process's CreateInstance, SaveInstance and DeleteInstanceRecord blocks for that whole time, which is exactly what the comment promises does not happen.
Evidence: the doc comment at pkg/store/image.go:118-119 ("The delete of the displaced image happens outside it, so the lock is held for two renames and not for seconds"); the leftover clear at image.go:161 inside the defer unlock() scope opened at image.go:152-156.
Either clear the leftover before taking the store lock (it is this pid's own and nothing else races it on one host), or narrow the comment to say the leftover clear is the exception.
| failures = append(failures, rmErr) | ||
| continue | ||
| } | ||
| _, _ = fmt.Fprintf(w, "Removed %s (%s, %s)\n", t.describe(), t.reason, formatSize(t.bytes)) |
There was a problem hiding this comment.
nit: hull prune reports a staging directory as Removed and counts its bytes as reclaimed when another sweeper actually freed it.
Failure: prune lists <digest>.tmp-<pid> with bytes from DirDiskUsage (prune.go:191-196), then a concurrent hull pull's sweep removes it. Prune's RemoveStagingDir reaches removeLockedDir, which now returns nil for a missing directory (pkg/store/image.go:245-247), so prune falls through to prune.go:143 and prints Removed <name> (leftover from an interrupted pull, 1.2 GB) and adds 1.2 GB to the Reclaimed total at prune.go:106 — a figure the run did not free. The same happens against a concurrent hull rmi, whose DeleteImage holds the store lock through its own RemoveAll (pkg/store/image.go:87-93) while prune's StagingInFlight check at prune.go:179 takes no store lock and sees the unlocked .old-<pid> as prunable.
Evidence: the nil-for-missing return at pkg/store/image.go:245-247; the unconditional Removed/reclaimed += at cmd/hull/prune.go:143-144.
This is the accepted cost of the earlier ENOENT fix, not a reason to undo it — but distinguishing "removed" from "already gone" would keep the printed total honest.
There was a problem hiding this comment.
The count is right now, but the rmi case in the second half of this comment still shows. hull rmi golang:1.23-alpine (15,713 entries), with hull prune started as soon as the .old- directory appeared:
| 7ddf2c8 | 159772e | |
|---|---|---|
hull prune |
Already gone: sha256:56e4….old-70355 (previous image left behind by an interrupted pull), Reclaimed 0.0 B |
Nothing to prune |
hull prune --dry-run |
Would remove sha256:56e4….old-70405 (previous image left behind by an interrupted pull, 258.6 MB), Would reclaim 258.6 MB |
Nothing to prune |
Nothing is lost, but both lines report an rmi in progress as an interrupted pull, and prune waits on the store lock for the length of the rmi's delete. The pid check this PR replaces skipped the directory because the rmi's pid was alive. Locking the aside directory in DeleteImage before the rename, as swapImage does for the displaced image, would let StagingInFlight see it. That would pair with the removeLockedDir(aside) suggested for the same rename in the latest review, and would let DeleteImage delete outside the store lock the way RemoveStagingDir now does.
| return nil, err | ||
| } | ||
| defer func() { _ = staging.Close() }() | ||
| defer func() { _ = os.RemoveAll(tmpDir) }() |
There was a problem hiding this comment.
nit: Releasing the staging lock inside swapImage leaves PullPlatform's deferred os.RemoveAll(tmpDir) unguarded, so a second pull of the same digest in the same process can have its freshly created staging directory deleted.
Failure: goroutine A and goroutine B both call PullPlatform for digest D. They compute the same tmpDir name (client.go:251 — same pid), so B blocks in CreateStaging's waiting lockDir (pkg/store/staging.go:100-110). A commits: swapImage renames tmpDir to images/D and closes the staging lock at pkg/store/image.go:182. B wakes, finds sameInode false, loops, MkdirAlls a fresh tmpDir and locks it — and then A's deferred os.RemoveAll(tmpDir) at client.go:257 fires and removes B's new directory. B's lock is now on an orphaned inode, UnpackLayers recreates the path, and nothing holds the name against a sweeper: #114 again, in-process. On the failure path the ordering is safe, because the deferred RemoveAll runs before the deferred Close and B is still blocked.
Evidence: the early release at pkg/store/image.go:182 versus the deferred delete at pkg/ociclient/client.go:256-257.
Not reachable from the CLI today — PullPlatform has exactly two call sites (cmd/hull/pull.go:61, cmd/hull/run.go:2772), both one pull per process, and compose runs no pulls concurrently. Worth closing anyway, since the hazard is in the exported package: skip the deferred RemoveAll once the commit has succeeded.
3eac5cb to
130e06c
Compare
|
@claude-fleet review |
|
Review task queued. Task: 135d9cc6-e4a3-42fb-8aa5-3f266bc36aac |
There was a problem hiding this comment.
Request changes: one should_fix, one nit. The core of the change — locks instead of pid-plus-mtime guesses — is sound, and both of its halves fail without the fix (checks below).
Findings:
- should_fix, pkg/store/integrity.go:218 —
ImageHolder.Liveis status-only, so a stalerunningrecord makes the repair and re-unpack paths unreachable - nit, pkg/store/image.go:253 — the pull's pre-unpack sweep now holds the exclusive store lock for the whole
RemoveAllof a leftover
Verified:
- "a pull holds a lock on its staging directory and on the displaced image, and a sweeper asks the lock":
CreateStagingreturns the directory locked with the inode re-checked against the name (pkg/store/staging.go:100-118),swapImagelocks the previous image before the rename aside and keeps it until the delete or the rollback (pkg/store/image.go:146-170),SweepStaging/RemoveStagingDirtake the same lock non-blocking and delete under it (pkg/store/staging.go:173-194, pkg/store/image.go:244-257). - The staging lock is released inside the locked section, right after the publishing rename (pkg/store/image.go:171), so a published image is never flock-held —
TestCommitImageSwapsAndRollsBackasserts it directly. The earliershould_fixabout a concurrent commit of the same digest failing is closed. - The own
.old-<pid>leftover is cleared before the store lock is taken (pkg/store/image.go:130-134), and under its own lock, so the invariant stated at pkg/store/image.go:116-119 holds forCommitImage. RemoveStagingDirreturns(false, nil)for a directory that is already gone (pkg/store/image.go:245-249) and prune printsAlready gonewithout counting the bytes (cmd/hull/prune.go:129-135);TestPruneDoesNotClaimWhatAnotherSweeperFreedcovers both halves.- A walk that fails outright now falls through to the locked re-check, not straight out:
VerifyImagereturns early only onerr == nil && short == nil(pkg/store/integrity.go:146-149), whichTestVerifyImageConfirmsAFailedWalkUnderTheLockexercises against a real swap. - The deferred
os.RemoveAll(tmpDir)is skipped once the commit published the directory (pkg/ociclient/client.go:256-262,publishedset at :317), closing the in-process second-pull hazard. - "
hull runwith the default pull policy does not count;--pull=alwaysdoes":resolveImageDigestshort-circuits oncachedDigestfor any policy butalways(cmd/hull/run.go:2762-2772), andVerifyImageis reached only fromPullPlatform. - "
hull runshares the rootfs read-only or clones it", the premise of the count being stable (pkg/store/integrity.go:38-41): the vz generic-container share isReadOnly: !containerRootfsDirectwithcontainerRootfsDirectset only on the HVI path, whoserootfsLinkis an APFS clone in the bundle (cmd/hull/run.go:816-839, :1366); the virtiofs/9pfs path copies the tree first (run.go:918-955). - The sentinel in
TestPullSkipsUnchangedDigestis back underrootfs/(pkg/ociclient/client_test.go:174), thedigesthandoff is anatomic.Pointer(client_test.go:498-512), and the staleTestDirDiskUsageCountsAHardLinkOncecomment left prune_test.go with the test (pkg/store/staging_test.go:55-74).
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok (3.1s, 13.2s)- Reverted
removeLockedDirto a blindos.RemoveAll(pkg/store/image.go:244) →TestPullLeavesAConcurrentPullsStagingAlonefails on both the.tmp-1and the.old-1assertion; restored. - Reverted the
VerifyImagecall inPullPlatform(pkg/ociclient/client.go:196) →TestPullRepairsATruncatedImagefails on "the second pull reused the truncated image" and on the follow-up verification; restored.git status --porcelainclean afterwards.
Smaller things:
- The late holders warning (pkg/ociclient/client.go:304-306) does not name a way out, where the early one does ("remove them, or
hull rmi --forceit, and pull again", client.go:200-202). It is the branch a user is more likely to be stuck in, since it costs a full download first. swapImagetreats aStat(finalDir)error other than ENOENT as "no previous image" (pkg/store/image.go:147), and the following rename then fails with ENOTEMPTY rather than saying what happened. Pre-existing shape, carried over.
Not checked:
cmd/hulldoes not build on this Linux host (darwin-only files:undefined: supervisor,composeExec, …), so no prune test ran here. I read cmd/hull/prune.go and prune_test.go instead.- The macOS-specific claims in the description (flock-on-directory behaviour, the 20601-entry repro,
golangci-lint).
Not checked:
- cmd/hull tests: the package does not build on this Linux host (darwin-only sources), so no prune test was executed; cmd/hull/prune.go and prune_test.go were read instead.
- The macOS-only claims in the description: flock semantics on a directory descriptor under APFS, the 20601-entry repro of #114, and the golangci-lint run.
Task 135d9cc6-e4a3-42fb-8aa5-3f266bc36aac on rp1, model claude-opus-5, effort medium.
| } | ||
| holders = append(holders, ImageHolder{ | ||
| ID: inst.ID, | ||
| Live: inst.Status == "running" || inst.Status == StatusStarting, |
There was a problem hiding this comment.
should_fix: ImageHolder.Live is decided on Status alone, so a stale running record counts as live and the pull can never re-unpack the image — and the doc comment's claim of parity with rmi does not hold.
Failure: the host reboots (or hull is killed) with an instance record left at Status: "running", and no hull ps has run since to reconcile it (cmd/hull/ps.go:87). hull is then upgraded with a bumped UnpackSchema, so ImageComplete is false on the schema check (pkg/store/store.go:291-301). hull pull ref skips the early return, downloads every layer, unpacks, and then imageHeld (pkg/ociclient/client.go:291, :325-345) sees Live: true for the dead record and discards the fresh tree. The pull exits 0 with a warning and the schema-stale rootfs stays — which is the state the schema bump exists to get out of (a schema-1 rootfs reaches the guest with setuid stripped). hull run does not escape it either: cachedDigest treats the incomplete image as a miss (cmd/hull/run.go:2718-2726), so every run re-downloads the image and discards it again. The same record blocks the rootfs-entries repair path.
Evidence: pkg/store/integrity.go:218 is inst.Status == "running" || inst.Status == StatusStarting, with the comment at :191-195 saying "as rmi counts it". rmi does not count it that way: instanceMayHaveVMM requires processIsAVMM(inst.PID, inst) for every record carrying a pid (cmd/hull/rmi.go:231-235), and the comment above it at rmi.go:219-226 gives this exact reason — a record left "running" by a death after the pid write otherwise "refused every removal for good" (#70).
Fix: give the holder check the liveness predicate cmd/hull already has (pkg/store cannot call processIsAVMM itself), or have the pull call it, so a running record with a pid that is not a VMM reads as stopped. Failing that, narrow the comment at :191-195 to say the test is weaker than rmi's and that a stale record pins the image.
| } | ||
| return nil | ||
| defer func() { _ = l.Close() }() | ||
| if err := os.RemoveAll(dir); err != nil { |
There was a problem hiding this comment.
nit: The pre-unpack sweep now deletes leftovers with the exclusive store lock held, where it used to delete them with no lock at all.
Failure: a store holds a <digest>.old-<pid> from a pull that died before its delete finished — a whole 20k-entry rootfs. The next hull pull calls SweepStaging (pkg/ociclient/client.go:240), which goes through RemoveStagingDir: s.mu.Lock() plus the store flock (pkg/store/image.go:224-233), then os.RemoveAll here. For the seconds that takes, every other process's CreateInstance, SaveInstance and DeleteInstanceRecord blocks on the same flock, so a hull ps or an instance state transition next door stalls. Before this PR that sweep was a lockless filepath.Glob + RemoveAll; only prune paid the store lock, and prune is a command the user chose to run.
Evidence: the removed lockless sweep in the diff at pkg/ociclient/client.go (the *.tmp-*/*.old-* glob); the store lock taken at pkg/store/image.go:227-232 and held across :253 by the deferred unlock; the competing takers in pkg/store/store.go. The same reasoning is already written down at pkg/store/image.go:116-119 for CommitImage ("the lock is held for two renames and not for seconds").
The directory's own flock is what excludes the other sweepers and the pull here, so the store lock around the RemoveAll looks removable: take it for the lock acquisition and drop it before the delete.
130e06c to
7ddf2c8
Compare
|
@claude-fleet review |
|
Review task queued. Task: 2dfc5915-97b1-4c66-be55-4c453624f839 |
There was a problem hiding this comment.
Approve: three nits, nothing blocking.
Every finding from the earlier rounds is addressed in the head commit. The lock design holds up: a pull's flock on the staging directory's descriptor follows the inode through both renames, the sweeper takes it non-blocking and deletes while holding it, and CommitImage releases the pull's lock inside the locked section so a published image is never flock-held.
Verified:
- The pre-unpack sweep is lock-driven and leaves a concurrent pull's
.tmp-and.old-alone, while still taking a dead pull's leftovers — pkg/store/staging.go:173-194, pkg/ociclient/client.go:247 (revert check below). - The staging lock is released at the publishing rename, not by the caller, so a second commit of the same digest does not find the image busy — pkg/store/image.go:181, asserted at pkg/store/image_test.go:388-392.
- Both slow deletes are outside the store lock: the own
.old-<pid>leftover before it (pkg/store/image.go:139), the displaced image after it (pkg/store/image.go:147).RemoveStagingDirtakes the store lock only for the lock acquisition (pkg/store/image.go:248-259) and deletes outside it. - A directory gone between prune's listing and its delete returns
(false, nil), and prune printsAlready gonewithout counting the bytes — pkg/store/image.go:236-240, cmd/hull/prune.go:131-136. VerifyImagefalls through to the locked re-check on a walk error as well as a shortfall, so a concurrent swap does not read as damage — pkg/store/integrity.go:146-149 (err == nil && short == nil), withrootfsShortreturning walk failures in the second slot at :177-180.- The holder liveness check is wired to the same predicate rmi uses, from both call sites — cmd/hull/pull.go:59 and cmd/hull/run.go:277 set
InstanceLive = instanceMayHaveVMM(cmd/hull/rmi.go:230-236); the nil fallback is documented as status-only at pkg/ociclient/client.go:41-45. - The claim behind the entry count, "nothing writes into a published rootfs" (pkg/store/integrity.go:39-41): on the vz generic-container path the share is read-only — cmd/hull/run.go:1367 is
ReadOnly: !containerRootfsDirect, andcontainerRootfsDirectis set only on the HVI branch that first replaces the symlink with an APFS clone (cmd/hull/run.go:830-836). - The sentinel is back under
rootfs/inTestPullSkipsUnchangedDigest(pkg/ociclient/client_test.go:174), and the stale hard-link comment is gone from prune_test.go — that test now lives at pkg/store/staging_test.go:55-74.
Checks run:
go test ./pkg/store/ ./pkg/ociclient/→ ok; same with-race→ ok.GOOS=darwin GOARCH=arm64 go build ./...andgo vet ./cmd/hull/ ./pkg/store/ ./pkg/ociclient/→ clean (the packages do not build on this Linux host otherwise).- Reverted the sweep at client.go:247 to the old
*.tmp-*/*.old-*glob →TestPullLeavesAConcurrentPullsStagingAlonefails on both the.tmp-1and the.old-1assertions. Restored. - Reverted
damaged := c.store.VerifyImage(digestStr)todamaged := error(nil)→TestPullRepairsATruncatedImageandTestPullKeepsADamagedImageAnInstanceHoldsfail. Restored;git status --porcelainclean afterwards. os.renameof a directory onto an existing non-empty directory →[Errno 39] Directory not empty; onto an empty one it succeeds. Backs finding 1.
Smaller things:
- The lock contract is restated in six places: pkg/store/staging.go:27-47 (file header), :90-99 (
CreateStaging), pkg/store/image.go:108-128 (CommitImage), :210-228 (RemoveStagingDir), pkg/ociclient/client.go:242-260, and docs/storage.md:162-170. "The lock follows the inode" alone appears at staging.go:38, image.go:109 and image.go:123. The file header in staging.go is the natural single home; the others could point at it and keep only what is local to them. hull run --pull=alwaysagainst a schema-stale image with a live holder now returns success with the stale rootfs rather than re-unpacking (pkg/ociclient/client.go:310-317 returns aPullResult, and cmd/hull/run.go:2773-2784 takes its digest). That is the right trade against deleting a running guest's tree, and it is warned, but the guest boots a schema-1 rootfs with setuid stripped — the state the schema bump exists to leave. Worth a line in the follow-ups alongside the two already listed.
Not checked:
- cmd/hull tests: the package does not build on this Linux host (darwin-only sources). It compiles and vets under GOOS=darwin, but prune_test.go was not executed.
- The issue #114 repro and the 20601-entry comparison from the description, which need a real registry and a macOS host.
- flock-on-a-directory-descriptor semantics on macOS (EWOULDBLOCK on a second descriptor, survival across rename); verified only on Linux, where the tests pass.
- golangci-lint v2.12.2, which CI runs.
Findings outside the diff:
- pkg/store/image.go:88 (nit):
DeleteImagerenames onto<digest>.old-<pid>without clearing a leftover of that name, which is the hazardCommitImagenow handles thirty lines below.
Failure: ahull pullat pid 500 is SIGKILLed during the best-effortos.RemoveAll(oldDir)(pkg/store/image.go:147), leaving a populated<digest>.old-500. The kernel later recycles 500 onto a hull that runshull rmi <digest>:os.Rename(imageDir, aside)here hits the existing non-empty directory andhull rmiexits non-zero withfailed to displace image <digest>: rename ...: directory not empty. The image stays, and the only way out is ahull pruneor another pull to sweep the leftover first.hull prunedeleting the same digest hits it the same way.
Evidence: the unguarded rename at pkg/store/image.go:88 against the leftover clearCommitImagedoes at pkg/store/image.go:136-141 (removeLockedDir(oldDir)), with its comment at :111-113 naming exactly this reason. Confirmed the syscall behaviour:os.renameof a directory onto an existing non-empty directory returns[Errno 39] Directory not empty; onto an empty one it succeeds.
This is pre-existing and not a regression — the recovery is cheap and nothing is lost — but it is the same bug the diff just fixed one function away.removeLockedDir(aside)before the rename closes it, and it is the behaviour the new comment at :64-65 already implies.
Task 2dfc5915-97b1-4c66-be55-4c453624f839 on ci-16gb-fsn1-1, model claude-opus-5, effort medium.
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return nil, fmt.Errorf("failed to create the staging directory: %w", err) | ||
| } | ||
| l, err := lockDir(dir, true) |
There was a problem hiding this comment.
nit: CreateStaging's blocking wait is justified by a claim the PR contradicts elsewhere: the lock's other holder is not always a sweeper.
Failure: pkg/store/image.go:136-138 states that a store shared between two pid namespaces can have another process's pull behind a .old-<pid> name, and the same holds for .tmp-<pid>. When it does, lockDir(dir, true) here blocks for the whole of that pull's download rather than the moment a sweeper's delete takes — and it blocks before newProgress is constructed (pkg/ociclient/client.go:274), so hull pull sits silent with no output and no timeout. The in-process case (two PullPlatform goroutines on one digest) behaves the same way.
Evidence: the comment at pkg/store/staging.go:96-99 ("the only other holder is a sweeper ... and that is over in a moment") against pkg/store/image.go:136-138.
The wait is still the right call — the alternative is failing a pull over a transient sweep. Narrowing the comment to say a same-name pull can also hold it, and for however long it runs, keeps a later change from being judged against the shorter guarantee.
| // into its guest, and displacing the directory would pull the root | ||
| // filesystem out from under it; rmi and prune refuse the same way. The | ||
| // damaged image is served as is, with the way out in the warning. | ||
| if c.store.ImageComplete(digestStr) { |
There was a problem hiding this comment.
nit: The gate in front of VerifyImage takes no store lock, so the swap window the new confirm-under-the-lock pass exists to tolerate can still cost a full re-download.
Failure: process A is in swapImage, between os.Rename(finalDir, oldDir) and os.Rename(stagingDir, finalDir) (pkg/store/image.go:165-176), both under the store flock. Process B runs hull pull --pull=always on the same digest and calls ImageComplete, which takes only the in-process s.mu.RLock and stats image.json (pkg/store/store.go:287-293) — no store flock, so it is not excluded from the window. It sees no directory, returns false, and B skips the cached path entirely and re-downloads and re-unpacks an image that is fine.
Evidence: ImageComplete at pkg/store/store.go:286-302 takes no lockFile(); VerifyImage's doc at pkg/store/integrity.go:132-140 promises that "a concurrent commit cannot read as damage", and the locked re-check at integrity.go:151-162 delivers that — but only for callers that get past this gate.
The window is two renames rather than a RemoveAll, and the direction is safe (a wasted download, never corruption), so this is scope for a later change rather than this one. Worth noting in the VerifyImage comment that the guarantee covers the verify and not the completeness check in front of it.
papazof
left a comment
papazof
left a comment
There was a problem hiding this comment.
I ran 7ddf2c8 on a Mac (macOS 26.5.1, arm64) against 159772e, with vz and hvi instances in a scratch store; the numbers in the comments come from those runs. The last two rounds hold up live. An instance running on an image stamped with an older unpack schema keeps its image, and its guest keeps working after a hull pull. A pull that sweeps a 15.7k-entry leftover no longer blocks the store lock: a second process taking the flock every 20 ms waited at most 0.000 s, against 0.34-0.56 s at 130e06c.
All of the following are non-blocking. The way out that the damaged-image warning names does not work while a holder is running. The late warning says a running instance "came into use" when it was running all along. Two pulls of one digest in one process fail each other's commit. The commit-time comment understates the window it leaves. The CreateStaging retry loop has no test. On the prune thread I added what is left of the rmi case.
Concurrent pulls swept each other's staging directories, causing failed pulls or published images with missing layers. Protect staging and displaced images with directory flocks, and make pull and prune sweeps delete only unlocked directories. Check the locked inode against the path after creation so a sweep between mkdir and locking cannot leave a pull writing unprotected. Publish and remove images by renaming under the store lock, then delete the displaced tree outside it while holding its directory lock. Release the staging lock as soon as publication succeeds. Give each displacement a random suffix so concurrent commits or removals in one process cannot collide with an earlier cleanup. Interrupted leftovers remain available to ordinary sweeps. Add deterministic staging retry and inode replacement tests, concurrent commit coverage, and checks that active image removal stays protected without holding the store lock. Prune distinguishes already-removed leftovers and explains that running instances must be stopped before forced image removal. Pulls made by older hull versions hold no directory lock, so an overlapping upgrade can still mistake their staging directories for abandoned ones. Fixes: #114 Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
Record the rootfs entry count after unpacking and check it before a pull reuses a cached image. Missing entries trigger a fresh unpack. Walk without the store lock, then confirm shortfalls or errors under it so a concurrent swap cannot make the verification report damage to a whole image. Keep images held by live instances, and complete images held by stopped instances, checking again before publication. Refresh resolution metadata on either skip path. Warnings name the holders and explain how to stop them before forced image removal, or remove the instances, then pull again. The holder check only sees a run once launchVMM records its image digest; bundle preparation, rootfs cloning, and holders recorded between the check and swap remain outside this protection. The late warning covers both pre-existing holders and instances started during the download. The count detects entries lost after it was recorded, not a tree already short at publication. Legacy images without a count remain trusted, and ordinary cached runs do not pay for the walk. Add store and pull regression tests for repair and holder protection. Refs: #114 Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
7ddf2c8 to
0492948
Compare
|
Addressed all six of papazof's comments, folded into the two semantic commits (
Validation: full Go suite passes for each commit; focused race tests, golangci-lint, and Linux store vet pass. |
papazof
left a comment
papazof
left a comment
There was a problem hiding this comment.
Thanks, all six check out on 0492948. I ran it on the same Mac (macOS 26.5.1, arm64) against 159772e:
- Both warnings now give steps that work. With a running holder,
hull stopthenhull rmi --forceremoves the image and the next pull repairs it. The late warning fires for an instance on a schema-2 image and keeps both the image and the guest. hull pruneandhull prune --dry-runstarted during ahull rmiof golang:1.23-alpine (15,713 entries) report "Nothing to prune" in all three runs. Ahull rmikilled mid-delete leaves a.old-<pid>-<unique>that the next prune removes.- Two goroutines committing one digest: 80,028 commits in 20 s, none failed.
hull rmiof the same image no longer holds the store lock while it deletes: a second process taking the flock every 20 ms waited at most 0.000 s, against 0.35-0.50 s at 159772e. An old-style.old-<pid>leftover is still swept by the next pull.go test -race ./pkg/store/ ./pkg/ociclient/andgo test ./cmd/hull/pass on darwin. The six new store tests pass 20 times in a row under-race, and theCreateStagingretry branches are now covered except a failingMkdirAll.
One optional nit, outside the diff: after an interrupted hull rmi, prune labels the leftover "previous image left behind by an interrupted pull" (cmd/hull/prune.go:196), while docs/storage.md now says "an interrupted pull or image removal". Matching the label to the docs would cover both.
2 participants
Fixes #114: concurrent pulls could sweep a staging directory another pull was still unpacking into, failing that pull or publishing an image with missing layers.
The change is organized into two commits:
The holder check sees a run only after
launchVMMrecords its image digest, following bundle preparation and rootfs cloning. It also cannot protect a holder recorded between that check and the swap. The count cannot detect a tree already short when counted; older images without a count remain trusted. Ordinary cached runs do not perform the walk. A pull from an older hull version holds no directory lock, so an overlapping upgrade retains that compatibility gap.Validation: the full Go suite, race tests for
pkg/store,pkg/ociclientandcmd/hull, golangci-lint, and Linux vet forpkg/storepass. Both commits pass the full Go suite independently. New concurrent-commit and legacy-leftover tests fail on the original PR head and pass with these changes. Deterministic tests cover staging disappearance, inode replacement, retry exhaustion, and active image removal retaining its directory lock while releasing the store lock.