imagecache: Re-unpack a layer retired while joining its flight

[igooch]

Fixes #1076. Overlaps #1079 — both fix the same flake, only one should merge.

The bug

ensureLayer and retireLayer shared one singleflight key per layer. That sharing was the retire/reuse interlock, but it made the pull path depend on something singleflight cannot express: a caller whose Do joins a flight rather than leading one learns nothing about what it joined. When the flight was a retirement, ensureLayer returned the layer path anyway — and the rename had just moved it aside. pull recorded a layer with nothing behind it, and its re-verify failed a healthy pull with layer dir vanished during pull (evicted?).

#1076 attributes this to an eviction cutoff postdating ensureLayer's mtime refresh. That can't fire at the default minAge = 2m: cutoff = now - 2m while a refreshed dir has mtime ≈ now, so retireLayer always vetoes. That window is real only at minAge ≈ 0.

The fix

Use a lock for the interlock, and leave the singleflight to do only what it's good at.

• Interlock: a per-layer semaphore.Weighted, held by ensureLayer across its stat and refresh-or-unpack, and by retireLayer across its stat and rename. The two still can't interleave, but a waiter now waits and then re-reads the pool, instead of inheriting a verdict it can't interpret. Retirement uses TryAcquire, so a GC pass never blocks behind a download — same effect as the old join-is-a-veto rule.
• Singleflight: deduplication only, where joining is unambiguous. The leader's outcome is the joiner's — one download per herd, failure included, as it was before eviction existed.

The interlock is per layer, never shared between them: it is held for the length of an unpack, so two layers on one lock would serialize unrelated downloads and let a retirement veto — and so re-date for a whole min-age window — a layer it could have taken. semaphore (already a dep, same module as singleflight) rather than sync.Mutex because retirement needs TryAcquire and the pull path gets a context-aware Acquire; k8s.io/utils/keymutex has no try-variant. The Acquire falls back to TryAcquire on a context error, serving only the cache hit: the semaphore prefers the context error over a free slot, and a cancelled pull leading the flight for an already-cached layer must not fail the lookup for healthy joiners — but with the layer absent, the context error is returned rather than starting a download the layer streams (bound to the pull's parent context) would never cancel. Entries are created on first use and never removed — the one piece of per-layer bookkeeping eviction does not reclaim — because releasing one safely while a caller waits on it needs refcounting.

No behavior change outside the interlock: the layer-presence check keeps the semantics it always had (any stat failure counts the layer absent), now shared by its three call sites as layerFSPresent.

Testing

The flake never reproduced naturally (~49,000 iterations, zero failures). With a throwaway instrumented build that widened the retire window and forced the evictor to win the race, joins-that-found-no-dir matched failures 1:1 in every run. Against this branch the same harness gives 0 failures at every head start, versus 66/182/186 before.

Five committed tests pin the interlock:

• retirement vetoed while the interlock is held
• ensure waiting out a retirement, then repacking
• ensure waiting out a failed retirement, then reusing
• joined ensure sharing its leader's failure
• distinct layers never sharing an interlock
• a cancelled context not failing a lookup whose layer is already cached

The three that race release the held lock only once the call under test is observably blocked, so they cannot pass without exercising the contended path.

Full package -race: 74 pass, 0 fail. TestConcurrentEnsureImageAndEvict 300× race-clean, TestRetireLayerVsEnsureImageRace 200× race-clean. make verify clean except metrics.sh (needs Docker), proto-fmt.sh (needs clang-format), shellcheck.sh — the latter two fail identically on main, and this touches no metrics, protos, or shell scripts.

Deliberately not fixed here

A joiner inherits a cancelled leader's context.Canceled, so pull B can fail on pull A's caller going away. That predates this PR — ensureLayer on main already returns the leader's error unconditionally — and the interlock neither causes nor worsens it. Undoing it means deciding whether shared work should follow any one caller's context at all, which is wider than this change; ensureLayer's doc comment now records the decision.

One caveat on the usual justification: I could not find a server-side retry on the Run/Restore path that would absorb it. EnsureImage failures reach maybeCrashActor, are not crash-tagged, and come back as a plain error to the Resume RPC caller — there is no requeue. So the self-healing argument rests on the RPC client, which I have not traced.

Follow-up (separate issue)

A pull whose last layer downloads for longer than minAge with no sibling completions stops touching its record, so eviction can legitimately remove its completed layers and the pull fails into a retry with the same vanished during pull string. That is the documented wedge-detection design, not the race this PR fixes; a time-based record heartbeat during long downloads would be its own change.

TestConcurrentEnsureImageAndEvict tests less than it appears: over 5,000 iterations it produced 33 eviction candidates and removed zero records, so the eviction-wins branch is almost never taken.

[Taahir Ahmed (ahmedtd)]

Can you clean up the PR description? It's very difficult to follow.