Repository navigation
history: remove ref count entry when last listener exits - #37
Merged
Merged
Conversation
Listen with a Ref filter increments h.refs[ref] and decrements it on
exit, but only removed the map entry when the ref had already been
marked deleted. A listener that exits normally therefore left a
zero-valued entry behind for the lifetime of the process.
delete() treats the presence of the key as "a listener is still
streaming this record" and only marks the ref in h.deleted, deferring
the physical removal to the last listener's exit. With no listener
left, that never happens: the record stays in the history database
and its lease is kept, UpdateBuildHistory{Delete} reports success but
is a no-op, a later Listen on the ref fails with "ref is deleted",
and the history GC (MaxAge/MaxEntries and orphan cleanup) cannot
remove the record until buildkitd restarts.
buildx calls ListenBuildHistory with the build's ref after every build
that writes a metadata file (provenance resolution), so any such record
becomes undeletable. On per-job sticky disks the record survives in
history.db across the commit and is re-exported by the next job.
Remove the map entry whenever the count reaches zero, and run the
pending delete if the ref was marked deleted in the meantime, so that
the invariant "key present in refs <=> active listener" holds.
Add an integration test that listens on a ref to completion, deletes
the ref and verifies the record is gone and the DELETED event is
emitted.
Same change as the upstream branch history-release-listener-refs.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
ajwerner
approved these changes
Sep 22, 2026
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
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.
Summary
Fixes the history-store ref-count leak that makes build records undeletable for the lifetime of a
buildkitdprocess (same patch ashistory-release-listener-refs, which is prepared for upstreammoby/buildkit).Listenwith aReffilter bumpsh.refs[ref]; its defer decremented the count but only removed the map key if the ref was already soft-deleted. A normal listen (buildx does one after every build that writes a metadata file — "resolving provenance") therefore leftrefs[ref] = 0behind.delete()treats a present key as "listener still streaming", marks the ref inh.deletedand defers the bbolt delete to the last listener's exit — which never comes:defer func() { h.mu.Lock() h.refs[req.Ref]-- - if _, ok := h.deleted[req.Ref]; ok { - if h.refs[req.Ref] == 0 { - delete(h.refs, req.Ref) + if h.refs[req.Ref] == 0 { + delete(h.refs, req.Ref) + if _, ok := h.deleted[req.Ref]; ok { h.delete(req.Ref) } } h.mu.Unlock() }()Invariant restored: key present in
refs⇔ an active listener.Effects before → after
UpdateBuildHistory{Delete}on such a ref: reported success but no-op; record + lease kept inhistory.db, laterListenon it fails withref … is deleted→ record physically removed,DELETEDevent emitted.MaxAge/MaxEntries, orphan cleanup) goes through the same guardeddelete, so those records were never reclaimed until restart → reclaimed normally.history.dbon the committed sticky disk and was re-exported by the next job under that job's identity (the ~32–47% stale rows that fa#5466 now filters agent-side). After this, the prune actually removes it.Test:
testDeleteBuildHistoryAfterListenerExit(client integration) — listen on the ref to EOF, delete, assert the ref is gone and theDELETEDevent arrives. Fails onmasterwithref … is deleted: file does not exist, passes with the fix (./hack/test integration, all oci worker variants);testListenBuildHistoryExcludesSoftDeletedRecordsstill green.Follow-up after merge: cut a fork release and bump
BUILDKIT_VERSIONin fa.Link to Devin session: https://app.devin.ai/sessions/0c6ebc2a6de54933bebe821e21ab8b76
Open in Devin Desktop: https://app.devin.ai/desktop/session/0c6ebc2a6de54933bebe821e21ab8b76?variant=devin
Requested by: @piob-io
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled. (Staging)Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.