Skip to content

Commit

Permalink
cache: release parents if load fails
Browse files Browse the repository at this point in the history
Before this, if a ref with multiple parents loaded some parents but then
failed, the already loaded parents would not be released and thus
leaked.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>
  • Loading branch information
sipsma committed Apr 13, 2022
1 parent a493fab commit 182d9df
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 0 deletions.
5 changes: 5 additions & 0 deletions cache/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,11 @@ func (cm *cacheManager) getRecord(ctx context.Context, id string, opts ...RefOpt
}

func (cm *cacheManager) parentsOf(ctx context.Context, md *cacheMetadata, opts ...RefOption) (ps parentRefs, rerr error) {
defer func() {
if rerr != nil {
ps.release(context.TODO())
}
}()
if parentID := md.getParent(); parentID != "" {
p, err := cm.get(ctx, parentID, nil, append(opts, NoUpdateLastUsed))
if err != nil {
Expand Down
58 changes: 58 additions & 0 deletions cache/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,64 @@ func TestMountReadOnly(t *testing.T) {
}
}

func TestLoadBrokenParents(t *testing.T) {
// Test that a ref that has a parent that can't be loaded will not result in any leaks
// of other parent refs
t.Parallel()

ctx := namespaces.WithNamespace(context.Background(), "buildkit-test")

tmpdir, err := os.MkdirTemp("", "cachemanager")
require.NoError(t, err)
defer os.RemoveAll(tmpdir)

snapshotter, err := native.NewSnapshotter(filepath.Join(tmpdir, "snapshots"))
require.NoError(t, err)

co, cleanup, err := newCacheManager(ctx, cmOpt{
tmpdir: tmpdir,
snapshotter: snapshotter,
snapshotterName: "native",
})
require.NoError(t, err)
defer cleanup()
cm := co.manager.(*cacheManager)

mutRef, err := cm.New(ctx, nil, nil)
require.NoError(t, err)
refA, err := mutRef.Commit(ctx)
require.NoError(t, err)
refAID := refA.ID()
mutRef, err = cm.New(ctx, nil, nil)
require.NoError(t, err)
refB, err := mutRef.Commit(ctx)
require.NoError(t, err)

_, err = cm.Merge(ctx, []ImmutableRef{refA, refB}, nil)
require.NoError(t, err)
checkDiskUsage(ctx, t, cm, 3, 0)

// set refB as deleted
require.NoError(t, refB.(*immutableRef).queueDeleted())
require.NoError(t, refB.(*immutableRef).commitMetadata())
require.NoError(t, cm.Close())
require.NoError(t, cleanup())

co, cleanup, err = newCacheManager(ctx, cmOpt{
tmpdir: tmpdir,
snapshotter: snapshotter,
snapshotterName: "native",
})
require.NoError(t, err)
defer cleanup()
cm = co.manager.(*cacheManager)

checkDiskUsage(ctx, t, cm, 0, 1)
refA, err = cm.Get(ctx, refAID, nil)
require.NoError(t, err)
require.Len(t, refA.(*immutableRef).refs, 1)
}

func checkDiskUsage(ctx context.Context, t *testing.T, cm Manager, inuse, unused int) {
du, err := cm.DiskUsage(ctx, client.DiskUsageInfo{})
require.NoError(t, err)
Expand Down

0 comments on commit 182d9df

Please sign in to comment.