fix(deploy): parser dedup, prune rm failure, e2e gaps (D-0422, D-1148, D-1164, D-1176)

internal/deploy/recreate.go

@@ -14,8 +14,8 @@ package deploy
 //              `Container <name> Recreate|Creating` lines (stdout and stderr
 //              mixed, so the command merges them). Service order, the
 //              healthcheck and restart facts come from the same JSON document
-//              the local rolling restart reads (docker.ServiceSpec), parsed
-//              here only because internal/docker exports no parser for it.
+//              the local rolling restart reads (docker.ServiceSpec), through
+//              docker.ParseServiceSpecs.
 
 import (
 	"context"
@@ -88,43 +88,26 @@ func ReadComposeModel(ctx context.Context, r Remote, remotePath string, files, e
 }
 
 func parseComposeModel(raw []byte) (ComposeModel, error) {
+	specs, err := docker.ParseServiceSpecs(raw)
+	if err != nil {
+		return ComposeModel{}, err
+	}
 	var doc struct {
 		Name     string `json:"name"`
 		Services map[string]struct {
-			Image         string `json:"image"`
-			ContainerName string `json:"container_name"`
-			Restart       string `json:"restart"`
-			Healthcheck   *struct {
-				Test        json.RawMessage `json:"test"`
-				Disable     bool            `json:"disable"`
-				Interval    json.RawMessage `json:"interval"`
-				Timeout     json.RawMessage `json:"timeout"`
-				StartPeriod json.RawMessage `json:"start_period"`
-				Retries     json.RawMessage `json:"retries"`
-			} `json:"healthcheck"`
-			Volumes  []Mount                    `json:"volumes"`
-			Networks map[string]json.RawMessage `json:"networks"`
+			Image         string                     `json:"image"`
+			ContainerName string                     `json:"container_name"`
+			Volumes       []Mount                    `json:"volumes"`
+			Networks      map[string]json.RawMessage `json:"networks"`
 		} `json:"services"`
 	}
 	if err := json.Unmarshal(raw, &doc); err != nil {
 		return ComposeModel{}, fmt.Errorf("parsing docker compose config JSON: %w", err)
 	}
 	m := ComposeModel{Project: doc.Name, Services: map[string]ModelService{}}
-	for name, s := range doc.Services {
-		has := false
-		if hc := s.Healthcheck; hc != nil && !hc.Disable {
-			var t []string
-			has = json.Unmarshal(hc.Test, &t) != nil || len(t) == 0 || t[0] != "NONE"
-		}
-		spec := docker.ServiceSpec{Name: name, HasHealthcheck: has, Restart: s.Restart}
-		if has {
-			hc := s.Healthcheck
-			var err error
-			if spec.HealthTimeout, err = docker.HealthTimeout(name, hc.Interval, hc.Timeout, hc.StartPeriod, hc.Retries); err != nil {
-				return ComposeModel{}, err
-			}
-		}
-		m.Services[name] = ModelService{
+	for _, spec := range specs {
+		s := doc.Services[spec.Name]
+		m.Services[spec.Name] = ModelService{
 			Spec:  spec,
 			Image: s.Image, ContainerName: s.ContainerName, Mounts: s.Volumes, Aliases: networkAliases(s.Networks),
 		}

internal/deploy/remote_backup_prune.go

@@ -80,8 +80,10 @@ func PrunePreDeployTars(ctx context.Context, r Remote, remotePath string, keep i
 	for _, n := range drop {
 		drop = append(drop, strings.TrimSuffix(n, ".tar.gz")+"-front.tar.gz", imageRecordPath(n)) // and its image record (D-0700)
 	}
+	// The loop keeps a failed rm in rc and exits with it, so an early failure is not
+	// hidden by a later success (D-1148).
 	rm := fmt.Sprintf(backupsDirProof, q(remotePath)) +
-		fmt.Sprintf(`for n in %s; do f="$x/$n"; if [ -f "$f" ] && [ ! -L "$f" ]; then rm -f -- "$f" && printf 'RM %%s\n' "$n"; fi; done`, strings.Join(quoteAll(drop), " "))
+		fmt.Sprintf(`rc=0; for n in %s; do f="$x/$n"; if [ -f "$f" ] && [ ! -L "$f" ]; then rm -f -- "$f" && printf 'RM %%s\n' "$n" || rc=1; fi; done; exit $rc`, strings.Join(quoteAll(drop), " "))
 	out, err = r.Run(ctx, shScript(rm))
 	if err != nil {
 		return nil, fmt.Errorf("removing old pre-deploy tars on %s: %w", r.Label(), err)

internal/deploy/remote_backup_prune_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -298,3 +299,27 @@ func TestPrunePreDeployTarsDropsFrontTarsWithTheirMain(t *testing.T) {
 		t.Error("a symlinked front tar was followed")
 	}
 }
+
+// D-1148: the removing loop's exit status was the last iteration's, so an rm that
+// failed early was hidden by a later success and the prune reported nothing.
+func TestPrunePreDeployTarsReportsAnEarlierRmFailure(t *testing.T) {
+	needTools(t)
+	realRm, err := exec.LookPath("rm")
+	if err != nil {
+		t.Skip("no rm on PATH")
+	}
+	stubs := t.TempDir()
+	// Fails for the tar dated 03 only; every other rm is the real one.
+	writeFile(t, filepath.Join(stubs, "rm"), "#!/bin/sh\nfor a in \"$@\"; do case \"$a\" in *"+tarName("03")+") exit 1;; esac; done\nexec "+realRm+" \"$@\"\n", 0o755)
+	t.Setenv("PATH", stubs+string(os.PathListSeparator)+os.Getenv("PATH"))
+	f := newFakeRemote(t)
+	rp := t.TempDir()
+	seedTars(t, rp, "01", "02", "03", "04", "05", "06", "07", "08")
+	// Newest first, the drop order is 03, 02, 01: the failure comes first, then two successes.
+	if _, err := PrunePreDeployTars(context.Background(), f, rp, 5); err == nil {
+		t.Fatal("a failed rm was reported as a clean prune")
+	}
+	if !exists(filepath.Join(rp, ".nself", "backups", tarName("03"))) {
+		t.Error("the stub should have kept tar 03; the test did not exercise a failure")
+	}
+}

internal/deploy/remote_fixes_test.go

@@ -7,12 +7,10 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"testing"
 	"time"
 
-	"github.com/nself-org/cli/internal/docker"
 	"github.com/nself-org/cli/internal/errs"
 )
 
@@ -305,72 +303,3 @@ func TestTarKeepsOddFragmentNames(t *testing.T) {
 		t.Errorf("tar lacks the fragment:\n%s", tarList(t, bk.Tar))
 	}
 }
-
-// F18 (CR S2): the copied parsers agree with internal/docker's on the same
-// documents, through a stub docker binary.
-func TestCopiedParsersMatchDocker(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.Skip("shell stub")
-	}
-	dir := t.TempDir()
-	cfg := `{"name":"p","services":{
-	  "a":{"restart":"no"},
-	  "b":{"restart":"always","healthcheck":{"test":["CMD","true"]}},
-	  "c":{"restart":"no","healthcheck":{"test":["NONE"]}},
-	  "d":{"restart":"no","healthcheck":{"disable":true,"test":["CMD","x"]}},
-	  "e":{"healthcheck":{"test":"curl -f localhost"}}}}`
-	psArray := `[{"ID":"1","Name":"p-a-1","State":"running","Health":"healthy","Labels":{"com.docker.compose.oneoff":"False"}},
-	  {"ID":"2","Name":"p-a-run-x","State":"exited","ExitCode":3,"Labels":"com.docker.compose.oneoff=True,x=y"},
-	  {"ID":"3","Name":"p-a-2","State":"exited","ExitCode":1}]`
-	psLines := strings.ReplaceAll(strings.ReplaceAll(psArray[1:len(psArray)-1], "},\n", "}\n"), "\t", "")
-	writeFile(t, filepath.Join(dir, "cfg.json"), cfg, 0o644)
-	stub := filepath.Join(dir, "docker")
-	script := "#!/bin/sh\ncase \"$*\" in\n*\"config --format json\"*) cat " + dir + "/cfg.json;;\n*\"ps -a --format json\"*) cat \"" + dir + "/ps-$PSFORM.json\";;\nesac\n"
-	writeFile(t, stub, script, 0o755)
-	writeFile(t, filepath.Join(dir, "ps-array.json"), psArray, 0o644)
-	writeFile(t, filepath.Join(dir, "ps-lines.json"), strings.ReplaceAll(psLines, ",\n  {", "\n{"), 0o644)
-	c := docker.NewCompose("f.yml")
-	c.DockerPath = stub
-	specs, err := docker.ComposeServiceSpecs(context.Background(), c, dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-	mine, err := parseComposeModel([]byte(cfg))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(specs) != len(mine.Services) {
-		t.Fatalf("service counts differ: %d vs %d", len(specs), len(mine.Services))
-	}
-	for _, sp := range specs {
-		if got := mine.Services[sp.Name].Spec; got != sp {
-			t.Errorf("service %s: docker %+v, copy %+v", sp.Name, sp, got)
-		}
-	}
-	for _, form := range []string{"array", "lines"} {
-		t.Setenv("PSFORM", form)
-		want, err := c.ComposeServiceStates(context.Background(), dir, "a")
-		if err != nil {
-			t.Fatal(err)
-		}
-		raw, _ := os.ReadFile(filepath.Join(dir, "ps-"+form+".json"))
-		all, err := decodeContainerStates(raw)
-		if err != nil {
-			t.Fatal(err)
-		}
-		var got []docker.ContainerState
-		for _, s := range all {
-			if !s.OneOff() {
-				got = append(got, s)
-			}
-		}
-		if len(got) != len(want) {
-			t.Fatalf("%s: %d rows vs docker's %d", form, len(got), len(want))
-		}
-		for i := range got {
-			if got[i].ID != want[i].ID || got[i].State != want[i].State || got[i].ExitCode != want[i].ExitCode || got[i].Health != want[i].Health {
-				t.Errorf("%s row %d: copy %+v, docker %+v", form, i, got[i], want[i])
-			}
-		}
-	}
-}

internal/deploy/remote_states.go

@@ -7,16 +7,13 @@ package deploy
 // Outputs: docker.ContainerState rows (with StartedAt) and the start marks
 //          taken before `up`.
 // Constraints: `docker compose ps -a --format json` and `docker inspect` over
-//              ssh. The rows are decoded into the exported docker.ContainerState
-//              and filtered with its OneOff(); internal/docker exports no
-//              decoder for the ps document (both shapes, array and one object
-//              per line, are handled here). Same StartedAt rules as
+//              ssh. The rows are decoded by docker.ParseContainerStates
+//              (both ps shapes, array and one object per line) and filtered
+//              with docker.ContainerState.OneOff(). Same StartedAt rules as
 //              docker.Compose.FreshStates.
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -32,7 +29,7 @@ func RemoteStates(ctx context.Context, r Remote, remotePath string, files, envFi
 	if err != nil {
 		return nil, err
 	}
-	all, err := decodeContainerStates(out)
+	all, err := docker.ParseContainerStates(out)
 	if err != nil {
 		return nil, err
 	}
@@ -95,29 +92,3 @@ func startedAt(ctx context.Context, r Remote, ids []string) ([]string, error) {
 	}
 	return lines, nil
 }
-
-// decodeContainerStates accepts one JSON array or one object per line.
-func decodeContainerStates(raw []byte) ([]docker.ContainerState, error) {
-	t := bytes.TrimSpace(raw)
-	if len(t) == 0 {
-		return nil, nil
-	}
-	var rows []docker.ContainerState
-	if t[0] == '[' {
-		if err := json.Unmarshal(t, &rows); err != nil {
-			return nil, fmt.Errorf("parsing compose ps JSON: %w", err)
-		}
-		return rows, nil
-	}
-	for _, line := range bytes.Split(t, []byte("\n")) {
-		if line = bytes.TrimSpace(line); len(line) == 0 {
-			continue
-		}
-		var s docker.ContainerState
-		if err := json.Unmarshal(line, &s); err != nil {
-			return nil, fmt.Errorf("parsing compose ps JSON: %w", err)
-		}
-		rows = append(rows, s)
-	}
-	return rows, nil
-}

internal/docker/compose_services_gate_test.go

@@ -184,15 +184,15 @@ func TestParseServiceSpecsHealthTiming(t *testing.T) {
 		"NONE":                            {`{"test":["NONE"],"interval":"5m"}`, false, 0},
 	} {
 		t.Run(name, func(t *testing.T) {
-			got, err := parseServiceSpecs([]byte(hcOf(tc.hc)))
+			got, err := ParseServiceSpecs([]byte(hcOf(tc.hc)))
 			if err != nil || len(got) != 1 || got[0].HasHealthcheck != tc.has || got[0].HealthTimeout != tc.want {
 				t.Errorf("got %+v, err %v; want has=%v HealthTimeout=%s", got, err, tc.has, tc.want)
 			}
 		})
 	}
 	for _, field := range []string{"interval", "timeout", "start_period"} {
 		raw := hcOf(`{"test":["CMD","true"],"` + field + `":"soon"}`)
-		if _, err := parseServiceSpecs([]byte(raw)); err == nil || !strings.Contains(err.Error(), field) {
+		if _, err := ParseServiceSpecs([]byte(raw)); err == nil || !strings.Contains(err.Error(), field) {
 			t.Errorf("bad %s duration must error naming the field, got %v", field, err)
 		}
 	}

internal/docker/compose_services_spec.go

@@ -24,15 +24,16 @@ func ComposeServiceSpecs(ctx context.Context, compose *Compose, workdir string)
 	if err != nil {
 		return nil, err
 	}
-	return parseServiceSpecs(out)
+	return ParseServiceSpecs(out)
 }
 
 // MaxHealthTimeout caps ServiceSpec.HealthTimeout: a healthcheck declared with
 // a huge retries or interval must not stall a deploy for hours.
 const MaxHealthTimeout = 10 * time.Minute
 
-// parseServiceSpecs decodes the `config --format json` document.
-func parseServiceSpecs(raw []byte) ([]ServiceSpec, error) {
+// ParseServiceSpecs decodes the `config --format json` document. internal/deploy
+// calls it on the document it reads over ssh.
+func ParseServiceSpecs(raw []byte) ([]ServiceSpec, error) {
 	var doc struct {
 		Services map[string]struct {
 			Restart     string `json:"restart"`
@@ -93,13 +94,6 @@ func healthDuration(name, field string, raw json.RawMessage, def time.Duration)
 	return min(d, MaxHealthTimeout), nil
 }
 
-// HealthTimeout is healthTimeout for callers that parse the resolved compose
-// config themselves (internal/deploy's copy of this parser), so both compute
-// the same wait.
-func HealthTimeout(name string, interval, timeout, startPeriod, retries json.RawMessage) (time.Duration, error) {
-	return healthTimeout(name, interval, timeout, startPeriod, retries)
-}
-
 // healthTimeout is start_period + retries*(interval+timeout), capped at
 // MaxHealthTimeout. Docker reads an absent or zero interval, timeout or retries
 // as its default (30s, 30s, 3), a non-positive start_period as 0. A bad value

internal/docker/compose_services_test.go

@@ -26,7 +26,7 @@ func TestComposeServiceSpecs(t *testing.T) {
 	  "echo":{"healthcheck":{"test":["CMD","echo","NONE"]}},
 	  "dbg":{"restart":"no","healthcheck":{"test":["CMD","true"]}},
 	  "init":{"restart":"no"}}}`)
-	got, err := parseServiceSpecs(raw)
+	got, err := ParseServiceSpecs(raw)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -45,7 +45,7 @@ func TestComposeServiceSpecs(t *testing.T) {
 	if !got[3].OneShot() || got[6].OneShot() || got[1].OneShot() {
 		t.Error("only restart \"no\" without a healthcheck is a one-shot; a healthcheck or an absent restart is long-running")
 	}
-	if _, err := parseServiceSpecs([]byte("not json")); err == nil {
+	if _, err := ParseServiceSpecs([]byte("not json")); err == nil {
 		t.Error("malformed config JSON must error, not read as zero services")
 	}
 
@@ -87,15 +87,15 @@ func TestParseContainerStates(t *testing.T) {
 	arr := `[{"Name":"a","State":"exited","ExitCode":3},{"Name":"b","State":"running","Health":"healthy"}]`
 	nd := `{"Name":"a","State":"exited","ExitCode":3}` + "\n" + `{"Name":"b","State":"running","Health":"healthy"}`
 	for name, raw := range map[string]string{"array": arr, "ndjson": nd} {
-		got, err := parseContainerStates([]byte(raw))
+		got, err := ParseContainerStates([]byte(raw))
 		if err != nil || len(got) != 2 || got[0].ExitCode != 3 || got[1].Health != "healthy" {
 			t.Errorf("%s: got %+v, err %v", name, got, err)
 		}
 	}
-	if got, err := parseContainerStates([]byte("  \n")); err != nil || got != nil {
+	if got, err := ParseContainerStates([]byte("  \n")); err != nil || got != nil {
 		t.Errorf("empty output = %v, %v; want nil, nil", got, err)
 	}
-	if _, err := parseContainerStates([]byte("[{")); err == nil {
+	if _, err := ParseContainerStates([]byte("[{")); err == nil {
 		t.Error("truncated JSON must error")
 	}
 }

internal/docker/compose_states.go

@@ -87,7 +87,7 @@ func (c *Compose) serviceStates(ctx context.Context, workdir, service string) ([
 	if err != nil {
 		return nil, err
 	}
-	all, err := parseContainerStates(out)
+	all, err := ParseContainerStates(out)
 	if err != nil {
 		return nil, err
 	}
@@ -227,9 +227,10 @@ func (c *Compose) FreshStates(ctx context.Context, workdir, service string, base
 	return kept, nil
 }
 
-// parseContainerStates accepts both ps JSON shapes: one array or one object per
+// ParseContainerStates accepts both ps JSON shapes: one array or one object per
 // line, depending on the compose release; blank lines and CRLF are tolerated.
-func parseContainerStates(raw []byte) ([]ContainerState, error) {
+// internal/deploy calls it on the output it reads over ssh.
+func ParseContainerStates(raw []byte) ([]ContainerState, error) {
 	trimmed := bytes.TrimSpace(raw)
 	if len(trimmed) == 0 {
 		return nil, nil

internal/docker/compose_states_bound_test.go

@@ -144,7 +144,7 @@ func TestParseHealthValues(t *testing.T) {
 	}
 	for name, tc := range ok {
 		t.Run(name, func(t *testing.T) {
-			got, err := parseServiceSpecs([]byte(hc(tc.body)))
+			got, err := ParseServiceSpecs([]byte(hc(tc.body)))
 			if err != nil || len(got) != 1 || got[0].HealthTimeout != tc.want {
 				t.Errorf("got %+v, err %v; want %s", got, err, tc.want)
 			}
@@ -159,7 +159,7 @@ func TestParseHealthValues(t *testing.T) {
 		"retries overflows int64": {`"retries":99999999999999999999`, "retries"},
 	} {
 		t.Run(name, func(t *testing.T) {
-			_, err := parseServiceSpecs([]byte(hc(tc.body)))
+			_, err := ParseServiceSpecs([]byte(hc(tc.body)))
 			if err == nil || !strings.Contains(err.Error(), "service s") || !strings.Contains(err.Error(), tc.field) {
 				t.Errorf("err = %v, want one naming service s and %s", err, tc.field)
 			}