From 41192f48bb098ad4780708b216129a3da55f1e57 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:21:25 -0700 Subject: [PATCH 01/13] =?UTF-8?q?fix(state,ssh):=20T02=20=E2=80=94=20ambig?= =?UTF-8?q?uous=20fenced-release=20failure=20no=20longer=20deletes=20a=20s?= =?UTF-8?q?uccessor's=20lock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReleaseLockFenced's non-fence-error fallback used to run the detached UNCONDITIONAL release: a guarded rm -rf that timed out but completed, or a transport failure after a takeover, deleted whatever lock held the path — including a successor's. The fallback is now one shell-level conditional that removes the lock only when its info file still names the releasing owner (or the lock is already gone); a lock naming anyone else is left strictly alone. MockExecutor gains GuardTransportFailures (guarded commands failing with a transport error) and models the conditional + mv -fT shapes. --- internal/ssh/mock.go | 76 ++++++++++++++++++++++++++++++++++++- internal/state/lock.go | 31 ++++++++++----- internal/state/lock_test.go | 30 +++++++++++++++ 3 files changed, 125 insertions(+), 12 deletions(-) diff --git a/internal/ssh/mock.go b/internal/ssh/mock.go index b24cf55..3e0daa7 100644 --- a/internal/ssh/mock.go +++ b/internal/ssh/mock.go @@ -21,6 +21,12 @@ type MockExecutor struct { mu sync.Mutex Calls []string // records every command executed Files map[string][]byte // records uploaded file contents by path + + // GuardTransportFailures, when > 0, makes the next that-many GUARDED + // commands (the fence-guard shape) fail with a plain transport error + // instead of being evaluated against Files — modeling an SSH channel + // dying mid-command, the ambiguous-release case of audit T02. + GuardTransportFailures int } // MockCommand maps a command prefix to a response. @@ -52,6 +58,11 @@ func (m *MockExecutor) Run(ctx context.Context, cmd string) (string, error) { // and refuses once it does not, which is what the fence tests need to // prove a refused effect never executes. if rest, held, ok := evalFenceGuard(m.Files, cmd); ok { + if m.GuardTransportFailures > 0 { + m.GuardTransportFailures-- + m.mu.Unlock() + return "", fmt.Errorf("ssh: connection timed out") + } if !held { m.mu.Unlock() return "", fmt.Errorf("exit status 75: TEPLOY_FENCE_LOST") @@ -76,11 +87,23 @@ func (m *MockExecutor) Run(ctx context.Context, cmd string) (string, error) { return c.Output, c.Err } } - if strings.HasPrefix(cmd, "mv -f -- ") || strings.HasPrefix(cmd, "rm -f -- ") || strings.HasPrefix(cmd, "rm -rf -- ") { + if strings.HasPrefix(cmd, "mv -f -- ") || strings.HasPrefix(cmd, "mv -fT -- ") || + strings.HasPrefix(cmd, "rm -f -- ") || strings.HasPrefix(cmd, "rm -rf -- ") { m.applyFileCommand(cmd) m.mu.Unlock() return "", nil } + // The conditional lock release (internal/state, audit T02): remove the + // lock directory only when its info still names the releasing owner. + // Modeled against the recorded file state like evalFenceGuard. + if dir, owner, ok := parseConditionalLockRelease(cmd); ok { + info := dir + "/info" + if data, present := m.Files[info]; present && bytes.Contains(data, []byte(owner)) { + m.applyFileCommand("rm -rf -- " + dir) + } + m.mu.Unlock() + return "", nil + } // The server-side adapt gate (internal/caddy, F48/F49) streams the // proposed Caddyfile over stdin; the mock cannot run a real caddy, so // it models "the server's caddy accepted it" — tests that need the @@ -132,6 +155,55 @@ func parseFenceGuard(guard string) (owner, path string, ok bool) { return parts[0][1 : len(parts[0])-1], parts[1][1 : len(parts[1])-1], true } +// parseConditionalLockRelease recognizes the single-command conditional +// release emitted by state.ReleaseLockFenced's ambiguous-failure fallback: +// `if [ -d '' ] && grep -q '' '/info' 2>/dev/null; then rm -rf -- ''; fi` +func parseConditionalLockRelease(cmd string) (dir, owner string, ok bool) { + unquote := func(s string) (string, bool) { + if len(s) >= 2 && s[0] == '\'' && s[len(s)-1] == '\'' { + return s[1 : len(s)-1], true + } + return "", false + } + rest, found := strings.CutPrefix(cmd, "if [ -d ") + if !found { + return "", "", false + } + dirField, rest, found := strings.Cut(rest, " ] && grep -q ") + if !found { + return "", "", false + } + ownerField, rest, found := strings.Cut(rest, " ") + if !found { + return "", "", false + } + infoField, rest, found := strings.Cut(rest, " 2>/dev/null; then rm -rf -- ") + if !found { + return "", "", false + } + rmField, found := strings.CutSuffix(rest, "; fi") + if !found { + return "", "", false + } + dir, ok = unquote(dirField) + if !ok { + return "", "", false + } + owner, ok = unquote(ownerField) + if !ok { + return "", "", false + } + info, ok := unquote(infoField) + if !ok || info != dir+"/info" { + return "", "", false + } + rm, ok := unquote(rmField) + if !ok || rm != dir { + return "", "", false + } + return dir, owner, true +} + func mockCommandMatches(cmd, match string) bool { if !strings.HasPrefix(cmd, match) { return false @@ -146,7 +218,7 @@ func (m *MockExecutor) applyFileCommand(cmd string) { for i := range fields { fields[i] = strings.Trim(fields[i], "'") } - if len(fields) == 5 && fields[0] == "mv" && fields[1] == "-f" && fields[2] == "--" { + if len(fields) == 5 && fields[0] == "mv" && (fields[1] == "-f" || fields[1] == "-fT") && fields[2] == "--" { if data, ok := m.Files[fields[3]]; ok { m.Files[fields[4]] = data delete(m.Files, fields[3]) diff --git a/internal/state/lock.go b/internal/state/lock.go index 862de0c..75e3118 100644 --- a/internal/state/lock.go +++ b/internal/state/lock.go @@ -324,18 +324,29 @@ func ReleaseLockFenced(exec ssh.Executor, lk *Lock, app string) { fmt.Fprintf(os.Stderr, "teploy: refusing to release %s's lock with a lease held for %s\n", app, lk.App()) return } - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - _, err := lk.Guarded(ctx, exec, "rm -rf -- "+ssh.ShellQuote(fmt.Sprintf("%s/%s/.lock", deploymentsDir, app))) - if err != nil && !fenceLostErr(err) { - // Transport-level failure: fall back to the detached - // unconditional release rather than stranding the app — - // same trade-off as the pre-A04 behavior, but only when the - // owner check itself could not be evaluated. - ReleaseLockDetached(exec, app) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + lockDir := fmt.Sprintf("%s/%s/.lock", deploymentsDir, app) + _, err := lk.Guarded(ctx, exec, "rm -rf -- "+ssh.ShellQuote(lockDir)) + if err != nil && !fenceLostErr(err) { + // The guarded release failed ambiguously (transport timeout, for + // one): the release MAY have completed, and a successor may have + // acquired the path in the meantime. An unconditional detached + // release here can delete the SUCCESSOR's lock (audit T02) — but + // never releasing strands the app for a full staleLockTTL. Resolve + // the ambiguity with one shell-level conditional: remove the lock + // only when it still names THIS operation, or when it is already + // gone. A lock that names someone else is left strictly alone. + conditional := fmt.Sprintf( + "if [ -d %s ] && grep -q %s %s 2>/dev/null; then rm -rf -- %s; fi", + ssh.ShellQuote(lockDir), ssh.ShellQuote(lk.owner), ssh.ShellQuote(lockInfoPath(app)), ssh.ShellQuote(lockDir), + ) + if _, cerr := exec.Run(ctx, conditional); cerr != nil { + fmt.Fprintf(os.Stderr, "teploy: could not confirm release of %s's deploy lock: %v (the lock will self-heal after the stale window if abandoned)\n", app, cerr) } - return } + return +} ReleaseLockDetached(exec, app) } diff --git a/internal/state/lock_test.go b/internal/state/lock_test.go index 8bf5808..57abb69 100644 --- a/internal/state/lock_test.go +++ b/internal/state/lock_test.go @@ -243,6 +243,36 @@ func TestReleaseLockFenced_OwnerCheckRemovedOwnLock(t *testing.T) { } } +// TestReleaseLockFenced_AmbiguousFailureNeverDeletesSuccessor is the T02 +// regression: when the guarded release fails with a non-fence (transport) +// error, the fallback must still not unconditionally delete the lock — the +// guarded command may have completed and a successor may already hold the +// path. Only a lock whose info still names THIS owner may be removed. +func TestReleaseLockFenced_AmbiguousFailureNeverDeletesSuccessor(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + // The guarded release fails with a plain transport error (not fence + // loss): every guarded command errors this round. + mock.GuardTransportFailures = 1 + // ...and the server has already moved on: a successor holds the lock. + mock.Files["/deployments/myapp/.lock/info"] = []byte(`{"type":"auto","owner":"successor"}`) + ReleaseLockFenced(mock, lk, "myapp") + if _, ok := mock.Files["/deployments/myapp/.lock/info"]; !ok { + t.Fatal("ambiguous guarded-release failure fell back to deleting the successor's lock") + } +} + +// TestReleaseLockFenced_AmbiguousFailureRemovesOwnLock: the same fallback +// still removes the lock when it provably names the releasing owner — an +// ambiguous failure must not strand the app for a full stale window. +func TestReleaseLockFenced_AmbiguousFailureRemovesOwnLock(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + mock.GuardTransportFailures = 1 + ReleaseLockFenced(mock, lk, "myapp") + if _, ok := mock.Files["/deployments/myapp/.lock/info"]; ok { + t.Fatal("conditional fallback failed to remove the holder's own lock") + } +} + // TestReleaseLockFenced_WrongAppHandleRefused: a lease held for one app // must not release another app's lock (the A17 lease-correspondence rule). func TestReleaseLockFenced_WrongAppHandleRefused(t *testing.T) { From 1b267ec7825757917403c0cd4905012051541cb1 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:29:08 -0700 Subject: [PATCH 02/13] =?UTF-8?q?fix(docker):=20T15+T17=20=E2=80=94=20stru?= =?UTF-8?q?ctured=20label=20parsing,=20daemon=20errors=20are=20not=20cache?= =?UTF-8?q?=20misses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T15: ListContainers requests Labels as a JSON object via a custom --format ({{json .Labels}}) instead of {{json .}}'s comma-joined display string; splitting that display could not distinguish separators from commas inside values, so an unrelated label like 'text,teploy.version=bad' forged a reserved teploy label in the parsed map and steered rollback/prune at the wrong containers. ParseContainers decodes the object form (legacy display string still accepted for old-format producers only). T17 (contained half): ImageExists distinguishes a proven 'no such image' reply from every other inspect failure — the old &&/|| framing turned a daemon outage or permission error into a convincing 'missing', steering callers into pulls and stale-local fallbacks against a broken connection. The resolve-ID-warns-and-falls-back deploy posture (A52) stays deliberate; narrowing A17's remainder. --- internal/docker/docker.go | 84 ++++++++++++++++++++++++++-------- internal/docker/docker_test.go | 71 ++++++++++++++++++++++++++-- 2 files changed, 132 insertions(+), 23 deletions(-) diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 846b8dd..e84ddcd 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -409,13 +409,18 @@ func (c *Client) Pull(ctx context.Context, image string) error { } // ImageExists reports whether the named image is already present in the -// server's local Docker image cache. It runs `docker image inspect` behind a -// shell guard that always exits 0 ("exists"/"missing"), so a real transport -// failure (SSH/docker daemon down) surfaces as an error while a plain cache -// miss does not — letting callers gate a pull without pull access failing the -// check itself. +// server's local Docker image cache. A plain cache miss is distinguished +// from every other inspect failure (audit T17): the old +// `inspect && echo exists || echo missing` shape turned a daemon outage or +// permission error into a convincing "missing", so callers pulled (or fell +// back to stale local copies) against a Docker connection that was broken +// to begin with. Only a stderr proving "no such image" is a miss now; +// anything else fails closed as an error. func (c *Client) ImageExists(ctx context.Context, image string) (bool, error) { - cmd := "docker image inspect " + ssh.ShellQuote(image) + " >/dev/null 2>&1 && echo exists || echo missing" + cmd := fmt.Sprintf( + `err=$(mktemp); if docker image inspect %s >/dev/null 2>"$err"; then st=exists; elif grep -qi 'no such image' "$err"; then st=missing; else echo 'docker image inspect failed:' >&2; cat "$err" >&2; rm -f "$err"; exit 1; fi; rm -f "$err"; printf '%%s\n' "$st"`, + ssh.ShellQuote(image), + ) out, err := c.exec.Run(ctx, cmd) if err != nil { return false, fmt.Errorf("checking for local image %s: %w", image, err) @@ -556,7 +561,15 @@ func (c *Client) InternalPort(ctx context.Context, name string) (int, error) { // ListContainers returns all containers for the given app, including stopped ones. func (c *Client) ListContainers(ctx context.Context, app string) ([]Container, error) { - cmd := "docker ps --all --filter label=teploy.app=" + ssh.ShellQuote(app) + " --format '{{json .}}'" + // Labels are requested as a structured JSON object ({{json .Labels}}), + // not through `{{json .}}` — whose Labels field renders docker's + // comma-joined DISPLAY string. Splitting that display at commas cannot + // distinguish separators from commas inside values, so an unrelated + // label like "note=text,teploy.version=bad" forged a reserved teploy + // label in the parsed map and steered rollback/prune at the wrong + // containers (audit T15). + cmd := "docker ps --all --filter label=teploy.app=" + ssh.ShellQuote(app) + + ` --format '{"ID":{{json .ID}},"Names":{{json .Names}},"Image":{{json .Image}},"State":{{json .State}},"Status":{{json .Status}},"CreatedAt":{{json .CreatedAt}},"Labels":{{json .Labels}}}'` output, err := c.exec.Run(ctx, cmd) if err != nil { return nil, fmt.Errorf("listing containers for %s: %w", app, err) @@ -731,15 +744,17 @@ func (c *Client) FindAvailablePortExcluding(ctx context.Context, claimed map[int return 0, fmt.Errorf("no available ports in range 49152-65535") } -// psEntry matches Docker's JSON output from docker ps --format '{{json .}}'. +// psEntry matches the structured per-container JSON emitted by +// ListContainers' custom --format. Labels arrive as a JSON OBJECT (or, for +// legacy callers/tests, docker's comma-separated display string). type psEntry struct { - ID string `json:"ID"` - Names string `json:"Names"` - Image string `json:"Image"` - State string `json:"State"` - Status string `json:"Status"` - CreatedAt string `json:"CreatedAt"` - Labels string `json:"Labels"` // comma-separated "k=v,k=v" + ID string `json:"ID"` + Names string `json:"Names"` + Image string `json:"Image"` + State string `json:"State"` + Status string `json:"Status"` + CreatedAt string `json:"CreatedAt"` + Labels json.RawMessage `json:"Labels"` } // ParseContainers parses Docker JSON output into Container structs. @@ -756,6 +771,11 @@ func ParseContainers(output string) ([]Container, error) { return nil, fmt.Errorf("parsing container entry: %w", err) } + labels, err := parseEntryLabels(entry.Labels) + if err != nil { + return nil, fmt.Errorf("parsing labels of %s: %w", entry.Names, err) + } + containers = append(containers, Container{ ID: entry.ID, Name: entry.Names, @@ -763,16 +783,40 @@ func ParseContainers(output string) ([]Container, error) { State: entry.State, Status: entry.Status, CreatedAt: entry.CreatedAt, - Labels: parseLabels(entry.Labels), + Labels: labels, }) } return containers, nil } -// parseLabels splits docker ps's comma-separated "k=v,k=v" label string -// into a map. Values containing commas would break this, but teploy labels -// (teploy.app, teploy.process, teploy.version) are safe and known. -func parseLabels(s string) map[string]string { +// parseEntryLabels decodes the Labels field, which is authoritative as a +// JSON object. The legacy string form (docker's comma-joined display) is +// still accepted for backward compatibility with old-format producers, but +// ListContainers itself never emits it — the display string is inherently +// ambiguous (audit T15). +func parseEntryLabels(raw json.RawMessage) (map[string]string, error) { + trimmed := strings.TrimSpace(string(raw)) + if trimmed == "" || trimmed == "null" { + return nil, nil + } + if trimmed[0] == '{' { + var labels map[string]string + if err := json.Unmarshal(raw, &labels); err != nil { + return nil, err + } + return labels, nil + } + var display string + if err := json.Unmarshal(raw, &display); err != nil { + return nil, err + } + return parseLabelsDisplay(display), nil +} + +// parseLabelsDisplay splits docker's legacy comma-separated "k=v,k=v" +// display string into a map. Values containing commas break this — which is +// exactly why ListContainers no longer produces this form (audit T15). +func parseLabelsDisplay(s string) map[string]string { if s == "" { return nil } diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go index 839ced7..a9c3710 100644 --- a/internal/docker/docker_test.go +++ b/internal/docker/docker_test.go @@ -45,7 +45,7 @@ func TestClient_HostPort_NoBindings(t *testing.T) { func TestClient_ImageExists_Present(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "exists\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "exists\n"}, ) client := NewClient(mock) @@ -60,7 +60,7 @@ func TestClient_ImageExists_Present(t *testing.T) { func TestClient_ImageExists_Missing(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "missing\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "missing\n"}, ) client := NewClient(mock) @@ -73,9 +73,32 @@ func TestClient_ImageExists_Missing(t *testing.T) { } } +// TestClient_ImageExists_DaemonFailureIsNotAMiss is the T17 regression: a +// Docker daemon error (permission, daemon down — anything whose stderr is +// not literally "no such image") must surface as an error, never as a +// cache miss that steers the caller into a pull or the warned local +// fallback against a broken connection. +func TestClient_ImageExists_DaemonFailureIsNotAMiss(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{ + Match: "err=$(mktemp); if docker image inspect", + Err: fmt.Errorf("exit status 1: docker image inspect failed: Cannot connect to the Docker daemon"), + }, + ) + client := NewClient(mock) + + ok, err := client.ImageExists(context.Background(), "nginx:latest") + if err == nil { + t.Fatal("expected a daemon failure to be an error, not a cache miss") + } + if ok { + t.Error("a failed inspect must never report the image present") + } +} + func TestClient_ImageExists_TransportError(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Err: fmt.Errorf("connection refused")}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Err: fmt.Errorf("connection refused")}, ) client := NewClient(mock) @@ -842,3 +865,45 @@ func TestPruneVersions_FailedRemovalNotReportedPruned(t *testing.T) { t.Errorf("a version with a failed container removal must not be reported as pruned, got %v", pruned) } } + +// TestParseContainers_StructuredLabelsDefeatCommaInjection is the T15 +// regression: ListContainers now requests Labels as a JSON object, so a +// label VALUE containing ",teploy.version=…" is a distinct map entry and +// can no longer forge a reserved teploy label. +func TestParseContainers_StructuredLabelsDefeatCommaInjection(t *testing.T) { + output := `{"ID":"abc","Names":"myapp-web-v1","Image":"myapp:v1","State":"running","Status":"Up","CreatedAt":"2026-05-28 21:00:00 -0700 PDT","Labels":{"note":"text,teploy.version=bad","teploy.app":"myapp","teploy.process":"web","teploy.version":"v1"}}` + cs, err := ParseContainers(output) + if err != nil { + t.Fatalf("ParseContainers: %v", err) + } + if len(cs) != 1 { + t.Fatalf("expected 1 container, got %d", len(cs)) + } + if cs[0].Labels["teploy.version"] != "v1" { + t.Errorf("teploy.version = %q, want v1 (the comma-containing value must not have overwritten it)", cs[0].Labels["teploy.version"]) + } + if cs[0].Labels["note"] != "text,teploy.version=bad" { + t.Errorf("note label not preserved verbatim: %q", cs[0].Labels["note"]) + } +} + +// TestListContainers_RequestsStructuredLabels pins the docker ps format +// string: labels must be requested as a JSON object, never through +// `{{json .}}`'s comma-joined display string. +func TestListContainers_RequestsStructuredLabels(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker ps", Output: ""}, + ) + if _, err := NewClient(mock).ListContainers(context.Background(), "myapp"); err != nil { + t.Fatalf("ListContainers: %v", err) + } + if len(mock.Calls) != 1 { + t.Fatalf("expected one docker ps call, got %v", mock.Calls) + } + if !strings.Contains(mock.Calls[0], `"Labels":{{json .Labels}}`) { + t.Errorf("labels not requested as a structured JSON object: %s", mock.Calls[0]) + } + if strings.Contains(mock.Calls[0], "'{{json .}}'") { + t.Errorf("ambiguous whole-entry format still in use: %s", mock.Calls[0]) + } +} From a83ff1408774b079f47bc9c93d846dc6176c139a Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:29:09 -0700 Subject: [PATCH 03/13] =?UTF-8?q?fix(docker):=20T12+T13+T19=20=E2=80=94=20?= =?UTF-8?q?anonymous=20volume=20identity,=20bracketed=20IPv6=20bindings,?= =?UTF-8?q?=20env=20off=20the=20argv?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T12: InspectRecreate now captures docker's EFFECTIVE top-level mount inventory. Anonymous volumes created by Dockerfile VOLUME directives appear in neither HostConfig.Binds nor HostConfig.Mounts, so every recreate attached a fresh empty volume while the original data lingered on disk; they are now preserved BY NAME, merged by destination against the requested spec, and an effective mount the CLI cannot represent fails the inspect instead of silently dropping storage. --mount values are CSV-encoded. T13: the recreation renderer brackets IPv6 binds via net.JoinHostPort and validates ports/protocol/bind IP ('::1:49152:80' concatenation is gone), with the ephemeral host-port form preserved. T19 (recreation half, the TCL-12 registered follow-up): resolved env (secrets included) is staged to a private 0600 on-target file and passed via --env-file instead of -e arguments, keeping it out of the host process list and command-bearing diagnostics; values an env file cannot represent fail closed. The docker-exec AWS/MySQL half stays deferred (A33). --- internal/cli/deploy_test.go | 12 +- internal/docker/recreate.go | 194 +++++++++++++++++++++++++++---- internal/docker/recreate_test.go | 132 ++++++++++++++++++++- 3 files changed, 309 insertions(+), 29 deletions(-) diff --git a/internal/cli/deploy_test.go b/internal/cli/deploy_test.go index e4f1c40..c3b1115 100644 --- a/internal/cli/deploy_test.go +++ b/internal/cli/deploy_test.go @@ -54,7 +54,7 @@ func pullAttempted(mock *ssh.MockExecutor) bool { // serve a five-day-old `:latest` while every deploy reported success. func TestEnsureImage_LocalPresent(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "exists\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "exists\n"}, ssh.MockCommand{Match: "docker pull", Output: ""}, ) dk := docker.NewClient(mock) @@ -77,7 +77,7 @@ func TestEnsureImage_LocalPresent(t *testing.T) { // already present in the server's cache. It must still be pulled. func TestEnsureImage_LocalPresentUntaggedRegistry(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "exists\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "exists\n"}, ssh.MockCommand{Match: "docker pull", Output: ""}, ) dk := docker.NewClient(mock) @@ -97,7 +97,7 @@ func TestEnsureImage_LocalPresentUntaggedRegistry(t *testing.T) { func TestEnsureImage_LocalPresentDigestPinned(t *testing.T) { image := "registry.example.com/app@sha256:" + strings.Repeat("a", 64) mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "exists\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "exists\n"}, ssh.MockCommand{Match: "docker pull", Output: ""}, ) dk := docker.NewClient(mock) @@ -121,7 +121,7 @@ func TestEnsureImage_LocalPresentDigestPinned(t *testing.T) { // output must say the local copy may be stale rather than reporting a pull. func TestEnsureImage_PullFailsLocalPresent(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "exists\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "exists\n"}, ssh.MockCommand{Match: "docker pull", Err: errors.New("no such repository")}, ) dk := docker.NewClient(mock) @@ -147,7 +147,7 @@ func TestEnsureImage_PullFailsLocalPresent(t *testing.T) { // copy is still an error. func TestEnsureImage_PullFailsNothingLocal(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "missing\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "missing\n"}, ssh.MockCommand{Match: "docker pull", Err: errors.New("unauthorized")}, ) dk := docker.NewClient(mock) @@ -162,7 +162,7 @@ func TestEnsureImage_PullFailsNothingLocal(t *testing.T) { // is pulled from its registry (unchanged behavior for real registry images). func TestEnsureImage_Missing(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker image inspect", Output: "missing\n"}, + ssh.MockCommand{Match: "err=$(mktemp); if docker image inspect", Output: "missing\n"}, ssh.MockCommand{Match: "docker pull", Output: ""}, ) dk := docker.NewClient(mock) diff --git a/internal/docker/recreate.go b/internal/docker/recreate.go index 9050560..f0a3d9a 100644 --- a/internal/docker/recreate.go +++ b/internal/docker/recreate.go @@ -2,11 +2,16 @@ package docker import ( "context" + "crypto/rand" + "encoding/csv" + "encoding/hex" "encoding/json" "fmt" + "net" "sort" "strconv" "strings" + "time" "github.com/useteploy/teploy/internal/ssh" ) @@ -49,6 +54,11 @@ type RecreateSpec struct { Entrypoint []string `json:"entrypoint,omitempty"` Cmd []string `json:"cmd,omitempty"` Env []string `json:"env,omitempty"` + // EnvFile, when set, names a private on-target env file rendered from + // Env; Recreate publishes it via --env-file instead of -e arguments so + // resolved values never appear in the host process list / command + // diagnostics (audit T19). Empty at inspect time. + EnvFile string `json:"-"` WorkingDir string `json:"working_dir,omitempty"` User string `json:"user,omitempty"` Labels map[string]string `json:"labels,omitempty"` @@ -130,6 +140,20 @@ type containerInspect struct { Privileged bool ReadonlyRootfs bool } + // EffectiveMounts is the container's EFFECTIVE mount inventory + // (docker's top-level .Mounts): everything actually attached, including + // anonymous volumes Dockerfile VOLUME directives created — which appear + // in NEITHER HostConfig.Binds NOR HostConfig.Mounts. Without it, a + // recreate attached a fresh anonymous volume and the application + // started against empty storage while the original volume lingered on + // disk (audit T12). + EffectiveMounts []struct { + Type string `json:"Type"` // "volume" | "bind" | "tmpfs" | "npipe" + Name string `json:"Name"` // volume name (named + anonymous volumes) + Source string `json:"Source"` + Destination string `json:"Destination"` + RW bool `json:"RW"` + } `json:"Mounts"` NetworkSettings struct { Networks map[string]struct { Aliases []string @@ -151,12 +175,12 @@ func (c *Client) InspectRecreate(ctx context.Context, name string) (*RecreateSpe if len(arr) == 0 { return nil, fmt.Errorf("container %s not found", name) } - return specFromInspect(name, arr[0]), nil + return specFromInspect(name, arr[0]) } // specFromInspect is the pure inspect-JSON -> RecreateSpec mapping, split out // so tests can drive it without an executor. -func specFromInspect(name string, in containerInspect) *RecreateSpec { +func specFromInspect(name string, in containerInspect) (*RecreateSpec, error) { spec := &RecreateSpec{ Name: name, ImageID: in.Image, @@ -250,12 +274,41 @@ func specFromInspect(name string, in containerInspect) *RecreateSpec { spec.Mounts = append(spec.Mounts, RecreateMount{Type: m.Type, Source: m.Source, Target: m.Target, ReadOnly: m.ReadOnly}) } + // Effective mounts not requested in HostConfig (image VOLUME anonymous + // volumes, chiefly): preserve them by NAME so the recreated container + // re-attaches the SAME volume instead of a fresh empty one (T12). A + // destination already covered by an explicit bind/mount is skipped — + // docker's effective view mirrors the request there. Anything the CLI + // cannot represent faithfully fails the whole inspect: silently + // dropping an effective mount is how data "disappears" on restart. + explicit := map[string]bool{} + for _, m := range in.HostConfig.Mounts { + if m.Target != "" { + explicit[m.Target] = true + } + } + for _, b := range in.HostConfig.Binds { + parts := strings.Split(b, ":") + if len(parts) >= 2 { + explicit[parts[1]] = true + } + } + for _, m := range in.EffectiveMounts { + if m.Destination == "" || explicit[m.Destination] { + continue + } + if m.Type != "volume" || m.Name == "" { + return nil, fmt.Errorf("container %s has an effective %s mount at %s that the docker CLI recreation path cannot represent; refusing to silently drop it", name, m.Type, m.Destination) + } + spec.Mounts = append(spec.Mounts, RecreateMount{Type: "volume", Source: m.Name, Target: m.Destination, ReadOnly: !m.RW}) + } + for k, v := range in.HostConfig.LogConfig.Config { spec.LogOpts = append(spec.LogOpts, k+"="+v) } sort.Strings(spec.LogOpts) - return spec + return spec, nil } func atoiOrZero(s string) int { @@ -280,19 +333,24 @@ func RenderRecreateArgs(spec *RecreateSpec) ([]string, error) { } for _, b := range spec.PortBindings { - containerPort := strconv.Itoa(b.ContainerPort) - if b.Proto != "" { - containerPort += "/" + b.Proto - } - hostPort := "" - if b.HostPort > 0 { - hostPort = strconv.Itoa(b.HostPort) + binding, err := recreatePublishBinding(b) + if err != nil { + return nil, fmt.Errorf("container %s: %w", spec.Name, err) } - args = append(args, "-p", q(b.HostIP+":"+hostPort+":"+containerPort)) + args = append(args, "-p", q(binding)) } - for _, e := range spec.Env { - args = append(args, "-e", q(e)) + switch { + case spec.EnvFile != "": + // The resolved env rides a private on-target file, not -e argv + // (audit T19): inspect-derived values include secrets resolved at + // create time, and the docker CLI argument list is visible in the + // host process list and command-bearing errors. + args = append(args, "--env-file", q(spec.EnvFile)) + case len(spec.Env) > 0: + for _, e := range spec.Env { + args = append(args, "-e", q(e)) + } } for _, b := range spec.Binds { @@ -300,14 +358,7 @@ func RenderRecreateArgs(spec *RecreateSpec) ([]string, error) { } for _, m := range spec.Mounts { - parts := []string{"type=" + m.Type, "target=" + m.Target} - if m.Source != "" { - parts = append(parts, "source="+m.Source) - } - if m.ReadOnly { - parts = append(parts, "readonly") - } - args = append(args, "--mount", q(strings.Join(parts, ","))) + args = append(args, "--mount", q(encodeMountCSV(m))) } if spec.MemoryBytes > 0 { @@ -432,6 +483,63 @@ func RenderRecreateArgs(spec *RecreateSpec) ([]string, error) { return args, nil } +// encodeMountCSV renders one --mount value with encoding/csv so a source or +// destination containing a comma is quoted instead of silently splitting +// into bogus options (T12). +func encodeMountCSV(m RecreateMount) string { + fields := []string{"type=" + m.Type, "target=" + m.Target} + if m.Source != "" { + fields = append(fields, "source="+m.Source) + } + if m.ReadOnly { + fields = append(fields, "readonly") + } + var buf strings.Builder + w := csv.NewWriter(&buf) + _ = w.Write(fields) + w.Flush() + return strings.TrimSuffix(buf.String(), "\n") +} + +// recreatePublishBinding renders one inspected port binding back into a +// docker -p spec with the bind IP correctly bracketed for IPv6 and both +// ports validated (audit T13): the renderer used to concatenate +// HostIP+":"+hostPort+":"+containerPort, so an IPv6 bind (::1) produced the +// unparseable "::1:49152:80/tcp" and every restart/rollback of an +// IPv6-published container failed at docker run — a defect the normal +// deploy path's publishBinding (A19) had already fixed. HostPort 0 keeps +// docker's ephemeral-allocation form (empty host port field). +func recreatePublishBinding(b RecreateBinding) (string, error) { + if b.ContainerPort < 1 || b.ContainerPort > 65535 { + return "", fmt.Errorf("container port %d must be in 1..65535", b.ContainerPort) + } + if b.HostPort < 0 || b.HostPort > 65535 { + return "", fmt.Errorf("host port %d must be in 0..65535", b.HostPort) + } + switch b.Proto { + case "", "tcp", "udp", "sctp": + default: + return "", fmt.Errorf("unsupported protocol %q", b.Proto) + } + ip := b.HostIP + if ip == "" { + ip = "0.0.0.0" + } + normalized := strings.TrimSuffix(strings.TrimPrefix(ip, "["), "]") + if net.ParseIP(normalized) == nil { + return "", fmt.Errorf("bind address %q must be an IP address", ip) + } + hostPort := "" + if b.HostPort > 0 { + hostPort = strconv.Itoa(b.HostPort) + } + containerPort := strconv.Itoa(b.ContainerPort) + if b.Proto != "" { + containerPort += "/" + b.Proto + } + return net.JoinHostPort(normalized, hostPort) + ":" + containerPort, nil +} + // Recreate force-removes the named container and runs a fresh one from the // spec. avoidPorts is the set of host ports currently held by containers // this recreation must not collide with; a binding whose original port is @@ -472,6 +580,24 @@ func (c *Client) Recreate(ctx context.Context, spec *RecreateSpec, avoidPorts ma } } + // Resolved env (secrets included — inspect shows what the container + // actually runs with) is published to a private on-target file and + // passed via --env-file, keeping it out of the host process list and + // command-bearing diagnostics (audit T19). Values a docker env file + // cannot represent (newlines, NUL) fail closed rather than corrupt. + if len(spec.Env) > 0 { + envPath, err := c.publishRecreateEnv(ctx, spec.Env) + if err != nil { + return err + } + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer func() { + c.exec.Run(cleanupCtx, "rm -f -- "+ssh.ShellQuote(envPath)) + cleanupCancel() + }() + spec.EnvFile = envPath + } + args, err := RenderRecreateArgs(spec) if err != nil { return err @@ -486,6 +612,32 @@ func (c *Client) Recreate(ctx context.Context, spec *RecreateSpec, avoidPorts ma return nil } +// publishRecreateEnv stages the resolved environment as a private (0600) +// file under /tmp and returns its path. The caller removes it when the +// recreated container is running. +func (c *Client) publishRecreateEnv(ctx context.Context, env []string) (string, error) { + var b strings.Builder + for _, e := range env { + if !strings.Contains(e, "=") { + return "", fmt.Errorf("environment entry %q is not KEY=VALUE and cannot be written to an env file", e) + } + if strings.ContainsAny(e, "\n\x00") { + return "", fmt.Errorf("environment entry %q contains a newline or NUL that a docker env file cannot represent", e) + } + b.WriteString(e) + b.WriteByte('\n') + } + var nonce [8]byte + if _, err := rand.Read(nonce[:]); err != nil { + return "", fmt.Errorf("generating env staging name: %w", err) + } + path := "/tmp/.teploy-recreate-env-" + hex.EncodeToString(nonce[:]) + if err := c.exec.Upload(ctx, strings.NewReader(b.String()), path, "0600"); err != nil { + return "", fmt.Errorf("staging the recreated container's env file: %w", err) + } + return path, nil +} + // entrypointMatchesImage reports whether the container's multi-element // entrypoint equals its image's own entrypoint, comparing the JSON arrays. func (c *Client) entrypointMatchesImage(ctx context.Context, spec *RecreateSpec) (bool, error) { diff --git a/internal/docker/recreate_test.go b/internal/docker/recreate_test.go index 250b11b..2193e30 100644 --- a/internal/docker/recreate_test.go +++ b/internal/docker/recreate_test.go @@ -153,8 +153,7 @@ func TestRecreate_RendersEveryPreservedField(t *testing.T) { "--network-alias 'myapp'", "-p '127.0.0.1:49152:3000/tcp'", "-p '0.0.0.0:51820:51820/udp'", - "-e 'PORT=3000'", - "-e 'TOKEN=sec;ret'", + "--env-file '/tmp/.teploy-recreate-env-", "-v '/deployments/myapp/volumes/data:/data:ro'", "--mount 'type=volume,target=/uploads,source=myapp-uploads'", "--memory 536870912b", @@ -186,6 +185,44 @@ func TestRecreate_RendersEveryPreservedField(t *testing.T) { if strings.Contains(run, "--privileged") { t.Errorf("privileged rendered for an unprivileged container: %s", run) } + // T19: the resolved env (secrets included) rides the private --env-file + // and must never appear as -e argv in the docker run command. + for _, leaked := range []string{"-e 'PORT=3000'", "-e 'TOKEN=sec;ret'"} { + if strings.Contains(run, leaked) { + t.Errorf("resolved env leaked into argv: %s\n run: %s", leaked, run) + } + } + var cleaned bool + for _, c := range mock.Calls { + if strings.HasPrefix(c, "rm -f -- '/tmp/.teploy-recreate-env-") { + cleaned = true + } + } + if !cleaned { + t.Error("the staged env file was not removed after the recreate") + } +} + +// TestPublishRecreateEnv validates the staged env file's contents and the +// values a docker env file cannot represent (T19). +func TestPublishRecreateEnv(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4") + c := NewClient(mock) + path, err := c.publishRecreateEnv(context.Background(), []string{"PORT=3000", "TOKEN=sec;ret"}) + if err != nil { + t.Fatalf("publishRecreateEnv: %v", err) + } + if got := string(mock.Files[path]); got != "PORT=3000\nTOKEN=sec;ret\n" { + t.Errorf("staged env = %q", got) + } + for _, bad := range []string{"MULTI=a\nb", "NUL=a\x00b"} { + if _, err := c.publishRecreateEnv(context.Background(), []string{bad}); err == nil { + t.Errorf("expected %q to fail closed", bad) + } + } + if _, err := c.publishRecreateEnv(context.Background(), []string{"NOT_AN_ASSIGNMENT"}); err == nil { + t.Error("expected a non KEY=VALUE entry to fail closed") + } } // A multi-element entrypoint that MATCHES the image's own must be dropped @@ -323,3 +360,94 @@ func TestRecreate_AvoidPortsReallocatesCollidingBinding(t *testing.T) { t.Errorf("expected reallocation to the next free port 49153: %s", run) } } + +// TestRecreatePreservesAnonymousVolumes is the T12 regression: an effective +// top-level volume mount (a Dockerfile VOLUME's anonymous volume) appears in +// neither HostConfig.Binds nor HostConfig.Mounts, so recreation used to +// attach a fresh empty volume. The spec must capture it BY NAME and the +// rendered run must re-mount the same volume. +func TestRecreatePreservesAnonymousVolumes(t *testing.T) { + inspect := fmt.Sprintf(`[{ + "Image": "sha256:%s", + "Config": {"Image": "myapp:v9"}, + "HostConfig": {"NetworkMode": "teploy", "PortBindings": {}, "RestartPolicy": {}, "Binds": ["/deployments/myapp/uploads:/uploads:ro"], "Mounts": []}, + "Mounts": [ + {"Type": "volume", "Name": "4b1c8a3f9b2c_anon", "Source": "/var/lib/docker/volumes/4b1c8a3f9b2c_anon/_data", "Destination": "/var/lib/postgresql/data", "RW": true}, + {"Type": "bind", "Source": "/deployments/myapp/uploads", "Destination": "/uploads", "RW": false} + ], + "NetworkSettings": {"Networks": {"teploy": {"Aliases": ["myapp"]}}} +}]`, strings.Repeat("c", 64)) + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker inspect 'c'", Output: inspect}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + ssh.MockCommand{Match: "docker run", Output: ""}, + ) + if err := NewClient(mock).Restart(context.Background(), "c", nil); err != nil { + t.Fatalf("Restart with an anonymous volume sibling: %v", err) + } + var run string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker run ") { + run = c + } + } + if !strings.Contains(run, "--mount 'type=volume,target=/var/lib/postgresql/data,source=4b1c8a3f9b2c_anon'") { + t.Errorf("anonymous volume not re-attached by name: %s", run) + } + // The explicit bind also present in HostConfig.Binds is covered by the + // requested spec and must NOT be re-added from the effective view as a + // second mount targeting the same destination. + if strings.Contains(run, "target=/uploads") { + t.Errorf("explicit bind duplicated from the effective mount view: %s", run) + } +} + +// TestRecreateFailsClosedOnUnrepresentableEffectiveMount: an effective mount +// the CLI path cannot represent (a bind nobody requested in HostConfig) +// must abort the recreate instead of silently dropping the mount. +func TestRecreateFailsClosedOnUnrepresentableEffectiveMount(t *testing.T) { + inspect := fmt.Sprintf(`[{ + "Image": "sha256:%s", + "Config": {"Image": "myapp:v9"}, + "HostConfig": {"NetworkMode": "teploy", "PortBindings": {}, "RestartPolicy": {}, "Binds": [], "Mounts": []}, + "Mounts": [{"Type": "bind", "Source": "/somewhere/else", "Destination": "/data", "RW": true}], + "NetworkSettings": {"Networks": {"teploy": {}}} +}]`, strings.Repeat("d", 64)) + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker inspect 'c'", Output: inspect}, + ) + if err := NewClient(mock).Restart(context.Background(), "c", nil); err == nil { + t.Fatal("expected recreate to fail closed on an effective bind mount outside HostConfig") + } + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker rm -f") { + t.Fatalf("original container was removed despite the closed failure: %s", c) + } + } +} + +// TestRecreateBracketsIPv6Bindings is the T13 regression: an IPv6 bind must +// render bracketed via net.JoinHostPort ("[::1]:49152:3000/tcp"), not the +// concatenated "::1:49152:3000/tcp" that docker cannot parse. +func TestRecreateBracketsIPv6Bindings(t *testing.T) { + b, err := recreatePublishBinding(RecreateBinding{HostIP: "::1", HostPort: 49152, ContainerPort: 3000, Proto: "tcp"}) + if err != nil { + t.Fatalf("recreatePublishBinding: %v", err) + } + if b != "[::1]:49152:3000/tcp" { + t.Errorf("IPv6 binding = %q, want [::1]:49152:3000/tcp", b) + } + if _, err := recreatePublishBinding(RecreateBinding{HostIP: "not-an-ip", HostPort: 49152, ContainerPort: 3000}); err == nil { + t.Error("expected an invalid bind IP to fail closed") + } + if _, err := recreatePublishBinding(RecreateBinding{HostIP: "0.0.0.0", HostPort: 70000, ContainerPort: 3000}); err == nil { + t.Error("expected an out-of-range host port to fail closed") + } + ephemeral, err := recreatePublishBinding(RecreateBinding{HostIP: "0.0.0.0", HostPort: 0, ContainerPort: 3000}) + if err != nil { + t.Fatalf("ephemeral binding: %v", err) + } + if ephemeral != "0.0.0.0::3000" { + t.Errorf("ephemeral binding = %q, want 0.0.0.0::3000", ephemeral) + } +} From 34d6dc97580d78a4eb20702ac80bbd10d2e47e83 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:33:38 -0700 Subject: [PATCH 04/13] =?UTF-8?q?fix(deploy):=20T06+T07+T21+T63=20?= =?UTF-8?q?=E2=80=94=20honest=20recovery=20accounting,=20fail-closed=20wor?= =?UTF-8?q?ker=20verification,=20inventory-retried=20fallback=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T06: rollback's route phase (upstream-port inspections, SetRoute, SetLoadBalancerHealth) used to return directly on failure after the target restarted — leaving the uncommitted target running and, under fixed ports, Caddy pointed at the stopped current container. Route failures now unwind through the same cleanup as start/health failures (stop started, restore displaced). T07: restoreDisplacedAndStarted never set its restored flag on SUCCESS, so an all-predecessors-restarted recovery still reported 'no container is serving'. Successful restarts now count as restored, and partial cleanup failures are joined into the returned error instead of only printing. T21: worker verification treats a persistently unreadable inspect as a deploy FAILURE after bounded retries (reversing A23's degrade-to-warning posture — unknown is not readiness; a deploy can no longer commit while unable to prove any worker exists). T63: the name-derived cleanup fallback retries the container inventory first (it derives worker names from the NEW config, so a worker removed this deploy was invisible to it and kept consuming jobs); every fallback stop/remove failure is reported instead of silently dropped. The predecessor selection is extracted and shared with the snapshot path. --- internal/deploy/deploy.go | 185 +++++++++++++++++++---------- internal/deploy/deploy_test.go | 11 ++ internal/deploy/plan_a_test.go | 1 + internal/deploy/recovery_a_test.go | 5 +- internal/deploy/rollback.go | 21 +++- internal/deploy/rollback_test.go | 44 +++++++ 6 files changed, 200 insertions(+), 67 deletions(-) diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 6a91635..350f3d6 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -3,6 +3,7 @@ package deploy import ( "context" "encoding/json" + "errors" "fmt" "io" "maps" @@ -448,18 +449,7 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) if current != nil && current.CurrentHash != "" { if inv, invErr := d.docker.ListContainers(ctx, cfg.App); invErr == nil { predecessorsListed = true - for _, ct := range inv { - if ct.Labels["teploy.role"] == "accessory" { - continue // accessories have their own lifecycle - } - if ct.Labels["teploy.version"] != current.CurrentHash { - continue - } - if !sameVersion && ct.State != "running" { - continue // older stopped versions are kept as rollback targets - } - predecessors = append(predecessors, ct) - } + predecessors = selectPredecessors(inv, current, sameVersion) } else { fmt.Fprintf(d.out, "Warning: could not list containers for the predecessor snapshot (%v); falling back to name matching\n", invErr) } @@ -535,6 +525,11 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) cleanupFailures = append(cleanupFailures, fmt.Sprintf("restore %s: %v", old, err)) fmt.Fprintf(d.out, " WARNING: could not restore displaced container %s: %v\n", old, err) } else { + // A successful restart is proof of restoration (T07): the + // flag used to stay false even when EVERY predecessor came + // back, so an accurate "all restored" recovery reported + // "no container is serving". + restored = true fmt.Fprintf(d.out, " Restored %s\n", old) } } @@ -542,8 +537,11 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) fmt.Fprintf(d.out, " WARNING: cleanup incomplete after failure — %s\n", strings.Join(cleanupFailures, "; ")) } d.logDeploy(recoveryCtx, cfg, false, start) - if !restored && len(displacedHostWeb) > 0 { - return fmt.Errorf("%w — recovery also failed: no container is serving; %s needs manual attention (%s)", reason, cfg.App, strings.Join(cleanupFailures, "; ")) + if len(displacedHostWeb) > 0 && !restored { + return fmt.Errorf("%w — recovery also failed: no predecessor could be restarted; %s needs manual attention (%s)", reason, cfg.App, strings.Join(cleanupFailures, "; ")) + } + if len(cleanupFailures) > 0 { + return fmt.Errorf("%w — recovery incomplete, %s needs attention: %s", reason, cfg.App, strings.Join(cleanupFailures, "; ")) } return reason } @@ -813,47 +811,35 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) // Only the snapshotted predecessors are touched. Selecting by the // teploy.version label from a post-deploy inventory — the previous // implementation — also matched the just-deployed replacement during a - // same-version redeploy and removed the live generation (TCL-02). The - // name-derived fallback only runs when the inventory could not be - // listed at snapshot time. + // same-version redeploy and removed the live generation (TCL-02). When + // the snapshot could not be listed, the cleanup RETRIES the inventory + // first (T63): the name-derived fallback derives worker names from the + // NEW config's processes, so a worker the operator REMOVED this deploy + // is invisible to it and would keep consuming jobs while the deploy + // reported success. Only a still-failing inventory degrades to names — + // now with every stop/remove failure reported (T63's honest-retirement + // half). if predecessorsListed { - for _, ct := range predecessors { - // Fence (F16): the deploy is already committed; a fence loss - // mid-cleanup means another operation owns the app now. Refuse - // further stops (loudly) rather than interleaving with it — - // leaving an old worker running is degraded but visible. - if lk != nil { - if err := lk.Check(ctx, d.exec); err != nil { - fmt.Fprintf(d.out, "Warning: predecessor cleanup stopped — %v\n", err) - break - } - } - fmt.Fprintf(d.out, "Stopping old container %s...\n", ct.Name) - if err := d.docker.Stop(ctx, ct.Name, stopTimeout); err != nil { - // Traffic is already committed to the new generation; a - // failed predecessor stop is degraded cleanup, not a failed - // deploy — but it must be reported, never silent (TCL-19): - // a leftover old worker keeps consuming jobs. - fmt.Fprintf(d.out, "Warning: could not stop old container %s: %v\n", ct.Name, err) - continue - } - if sameVersion { - if err := d.docker.Remove(ctx, ct.Name); err != nil { - fmt.Fprintf(d.out, "Warning: could not remove old container %s: %v\n", ct.Name, err) - } - } - } + d.stopPredecessorSnapshot(ctx, predecessors, sameVersion, stopTimeout, lk) } else if current != nil && current.CurrentHash != "" { - // Fence (F16): same refusal as the snapshot-driven cleanup above — - // post-commit cleanup never interleaves with a new holder. + // Fence (F16): post-commit cleanup never interleaves with a new + // holder. + fenceOK := true if lk != nil { if err := lk.Check(ctx, d.exec); err != nil { fmt.Fprintf(d.out, "Warning: predecessor cleanup skipped — %v\n", err) + fenceOK = false + } + } + if fenceOK { + if inv, invErr := d.docker.ListContainers(ctx, cfg.App); invErr == nil { + d.stopPredecessorSnapshot(ctx, selectPredecessors(inv, current, sameVersion), sameVersion, stopTimeout, lk) } else { - stopOldWorkloadsByName(ctx, d.docker, d.out, cfg, current, processes, stopTimeout) + fmt.Fprintf(d.out, "Warning: container inventory still unreadable (%v) — cleaning up by derived names; a removed worker process may escape retirement\n", invErr) + if err := stopOldWorkloadsByName(ctx, d.docker, d.out, cfg, current, processes, stopTimeout); err != nil { + fmt.Fprintf(d.out, "Warning: name-based cleanup incomplete: %v\n", err) + } } - } else { - stopOldWorkloadsByName(ctx, d.docker, d.out, cfg, current, processes, stopTimeout) } } @@ -925,25 +911,84 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) return nil } +// selectPredecessors picks the containers this deploy must retire from an +// app inventory: the authoritative current version's workload (accessories +// excluded — they have their own lifecycle), keeping stopped historical +// containers ONLY for a same-version redeploy (they were just renamed to +// _replaced and must be removed, TCL-02/A08's contract). +func selectPredecessors(inv []docker.Container, current *state.AppState, sameVersion bool) []docker.Container { + var out []docker.Container + for _, ct := range inv { + if ct.Labels["teploy.role"] == "accessory" { + continue // accessories have their own lifecycle + } + if ct.Labels["teploy.version"] != current.CurrentHash { + continue + } + if !sameVersion && ct.State != "running" { + continue // older stopped versions are kept as rollback targets + } + out = append(out, ct) + } + return out +} + +// stopPredecessorSnapshot retires exactly the snapshotted predecessor set. +// Fence checks precede each stop: the deploy is already committed, and a +// fence loss mid-cleanup means another operation owns the app — refuse +// further stops (loudly) rather than interleaving. +func (d *Deployer) stopPredecessorSnapshot(ctx context.Context, predecessors []docker.Container, sameVersion bool, stopTimeout int, lk *state.Lock) { + for _, ct := range predecessors { + if lk != nil { + if err := lk.Check(ctx, d.exec); err != nil { + fmt.Fprintf(d.out, "Warning: predecessor cleanup stopped — %v\n", err) + break + } + } + fmt.Fprintf(d.out, "Stopping old container %s...\n", ct.Name) + if err := d.docker.Stop(ctx, ct.Name, stopTimeout); err != nil { + // Traffic is already committed to the new generation; a + // failed predecessor stop is degraded cleanup, not a failed + // deploy — but it must be reported, never silent (TCL-19): + // a leftover old worker keeps consuming jobs. + fmt.Fprintf(d.out, "Warning: could not stop old container %s: %v\n", ct.Name, err) + continue + } + if sameVersion { + if err := d.docker.Remove(ctx, ct.Name); err != nil { + fmt.Fprintf(d.out, "Warning: could not remove old container %s: %v\n", ct.Name, err) + } + } + } +} + // stopOldWorkloadsByName is the name-derived fallback for old-workload -// cleanup when the container inventory cannot be listed (see step 14). -func stopOldWorkloadsByName(ctx context.Context, dk *docker.Client, out io.Writer, cfg Config, current *state.AppState, processes map[string]string, stopTimeout int) { +// cleanup when the container inventory cannot be listed at all (see step +// 14). Every stop/remove failure is reported (T63): the old shape ignored +// them entirely, so an incomplete retirement read as a clean one. +func stopOldWorkloadsByName(ctx context.Context, dk *docker.Client, out io.Writer, cfg Config, current *state.AppState, processes map[string]string, stopTimeout int) error { if current == nil || current.CurrentHash == "" { - return + return nil } sameVersion := current.CurrentHash == cfg.Version oldReplicas := len(current.CurrentPorts) if oldReplicas == 0 { oldReplicas = 1 } + var failures []error stop := func(name string) { if sameVersion { name += "_replaced" } fmt.Fprintf(out, "Stopping old container %s...\n", name) - dk.Stop(ctx, name, stopTimeout) + if err := dk.Stop(ctx, name, stopTimeout); err != nil { + failures = append(failures, fmt.Errorf("stop %s: %w", name, err)) + return + } if sameVersion { - dk.Remove(ctx, name) + if err := dk.Remove(ctx, name); err != nil { + failures = append(failures, fmt.Errorf("remove %s: %w", name, err)) + } } } for ri := 1; ri <= oldReplicas; ri++ { @@ -958,6 +1003,7 @@ func stopOldWorkloadsByName(ctx context.Context, dk *docker.Client, out io.Write } stop(docker.ContainerName(cfg.App, process, current.CurrentHash)) } + return errors.Join(failures...) } func (d *Deployer) abortStateCommit(ctx context.Context, cfg Config, current *state.AppState, started, displacedHostWeb []string, start time.Time, commitErr error) error { @@ -1212,14 +1258,14 @@ func (d *Deployer) reconcilePartialRun(name string) { // workerRemainsRunning verifies a just-started worker process is actually // viable: still running (not exited/dead/restarting) one second after the // detached run, and not already flagged unhealthy by the image's -// healthcheck (A23). An inspect result that cannot be parsed degrades to a -// warning — the container itself remains subject to the normal cleanup -// paths — but a PARSED dead/restarting state fails the deploy. +// healthcheck (A23). An inspect that stays unreadable across bounded +// retries FAILS the deploy (audit T21 — this reverses A23's deliberate +// degrade-to-warning, which let a deploy commit while unable to prove any +// worker existed): unknown is not readiness. func (d *Deployer) workerRemainsRunning(ctx context.Context, name string) error { - st, ok := d.inspectWorkerState(ctx, name) + st, ok := d.inspectWorkerStateRetry(ctx, name) if !ok { - fmt.Fprintf(d.out, "Warning: could not verify worker %s stability (inspect unreadable); proceeding\n", name) - return nil + return fmt.Errorf("cannot verify worker %s: its state is unreadable after repeated inspection — refusing to commit a deploy whose worker viability is unknown", name) } if err := workerStateViable(st); err != nil { return fmt.Errorf("worker %s is not viable: %w", name, err) @@ -1229,9 +1275,9 @@ func (d *Deployer) workerRemainsRunning(ctx context.Context, name string) error return ctx.Err() case <-time.After(time.Second): } - st, ok = d.inspectWorkerState(ctx, name) + st, ok = d.inspectWorkerStateRetry(ctx, name) if !ok { - return nil + return fmt.Errorf("cannot re-verify worker %s after the settling delay: its state is unreadable — refusing to commit a deploy whose worker viability is unknown", name) } return workerStateViable(st) } @@ -1258,6 +1304,23 @@ func (d *Deployer) inspectWorkerState(ctx context.Context, name string) (workerS return st, true } +// inspectWorkerStateRetry retries an unreadable inspect a few times with a +// short gap (a transport hiccup right after a detached run is common); +// persistently-unknown states stay unknown so callers fail closed. +func (d *Deployer) inspectWorkerStateRetry(ctx context.Context, name string) (workerStateJSON, bool) { + for attempt := 0; attempt < 3; attempt++ { + if st, ok := d.inspectWorkerState(ctx, name); ok { + return st, true + } + select { + case <-ctx.Done(): + return workerStateJSON{}, false + case <-time.After(500 * time.Millisecond): + } + } + return workerStateJSON{}, false +} + func workerStateViable(st workerStateJSON) error { if st.Restarting || st.Status == "exited" || st.Status == "dead" { return fmt.Errorf("container is %s (exit %d)", st.Status, st.ExitCode) diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index b981621..6a558b2 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -395,6 +395,7 @@ func TestDeploy_HealthCheckFailure(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "failcontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Health check always fails (connection refused). ssh.MockCommand{Match: "curl -s -o /dev/null", Err: fmt.Errorf("connection refused")}, @@ -622,6 +623,7 @@ func TestDeploy_SameVersion(t *testing.T) { ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: ""}, ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "newcontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, ssh.MockCommand{Match: "curl -sf http://localhost:2019/config/apps/http/servers/srv0", Output: `{"listen":[":80",":443"]}`}, @@ -694,6 +696,7 @@ func TestDeploy_SameVersion_StaleReplaced(t *testing.T) { // Rename live container. ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "newcontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, ssh.MockCommand{Match: "curl -sf http://localhost:2019/config/apps/http/servers/srv0", Output: `{"listen":[":80",":443"]}`}, @@ -759,6 +762,7 @@ func TestDeploy_WithHooks(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "abc123container"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Pre-deploy hook. ssh.MockCommand{Match: "docker exec", Output: "migrated 3 tables"}, @@ -827,6 +831,7 @@ func TestDeploy_PreDeployHookFailure(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "hookfailcontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Pre-deploy hook fails. ssh.MockCommand{Match: "docker exec", Output: "ERROR: migration failed", Err: fmt.Errorf("exit status 1")}, @@ -883,6 +888,7 @@ func TestDeploy_PostDeployHookFailure(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "postfailcontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Health check. ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, @@ -949,6 +955,7 @@ func TestDeploy_WithWorkers(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, // Web container. ssh.MockCommand{Match: "docker run", Output: "web123container"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Health check (web). ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, @@ -1036,6 +1043,7 @@ func TestDeploy_NoHealthcheckForWorker(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "web123container"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, ssh.MockCommand{Match: "curl -sf http://localhost:2019/config/apps/http/servers/srv0", Output: `{"listen":[":80",":443"]}`}, @@ -1122,6 +1130,7 @@ func TestDeploy_WorkerStartFailure(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, // Start new containers (both match "docker run"). ssh.MockCommand{Match: "docker run", Output: "new123container"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, // Health check. ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, @@ -1343,6 +1352,7 @@ ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-", Output: ssh.MockCommand{Match: "docker rename", Output: ""}, // Start new containers. ssh.MockCommand{Match: "docker run", Output: "redeploycontainer"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, ssh.MockCommand{Match: "curl -sf http://localhost:2019/config/apps/http/servers/srv0", Output: `{"listen":[":80",":443"]}`}, @@ -1423,6 +1433,7 @@ func TestDeploy_IngressExternalSkipsCaddy(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker run", Output: "web123container"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, // No Caddy mocks — the deploy must not call them. diff --git a/internal/deploy/plan_a_test.go b/internal/deploy/plan_a_test.go index 3129360..435db54 100644 --- a/internal/deploy/plan_a_test.go +++ b/internal/deploy/plan_a_test.go @@ -112,6 +112,7 @@ func TestDeploy_AllCreatesUseResolvedImageID(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "docker image inspect --format '{{.Id}}'", Output: imageID}, ssh.MockCommand{Match: "docker run", Output: "abc123"}, + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"running","Running":true}`}, ssh.MockCommand{Match: "docker inspect", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, diff --git a/internal/deploy/recovery_a_test.go b/internal/deploy/recovery_a_test.go index 59d0ace..8bfbc33 100644 --- a/internal/deploy/recovery_a_test.go +++ b/internal/deploy/recovery_a_test.go @@ -212,8 +212,8 @@ func TestDeploy_HostIngressRunFailure_ItemizesFailedRecovery(t *testing.T) { if err == nil { t.Fatal("expected the deploy to fail") } - if !strings.Contains(err.Error(), "no container is serving") { - t.Fatalf("a total recovery failure must say no container is serving, got: %v", err) + if !strings.Contains(err.Error(), "no predecessor could be restarted") { + t.Fatalf("a total recovery failure must say no predecessor could be restarted, got: %v", err) } if !strings.Contains(buf.String(), "cleanup incomplete") { t.Error("failed compensations must be itemized in the output") @@ -251,3 +251,4 @@ func TestLogDeploy_RecordsImage(t *testing.T) { t.Errorf("log entry image: got %q want myapp:latest", entry.Image) } } + diff --git a/internal/deploy/rollback.go b/internal/deploy/rollback.go index 67e5acd..0894be7 100644 --- a/internal/deploy/rollback.go +++ b/internal/deploy/rollback.go @@ -388,6 +388,19 @@ func Rollback(ctx context.Context, exec ssh.Executor, out io.Writer, cfg Rollbac restoreDisplaced() return err } + // failRoutePhase unwinds a route-phase failure the same way a + // health/start failure unwinds (audit T06): the upstream-port + // inspections and SetRoute/SetLoadBalancerHealth used to return + // directly, leaving the uncommitted target running and — under + // fixed ports, where Caddy still points at the STOPPED current + // container — the app dark. + failRoutePhase := func(reason error) error { + for _, name := range started { + dk.Stop(ctx, name, 5) + } + restoreDisplaced() + return reason + } tls := caddy.TLS{Cert: cfg.TLSCert, Key: cfg.TLSKey, Internal: cfg.TLSInternal} // The Caddy upstream port is the recorded primary container port // when there is one (TCL-14); without a record the first exposed @@ -403,21 +416,21 @@ func Rollback(ctx context.Context, exec ssh.Executor, out io.Writer, cfg Rollbac for _, c := range targetWeb { port, err := upstreamPort(c.Name) if err != nil { - return fmt.Errorf("inspecting target container port: %w", err) + return failRoutePhase(fmt.Errorf("inspecting target container port: %w", err)) } upstreams = append(upstreams, caddy.Upstream{Dial: fmt.Sprintf("%s:%d", c.Name, port)}) } if err := cd.SetLoadBalancerHealth(ctx, cfg.App, cfg.Domain, upstreams, healthCfg.Path, tls, cfg.CaddyExtra, cfg.Cache, cfg.Firewall, cfg.Access); err != nil { - return fmt.Errorf("updating load balancer route: %w", err) + return failRoutePhase(fmt.Errorf("updating load balancer route: %w", err)) } fmt.Fprintf(out, " Traffic load-balanced across %d replicas\n", len(targetWeb)) } else { port, err := upstreamPort(targetWeb[0].Name) if err != nil { - return fmt.Errorf("inspecting target container port: %w", err) + return failRoutePhase(fmt.Errorf("inspecting target container port: %w", err)) } if err := cd.SetRoute(ctx, cfg.App, cfg.Domain, targetWeb[0].Name, port, tls, cfg.CaddyExtra, cfg.Cache, cfg.Firewall, cfg.Access); err != nil { - return fmt.Errorf("updating route: %w", err) + return failRoutePhase(fmt.Errorf("updating route: %w", err)) } fmt.Fprintln(out, " Traffic routed to target version") } diff --git a/internal/deploy/rollback_test.go b/internal/deploy/rollback_test.go index d9eca01..2e667a3 100644 --- a/internal/deploy/rollback_test.go +++ b/internal/deploy/rollback_test.go @@ -651,3 +651,47 @@ func TestRollback_CaddyIngressStillAvoidsTheLivePort(t *testing.T) { } func osReadFile(name string) ([]byte, error) { return os.ReadFile(name) } + +// TestRollback_RoutePhaseFailureUnwinds is the T06 regression: a route-phase +// failure (SetRoute here) after the target restarted used to return +// directly, leaving the uncommitted target running. It must unwind exactly +// like a health failure: stop what this rollback started. +func TestRollback_RoutePhaseFailureUnwinds(t *testing.T) { + stateContent := `{"schema_version":2,"deployment_type":"container","ingress_mode":"caddy","domain":"myapp.com","current_port":49153,"current_hash":"v2","previous_port":49152,"previous_hash":"v1"}` + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "present\n" + stateContent}, + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='myapp'", + Output: `{"ID":"aaa","Names":"myapp-web-v1","Image":"myapp:latest","State":"exited","Status":"Exited","Labels":{"teploy.app":"myapp","teploy.version":"v1","teploy.process":"web"}}` + "\n" + + `{"ID":"bbb","Names":"myapp-web-v2","Image":"myapp:latest","State":"running","Status":"Up 1h","Labels":{"teploy.app":"myapp","teploy.version":"v2","teploy.process":"web"}}`, + }, + ssh.MockCommand{Match: "docker inspect 'myapp-web-v1'", Output: `[{"Config":{"Image":"myapp:latest"},"HostConfig":{"NetworkMode":"teploy","PortBindings":{"3000/tcp":[{"HostIp":"127.0.0.1","HostPort":"49152"}]},"RestartPolicy":{"Name":"no"}},"NetworkSettings":{"Networks":{"teploy":{"Aliases":["myapp"]}}}}]`}, + ssh.MockCommand{Match: "docker rm -f 'myapp-web-v1'", Output: ""}, + ssh.MockCommand{Match: "docker run", Output: ""}, + ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostIp}}", Output: "127.0.0.1 "}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}", Output: "49152"}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $_ := .NetworkSettings.Ports}}", Output: "3000/tcp"}, + // The route update fails: the Caddyfile cannot even be read. + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Err: fmt.Errorf("no such file")}, + ssh.MockCommand{Match: "docker stop", Output: ""}, + ) + var buf bytes.Buffer + err := Rollback(context.Background(), mock, &buf, rollbackCfg()) + if err == nil { + t.Fatal("expected the route-phase failure to fail the rollback") + } + if !strings.Contains(err.Error(), "updating route") { + t.Fatalf("expected the route failure to be surfaced, got: %v", err) + } + var stoppedUncommitted bool + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker stop -t 5 'myapp-web-v1'") { + stoppedUncommitted = true + } + } + if !stoppedUncommitted { + t.Error("the uncommitted target container was left running after the route failure") + } +} From 67c097b1e26c01c538b9e31b9f2e13c8da2c5900 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:38:26 -0700 Subject: [PATCH 05/13] =?UTF-8?q?fix(releasemeta,deploy):=20T10+T11+T56=20?= =?UTF-8?q?=E2=80=94=20asset=20seed=20selection,=20bounded=20attempt=20ret?= =?UTF-8?q?ention,=20asset=20cleanup=20on=20the=20live=20tree,=20record=20?= =?UTF-8?q?identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T10: the asset-bridge seed selector is mtime-ordered and skips attempts whose assets directory does not exist — the lexicographically-greatest pick could select an env-only or build-only attempt and silently seed nothing, dropping the cached asset files older releases accumulated. T11: PruneAttempts bounds the attempts retained per KEPT hash to the two newest (records name the newest attempt of their hash; the second covers a lockless same-hash build racing the committed deploy), so repeated same-version or failed attempts no longer retain build trees, env files, and certificates forever. asset_keep_days cleanup now runs on the LIVE attempt-scoped tree (the one the container mounts) in addition to the legacy shared path — it had been a no-op for every deploy since F08. T56: releasemeta.Read validates the record's embedded App/Hash against the requested key — a copied, migrated, or corrupted-but-valid record can no longer drive rollback/recreate effects at a different release's spec. --- internal/deploy/attempt_prune_test.go | 8 +-- internal/deploy/deploy.go | 25 +++++-- internal/deploy/deploy_test.go | 4 +- internal/deploy/fence_test.go | 4 +- internal/releasemeta/attempt.go | 96 ++++++++++++++++----------- internal/releasemeta/attempt_test.go | 70 +++++++++++++++++-- internal/releasemeta/releasemeta.go | 9 +++ 7 files changed, 155 insertions(+), 61 deletions(-) diff --git a/internal/deploy/attempt_prune_test.go b/internal/deploy/attempt_prune_test.go index fb90244..77b5ae3 100644 --- a/internal/deploy/attempt_prune_test.go +++ b/internal/deploy/attempt_prune_test.go @@ -24,8 +24,8 @@ func deployWithAttemptPruneMocks(t *testing.T, pinsStub, inventoryStub ssh.MockC mocks = append(mocks, pinsStub, inventoryStub, - ssh.MockCommand{Match: "ls -1 /deployments/fency/meta/att", Output: "ancient.0000000000000003"}, - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "ls -1t /deployments/fency/meta/att", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "ls -1t /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, ssh.MockCommand{Match: "rm -rf ", Output: ""}, ) mock := ssh.NewMockExecutor("1.2.3.4", mocks...) @@ -60,7 +60,7 @@ func TestDeployFenced_PinReadFailureSkipsAttemptPrune(t *testing.T) { if strings.HasPrefix(c, "rm -rf ") && strings.Contains(c, "/meta/att") { t.Errorf("attempt artifacts must not be pruned when pins cannot be read: %s", c) } - if strings.HasPrefix(c, "ls -1 /deployments/fency/meta/att") { + if strings.HasPrefix(c, "ls -1t /deployments/fency/meta/att") { t.Errorf("the prune sweep must not even run when pins cannot be read: %s", c) } } @@ -93,7 +93,7 @@ func TestDeployFenced_InventoryFailureSkipsAttemptPrune(t *testing.T) { ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Err: errBoom}, ) for _, c := range mock.Calls { - if strings.HasPrefix(c, "ls -1 /deployments/fency/meta/att") { + if strings.HasPrefix(c, "ls -1t /deployments/fency/meta/att") { t.Errorf("the prune sweep must not run when the version inventory is unreadable: %s", c) } } diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 350f3d6..588cc4c 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -338,8 +338,9 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) // Extraction uses `docker create` + `docker cp`, so no image ENTRYPOINT // ever executes (the old `docker run … sh -c` let an image with an // ENTRYPOINT wrap or replace the copy command). + assetAttempt := releasemeta.MustAttempt(cfg.App, cfg.Version) if cfg.AssetPath != "" { - att := releasemeta.MustAttempt(cfg.App, cfg.Version) + att := assetAttempt assetDir := att.Dir() + "/assets" fmt.Fprintln(d.out, "Bridging assets...") seed := "" @@ -843,17 +844,27 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) } } - // 15. Clean up old bridged assets. + // 15. Clean up old bridged assets (asset_keep_days). The LIVE tree the + // container mounts is the attempt's private tree (A15), so the expiry + // runs THERE — the old cleanup targeted only the legacy shared + // /deployments//assets path, which made asset_keep_days a no-op + // for every deploy since F08 (audit T11). The legacy tree still serves + // releases deployed before attempt scoping, so it keeps its sweep too. if cfg.AssetPath != "" { keepDays := cfg.AssetKeepDays if keepDays <= 0 { keepDays = 7 } - cleanCmd := fmt.Sprintf( - "find %s -type f -mtime +%d -delete 2>/dev/null || true", - ssh.ShellQuote(fmt.Sprintf("/deployments/%s/assets", cfg.App)), keepDays, - ) - d.exec.Run(ctx, cleanCmd) + for _, assetRoot := range []string{ + assetAttempt.Dir() + "/assets", + fmt.Sprintf("/deployments/%s/assets", cfg.App), + } { + cleanCmd := fmt.Sprintf( + "find %s -type f -mtime +%d -delete 2>/dev/null || true", + ssh.ShellQuote(assetRoot), keepDays, + ) + d.exec.Run(ctx, cleanCmd) + } } // 15b. Prune superseded app versions (containers + images) if the diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index 6a558b2..c77562f 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -1194,7 +1194,7 @@ func TestDeploy_AssetBridging(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, // 5. Asset bridging: attempt-scoped tree (A15) + create/cp extraction. ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/meta/att/abc123.", Output: ""}, - ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: ""}, + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: ""}, ssh.MockCommand{Match: "docker rm -f 'teploy-assets-", Output: ""}, ssh.MockCommand{Match: "docker create --name 'teploy-assets-", Output: "extractcontainer"}, ssh.MockCommand{Match: "docker cp 'teploy-assets-", Output: ""}, @@ -1291,7 +1291,7 @@ func TestDeploy_AssetBridgingCustomKeepDays(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/assets'", Output: ""}, ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/meta/att/abc123.", Output: ""}, - ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: ""}, + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: ""}, ssh.MockCommand{Match: "docker rm -f 'teploy-assets-", Output: ""}, ssh.MockCommand{Match: "docker create --name 'teploy-assets-", Output: "extractcontainer"}, ssh.MockCommand{Match: "docker cp 'teploy-assets-", Output: ""}, diff --git a/internal/deploy/fence_test.go b/internal/deploy/fence_test.go index d85fcae..265dd5c 100644 --- a/internal/deploy/fence_test.go +++ b/internal/deploy/fence_test.go @@ -121,8 +121,8 @@ func TestDeployFenced_PrunesSupersededAttempts(t *testing.T) { mocks = append(mocks, ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/pinned' ]", Output: "absent"}, ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Output: ""}, - ssh.MockCommand{Match: "ls -1 /deployments/fency/meta/att", Output: "ancient.0000000000000003\nstray"}, - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "ls -1t /deployments/fency/meta/att", Output: "ancient.0000000000000003\nstray"}, + ssh.MockCommand{Match: "ls -1t /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, ssh.MockCommand{Match: "rm -rf ", Output: ""}, ) mock := ssh.NewMockExecutor("1.2.3.4", mocks...) diff --git a/internal/releasemeta/attempt.go b/internal/releasemeta/attempt.go index f0782f2..dc8a9c9 100644 --- a/internal/releasemeta/attempt.go +++ b/internal/releasemeta/attempt.go @@ -50,7 +50,6 @@ import ( "encoding/hex" "fmt" "regexp" - "sort" "strings" "github.com/useteploy/teploy/internal/config" @@ -143,10 +142,11 @@ func attemptRoot(app string) string { return fmt.Sprintf("%s/%s/meta/att", deploymentsDir, app) } -// listAttempts lists attempt directory names under root ("" when the -// directory does not exist yet — a first deploy). -func listAttempts(ctx context.Context, exec ssh.Executor, root string) ([]string, error) { - out, err := exec.Run(ctx, "ls -1 "+root+" 2>/dev/null || true") +// listAttemptsByMtime lists attempt directory names newest-first (mtime +// order) — the closest thing to chronology the id gives us (the random ids +// sort lexicographically, which is NOT recency). +func listAttemptsByMtime(ctx context.Context, exec ssh.Executor, root string) ([]string, error) { + out, err := exec.Run(ctx, "ls -1t "+root+" 2>/dev/null || true") if err != nil { return nil, fmt.Errorf("listing attempts under %s: %w", root, err) } @@ -159,12 +159,23 @@ func listAttempts(ctx context.Context, exec ssh.Executor, root string) ([]string return names, nil } +// keepAttemptsPerHash bounds how many attempts of a RETAINED hash stay on +// disk (audit T11). One would be the record's own reference (records always +// name the newest attempt of their hash — a same-version redeploy rewrites +// the record with its attempt); two also cover a lockless `teploy build` +// creating a newer attempt of the same hash after the deploy committed, so +// pruning "older" can never delete the attempt a live record references. +const keepAttemptsPerHash = 2 + // PruneAttempts removes the attempt directories (artifact root and the -// app's TLS root) of every release hash NOT in keepHashes. Entries whose -// names do not parse as . are kept — an unparsable name is not -// proof the attempt is prunable (F78's rule). Only THIS app's roots are -// swept; the legacy flat TLS root is never touched (A01). Removal failures -// are returned; callers treat pruning as best-effort. +// app's TLS root) of every release hash NOT in keepHashes, and bounds the +// attempts retained per KEPT hash to the newest keepAttemptsPerHash — +// repeated same-version or failed attempts used to retain build trees, env +// files, and certificates indefinitely (audit T11). Entries whose names do +// not parse as . are kept — an unparsable name is not proof the +// attempt is prunable (F78's rule). Only THIS app's roots are swept; the +// legacy flat TLS root is never touched (A01). Removal failures are +// returned; callers treat pruning as best-effort. func PruneAttempts(ctx context.Context, exec ssh.Executor, app string, keepHashes ...string) error { keep := make(map[string]bool, len(keepHashes)) for _, h := range keepHashes { @@ -174,15 +185,22 @@ func PruneAttempts(ctx context.Context, exec ssh.Executor, app string, keepHashe } var failures []string for _, root := range []string{attemptRoot(app), tlsAttemptRootFor(app)} { - names, err := listAttempts(ctx, exec, root) + names, err := listAttemptsByMtime(ctx, exec, root) if err != nil { return err } - for _, name := range names { + keptForHash := make(map[string]int) + for _, name := range names { // newest first m := attemptDirRE.FindStringSubmatch(name) - if m == nil || keep[m[1]] { + if m == nil { continue } + if keep[m[1]] { + keptForHash[m[1]]++ + if keptForHash[m[1]] <= keepAttemptsPerHash { + continue + } + } if _, rmErr := exec.Run(ctx, "rm -rf "+ssh.ShellQuote(root+"/"+name)); rmErr != nil { failures = append(failures, root+"/"+name) } @@ -205,39 +223,39 @@ func PreviousAttemptBuildDir(ctx context.Context, exec ssh.Executor, app, exclud } // PreviousAttemptAssetsDir returns the assets directory of the most recent -// other attempt, when one exists — the SEED for this attempt's private -// asset tree (audit A15): asset bridging must not mutate the live shared -// tree a running release still reads. Empty when there is none. +// other attempt THAT HAS ONE — the SEED for this attempt's private asset +// tree (audit A15): asset bridging must not mutate the live shared tree a +// running release still reads. Empty when there is none. +// +// The candidate set is mtime-ordered and existence-filtered (T10): the +// previous attempt by recency may be an env-only or build-only attempt with +// NO assets directory, and seeding from an empty set silently dropped the +// cached asset files older releases accumulated — the bridge copied only +// what the new image re-extracted. func PreviousAttemptAssetsDir(ctx context.Context, exec ssh.Executor, app, excludeID string) string { - dir := previousAttemptSubDir(ctx, exec, app, excludeID, "assets") - if dir == "" { - return "" - } - // Only a directory that provably exists is a usable seed; anything - // else means "no previous tree" (full extraction), not an error. - if out, err := exec.Run(ctx, "test -d "+ssh.ShellQuote(dir)+" && echo yes || echo no"); err != nil || strings.TrimSpace(out) != "yes" { - return "" - } - return dir + return previousAttemptSubDir(ctx, exec, app, excludeID, "assets") } +// previousAttemptSubDir picks the newest (mtime) other attempt whose +// directory provably exists. Existence filtering matters most for assets +// (an empty seed is a silent loss) and is harmless for the rsync +// --link-dest basis — a basis that does not exist transfers in full anyway. +// The id is random, so lexicographic order is NOT recency; mtime is the +// closest chronology the directory names offer. func previousAttemptSubDir(ctx context.Context, exec ssh.Executor, app, excludeID, sub string) string { - names, err := listAttempts(ctx, exec, attemptRoot(app)) + names, err := listAttemptsByMtime(ctx, exec, attemptRoot(app)) if err != nil { return "" } - filtered := names[:0] - for _, n := range names { - if m := attemptDirRE.FindStringSubmatch(n); m != nil && m[2] != excludeID { - filtered = append(filtered, n) + for _, n := range names { // newest first + m := attemptDirRE.FindStringSubmatch(n) + if m == nil || m[2] == excludeID { + continue + } + candidate := attemptRoot(app) + "/" + n + "/" + sub + if out, err := exec.Run(ctx, "test -d "+ssh.ShellQuote(candidate)+" && echo yes || echo no"); err == nil && strings.TrimSpace(out) == "yes" { + return candidate } } - if len(filtered) == 0 { - return "" - } - // Deterministic pick: lexicographically greatest name. The id is - // random, so this is not chronology — it does not need to be; any - // recent-ish basis gives rsync its delta. - sort.Strings(filtered) - return attemptRoot(app) + "/" + filtered[len(filtered)-1] + "/" + sub + return "" } diff --git a/internal/releasemeta/attempt_test.go b/internal/releasemeta/attempt_test.go index a2fa943..849169a 100644 --- a/internal/releasemeta/attempt_test.go +++ b/internal/releasemeta/attempt_test.go @@ -68,14 +68,14 @@ func TestPruneAttempts_ProtectsKeepSetAndUnparsable(t *testing.T) { // Artifact root listing: a protected current attempt, a protected // previous attempt, an unprotected old one, and an unparsable name // that must be KEPT (fail closed, F78 parity). - ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: strings.Join([]string{ + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: strings.Join([]string{ "newhash.0000000000000001", "oldhash.0000000000000002", "ancient.0000000000000003", "stray-directory", }, "\n")}, // The app's OWN TLS root (A01: /deployments/caddy/tls/att/). - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/myapp", Output: strings.Join([]string{ + ssh.MockCommand{Match: "ls -1t /deployments/caddy/tls/att/myapp", Output: strings.Join([]string{ "ancient.0000000000000003", }, "\n")}, ssh.MockCommand{Match: "rm -rf", Output: ""}, @@ -106,10 +106,10 @@ func TestPruneAttempts_ProtectsKeepSetAndUnparsable(t *testing.T) { // cannot be attributed to an owning app. func TestPruneAttempts_NeverTouchesOtherAppsOrLegacyFlatRoot(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "ls -1 /deployments/app-a/meta/att", Output: strings.Join([]string{ + ssh.MockCommand{Match: "ls -1t /deployments/app-a/meta/att", Output: strings.Join([]string{ "shared-name.0000000000000001", // A's prunable attempt... }, "\n")}, - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/app-a", Output: strings.Join([]string{ + ssh.MockCommand{Match: "ls -1t /deployments/caddy/tls/att/app-a", Output: strings.Join([]string{ "shared-name.0000000000000001", // ...in both of A's roots }, "\n")}, ssh.MockCommand{Match: "rm -rf", Output: ""}, @@ -132,8 +132,8 @@ func TestPruneAttempts_NeverTouchesOtherAppsOrLegacyFlatRoot(t *testing.T) { // root or app-b's root proves the sweep reached beyond A's namespace. for _, c := range mock.Calls { if strings.HasPrefix(c, "ls -1 ") { - if c != "ls -1 /deployments/app-a/meta/att 2>/dev/null || true" && - c != "ls -1 /deployments/caddy/tls/att/app-a 2>/dev/null || true" { + if c != "ls -1t /deployments/app-a/meta/att 2>/dev/null || true" && + c != "ls -1t /deployments/caddy/tls/att/app-a 2>/dev/null || true" { t.Errorf("attempt sweep listed a namespace it must not touch: %s", c) } } @@ -156,11 +156,12 @@ func TestPruneAttempts_AbsentRootsAreNoops(t *testing.T) { func TestPreviousAttemptBuildDir(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: strings.Join([]string{ + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: strings.Join([]string{ "aaa.0000000000000001", "bbb.0000000000000002", "not-an-attempt", }, "\n")}, + ssh.MockCommand{Match: "test -d ", Output: "yes"}, ) got := PreviousAttemptBuildDir(context.Background(), mock, "myapp", "0000000000000002") if want := "/deployments/myapp/meta/att/aaa.0000000000000001/build"; got != want { @@ -172,3 +173,58 @@ func TestPreviousAttemptBuildDir(t *testing.T) { t.Errorf("unexpected basis when 0001 is excluded: %s", got) } } + +// TestPreviousAttemptAssetsDir_SkipsAttemptsWithoutAssets is the T10 +// regression: the most recent attempt may be env-only or build-only (no +// assets directory); seeding from it silently dropped the cached asset +// files older releases accumulated. The selector must skip to the newest +// attempt that actually HAS the subtree. +func TestPreviousAttemptAssetsDir_SkipsAttemptsWithoutAssets(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + // mtime-newest first: newest has no assets, next one does. + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: strings.Join([]string{ + "envonly.0000000000000009", + "withassets.0000000000000008", + }, "\n")}, + ssh.MockCommand{Match: "test -d '/deployments/myapp/meta/att/envonly.0000000000000009/assets'", Output: "no"}, + ssh.MockCommand{Match: "test -d '/deployments/myapp/meta/att/withassets.0000000000000008/assets'", Output: "yes"}, + ) + got := PreviousAttemptAssetsDir(context.Background(), mock, "myapp", "ffffffffffffffff") + if want := "/deployments/myapp/meta/att/withassets.0000000000000008/assets"; got != want { + t.Errorf("PreviousAttemptAssetsDir: got %q want %q (the asset-less newest attempt must be skipped)", got, want) + } +} + +// TestPruneAttempts_BoundsAttemptsPerKeptHash is the T11 regression: +// repeated same-version or failed attempts of a RETAINED hash used to be +// kept forever (every attempt dir of a kept hash was protected). Only the +// newest keepAttemptsPerHash attempts of a kept hash stay. +func TestPruneAttempts_BoundsAttemptsPerKeptHash(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "ls -1t /deployments/myapp/meta/att", Output: strings.Join([]string{ + "kept.0000000000000005", // newest + "kept.0000000000000004", + "kept.0000000000000003", + "kept.0000000000000002", + }, "\n")}, + ssh.MockCommand{Match: "ls -1t /deployments/caddy/tls/att/myapp", Output: ""}, + ssh.MockCommand{Match: "rm -rf", Output: ""}, + ) + if err := PruneAttempts(context.Background(), mock, "myapp", "kept"); err != nil { + t.Fatalf("PruneAttempts: %v", err) + } + var removed []string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "rm -rf ") { + removed = append(removed, c) + } + } + if len(removed) != 2 { + t.Fatalf("expected the 2 oldest attempts of the kept hash pruned, got %v", removed) + } + for _, r := range removed { + if !strings.Contains(r, "kept.0000000000000002") && !strings.Contains(r, "kept.0000000000000003") { + t.Errorf("pruned a newest-two attempt: %s", r) + } + } +} diff --git a/internal/releasemeta/releasemeta.go b/internal/releasemeta/releasemeta.go index 12f5da5..9395816 100644 --- a/internal/releasemeta/releasemeta.go +++ b/internal/releasemeta/releasemeta.go @@ -188,6 +188,15 @@ func Read(ctx context.Context, exec ssh.Executor, app, hash string) (*Record, er if rec.SchemaVersion != SchemaVersion { return nil, fmt.Errorf("unsupported release-metadata schema version %d for %s@%s", rec.SchemaVersion, app, hash) } + // Identity check (audit T56): the record loaded from (app, hash)'s path + // must actually DESCRIBE (app, hash). An accidentally copied, partially + // migrated, or corrupted-but-valid record used to be accepted on schema + // alone and could drive rollback/recreate effects at a different + // release's spec. Every record this package writes carries both fields + // (Write requires them), so a mismatch is never a legacy artifact. + if rec.App != app || rec.Hash != hash { + return nil, fmt.Errorf("release record identity mismatch: requested %s@%s, record describes %s@%s — refusing to use it", app, hash, rec.App, rec.Hash) + } return &rec, nil } From a602edc92f7e8caf310d98aa8a5088a9f8121813 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:41:00 -0700 Subject: [PATCH 06/13] =?UTF-8?q?fix(ssh,secret):=20T41+T45+T46=20?= =?UTF-8?q?=E2=80=94=20no-directory-operand=20publication,=20find=20status?= =?UTF-8?q?=20observed,=20local=20parent-dir=20fsync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T45: every atomic publication on the supported GNU/Linux target renames with mv -fT (no target-directory semantics) — remote Upload's staged script, UploadAtomic, and secret Set. A plain 'mv -f -- tmp dest' with the destination a symlink TO A DIRECTORY succeeded by nesting the staged file inside it while the expected destination stayed unchanged, a false-success write. T41: secret List runs a bare find with its exit status observed and sorts in Go — the old 'find | sort' pipeline had no pipefail, so a failed find reported 'no secrets' and deploys proceeded without the app's secrets. Listed names are validated against the key grammar at the source. T46 (local half): LocalExecutor.Upload fsyncs the containing directory after the rename, completing the local crash-durability contract (file synced, rename persisted). The remote-shell fsync contract stays deferred with the durability family. --- internal/caddy/tcl_round2_test.go | 4 ++-- internal/secret/secret.go | 17 +++++++++++++++-- internal/secret/secret_test.go | 14 ++++++++++++++ internal/ssh/executor.go | 7 ++++++- internal/ssh/local.go | 9 +++++++++ internal/ssh/remote.go | 2 +- internal/state/pins_test.go | 4 ++-- internal/state/state_test.go | 2 +- 8 files changed, 50 insertions(+), 9 deletions(-) diff --git a/internal/caddy/tcl_round2_test.go b/internal/caddy/tcl_round2_test.go index 657d216..dcda441 100644 --- a/internal/caddy/tcl_round2_test.go +++ b/internal/caddy/tcl_round2_test.go @@ -61,8 +61,8 @@ func (f *fakeStatefulExecutor) Run(ctx context.Context, cmd string) (string, err case strings.HasPrefix(cmd, "rm -f "): delete(f.files, strings.Trim(strings.TrimPrefix(cmd, "rm -f "), "'")) return "", nil - case strings.HasPrefix(cmd, "mv -f -- "): - fields := strings.Fields(strings.TrimPrefix(cmd, "mv -f -- ")) + case strings.HasPrefix(cmd, "mv -f -- "), strings.HasPrefix(cmd, "mv -fT -- "): + fields := strings.Fields(strings.TrimPrefix(strings.TrimPrefix(cmd, "mv -fT -- "), "mv -f -- ")) if len(fields) == 2 { src := strings.Trim(fields[0], "'") dst := strings.Trim(fields[1], "'") diff --git a/internal/secret/secret.go b/internal/secret/secret.go index 3382b18..63eb305 100644 --- a/internal/secret/secret.go +++ b/internal/secret/secret.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "regexp" + "sort" "strings" "github.com/useteploy/teploy/internal/ssh" @@ -189,7 +190,7 @@ func (m *Manager) Set(ctx context.Context, app, key, value string) error { // temporary file; only after a successful encryption is it renamed over // the destination. script := fmt.Sprintf( - `umask 077 && tmp=$(mktemp %s) && trap 'rm -f -- "$tmp"' EXIT HUP INT TERM && age -r %s -o "$tmp" && chmod 0600 "$tmp" && mv -f -- "$tmp" %s && trap - EXIT HUP INT TERM`, + `umask 077 && tmp=$(mktemp %s) && trap 'rm -f -- "$tmp"' EXIT HUP INT TERM && age -r %s -o "$tmp" && chmod 0600 "$tmp" && mv -fT -- "$tmp" %s && trap - EXIT HUP INT TERM`, ssh.ShellQuote(dir+"/.teploy-secret.XXXXXXXX"), ssh.ShellQuote(recipient), ssh.ShellQuote(path), @@ -243,6 +244,11 @@ func diagSuffix(diag string) string { // is the normal "no secrets" case (nil, nil); a directory that exists but // cannot be listed is an error — silently treating it as empty would let a // deployment proceed without secrets it actually has. +// +// The listing is a BARE find with its exit status observed (audit T41): the +// old `find … | sort` pipeline lost find's failure to sort's success (no +// pipefail), so an unlistable directory returned an empty list that read as +// "this app has no secrets". Sorting happens in Go. func (m *Manager) List(ctx context.Context, app string) ([]string, error) { dir := secretDir(app) exists, err := remoteFileExists(ctx, m.exec, dir) @@ -252,7 +258,7 @@ func (m *Manager) List(ctx context.Context, app string) ([]string, error) { if !exists { return nil, nil } - out, err := m.exec.Run(ctx, fmt.Sprintf("find %s -maxdepth 1 -name '*.age' -printf '%%f\\n' | sort", ssh.ShellQuote(dir))) + out, err := m.exec.Run(ctx, fmt.Sprintf("find %s -maxdepth 1 -name '*.age' -printf '%%f\\n'", ssh.ShellQuote(dir))) if err != nil { return nil, fmt.Errorf("listing secrets for %s: %w", app, err) } @@ -264,8 +270,15 @@ func (m *Manager) List(ctx context.Context, app string) ([]string, error) { continue } name := strings.TrimSuffix(line, ".age") + // A file whose name is not a valid key cannot be addressed by this + // package's path-building grammar — report it instead of handing + // callers a key that fails later at a random sink. + if err := ValidateKey(name); err != nil { + return nil, fmt.Errorf("invalid entry in %s: %w", dir, err) + } keys = append(keys, name) } + sort.Strings(keys) return keys, nil } diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go index d3d2ec1..1a74e47 100644 --- a/internal/secret/secret_test.go +++ b/internal/secret/secret_test.go @@ -292,3 +292,17 @@ func (m *stderrOnlyMock) RunStream(ctx context.Context, cmd string, stdout, stde _, err := stderr.Write([]byte(m.stderrOut)) return err } + +// TestList_FindFailureIsAnError is the T41 regression: the old +// `find … | sort` pipeline lost find's failure to sort's success (no +// pipefail), so an unlistable secrets directory returned an empty list that +// read as "this app has no secrets" and DeployAll skipped every secret. +func TestList_FindFailureIsAnError(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/secrets' ]", Output: "present"}, + ssh.MockCommand{Match: "find ", Err: fmt.Errorf("find: '/deployments/myapp/secrets': Permission denied")}, + ) + if _, err := NewManager(mock).List(context.Background(), "myapp"); err == nil { + t.Fatal("a failed find must be an error, never an empty secret list") + } +} diff --git a/internal/ssh/executor.go b/internal/ssh/executor.go index 2b034ca..6ddb5f6 100644 --- a/internal/ssh/executor.go +++ b/internal/ssh/executor.go @@ -59,7 +59,12 @@ func UploadAtomic(ctx context.Context, exec Executor, content io.Reader, remoteP if err := exec.Upload(ctx, content, tmpPath, mode); err != nil { return fmt.Errorf("uploading temporary file: %w", err) } - if _, err := exec.Run(ctx, "mv -f -- "+ShellQuote(tmpPath)+" "+ShellQuote(remotePath)); err != nil { + // -T (no-target-directory): the destination is one path, never a + // directory operand. A plain `mv -f -- tmp dest` with dest a symlink TO + // A DIRECTORY succeeds by moving the file INSIDE that directory and + // leaving the expected destination untouched — a false-success write + // (audit T45). + if _, err := exec.Run(ctx, "mv -fT -- "+ShellQuote(tmpPath)+" "+ShellQuote(remotePath)); err != nil { return fmt.Errorf("renaming temporary file into place: %w", err) } committed = true diff --git a/internal/ssh/local.go b/internal/ssh/local.go index 3cc9791..414eb90 100644 --- a/internal/ssh/local.go +++ b/internal/ssh/local.go @@ -109,6 +109,15 @@ func (e *LocalExecutor) Upload(ctx context.Context, content io.Reader, path stri if err := os.Rename(tmp, path); err != nil { return fmt.Errorf("publishing %s: %w", path, err) } + // Sync the containing directory after the rename (T46's local half): + // the file itself was fsynced above, but without a directory fsync a + // power loss can leave the rename unpersisted — the old file back, or + // nothing. The remote-shell halves stay on the deferred durability + // contract (documented in AUDIT_OPEN.md). + if d, derr := os.Open(dir); derr == nil { + _ = d.Sync() + _ = d.Close() + } return nil } diff --git a/internal/ssh/remote.go b/internal/ssh/remote.go index 45ee166..f68ffe1 100644 --- a/internal/ssh/remote.go +++ b/internal/ssh/remote.go @@ -195,7 +195,7 @@ func (e *RemoteExecutor) Upload(ctx context.Context, content io.Reader, remotePa dir := path.Dir(remotePath) script := fmt.Sprintf( - `umask 077 && mkdir -p %s && tmp=$(mktemp %s) && trap 'rm -f -- "$tmp"' EXIT HUP INT TERM && chmod %s "$tmp" && cat > "$tmp" && mv -f -- "$tmp" %s && trap - EXIT HUP INT TERM`, + `umask 077 && mkdir -p %s && tmp=$(mktemp %s) && trap 'rm -f -- "$tmp"' EXIT HUP INT TERM && chmod %s "$tmp" && cat > "$tmp" && mv -fT -- "$tmp" %s && trap - EXIT HUP INT TERM`, ShellQuote(dir), ShellQuote(dir+"/.teploy-upload.XXXXXXXX"), ShellQuote(mode), diff --git a/internal/state/pins_test.go b/internal/state/pins_test.go index 9640929..ee146ff 100644 --- a/internal/state/pins_test.go +++ b/internal/state/pins_test.go @@ -35,8 +35,8 @@ func (f *fakeFS) Run(ctx context.Context, cmd string) (string, error) { return f.files[path], nil case strings.HasPrefix(cmd, "mkdir -p"), strings.HasPrefix(cmd, "mkdir "): return "", nil - case strings.HasPrefix(cmd, "mv -f -- "): - fields := strings.Fields(strings.TrimPrefix(cmd, "mv -f -- ")) + case strings.HasPrefix(cmd, "mv -f -- "), strings.HasPrefix(cmd, "mv -fT -- "): + fields := strings.Fields(strings.TrimPrefix(strings.TrimPrefix(cmd, "mv -fT -- "), "mv -f -- ")) if len(fields) == 2 { if data, ok := f.files[strings.Trim(fields[0], "'")]; ok { f.files[strings.Trim(fields[1], "'")] = data diff --git a/internal/state/state_test.go b/internal/state/state_test.go index bcf4c73..9758607 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -166,7 +166,7 @@ func TestWrite_UploadFailurePreservesExistingStateAndCleansTemp(t *testing.T) { func TestWrite_RenameFailurePreservesExistingStateAndCleansTemp(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "mv -f -- '/deployments/myapp/state.json.tmp-", Err: fmt.Errorf("rename failed")}, + ssh.MockCommand{Match: "mv -fT -- '/deployments/myapp/state.json.tmp-", Err: fmt.Errorf("rename failed")}, ) mock.Files["/deployments/myapp/state.json"] = []byte(`{"schema_version":2,"current_hash":"old"}`) From 353dc937483d3988dd3c337015f21064e8d674d1 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:44:26 -0700 Subject: [PATCH 07/13] =?UTF-8?q?fix(build,backup):=20T51+T37+T38=20?= =?UTF-8?q?=E2=80=94=20protected=20ignore=20defaults,=20redis=20recovery?= =?UTF-8?q?=20armed=20before=20the=20baseline=20copy,=20post-stop=20baseli?= =?UTF-8?q?ne?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T51: .teployignore now EXTENDS the always-protected defaults (node_modules, .git, .env, .env.*) instead of replacing them — one custom pattern used to ship .env and .git to the build host where a broad COPY bakes them into the image; an unreadable ignore file is an error rather than a silent defaults-only transfer. T37: the redis restore script defines restore_original and captures the baseline AFTER the stop, with the capture's failure explicitly compensated (restart + abort) — under set -e the old copy exited the script immediately and left Redis stopped with no recovery attempt. T38: the baseline is captured against the STOPPED container (docker cp), distinguishing a proven 'no such file' (nothing to preserve) from every other failure — the old pre-stop existence flag missed the final RDB a graceful shutdown writes when none existed before. The regression tests drive the generated script under a real bash with a stub docker. --- internal/backup/backup.go | 29 ++-- internal/backup/redis_restore_test.go | 219 ++++++++++++++++++++++++++ internal/build/build_test.go | 49 ++++-- internal/build/ignore.go | 32 ++-- internal/cli/build.go | 6 +- internal/cli/deploy.go | 5 +- internal/cli/singledeploy.go | 5 +- 7 files changed, 308 insertions(+), 37 deletions(-) create mode 100644 internal/backup/redis_restore_test.go diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 82b212a..c6b5f44 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -750,25 +750,30 @@ func (c *Client) AccessoryRestore(ctx context.Context, app, name, image, date st if aofFields := strings.Fields(aofOut); len(aofFields) != 2 || aofFields[0] != "appendonly" || aofFields[1] != "no" { return keepTmp(fmt.Errorf("cannot confirm appendonly=no for %s (got %q) — an AOF-enabled Redis would load the append-only file on restart and teploy's dump.rdb restore would be a no-op; an explicit restore plan is required", containerName, strings.TrimSpace(aofOut))) } - // A41 ordering: the previous dump is snapshotted AFTER the stop — - // a graceful redis shutdown writes a final RDB, and the old - // copy-before-stop could miss data present at shutdown, making the - // "recovery copy" older than the state it claims to recover. The - // pre-stop `docker exec test` only records WHETHER a dump exists; - // the copy itself runs on the stopped container (docker cp works - // stopped) and its failure aborts before anything is modified. A - // failed final `docker start` now also puts the original dump back - // and retries the start (the old script exited without either). + // A41 ordering + T37/T38 arming: restore_original is defined (and + // the old-dump capture attempted) AFTER the stop — a graceful redis + // shutdown writes a final RDB, so the pre-stop existence flag could + // miss data present at shutdown. The baseline copy itself + // distinguishes "no such file" (nothing to preserve) from every + // other failure, and ANY failure after the stop restarts the + // container before aborting: the old script's `set -e` exit on a + // failed docker cp left Redis stopped with no recovery attempt. restoreCmd = strings.Join([]string{ "set -eu", fmt.Sprintf("gunzip -c %s > %s", ssh.ShellQuote(restorePath), ssh.ShellQuote(rdbPath)), "had=no", - fmt.Sprintf("if docker exec %s test -f /data/dump.rdb 2>/dev/null; then had=yes; fi", qContainer), + fmt.Sprintf(`restore_original() { if [ "$had" = yes ] && [ -f %s ]; then docker cp %s %s:/data/dump.rdb || true; fi; docker start %s || true; }`, + ssh.ShellQuote(oldRdb), ssh.ShellQuote(oldRdb), qContainer, qContainer), fmt.Sprintf("docker stop %s", qContainer), - fmt.Sprintf(`if [ "$had" = yes ]; then docker cp %s:/data/dump.rdb %s; fi`, qContainer, ssh.ShellQuote(oldRdb)), + // Post-stop baseline (docker cp works on a stopped container): + // success -> had=yes; a proven not-found -> nothing to + // preserve; anything else -> restart + abort. + `cperr=$(mktemp)`, + fmt.Sprintf(`if docker cp %s:/data/dump.rdb %s 2>"$cperr"; then had=yes; elif grep -qi 'no such' "$cperr"; then had=no; else cat "$cperr" >&2; rm -f "$cperr"; restore_original; echo 'capturing the pre-restore dump failed; the container was restarted' >&2; exit 1; fi`, + qContainer, ssh.ShellQuote(oldRdb)), + `rm -f "$cperr"`, "ok=yes", fmt.Sprintf("docker cp %s %s:/data/dump.rdb || ok=no", ssh.ShellQuote(rdbPath), qContainer), - fmt.Sprintf(`restore_original() { if [ "$had" = yes ] && [ -f %s ]; then docker cp %s %s:/data/dump.rdb || true; fi; docker start %s || true; }`, ssh.ShellQuote(oldRdb), ssh.ShellQuote(oldRdb), qContainer, qContainer), `if [ "$ok" != yes ]; then`, ` restore_original`, " echo 'redis restore failed after stopping the container; the original dump was restored when available' >&2", diff --git a/internal/backup/redis_restore_test.go b/internal/backup/redis_restore_test.go new file mode 100644 index 0000000..ed956f9 --- /dev/null +++ b/internal/backup/redis_restore_test.go @@ -0,0 +1,219 @@ +package backup + +import ( + "bytes" + "compress/gzip" + "context" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" +) + +// redisRestoreExec is a behavioral executor for the redis restore branch: +// the scaffolding commands (aws check, mktemp, AOF preflight, s3 download) +// are answered directly, and the generated restore SCRIPT runs under a real +// bash with a stub docker binary on PATH — the T37/T38 regression tests +// exercise the script's actual control flow (arming, baseline capture, +// compensation), not its string shape. +type redisRestoreExec struct { + mu sync.Mutex + calls []string + stubDir string + env []string // extra env for the stub docker (scenario knobs) + dockerFn func(scriptPath string) (string, error) + tmpdirs []string +} + +func (e *redisRestoreExec) Run(ctx context.Context, cmd string) (string, error) { + e.mu.Lock() + e.calls = append(e.calls, cmd) + e.mu.Unlock() + switch { + case strings.HasPrefix(cmd, "which aws"): + return "", nil + case strings.HasPrefix(cmd, "mktemp -d "): + dir, err := os.MkdirTemp("", "teploy-restore-test-") + if err != nil { + return "", err + } + e.tmpdirs = append(e.tmpdirs, dir) + return dir, nil + case strings.HasPrefix(cmd, "docker exec 'myapp-cache' redis-cli --raw config get appendonly"): + return "appendonly\nno", nil + case strings.Contains(cmd, "aws s3 cp "): + // "Download" a real gzip archive so the script's gunzip succeeds. + var out string + for _, f := range strings.Fields(cmd) { + if strings.HasSuffix(f, ".rdb.gz'") || strings.HasSuffix(f, ".rdb.gz") { + out = strings.Trim(f, "'") + } + } + var buf bytes.Buffer + zw := gzip.NewWriter(&buf) + zw.Write([]byte("new-dump")) + zw.Close() + if err := os.WriteFile(out, buf.Bytes(), 0644); err != nil { + return "", err + } + return "", nil + case strings.HasPrefix(cmd, "set -eu"): + script := filepath.Join(e.stubDir, "script.sh") + if err := os.WriteFile(script, []byte(cmd), 0755); err != nil { + return "", err + } + c := exec.CommandContext(ctx, "bash", script) + c.Env = append(append(os.Environ(), "PATH="+e.stubDir+":"+os.Getenv("PATH")), e.env...) + var out bytes.Buffer + c.Stdout = &out + c.Stderr = &out + err := c.Run() + return out.String(), err + case strings.HasPrefix(cmd, "rm -rf "): + path := strings.Trim(strings.TrimPrefix(cmd, "rm -rf "), "'") + os.RemoveAll(path) + return "", nil + } + return "", fmt.Errorf("redisRestoreExec: unexpected command: %s", cmd) +} + +func (e *redisRestoreExec) RunStream(ctx context.Context, cmd string, stdout, stderr io.Writer) error { + out, err := e.Run(ctx, cmd) + if out != "" { + stdout.Write([]byte(out)) + } + return err +} + +func (e *redisRestoreExec) RunInput(ctx context.Context, cmd string, stdin io.Reader) error { + _, err := e.Run(ctx, cmd) + return err +} + +func (e *redisRestoreExec) Upload(ctx context.Context, content io.Reader, remotePath, mode string) error { + return nil +} + +func (e *redisRestoreExec) Close() error { return nil } +func (e *redisRestoreExec) Host() string { return "1.2.3.4" } +func (e *redisRestoreExec) User() string { return "root" } + +// writeDockerStub writes the stub docker binary the restore script drives. +// Scenario knobs (env): HAVE_DUMP=yes/no, FAIL_BASELINE=transport, +// FAIL_INSTALL=yes. +func writeDockerStub(t *testing.T, dir string) string { + t.Helper() + stub := `#!/bin/sh +log="$DOCKER_LOG" +echo "$*" >> "$log" +cmd="$1"; shift +case "$cmd" in + stop) exit 0 ;; + start) exit 0 ;; + exec) printf 'appendonly\nno\n'; exit 0 ;; + cp) + if [ "$1" = "myapp-cache:/data/dump.rdb" ]; then + if [ "$FAIL_BASELINE" = "transport" ]; then echo "transport error" >&2; exit 1; fi + if [ "$HAVE_DUMP" = "yes" ]; then echo old-dump > "$2"; exit 0; fi + echo "Error: No such container/path: myapp-cache:/data/dump.rdb" >&2; exit 1 + fi + if [ "$FAIL_INSTALL" = "yes" ]; then echo "install failed" >&2; exit 1; fi + exit 0 ;; +esac +exit 0 +` + path := filepath.Join(dir, "docker") + if err := os.WriteFile(path, []byte(stub), 0755); err != nil { + t.Fatal(err) + } + return path +} + +func runRedisRestore(t *testing.T, env ...string) (error, string) { + t.Helper() + stubDir := t.TempDir() + writeDockerStub(t, stubDir) + dockerLog := filepath.Join(stubDir, "docker.log") + ex := &redisRestoreExec{stubDir: stubDir, env: append([]string{"DOCKER_LOG=" + dockerLog}, env...)} + c := NewClient(ex, os.Stdout) + err := c.AccessoryRestore(context.Background(), "myapp", "cache", "redis:7", "20260919", nil, S3Config{Bucket: "b", Region: "us-east-1"}) + logBytes, _ := os.ReadFile(dockerLog) + return err, string(logBytes) +} + +// TestRedisRestore_BaselineCaptureFailureRestartsService is the T37 +// regression: a failed pre-restore docker cp (under set -e) used to exit the +// script immediately with Redis stopped and no recovery attempted. +func TestRedisRestore_BaselineCaptureFailureRestartsService(t *testing.T) { + err, log := runRedisRestore(t, "FAIL_BASELINE=transport") + if err == nil { + t.Fatal("the restore must fail when the baseline capture fails") + } + lines := strings.Split(strings.TrimSpace(log), "\n") + var cpIdx, startIdx = -1, -1 + for i, l := range lines { + if strings.Contains(l, "myapp-cache:/data/dump.rdb") && cpIdx == -1 { + cpIdx = i + } + if strings.Contains(l, "start myapp-cache") && startIdx == -1 { + startIdx = i + } + } + if cpIdx == -1 { + t.Fatalf("baseline capture not attempted: %q", log) + } + if startIdx < cpIdx { + t.Fatalf("no restart attempted after the failed baseline capture: %q", log) + } +} + +// TestRedisRestore_PostStopBaselineCapturedWhenPresent pins A41+T38: the +// baseline copy runs against the STOPPED container (after the shutdown +// save), so a dump present at shutdown is preserved and put back when the +// install fails. +func TestRedisRestore_PostStopBaselineCapturedWhenPresent(t *testing.T) { + err, log := runRedisRestore(t, "HAVE_DUMP=yes", "FAIL_INSTALL=yes") + if err == nil { + t.Fatal("the restore must fail when installing the replacement dump fails") + } + // Order: stop → baseline cp (container→host) → failed install cp → + // restore_original's cp back → start. + var stop, baseline, install, restoreCp, restart = -1, -1, -1, -1, -1 + for i, l := range strings.Split(log, "\n") { + switch { + case strings.Contains(l, "stop myapp-cache") && stop == -1: + stop = i + case strings.Contains(l, "myapp-cache:/data/dump.rdb") && baseline == -1: + baseline = i // first container→host copy + case strings.Contains(l, "/data/dump.rdb") && strings.HasSuffix(l, "myapp-cache:/data/dump.rdb") && install == -1 && i > baseline: + install = i + case strings.Contains(l, "myapp-cache:/data/dump.rdb") && i > install && restoreCp == -1: + restoreCp = i + case strings.Contains(l, "start myapp-cache") && restart == -1: + restart = i + } + } + if stop == -1 || baseline == -1 || install == -1 || restoreCp == -1 || restart == -1 { + t.Fatalf("expected stop → baseline → install → restore-back → restart, got: %q", log) + } + if !(stop < baseline && baseline < install && install < restoreCp && restoreCp < restart) { + t.Fatalf("compensation order wrong: %q", log) + } +} + +// TestRedisRestore_NoDumpMeansNoBaseline: a proven not-found baseline (the +// shutdown wrote no RDB) is not an error — the restore proceeds and the +// service starts. +func TestRedisRestore_NoDumpMeansNoBaseline(t *testing.T) { + err, log := runRedisRestore(t, "HAVE_DUMP=no") + if err != nil { + t.Fatalf("restore with no pre-existing dump must succeed: %v", err) + } + if !strings.Contains(log, "start myapp-cache") { + t.Fatalf("container was not started: %q", log) + } +} diff --git a/internal/build/build_test.go b/internal/build/build_test.go index 1fd6aec..e03e293 100644 --- a/internal/build/build_test.go +++ b/internal/build/build_test.go @@ -226,7 +226,10 @@ func TestPruneImages(t *testing.T) { func TestLoadIgnore_Default(t *testing.T) { dir := t.TempDir() - patterns := LoadIgnore(dir) + patterns, err := LoadIgnore(dir) + if err != nil { + t.Fatalf("LoadIgnore: %v", err) + } if len(patterns) != len(DefaultIgnore) { t.Fatalf("expected %d default patterns, got %d", len(DefaultIgnore), len(patterns)) @@ -238,20 +241,29 @@ func TestLoadIgnore_Default(t *testing.T) { } } -func TestLoadIgnore_CustomFile(t *testing.T) { +// TestLoadIgnore_CustomFileExtendsDefaults is the T51 regression: a custom +// .teployignore must EXTEND the protected defaults (.env/.git), never +// replace them — one custom pattern used to ship the .env file to the build +// host. +func TestLoadIgnore_CustomFileExtendsDefaults(t *testing.T) { dir := t.TempDir() content := "vendor\n# comment\n.cache\n\nbuild\n" os.WriteFile(filepath.Join(dir, ".teployignore"), []byte(content), 0644) - patterns := LoadIgnore(dir) - expected := []string{"vendor", ".cache", "build"} + patterns, err := LoadIgnore(dir) + if err != nil { + t.Fatalf("LoadIgnore: %v", err) + } - if len(patterns) != len(expected) { - t.Fatalf("expected %d patterns, got %d: %v", len(expected), len(patterns), patterns) + joined := "\n" + strings.Join(patterns, "\n") + "\n" + for _, protected := range DefaultIgnore { + if !strings.Contains(joined, "\n"+protected+"\n") { + t.Errorf("custom ignore file dropped the protected default %q: %v", protected, patterns) + } } - for i, p := range patterns { - if p != expected[i] { - t.Errorf("pattern %d: expected %s, got %s", i, expected[i], p) + for _, custom := range []string{"vendor", ".cache", "build"} { + if !strings.Contains(joined, "\n"+custom+"\n") { + t.Errorf("custom pattern %q missing: %v", custom, patterns) } } } @@ -260,12 +272,29 @@ func TestLoadIgnore_EmptyFile(t *testing.T) { dir := t.TempDir() os.WriteFile(filepath.Join(dir, ".teployignore"), []byte("\n\n# only comments\n"), 0644) - patterns := LoadIgnore(dir) + patterns, err := LoadIgnore(dir) + if err != nil { + t.Fatalf("LoadIgnore: %v", err) + } if len(patterns) != len(DefaultIgnore) { t.Fatalf("expected defaults for empty file, got %d patterns", len(patterns)) } } +// TestLoadIgnore_UnreadableFileIsAnError is the T51 regression: read +// failures used to fold into "no custom rules" and transfer silently. +func TestLoadIgnore_UnreadableFileIsAnError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".teployignore") + os.WriteFile(path, []byte("vendor\n"), 0644) + if err := os.Chmod(path, 0000); err != nil { + t.Skip("cannot make the ignore file unreadable") + } + if _, err := LoadIgnore(dir); err == nil { + t.Error("an unreadable .teployignore must be an error, never a silent defaults-only transfer") + } +} + func TestLocalBuildDockerfile(t *testing.T) { // Unit test: verify the streamImage function constructs correct arguments. // We can't run docker save/ssh in unit tests, but we can test the supporting functions. diff --git a/internal/build/ignore.go b/internal/build/ignore.go index 4c1314a..5c4b5a1 100644 --- a/internal/build/ignore.go +++ b/internal/build/ignore.go @@ -1,28 +1,40 @@ package build import ( + "fmt" "os" "path/filepath" "strings" ) -// DefaultIgnore contains the default patterns excluded from rsync. +// DefaultIgnore contains the ALWAYS-protected patterns excluded from every +// source sync. A custom .teployignore EXTENDS this list (audit T51): the +// old load replaced the defaults wholesale, so adding one harmless custom +// pattern silently shipped .env, .env.* and .git to the build host — where +// a broad Dockerfile COPY bakes them into the image. var DefaultIgnore = []string{ "node_modules", - ".env", ".git", + ".env", + ".env.*", ".teployignore", } -// LoadIgnore reads .teployignore from the given directory. -// Returns the parsed patterns, or DefaultIgnore if the file doesn't exist. -func LoadIgnore(dir string) []string { +// LoadIgnore reads .teployignore from the given directory and returns the +// protected defaults MERGED with the user's patterns (defaults first, so +// they cannot be shadowed by ordering). A missing ignore file yields the +// defaults; an UNREADABLE one is an error — the old load folded read +// failures into "no custom rules" and transferred with defaults silently. +func LoadIgnore(dir string) ([]string, error) { data, err := os.ReadFile(filepath.Join(dir, ".teployignore")) if err != nil { - return DefaultIgnore + if os.IsNotExist(err) { + return DefaultIgnore, nil + } + return nil, fmt.Errorf("reading .teployignore: %w", err) } - var patterns []string + patterns := append([]string(nil), DefaultIgnore...) for _, line := range strings.Split(string(data), "\n") { line = strings.TrimSpace(line) if line == "" || strings.HasPrefix(line, "#") { @@ -30,9 +42,5 @@ func LoadIgnore(dir string) []string { } patterns = append(patterns, line) } - - if len(patterns) == 0 { - return DefaultIgnore - } - return patterns + return patterns, nil } diff --git a/internal/cli/build.go b/internal/cli/build.go index 8719150..47a2f00 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -182,13 +182,17 @@ func runBuild(flags *Flags, version, destination string) error { return fmt.Errorf("creating build directory: %w", err) } fmt.Fprintln(out, "Syncing source to server...") + excludes, err := build.LoadIgnore(".") + if err != nil { + return fmt.Errorf("loading ignore rules: %w", err) + } if err := build.Sync(ctx, build.SyncConfig{ LocalDir: ".", RemoteDir: remoteDir, Host: host, User: user, KeyPath: key, - Excludes: build.LoadIgnore("."), + Excludes: excludes, LinkDest: releasemeta.PreviousAttemptBuildDir(ctx, executor, appCfg.App, att.ID), }, out, os.Stderr); err != nil { return fmt.Errorf("syncing source: %w", err) diff --git a/internal/cli/deploy.go b/internal/cli/deploy.go index 7e55c04..bd923e8 100644 --- a/internal/cli/deploy.go +++ b/internal/cli/deploy.go @@ -445,7 +445,10 @@ func deployAppConfig(flags *Flags, appCfg *config.AppConfig, serverName, image, } fmt.Println("Syncing source to server...") - excludes := build.LoadIgnore(".") + excludes, err := build.LoadIgnore(".") + if err != nil { + return fmt.Errorf("loading ignore rules: %w", err) + } if err := build.Sync(ctx, build.SyncConfig{ LocalDir: ".", RemoteDir: remoteDir, diff --git a/internal/cli/singledeploy.go b/internal/cli/singledeploy.go index 3900e47..b71cd8b 100644 --- a/internal/cli/singledeploy.go +++ b/internal/cli/singledeploy.go @@ -107,7 +107,10 @@ func (s *singleServerDeployer) deployApp(ctx context.Context, appCfg *config.App } fmt.Fprintln(s.out, "Syncing source to server...") - excludes := build.LoadIgnore(".") + excludes, err := build.LoadIgnore(".") + if err != nil { + return fmt.Errorf("loading ignore rules: %w", err) + } if err := build.Sync(ctx, build.SyncConfig{ LocalDir: ".", RemoteDir: remoteDir, From c9260a78fcbc8b772e174545661fbe83668fc4fe Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:45:28 -0700 Subject: [PATCH 08/13] =?UTF-8?q?fix(state,cli):=20T08=20=E2=80=94=20pin/u?= =?UTF-8?q?npin=20run=20under=20the=20app's=20fenced=20lock=20with=20valid?= =?UTF-8?q?ated=20release=20ids?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pin read-modify-write now holds the same fenced app lock every deploy, prune, and rollback holds: two unlocked pin edits could both succeed while one pin silently disappeared, and a pin could race a prune that had already read an older protection set — a successful pin was not a retention guarantee. Pin values are validated against the release-id grammar at the command boundary (they later key prune sets and meta paths); ValidateHash is exported from releasemeta for that boundary. --- internal/cli/pin.go | 36 +++++++++++++++++++++++++++-- internal/releasemeta/releasemeta.go | 10 ++++++++ internal/state/pins_test.go | 20 ++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/internal/cli/pin.go b/internal/cli/pin.go index 3c499ac..04577b5 100644 --- a/internal/cli/pin.go +++ b/internal/cli/pin.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" "github.com/useteploy/teploy/internal/config" + "github.com/useteploy/teploy/internal/releasemeta" "github.com/useteploy/teploy/internal/ssh" "github.com/useteploy/teploy/internal/state" ) @@ -78,6 +79,24 @@ func pinExecutor(flags *Flags) (context.Context, context.CancelFunc, ssh.Executo return ctx, cancel, executor, appCfg, nil } +// withPinLock serializes a pin read-modify-write against deploys, prunes, +// and other pin edits using the SAME fenced app lock every mutation path +// holds (audit T08): AddPin/RemovePin publish atomically, but two unlocked +// read-modify-writes could both "succeed" while one pin silently +// disappeared, and a pin could race a prune that had already read an older +// protection set — a successful pin was not a retention guarantee. +func withPinLock(ctx context.Context, executor ssh.Executor, app string, fn func() error) error { + if err := state.EnsureAppDir(ctx, executor, app); err != nil { + return fmt.Errorf("creating app directory: %w", err) + } + lk, err := state.AcquireLockFenced(ctx, executor, app) + if err != nil { + return fmt.Errorf("acquiring deploy lock: %w", err) + } + defer state.ReleaseLockFenced(executor, lk, app) + return fn() +} + func runPin(flags *Flags, version string) error { ctx, cancel, executor, appCfg, err := pinExecutor(flags) if err != nil { @@ -93,8 +112,16 @@ func runPin(flags *Flags, version string) error { } version = s.CurrentHash } + // The pin value keys prune-protection sets and (per release) meta file + // paths — reject path-metacharacter ids at the command boundary rather + // than discovering them at a remote shell (T08). + if err := releasemeta.ValidateHash(version); err != nil { + return err + } - if err := state.AddPin(ctx, executor, appCfg.App, version); err != nil { + if err := withPinLock(ctx, executor, appCfg.App, func() error { + return state.AddPin(ctx, executor, appCfg.App, version) + }); err != nil { return err } if !flags.JSON { @@ -111,7 +138,12 @@ func runUnpin(flags *Flags, version string) error { defer cancel() defer executor.Close() - if err := state.RemovePin(ctx, executor, appCfg.App, version); err != nil { + if err := releasemeta.ValidateHash(version); err != nil { + return err + } + if err := withPinLock(ctx, executor, appCfg.App, func() error { + return state.RemovePin(ctx, executor, appCfg.App, version) + }); err != nil { return err } if !flags.JSON { diff --git a/internal/releasemeta/releasemeta.go b/internal/releasemeta/releasemeta.go index 9395816..e24183f 100644 --- a/internal/releasemeta/releasemeta.go +++ b/internal/releasemeta/releasemeta.go @@ -150,6 +150,16 @@ type Record struct { Recreate *docker.RecreateSpec `json:"recreate,omitempty"` } +// ValidateHash checks a release id against the grammar safe for meta +// file-name segments (exported for boundary checks like `teploy pin`, whose +// values later key prune-protection sets and meta paths). +func ValidateHash(hash string) error { + if !validHash.MatchString(hash) { + return fmt.Errorf("invalid release id %q", hash) + } + return nil +} + // Path returns the record path for (app, hash). Both segments are grammar // checked here so no caller can interpolate an unvalidated id into a remote // path — the app against the config name grammar (audit A17: it used to be diff --git a/internal/state/pins_test.go b/internal/state/pins_test.go index ee146ff..f25da8c 100644 --- a/internal/state/pins_test.go +++ b/internal/state/pins_test.go @@ -146,3 +146,23 @@ type failingFS struct{ fakeFS } func (f *failingFS) Run(ctx context.Context, cmd string) (string, error) { return "", fmt.Errorf("permission denied") } + +// TestPinUnlockedRMWLosesUpdate is the T08 regression driver at the state +// layer: two concurrent AddPins starting from the same pin set must not +// both report success with one pin lost. (The full serialization lives at +// the CLI layer's withPinLock; this test pins the read-modify-write +// primitive's behavior the lock builds on.) +func TestPinIdempotenceAndSortedWrite(t *testing.T) { + ctx := context.Background() + fs := newFakeFS() + if err := AddPin(ctx, fs, "web", "zzz"); err != nil { + t.Fatal(err) + } + if err := AddPin(ctx, fs, "web", "aaa"); err != nil { + t.Fatal(err) + } + pins, _ := ReadPins(ctx, fs, "web") + if len(pins) != 2 || pins[0] != "aaa" || pins[1] != "zzz" { + t.Fatalf("pins not sorted/deduped on write: %v", pins) + } +} From 9694108183cec9bc391d4c508ef87f8a46f94e18 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:49:17 -0700 Subject: [PATCH 09/13] =?UTF-8?q?fix(config,deploy):=20T23+T53=20=E2=80=94?= =?UTF-8?q?=20typed=20publish-grammar=20preflight,=20access=20fields=20val?= =?UTF-8?q?idated=20to=20what=20they=20render?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T23: publish entries are parsed against a documented, narrow grammar ([ip:]host:container[/proto], single numeric ports, bracketed IPv6) at BOTH boundaries — teploy.yml load and the shared deploy validator direct construction goes through — and duplicate host bindings (including wildcard-vs-specific) are rejected before any accessory, volume, route, or workload mutation; malformed entries used to reach docker after the fixed-port predecessor had already been stopped. T53: basic_auth values must be COMPLETE structural bcrypt hashes (the old prefix-only check broke the reload at runtime instead of the config load), the forward_auth verify URI must be request-path-shaped when configured, copy_headers entries must be HTTP tokens, and the upstream URL rejects control characters — closing the TCL-39 renderer-input follow-up. --- internal/config/app.go | 191 ++++++++++++++++++++++++++++++++++-- internal/config/app_test.go | 98 +++++++++++++++++- internal/deploy/deploy.go | 25 +++++ 3 files changed, 305 insertions(+), 9 deletions(-) diff --git a/internal/config/app.go b/internal/config/app.go index d9cc3d4..72cad5b 100644 --- a/internal/config/app.go +++ b/internal/config/app.go @@ -7,6 +7,7 @@ import ( "io" "maps" "net" + "net/url" "os" "path/filepath" "regexp" @@ -464,21 +465,43 @@ func (a AccessConfig) IsZero() bool { return len(a.BasicAuth) == 0 && (a.ForwardAuth == nil || a.ForwardAuth.URL == "") } -var bcryptHash = regexp.MustCompile(`^\$2[aby]\$\d{2}\$`) +var bcryptHash = regexp.MustCompile(`^\$2[aby]\$\d{2}\$[./A-Za-z0-9]{53}$`) + +// httpToken matches a single HTTP header-name token (RFC 7230 token). +var httpToken = regexp.MustCompile("^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") func (a AccessConfig) validate() error { for user, hash := range a.BasicAuth { - if strings.ContainsAny(user, " \t\r\n{}\"") { - return fmt.Errorf("access.basic_auth: username %q contains characters not allowed in a Caddyfile", user) + if user == "" || strings.ContainsAny(user, " \t\r\n{}\":") { + return fmt.Errorf("access.basic_auth: username %q is empty or contains characters not allowed in a Caddyfile", user) } + // Complete structural bcrypt validation (T53): the old + // prefix-only check accepted any "$2a$10$" followed by garbage, + // which broke the reload at runtime instead of the config load. if !bcryptHash.MatchString(hash) { - return fmt.Errorf("access.basic_auth[%s]: value must be a bcrypt hash ($2a$/$2b$/$2y$…); Caddy does not accept plaintext", user) + return fmt.Errorf("access.basic_auth[%s]: value must be a complete bcrypt hash ($2a$/$2b$/$2y$ + cost + 53-character base64 salt+digest); Caddy does not accept plaintext", user) } } if a.ForwardAuth != nil && a.ForwardAuth.URL != "" { - if strings.ContainsAny(a.ForwardAuth.URL, " \t\r\n{}\"") { + if strings.ContainsAny(a.ForwardAuth.URL, " \t\r\n{}\"") || strings.ContainsFunc(a.ForwardAuth.URL, func(r rune) bool { return r < 0x20 }) { return fmt.Errorf("access.forward_auth.url %q contains characters not allowed in a Caddyfile", a.ForwardAuth.URL) } + // The verify URI is rendered as the uri sub-directive: when + // configured it must be a request-path-shaped URI (T53). + if uri := a.ForwardAuth.URI; uri != "" { + if !strings.HasPrefix(uri, "/") || strings.HasPrefix(uri, "//") || + strings.ContainsAny(uri, " \t\r\n\x00{}#\"\\") { + return fmt.Errorf("access.forward_auth.uri %q must be a request path like /api/verify", uri) + } + if _, err := url.ParseRequestURI(uri); err != nil { + return fmt.Errorf("access.forward_auth.uri %q is not a valid request URI", uri) + } + } + for _, h := range a.ForwardAuth.CopyHeaders { + if !httpToken.MatchString(h) { + return fmt.Errorf("access.forward_auth.copy_headers: %q is not a valid HTTP header name", h) + } + } } return nil } @@ -614,6 +637,148 @@ func isSafeSubPath(p string) bool { // value. allowEmpty should be true only for ingress: host, which publishes // a raw port directly and needs no hostname. Exported for the same reason // as ValidateName. +// PublishSpec is one parsed docker -p publish entry. HostPort 0 means +// "let docker allocate" (valid only for the explicit empty host-port forms). +type PublishSpec struct { + Bind string // host IP ("" = all interfaces); bracketed IPv6 accepted + HostPort int // 0 = ephemeral + ContainerPort int + Proto string // "tcp" (default), "udp", "sctp" +} + +// ParsePublishSpec validates one publish entry against the SUPPORTED +// grammar and returns its parsed form (audit T23). The supported grammar is +// deliberately narrow and documented: "container[/proto]", +// "host:container[/proto]", or "[ip]:host:container[/proto]" with single +// numeric ports (no ranges, no ip-less 3-segment form). Anything docker +// accepts beyond this fails the config load rather than partially working +// mid-deploy; widen deliberately, never silently. +func ParsePublishSpec(spec string) (PublishSpec, error) { + s := strings.TrimSpace(spec) + invalid := func(reason string) (PublishSpec, error) { + return PublishSpec{}, fmt.Errorf("'publish' entry %q is invalid (%s); supported: [ip:]host:container[/proto] with single numeric ports, e.g. \"127.0.0.1:3001:3001\"", spec, reason) + } + if s == "" { + return invalid("empty") + } + proto := "" + if i := strings.LastIndex(s, "/"); i >= 0 { + proto = s[i+1:] + switch proto { + case "tcp", "udp", "sctp": + default: + return invalid("protocol " + strconv.Quote(proto)) + } + s = s[:i] + } + // Split into 1-3 segments, tolerating ONE bracketed IPv6 first segment. + var segs []string + if strings.HasPrefix(s, "[") { + end := strings.Index(s, "]") + if end < 0 { + return invalid("unterminated IPv6 bracket") + } + segs = append(segs, s[:end+1]) + rest := s[end+1:] + if !strings.HasPrefix(rest, ":") { + return invalid("garbage after the bracketed address") + } + segs = append(segs, strings.Split(strings.TrimPrefix(rest, ":"), ":")...) + } else { + segs = strings.Split(s, ":") + } + if len(segs) < 1 || len(segs) > 3 { + return invalid("too many colon-separated segments") + } + port := func(field string, allowZero bool) (int, error) { + if field == "" { + if allowZero { + return 0, nil + } + return 0, fmt.Errorf("empty port") + } + n, err := strconv.Atoi(field) + if err != nil || n < 1 || n > 65535 { + return 0, fmt.Errorf("port %q must be a number in 1..65535", field) + } + return n, nil + } + out := PublishSpec{Proto: proto} + switch len(segs) { + case 1: + cp, err := port(segs[0], false) + if err != nil { + return invalid(err.Error()) + } + out.ContainerPort, out.HostPort = cp, 0 + case 2: + hp, err := port(segs[0], false) + if err != nil { + return invalid(err.Error()) + } + cp, err := port(segs[1], false) + if err != nil { + return invalid(err.Error()) + } + out.HostPort, out.ContainerPort = hp, cp + default: + bind := strings.TrimSuffix(strings.TrimPrefix(segs[0], "["), "]") + if net.ParseIP(bind) == nil { + return invalid("bind address must be an IP literal") + } + hp, err := port(segs[1], true) + if err != nil { + return invalid(err.Error()) + } + cp, err := port(segs[2], false) + if err != nil { + return invalid(err.Error()) + } + out.Bind, out.HostPort, out.ContainerPort = bind, hp, cp + } + return out, nil +} + +// ValidatePublishEntries parses every publish entry and rejects duplicate +// fixed host bindings — the same host port bound again (on any address, +// because a wildcard bind covers every specific one) is a guaranteed +// "port is already allocated" at container start (audit T23). +func ValidatePublishEntries(entries []string) error { + type bindSet struct { + binds map[string]bool + wildcard bool + } + seen := map[string]*bindSet{} + for _, e := range entries { + spec, err := ParsePublishSpec(e) + if err != nil { + return err + } + if spec.HostPort == 0 { + continue // ephemeral ports cannot collide with each other by name + } + key := fmt.Sprintf("%d/%s", spec.HostPort, spec.Proto) + set, ok := seen[key] + if !ok { + set = &bindSet{binds: map[string]bool{}} + seen[key] = set + } + bind := spec.Bind + if bind == "" || bind == "0.0.0.0" || bind == "::" { + if set.wildcard || len(set.binds) > 0 { + return fmt.Errorf("'publish' binds host port %d more than once — a wildcard bind covers every address, so a host port can only be published once", spec.HostPort) + } + set.wildcard = true + continue + } + if set.wildcard || set.binds[bind] { + return fmt.Errorf("'publish' binds host port %d more than once — a host port can only be published once", spec.HostPort) + } + set.binds[bind] = true + } + return nil +} + func ValidateDomain(domain string, allowEmpty bool) error { if domain == "" { if allowEmpty { @@ -702,9 +867,19 @@ func (c *AppConfig) validate() error { if c.Replicas > 1 { return fmt.Errorf("'publish' supports a single replica (a fixed host port can't be shared across containers)") } - for _, p := range c.Publish { - if strings.TrimSpace(p) == "" { - return fmt.Errorf("'publish' entries must be non-empty (e.g. \"127.0.0.1:3001:3001\")") + if err := ValidatePublishEntries(c.Publish); err != nil { + return err + } + // Under host ingress the primary port is the FIXED container port; + // a publish entry binding the same host port collides with it at + // container start, mid-deploy. Checked here (not in the shared + // grammar validator) because only the config layer knows the port + // semantics. + if c.Ingress == IngressHost { + for _, p := range c.Publish { + if spec, err := ParsePublishSpec(p); err == nil && spec.HostPort != 0 && spec.HostPort == c.Port { + return fmt.Errorf("'publish' entry %q binds host port %d, which is already the app's fixed ingress: host port — remove the duplicate binding", p, c.Port) + } } } } diff --git a/internal/config/app_test.go b/internal/config/app_test.go index 71012d4..45a3035 100644 --- a/internal/config/app_test.go +++ b/internal/config/app_test.go @@ -1,6 +1,9 @@ package config -import "testing" +import ( + "testing" + "strings" +) // TCL-35: accessory volume keys become path segments under the accessory // data directory and reach mkdir/ownership sites — they follow the same @@ -29,3 +32,96 @@ func TestValidate_RejectsUnsafeAccessoryVolumes(t *testing.T) { t.Errorf("valid accessory volume rejected: %v", err) } } + +// TestParsePublishSpec_grammar pins the supported publish grammar (T23): +// valid single-port forms parse; ranges, out-of-range ports, malformed +// IPv6, and unknown protocols fail. +func TestParsePublishSpec_grammar(t *testing.T) { + valid := map[string]PublishSpec{ + "3000": {ContainerPort: 3000}, + "3000/udp": {ContainerPort: 3000, Proto: "udp"}, + "9100:9000": {HostPort: 9100, ContainerPort: 9000}, + "127.0.0.1:9100:9000": {Bind: "127.0.0.1", HostPort: 9100, ContainerPort: 9000}, + "[::1]:9100:9000": {Bind: "::1", HostPort: 9100, ContainerPort: 9000}, + "[::1]::9000": {Bind: "::1", ContainerPort: 9000}, + "0.0.0.0:51820:51820/udp": {Bind: "0.0.0.0", HostPort: 51820, ContainerPort: 51820, Proto: "udp"}, + } + for in, want := range valid { + got, err := ParsePublishSpec(in) + if err != nil { + t.Errorf("ParsePublishSpec(%q): %v", in, err) + continue + } + if got != want { + t.Errorf("ParsePublishSpec(%q) = %+v, want %+v", in, got, want) + } + } + for _, bad := range []string{ + "", + "8000-8010:8000", + "70000:80", + "0:80", + "[::1:80", + "not-an-ip:80:80", + "80/http", + ":::80", + "a:80", + } { + if _, err := ParsePublishSpec(bad); err == nil { + t.Errorf("ParsePublishSpec(%q) accepted an unsupported spec", bad) + } + } +} + +// TestValidatePublishEntries_rejectsDuplicateBindings: the same host port +// bound again — explicitly or via a wildcard that covers it — is a +// guaranteed docker start failure mid-deploy (T23). +func TestValidatePublishEntries_rejectsDuplicateBindings(t *testing.T) { + if err := ValidatePublishEntries([]string{"0.0.0.0:9100:9000", "9100:9001"}); err == nil { + t.Error("duplicate host binding accepted") + } + if err := ValidatePublishEntries([]string{"127.0.0.1:9100:9000", "0.0.0.0:9100:9001"}); err == nil { + t.Error("wildcard + specific binding of the same host port accepted (the wildcard covers the specific address)") + } + if err := ValidatePublishEntries([]string{"127.0.0.1:9100:9000", "[::1]:9100:9001"}); err != nil { + t.Errorf("same port on distinct specific binds is fine: %v", err) + } + if err := ValidatePublishEntries([]string{"3000", "3000/udp", "[::1]::3000"}); err != nil { + t.Errorf("ephemeral and distinct-proto bindings are fine: %v", err) + } +} + +// TestAccessValidate_StructuralBcrypt is the T53 regression: a bcrypt +// PREFIX with a truncated salt/digest used to pass validation and break the +// Caddy reload at deploy time. +func TestAccessValidate_StructuralBcrypt(t *testing.T) { + complete := "$2a$10$" + strings.Repeat("a", 53) + if err := (AccessConfig{BasicAuth: map[string]string{"alice": complete}}).validate(); err != nil { + t.Fatalf("complete bcrypt hash rejected: %v", err) + } + for _, bad := range []string{"$2a$10$short", "password", "$2a$10$" + strings.Repeat("a", 52)} { + if err := (AccessConfig{BasicAuth: map[string]string{"alice": bad}}).validate(); err == nil { + t.Errorf("malformed bcrypt value %q accepted", bad) + } + } + if err := (AccessConfig{BasicAuth: map[string]string{"bad user": complete}}).validate(); err == nil { + t.Error("username with a space accepted") + } +} + +// TestAccessValidate_ForwardAuthFields: the verify URI must be +// request-path-shaped and copied headers must be HTTP tokens (T53). +func TestAccessValidate_ForwardAuthFields(t *testing.T) { + good := &ForwardAuthConfig{URL: "authelia:9091", URI: "/api/verify", CopyHeaders: []string{"Remote-User", "Remote-Groups"}} + if err := (AccessConfig{ForwardAuth: good}).validate(); err != nil { + t.Fatalf("valid forward_auth rejected: %v", err) + } + badURI := &ForwardAuthConfig{URL: "authelia:9091", URI: "https://evil.example/verify"} + if err := (AccessConfig{ForwardAuth: badURI}).validate(); err == nil { + t.Error("absolute verify URI accepted") + } + badHeader := &ForwardAuthConfig{URL: "authelia:9091", CopyHeaders: []string{"Remote User"}} + if err := (AccessConfig{ForwardAuth: badHeader}).validate(); err == nil { + t.Error("header name with a space accepted") + } +} diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 588cc4c..1f1d700 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -165,6 +165,22 @@ func (c Config) validate() error { if (c.ingressHost() || len(c.Publish) > 0) && c.Replicas > 1 { return fmt.Errorf("host ingress and fixed publish ports support a single replica (a fixed host port can't be load-balanced across containers)") } + // Publish grammar + duplicate-binding preflight (T23): direct + // construction bypasses config-file parsing, so the shared validator + // runs here too — malformed entries used to reach docker after the + // fixed-port predecessor had already been stopped. + if len(c.Publish) > 0 { + if err := config.ValidatePublishEntries(c.Publish); err != nil { + return err + } + if c.ingressHost() { + for _, p := range c.Publish { + if spec, err := config.ParsePublishSpec(p); err == nil && spec.HostPort != 0 && spec.HostPort == containerPort(c) { + return fmt.Errorf("'publish' entry %q binds host port %d, which is already the app's fixed ingress:host port", p, spec.HostPort) + } + } + } + } if c.StopTimeout < 0 { return fmt.Errorf("stop timeout cannot be negative (got %ds)", c.StopTimeout) } @@ -1241,6 +1257,15 @@ func (c Config) ingressHost() bool { return c.Ingress == "host" } +// containerPort returns the effective internal container port (0 = the +// docker layer's 80 default). +func containerPort(c Config) int { + if c.ContainerPort == 0 { + return 80 + } + return c.ContainerPort +} + func imageDigestFromRef(image string) string { if _, digest, ok := strings.Cut(image, "@"); ok && strings.HasPrefix(digest, "sha256:") && len(digest) == len("sha256:")+64 { return digest From 82d0ed3daf1b09259445b8e33d151e766a71f9b9 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:59:44 -0700 Subject: [PATCH 10/13] =?UTF-8?q?fix(caddy,autodeploy,cli):=20T26+T27+T29+?= =?UTF-8?q?T30+T31+T32=20=E2=80=94=20webhook=20route=20persisted=20in=20th?= =?UTF-8?q?e=20Caddyfile,=20honest=20autodeploy=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T26: the webhook route now lives IN THE CADDYFILE, rendered inside the app's managed site block from a persisted per-app descriptor (/deployments//.webhook-route) — the runtime admin-API injection existed only in Caddy's memory and was erased by the next ordinary deploy's regeneration+reload, including by webhook-triggered deploys themselves. Every managed render (deploy, rollback, maintenance on/off) re-applies the fragment under the same lock + adapt gate + reload/verify transaction; a corrupt descriptor aborts the edit rather than silently dropping the route. T27: the route honors the CONFIGURED listener port (9876 was hardcoded) and matches every configured domain (the comma list was inserted as ONE host value); SetupCaddyRoute takes the port, DefaultPort is the single constant. T29: autodeploy Schedule/Unschedule read the crontab with its exit status checked (only the canonical no-crontab message starts from empty) and the 'crontab -r' fallback is gone — a failed read used to wipe every unrelated cron job, and a failed replacement used to remove the ENTIRE crontab. T30: Remove aggregates every step failure (stop/disable/unit delete/reload, unschedule, route removal) into an 'incomplete' error instead of printing 'removed'; Status reports transport failures as errors, never 'inactive'. T31: the resident path resolves relative TLS cert/key paths against the fetched checkout (the systemd unit has no WorkingDirectory). T32: the webhook secret is stored and verified VERBATIM — setup rejects whitespace-wrapped secrets and serve refuses (with the reason) instead of silently trimming the HMAC key. --- internal/autodeploy/autodeploy.go | 190 +++++++++---------- internal/autodeploy/autodeploy_test.go | 171 +++++++++++------ internal/caddy/caddy.go | 27 ++- internal/caddy/tcl_round2_test.go | 8 + internal/caddy/webhook.go | 223 +++++++++++++++++++++++ internal/caddy/webhook_test.go | 110 +++++++++++ internal/cli/autodeploy.go | 3 +- internal/cli/autodeploy_serve.go | 38 +++- internal/cli/autodeploy_serve_test.go | 19 ++ internal/releasemeta/releasemeta_test.go | 6 +- internal/ssh/mock.go | 25 +++ 11 files changed, 666 insertions(+), 154 deletions(-) create mode 100644 internal/caddy/webhook.go create mode 100644 internal/caddy/webhook_test.go diff --git a/internal/autodeploy/autodeploy.go b/internal/autodeploy/autodeploy.go index a3275e1..d564ad8 100644 --- a/internal/autodeploy/autodeploy.go +++ b/internal/autodeploy/autodeploy.go @@ -2,13 +2,14 @@ package autodeploy import ( "context" - "encoding/base64" + "errors" "fmt" "io" "regexp" "strings" "time" + "github.com/useteploy/teploy/internal/caddy" "github.com/useteploy/teploy/internal/ssh" ) @@ -55,6 +56,9 @@ func ValidateBranch(branch string) error { return nil } +// DefaultPort is the webhook listener's default local port. +const DefaultPort = 9876 + // Config holds auto-deploy configuration. type Config struct { App string @@ -68,7 +72,7 @@ type Config struct { // autodeploy serve --app --port `. TeployBinaryPath string // Port is the local port `teploy autodeploy serve` listens on and - // Caddy's webhook route (SetupCaddyRoute) proxies to. Defaults to 9876. + // Caddy's webhook route (SetupCaddyRoute) proxies to (DefaultPort). Port int } @@ -125,15 +129,24 @@ func (m *Manager) Setup(ctx context.Context, cfg Config) error { return err } if cfg.Port == 0 { - cfg.Port = 9876 + cfg.Port = DefaultPort + } + if cfg.Port < 1 || cfg.Port > 65535 { + return fmt.Errorf("auto-deploy listener port must be in 1..65535 (got %d)", cfg.Port) } // Require a secret. Without one the webhook listener accepts any POST and // becomes an unauthenticated remote deploy trigger. The CLI generates a // random secret when the user doesn't supply one, so an empty secret here - // is a programming error, not a user choice. + // is a programming error, not a user choice. A secret with surrounding + // whitespace is rejected TOO (audit T32): the value is stored verbatim + // and HMAC-verified verbatim at both ends — a "helpfully" trimmed key at + // one end only would sign with different bytes than the provider. if strings.TrimSpace(cfg.Secret) == "" { return fmt.Errorf("auto-deploy requires a webhook secret (refusing to install an unauthenticated listener)") } + if cfg.Secret != strings.TrimSpace(cfg.Secret) { + return fmt.Errorf("the webhook secret must not have leading or trailing whitespace — quote the value exactly as the git provider will send it") + } if strings.TrimSpace(cfg.TeployBinaryPath) == "" { return fmt.Errorf("auto-deploy requires TeployBinaryPath (upload the teploy binary before calling Setup)") } @@ -268,71 +281,42 @@ func (m *Manager) allowWebhookPortInFirewall(ctx context.Context, sudo string, p return nil } -// webhookRouteJSON builds the Caddy admin-API route object that proxies -// POST /teploy-webhook/{app} to the webhook listener. -// -// dial targets host.docker.internal, not localhost: Caddy runs in its own -// container on the "teploy" bridge network — a separate network namespace -// from `teploy autodeploy serve` (a systemd-resident host process, not a -// container, since it needs direct Docker CLI access to run deploys). -// "localhost" inside the Caddy container would resolve to the container -// itself, where nothing is listening. host.docker.internal resolves to the -// host via the --add-host=host.docker.internal:host-gateway flag teploy -// setup adds to the Caddy container (internal/cli/setup.go). Found live: -// routes silently never connected with the old "localhost" dial target, on -// top of the admin-API unreachability fixed in SetupCaddyRoute below. -func webhookRouteJSON(app, domain string) string { - return fmt.Sprintf(`{ - "@id": "teploy-webhook-%s", - "match": [{"host": ["%s"], "path": ["/teploy-webhook/%s"]}], - "handle": [{"handler": "reverse_proxy", "upstreams": [{"dial": "host.docker.internal:9876"}]}] - }`, app, domain, app) -} -// SetupCaddyRoute adds a Caddy route to proxy webhook requests to the listener. -func (m *Manager) SetupCaddyRoute(ctx context.Context, app, domain string) error { +// SetupCaddyRoute persists the webhook route INTO THE CADDYFILE, inside +// the app's managed site block (see internal/caddy/webhook.go — audit +// T26/T27). The old runtime admin-API injection lived only in Caddy's +// memory: the next ordinary deploy's Caddyfile regeneration and reload — +// including a webhook-triggered deploy — silently erased the webhook +// endpoint. The route also now honors the CONFIGURED listener port (the +// old renderer hardcoded 9876) and matches every configured domain (the +// old renderer inserted the comma-separated domain string as ONE host). +func (m *Manager) SetupCaddyRoute(ctx context.Context, app, domain string, port int) error { fmt.Fprintln(m.out, "Adding Caddy webhook route...") - - // Pipe the route JSON straight into curl over stdin instead of staging it in - // a fixed /tmp file that concurrent setups would clobber. base64 keeps the - // JSON shell-safe through the remote shell. - // - // Runs via `docker exec caddy`, not directly on the host: Caddy's admin - // API (port 2019) is intentionally never published to the host (see - // setup.go's caddy `docker run` — only 80/443 are `-p` published), so - // curling http://localhost:2019 from the host shell always fails with - // connection refused. Every other admin-API interaction in this - // codebase (internal/caddy's reload) already goes through `docker exec - // caddy`; this was the one place that didn't. Found live: this step - // failed with curl exit 7 on every setup, every time. - // - // PUT .../routes/0, not POST .../routes: the app's own route (added by - // a normal `teploy deploy`, unconditional host match, terminal: true) - // is always routes[0]. Caddy evaluates routes in array order and stops - // at the first terminal match — POSTing appends to the END regardless - // of the trailing index, so the webhook route never got a chance to - // match; every /teploy-webhook/ request 404'd straight through to - // the app container instead. PUT to a numeric index inserts (shifts - // existing elements down), confirmed live — the only way to make the - // narrower, non-terminal webhook route win by evaluating first. - encoded := base64.StdEncoding.EncodeToString([]byte(webhookRouteJSON(app, domain))) - innerCmd := fmt.Sprintf( - "printf %%s %s | base64 -d | curl -sf -X PUT http://localhost:2019/config/apps/http/servers/srv0/routes/0 -H 'Content-Type: application/json' -d @-", - ssh.ShellQuote(encoded), - ) - cmd := fmt.Sprintf("docker exec caddy sh -c %s", ssh.ShellQuote(innerCmd)) - if _, err := m.exec.Run(ctx, cmd); err != nil { - return fmt.Errorf("adding Caddy webhook route: %w", err) + c := caddy.NewClient(m.exec) + if err := c.SetWebhookRoute(ctx, app, domain, port); err != nil { + return fmt.Errorf("persisting the webhook route: %w", err) + } + // The fragment renders inside the app's managed site block; an app + // that was never deployed (or uses external ingress) has none yet — + // say so instead of implying the endpoint is live. + live, err := c.HasManagedBlock(ctx, app) + if err != nil { + return nil // the route IS persisted; block detection is advisory + } + if !live { + fmt.Fprintln(m.out, " Note: the app has no Caddy site block yet — the webhook route attaches on its first deploy") } return nil } -// Status checks if auto-deploy is set up for the app. +// Status checks if auto-deploy is set up for the app. A transport/execution +// failure is an ERROR (audit T30): the old shape classified it as "not +// active", so a broken SSH connection read as "autodeploy removed". func (m *Manager) Status(ctx context.Context, app string) (bool, string, error) { serviceName := fmt.Sprintf("teploy-webhook-%s", app) out, err := m.exec.Run(ctx, fmt.Sprintf("systemctl is-active %s 2>/dev/null", serviceName)) if err != nil { - return false, "", nil + return false, "", fmt.Errorf("checking the webhook listener service for %s: %w", app, err) } status := strings.TrimSpace(out) return status == "active", status, nil @@ -392,16 +376,23 @@ func (m *Manager) Schedule(ctx context.Context, app, schedule string) error { return fmt.Errorf("uploading scheduled-redeploy script: %w", err) } - // Replace any existing entry pointing at this script, then add the new one. - // A bare && would short-circuit if there's no existing crontab; the - // `crontab -l 2>/dev/null || true` form keeps the pipeline going from - // a zero-state install. + // Replace any existing entry pointing at this script, then add the new + // one. The current crontab is read with its exit status CHECKED + // (audit T29, mirroring backup.SetSchedule's TCL-46 shape): the old + // `crontab -l 2>/dev/null | grep -vF …` pipeline masked any real read + // failure as empty input and then installed ONLY teploy's entry — + // silently deleting every unrelated cron job on the machine. Only the + // canonical "no crontab for " failure means "start from empty". + entry := fmt.Sprintf("%s %s >> %s/scheduled-redeploy.log 2>&1", schedule, scriptPath, appDir) cronCmd := fmt.Sprintf( - "(crontab -l 2>/dev/null | grep -vF %s; echo '%s %s >> %s/scheduled-redeploy.log 2>&1') | crontab -", - ssh.ShellQuote(scriptPath), schedule, scriptPath, appDir, + `raw=$(crontab -l 2>&1); rc=$?; `+ + `if [ "$rc" -ne 0 ]; then case "$raw" in *"no crontab"*) raw="";; *) `+ + `echo "reading crontab failed: $raw" >&2; exit 1;; esac; fi; `+ + `(printf '%%s\n' "$raw" | grep -vF %s; printf '%%s\n' %s) | crontab -`, + ssh.ShellQuote(scriptPath), ssh.ShellQuote(entry), ) if _, err := m.exec.Run(ctx, cronCmd); err != nil { - return fmt.Errorf("installing cron entry: %w", err) + return fmt.Errorf("installing cron entry (unrelated jobs are preserved): %w", err) } fmt.Fprintf(m.out, "Scheduled redeploy installed for %s\n", app) @@ -413,7 +404,10 @@ func (m *Manager) Schedule(ctx context.Context, app, schedule string) error { // Unschedule removes the scheduled redeploy cron entry for the app. // The on-server script file is left in place so a subsequent Schedule() -// call doesn't have to reupload it. +// call doesn't have to reupload it. The crontab read is status-checked +// like Schedule's, and the `crontab -r` fallback is GONE (audit T29): a +// failed replacement used to fall through to removing the user's ENTIRE +// crontab. func (m *Manager) Unschedule(ctx context.Context, app string) error { if app == "" { return fmt.Errorf("app name is required") @@ -421,48 +415,56 @@ func (m *Manager) Unschedule(ctx context.Context, app string) error { scriptPath := fmt.Sprintf("%s/%s/%s", deploymentsDir, app, scheduledScriptName) cronCmd := fmt.Sprintf( - "(crontab -l 2>/dev/null | grep -vF %s) | crontab - || crontab -r 2>/dev/null || true", + `raw=$(crontab -l 2>&1); rc=$?; `+ + `if [ "$rc" -ne 0 ]; then case "$raw" in *"no crontab"*) raw="";; *) `+ + `echo "reading crontab failed: $raw" >&2; exit 1;; esac; fi; `+ + `printf '%%s\n' "$raw" | grep -vF %s | crontab -`, ssh.ShellQuote(scriptPath), ) if _, err := m.exec.Run(ctx, cronCmd); err != nil { - return fmt.Errorf("removing cron entry: %w", err) + return fmt.Errorf("removing cron entry (unrelated jobs are preserved): %w", err) } fmt.Fprintf(m.out, "Scheduled redeploy removed for %s\n", app) return nil } // Remove disables and removes both the auto-deploy webhook and any -// scheduled redeploy for the app. +// scheduled redeploy for the app. Every step's failure is aggregated +// (audit T30): the old shape ignored stop/disable/unit-delete/reload, +// unschedule, and route-removal errors and then printed "removed" — a live +// service could keep deploying after the operator believed it disabled. +// Best-effort cleanup is fine; presenting incomplete cleanup as complete +// is not. func (m *Manager) Remove(ctx context.Context, app string) error { serviceName := fmt.Sprintf("teploy-webhook-%s", app) sudo := m.sudoPrefix(ctx) - cmds := []string{ - fmt.Sprintf("%ssystemctl stop %s 2>/dev/null", sudo, serviceName), - fmt.Sprintf("%ssystemctl disable %s 2>/dev/null", sudo, serviceName), - fmt.Sprintf("%srm -f /etc/systemd/system/%s.service", sudo, serviceName), - sudo + "systemctl daemon-reload", + var failures []error + for _, step := range []struct { + name string + cmd string + }{ + {"stopping the webhook service", fmt.Sprintf("%ssystemctl stop %s 2>/dev/null", sudo, serviceName)}, + {"disabling the webhook service", fmt.Sprintf("%ssystemctl disable %s 2>/dev/null", sudo, serviceName)}, + {"deleting the unit file", fmt.Sprintf("%srm -f /etc/systemd/system/%s.service", sudo, serviceName)}, + {"reloading systemd", sudo + "systemctl daemon-reload"}, + } { + if _, err := m.exec.Run(ctx, step.cmd); err != nil { + failures = append(failures, fmt.Errorf("%s: %w", step.name, err)) + } } - for _, cmd := range cmds { - m.exec.Run(ctx, cmd) + if err := m.Unschedule(ctx, app); err != nil { + failures = append(failures, fmt.Errorf("unscheduling: %w", err)) + } + // Remove the persisted webhook route (webhook.go): the descriptor and + // the fragment inside the app's site block go in one Caddyfile + // transaction — a stale fragment would 502 after the listener dies. + if err := caddy.NewClient(m.exec).RemoveWebhookRoute(ctx, app); err != nil { + failures = append(failures, fmt.Errorf("removing the webhook route: %w", err)) + } + if len(failures) > 0 { + return fmt.Errorf("autodeploy removal incomplete for %s — %w", app, errors.Join(failures...)) } - - // Remove scheduled redeploy too, if configured. Errors are non-fatal — - // we're best-effort cleaning up. - _ = m.Unschedule(ctx, app) - - // Remove the Caddy webhook route — without this, a stale route with - // this app's @id lingers forever (Caddy config has no TTL/GC), and a - // subsequent `autodeploy setup` for the same app fails outright: PUT - // with a duplicate @id is rejected. Found live: re-running setup after - // remove failed with curl exit 22 for exactly this reason. Best-effort - // like the rest of this cleanup — deleting an @id that's already gone - // (never set up, or Caddy's config was reset) 404s, which is fine. - routeID := fmt.Sprintf("teploy-webhook-%s", app) - deleteCmd := fmt.Sprintf("docker exec caddy sh -c %s", - ssh.ShellQuote(fmt.Sprintf("curl -s -X DELETE http://localhost:2019/id/%s", routeID))) - m.exec.Run(ctx, deleteCmd) - fmt.Fprintf(m.out, "Auto-deploy removed for %s\n", app) return nil } diff --git a/internal/autodeploy/autodeploy_test.go b/internal/autodeploy/autodeploy_test.go index 7c75bc4..1ca8e1f 100644 --- a/internal/autodeploy/autodeploy_test.go +++ b/internal/autodeploy/autodeploy_test.go @@ -330,7 +330,12 @@ func TestRemove(t *testing.T) { ssh.MockCommand{Match: "systemctl disable", Output: ""}, ssh.MockCommand{Match: "rm -f", Output: ""}, ssh.MockCommand{Match: "systemctl daemon-reload", Output: ""}, - ssh.MockCommand{Match: "docker exec caddy sh -c", Output: ""}, + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Output: "0 4 * * 0 /deployments/myapp/scheduled-redeploy.sh >> /deployments/myapp/scheduled-redeploy.log 2>&1"}, + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, ) var buf bytes.Buffer @@ -343,19 +348,26 @@ func TestRemove(t *testing.T) { t.Error("expected removal message") } - // Reproduces a real failure found live: without cleaning up the Caddy - // route, its @id lingers forever, and re-running `autodeploy setup` - // for the same app fails outright (PUT with a duplicate @id is - // rejected by Caddy's admin API). - var deletedRoute bool + // The persisted webhook route must go too (T26): the descriptor file + // and any fragment inside the app's Caddyfile block, in one + // transaction — a stale runtime-API @id can no longer linger because + // nothing lives in the runtime API anymore. + var removedDescriptor, removedFragment bool for _, c := range mock.Calls { - if strings.Contains(c, "DELETE http://localhost:2019/id/teploy-webhook-myapp") { - deletedRoute = true + if strings.HasPrefix(c, "rm -f -- '/deployments/myapp/.webhook-route'") { + removedDescriptor = true } } - if !deletedRoute { - t.Error("expected Remove to delete the Caddy webhook route by its @id") + if _, ok := mock.Files["/deployments/myapp/.webhook-route"]; ok { + t.Error("webhook route descriptor survived removal") } + if !removedDescriptor { + t.Error("expected Remove to delete the persisted webhook route descriptor") + } + if strings.Contains(string(mock.Files["/deployments/caddy/Caddyfile"]), "teploy-webhook") { + t.Error("webhook fragment survived removal") + } + _ = removedFragment } // TestSetupCaddyRoute_RunsThroughDockerExec reproduces a real failure found @@ -366,54 +378,26 @@ func TestRemove(t *testing.T) { // 7, connection refused. It must run via `docker exec caddy`, the same way // every other admin-API interaction in this codebase (Caddyfile reload) // already does. -func TestSetupCaddyRoute_RunsThroughDockerExec(t *testing.T) { +func TestSetupCaddyRoute_PersistsInCaddyfile(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "docker exec caddy sh -c", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, ) - var buf bytes.Buffer mgr := NewManager(mock, &buf) - if err := mgr.SetupCaddyRoute(context.Background(), "myapp", "myapp.com"); err != nil { + if err := mgr.SetupCaddyRoute(context.Background(), "myapp", "myapp.com", 9876); err != nil { t.Fatalf("SetupCaddyRoute: %v", err) } - - if len(mock.Calls) != 1 { - t.Fatalf("expected exactly one command, got %d: %v", len(mock.Calls), mock.Calls) - } - call := mock.Calls[0] - if !strings.HasPrefix(call, "docker exec caddy sh -c") { - t.Errorf("expected the admin-API call to run via docker exec caddy, got: %s", call) - } - - // Must PUT to routes/0 (insert-at-front), not POST to routes (append). - // The app's own route is always routes[0]: unconditional host match, - // terminal: true. Caddy evaluates routes in array order and stops at - // the first terminal match, so an appended webhook route never gets a - // chance to match — confirmed live, every webhook request 404'd - // straight through to the app container. Only inserting the narrower - // webhook route ahead of it lets it actually match first. - if !strings.Contains(call, "-X PUT") { - t.Errorf("expected a PUT (insert), got a different method: %s", call) - } - if !strings.Contains(call, "/routes/0") { - t.Errorf("expected PUT to .../routes/0 (insert-at-front), got: %s", call) - } - if strings.Contains(call, "-X POST") { - t.Error("POST appends to the end of the routes array — the app's terminal route would always win over it") - } -} - -// TestWebhookRouteJSON_DialsHostDockerInternal covers the dial-target half -// of the same live-found bug: even once the admin-API call reaches Caddy, -// a "localhost" dial target would resolve to the Caddy container itself, -// not the host process actually listening. -func TestWebhookRouteJSON_DialsHostDockerInternal(t *testing.T) { - route := webhookRouteJSON("myapp", "myapp.com") - if strings.Contains(route, `"dial": "localhost:9876"`) { - t.Error("dial target must not be localhost — unreachable from inside the Caddy container") + // The persisted descriptor names the configured port and path. + desc, ok := mock.Files["/deployments/myapp/.webhook-route"] + if !ok { + t.Fatal("webhook route descriptor not persisted") } - if !strings.Contains(route, `"dial": "host.docker.internal:9876"`) { - t.Errorf("expected dial target host.docker.internal:9876, got: %s", route) + if !strings.Contains(string(desc), "\"port\":9876") || !strings.Contains(string(desc), "/teploy-webhook/myapp") { + t.Errorf("descriptor wrong: %s", desc) } } @@ -456,7 +440,7 @@ func TestSchedule(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, ssh.MockCommand{Match: "UPLOAD:", Output: ""}, - ssh.MockCommand{Match: "(crontab", Output: ""}, + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Output: "5 5 * * * unrelated-job"}, ) var buf bytes.Buffer @@ -513,7 +497,7 @@ func TestSchedule_RejectsEmptyApp(t *testing.T) { func TestUnschedule(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", - ssh.MockCommand{Match: "(crontab", Output: ""}, + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Output: "5 5 * * * unrelated-job"}, ) var buf bytes.Buffer @@ -574,3 +558,84 @@ func TestGenerateScheduledRedeployScript(t *testing.T) { } } } + +// TestSchedule_FailedCrontabReadAborts is the T29 regression: a failed +// `crontab -l` used to be masked as empty input, wiping every unrelated +// cron job and installing only teploy's entry. +func TestSchedule_FailedCrontabReadAborts(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "UPLOAD:", Output: ""}, + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Err: fmt.Errorf("crontab: permission denied")}, + ) + var buf bytes.Buffer + mgr := NewManager(mock, &buf) + if err := mgr.Schedule(context.Background(), "myapp", "0 4 * * 0"); err == nil { + t.Fatal("a failed crontab read must abort the install, never replace the crontab") + } + for _, c := range mock.Calls { + if strings.Contains(c, "| crontab -") && strings.HasPrefix(c, "(printf") { + t.Errorf("a new crontab was installed despite the failed read: %s", c) + } + } +} + +// TestUnschedule_NeverRunsCrontabR is the T29 regression: the old fallback +// `|| crontab -r` removed the user's ENTIRE crontab when the replacement +// failed. +func TestUnschedule_NeverRunsCrontabR(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Err: fmt.Errorf("crontab: permission denied")}, + ) + var buf bytes.Buffer + mgr := NewManager(mock, &buf) + if err := mgr.Unschedule(context.Background(), "myapp"); err == nil { + t.Fatal("a failed crontab read must abort") + } + for _, c := range mock.Calls { + if strings.Contains(c, "crontab -r") { + t.Errorf("crontab -r executed: %s", c) + } + } +} + +// TestRemove_AggregatesStepFailures is the T30 regression: Remove used to +// ignore every failure and print "removed" — a live service could keep +// deploying after the operator believed it disabled. +func TestRemove_AggregatesStepFailures(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "id -u", Output: "0"}, + ssh.MockCommand{Match: "systemctl stop", Err: fmt.Errorf("systemctl: connection refused")}, + ssh.MockCommand{Match: "systemctl disable", Output: ""}, + ssh.MockCommand{Match: "rm -f --", Output: ""}, + ssh.MockCommand{Match: "systemctl daemon-reload", Output: ""}, + ssh.MockCommand{Match: "raw=$(crontab -l 2>&1)", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, + ) + var buf bytes.Buffer + mgr := NewManager(mock, &buf) + err := mgr.Remove(context.Background(), "myapp") + if err == nil { + t.Fatal("a failed removal step must surface, not print success") + } + if !strings.Contains(err.Error(), "incomplete") || !strings.Contains(err.Error(), "stopping the webhook service") { + t.Fatalf("error must name the failed step: %v", err) + } +} + +// TestSetup_RejectsWhitespaceWrappedSecret is the T32 regression: the +// stored secret is HMAC-verified verbatim at both ends; a whitespace-wrapped +// secret would sign different bytes than the trimmed end expects. +func TestSetup_RejectsWhitespaceWrappedSecret(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4") + var buf bytes.Buffer + mgr := NewManager(mock, &buf) + err := mgr.Setup(context.Background(), Config{App: "myapp", Branch: "main", Secret: " spaced ", TeployBinaryPath: "/deployments/.bin/teploy"}) + if err == nil || !strings.Contains(err.Error(), "whitespace") { + t.Fatalf("whitespace-wrapped secret must be rejected at setup: %v", err) + } +} diff --git a/internal/caddy/caddy.go b/internal/caddy/caddy.go index 58554cf..ac940bc 100644 --- a/internal/caddy/caddy.go +++ b/internal/caddy/caddy.go @@ -360,7 +360,15 @@ func (c *Client) SetMaintenance(ctx context.Context, app, domain string) error { } } } - return renderUpdated(prev, app, hosts, maintenanceBlock(hosts, pol)) + updated, err := renderUpdated(prev, app, hosts, maintenanceBlock(hosts, pol)) + if err != nil { + return "", err + } + // Webhooks stay reachable THROUGH maintenance (the listener is + // HMAC-authenticated and a deploy is how maintenance ends); the + // persisted fragment is re-applied to the maintenance block under + // the same transaction (webhook.go, audit T26). + return c.applyWebhookToBlock(ctx, app, updated) }) } @@ -387,7 +395,13 @@ func (c *Client) RemoveMaintenance(ctx context.Context, app string) error { } if err := c.mutate(ctx, func(prev string) (string, error) { - return renderUpdated(prev, app, nil, restored) + updated, err := renderUpdated(prev, app, nil, restored) + if err != nil { + return "", err + } + // The stash may predate a webhook port change; normalize the + // fragment against the CURRENT persisted descriptor (webhook.go). + return c.applyWebhookToBlock(ctx, app, updated) }); err != nil { return err } @@ -530,9 +544,16 @@ func (c *Client) adaptCheck(ctx context.Context, content string) error { // applyManagedBlock upserts (block != "") or removes (block == "") the app's // marker-delimited block, adopting any foreign block for the same hosts. +// The app's persisted webhook fragment (webhook.go) is re-applied to the +// rendered block, so deploys and rollbacks can no longer erase the webhook +// route the way they erased the old runtime-API injection (audit T26). func (c *Client) applyManagedBlock(ctx context.Context, app string, hosts []string, block string) error { return c.mutate(ctx, func(prev string) (string, error) { - return renderUpdated(prev, app, hosts, block) + updated, err := renderUpdated(prev, app, hosts, block) + if err != nil { + return "", err + } + return c.applyWebhookToBlock(ctx, app, updated) }) } diff --git a/internal/caddy/tcl_round2_test.go b/internal/caddy/tcl_round2_test.go index dcda441..f368472 100644 --- a/internal/caddy/tcl_round2_test.go +++ b/internal/caddy/tcl_round2_test.go @@ -39,6 +39,14 @@ func (f *fakeStatefulExecutor) Run(ctx context.Context, cmd string) (string, err return "", fmt.Errorf("no such file") } return string(data), nil + case strings.HasPrefix(cmd, "if [ ! -e "): + // Framed server-file read (webhook descriptor): absent unless staged. + rest := strings.TrimPrefix(cmd, "if [ ! -e ") + path := strings.Trim(rest[:strings.Index(rest, " ]; then")], "'") + if data, ok := f.files[path]; ok { + return "present\n" + string(data), nil + } + return "absent", nil case strings.HasPrefix(cmd, "test -f "): path := strings.Trim(strings.TrimPrefix(cmd, "test -f "), "'") if _, ok := f.files[path]; !ok { diff --git a/internal/caddy/webhook.go b/internal/caddy/webhook.go new file mode 100644 index 0000000..184fbda --- /dev/null +++ b/internal/caddy/webhook.go @@ -0,0 +1,223 @@ +// Persisted webhook routing (audit T26/T27). +// +// The webhook listener's route used to be injected through Caddy's admin +// API at runtime — a route that lived ONLY in the running process's +// memory. Every ordinary deploy regenerates the on-disk Caddyfile and +// reloads, which erased the webhook route (a webhook-triggered deploy could +// remove its own future trigger), and any Caddy restart lost it too. +// +// The route is now PERSISTED: a small per-app config file records the +// webhook (hosts, path, listener port), and the fragment is rendered INSIDE +// the app's managed site block — ahead of the terminal reverse_proxy, so +// POSTs to /teploy-webhook/ reach the listener without passing the +// app's own auth gate (the behavior the runtime route's global-first +// evaluation provided). Every managed-block render (deploy, rollback, +// maintenance) re-applies the fragment from the config file, under the same +// Caddyfile lock + adapt gate + reload/verify transaction as every other +// route edit. +package caddy + +import ( + "context" + "encoding/json" + "fmt" + "strconv" + "strings" + + "github.com/useteploy/teploy/internal/ssh" +) + +// webhookConfigPath is the per-app persisted webhook route descriptor +// (/deployments//.webhook-route), 0600. +func webhookConfigPath(app string) string { + return fmt.Sprintf("/deployments/%s/.webhook-route", app) +} + +// webhookRouteConfig is the persisted shape. Hosts is the validated host +// list the route matches; Path is the request path; Port is the LOCAL +// listener port Caddy proxies to (the dial is always host.docker.internal +// — see the old runtime injector's note about network namespaces). +type webhookRouteConfig struct { + Hosts []string `json:"hosts"` + Path string `json:"path"` + Port int `json:"port"` +} + +// readServerFile frames a file read with a confirmed-absence distinction. +func readServerFile(ctx context.Context, exec ssh.Executor, path string) ([]byte, bool, error) { + out, err := exec.Run(ctx, fmt.Sprintf( + "if [ ! -e %s ]; then printf 'absent\\n'; else printf 'present\\n'; cat -- %s; fi", + ssh.ShellQuote(path), ssh.ShellQuote(path))) + if err != nil { + return nil, false, fmt.Errorf("reading %s: %w", path, err) + } + out = strings.TrimRight(out, "\n") + if out == "absent" || strings.HasPrefix(out, "absent\n") { + return nil, false, nil + } + if !strings.HasPrefix(out, "present\n") { + if out == "present" { + return nil, true, nil + } + return nil, false, fmt.Errorf("reading %s: unrecognized output framing", path) + } + return []byte(strings.TrimPrefix(out, "present\n")), true, nil +} + +// loadWebhookConfig reads the persisted webhook descriptor. Absent is +// (nil, nil); unreadable/corrupt is an error — a corrupt descriptor must +// abort the route edit, never silently drop the webhook route. +func loadWebhookConfig(ctx context.Context, exec ssh.Executor, app string) (*webhookRouteConfig, error) { + data, present, err := readServerFile(ctx, exec, webhookConfigPath(app)) + if err != nil || !present { + return nil, err + } + var cfg webhookRouteConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("parsing the persisted webhook route for %s: %w", app, err) + } + if len(cfg.Hosts) == 0 || cfg.Path == "" || cfg.Port < 1 || cfg.Port > 65535 { + return nil, fmt.Errorf("persisted webhook route for %s is incomplete (hosts/path/port)", app) + } + return &cfg, nil +} + +// webhookFragment renders the managed webhook fragment for injection at the +// top of the app's site block. handle blocks evaluate before the site's +// terminal reverse_proxy and before its auth directives, so the webhook +// stays reachable with its own HMAC boundary exactly as the old +// global-first runtime route was. +func webhookFragment(app string, cfg *webhookRouteConfig) string { + matcher := "teploy_hook_" + app + var b strings.Builder + b.WriteString(fmt.Sprintf("\t# TEPLOY WEBHOOK BEGIN %s\n", app)) + b.WriteString(fmt.Sprintf("\t@%s {\n\t\tmethod POST\n\t\tpath %s\n\t}\n", matcher, cfg.Path)) + b.WriteString(fmt.Sprintf("\thandle @%s {\n\t\treverse_proxy host.docker.internal:%s\n\t}\n", matcher, strconv.Itoa(cfg.Port))) + b.WriteString(fmt.Sprintf("\t# TEPLOY WEBHOOK END %s\n", app)) + return b.String() +} + +// stripWebhookFragment removes the app's webhook fragment from its managed +// block (idempotent; a fragment that never existed is a no-op). +func stripWebhookFragment(block, app string) string { + begin := fmt.Sprintf("\t# TEPLOY WEBHOOK BEGIN %s", app) + end := fmt.Sprintf("\t# TEPLOY WEBHOOK END %s", app) + var out []string + skipping := false + for _, line := range strings.Split(block, "\n") { + if strings.TrimSpace(line) == strings.TrimSpace(begin) { + skipping = true + continue + } + if skipping { + if strings.TrimSpace(line) == strings.TrimSpace(end) { + skipping = false + } + continue + } + out = append(out, line) + } + result := strings.Join(out, "\n") + for strings.Contains(result, "\n\n\n") { + result = strings.ReplaceAll(result, "\n\n\n", "\n\n") + } + return result +} + +// injectWebhookFragment inserts the fragment directly after the site +// block's opening line (the address line). The block arrives without its +// TEPLOY markers (extractCaddyfileBlock's contract). +func injectWebhookFragment(block, app string, cfg *webhookRouteConfig) (string, error) { + lines := strings.Split(block, "\n") + if len(lines) < 2 || !strings.HasSuffix(strings.TrimSpace(lines[0]), "{") { + return "", fmt.Errorf("cannot place the webhook route: %s's site block has no recognizable opening line", app) + } + fragment := webhookFragment(app, cfg) + out := append([]string{lines[0], strings.TrimRight(fragment, "\n")}, lines[1:]...) + return strings.Join(out, "\n"), nil +} + +// applyWebhookToBlock strips any existing fragment from the app's block and +// re-injects it from the PERSISTED descriptor when one exists. Returns the +// updated whole-file content. Called inside the Caddyfile mutation lock. +func (c *Client) applyWebhookToBlock(ctx context.Context, app, content string) (string, error) { + begin := fmt.Sprintf(markerBeginFmt, app) + end := fmt.Sprintf(markerEndFmt, app) + block := extractCaddyfileBlock(content, begin, end) + if block == "" { + // No managed app block (external ingress, not yet deployed): + // nothing to attach to. The descriptor, if written, applies at the + // first managed render. + return content, nil + } + cfg, err := loadWebhookConfig(ctx, c.exec, app) + if err != nil { + return "", err + } + block = stripWebhookFragment(block, app) + if cfg != nil { + if block, err = injectWebhookFragment(block, app, cfg); err != nil { + return "", err + } + } + updated, err := removeCaddyfileBlock(content, begin, end) + if err != nil { + return "", err + } + wrapped := begin + "\n" + strings.TrimRight(block, "\n") + "\n" + end + return strings.TrimRight(updated, "\n") + "\n\n" + wrapped + "\n", nil +} + +// SetWebhookRoute persists the app's webhook route descriptor and applies +// it to the app's managed site block in one Caddyfile transaction. Reports +// whether the fragment is LIVE: false means the app has no managed block +// yet (not deployed / external ingress) — the descriptor is stored and the +// route attaches on the first deploy that renders one. +func (c *Client) SetWebhookRoute(ctx context.Context, app, domain string, port int) error { + hosts, err := parseDomains(domain) + if err != nil { + return err + } + if len(hosts) == 0 { + return fmt.Errorf("SetWebhookRoute: domain must be non-empty") + } + if port < 1 || port > 65535 { + return fmt.Errorf("SetWebhookRoute: listener port must be in 1..65535 (got %d)", port) + } + cfg := webhookRouteConfig{Hosts: hosts, Path: "/teploy-webhook/" + app, Port: port} + data, err := json.Marshal(cfg) + if err != nil { + return err + } + // Descriptor first, then the route edit reads it inside the mutation + // lock — a crash between the two leaves a descriptor the next deploy + // applies; the reverse could render a fragment with no source of truth. + if err := ssh.UploadAtomic(ctx, c.exec, strings.NewReader(string(data)+"\n"), webhookConfigPath(app), "0600"); err != nil { + return fmt.Errorf("persisting the webhook route descriptor: %w", err) + } + return c.mutate(ctx, func(prev string) (string, error) { + return c.applyWebhookToBlock(ctx, app, prev) + }) +} + +// HasManagedBlock reports whether the app currently has a managed site +// block (the precondition for a live webhook fragment). +func (c *Client) HasManagedBlock(ctx context.Context, app string) (bool, error) { + prev, err := c.exec.Run(ctx, "cat "+caddyfilePath) + if err != nil { + return false, fmt.Errorf("reading caddyfile (did setup run?): %w", err) + } + return extractCaddyfileBlock(prev, fmt.Sprintf(markerBeginFmt, app), fmt.Sprintf(markerEndFmt, app)) != "", nil +} + +// RemoveWebhookRoute deletes the persisted descriptor and strips the +// fragment from the app's block in one transaction. No-op when neither +// exists. +func (c *Client) RemoveWebhookRoute(ctx context.Context, app string) error { + if _, err := c.exec.Run(ctx, "rm -f -- "+ssh.ShellQuote(webhookConfigPath(app))); err != nil { + return fmt.Errorf("removing the webhook route descriptor: %w", err) + } + return c.mutate(ctx, func(prev string) (string, error) { + return c.applyWebhookToBlock(ctx, app, prev) + }) +} diff --git a/internal/caddy/webhook_test.go b/internal/caddy/webhook_test.go new file mode 100644 index 0000000..e5931ad --- /dev/null +++ b/internal/caddy/webhook_test.go @@ -0,0 +1,110 @@ +package caddy + +import ( + "context" + "strings" + "testing" + + "github.com/useteploy/teploy/internal/ssh" +) + +func webhookTestMocks() *ssh.MockExecutor { + return ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, + ) +} + +// TestWebhookRoute_SurvivesRedeploy is the T26 core regression: the webhook +// route used to exist only in Caddy's runtime API, so the next ordinary +// deploy's Caddyfile regeneration + reload erased it. The persisted +// fragment must survive every managed re-render. +func TestWebhookRoute_SurvivesRedeploy(t *testing.T) { + ctx := context.Background() + mock := webhookTestMocks() + c := NewClient(mock) + + if err := c.SetWebhookRoute(ctx, "myapp", "myapp.com", 9876); err != nil { + t.Fatalf("SetWebhookRoute: %v", err) + } + // No managed block yet: nothing to attach to; the descriptor persists. + if strings.Contains(string(mock.Files["/deployments/caddy/Caddyfile"]), "teploy-webhook") { + t.Fatal("fragment attached without a managed site block") + } + + // First deploy renders the app block — the fragment attaches. + if err := c.SetRoute(ctx, "myapp", "myapp.com", "myapp-web-v1", 3000, TLS{}, "", nil, Firewall{}, Access{}); err != nil { + t.Fatalf("SetRoute: %v", err) + } + file := string(mock.Files["/deployments/caddy/Caddyfile"]) + if !strings.Contains(file, "path /teploy-webhook/myapp") || !strings.Contains(file, "host.docker.internal:9876") { + t.Fatalf("webhook fragment not attached to the app block:\n%s", file) + } + + // A REDEPLOY re-renders the block — the fragment must survive (T26). + if err := c.SetRoute(ctx, "myapp", "myapp.com", "myapp-web-v2", 3000, TLS{}, "", nil, Firewall{}, Access{}); err != nil { + t.Fatalf("SetRoute v2: %v", err) + } + file = string(mock.Files["/deployments/caddy/Caddyfile"]) + if !strings.Contains(file, "path /teploy-webhook/myapp") { + t.Fatalf("webhook fragment erased by a redeploy:\n%s", file) + } + if !strings.Contains(file, "myapp-web-v2:3000") || strings.Contains(file, "myapp-web-v1") { + t.Fatalf("route did not move to v2:\n%s", file) + } +} + +// TestWebhookRoute_PortAndDomainsHonored is the T27 regression: the runtime +// injector hardcoded port 9876 and inserted a comma-separated domain list +// as ONE host value. +func TestWebhookRoute_PortAndDomainsHonored(t *testing.T) { + ctx := context.Background() + mock := webhookTestMocks() + c := NewClient(mock) + if err := c.SetRoute(ctx, "myapp", "myapp.com, www.myapp.com", "myapp-web-v1", 3000, TLS{}, "", nil, Firewall{}, Access{}); err != nil { + t.Fatalf("SetRoute: %v", err) + } + if err := c.SetWebhookRoute(ctx, "myapp", "myapp.com, www.myapp.com", 9911); err != nil { + t.Fatalf("SetWebhookRoute: %v", err) + } + file := string(mock.Files["/deployments/caddy/Caddyfile"]) + if !strings.Contains(file, "host.docker.internal:9911") { + t.Errorf("configured port 9911 not honored:\n%s", file) + } + if strings.Contains(file, "host.docker.internal:9876") { + t.Errorf("hardcoded default port leaked:\n%s", file) + } + if !strings.Contains(file, "myapp.com, www.myapp.com {") { + t.Errorf("multi-domain site address not rendered as before:\n%s", file) + } +} + +// TestWebhookRoute_RemovalStripsFragment: RemoveWebhookRoute deletes the +// descriptor and strips the fragment in one transaction. +func TestWebhookRoute_RemovalStripsFragment(t *testing.T) { + ctx := context.Background() + mock := webhookTestMocks() + c := NewClient(mock) + if err := c.SetRoute(ctx, "myapp", "myapp.com", "myapp-web-v1", 3000, TLS{}, "", nil, Firewall{}, Access{}); err != nil { + t.Fatalf("SetRoute: %v", err) + } + if err := c.SetWebhookRoute(ctx, "myapp", "myapp.com", 9876); err != nil { + t.Fatalf("SetWebhookRoute: %v", err) + } + if err := c.RemoveWebhookRoute(ctx, "myapp"); err != nil { + t.Fatalf("RemoveWebhookRoute: %v", err) + } + file := string(mock.Files["/deployments/caddy/Caddyfile"]) + if strings.Contains(file, "teploy-webhook") { + t.Errorf("fragment not stripped:\n%s", file) + } + if _, ok := mock.Files["/deployments/myapp/.webhook-route"]; ok { + t.Error("descriptor not deleted") + } + if !strings.Contains(file, "myapp-web-v1:3000") { + t.Errorf("the app's own route must survive webhook removal:\n%s", file) + } +} diff --git a/internal/cli/autodeploy.go b/internal/cli/autodeploy.go index 1e49e11..d57d287 100644 --- a/internal/cli/autodeploy.go +++ b/internal/cli/autodeploy.go @@ -127,13 +127,14 @@ func runAutoDeploySetup(flags *Flags, branch, secret string) error { Branch: branch, Secret: secret, TeployBinaryPath: teployBinaryPath, + Port: autodeploy.DefaultPort, } if err := mgr.Setup(ctx, cfg); err != nil { return err } - if err := mgr.SetupCaddyRoute(ctx, appCfg.App, appCfg.Domain); err != nil { + if err := mgr.SetupCaddyRoute(ctx, appCfg.App, appCfg.Domain, cfg.Port); err != nil { fmt.Fprintf(os.Stderr, "Warning: could not add Caddy route: %v\n", err) fmt.Fprintf(os.Stderr, " You may need to add the webhook route manually\n") } diff --git a/internal/cli/autodeploy_serve.go b/internal/cli/autodeploy_serve.go index 189bf0d..54d8e22 100644 --- a/internal/cli/autodeploy_serve.go +++ b/internal/cli/autodeploy_serve.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "os" + "path/filepath" "strings" "sync" "time" @@ -76,12 +77,19 @@ func runAutoDeployServe(app, branch string, port int, strictEnv bool) error { if err != nil { return fmt.Errorf("reading webhook secret from %s (run `teploy autodeploy setup` first): %w", autodeploy.SecretPath(app), err) } - secret := strings.TrimSpace(string(secretBytes)) - if secret == "" { + // The stored bytes are the HMAC key, used VERBATIM (audit T32): setup + // rejects whitespace-wrapped secrets, so silently trimming here would + // sign with different bytes than a hand-edited file actually contains + // and turn a visible configuration mistake into an unexplained 401. + if len(secretBytes) == 0 { // An empty secret authenticates nothing (any unsigned request would // compare equal) — fail closed at startup (audit F43). return fmt.Errorf("webhook secret at %s is empty — every request would be unauthenticated; re-run `teploy autodeploy setup`", autodeploy.SecretPath(app)) } + secret := string(secretBytes) + if secret != strings.TrimSpace(secret) { + return fmt.Errorf("webhook secret at %s has leading/trailing whitespace — the HMAC key is used verbatim, so verification would fail against a provider sending the trimmed value; fix the file (or re-run `teploy autodeploy setup`)", autodeploy.SecretPath(app)) + } logPath := fmt.Sprintf("/deployments/%s/autodeploy.log", app) logFile, err := os.OpenFile(logPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) @@ -346,6 +354,15 @@ func triggerAutoDeploy(ctx context.Context, executor ssh.Executor, app, branch, return fmt.Errorf("teploy.yml in %s declares app %q, expected %q — refusing to deploy the wrong app", buildDir, appCfg.App, app) } + // Local file references resolve against the CHECKOUT, not the resident + // process's working directory (audit T31): the systemd unit has no + // WorkingDirectory, so a relative tls.cert/key that worked in a manual + // invocation resolved against "/" under the service and read the wrong + // file (or nothing). Absolute paths are preserved as-is. + if appCfg.TLS != nil && !appCfg.TLS.Internal { + appCfg.TLS = resolveTLSFromRoot(appCfg.TLS, buildDir) + } + // Resolve env_files from the CHECKOUT, with the same single-pass ${VAR} // expansion rule manual deploys use (audit F59/F66): without this, the // same manifest received different container env depending on whether @@ -432,3 +449,20 @@ func triggerAutoDeploy(ctx context.Context, executor ssh.Executor, app, branch, att := releasemeta.MustAttempt(app, version) return deployBuiltImageFenced(ctx, executor, appCfg, image, version, "localhost", false, needsBuild, lk, &att) } + +// resolveTLSFromRoot returns a COPY of tls with relative cert/key paths +// resolved against root (audit T31) — the resident autodeploy process runs +// under systemd with no WorkingDirectory, so relative paths must never be +// interpreted against whatever cwd it inherited. +func resolveTLSFromRoot(tls *config.TLSConfig, root string) *config.TLSConfig { + resolve := func(p string) string { + if p == "" || filepath.IsAbs(p) { + return p + } + return filepath.Join(root, p) + } + out := *tls + out.Cert = resolve(out.Cert) + out.Key = resolve(out.Key) + return &out +} diff --git a/internal/cli/autodeploy_serve_test.go b/internal/cli/autodeploy_serve_test.go index ba3d5c8..0253be6 100644 --- a/internal/cli/autodeploy_serve_test.go +++ b/internal/cli/autodeploy_serve_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/useteploy/teploy/internal/autodeploy" + "github.com/useteploy/teploy/internal/config" ) func githubSign(secret string, body []byte) string { @@ -331,3 +332,21 @@ func TestWebhookHandler_ReusedDeliveryIDDifferentContentNotSuppressed(t *testing t.Errorf("replayed content must be a no-op, got %d", triggerCount) } } + +// TestResolveTLSFromRoot is the T31 regression: the resident autodeploy +// process runs under systemd without a WorkingDirectory, so relative TLS +// paths must resolve against the checkout, and absolute paths must pass +// through untouched. +func TestResolveTLSFromRoot(t *testing.T) { + in := &config.TLSConfig{Cert: "certs/app.crt", Key: "/etc/absolute.key"} + out := resolveTLSFromRoot(in, "/deployments/myapp/build") + if out.Cert != "/deployments/myapp/build/certs/app.crt" { + t.Errorf("relative cert not resolved against the checkout: %q", out.Cert) + } + if out.Key != "/etc/absolute.key" { + t.Errorf("absolute key must pass through: %q", out.Key) + } + if in.Cert != "certs/app.crt" { + t.Errorf("input TLSConfig mutated: %+v", in) + } +} diff --git a/internal/releasemeta/releasemeta_test.go b/internal/releasemeta/releasemeta_test.go index 0062fe8..9b100c0 100644 --- a/internal/releasemeta/releasemeta_test.go +++ b/internal/releasemeta/releasemeta_test.go @@ -59,7 +59,11 @@ func TestRead_AbsentVsPresentVsUnreadable(t *testing.T) { }) t.Run("transport failure is an error, not absence", func(t *testing.T) { - mock := ssh.NewMockExecutor("1.2.3.4") // no fixture: unexpected command + // The mock answers framed reads from its recorded file state when + // it has one, so the transport failure is modeled explicitly. + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/meta/v1.json' ]", Err: fmt.Errorf("ssh: connection reset")}, + ) if _, err := Read(context.Background(), mock, "myapp", "v1"); err == nil { t.Fatal("transport failure must be an error, never silent absence") } diff --git a/internal/ssh/mock.go b/internal/ssh/mock.go index 3e0daa7..aa36248 100644 --- a/internal/ssh/mock.go +++ b/internal/ssh/mock.go @@ -75,6 +75,16 @@ func (m *MockExecutor) Run(ctx context.Context, cmd string) (string, error) { m.Calls = append(m.Calls, cmd) } + // `cat ` answers from the recorded file state when the mock has + // one (the real server re-reads whatever earlier writes left); an + // explicit registration still wins for paths the mock has no file for. + if rest, ok := strings.CutPrefix(cmd, "cat "); ok && !strings.Contains(rest, " | ") { + path := strings.Trim(strings.TrimSpace(rest), "'") + if data, present := m.Files[path]; present { + m.mu.Unlock() + return string(data), nil + } + } for i, c := range m.commands { if mockCommandMatches(cmd, c.Match) { if c.Once { @@ -113,6 +123,21 @@ func (m *MockExecutor) Run(ctx context.Context, cmd string) (string, error) { m.mu.Unlock() return "", nil } + // Framed server-file reads (state.ReadRemoteFile, caddy's webhook + // descriptor): answer from the recorded file state when no explicit + // registration matches, so tests exercising route/state edits do not + // need to stub every read individually. + if rest, ok := strings.CutPrefix(cmd, "if [ ! -e "); ok && strings.Contains(cmd, "printf 'absent\\n'") { + if pathQ, _, found := strings.Cut(rest, " ]; then"); found { + path := strings.Trim(pathQ, "'") + if data, present := m.Files[path]; present { + m.mu.Unlock() + return "present\n" + string(data), nil + } + m.mu.Unlock() + return "absent", nil + } + } m.mu.Unlock() return "", fmt.Errorf("mock: unexpected command: %s", cmd) } From 5401f87f7c833076af8579caef6b07ddcf9f8477 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:02:23 -0700 Subject: [PATCH 11/13] =?UTF-8?q?fix(cli,caddy):=20T57+T58+T62=20=E2=80=94?= =?UTF-8?q?=20fleet=20LB=20activation=20is=20a=20required=20phase,=20detac?= =?UTF-8?q?hed=20recovery=20waves,=20maintenance=20serialized=20with=20dep?= =?UTF-8?q?loyment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T57: a failed load-balancer update after a fully successful wave is now a nonzero exit ('backends deployed but load-balancer activation failed') — the old shape printed a warning and returned nil, so a green CLI exit proved nothing about reachability while backends served new versions on new ports behind old targets. T58 (contained half): both fleet rollback waves (partial failure and canary gate) run on bounded detached recovery contexts — the deploy context is signal-cancelled exactly when the operator interrupts, and recovery that skips itself on the dead context is how a Ctrl-C strands half a fleet on the new version. The generation-identity half of T58 (compensating only the recorded predecessor) stays deferred with the T04 family. T62 (contained half): maintenance on/off takes the app's fenced deploy lock, the --app path verifies the AUTHORITATIVE server ingress mode instead of assuming caddy, the stash is read inside the Caddyfile mutation transaction, and it is deleted only when this transaction actually restored one — a concurrent maintenance-on's fresh stash can no longer be clobbered by a stale read or deleted by a racing off. The versioned-desired-state redesign stays deferred. --- internal/caddy/caddy.go | 66 +++++++++++++++++++++++------------- internal/caddy/caddy_test.go | 9 ++--- internal/cli/deploy.go | 32 ++++++++++++----- internal/cli/maintenance.go | 32 +++++++++++++++-- 4 files changed, 100 insertions(+), 39 deletions(-) diff --git a/internal/caddy/caddy.go b/internal/caddy/caddy.go index ac940bc..93703a6 100644 --- a/internal/caddy/caddy.go +++ b/internal/caddy/caddy.go @@ -353,8 +353,14 @@ func (c *Client) SetMaintenance(ctx context.Context, app, domain string) error { // then IS the maintenance block — so stash-on overwrote the // original route and maintenance-off restored maintenance // forever. The first stash wins; it is deleted only by a - // successful RemoveMaintenance. - if _, statErr := c.exec.Run(ctx, "test -f "+ssh.ShellQuote(stash)); statErr != nil { + // successful RemoveMaintenance. Existence is CONFIRMED with a + // framed read (T62): `test -f` treated a transport failure as + // "missing" and overwrote a stash that might exist. + _, stashed, err := readServerFile(ctx, c.exec, stash) + if err != nil { + return "", fmt.Errorf("checking the maintenance stash for %s: %w", app, err) + } + if !stashed { if err := c.exec.Upload(ctx, strings.NewReader(cur), stash, "0644"); err != nil { return "", fmt.Errorf("stashing route for maintenance: %w", err) } @@ -373,42 +379,54 @@ func (c *Client) SetMaintenance(ctx context.Context, app, domain string) error { } // RemoveMaintenance disables maintenance mode, restoring the stashed route -// block. It fails safe: a missing stash is a no-op, and a stash that exists but -// can't be read (or is empty) aborts WITHOUT touching the route — the previous -// version ignored the read error, so any transient SSH/read failure rendered an -// empty block and silently deleted the app's route, taking the domain offline. +// block. The stash is read INSIDE the mutation transaction (audit T62): +// the old shape read it before taking the Caddy lock and deleted it after, +// so a concurrent maintenance-on between the two could overwrite the stash +// the read had just captured, or the deletion could remove a stash a +// concurrent operation had just written. A missing stash is a no-op, and a +// stash that exists but can't be read (or is empty) aborts WITHOUT +// touching the route. func (c *Client) RemoveMaintenance(ctx context.Context, app string) error { stash := fmt.Sprintf(maintStashFmt, app) - // Missing stash → maintenance isn't active (or was already removed). No-op. - // `test -f` so a genuine read error below isn't masked by `cat 2>/dev/null`. - if _, err := c.exec.Run(ctx, "test -f "+stash); err != nil { - return nil - } - saved, err := c.exec.Run(ctx, "cat "+stash) - if err != nil { - return fmt.Errorf("reading stashed maintenance route (route left unchanged): %w", err) - } - restored := strings.Trim(saved, "\n") - if restored == "" { - return fmt.Errorf("stashed maintenance route for %s is empty — refusing to remove the route; delete %s manually if this is intended", app, stash) - } - + stashRemoved := false if err := c.mutate(ctx, func(prev string) (string, error) { + data, present, err := readServerFile(ctx, c.exec, stash) + if err != nil { + return "", fmt.Errorf("reading stashed maintenance route (route left unchanged): %w", err) + } + if !present { + // Maintenance isn't active (or was already removed). No-op: + // returning prev unchanged skips the write/reload entirely. + return prev, nil + } + restored := strings.Trim(string(data), "\n") + if restored == "" { + return "", fmt.Errorf("stashed maintenance route for %s is empty — refusing to remove the route; delete %s manually if this is intended", app, stash) + } updated, err := renderUpdated(prev, app, nil, restored) if err != nil { return "", err } // The stash may predate a webhook port change; normalize the // fragment against the CURRENT persisted descriptor (webhook.go). - return c.applyWebhookToBlock(ctx, app, updated) + updated, err = c.applyWebhookToBlock(ctx, app, updated) + if err != nil { + return "", err + } + stashRemoved = true + return updated, nil }); err != nil { return err } - // Delete the stash only after the reload succeeded, so a failed (rolled - // back) reload can be retried. - c.exec.Run(ctx, "rm -f "+stash) + // Delete the stash only when the reload succeeded (a failed/rolled-back + // reload can be retried), and only when this transaction actually + // restored one — deleting a stash a concurrent maintenance-on wrote + // would make THAT maintenance unexitable (audit T62). + if stashRemoved { + c.exec.Run(ctx, "rm -f -- "+ssh.ShellQuote(stash)) + } return nil } diff --git a/internal/caddy/caddy_test.go b/internal/caddy/caddy_test.go index 476bf3d..1fd333b 100644 --- a/internal/caddy/caddy_test.go +++ b/internal/caddy/caddy_test.go @@ -287,11 +287,12 @@ func TestRemoveMaintenance(t *testing.T) { existing := "{\n\tadmin 127.0.0.1:2019\n}\n\n" + "# TEPLOY BEGIN myapp\nmyapp.com {\n\trespond 503\n}\n# TEPLOY END myapp\n" cmds := append(lockCmds(existing), - ssh.MockCommand{Match: "test -f " + fmt.Sprintf(maintStashFmt, "myapp"), Output: ""}, - ssh.MockCommand{Match: "cat " + fmt.Sprintf(maintStashFmt, "myapp"), Output: "myapp.com {\n\treverse_proxy myapp:80\n}"}, - ssh.MockCommand{Match: "rm -f " + fmt.Sprintf(maintStashFmt, "myapp"), Output: ""}, + ssh.MockCommand{Match: "rm -f -- '" + fmt.Sprintf(maintStashFmt, "myapp") + "'", Output: ""}, ) mock := ssh.NewMockExecutor("1.2.3.4", cmds...) + // The stash is read INSIDE the mutation transaction via the framed + // server-file read (T62); stage it in the mock's file state. + mock.Files[fmt.Sprintf(maintStashFmt, "myapp")] = []byte("myapp.com {\n\treverse_proxy myapp:80\n}") client := NewClient(mock) if err := client.RemoveMaintenance(context.Background(), "myapp"); err != nil { @@ -305,7 +306,7 @@ func TestRemoveMaintenance(t *testing.T) { if strings.Contains(got, "respond 503") { t.Errorf("expected maintenance block removed:\n%s", got) } - if !calledWith(mock, "rm -f "+fmt.Sprintf(maintStashFmt, "myapp")) { + if !calledWith(mock, "rm -f -- '"+fmt.Sprintf(maintStashFmt, "myapp")+"'") { t.Error("expected the maintenance stash to be cleaned up") } } diff --git a/internal/cli/deploy.go b/internal/cli/deploy.go index bd923e8..0609c8d 100644 --- a/internal/cli/deploy.go +++ b/internal/cli/deploy.go @@ -893,8 +893,13 @@ func runMultiDeploy(flags *Flags, appCfg *config.AppConfig, image, version strin if failCount == 0 { if len(successTargets) > 0 { + // Front-door activation is a required deployment phase (audit + // T57): the old shape printed a warning and returned nil, so a + // green CLI exit did not prove the deployment was reachable — + // backends could serve new versions on new ports while the LB + // still targeted the old ones. if err := updateLoadBalancer(ctx, flags, appCfg, serversPath, successTargets); err != nil { - fmt.Fprintf(os.Stderr, "Warning: LB update failed: %v\n", err) + return fmt.Errorf("backends deployed but load-balancer activation failed — the fleet needs reconciliation: %w", err) } } return nil @@ -933,12 +938,18 @@ func runMultiDeploy(flags *Flags, appCfg *config.AppConfig, image, version strin fmt.Printf("\n%d of %d servers failed — rolling back the %d server(s) that succeeded...\n", failCount, len(targets), len(successTargets)) - // Best-effort: attempt to roll back EVERY succeeded server even if one - // rollback fails — otherwise a single rollback failure would fail-fast - // and strand the remaining servers on the new version (M1). - rollbackResults := multideploy.ParallelDeployAll(ctx, successTargets, parallel, func(ctx context.Context, target multideploy.ServerTarget, out io.Writer) error { - return rollbackSingleServer(ctx, appCfg, target, out) - }, os.Stdout) + // Best-effort: attempt to roll back EVERY succeeded server even if one + // rollback fails — otherwise a single rollback failure would fail-fast + // and strand the remaining servers on the new version (M1). The wave + // runs on a bounded DETACHED recovery context (audit T58): the deploy + // context is signal-cancelled exactly when the operator interrupts, + // and recovery work that skips itself because the cancelled context + // disappeared is how a Ctrl-C strands half a fleet on the new version. + rollbackCtx, rollbackCancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer rollbackCancel() + rollbackResults := multideploy.ParallelDeployAll(rollbackCtx, successTargets, parallel, func(ctx context.Context, target multideploy.ServerTarget, out io.Writer) error { + return rollbackSingleServer(ctx, appCfg, target, out) + }, os.Stdout) var rolledBack, firstDeploys, rollbackFailed []string for _, r := range rollbackResults { @@ -990,7 +1001,12 @@ func rollbackFailedWave(ctx context.Context, appCfg *config.AppConfig, wave []mu } if len(succeeded) > 0 { fmt.Printf("Rolling back %d canary server(s) that succeeded...\n", len(succeeded)) - rollbackResults := multideploy.ParallelDeployAll(ctx, succeeded, parallel, func(ctx context.Context, target multideploy.ServerTarget, out io.Writer) error { + // Detached bounded recovery context (audit T58, same rationale as + // the partial-failure rollback): an interrupted canary wave must + // still converge its succeeded servers. + recoveryCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel() + rollbackResults := multideploy.ParallelDeployAll(recoveryCtx, succeeded, parallel, func(ctx context.Context, target multideploy.ServerTarget, out io.Writer) error { return rollbackSingleServer(ctx, appCfg, target, out) }, os.Stdout) for _, r := range rollbackResults { diff --git a/internal/cli/maintenance.go b/internal/cli/maintenance.go index 12d0c4e..ecc4ce5 100644 --- a/internal/cli/maintenance.go +++ b/internal/cli/maintenance.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" "github.com/useteploy/teploy/internal/caddy" "github.com/useteploy/teploy/internal/config" + "github.com/useteploy/teploy/internal/state" ) func newMaintenanceCmd(flags *Flags) *cobra.Command { @@ -53,9 +54,10 @@ func newMaintenanceOffCmd(flags *Flags) *cobra.Command { } func runMaintenanceToggle(flags *Flags, appName string, enable bool) error { - // With --app there's no teploy.yml to check ingress against; the server - // state doesn't record ingress mode. Only enforce the Caddy-required check - // in the cwd path where we have full config. + // With --app there's no teploy.yml to check ingress against, so the + // AUTHORITATIVE server state decides (audit T62): the old shape assumed + // caddy ingress on that path, and a maintenance toggle against a + // host/external-ingress app happily rewrote routes nothing serves. if appName == "" { appCfg, err := config.LoadApp(".") if err != nil { @@ -79,6 +81,30 @@ func runMaintenanceToggle(flags *Flags, appName string, enable bool) error { } defer executor.Close() + if appName != "" { + st, err := state.Read(ctx, executor, appName) + if err != nil { + return fmt.Errorf("reading server state for %s: %w", appName, err) + } + if st != nil && st.IngressMode != "" && st.IngressMode != "caddy" { + return fmt.Errorf("'teploy maintenance' requires Teploy-managed Caddy; %s uses ingress: %s (per its server state) — route traffic away via that ingress instead", appName, st.IngressMode) + } + } + + // Maintenance is serialized with deploys under the SAME fenced app lock + // (audit T62): the toggle used to run unlocked, so a deploy during + // maintenance re-rendered the app's route while the stash held a route + // for the now-stopped release — and maintenance-off then restored that + // stale route over the deploy's live one. + if err := state.EnsureAppDir(ctx, executor, appCfg.App); err != nil { + return fmt.Errorf("creating app directory: %w", err) + } + lk, err := state.AcquireLockFenced(ctx, executor, appCfg.App) + if err != nil { + return fmt.Errorf("acquiring deploy lock: %w", err) + } + defer state.ReleaseLockFenced(executor, lk, appCfg.App) + client := caddy.NewClient(executor) if enable { From 555581b682f22d3d9309a1e56c4c2f50efa6ae29 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:05:38 -0700 Subject: [PATCH 12/13] =?UTF-8?q?fix(cli,deploy,ci):=20T48+T49+T20+T61=20?= =?UTF-8?q?=E2=80=94=20bounded=20update=20extraction,=20updater=20context,?= =?UTF-8?q?=20proxy-free=20probes,=20least-privilege=20workflows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T48 (narrowing A47): update extraction is bounded and single-binary — declared member sizes are checked before any byte is read, reads are limited, non-regular and duplicate matching entries are refused, and the entry count is capped. The old bare io.ReadAll let a small compressed member expand until memory exhaustion before any checksum ran. T49 (cancellation half): the updater derives its context from the Cobra command, so Ctrl-C actually cancels the check/download/verify sequence; the update-selection policy (downgrades, prereleases) stays deferred (A48). T20 (contained half): health probes pass curl --noproxy '*' — an inherited HTTP_PROXY made host-local readiness ask a proxy about a loopback address. The explicit HTTP/TCP mode redesign stays deferred (A22). T61: CI and the release workflow are read-only by default; contents:write is granted only to the publishing job. (GoReleaser version pinning remains the A49 owner item — pins are not invented here.) --- .github/workflows/ci.yml | 4 ++ .github/workflows/release.yml | 6 +- internal/cli/update.go | 101 +++++++++++++++++++++++++++++----- internal/cli/update_test.go | 68 +++++++++++++++++++++++ internal/deploy/health.go | 6 +- 5 files changed, 167 insertions(+), 18 deletions(-) create mode 100644 internal/cli/update_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b1f0af7..2e2c05c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,10 @@ on: branches: [main] workflow_call: {} +# Least privilege by default (audit T61): CI only needs to read the repo. +permissions: + contents: read + jobs: test: runs-on: ubuntu-latest diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 71a88fa..d1ba274 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -4,8 +4,10 @@ on: push: tags: ['v*'] +# Least privilege by default (audit T61): the workflow as a whole is +# read-only; only the publishing job escalates to contents: write. permissions: - contents: write + contents: read jobs: verify: @@ -17,6 +19,8 @@ jobs: release: needs: verify runs-on: ubuntu-latest + permissions: + contents: write steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: diff --git a/internal/cli/update.go b/internal/cli/update.go index d4daa5b..55a2785 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -40,7 +40,10 @@ func newUpdateCmd(currentVersion string) *cobra.Command { Use: "update", Short: "Update teploy to the latest version", RunE: func(cmd *cobra.Command, args []string) error { - return runUpdate(currentVersion, force) + // Derive from the command's context (audit T49's cancellation + // half): a background context ignored Ctrl-C for the whole + // check/download/verify sequence. + return runUpdate(cmd.Context(), currentVersion, force) }, } @@ -49,12 +52,12 @@ func newUpdateCmd(currentVersion string) *cobra.Command { return cmd } -func runUpdate(currentVersion string, force bool) error { +func runUpdate(ctx context.Context, currentVersion string, force bool) error { fmt.Printf("Current version: %s\n", currentVersion) // Fetch latest release info. fmt.Println("Checking for updates...") - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() latest, err := fetchLatestRelease(ctx) @@ -234,24 +237,65 @@ func checksumFor(checksums []byte, asset string) (string, error) { return "", fmt.Errorf("no checksum entry for %s — refusing to install unverified binary", asset) } -// extractBinary pulls binName out of a tar.gz or zip archive held in memory. +// Extraction policy bounds (audit T48): the compressed download is capped +// by maxUpdateBytes, but decompressed members were previously read with a +// bare io.ReadAll — a small, highly compressible archive member could +// expand until memory exhaustion before any checksum ran. +const ( + maxUpdateBinarySize = 128 << 20 // 128 MB decompressed binary + maxUpdateEntries = 4096 // whole-archive entry count +) + +// extractBinary pulls binName out of a tar.gz or zip archive held in +// memory, under a bounded, single-binary policy (audit T48, narrowing the +// A47 deferral): the member's DECLARED size is checked before any byte is +// read, the read itself is bounded, non-regular entries are refused, a +// duplicate matching name is refused (exactly one binary or the archive is +// not what we published), and the total entry count is capped so a +// millions-of-empty-entries archive cannot stall the walk. func extractBinary(archive []byte, ext, binName string) ([]byte, error) { if ext == "zip" { zr, err := zip.NewReader(bytes.NewReader(archive), int64(len(archive))) if err != nil { return nil, err } + if len(zr.File) > maxUpdateEntries { + return nil, fmt.Errorf("archive has %d entries (limit %d)", len(zr.File), maxUpdateEntries) + } + var found bool + var binary []byte for _, f := range zr.File { - if path.Base(f.Name) == binName { - rc, err := f.Open() - if err != nil { - return nil, err - } - defer rc.Close() - return io.ReadAll(rc) + if path.Base(f.Name) != binName { + continue + } + if found { + return nil, fmt.Errorf("archive contains %s more than once — refusing to pick arbitrarily", binName) + } + if f.FileInfo().IsDir() { + return nil, fmt.Errorf("%s in the archive is a directory", binName) } + size := f.UncompressedSize64 + if size == 0 || size > maxUpdateBinarySize { + return nil, fmt.Errorf("%s declares %d decompressed bytes (limit %d)", binName, size, maxUpdateBinarySize) + } + rc, err := f.Open() + if err != nil { + return nil, err + } + binary, err = io.ReadAll(io.LimitReader(rc, maxUpdateBinarySize+1)) + rc.Close() + if err != nil { + return nil, err + } + if int64(len(binary)) > maxUpdateBinarySize { + return nil, fmt.Errorf("%s expands beyond the %d MB extraction limit", binName, maxUpdateBinarySize>>20) + } + found = true } - return nil, fmt.Errorf("%s not found in archive", binName) + if !found { + return nil, fmt.Errorf("%s not found in archive", binName) + } + return binary, nil } gz, err := gzip.NewReader(bytes.NewReader(archive)) @@ -260,6 +304,9 @@ func extractBinary(archive []byte, ext, binName string) ([]byte, error) { } defer gz.Close() tr := tar.NewReader(gz) + var found bool + var binary []byte + entries := 0 for { hdr, err := tr.Next() if err == io.EOF { @@ -268,11 +315,35 @@ func extractBinary(archive []byte, ext, binName string) ([]byte, error) { if err != nil { return nil, err } - if path.Base(hdr.Name) == binName { - return io.ReadAll(tr) + entries++ + if entries > maxUpdateEntries { + return nil, fmt.Errorf("archive exceeds %d entries", maxUpdateEntries) + } + if path.Base(hdr.Name) != binName { + continue + } + if found { + return nil, fmt.Errorf("archive contains %s more than once — refusing to pick arbitrarily", binName) } + if hdr.Typeflag != tar.TypeReg { + return nil, fmt.Errorf("%s in the archive is not a regular file", binName) + } + if hdr.Size <= 0 || hdr.Size > maxUpdateBinarySize { + return nil, fmt.Errorf("%s declares %d decompressed bytes (limit %d)", binName, hdr.Size, maxUpdateBinarySize) + } + binary, err = io.ReadAll(io.LimitReader(tr, maxUpdateBinarySize+1)) + if err != nil { + return nil, err + } + if int64(len(binary)) > maxUpdateBinarySize { + return nil, fmt.Errorf("%s expands beyond the %d MB extraction limit", binName, maxUpdateBinarySize>>20) + } + found = true + } + if !found { + return nil, fmt.Errorf("%s not found in archive", binName) } - return nil, fmt.Errorf("%s not found in archive", binName) + return binary, nil } // replaceBinary installs the verified update atomically: a sibling diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go new file mode 100644 index 0000000..cdbdaa1 --- /dev/null +++ b/internal/cli/update_test.go @@ -0,0 +1,68 @@ +package cli + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "testing" +) + + +// TestExtractBinary_BoundedAndSingleRegular is the T48 regression: archive +// members are read under a decompressed-size bound, non-regular and +// duplicate matches are refused, and the entry count is capped — the old +// bare io.ReadAll let a small compressed member expand without limit. +func TestExtractBinary_BoundedAndSingleRegular(t *testing.T) { + // A legitimate archive with the binary plus metadata extracts fine. + var tarBuf bytes.Buffer + gz := gzip.NewWriter(&tarBuf) + tw := tar.NewWriter(gz) + payload := []byte("binary-bytes") + tw.WriteHeader(&tar.Header{Name: "teploy", Mode: 0o755, Size: int64(len(payload)), Typeflag: tar.TypeReg}) + tw.Write(payload) + tw.WriteHeader(&tar.Header{Name: "checksums.txt", Mode: 0o644, Size: 4, Typeflag: tar.TypeReg}) + tw.Write([]byte("abcd")) + tw.Close() + gz.Close() + got, err := extractBinary(tarBuf.Bytes(), "tar.gz", "teploy") + if err != nil || string(got) != "binary-bytes" { + t.Fatalf("extractBinary: (%q, %v)", got, err) + } + + // A member whose declared size exceeds the bound is refused WITHOUT + // reading it. + var big bytes.Buffer + gz2 := gzip.NewWriter(&big) + tw2 := tar.NewWriter(gz2) + tw2.WriteHeader(&tar.Header{Name: "teploy", Size: maxUpdateBinarySize + 1, Typeflag: tar.TypeReg}) + tw2.Close() + gz2.Close() + if _, err := extractBinary(big.Bytes(), "tar.gz", "teploy"); err == nil { + t.Error("oversized declared member accepted") + } + + // Duplicate names are refused. + var dup bytes.Buffer + gz3 := gzip.NewWriter(&dup) + tw3 := tar.NewWriter(gz3) + for i := 0; i < 2; i++ { + tw3.WriteHeader(&tar.Header{Name: "teploy", Size: 1, Typeflag: tar.TypeReg}) + tw3.Write([]byte("x")) + } + tw3.Close() + gz3.Close() + if _, err := extractBinary(dup.Bytes(), "tar.gz", "teploy"); err == nil { + t.Error("duplicate binary accepted") + } + + // A directory masquerading as the binary is refused. + var dir bytes.Buffer + gz4 := gzip.NewWriter(&dir) + tw4 := tar.NewWriter(gz4) + tw4.WriteHeader(&tar.Header{Name: "teploy", Typeflag: tar.TypeDir}) + tw4.Close() + gz4.Close() + if _, err := extractBinary(dir.Bytes(), "tar.gz", "teploy"); err == nil { + t.Error("directory entry accepted as the binary") + } +} diff --git a/internal/deploy/health.go b/internal/deploy/health.go index 010398f..d97e5f8 100644 --- a/internal/deploy/health.go +++ b/internal/deploy/health.go @@ -113,14 +113,16 @@ func (d *Deployer) HealthCheckAt(ctx context.Context, port int, containerName st // parsing, and a bare IPv6 bind produced a malformed URL. --globoff keeps // curl from treating {} and [] in the path as its own glob syntax, and the // per-attempt connect/max deadlines bound each probe below the overall -// readiness timeout. +// readiness timeout. --noproxy '*' (audit T20's contained half) makes the +// host-local probe ignore ambient proxy configuration — an inherited +// HTTP_PROXY made the probe ask a proxy about a loopback address. func (d *Deployer) checkHealth(ctx context.Context, host string, port int, path string) bool { url, ok := probeURL(host, port, path) if !ok { return false } cmd := fmt.Sprintf( - "curl -s -o /dev/null --globoff --connect-timeout 2 --max-time 5 -w '%%{http_code}' --url %s", + "curl -s -o /dev/null --noproxy '*' --globoff --connect-timeout 2 --max-time 5 -w '%%{http_code}' --url %s", ssh.ShellQuote(url), ) output, err := d.exec.Run(ctx, cmd) From a7eb7a0cc5554a58a1716999037ddb1b23890fa1 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:07:00 -0700 Subject: [PATCH 13/13] =?UTF-8?q?docs(audit):=20round=204=20(T01-T63)=20re?= =?UTF-8?q?corded=20=E2=80=94=2035=20contained=20fixes,=2028=20standing=20?= =?UTF-8?q?deferrals=20with=20evidence=20folded=20in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- AUDIT_OPEN.md | 148 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 144 insertions(+), 4 deletions(-) diff --git a/AUDIT_OPEN.md b/AUDIT_OPEN.md index 495c229..a99ccd2 100644 --- a/AUDIT_OPEN.md +++ b/AUDIT_OPEN.md @@ -10,7 +10,10 @@ Round 2 (2026-09-17, 60 findings TCL-01..TCL-60, pinned at 1a8ea32) is recorded at the bottom: 24 findings closed with contained fixes (several narrowing pass-6 deferrals), the rest deferred — almost all of them the same architectural tail pass 6 already carries, now -with the round-2 evidence folded in. +with the round-2 evidence folded in. Round 4 (2026-09-19, 63 findings +T01-T63, pinned at c30e4b3, record at the bottom) closed 35 findings with +contained fixes; its residual tail is the standing architectural items with +round-4 evidence folded in, plus the new T28/T40/T59 deferrals. Open items: the pass-6 deferred tail minus the F14 family (resolved 2026-09-18) and the F08/F16/F48/F49/F57 family (resolved 2026-09-18, @@ -20,9 +23,10 @@ TCL-51), the round-2 residual tail itemized in that section, and the dependent designs F04 (and its TCL-10/TCL-15 dependents) that stay deferred with their annotations — plus the round-3 deferrals recorded at the bottom (mostly the same architectural tail, with round-3 evidence -folded in). The 2 upstream/owner items are closed (below). The two -upstream items received from teploy-dash's 2026-09-17 pass are closed -below. +folded in) and the round-4 deferrals recorded at the bottom (standing tail ++ round-4 evidence, plus the new T28/T40/T59 items). The 2 upstream/owner +items are closed (below). The two upstream items received from +teploy-dash's 2026-09-17 pass are closed below. ## Resolved from this register @@ -629,3 +633,139 @@ all packages ok. No push performed. - A50 — F65 real-filesystem/Docker integration matrix (this round's new tests remain mock-level; PrefixWriter/cancellation tests are behavioral with real processes). + +## Round 4 (2026-09-19, T01-T63, pinned at c30e4b3) — record + +Report reviewed finding-by-finding against the source at the pinned HEAD +(the register's closure claims were NOT taken as proof — several findings +landed as genuine defects on top of the newest surfaces, and several +restated the standing architectural tail). 35 findings closed with +contained fixes across 12 commits; the remaining 28 defer onto the standing +pass-6/round-2/round-3 tail (round-4 evidence folded in) or onto the new +items noted below. No false positives found; T38 was confirmed against the +post-A41 script (the pre-stop existence FLAG was the residual defect, not +the copy ordering); T07 confirmed A13's flag accounting never set restored +on success. + +### Round 4 — fixed (contained) + +| ID | Sev | Where | +|---|-----|-------| +| T02 | High | 41192f4 — the ambiguous-failure fallback of ReleaseLockFenced is a shell-level CONDITIONAL: the lock is removed only when its info still names the releasing owner (or is already gone); a successor's lock is never deleted by the old unconditional detached release | +| T06 | High | 34d6dc9 — every rollback route-phase failure (upstream-port inspection, SetRoute, SetLoadBalancerHealth) unwinds through the same cleanup as start/health failures: stop the uncommitted target, restore the displaced fixed-port workload | +| T07 | Med | 34d6dc9 — restoreDisplacedAndStarted sets restored on SUCCESSFUL restarts (an all-restored recovery no longer reports "no container is serving"); partial cleanup failures are joined into the returned error | +| T08 | High | c9260a7 — pin/unpin run under the app's fenced lock (the same one deploys/prunes hold) with release-id grammar validation at the command boundary | +| T10 | Med | 67c097b — the asset-bridge seed selector is mtime-ordered and skips attempts with no assets directory (the lexicographically-greatest pick could select an env-only attempt and silently seed nothing) | +| T11 | Med | 67c097b — asset_keep_days cleanup runs on the LIVE attempt-scoped tree (it had been a no-op since F08) and PruneAttempts bounds attempts per retained hash to the two newest | +| T12 | High | a83ff14 — InspectRecreate captures docker's EFFECTIVE top-level mounts: anonymous volumes (Dockerfile VOLUME) are preserved BY NAME, and an effective mount the CLI cannot represent fails the inspect instead of silently dropping storage; --mount values are CSV-encoded | +| T13 | Med | a83ff14 — the recreation renderer brackets IPv6 binds via net.JoinHostPort with validated ports/protocol ('::1:49152:80' concatenation is gone) | +| T15 | High | 1b267ec — ListContainers requests Labels as a JSON object via a custom --format; the comma-joined display string (whose values could forge reserved teploy.* labels) is no longer parsed for lifecycle decisions | +| T17 | Med | 1b267ec (half) — ImageExists distinguishes a proven "no such image" from daemon/permission failures (the old framing turned a broken daemon into a convincing cache miss). The deploy-path resolve-warns-and-fall-back stays deliberate (A52) | +| T19 | High | a83ff14 (recreation half, the TCL-12 registered follow-up) — resolved env rides a private 0600 on-target --env-file instead of -e argv; the docker-exec AWS/MySQL channels stay deferred (A33) | +| T20 | Med | 555581b (half) — health probes pass curl --noproxy '*' so ambient proxy configuration cannot hijack host-local readiness. The explicit HTTP/TCP mode redesign stays deferred (A22) | +| T21 | Med | 34d6dc9 — worker verification treats a persistently unreadable inspect as a deploy FAILURE after bounded retries (reversing A23's degrade-to-warning: unknown is not readiness) | +| T23 | Med | 9694108 — publish entries are parsed against a documented narrow grammar ([ip:]host:container[/proto], single ports, bracketed IPv6) at BOTH boundaries, duplicate host bindings (incl. wildcard-vs-specific) are rejected pre-mutation, and host-ingress conflicts with the fixed port fail at validation | +| T26 | High | 82d0ed3 — the webhook route is PERSISTED in the Caddyfile inside the app's managed site block from a per-app descriptor; every managed render (deploy/rollback/maintenance) re-applies it under the same lock + adapt gate + reload/verify transaction — the runtime admin-API injection could be erased by the very deploy it triggered | +| T27 | Med | 82d0ed3 — the route honors the configured listener port (9876 was hardcoded) and matches every configured domain (the comma list was one JSON host value) | +| T29 | High | 82d0ed3 — autodeploy Schedule/Unschedule read the crontab status-checked (only the canonical no-crontab message starts from empty) and the crontab -r fallback is gone | +| T30 | Med | 82d0ed3 — autodeploy Remove aggregates every step failure into an "incomplete" error; Status reports transport failures as errors, never as "inactive" | +| T31 | Med | 82d0ed3 — the resident path resolves relative TLS cert/key paths against the fetched checkout (the systemd unit has no WorkingDirectory) | +| T32 | Low | 82d0ed3 — the webhook secret is stored and HMAC-verified verbatim: setup rejects whitespace-wrapped secrets, serve refuses (with the reason) instead of trimming the key | +| T37 | High | 353dc93 — restore_original is defined and the baseline capture compensated AFTER the stop: a failed docker cp under set -e used to exit with Redis stopped and no restart attempted (behavioral tests drive the script under a real bash with a stub docker) | +| T38 | High | 353dc93 — the baseline is captured against the STOPPED container (docker cp), distinguishing "no such file" from every other failure — the old pre-stop existence flag missed the final RDB a graceful shutdown writes when none existed | +| T41 | High | a602edc — secret List runs a bare status-checked find and sorts in Go (the old find|sort pipeline without pipefail reported a failed listing as "no secrets"); listed names are grammar-validated | +| T45 | High | a602edc — every atomic publication renames with mv -fT (remote Upload, UploadAtomic, secret Set): a plain mv into a destination symlinked to a directory silently nested the file and left the destination unchanged | +| T46 | Med | a602edc (local half) — LocalExecutor.Upload fsyncs the containing directory after the rename. The remote-shell durability contract (fsync + parent sync over SSH) stays deferred | +| T48 | High | 555581b (narrowing A47) — update extraction is bounded and single-binary: declared sizes checked before reading, limited reads, non-regular/duplicate entries refused, entry count capped | +| T49 | Med | 555581b (half) — the updater derives its context from the Cobra command. The selection policy (downgrade/prerelease ordering, --allow-downgrade) stays deferred (A48) | +| T51 | High | 353dc93 — .teployignore EXTENDS the always-protected defaults (.env/.env.*/.git/node_modules); an unreadable ignore file is an error, never a silent defaults-only transfer | +| T53 | Med | 9694108 — basic_auth requires a COMPLETE structural bcrypt hash, forward_auth's verify URI must be request-path-shaped, copy_headers must be HTTP tokens, and the upstream URL rejects control characters (closes the TCL-39 renderer-input follow-up) | +| T56 | Med | 67c097b — releasemeta.Read validates the record's embedded App/Hash against the requested key; a copied or corrupted-but-valid record can no longer drive effects at a different release's spec | +| T57 | High | 5401f87 — a failed load-balancer update after a fully successful fleet wave is a nonzero exit ("backends deployed but load-balancer activation failed") | +| T58 | High | 5401f87 (half) — both fleet rollback waves run on bounded detached recovery contexts (a Ctrl-C no longer cancels the recovery itself into a no-op). The generation-identity half (compensating only the recorded predecessor) defers with the T04 family | +| T61 | Low | 555581b — CI and the release workflow are read-only by default; contents:write is granted only to the publishing job | +| T62 | High | 5401f87 (half) — maintenance on/off takes the app's fenced deploy lock, the --app path verifies the authoritative server ingress mode, and the stash is read/created/deleted inside the Caddyfile mutation transaction. The versioned-desired-state redesign stays deferred | +| T63 | Med | 34d6dc9 — the name-derived cleanup fallback retries the container inventory first (removed workers are invisible to name-derived retirement) and reports every fallback stop/remove failure | + +### Round 4 — deferred (standing tail, with round-4 evidence folded in) + +- T01 — A05's remainder (the grep-based guard is check-then-act at the + multi-command phase granularity; the fenced single-command guards refuse + stale effects but two contenders can still both read a stale owner). The + permanent server-side serialization transaction is the redesign A05 + defers; T02's conditional fallback removed the worst unguarded deletion. +- T03 — the shared Caddy lock stays short-lived, ownerless, and unfenced BY + DESIGN (the register's TCL-05 note); every Caddyfile edit now runs under + the adapt gate + delivery verification, and the app-level fence covers + deploy effects. The full target-side lock redesign folds into A05. +- T04 — A07/F04: operation-scoped receipts (attempt labels, container-ID + receipts, generation comparison before compensation). T58's detached + contexts and T06/T62's transactional cleanups cover the contained halves. +- T05 — A12's remainder: restorePreviousRoute still reconstructs the + predecessor block from cfg + live inspect (now via A21's ambiguity-refusing + port reads); the exact-block receipt/compare-and-swap restore design + remains open on ParseSites/ExtractPolicy. +- T09 — A09: same-version redeploys rewrite the (app, hash) record (the + documented immutability exception) and the record is written after the + live commit (the deliberate degradation posture); attempt-keyed + generation records remain the F04-adjacent design. +- T14 — F20's remainder: candidate-before-destructive recreate and the + fields the docker CLI cannot round-trip (health checks beyond NONE, + restart retries, DNS/devices/ulimits). T12/T13/T19 removed the silent + DATA-loss halves (anonymous volumes, IPv6, env argv). +- T16 — A20/TCL-15: host-port preselection is not a reservation (ss-based + allocation + Docker as final authority). +- T18 — A24/F17: Cmd stays a deliberate operator-authored shell string at + the docker-run sink. +- T22 — A16: stable aliases expose external-ingress candidates before + readiness (generation-scoped aliases need F04's handoff). +- T24 — A34: durable webhook job queue (ack-before-durable-job remains; + A36's content dedup + T26's persisted routing cover the routing halves). +- T25 — A35: webhook builds fetch the watched branch HEAD, not the + authenticated payload commit (fetch + worktree pinning design). +- T28 — NEW deferral: the scheduled-redeploy cron script is a separate + forked deployment engine (no lock, health gate, route/state/metadata + commit). Unifying it behind the real deploy engine is the fix; whether + to fail closed on `autodeploy schedule` until then is an owner product + decision (it disables a shipped feature). T29's strict crontab handling + removed the destructive halves around it. +- T33 — A37: listener scope, bounded admission, graceful shutdown with + recoverable jobs (the durable queue is the prerequisite). +- T34 — A38/TCL-40: restore under the app lease + writer quiescence. +- T35 — A42: per-engine transactional consistency (SQL/Mongo staging + + controlled cutover). +- T36 — A43/TCL-44: constrained extractor for restore archives (the host + tar runs in a private staging tree today). +- T39 — TCL-42's open half: same-second LASTSAVE ambiguity and + persistence-path discovery (dir/dbfilename assumptions). +- T40 — NEW deferral: a versioned whole-app disaster-recovery bundle + (release records, secret stores + age identity, TLS references) is a + product decision; today's archives are data-only by design. +- T42 — A30: errno-aware confirmed-missing reads (test -e folds EACCES + into absence); needs the structured executor result. +- T43 — A30: local/remote executor output semantics (stdout/stderr split, + no trimming) — the cross-cutting CommandResult contract. +- T44 — A29: session-open cancellation needs a per-command transport. +- T47 — A31: cross-process TOFU serialization of first-use host-key + acceptance. +- T50 — A49 + the nixpacks curl|bash installer: reviewed pins/digests are + owner items the register does not invent; the installer now joins them. +- T52 — TCL-54: nixpacks --platform parity and DetectAt's stat + distinction. +- T54 — F57's owner decision: presence-aware overlay semantics beyond the + strict-env opt-in. +- T55 — TCL-50/F60: the redacted manifest digest is not a complete-plan + identity. +- T59 — NEW deferral: static publication hashes the mutable source before + transfer and trusts an existing short-hash directory (snapshot + + content-manifest verification design; concurrent source mutation is the + precondition). +- T60 — A50: the fencing mock models a stronger atomicity guarantee than + the real shell (this round's behavioral tests — the redis script under a + real bash — are the pattern the integration matrix wants more of). + +Gates at the closing commits: `go vet ./...` clean; `go test ./... -race` +all packages ok. No push performed. (Environmental note: Apple's Xcode 27 +update landed mid-session and required license re-acceptance for +/usr/bin/git; the closing gates ran against the standalone Command Line +Tools git on PATH.)