Skip to content

history: remove ref count entry when last listener exits - #37

Merged
piob-io merged 1 commit into
masterfrom
devin/1790101322-history-listener-refs
Sep 22, 2026
Merged

piob-io merged 1 commit into
masterfrom
devin/1790101322-history-listener-refs

Conversation

@piob-io

@piob-io piob-io commented Sep 22, 2026 •

Copy link
Copy Markdown

Summary

Fixes the history-store ref-count leak that makes build records undeletable for the lifetime of a buildkitd process (same patch as history-release-listener-refs, which is prepared for upstream moby/buildkit).

Listen with a Ref filter bumps h.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 left refs[ref] = 0 behind. delete() treats a present key as "listener still streaming", marks the ref in h.deleted and 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 in history.db, later Listen on it fails with ref … is deleted → record physically removed, DELETED event emitted.
  • History GC (MaxAge/MaxEntries, orphan cleanup) goes through the same guarded delete, so those records were never reclaimed until restart → reclaimed normally.
  • For us: the action's post-build prune "deleted" the record, but it survived in history.db on 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 the DELETED event arrives. Fails on master with ref … is deleted: file does not exist, passes with the fix (./hack/test integration, all oci worker variants); testListenBuildHistoryExcludesSoftDeletedRecords still green.

Follow-up after merge: cut a fork release and bump BUILDKIT_VERSION in 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


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled. (Staging)


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

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>
@devin-ai-integration

Copy link
Copy Markdown

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@piob-io
piob-io merged commit eda7c51 into master Sep 22, 2026
124 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants