Skip to content

imagecache: Pin a pull's layers so eviction cannot retire them mid-pull - #1509

Merged
Benjamin Elder (BenTheElder) merged 3 commits into
agent-substrate:mainfrom
igooch:fix/ensurelayer-retire-flight-join
Oct 19, 2026
Merged

Benjamin Elder (BenTheElder) merged 3 commits into
agent-substrate:mainfrom
igooch:fix/ensurelayer-retire-flight-join

Conversation

@igooch

@igooch igooch commented Sep 5, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1076.

The bug

ensureLayer and retireLayer shared one singleflight key per layer, and that shared flight was the retire/reuse interlock. A caller that joins a flight rather than leading it runs no code and learns nothing about what the flight did. When the flight was a retirement, ensureLayer returned the layer path anyway, just after the rename had moved it aside, and the pull failed its final check with layer dir vanished during pull. The window is microseconds wide, so it shows up as a rare CI flake.

The fix

Count a pull's in-flight use of each layer instead of using the flight as a lock.

  • A pull pins each of its layers before any layer work and unpins when it returns.
  • retireLayer never retires a pinned layer. The pin check, a re-stat and the rename run under one mutex, so either the pin lands first and retirement is vetoed, or the rename lands first and the pull unpacks the layer again. A missing or fresh layer is ruled out before the mutex is taken.
  • Retirement no longer uses the layer singleflight, so a joined flight is always another pull's, and its result can be trusted.

Tradeoff

A wedged pull holds its layers until it returns, bounded by the pull timeout. Under disk pressure that holds space longer than before. In exchange, a slow pull can no longer lose layers it already finished.

@ahmedtd

Copy link
Copy Markdown
Collaborator

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

@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch 10 times, most recently from 8c113ee to db9b3aa Compare September 8, 2026 02:08
@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch 2 times, most recently from bad54ac to 9618ddd Compare September 9, 2026 18:08
@bowei Bowei Du (bowei) added kind/bug Something isn't working / bugfixes area/storage labels Sep 17, 2026
@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch 2 times, most recently from 177a278 to 7d468f0 Compare September 28, 2026 20:54
@igooch igooch changed the title imagecache: Re-unpack a layer retired while joining its flight imagecache: Pin a pull's layers so eviction cannot retire them mid-pull Sep 28, 2026
@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch from 7d468f0 to f759658 Compare September 28, 2026 21:33
@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch 3 times, most recently from ba7a5da to 82b9951 Compare September 28, 2026 21:59

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI assisted.

With my human eyeballs: the pinMu seems like it could be improved, but I'd rather deflake CI and follow-up.

Comment thread internal/imagecache/retire.go Resolved
Comment thread internal/imagecache/README.md Resolved
Comment thread internal/imagecache/retire.go Resolved
Comment thread internal/imagecache/imagecache.go Resolved
@igooch

igooch commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main with the nits addressed. #2279 is stacked on this and rebased too.

@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch from 8a851d3 to 979f917 Compare October 16, 2026 05:55
@zlammerts-svg

Copy link
Copy Markdown
Collaborator

igooch Good morning, can you please make sure this PR gets merged today? Thanks!

@igooch
igooch removed the request for review from Bowei Du (bowei) October 19, 2026 16:19
ensureLayer and retireLayer shared one singleflight key per layer, and
that sharing was the retire/reuse interlock: reuse refreshed the layer's
mtime inside the flight, retirement re-checked it inside the same flight.
But a caller whose Do joins a flight rather than leading one runs no
closure and learns nothing about what it joined. When the flight was a
retirement, ensureLayer returned the layer path anyway, which the rename
had just moved aside. The pull recorded a layer with nothing behind it,
and its final re-verify failed a healthy pull with "layer dir vanished
during pull". The window is microseconds wide, so it surfaced only as a
rare failure under load.

Protect a pull's layers the way a cache shrinker protects objects in use,
with a count of current users rather than a lock. A pull pins each of its
layers before any layer work and unpins when it returns. retireLayer
checks the pin and the mtime and renames under the same mutex, and never
retires a pinned layer: either the pin lands first and the retirement is
vetoed, or the rename lands first and the pull finds the dir missing and
unpacks it again. Retirement no longer touches the layer singleflight, so
every flight a pull can join is another pull's ensure, whose result it
can trust.

The pin map holds only layers in use by in-flight pulls and drops entries
at zero. The pin is taken per pull, outside the flight, so a pull that
joins another's flight is covered by its own pin once the leader's lapses.

This changes what happens to a wedged pull. Its record still ages out
after min-age, but its finished layers now stay until the pull returns,
instead of being retired while the download is still running and failing
the pull at its re-verify.
Closes the gaps left when pins replaced the shared layer flight.

pull's re-check under the image flight returned layer dirs through
cachedImage, without touching the record, so that one path handed out
dirs with neither a pin nor the min-age cover every other hit gets. It
now goes through cachedImageHit like the hit path.

retireLayer logged under pinMu, the lock every pull takes to pin, so a
slow log handler stalled every pull on the node. The critical section
now does only the pin check, a re-stat and the rename, and the caller
logs after it returns.

dryRunRetire re-implemented the stat and cutoff rule and checked pins
before existence, so for a pinned layer that had not landed yet the
dry run kept a record the real pass evicted. It now calls retirable in
the same order retireLayer does.

A pull took pinMu once per layer; pinLayers pins the whole set under
one acquisition. The unpin's idempotence guard had no caller and is
gone.

TestPullPinsLayersAgainstEviction waited for the record mtime to reach
the layer dir's, which coarse filesystem timestamps satisfy before the
pull's touch, so the backdate could race the touch and skip the record
as fresh. It now backdates and runs the passes until the record reaches
retirement; the touch happens once, so a pass it refreshed changed
nothing.
@igooch
igooch force-pushed the fix/ensurelayer-retire-flight-join branch from 78582a9 to 7fe236c Compare October 19, 2026 17:24

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the review delay, lot of PRs in flight and this package has gotten pretty convoluted in main, whee agents :^)

I think we could merge this now, but the nit on retireLayer might be worth fixing first.

Comment thread internal/imagecache/imagecache.go Resolved
Comment thread internal/imagecache/retire.go Resolved
A pull pins every layer before any of them is unpacked, so a pinned
layer with no dir under its final name is one still unpacking, not one
already removed. retireLayer reported it gone, so while every layer only
a record referenced was still downloading, an eviction pass deleted that
record, and if the pull then failed the layers it had finished had no
record until the next pull of that image or the startup orphan sweep.

Such a layer is now vetoed in retireLayer and dryRunRetire, which keeps
the record for the pull the same way a landed pinned layer does. The pin
check for a missing layer reads the map under pinMu; the fresh-layer
fast path still stays off the lock.
Comment on lines +91 to +99
if st, mtime, err := retirable(dir, cutoff); st != retireRetired {
if st == retireGone && s.pinned(hex) {
return "", retireVetoed, nil
}
if st == retireVetoed && err == nil {
slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime))
}
return "", st, err
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – My earlier suggestion left a narrow gap here. If the layer lands and the pull then fails on another layer and unpins, all between the stat and pinned(), this returns gone for a layer that is now on disk with no record. dryRunRetire checks the pin first, so it doesn't have this gap.

Letting a missing layer fall through to renameAsideUnlessPinned closes it, since that checks the pin and then re-stats under pinMu. The veto then logs like a landed pinned layer does, and pinned() goes back to dry-run only, as its comment says. The window is tiny, so this can be a follow-up.

Suggested change
if st, mtime, err := retirable(dir, cutoff); st != retireRetired {
if st == retireGone && s.pinned(hex) {
return "", retireVetoed, nil
}
if st == retireVetoed && err == nil {
slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime))
}
return "", st, err
}
if st, mtime, err := retirable(dir, cutoff); st == retireVetoed {
if err == nil {
slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime))
}
return "", st, err
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I'm choosing to take this as a follow-up, because narrowing the race is desirable to keep CI moving]

@BenTheElder
Benjamin Elder (BenTheElder) added this pull request to the merge queue Oct 19, 2026
Merged via the queue into agent-substrate:main with commit 3629767 Oct 19, 2026
15 checks passed
haiyanmeng pushed a commit to haiyanmeng/substrate that referenced this pull request Oct 20, 2026
…r a failed pull (agent-substrate#2989)

Follow-up to
agent-substrate#1509 (comment).

`retireLayer` stats a layer before checking its pin. If the layer lands,
and its pull then fails and unpins in between, eviction reports the
layer gone and drops the image record, so the unpacked layer has no
record until the image is pulled again. A missing layer now goes through
`renameAsideUnlessPinned`, which checks the pin and stats again under
`pinMu`. `dryRunRetire` already checks the pin first.

There is no new test, since the window can't be hit without a hook
between the two checks. `TestRetireLayerStatuses` still covers the
missing and missing+pinned cases.

AI use: AI-assisted, I reviewed the change.

- [x] Tests pass
@igooch
igooch deleted the fix/ensurelayer-retire-flight-join branch October 27, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage kind/bug Something isn't working / bugfixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestConcurrentEnsureImageAndEvict flakes

5 participants