Merge pull request #391 from dokku/328-bug-probe-swallows-local-exec-errors-and-reports-resources-as-absent

fix: stop probe from swallowing local exec errors

Author: josegonzalez

commands/play_executor_test.go

@@ -133,18 +133,15 @@ func TestMultiPlayPlayFilterComposesWithTags(t *testing.T) {
   tasks:
     - name: deploy-api
       tags: [deploy]
-      dokku_app:
-        app: docket-test-deploy-api
+      dokku_stub: { key: deploy-api }
     - name: configure-api
       tags: [configure]
-      dokku_app:
-        app: docket-test-configure-api
+      dokku_stub: { key: configure-api }
 - name: worker
   tasks:
     - name: deploy-worker
       tags: [deploy]
-      dokku_app:
-        app: docket-test-deploy-worker
+      dokku_stub: { key: deploy-worker }
 `)
 	stdout, _, _ := runPlan(t, path, "--play", "api", "--tags", "deploy")
 	if !strings.Contains(stdout, "deploy-api") {
@@ -168,14 +165,12 @@ func TestMultiPlayPlayLevelTagsPropagateToTasks(t *testing.T) {
   tags: [api]
   tasks:
     - name: deploy-api
-      dokku_app:
-        app: docket-test-deploy-api
+      dokku_stub: { key: deploy-api }
 - name: worker
   tags: [worker]
   tasks:
     - name: deploy-worker
-      dokku_app:
-        app: docket-test-deploy-worker
+      dokku_stub: { key: deploy-worker }
 `)
 	stdout, _, _ := runPlan(t, path, "--tags", "api")
 	if !strings.Contains(stdout, "deploy-api") {
@@ -238,6 +233,41 @@ func TestPlanFailedWhenClearsError(t *testing.T) {
 	}
 }
 
+// TestPlanProbeErrorRendersMarkerAndExits pins the documented contract
+// that an uncleared probe error renders [!] on stderr and makes plan
+// exit 1, and that errors win over drift under --detailed-exitcode (exit
+// 1, not 2). This is the end-to-end guard for #328: a probe that could
+// not run must not be reported as absent with an optimistic [+] create.
+func TestPlanProbeErrorRendersMarkerAndExits(t *testing.T) {
+	defer stubReset()
+	stubSet("a", StubFixture{
+		PlanError: errors.New(`exec: "dokku": executable file not found in $PATH`),
+	})
+
+	path := writeMultiPlayTasks(t, `---
+- tasks:
+    - name: needs-dokku
+      dokku_stub: { key: a }
+`)
+
+	stdout, stderr, exit := runPlan(t, path)
+	if exit != 1 {
+		t.Fatalf("exit = %d, want 1; stdout=%s stderr=%s", exit, stdout, stderr)
+	}
+	if !strings.Contains(stderr, "[!]") {
+		t.Errorf("expected [!] marker on stderr; got stdout=%s stderr=%s", stdout, stderr)
+	}
+	if strings.Contains(stdout, "[+]") {
+		t.Errorf("plan must not predict [+] create for state it never read; got:\n%s", stdout)
+	}
+
+	// --detailed-exitcode: errors win over drift, so exit stays 1, not 2.
+	_, _, exit = runPlan(t, path, "--detailed-exitcode")
+	if exit != 1 {
+		t.Errorf("detailed-exitcode exit = %d, want 1 (errors win over drift)", exit)
+	}
+}
+
 // TestPlanChangedWhenTrueRecomputesStatus pins #313: changed_when: 'true'
 // on an in-sync task must flip the marker and the JSON status to a
 // would-change verdict, not leave the stale [ok] / "status":"ok" the

docs/command-reference.md

@@ -149,6 +149,10 @@ decide whether to mutate. A few tasks (notably `dokku_git_auth`, `dokku_registry
 `dokku_storage_ensure`) cannot read their state without running the underlying command, so they
 always report drift with a `(... not probed)` reason.
 
+When the probe command cannot run at all - for example the local `dokku` CLI is not installed, or
+the configured SSH host is unreachable - the task renders `[!]` and `plan` exits `1`, rather than
+optimistically predicting `[+] create` for state it never actually read.
+
 | Flag | Effect |
 |------|--------|
 | `--tasks <path>` | Use a specific recipe. Accepts a local path or an `http(s)://` URL (fetched over HTTP). |

subprocess/exec.go

@@ -79,6 +79,15 @@ type ExecCommandInput struct {
 type ExecError struct {
 	Response ExecCommandResponse
 	Err      error
+
+	// Ran is true only when the command executed to completion and
+	// Response.ExitCode is its real exit status. It is false when the
+	// command could not be started (binary not found, permission denied)
+	// or was cancelled, in which cases Response.ExitCode is not
+	// meaningful. Probe() uses this to tell a dokku-level "state absent"
+	// (Ran, non-zero exit) apart from a real execution failure that must
+	// be propagated.
+	Ran bool
 }
 
 // Error returns the wrapped error's message so existing string-based
@@ -312,6 +321,10 @@ func defaultExecRunner(ctx context.Context, input ExecCommandInput) (ExecCommand
 
 	res, err := cmd.Execute(ctx)
 	if err != nil {
+		// The command could not be run to completion: the binary was not
+		// found, was not executable, or the context was cancelled. The
+		// exit code is not meaningful, so Ran stays false and callers such
+		// as Probe surface the failure instead of reading it as absence.
 		response := ExecCommandResponse{
 			Command:   resolved,
 			Stdout:    res.Stdout,
@@ -330,7 +343,7 @@ func defaultExecRunner(ctx context.Context, input ExecCommandInput) (ExecCommand
 			ExitCode:  res.ExitCode,
 			Cancelled: res.Cancelled,
 		}
-		return response, &ExecError{Response: response, Err: errors.New(res.Stderr)}
+		return response, &ExecError{Response: response, Err: errors.New(res.Stderr), Ran: true}
 	}
 
 	return ExecCommandResponse{

subprocess/exec_test.go

@@ -3,6 +3,7 @@ package subprocess
 import (
 	"bytes"
 	"context"
+	"errors"
 	"log"
 	"strings"
 	"testing"
@@ -170,6 +171,15 @@ func TestCallExecCommandFailure(t *testing.T) {
 	if resp.ExitCode == 0 {
 		t.Error("expected non-zero exit code")
 	}
+	// The command ran and exited non-zero, so the exit code is real:
+	// ExecError.Ran must be true so Probe reads it as "state absent".
+	var execErr *ExecError
+	if !errors.As(err, &execErr) {
+		t.Fatalf("expected *ExecError, got %T", err)
+	}
+	if !execErr.Ran {
+		t.Error("ExecError.Ran should be true when the command ran and exited non-zero")
+	}
 }
 
 func TestCallExecCommandNotFound(t *testing.T) {
@@ -179,6 +189,16 @@ func TestCallExecCommandNotFound(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected error for nonexistent command")
 	}
+	// The command could not be started, so there is no real exit code:
+	// ExecError.Ran must be false so Probe propagates the failure instead
+	// of reporting the probed state as absent.
+	var execErr *ExecError
+	if !errors.As(err, &execErr) {
+		t.Fatalf("expected *ExecError, got %T", err)
+	}
+	if execErr.Ran {
+		t.Error("ExecError.Ran should be false when the binary is not found")
+	}
 }
 
 func TestCallExecCommandWithEnv(t *testing.T) {

subprocess/ssh.go

@@ -217,12 +217,18 @@ var (
 )
 
 // Probe runs input as a state probe and reports whether it matched
-// (exit 0). A dokku-level non-zero exit is reported as `(false, nil)`,
-// i.e. "the probed state is absent," so callers can write idempotent
-// probes without unwrapping errors themselves. A transport-level
-// failure (`*SSHError`) is propagated as `(false, err)` so the caller
-// can short-circuit `Plan()` with `PlanResult{Error: err}` and let the
-// formatter render `! ssh: ...`.
+// (exit 0). A non-zero exit from a command that actually ran is reported
+// as `(false, nil)`, i.e. "the probed state is absent," so callers can
+// write idempotent probes without unwrapping errors themselves.
+//
+// Any failure that means the probe never produced a real answer is
+// propagated as `(false, err)` so the caller can short-circuit `Plan()`
+// with `PlanResult{Error: err}` and let the formatter render `[!]`. That
+// covers a transport-level failure (`*SSHError`), a command that could
+// not be executed at all (the dokku binary is missing or not
+// executable), and a cancelled probe. Distinguishing "ran and said no"
+// from "could not run" relies on `ExecError.Ran`, since binary-not-found
+// reports `ExitCode 0` and so cannot be told apart by exit code.
 //
 // Use this for any plan-time probe that today reads exit code only
 // (`apps:exists`, `network:exists`, `<service>:linked`, etc.). Probes
@@ -231,11 +237,22 @@ var (
 func Probe(input ExecCommandInput) (bool, error) {
 	result, err := CallExecCommand(input)
 	if err != nil {
+		// Transport-level failure (ssh connect/auth/host-key): propagate
+		// so the caller can render `! ssh: ...`.
 		var sshErr *SSHError
 		if errors.As(err, &sshErr) {
 			return false, err
 		}
-		return false, nil
+		// The command executed and exited non-zero: the probed state is
+		// absent. Report (false, nil) so idempotent probes need not unwrap.
+		var execErr *ExecError
+		if errors.As(err, &execErr) && execErr.Ran {
+			return false, nil
+		}
+		// Anything else - the command could not be executed (binary not
+		// found, permission denied) or was cancelled - is a real failure
+		// the caller must surface, not "state absent".
+		return false, err
 	}
 	return result.ExitCode == 0, nil
 }
@@ -390,7 +407,10 @@ func classifySshResult(target sshTarget, remote []string, resp ExecCommandRespon
 		}
 	}
 	if resp.ExitCode != 0 {
-		return resp, &ExecError{Response: resp, Err: errors.New(resp.Stderr)}
+		// The remote dokku command ran and exited non-zero (not a
+		// transport failure). Ran marks the exit code as authoritative so
+		// Probe treats it as "state absent" rather than an execution error.
+		return resp, &ExecError{Response: resp, Err: errors.New(resp.Stderr), Ran: true}
 	}
 	return resp, nil
 }

subprocess/ssh_test.go

@@ -1,6 +1,7 @@
 package subprocess
 
 import (
+	"context"
 	"errors"
 	"strings"
 	"testing"
@@ -430,6 +431,62 @@ func TestProbeSshTransportErrorPropagates(t *testing.T) {
 	}
 }
 
+func TestProbeLocalExecErrorPropagates(t *testing.T) {
+	// A local probe whose binary is not on PATH must surface the failure
+	// rather than reporting the probed state as absent. This is the
+	// no-dokku-installed scenario: without propagation, plan would print a
+	// confident [+] create for every resource instead of [!].
+	matched, err := Probe(ExecCommandInput{
+		Command: "nonexistent-binary-docket-test-12345",
+		Args:    []string{"--quiet", "apps:exists", "anything"},
+	})
+	if matched {
+		t.Error("Probe should report matched=false when the binary is missing")
+	}
+	if err == nil {
+		t.Fatal("Probe should propagate a binary-not-found error, not swallow it as absent")
+	}
+}
+
+func TestProbeExecRanFlagControlsAbsence(t *testing.T) {
+	// The Ran flag is the sole signal that separates "the command ran and
+	// exited non-zero" (state absent) from "the command could not run"
+	// (real failure). Inject each case via the swappable runner so the
+	// behaviour is pinned without spawning a process.
+	t.Run("ran non-zero reports absent", func(t *testing.T) {
+		defer SetExecRunner(func(_ context.Context, _ ExecCommandInput) (ExecCommandResponse, error) {
+			resp := ExecCommandResponse{ExitCode: 1}
+			return resp, &ExecError{Response: resp, Err: errors.New("absent"), Ran: true}
+		})()
+
+		matched, err := Probe(ExecCommandInput{Command: "dokku"})
+		if err != nil {
+			t.Fatalf("Probe should treat a ran non-zero exit as absent, got err %v", err)
+		}
+		if matched {
+			t.Error("Probe should report matched=false for a ran non-zero exit")
+		}
+	})
+
+	t.Run("cancelled probe propagates", func(t *testing.T) {
+		defer SetExecRunner(func(_ context.Context, _ ExecCommandInput) (ExecCommandResponse, error) {
+			resp := ExecCommandResponse{ExitCode: -1, Cancelled: true}
+			return resp, &ExecError{Response: resp, Err: context.Canceled}
+		})()
+
+		matched, err := Probe(ExecCommandInput{Command: "dokku"})
+		if matched {
+			t.Error("Probe should report matched=false on a cancelled probe")
+		}
+		if err == nil {
+			t.Fatal("Probe should propagate a cancelled probe, not swallow it as absent")
+		}
+		if !errors.Is(err, context.Canceled) {
+			t.Errorf("propagated error should wrap context.Canceled, got %v", err)
+		}
+	})
+}
+
 func TestSetAndGetDefaultHost(t *testing.T) {
 	t.Cleanup(func() { SetDefaultHost("") })
 	SetDefaultHost("alice@host:2222")

tasks/app_lock_task.go

@@ -131,9 +131,10 @@ func (t AppLockTask) ExportApp(app string) ([]interface{}, error) {
 	return []interface{}{AppLockTask{App: app, State: StatePresent}}, nil
 }
 
-// appLocked checks if a dokku app is locked. Returns (false,
-// *subprocess.SSHError) on transport failure; (false, nil) when dokku
-// reports unlocked; (true, nil) when locked.
+// appLocked checks if a dokku app is locked. Returns (false, err) when
+// the probe could not run - a transport failure, a missing dokku binary,
+// or a cancellation; (false, nil) when dokku reports unlocked; (true,
+// nil) when locked.
 func appLocked(app string) (bool, error) {
 	return subprocess.Probe(subprocess.ExecCommandInput{
 		Command: "dokku",

tasks/app_task.go

@@ -115,8 +115,9 @@ func (t AppTask) ExportApp(app string) ([]interface{}, error) {
 
 // appExists checks if an app exists. Returns (true, nil) when the app
 // is present, (false, nil) when dokku reports it absent, and
-// (false, *subprocess.SSHError) when the probe could not reach the
-// server. Plan() callers must short-circuit on the error.
+// (false, err) when the probe could not run - a transport failure, a
+// missing dokku binary, or a cancellation. Plan() callers must
+// short-circuit on the error.
 func appExists(appName string) (bool, error) {
 	return subprocess.Probe(subprocess.ExecCommandInput{
 		Command: "dokku",

tasks/network_task.go

@@ -164,9 +164,10 @@ func (t NetworkTask) ExportGlobal() ([]interface{}, error) {
 	return out, nil
 }
 
-// networkExists checks if a Docker network exists. Returns (false,
-// *subprocess.SSHError) on transport failure; (false, nil) when dokku
-// reports the network absent; (true, nil) when present.
+// networkExists checks if a Docker network exists. Returns (false, err)
+// when the probe could not run - a transport failure, a missing dokku
+// binary, or a cancellation; (false, nil) when dokku reports the network
+// absent; (true, nil) when present.
 func networkExists(name string) (bool, error) {
 	return subprocess.Probe(subprocess.ExecCommandInput{
 		Command: "dokku",

tasks/service_create_task.go

@@ -147,9 +147,10 @@ func (t ServiceCreateTask) Plan() PlanResult {
 	})
 }
 
-// serviceExists checks if a dokku service exists. Returns (false,
-// *subprocess.SSHError) on transport failure, (false, nil) when dokku
-// reports the service absent, (true, nil) when present.
+// serviceExists checks if a dokku service exists. Returns (false, err)
+// when the probe could not run - a transport failure, a missing dokku
+// binary, or a cancellation, (false, nil) when dokku reports the service
+// absent, (true, nil) when present.
 func serviceExists(service, name string) (bool, error) {
 	return subprocess.Probe(subprocess.ExecCommandInput{
 		Command: "dokku",