From 8d707775375bf7d3ce182d8b6ed51f1e9888204c Mon Sep 17 00:00:00 2001
From: Tonis Tiigi <tonistiigi@gmail.com>
Date: Wed, 3 Feb 2021 21:59:25 -0800
Subject: [PATCH] fileop: fix checksum to contain indexes of inputs

Cache mismatch can happen if fileop switches input indexes
between different actions.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
---
 client/client_test.go        | 30 ++++++++++++++++++++++++
 solver/llbsolver/ops/file.go | 45 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/client/client_test.go b/client/client_test.go
index 3b78a679224c..e7807d08a9bd 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -122,6 +122,7 @@ func TestIntegration(t *testing.T) {
 		testSourceMapFromRef,
 		testLazyImagePush,
 		testStargzLazyPull,
+		testFileOpInputSwap,
 	}, mirrors)
 
 	integration.Run(t, []integration.Test{
@@ -1128,7 +1129,36 @@ func testFileOpCopyRm(t *testing.T, sb integration.Sandbox) {
 	dt, err = ioutil.ReadFile(filepath.Join(destDir, "file2"))
 	require.NoError(t, err)
 	require.Equal(t, []byte("file2"), dt)
+}
+
+// testFileOpInputSwap is a regression test that cache is invalidated when subset of fileop is built
+func testFileOpInputSwap(t *testing.T, sb integration.Sandbox) {
+	requiresLinux(t)
+	c, err := New(context.TODO(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
 
+	base := llb.Scratch().File(llb.Mkfile("/foo", 0600, []byte("foo")))
+
+	src := llb.Scratch().File(llb.Mkfile("/bar", 0600, []byte("bar")))
+
+	st := base.File(llb.Copy(src, "/bar", "/baz"))
+
+	def, err := st.Marshal(context.TODO())
+	require.NoError(t, err)
+
+	_, err = c.Solve(context.TODO(), def, SolveOpt{}, nil)
+	require.NoError(t, err)
+
+	// bar does not exist in base but index of all inputs remains the same
+	st = base.File(llb.Copy(base, "/bar", "/baz"))
+
+	def, err = st.Marshal(context.TODO())
+	require.NoError(t, err)
+
+	_, err = c.Solve(context.TODO(), def, SolveOpt{}, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "bar: no such file")
 }
 
 func testFileOpRmWildcard(t *testing.T, sb integration.Sandbox) {
diff --git a/solver/llbsolver/ops/file.go b/solver/llbsolver/ops/file.go
index 1111cdb72e84..548bae1928e6 100644
--- a/solver/llbsolver/ops/file.go
+++ b/solver/llbsolver/ops/file.go
@@ -61,6 +61,8 @@ func (f *fileOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 		}
 	}
 
+	indexes := make([][]int, 0, len(f.op.Actions))
+
 	for _, action := range f.op.Actions {
 		var dt []byte
 		var err error
@@ -103,14 +105,21 @@ func (f *fileOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 		}
 
 		actions = append(actions, dt)
+		indexes = append(indexes, []int{int(action.Input), int(action.SecondaryInput), int(action.Output)})
+	}
+
+	if isDefaultIndexes(indexes) {
+		indexes = nil
 	}
 
 	dt, err := json.Marshal(struct {
 		Type    string
 		Actions [][]byte
+		Indexes [][]int `json:"indexes,omitempty"`
 	}{
 		Type:    fileCacheType,
 		Actions: actions,
+		Indexes: indexes,
 	})
 	if err != nil {
 		return nil, false, err
@@ -611,3 +620,39 @@ func (s *FileOpSolver) getInput(ctx context.Context, idx int, inputs []fileoptyp
 	}
 	return inp.(input), err
 }
+
+func isDefaultIndexes(idxs [][]int) bool {
+	// Older version of checksum did not contain indexes for actions resulting in possibility for a wrong cache match.
+	// We detect the most common pattern for indexes and maintain old checksum for that case to minimize cache misses on upgrade.
+	// If a future change causes braking changes in instruction cache consider removing this exception.
+	if len(idxs) == 0 {
+		return false
+	}
+
+	for i, idx := range idxs {
+		if len(idx) != 3 {
+			return false
+		}
+		// input for first action is first input
+		if i == 0 && idx[0] != 0 {
+			return false
+		}
+		// input for other actions is previous action
+		if i != 0 && idx[0] != len(idxs)+(i-1) {
+			return false
+		}
+		// secondary input is second input or -1
+		if idx[1] != -1 && idx[1] != 1 {
+			return false
+		}
+		// last action creates output
+		if i == len(idxs)-1 && idx[2] != 0 {
+			return false
+		}
+		// other actions do not create an output
+		if i != len(idxs)-1 && idx[2] != -1 {
+			return false
+		}
+	}
+	return true
+}