From 0e46e235f092f979646f4a2966c894a24f2bb419 Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Mon, 16 Mar 2026 14:41:49 -0500 Subject: [PATCH] dap: defer 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. It will then wrap the reference with a version that causes `Evaluate` to do nothing. This prevents the overeager evaluation but allows the reference to be evaluated if we need to read the filesystem. Signed-off-by: Jonathan A. Sternberg --- dap/thread.go | 44 ++++++++++++++++++++++++++++++----- tests/dap_build.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/dap/thread.go b/dap/thread.go index 03dd5abd8331..db35dd67efb9 100644 --- a/dap/thread.go +++ b/dap/thread.go @@ -2,6 +2,7 @@ package dap import ( "context" + "maps" "path/filepath" "slices" "strings" @@ -138,6 +139,11 @@ type step struct { // breakpoint resolution. dgst digest.Digest + // deferred holds the inputs that should have its evaluation deferred. + // These inputs are still included in the references but will only be + // evaluated when needed. + deferred map[int]bool + // in holds the next target when step in is used. in *step @@ -241,10 +247,21 @@ func (t *thread) createBranch(dgst digest.Digest, exitpoint *step) (entrypoint * // If this branch is empty (signified by a nil return value) then // skip it. if head.in == nil { + // Always mark this input as deferred since it doesn't have + // an associated branch. + if entrypoint.deferred == nil { + entrypoint.deferred = make(map[int]bool) + } + entrypoint.deferred[i] = true continue } - entrypoint.dgst = "" + + // Filter this input from the target so it doesn't get solved + // when moving to this step. + head.deferred = make(map[int]bool) + maps.Copy(head.deferred, entrypoint.deferred) + head.deferred[i] = true entrypoint = &head } @@ -256,11 +273,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), + deferred: entrypoint.deferred, + in: entrypoint, + next: entrypoint, + out: entrypoint.out, + parent: -1, } head.frame = t.getStackFrame(head.dgst, entrypoint) entrypoint = head @@ -628,6 +646,12 @@ func (t *thread) solveInputs(ctx context.Context, target *step) (string, map[str if err != nil { return "", nil, err } + + // If we have marked this input to be deferred, wrap it in a reference + // that suppresses the evaluate call. + if target.deferred[i] { + ref = &deferredReference{Reference: ref} + } refs[k] = ref } return root, refs, nil @@ -823,3 +847,11 @@ func (r *mountReference) ReadDir(ctx context.Context, req gateway.ReadDirRequest MountIndex: r.index, }) } + +type deferredReference struct { + gateway.Reference +} + +func (r *deferredReference) Evaluate(ctx context.Context) error { + return nil +} diff --git a/tests/dap_build.go b/tests/dap_build.go index d4bd6939dd58..25ed26f3d5cd 100644 --- a/tests/dap_build.go +++ b/tests/dap_build.go @@ -7,6 +7,7 @@ import ( "path" "runtime" "slices" + "strings" "syscall" "testing" "time" @@ -85,6 +86,7 @@ var dapBuildTests = []func(t *testing.T, sb integration.Sandbox){ testDapBuildStepNext, testDapBuildStepOut, testDapBuildVariables, + testDapBuildDeferredEval, } func testDapBuild(t *testing.T, sb integration.Sandbox) { @@ -857,6 +859,61 @@ 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 := make(chan struct{}, 1) + client.RegisterEvent("output", func(em dap.EventMessage) { + e := em.(*dap.OutputEvent) + if strings.Contains(e.Body.Output, "RUN cp /etc/foo /etc/bar") { + select { + case seen <- struct{}{}: + default: + } + } + }) + + 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. + select { + case <-seen: + // We should not have seen this message since the branch this + // message comes from should be deferred because we have + // not passed the breakpoint. + t.Fatal("step has been invoked before intended") + case <-time.After(100 * time.Millisecond): + } + + doNext(t, client, stopped.Body.ThreadId) + + stopped = waitForInterrupt[*dap.StoppedEvent](t, interruptCh) + require.NotNil(t, stopped) + + select { + case <-seen: + // Wait up to 5 seconds for the input to be seen. + case <-time.After(time.Second): + t.Fatal("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()