imagecache: write the image record before unpacking layers - #717
Merged
Dmitry Berkovich (dberkov) merged 1 commit intoAug 4, 2026
Merged
Conversation
Inverts the pull's write order: the image record (whose diffID list is known from the config before any download) is written before the unpack loop, touched as each layer completes, and rewritten after the loop. A final re-verify confirms every returned layer dir is on disk. Why: the record's diffID list is what eviction's refcounts will trust, so writing it first means every layer of a pull is referenced before it can exist — the pull needs no other protection, and that protection survives atelet restarts. The per-layer touch makes liveness progress-based: a pull that is advancing keeps its record fresh indefinitely, while a wedged pull ages out and is reclaimed as an ordinary LRU entry. The end-of-pull rewrite guarantees success always leaves a record even if eviction removed it mid-flight, and the re-verify turns the residual failure case into a clean retry instead of a bundle spec naming a nonexistent lowerdir. Semantics note: a record now means 'known image, possibly partially present' — which is what it always meant to readers: cachedImage verifies every layer and re-pulls only what is missing, so an interrupted pull's record is resumable progress. Also new: the config diffID list is cross-checked against each layer's actual diffID, and a config/manifest layer-count mismatch fails the pull up front. Tests: a gated registry holds real pulls mid-flight by blocking chosen blob downloads — record removed mid-pull is rewritten on success; per-layer completions touch the record while later layers are still gated; a yanked layer dir fails the pull with a retryable error. Extracted from the phase2-gc-record-first prototype branch. This slice: go test -race green standalone; behavior-visible change even without GC: an interrupted pull now leaves a valid partial record.
Dmitry Berkovich (dberkov)
approved these changes
Aug 4, 2026
Dmitry Berkovich (dberkov)
merged commit Aug 4, 2026
848b75b
into
agent-substrate:main
11 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth slice of #463 Phase 2, and the semantic pivot of the GC design: the pull
now writes the image record before unpacking, touches it as each layer
completes, rewrites it after the loop, and re-verifies every returned layer dir.
Why record-first: the record's diffID list (known from the config before any
download) is what the eviction engine's refcounts will trust — writing it first
makes every layer referenced before it can exist, so pulls need no separate
protection mechanism, and the protection survives atelet restarts. The
per-layer touch makes liveness progress-based: a wedged pull ages out through
ordinary LRU; the end-of-pull rewrite means success always implies a record;
the re-verify turns the residual race into a clean RPC retry instead of a
bundle spec naming a missing lowerdir.
Also new: the config's diffID list is cross-checked against the manifest's
layer count up front and against each layer's actual diffID as it lands, so a
record can never reference something different from what is on disk.
Note for Dmitry Berkovich (@dberkov): this inverts Phase 1's record-last ordering, which
appears incidental (amendment 4 in the Phase 2 comment on #463 — please
contradict if there was intent). A record now explicitly means "known image,
possibly partially present", which is what every reader already handles:
cachedImageverifies each layer and re-pulls only gaps. The one behaviorchange observable without GC: an interrupted pull leaves a valid partial
record — resumable progress — instead of nothing.
Testing: a gated registry holds real pulls mid-flight by blocking chosen
blob downloads — interrupted pull leaves a resumable record and a retry
completes it; a record deleted mid-pull is rewritten on success; per-layer
completions freshen the record while later layers are still gated; a yanked
layer dir fails the pull with a retryable error.
go test -racegreen.