Refactor to file action indexed outputs

Signed-off-by: Edgar Lee <edgarl@netflix.com>

client/build_test.go

@@ -1279,8 +1279,8 @@ func testClientGatewayFileActionError(t *testing.T, sb integration.Sandbox) {
 		require.True(t, errors.As(solveErr, &se))
 		// There are no inputs because rootfs is scratch.
 		require.Len(t, se.Solve.InputIDs, 0)
-		// There is one output for the mutable mount used for the fileop actions.
+		// There is one output for every action (3).
-		require.Len(t, se.Solve.OutputIDs, 1)
+		require.Len(t, se.Solve.OutputIDs, 3)
 
 		op, ok := se.Solve.Op.Op.(*pb.Op_File)
 		require.True(t, ok)
@@ -1289,10 +1289,10 @@ func testClientGatewayFileActionError(t *testing.T, sb integration.Sandbox) {
 		require.True(t, ok)
 
 		idx := subject.File.Index
-		require.Greater(t, len(op.File.Actions), int(idx))
+		require.Less(t, int(idx), len(op.File.Actions))
 
 		// Verify the index is pointing to the action that failed.
-		action := op.File.Actions[subject.File.Index]
+		action := op.File.Actions[idx]
 		mkfile, ok := action.Action.(*pb.FileAction_Mkfile)
 		require.True(t, ok)
 		require.Equal(t, mkfile.Mkfile.Path, "/notfound/foo")
@@ -1317,7 +1317,7 @@ func testClientGatewayFileActionError(t *testing.T, sb integration.Sandbox) {
 			}, {
 				Dest:      "/problem",
 				MountType: pb.MountType_BIND,
-				ResultID:  se.Solve.OutputIDs[0],
+				ResultID:  se.Solve.OutputIDs[idx],
 			}},
 		})
 		require.NoError(t, err)

frontend/gateway/forwarder/forward.go

@@ -158,6 +158,9 @@ func (c *bridgeClient) registerResultIDs(results ...solver.Result) (ids []string
 
 	ids = make([]string, len(results))
 	for i, res := range results {
+		if res == nil {
+			continue
+		}
 		workerRef, ok := res.Sys().(*worker.WorkerRef)
 		if !ok {
 			return ids, errors.Errorf("unexpected type for result, got %T", res.Sys())

frontend/gateway/gateway.go

@@ -533,6 +533,9 @@ func (lbf *llbBridgeForwarder) registerResultIDs(results ...solver.Result) (ids
 
 	ids = make([]string, len(results))
 	for i, res := range results {
+		if res == nil {
+			continue
+		}
 		workerRef, ok := res.Sys().(*worker.WorkerRef)
 		if !ok {
 			return ids, errors.Errorf("unexpected type for result, got %T", res.Sys())

solver/llbsolver/file/refmanager.go

@@ -29,7 +29,7 @@ func (rm *RefManager) Prepare(ctx context.Context, ref fileoptypes.Ref, readonly
 		if err != nil {
 			return nil, err
 		}
-		return &Mount{m: m}, nil
+		return &Mount{m: m, readonly: readonly}, nil
 	}
 
 	mr, err := rm.cm.New(ctx, ir, g, cache.WithDescription("fileop target"), cache.CachePolicyRetain)
@@ -40,7 +40,7 @@ func (rm *RefManager) Prepare(ctx context.Context, ref fileoptypes.Ref, readonly
 	if err != nil {
 		return nil, err
 	}
-	return &Mount{m: m, mr: mr}, nil
+	return &Mount{m: m, mr: mr, readonly: readonly}, nil
 }
 
 func (rm *RefManager) Commit(ctx context.Context, mount fileoptypes.Mount) (fileoptypes.Ref, error) {
@@ -58,8 +58,9 @@ func (rm *RefManager) Commit(ctx context.Context, mount fileoptypes.Mount) (file
 }
 
 type Mount struct {
-	m  snapshot.Mountable
+	m        snapshot.Mountable
-	mr cache.MutableRef
+	mr       cache.MutableRef
+	readonly bool
 }
 
 func (m *Mount) Release(ctx context.Context) error {
@@ -69,3 +70,7 @@ func (m *Mount) Release(ctx context.Context) error {
 	return nil
 }
 func (m *Mount) IsFileOpMount() {}
+
+func (m *Mount) Readonly() bool {
+	return m.readonly
+}

solver/llbsolver/ops/file.go

@@ -160,7 +160,6 @@ func (f *fileOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
 	outs, err := f.solver.Solve(ctx, inpRefs, f.op.Actions, g)
 	if err != nil {
 		return nil, err
-		// return nil, errdefs.WithExecError(err, inputs, nil)
 	}
 
 	outResults := make([]solver.Result, 0, len(outs))
@@ -347,7 +346,7 @@ func (s *FileOpSolver) Solve(ctx context.Context, inputs []fileoptypes.Ref, acti
 				}
 				inp, err := s.getInput(ctx, idx, inputs, actions, g)
 				if err != nil {
-					return errdefs.WithFileActionError(err, idx)
+					return errdefs.WithFileActionError(err, idx-len(inputs))
 				}
 				outs[i] = inp.ref
 				return nil
@@ -405,32 +404,42 @@ func (s *FileOpSolver) getInput(ctx context.Context, idx int, inputs []fileoptyp
 
 		var inpMount, inpMountSecondary fileoptypes.Mount
 		var toRelease []fileoptypes.Mount
-		var inpMountPrepared bool
+		action := actions[idx-len(inputs)]
+
 		defer func() {
+			if err != nil && inpMount != nil {
+				inputRes := make([]solver.Result, len(inputs))
+				for i, input := range inputs {
+					inputRes[i] = worker.NewWorkerRefResult(input.(cache.ImmutableRef), s.w)
+				}
+
+				outputRes := make([]solver.Result, len(actions))
+
+				// Commit the mutable for the primary input of the failed action.
+				if !inpMount.Readonly() {
+					ref, cerr := s.r.Commit(ctx, inpMount)
+					if cerr == nil {
+						outputRes[idx-len(inputs)] = worker.NewWorkerRefResult(ref.(cache.ImmutableRef), s.w)
+					}
+					inpMount.Release(context.TODO())
+				}
+
+				// If the action has a secondary input, commit it and set the ref on
+				// the output results.
+				if inpMountSecondary != nil && !inpMountSecondary.Readonly() {
+					ref2, cerr := s.r.Commit(ctx, inpMountSecondary)
+					if cerr == nil {
+						outputRes[int(action.SecondaryInput)-len(inputs)] = worker.NewWorkerRefResult(ref2.(cache.ImmutableRef), s.w)
+					}
+				}
+
+				err = errdefs.WithExecError(err, inputRes, outputRes)
+			}
 			for _, m := range toRelease {
 				m.Release(context.TODO())
 			}
-			if err != nil && inpMount != nil {
-				if inpMountPrepared {
-					inpMount.Release(context.TODO())
-				}
-				var (
-					inputRes  []solver.Result
-					outputRes []solver.Result
-				)
-				for _, input := range inputs {
-					inputRes = append(inputRes, worker.NewWorkerRefResult(input.(cache.ImmutableRef), s.w))
-				}
-				ref, cerr := s.r.Commit(ctx, inpMount)
-				if cerr == nil {
-					outputRes = append(outputRes, worker.NewWorkerRefResult(ref.(cache.ImmutableRef), s.w))
-				}
-				err = errdefs.WithExecError(err, inputRes, outputRes)
-			}
 		}()
 
-		action := actions[idx-len(inputs)]
-
 		loadInput := func(ctx context.Context) func() error {
 			return func() error {
 				inp, err := s.getInput(ctx, int(action.Input), inputs, actions, g)
@@ -443,7 +452,6 @@ func (s *FileOpSolver) getInput(ctx context.Context, idx int, inputs []fileoptyp
 						return err
 					}
 					inpMount = m
-					inpMountPrepared = true
 					return nil
 				}
 				inpMount = inp.mount
@@ -542,7 +550,6 @@ func (s *FileOpSolver) getInput(ctx context.Context, idx int, inputs []fileoptyp
 				return nil, err
 			}
 			inpMount = m
-			inpMountPrepared = true
 		}
 
 		switch a := action.Action.(type) {

solver/llbsolver/ops/file_test.go

@@ -581,6 +581,7 @@ type testMount struct {
 	callback  func()
 	unmounted bool
 	active    *testFileRef
+	readonly  bool
 }
 
 func (tm *testMount) addUser(user, group fileoptypes.Mount) {
@@ -617,6 +618,10 @@ func (tm *testMount) Release(ctx context.Context) error {
 	return nil
 }
 
+func (tm *testMount) Readonly() bool {
+	return tm.readonly
+}
+
 type testFileBackend struct {
 }
 
@@ -674,7 +679,7 @@ func (b *testFileRefBackend) Prepare(ctx context.Context, ref fileoptypes.Ref, r
 	rr := ref.(*testFileRef)
 	m := rr.mount
 	if m == nil {
-		m = &testMount{b: b, id: "mount-" + rr.id, callback: rr.callback}
+		m = &testMount{b: b, id: "mount-" + rr.id, callback: rr.callback, readonly: readonly}
 	}
 	m.initID = m.id
 	m.active = active

solver/llbsolver/ops/fileoptypes/types.go

@@ -14,6 +14,7 @@ type Ref interface {
 type Mount interface {
 	IsFileOpMount()
 	Release(context.Context) error
+	Readonly() bool
 }
 
 type Backend interface {

solver/llbsolver/solver.go

@@ -144,6 +144,7 @@ func (s *Solver) Solve(ctx context.Context, id string, sessionID string, req fro
 		eg.Go(func() error {
 			_, err := ref.Result(ctx2)
 			if err != nil {
+				// Also release any results referenced by exec errors.
 				var ee *errdefs.ExecError
 				if errors.As(err, &ee) {
 					ee.EachRef(func(res solver.Result) error {