From df10cbfc951b5b0f4c5f28403d9f411be3eecb3a Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 27 Oct 2026 21:27:47 -0700 Subject: [PATCH] cache: avoid losing finalize commit on cancel On the containerd worker the snapshotter is a gRPC client. If a build is cancelled while finalize is committing a lazy ref, containerd can apply the commit while the client returns Canceled. finalize then deleted the lease protecting the committed snapshot and kept equalMutable, so the record pointed at a mutable snapshot that no longer existed. Every later build using the record failed with "failed to stat active key during commit: snapshot ... does not exist" until the daemon restarted. Run finalize without cancellation so the commit result is not lost. If Commit still returns an error, check whether the target snapshot was committed and, if so, complete the finalize. Keep the lease when that cannot be determined. Signed-off-by: Tonis Tiigi --- cache/manager_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++ cache/refs.go | 26 ++++++++--- 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/cache/manager_test.go b/cache/manager_test.go index 16b9b44f7..629a4edf1 100644 --- a/cache/manager_test.go +++ b/cache/manager_test.go @@ -3015,6 +3015,108 @@ func TestLoadHalfFinalizedRef(t *testing.T) { require.NoError(t, err) } +func TestFinalizeAlreadyCommitted(t *testing.T) { + // This test simulates a snapshotter commit that was applied but reported + // as failed (e.g. a canceled containerd RPC). Finalize must notice the + // committed snapshot instead of failing on the missing active snapshot. + t.Parallel() + + ctx := namespaces.WithNamespace(t.Context(), "buildkit-test") + + tmpdir := t.TempDir() + + snapshotter, err := native.NewSnapshotter(filepath.Join(tmpdir, "snapshots")) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, snapshotter.Close()) + }) + + co, cleanup, err := newCacheManager(ctx, t, cmOpt{ + tmpdir: tmpdir, + snapshotter: snapshotter, + snapshotterName: "native", + }) + require.NoError(t, err) + t.Cleanup(cleanup) + cm := co.manager.(*cacheManager) + + mref, err := cm.New(ctx, nil, nil, CachePolicyRetain) + require.NoError(t, err) + mutRef := mref.(*mutableRef) + + iref, err := mutRef.Commit(ctx) + require.NoError(t, err) + immutRef := iref.(*immutableRef) + + err = cm.Snapshotter.Commit(ctx, immutRef.getSnapshotID(), mutRef.getSnapshotID()) + require.NoError(t, err) + + require.NoError(t, iref.Finalize(ctx)) + require.Nil(t, immutRef.equalMutable) + + // the committed snapshot must stay protected by the ref's lease + resources, err := co.lm.ListResources(ctx, leases.Lease{ID: immutRef.ID()}) + require.NoError(t, err) + require.Contains(t, resources, leases.Resource{ + ID: immutRef.getSnapshotID(), + Type: "snapshots/" + cm.Snapshotter.Name(), + }) + + m, err := iref.Mount(ctx, true, nil) + require.NoError(t, err) + mnts, release, err := m.Mount() + require.NoError(t, err) + require.NotEmpty(t, mnts) + require.NoError(t, release()) + + require.NoError(t, mref.Release(ctx)) + require.NoError(t, iref.Release(ctx)) +} + +func TestFinalizeCanceledContext(t *testing.T) { + // Finalize ignores cancellation, as a remote snapshotter may apply a commit + // that the caller observes as canceled. + t.Parallel() + + ctx := namespaces.WithNamespace(t.Context(), "buildkit-test") + + tmpdir := t.TempDir() + + snapshotter, err := native.NewSnapshotter(filepath.Join(tmpdir, "snapshots")) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, snapshotter.Close()) + }) + + co, cleanup, err := newCacheManager(ctx, t, cmOpt{ + tmpdir: tmpdir, + snapshotter: snapshotter, + snapshotterName: "native", + }) + require.NoError(t, err) + t.Cleanup(cleanup) + cm := co.manager.(*cacheManager) + + mref, err := cm.New(ctx, nil, nil, CachePolicyRetain) + require.NoError(t, err) + + iref, err := mref.Commit(ctx) + require.NoError(t, err) + immutRef := iref.(*immutableRef) + + cctx, cancel := context.WithCancelCause(ctx) + cancel(errors.WithStack(context.Canceled)) + require.NoError(t, iref.Finalize(cctx)) + require.Nil(t, immutRef.equalMutable) + + info, err := cm.Snapshotter.Stat(ctx, immutRef.getSnapshotID()) + require.NoError(t, err) + require.Equal(t, snapshots.KindCommitted, info.Kind) + + require.NoError(t, mref.Release(ctx)) + require.NoError(t, iref.Release(ctx)) +} + func TestMountReadOnly(t *testing.T) { t.Parallel() if runtime.GOOS != "linux" { diff --git a/cache/refs.go b/cache/refs.go index 804bf1415..e6e6d4ef7 100644 --- a/cache/refs.go +++ b/cache/refs.go @@ -1538,6 +1538,11 @@ func (cr *cacheRecord) finalize(ctx context.Context) error { return nil } + // The snapshotter may apply the commit even when the caller sees a + // cancellation (e.g. a containerd RPC canceled in flight), leaving the + // mutable snapshot gone while the record still points to it. + ctx = context.WithoutCancel(ctx) + _, err := cr.cm.LeaseManager.Create(ctx, func(l *leases.Lease) error { l.ID = cr.ID() l.Labels = map[string]string{ @@ -1555,13 +1560,23 @@ func (cr *cacheRecord) finalize(ctx context.Context) error { ID: cr.getSnapshotID(), Type: "snapshots/" + cr.cm.Snapshotter.Name(), }); err != nil { - _ = cr.cm.LeaseManager.Delete(context.WithoutCancel(ctx), leases.Lease{ID: cr.ID()}) + _ = cr.cm.LeaseManager.Delete(ctx, leases.Lease{ID: cr.ID()}) return errors.Wrapf(err, "failed to add snapshot %s to lease", cr.getSnapshotID()) } if err := cr.cm.Snapshotter.Commit(ctx, cr.getSnapshotID(), mutable.getSnapshotID()); err != nil { - _ = cr.cm.LeaseManager.Delete(context.WithoutCancel(ctx), leases.Lease{ID: cr.ID()}) - return errors.Wrapf(err, "failed to commit %s to %s during finalize", mutable.getSnapshotID(), cr.getSnapshotID()) + // the commit may still have been applied, e.g. if the response was lost + info, statErr := cr.cm.Snapshotter.Stat(ctx, cr.getSnapshotID()) + if statErr != nil || info.Kind != snapshots.KindCommitted { + // keep the lease if it is unknown whether the commit was applied + if statErr == nil || errors.Is(statErr, cerrdefs.ErrNotFound) { + _ = cr.cm.LeaseManager.Delete(ctx, leases.Lease{ID: cr.ID()}) + } else { + bklog.G(ctx).WithError(statErr).Warnf("failed to check snapshot %s after commit error, keeping lease", cr.getSnapshotID()) + } + return errors.Wrapf(err, "failed to commit %s to %s during finalize", mutable.getSnapshotID(), cr.getSnapshotID()) + } + bklog.G(ctx).WithError(err).Warnf("snapshot %s was committed to %s despite commit error", mutable.getSnapshotID(), cr.getSnapshotID()) } cr.mountCache = nil @@ -1569,9 +1584,8 @@ func (cr *cacheRecord) finalize(ctx context.Context) error { go func() { cr.cm.mu.Lock() defer cr.cm.mu.Unlock() - cleanupCtx := context.WithoutCancel(ctx) - if err := mutable.remove(cleanupCtx, true); err != nil { - bklog.G(cleanupCtx).Error(err) + if err := mutable.remove(ctx, true); err != nil { + bklog.G(ctx).Error(err) } }()