imagecache: Re-unpack a layer retired while joining its flight - #1509
Open
igooch wants to merge 4 commits into
Open
imagecache: Re-unpack a layer retired while joining its flight#1509igooch wants to merge 4 commits into
igooch wants to merge 4 commits into
Conversation
Collaborator
|
Can you clean up the PR description? It's very difficult to follow. |
The reuse/retire interlock was one-sided. retireLayer treats joining a call already in flight as a veto, because its closure never ran and it therefore knows nothing about the dir. ensureLayer made the same kind of join and drew the opposite conclusion: singleflight.Do returned the other call's nil error, so ensureLayer returned the layer path as if it had verified it. When the flight it joined was a retirement, that path had just been renamed aside and nothing unpacked it back. The pull path then recorded the layer and its final re-verify reported "layer dir vanished during pull", failing an otherwise healthy pull into an RPC retry. The window is a few microseconds wide, so it surfaced only as a rare failure under CI load. Give ensureLayer the guard retireLayer already has: settle for a dir only when we ran the flight ourselves, or when the tree is still on disk once the joined flight finishes. Anything else means we joined a retirement, so re-enter and unpack the layer again, bounded by maxEnsureLayerFlights. Checking the dir rather than always re-entering keeps the dedup intact, so concurrent pulls of one image still collapse onto a single unpack. The pull path keeps its re-verify: it still guards a dir removed by something outside the flight, which the singleflight cannot see.
…esult Review of the join-recovery logic found the guard trusted half of what it set out to distrust. A joined flight's error returned before the ran check, so a caller that joined a retirement whose rename failed reported "while retiring layer ..." and failed its pull, even though that failure left the layer on disk and usable; joining an unpack cancelled by the leader's context poisoned joiners whose own context was still live. Gate the error on ran. Nothing a joined flight reports is the joiner's to act on -- neither its error nor its success -- so the pool decides, which is what the recovery path was already doing for the nil case. Distinguish a stat error from an absent layer while here. Reading a transient EIO as "not there" sent the caller down the unpack path, which then failed at the commit rename with the healthy dir still in place. retireLayer draws the same distinction; layerFSPresent now names it for the two sites in ensureLayer. The copies in cachedImage and the pull re-verify keep treating any error as absence, which is a separate behavior question. Also: check the caller's context before re-entering, since re-entry means unpacking a possibly multi-GiB layer again, and stop logging "unpacking again" on the last iteration, where the next thing that happens is the error return. The tests released the held flight on a 20ms timer, so a loaded machine could close it early, let ensureLayer lead its own flight, and pass without exercising the join at all. They now release only once a goroutine is observably parked joining the flight, and fail if that never happens.
igooch
force-pushed
the
fix/ensurelayer-retire-flight-join
branch
from
September 5, 2026 15:31
63b89d4 to
642f5b9
Compare
…joined The bounded re-entry loop in ensureLayer counted joins, not causes. Any joined flight that left the layer dir absent went around again, but a sibling pull's failed unpack leaves the dir absent just as a retirement does. That conflation had three faces: a caller that exhausted the budget joining failing sibling pulls reported "retired repeatedly under concurrent eviction" with no eviction anywhere and the real error (registry down, ENOSPC) discarded; each joiner of a persistently failing download re-led full download attempts of its own instead of sharing the leader's failure; and the joined-retirement-with-dir-present path returned a dir whose mtime the flight never refreshed. Make the closures say what they did: both return a flightOp through the shared singleflight. A joined ensure now resolves the call either way -- one download shared by every waiter, success and failure alike, which is the collapse semantics the pull path had before eviction existed. Only a joined retirement re-enters, and it re-enters unconditionally, so a present layer is re-accepted through a led flight whose mtime refresh runs inside the interlock rather than through an unguarded stat. The loop's terminal error is now literally true, and the bound derives cleanly: two flights cover a retirement, the third an overlapped eviction pass that pre-checked the dir before the rename landed. Also convert the remaining layer-presence stats to layerFSPresent, so a transient stat failure surfaces as an error everywhere instead of reading as an absent layer in cachedImage and the pull re-verify; and grow the stack buffer in waitForFlightJoiner, which runtime.Stack silently truncates at a fixed size, starving the matcher on goroutine-heavy runs.
Sharing a joined ensure flight's failure is right for failures any leader would hit -- a down registry, a corrupt blob -- but a leader cancelled by its own caller is not that: pull A's errgroup collapsing says nothing about the layer or the registry, and inheriting it failed pull B's whole image with someone else's cancellation. On a joined ensure failure that is a context error while our own context is live, go around the loop and lead a fresh flight instead. Such joins consume the same flight budget as retirements, so the terminal error distinguishes the two, and it renders the leader's error with %v rather than wrapping it: a %w would make the give-up error itself satisfy errors.Is(err, context.Canceled) for a caller whose context was never cancelled. Also correct the derivation comment on maxEnsureLayerFlights: the third flight is not for overlapped eviction passes, which evictMu rules out, but for a retirement whose rename failed -- the dir stays with its old mtime, the kept record's refcounts are restored, and a later candidate image sharing the layer leads a second retire flight in the same pass. retireLayer's pre-flight comment now notes that its absent-dir return is load-bearing for that bound, and the join check documents that ran implies opEnsure.
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.
Fixes #1076
The bug
ensureLayerandretireLayerdeliberately share one singleflight key per layer — that sharing is the reuse/retire interlock — so either can join a flight the other is leading. The interlock was one-sided about joins:retireLayertreats a join as a veto (its closure never ran, so it learned nothing), butensureLayerdrew the opposite conclusion from the same situation.Doreturned the other call'snilerror, andensureLayerreturned the layer path as if verified.When the flight it joined was a retirement, that path had just been renamed aside and nothing unpacked it back.
pullrecorded the layer anyway, and its re-verify failed a healthy pull withlayer dir vanished during pull (evicted?), surfacing as an RPC retry. The window is a few microseconds wide, hence CI-only.Correcting the diagnosis in #1076
The issue blames an eviction cutoff postdating
ensureLayer's mtime refresh. That can't fire here: with the defaultminAge = 2m,cutoff = now - 2mwhile a refreshed dir has mtime ≈now, soretireLayeralways vetoes. That window is real only atminAge ≈ 0.Evidence
Counting joins that leave no
fsdir behind matches the failures exactly (34→34, 149→149, 191→191, 1→1): no failure ever occurred without a join, the rate tracks the flight-window width, and guarding the join drops it to zero while the joins continue.A/B on this branch, same harness, 200 iterations per head start:
The fix
Make the flight say what it did. Every layer-flight closure now returns a
flightOpthrough the singleflight, so a caller that joined knows which operation it joined and treats the two cases differently:Re-entry is bounded by
maxEnsureLayerFlights— singleflight has no fairness, so "retry until we lead" has no bound of its own — with a ctx check between flights. No livelock:retireLayervetoes on the symmetric join.Presence checks now go through
layerFSPresent, which reads onlyErrNotExistas absence. A transient stat error (EIO and friends) is reported instead of being mistaken for a missing layer and triggering a re-download that would fail at the commit rename anyway.Unchanged on purpose
pull's re-verify andTestPullReverifyFailsCleanlyOnYankedLayerstay. The re-verify guards a dir removed outside the flight, which the singleflight can't see. Fixing the cause didn't require weakening the backstop.Testing
Three deterministic tests pin the three join cases. Each holds the layer flight open the way a real leader would and releases it only once the
ensureLayerunder test is observably parked joining it (waitForFlightJoinerscans the goroutine dump — releasing on a timer would let a loaded machine close the flight early and pass the test without ever exercising the join path):TestEnsureLayerJoiningRetireFlightRepacksLayer— joined a retirement that renamed the dir away: unpack it again. Fails onmain, passes here.TestEnsureLayerJoiningFailedFlightKeepsLiveLayer— joined a retirement whose rename failed: the layer is live and usable, so lead a flight and return it rather than failing the pull with an eviction-internal error.TestEnsureLayerJoiningFailedEnsureSharesError— joined a failed ensure: propagate the leader's error rather than piling on download attempts.Full package
-race: 70 pass, 0 fail.TestConcurrentEnsureImageAndEvict: 500 consecutive-raceruns clean.make verifyclean exceptmetrics.sh(needs Docker),proto-fmt.sh(needsclang-format),shellcheck.sh— the latter two fail identically onmain, and this touches no metrics, protos, or shell scripts.Follow-up (separate issue)
TestConcurrentEnsureImageAndEvicttests less than it appears: over 5,000 iterations it produced 33 candidates, skipped all as fresh, and removed zero records — theEnsureImagehit beats the evictor ~99.3% of the time, so the eviction-wins branch is almost never taken.