dap: filter inputs for a step to prevent overeager evaluation

When the debug thread was updated to always solve inputs from the
operation that it was tied to it became a bit overeager to evaluate
them. The intention of the steps is to have a single direct parent and
then potentially multiple "function calls" that can be evaluated with
step into and step out to leave.

With the change, that logic stayed in, but the inputs were always being
evaluated before they were stepped into or over. Now, when we construct
the steps, we also attach a list of inputs that we should defer
evaluation on to ensure we don't execute inputs that haven't been
executed yet.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>

Author: jsternberg

dap/thread.go

@@ -2,6 +2,7 @@ package dap
 
 import (
 	"context"
+	"maps"
 	"path"
 	"path/filepath"
 	"slices"
@@ -131,6 +132,10 @@ type step struct {
 	// breakpoint resolution.
 	dgst digest.Digest
 
+	// filtered holds the inputs that should be filtered out of the
+	// solved inputs.
+	filtered map[int]bool
+
 	// in holds the next target when step in is used.
 	in *step
 
@@ -225,6 +230,13 @@ func (t *thread) createBranch(dgst digest.Digest, exitpoint *step) (entrypoint *
 			// Create the routine associated with this input.
 			// Associate it with the entrypoint in step.
 			head.in = t.createBranch(digest.Digest(inp.Digest), entrypoint)
+
+			// Filter this input from the target so it doesn't get solved
+			// when moving to this step.
+			head.filtered = make(map[int]bool)
+			maps.Copy(head.filtered, entrypoint.filtered)
+			head.filtered[i] = true
+
 			entrypoint = &head
 		}
 
@@ -236,11 +248,12 @@ func (t *thread) createBranch(dgst digest.Digest, exitpoint *step) (entrypoint *
 
 		// Create a new step that refers to the direct parent.
 		head := &step{
-			dgst:   digest.Digest(op.Inputs[entrypoint.parent].Digest),
-			in:     entrypoint,
-			next:   entrypoint,
-			out:    entrypoint.out,
-			parent: -1,
+			dgst:     digest.Digest(op.Inputs[entrypoint.parent].Digest),
+			filtered: entrypoint.filtered,
+			in:       entrypoint,
+			next:     entrypoint,
+			out:      entrypoint.out,
+			parent:   -1,
 		}
 		head.frame = t.getStackFrame(head.dgst, entrypoint)
 		entrypoint = head
@@ -593,6 +606,10 @@ func (t *thread) solveInputs(ctx context.Context, target *step) (string, map[str
 	var root string
 	refs := make(map[string]gateway.Reference)
 	for i, input := range op.Inputs {
+		if target.filtered[i] {
+			continue
+		}
+
 		k := t.determineInputName(op, i, input)
 		if _, ok := refs[k]; ok || k == "" {
 			continue

tests/dap_build.go

@@ -7,6 +7,7 @@ import (
 	"path"
 	"runtime"
 	"slices"
+	"strings"
 	"syscall"
 	"testing"
 	"time"
@@ -83,6 +84,7 @@ var dapBuildTests = []func(t *testing.T, sb integration.Sandbox){
 	testDapBuildStepNext,
 	testDapBuildStepOut,
 	testDapBuildVariables,
+	testDapBuildDeferredEval,
 }
 
 func testDapBuild(t *testing.T, sb integration.Sandbox) {
@@ -794,6 +796,53 @@ func testDapBuildVariables(t *testing.T, sb integration.Sandbox) {
 	}
 }
 
+func testDapBuildDeferredEval(t *testing.T, sb integration.Sandbox) {
+	dir := createTestProject(t)
+	client, done, err := dapBuildCmd(t, sb)
+	require.NoError(t, err)
+
+	// Track when we see this message.
+	seen := false
+	client.RegisterEvent("output", func(em dap.EventMessage) {
+		e := em.(*dap.OutputEvent)
+		seen = seen || strings.Contains(e.Body.Output, "RUN cp /etc/foo /etc/bar")
+	})
+
+	interruptCh := pollInterruptEvents(client)
+	doLaunch(t, client, commands.LaunchConfig{
+		Dockerfile:  path.Join(dir, "Dockerfile"),
+		ContextPath: dir,
+	},
+		dap.SourceBreakpoint{Line: 7},
+	)
+
+	stopped := waitForInterrupt[*dap.StoppedEvent](t, interruptCh)
+	require.NotNil(t, stopped)
+
+	// The output event is usually immediate but it can sometimes be delayed due to
+	// the multithreading in the printer. Just wait for a little bit.
+	<-time.After(100 * time.Millisecond)
+
+	// We should not have seen this message since the branch this
+	// message comes from should be deferred because we have
+	// not passed the breakpoint.
+	require.False(t, seen, "step has been invoked before intended")
+	doNext(t, client, stopped.Body.ThreadId)
+
+	stopped = waitForInterrupt[*dap.StoppedEvent](t, interruptCh)
+	require.NotNil(t, stopped)
+
+	if !seen {
+		// If we haven't seen the output then wait for a little bit
+		// due to the printer being potentially delayed.
+		<-time.After(100 * time.Millisecond)
+	}
+	require.True(t, seen, "step should have been seen")
+
+	var exitErr *exec.ExitError
+	require.ErrorAs(t, done(true), &exitErr)
+}
+
 func doLaunch(t *testing.T, client *daptest.Client, config commands.LaunchConfig, bps ...dap.SourceBreakpoint) {
 	t.Helper()