From 3e9ddd604c4ef4e3d8330aac61fdfd5b3fbcdbd3 Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 02:40:33 -0700 Subject: [PATCH 1/4] fix(api): reject unknown tier in metrics prune instead of pruning raw --- docs/architecture/metrics.md | 2 +- internal/api/system.go | 38 +++++++++++++++++++------ internal/api/system_test.go | 55 ++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 9 deletions(-) diff --git a/docs/architecture/metrics.md b/docs/architecture/metrics.md index dbf91280..3ea72a67 100644 --- a/docs/architecture/metrics.md +++ b/docs/architecture/metrics.md @@ -45,4 +45,4 @@ A separate code path in the proxy package (the `simpledeploy_metrics` Caddy modu ## Pruning -Pruning runs nightly. Manual prune is exposed at `POST /api/system/prune/{metrics,request-stats}` for emergencies. +Pruning runs nightly. Manual prune is exposed at `POST /api/system/prune/{metrics,request-stats}` for emergencies (super admin). The body is `{"tier": "raw", "days": 30}`: `tier` is one of `raw`, `1m`, `5m`, `1h`, `1d` (default `raw`) and `days` defaults to 30. An unknown tier or an unreadable body returns `400` and deletes nothing. diff --git a/internal/api/system.go b/internal/api/system.go index e2415b4b..7293980f 100644 --- a/internal/api/system.go +++ b/internal/api/system.go @@ -4,13 +4,16 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" + "io" "net" "net/http" "os" "path/filepath" "regexp" "runtime" + "slices" "strconv" "strings" "syscall" @@ -77,8 +80,12 @@ type appSummary struct { type pruneRequest struct { Days int `json:"days"` - Tier string `json:"tier"` // raw, 1m, 5m, 1h — empty defaults to "raw" + Tier string `json:"tier"` // one of pruneTiers; empty defaults to "raw" } + +// pruneTiers are the tiers a manual prune accepts. +var pruneTiers = []string{"raw", "1m", "5m", "1h", "1d"} + type pruneResponse struct { Deleted int64 `json:"deleted"` Message string `json:"message"` @@ -208,21 +215,32 @@ func (s *Server) handleStorageBreakdown(w http.ResponseWriter, r *http.Request) json.NewEncoder(w).Encode(storageBreakdownResponse{Metrics: metrics, RequestStats: rs}) } -func parsePruneRequest(r *http.Request) pruneRequest { +// parsePruneRequest reads a prune body. An empty body or tier means raw +// and 30 days. An unknown tier is an error, so a typo never prunes another +// tier. +func parsePruneRequest(r *http.Request) (pruneRequest, error) { var req pruneRequest - json.NewDecoder(r.Body).Decode(&req) + if err := json.NewDecoder(r.Body).Decode(&req); err != nil && !errors.Is(err, io.EOF) { + return req, errors.New("invalid request body") + } if req.Days <= 0 { req.Days = 30 } - validTiers := map[string]bool{"raw": true, "1m": true, "5m": true, "1h": true, "1d": true} - if !validTiers[req.Tier] { + if req.Tier == "" { req.Tier = "raw" } - return req + if !slices.Contains(pruneTiers, req.Tier) { + return req, fmt.Errorf("unknown tier %q: use one of %s", req.Tier, strings.Join(pruneTiers, ", ")) + } + return req, nil } func (s *Server) handlePruneMetrics(w http.ResponseWriter, r *http.Request) { - req := parsePruneRequest(r) + req, err := parsePruneRequest(r) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } cutoff := time.Now().AddDate(0, 0, -req.Days) n, err := s.store.PruneMetrics(req.Tier, cutoff) if err != nil { @@ -234,7 +252,11 @@ func (s *Server) handlePruneMetrics(w http.ResponseWriter, r *http.Request) { } func (s *Server) handlePruneRequestMetrics(w http.ResponseWriter, r *http.Request) { - req := parsePruneRequest(r) + req, err := parsePruneRequest(r) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } cutoff := time.Now().AddDate(0, 0, -req.Days) n, err := s.store.PruneRequestMetrics(req.Tier, cutoff) if err != nil { diff --git a/internal/api/system_test.go b/internal/api/system_test.go index 041ee450..7d060f07 100644 --- a/internal/api/system_test.go +++ b/internal/api/system_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -36,3 +37,57 @@ func TestSystemInfoDeploymentFields(t *testing.T) { t.Errorf("deployment_mode = %q, want %q", sd.DeploymentMode, "native") } } + +func TestPruneRejectsUnknownTier(t *testing.T) { + srv, _ := newTestServer(t) + cookie := superAdminCookie(t, srv.jwt) + for _, path := range []string{"/api/system/prune/metrics", "/api/system/prune/request-stats"} { + rr := httptest.NewRecorder() + srv.Handler().ServeHTTP(rr, authedRequest(t, http.MethodPost, path, map[string]any{"tier": "1hr", "days": 1}, cookie)) + if rr.Code != http.StatusBadRequest { + t.Fatalf("%s: status = %d, want 400; body: %s", path, rr.Code, rr.Body.String()) + } + if body := rr.Body.String(); !strings.Contains(body, `"1hr"`) || !strings.Contains(body, "raw, 1m, 5m, 1h, 1d") { + t.Errorf("%s: body = %q, want the bad tier and the valid ones", path, body) + } + } +} + +func TestPruneTierDefaultsAndValid(t *testing.T) { + srv, _ := newTestServer(t) + cookie := superAdminCookie(t, srv.jwt) + cases := []struct { + body any + tier string + }{ + {nil, "raw"}, + {map[string]any{"days": 1}, "raw"}, + {map[string]any{"tier": "1h", "days": 1}, "1h"}, + } + for _, tc := range cases { + rr := httptest.NewRecorder() + srv.Handler().ServeHTTP(rr, authedRequest(t, http.MethodPost, "/api/system/prune/metrics", tc.body, cookie)) + if rr.Code != http.StatusOK { + t.Fatalf("body %v: status = %d, want 200; body: %s", tc.body, rr.Code, rr.Body.String()) + } + var resp pruneResponse + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatal(err) + } + if !strings.Contains(resp.Message, "["+tc.tier+"]") { + t.Errorf("body %v: message = %q, want tier %s", tc.body, resp.Message, tc.tier) + } + } +} + +func TestPruneRejectsInvalidJSON(t *testing.T) { + srv, _ := newTestServer(t) + cookie := superAdminCookie(t, srv.jwt) + req := httptest.NewRequest(http.MethodPost, "/api/system/prune/metrics", strings.NewReader(`{"tier": "raw",`)) + req.AddCookie(cookie) + rr := httptest.NewRecorder() + srv.Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", rr.Code, rr.Body.String()) + } +} From 31f4cd0326f78b729a19892537dcd110a96efe91 Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 02:43:41 -0700 Subject: [PATCH 2/4] fix: show why docker compose failed in deploy, rollback and pull errors --- docs/architecture/deployer.md | 6 ++ docs/operations/security-audit.md | 2 +- docs/operations/troubleshooting.md | 14 ++++ internal/audit/render/deploy.go | 3 + internal/audit/render/render_test.go | 9 +++ internal/deployer/composeerr.go | 111 ++++++++++++++++++++++++++ internal/deployer/composeerr_test.go | 114 +++++++++++++++++++++++++++ internal/deployer/deployer.go | 16 ++-- 8 files changed, 266 insertions(+), 9 deletions(-) create mode 100644 internal/deployer/composeerr.go create mode 100644 internal/deployer/composeerr_test.go diff --git a/docs/architecture/deployer.md b/docs/architecture/deployer.md index ee2341e4..eec1c233 100644 --- a/docs/architecture/deployer.md +++ b/docs/architecture/deployer.md @@ -23,6 +23,12 @@ Each app's compose project is named `simpledeploy-`. This isolates app The whole sequence is wrapped in a tracker that the API surfaces as live deploy logs over WebSocket and as a "deploying" flag on the app. +## Failure messages + +When a tracked `docker compose` command (deploy, rollback, restart, pull) exits non-zero, `Err` is a `*ComposeError` whose `Detail` says why: the last few stderr lines (at most 3, 1000 bytes, keeping the end) that are not progress (`Container x Started`, `web Pulling`, layer downloads) or warnings, with repeats dropped (a failed pull prints the same error twice). Compose prints its error last, so this is normally the single line that names the problem. `Error()` reads `compose up: : exit status 1`; that string is what the `deploy_failed` / `rollback` audit row, the activity feed, and the logs show. The full output stays in the deploy log and the `deploy_events` row. + +Registry credentials reach docker only through the temporary config file (never args or stdin), so compose output does not contain them. As defense in depth, each registry password and its base64 `user:password` form are replaced with `[redacted]` in `Detail` and in the stored output. Detail and output are visible to the same users who can open the app's deploy log. + ## Compose check before `up` `Deploy`, `RollbackDeploy`, `Pull`, `Scale` and `Cancel` (which runs a follow-up `up`) all call `checkComposeFile` before running `docker compose`. It re-reads `docker-compose.yml` from disk with `compose.ParseFile`, which interpolates `${VARS}` from the app's `.env` and the server environment the way `docker compose` does and refuses a symlinked compose file or `.env`, then runs `compose.ValidateComposeSecurity`. A file changed after the caller parsed it (git sync, version restore, manual edit) is therefore still checked right before it runs. `Restart` (`compose restart`) and `Stop`/`Start` do not create containers and skip the check. diff --git a/docs/operations/security-audit.md b/docs/operations/security-audit.md index e86efa98..06557474 100644 --- a/docs/operations/security-audit.md +++ b/docs/operations/security-audit.md @@ -24,7 +24,7 @@ The activity log is a persistent, queryable record of everything that changes in | `webhook` | Webhooks created/changed/removed | | `registry` | Registry credentials added/removed | | `access` | User app-access grants and revocations | -| `deploy` | Deploy succeeded, deploy failed (with error), rollback | +| `deploy` | Deploy succeeded, deploy failed (with the reason docker compose gave), rollback (or why it failed) | | `lifecycle` | App created, stopped, started, restarted, scaled, removed | | `auth` | Login success/failure, password change (global only) | | `system` | User CRUD, API key CRUD, public-host change, git sync config, retention settings (global only) | diff --git a/docs/operations/troubleshooting.md b/docs/operations/troubleshooting.md index aede000e..aa272725 100644 --- a/docs/operations/troubleshooting.md +++ b/docs/operations/troubleshooting.md @@ -25,6 +25,20 @@ simpledeploy registry list - Network issue: confirm DNS works (`getent hosts registry-1.docker.io`) and outbound 443 is open. - Rate limited by Docker Hub: log in to a paid account or use a registry mirror. +## Deploy failed + +**Symptom:** The app shows `error` and the activity feed says `Deploy failed: compose up: : exit status 1` (or `Rollback to version N failed: ...`). + +**What it means:** `docker compose up` itself stopped with an error. `` is the last lines docker compose printed, so it usually names the problem. The full output is in the deploy log and the app's deploy history. + +**Common reasons:** +- `pull access denied` or `unauthorized`: the image name or tag is wrong, or the image is private. Fix the `image:` line or add the registry under **Settings -> Registries**. +- `port is already allocated`: another app or program uses that host port. Pick a different host port in `ports:`. +- `manifest unknown` or `not found`: the tag does not exist. +- `required variable X is missing a value`: add `X` in the **Config** tab. + +Fix the cause and redeploy. + ## Deploy reported "Unstable" **Symptom:** The deploy wizard shows a yellow "Unstable" status. The per-service summary lists one or more services as `restarting`, `unhealthy`, or with `restarted Nx`. diff --git a/internal/audit/render/deploy.go b/internal/audit/render/deploy.go index d77c0839..0f68f7ba 100644 --- a/internal/audit/render/deploy.go +++ b/internal/audit/render/deploy.go @@ -50,5 +50,8 @@ func renderDeployFailed(before, after []byte) (string, string) { func renderDeployRollback(before, after []byte) (string, string) { var a deployView _ = json.Unmarshal(after, &a) + if a.Error != "" { + return fmt.Sprintf("Rollback to version %d failed: %s", a.Version, a.Error), "" + } return fmt.Sprintf("Rolled back to version %d", a.Version), "" } diff --git a/internal/audit/render/render_test.go b/internal/audit/render/render_test.go index 739b74b1..ea408357 100644 --- a/internal/audit/render/render_test.go +++ b/internal/audit/render/render_test.go @@ -392,6 +392,15 @@ func TestDeployRollback(t *testing.T) { } } +// A failed rollback must say so and why, not "Rolled back". +func TestDeployRollbackFailed(t *testing.T) { + s, _ := Render("deploy", "rollback", nil, []byte(`{"version":5,"error":"compose up: port is already allocated: exit status 1"}`)) + want := "Rollback to version 5 failed: compose up: port is already allocated: exit status 1" + if s != want { + t.Errorf("summary = %q, want %q", s, want) + } +} + // --- lifecycle --- func TestLifecycleCreated(t *testing.T) { diff --git a/internal/deployer/composeerr.go b/internal/deployer/composeerr.go new file mode 100644 index 00000000..a2c4f0a5 --- /dev/null +++ b/internal/deployer/composeerr.go @@ -0,0 +1,111 @@ +package deployer + +import ( + "encoding/base64" + "regexp" + "slices" + "strings" + "unicode/utf8" +) + +const ( + maxDetailLines = 3 + maxDetailBytes = 1000 +) + +// ComposeError is a failed docker compose command. Detail is the end of its +// error output, which says why it failed (image pull denied, port in use, +// ...), with registry credentials removed. It is what the deploy failure +// shows in the activity feed and the logs; the full output stays in the +// deploy log and deploy events. +type ComposeError struct { + Op string // "compose up", "compose pull", ... + Detail string + Err error +} + +func (e *ComposeError) Error() string { + if e.Detail == "" { + return e.Op + ": " + e.Err.Error() + } + return e.Op + ": " + e.Detail + ": " + e.Err.Error() +} + +func (e *ComposeError) Unwrap() error { return e.Err } + +func composeError(op string, err error, stderr string, auths []RegistryAuth) error { + return &ComposeError{Op: op, Detail: failureDetail(redactAuths(stderr, auths)), Err: err} +} + +// redactAuths removes registry credentials from command output. They only +// reach docker through the temporary config file, never args or stdin, so +// this is defense in depth. +func redactAuths(s string, auths []RegistryAuth) string { + for _, a := range auths { + if a.Password == "" { + continue + } + s = strings.ReplaceAll(s, base64.StdEncoding.EncodeToString([]byte(a.Username+":"+a.Password)), "[redacted]") + s = strings.ReplaceAll(s, a.Password, "[redacted]") + } + return s +} + +// noiseLine matches docker compose progress lines ("Container x Started", +// "web Pulling", "a1b2c3 Downloading ...") and warnings: they never say why +// a command failed. +var noiseLine = regexp.MustCompile(`^(?:(?:Container|Network|Volume|Image)\s+\S+\s+\S+(?:\s+[\d.]+s)?$|\S+\s+(?:Pulling|Pulled|Waiting|Downloading|Download complete|Extracting|Verifying Checksum|Pull complete|Already exists|Skipped|Interrupted)\b|WARN\[|.*\blevel=warning\b)`) + +// failureDetail picks the lines of a failed compose command's stderr that +// say why it failed. Compose prints its error last, so these are the last +// few lines that are not progress or warnings, without repeats, capped at +// maxDetailBytes (keeping the end). +func failureDetail(stderr string) string { + lines := strings.Split(stderr, "\n") + var picked []string + for i := len(lines) - 1; i >= 0 && len(picked) < maxDetailLines; i-- { + l := strings.TrimSpace(lines[i]) + if l == "" || noiseLine.MatchString(l) || restates(l, picked) { + continue + } + picked = append(picked, l) + } + slices.Reverse(picked) + detail := strings.Join(picked, "\n") + if len(detail) > maxDetailBytes { + cut := len(detail) - maxDetailBytes + for cut < len(detail) && !utf8.RuneStart(detail[cut]) { + cut++ + } + detail = "..." + detail[cut:] + } + return detail +} + +// restates reports whether line repeats an error already picked: a failed +// pull prints "web Error " and then "Error response from daemon: ". +func restates(line string, picked []string) bool { + msg := errorMessage(line) + for _, p := range picked { + if strings.Contains(errorMessage(p), msg) { + return true + } + } + return false +} + +// errorMessage is line lowercased, with spacing collapsed and a leading +// "Error response from daemon:", "Error:" or " Error" removed. +func errorMessage(line string) string { + f := strings.Fields(strings.ToLower(line)) + s := strings.Join(f, " ") + for _, p := range []string{"error response from daemon: ", "error: "} { + if strings.HasPrefix(s, p) { + return s[len(p):] + } + } + if len(f) > 2 && f[1] == "error" { + return strings.Join(f[2:], " ") + } + return s +} diff --git a/internal/deployer/composeerr_test.go b/internal/deployer/composeerr_test.go new file mode 100644 index 00000000..eb30fcc8 --- /dev/null +++ b/internal/deployer/composeerr_test.go @@ -0,0 +1,114 @@ +package deployer + +import ( + "context" + "encoding/base64" + "errors" + "slices" + "strings" + "testing" +) + +// portInUse is what `docker compose up` prints when a host port is taken. +const portInUse = `time="2026-01-01T00:00:00Z" level=warning msg="/apps/myapp/docker-compose.yml: the attribute ` + "`version`" + ` is obsolete" + Network simpledeploy-myapp_default Creating + Network simpledeploy-myapp_default Created + Container simpledeploy-myapp-web-1 Creating + Container simpledeploy-myapp-web-1 Created + Container simpledeploy-myapp-web-1 Starting +Error response from daemon: driver failed programming external connectivity on endpoint simpledeploy-myapp-web-1: Bind for 127.0.0.1:8080 failed: port is already allocated +` + +// failOn makes runner calls whose args contain sub fail with stderr. +func failOn(sub, stderr string) func(RunCall) (string, string, error, bool) { + return func(c RunCall) (string, string, error, bool) { + if slices.Contains(c.Args, sub) { + return "", stderr, errors.New("exit status 1"), true + } + return "", "", nil, false + } +} + +func TestDeployErrorSaysWhyComposeFailed(t *testing.T) { + audit := &fakeAudit{} + d := &Deployer{runner: &MockRunner{Respond: failOn("up", portInUse)}, audit: audit} + + res := d.Deploy(context.Background(), testApp(t, "myapp")) + if res.Err == nil { + t.Fatal("expected error") + } + msg := res.Err.Error() + if !strings.Contains(msg, "port is already allocated") { + t.Errorf("error = %q, want the compose error line", msg) + } + if strings.Contains(msg, "Starting") || strings.Contains(msg, "level=warning") { + t.Errorf("error = %q, want no progress or warning lines", msg) + } + var ce *ComposeError + if !errors.As(res.Err, &ce) || ce.Op != "compose up" { + t.Errorf("error = %#v, want *ComposeError for compose up", res.Err) + } + evt, ok := audit.last() + if !ok || evt.Action != "deploy_failed" || !strings.Contains(evt.Error, "port is already allocated") { + t.Errorf("audit = %+v, want deploy_failed with the compose error", evt) + } +} + +func TestRollbackAndPullErrorsSayWhy(t *testing.T) { + denied := " web Pulling\n web Error pull access denied for acme/private, repository does not exist or may require 'docker login'\nError response from daemon: pull access denied for acme/private, repository does not exist or may require 'docker login'\n" + + d := &Deployer{runner: &MockRunner{Respond: failOn("up", denied)}} + res := d.RollbackDeploy(context.Background(), testApp(t, "myapp"), 2, nil) + if res.Err == nil || !strings.Contains(res.Err.Error(), "pull access denied for acme/private") { + t.Errorf("rollback error = %v, want the pull error", res.Err) + } + + d = &Deployer{runner: &MockRunner{Respond: failOn("pull", denied)}} + res = d.Pull(context.Background(), testApp(t, "myapp"), nil) + if res.Err == nil || !strings.Contains(res.Err.Error(), "pull access denied for acme/private") { + t.Errorf("pull error = %v, want the pull error", res.Err) + } +} + +func TestDeployErrorRedactsRegistryCredentials(t *testing.T) { + auth := RegistryAuth{URL: "registry.example.com", Username: "bot", Password: "hunter2-s3cret"} + b64 := base64.StdEncoding.EncodeToString([]byte(auth.Username + ":" + auth.Password)) + stderr := "Error response from daemon: login failed for bot:hunter2-s3cret (auth " + b64 + ")\n" + d := &Deployer{runner: &MockRunner{Respond: failOn("up", stderr)}} + + res := d.Deploy(context.Background(), testApp(t, "myapp"), auth) + if res.Err == nil { + t.Fatal("expected error") + } + for _, s := range []string{res.Err.Error(), res.Output} { + if strings.Contains(s, auth.Password) || strings.Contains(s, b64) { + t.Errorf("credentials leaked: %q", s) + } + } + if !strings.Contains(res.Err.Error(), "login failed") { + t.Errorf("error = %q, want the rest of the message kept", res.Err) + } +} + +func TestFailureDetail(t *testing.T) { + long := "Error response from daemon: " + strings.Repeat("x", 3000) + " the end" + cases := []struct { + name, stderr, want string + }{ + {"port in use", portInUse, "Error response from daemon: driver failed programming external connectivity on endpoint simpledeploy-myapp-web-1: Bind for 127.0.0.1:8080 failed: port is already allocated"}, + {"pull error printed twice", " web Pulling\n web Error pull access denied for x\nError response from daemon: pull access denied for x\n", "Error response from daemon: pull access denied for x"}, + {"unhealthy dependency", " Container a-db-1 Waiting\n Container a-db-1 Error\ndependency failed to start: container a-db-1 is unhealthy\n", "dependency failed to start: container a-db-1 is unhealthy"}, + {"last lines kept in order", "one\ntwo\nthree\nfour\n", "two\nthree\nfour"}, + {"only progress", " Container a-web-1 Starting\n", ""}, + {"empty", "", ""}, + } + for _, tc := range cases { + if got := failureDetail(tc.stderr); got != tc.want { + t.Errorf("%s: got %q, want %q", tc.name, got, tc.want) + } + } + got := failureDetail(long) + if len(got) > maxDetailBytes+len("...") || !strings.HasPrefix(got, "...") || !strings.HasSuffix(got, " the end") { + t.Errorf("long: got %d bytes %q..., want capped with the end kept", len(got), got[:20]) + } +} diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go index 9dd5c87a..93675f0d 100644 --- a/internal/deployer/deployer.go +++ b/internal/deployer/deployer.go @@ -130,10 +130,10 @@ func (d *Deployer) Deploy(ctx context.Context, app *compose.AppConfig, auths ... args = append([]string{"--config", tmpDir}, args...) } stdout, stderr, err := d.runCmd(ctx, dl, "docker", args...) - output := strings.TrimSpace(stdout + "\n" + stderr) + output := redactAuths(strings.TrimSpace(stdout+"\n"+stderr), auths) status, services := d.finishDeploy(ctx, dl, app.Name, project, "deploy", err) if err != nil { - return DeployResult{Output: output, Err: fmt.Errorf("compose up: %w", err), Status: status, Services: services} + return DeployResult{Output: output, Err: composeError("compose up", err, stderr, auths), Status: status, Services: services} } return DeployResult{Output: output, Status: status, Services: services} } @@ -188,10 +188,10 @@ func (d *Deployer) RollbackDeploy(ctx context.Context, app *compose.AppConfig, v args = append([]string{"--config", tmpDir}, args...) } stdout, stderr, err := d.runCmd(ctx, dl, "docker", args...) - output := strings.TrimSpace(stdout + "\n" + stderr) + output := redactAuths(strings.TrimSpace(stdout+"\n"+stderr), auths) status, services := d.finishDeploy(ctx, dl, app.Name, project, "deploy", err) if err != nil { - return DeployResult{Output: output, Err: fmt.Errorf("compose up: %w", err), Status: status, Services: services} + return DeployResult{Output: output, Err: composeError("compose up", err, stderr, auths), Status: status, Services: services} } return DeployResult{Output: output, Status: status, Services: services} } @@ -234,7 +234,7 @@ func (d *Deployer) Restart(ctx context.Context, app *compose.AppConfig) DeployRe output := strings.TrimSpace(stdout + "\n" + stderr) status, services := d.finishDeploy(ctx, dl, app.Name, project, "restart", err) if err != nil { - return DeployResult{Output: output, Err: fmt.Errorf("compose restart: %w", err), Status: status, Services: services} + return DeployResult{Output: output, Err: composeError("compose restart", err, stderr, nil), Status: status, Services: services} } return DeployResult{Output: output, Status: status, Services: services} } @@ -286,10 +286,10 @@ func (d *Deployer) Pull(ctx context.Context, app *compose.AppConfig, auths []Reg } stdout, stderr, err := d.runCmd(ctx, dl, "docker", pullArgs...) - output := strings.TrimSpace(stdout + "\n" + stderr) + output := redactAuths(strings.TrimSpace(stdout+"\n"+stderr), auths) if err != nil { d.Tracker.DoneWithLogStatus(app.Name, "pull_failed", "failed", nil) - return DeployResult{Output: output, Err: fmt.Errorf("compose pull: %w", err), Status: "failed"} + return DeployResult{Output: output, Err: composeError("compose pull", err, stderr, auths), Status: "failed"} } upArgs := []string{ @@ -304,7 +304,7 @@ func (d *Deployer) Pull(ctx context.Context, app *compose.AppConfig, auths []Reg output = strings.TrimSpace(output) status, services := d.finishDeploy(ctx, dl, app.Name, project, "pull", err) if err != nil { - return DeployResult{Output: output, Err: fmt.Errorf("compose up after pull: %w", err), Status: status, Services: services} + return DeployResult{Output: output, Err: composeError("compose up after pull", err, stderr, nil), Status: status, Services: services} } return DeployResult{Output: output, Status: status, Services: services} } From 7076f9eabb13d08456172bddd20f8587a533a69a Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 03:10:27 -0700 Subject: [PATCH 3/4] fix(api): return 409 for rollback, restart and pull while a deploy is running --- docs/architecture/reconciler.md | 2 +- docs/reference/api.md | 12 ++-- internal/api/actions.go | 30 ++++++++++ internal/api/actions_busy_test.go | 97 ++++++++++++++++++++++++++++++ internal/deployer/tracker.go | 5 ++ internal/reconciler/busy_test.go | 98 +++++++++++++++++++++++++++++++ internal/reconciler/reconciler.go | 19 +++++- 7 files changed, 254 insertions(+), 9 deletions(-) create mode 100644 internal/api/actions_busy_test.go create mode 100644 internal/reconciler/busy_test.go diff --git a/docs/architecture/reconciler.md b/docs/architecture/reconciler.md index 526a5fbc..ba39a350 100644 --- a/docs/architecture/reconciler.md +++ b/docs/architecture/reconciler.md @@ -57,7 +57,7 @@ After every deploy (success or fail), `apps.status` is updated to `running` or ` ## Compose versions -Migration 009 also added `compose_versions`. After every successful deploy, the current compose file content is inserted with the same hash; the 10 most recent per app are kept. The UI's "Versions" tab lists these, and `RollbackOne(slug, versionID)` writes the saved content back to disk and triggers a redeploy. The version must belong to the app (`ErrVersionNotFound` otherwise), and it is checked with the app's current `.env` against today's rules before it replaces the file, so a refused rollback leaves the current file in place. +Migration 009 also added `compose_versions`. After every successful deploy, the current compose file content is inserted with the same hash; the 10 most recent per app are kept. The UI's "Versions" tab lists these, and `RollbackOne(slug, versionID)` writes the saved content back to disk and triggers a redeploy. The version must belong to the app (`ErrVersionNotFound` otherwise), and it is checked with the app's current `.env` against today's rules before it replaces the file, so a refused rollback leaves the current file in place. While another tracked operation (deploy, rollback, restart, pull) of the app runs, `RollbackOne` returns `deployer.ErrDeployInProgress` without touching the file (the API answers `409`); if one starts between that check and the deployer's own, the previous file is written back. `RestartOne` and `PullOne` return the same error when skipped, and their API handlers check first and answer `409` instead of a `202` that would do nothing. ## Registry credential resolution diff --git a/docs/reference/api.md b/docs/reference/api.md index e8689d2d..54535586 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -82,13 +82,15 @@ Deploy an app by uploading a compose file. `name` becomes the slug and the compo // Request {"name": "myapp", "compose": ""} -// Response 201 -{"name": "myapp", "status": "deployed"} +// Response 202 (deploy runs in the background) +{"name": "myapp", "status": "started"} // Response 400 (compose file fails security checks) {"error": "compose file contains disallowed directives", "violations": ["service \"web\": privileged mode not allowed"]} ``` +A compose file saved while the app is already deploying (or restarting, pulling, rolling back) is deployed as soon as that finishes. + Requires `super_admin`. Rollback and version restore check the stored version with the app's current `.env`: one that fails the checks or does not load gets the same `400` `{error, violations}` body, and one using a domain another app now has gets `409`. ### `DELETE /api/apps/{slug}` @@ -139,7 +141,7 @@ Stop, start, scale and rollback wait for `docker compose` to finish before respo ### `POST /api/apps/{slug}/restart` -Restart the app's existing containers in place (`docker compose restart`). Containers are not recreated, so compose changes and newer images are not picked up (use pull or a deploy for that). Runs in the background and returns `202` with `{"status": "started"}`. +Restart the app's existing containers in place (`docker compose restart`). Containers are not recreated, so compose changes and newer images are not picked up (use pull or a deploy for that). Runs in the background and returns `202` with `{"status": "started"}`. `409` while a deploy, rollback, restart or pull of the app is still running; try again when it finishes. ### `POST /api/apps/{slug}/stop` @@ -151,7 +153,7 @@ Start stopped containers. Returns `{"status": "ok"}`. ### `POST /api/apps/{slug}/pull` -Pull latest images and redeploy. Uses configured registry auth if available. Runs in the background and returns `202` with `{"status": "started"}`. +Pull latest images and redeploy. Uses configured registry auth if available. Runs in the background and returns `202` with `{"status": "started"}`. `409` while another deploy, rollback, restart or pull of the app is running. ### `POST /api/apps/{slug}/scale` @@ -197,7 +199,7 @@ Rollback to a previous compose version. {"status": "ok"} ``` -`400` with `{error, violations}` when the version fails today's checks or does not load; `409` when one of its domains now belongs to another app. +`400` with `{error, violations}` when the version fails today's checks or does not load; `409` when one of its domains now belongs to another app, or while a deploy, restart or pull of the app is running (nothing changes; try again when it finishes). When `docker compose` fails, `500` with `rollback failed: compose up: : exit status 1`. ### `GET /api/apps/{slug}/events` diff --git a/internal/api/actions.go b/internal/api/actions.go index 17df1dcb..db1e488e 100644 --- a/internal/api/actions.go +++ b/internal/api/actions.go @@ -14,6 +14,7 @@ import ( "github.com/vazra/simpledeploy/internal/audit" "github.com/vazra/simpledeploy/internal/compose" + "github.com/vazra/simpledeploy/internal/deployer" "github.com/vazra/simpledeploy/internal/store" ) @@ -41,12 +42,26 @@ func detachedCtx(r *http.Request) (context.Context, context.CancelFunc) { return context.WithTimeout(context.WithoutCancel(r.Context()), composeOpTimeout) } +// refuseWhileDeploying answers 409 when a deploy of slug is running. Restart +// and pull run in the background, where the deployer would skip them +// silently after the client already got 202. +func refuseWhileDeploying(w http.ResponseWriter, rec reconciler, slug string) bool { + if !rec.IsDeploying(slug) { + return false + } + http.Error(w, deployer.ErrDeployInProgress.Error(), http.StatusConflict) + return true +} + func (s *Server) handleRestart(w http.ResponseWriter, r *http.Request) { slug := r.PathValue("slug") if s.reconciler == nil { http.Error(w, "reconciler not configured", http.StatusInternalServerError) return } + if refuseWhileDeploying(w, s.reconciler, slug) { + return + } ctx := r.Context() go func() { if err := s.reconciler.RestartOne(context.Background(), slug); err != nil { @@ -117,6 +132,9 @@ func (s *Server) handlePull(w http.ResponseWriter, r *http.Request) { http.Error(w, "reconciler not configured", http.StatusInternalServerError) return } + if refuseWhileDeploying(w, s.reconciler, slug) { + return + } go func() { if err := s.reconciler.PullOne(context.Background(), slug); err != nil { fmt.Fprintf(os.Stderr, "pull %s: %v\n", slug, err) @@ -376,6 +394,18 @@ func (s *Server) handleRollback(w http.ResponseWriter, r *http.Request) { http.Error(w, dotEnvProblem, http.StatusBadRequest) return } + if errors.Is(err, deployer.ErrDeployInProgress) { + http.Error(w, err.Error(), http.StatusConflict) + return + } + // docker compose ran and failed: say why (secrets are already + // removed; the app's deploy log shows the same output). + var ce *deployer.ComposeError + if errors.As(err, &ce) { + log.Printf("[api] rollback %s: %v", slug, err) + http.Error(w, "rollback failed: "+ce.Error(), http.StatusInternalServerError) + return + } httpError(w, err, http.StatusInternalServerError) return } diff --git a/internal/api/actions_busy_test.go b/internal/api/actions_busy_test.go new file mode 100644 index 00000000..0f6a0ef9 --- /dev/null +++ b/internal/api/actions_busy_test.go @@ -0,0 +1,97 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/vazra/simpledeploy/internal/deployer" +) + +// busyReconciler has a deploy in flight for every app. +type busyReconciler struct { + mockReconcilerFull + rollbackErr error +} + +func (b *busyReconciler) IsDeploying(string) bool { return true } +func (b *busyReconciler) RollbackOne(ctx context.Context, slug string, id int64) error { + b.calls = append(b.calls, fmt.Sprintf("RollbackOne:%s:%d", slug, id)) + return b.rollbackErr +} + +func postAction(t *testing.T, srv *Server, path string, body any) *httptest.ResponseRecorder { + t.Helper() + var b []byte + if body != nil { + b, _ = json.Marshal(body) + } + req := httptest.NewRequest(http.MethodPost, path, bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + req.AddCookie(superAdminCookie(t, srv.jwt)) + w := httptest.NewRecorder() + srv.Handler().ServeHTTP(w, req) + return w +} + +func TestRollbackWhileDeployingReturns409(t *testing.T) { + srv, _ := newTestServer(t) + srv.SetAppsDir(t.TempDir()) + srv.SetReconciler(&busyReconciler{rollbackErr: deployer.ErrDeployInProgress}) + verID := seedVersion(t, srv, "myapp") + + w := postAction(t, srv, "/api/apps/myapp/rollback", map[string]any{"version_id": verID}) + if w.Code != http.StatusConflict { + t.Fatalf("status = %d, want 409; body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "a deploy is already running for this app") { + t.Errorf("body = %q, want the busy message", w.Body.String()) + } +} + +func TestRollbackComposeFailureSaysWhy(t *testing.T) { + srv, _ := newTestServer(t) + srv.SetAppsDir(t.TempDir()) + ce := &deployer.ComposeError{Op: "compose up", Detail: "Bind for 127.0.0.1:8080 failed: port is already allocated", Err: errors.New("exit status 1")} + rec := &busyReconciler{rollbackErr: fmt.Errorf("redeploy: %w", ce)} + srv.SetReconciler(¬Busy{rec}) + verID := seedVersion(t, srv, "myapp") + + w := postAction(t, srv, "/api/apps/myapp/rollback", map[string]any{"version_id": verID}) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500; body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "port is already allocated") { + t.Errorf("body = %q, want the compose error", w.Body.String()) + } +} + +type notBusy struct{ *busyReconciler } + +func (notBusy) IsDeploying(string) bool { return false } + +func TestRestartAndPullWhileDeployingReturn409(t *testing.T) { + for _, action := range []string{"restart", "pull"} { + srv, _ := newTestServer(t) + srv.SetAppsDir(t.TempDir()) + rec := &busyReconciler{} + srv.SetReconciler(rec) + + w := postAction(t, srv, "/api/apps/myapp/"+action, nil) + if w.Code != http.StatusConflict { + t.Fatalf("%s: status = %d, want 409; body: %s", action, w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "a deploy is already running for this app") { + t.Errorf("%s: body = %q, want the busy message", action, w.Body.String()) + } + if len(rec.calls) != 0 { + t.Errorf("%s: calls = %v, want none", action, rec.calls) + } + } +} diff --git a/internal/deployer/tracker.go b/internal/deployer/tracker.go index f4f8a6f3..55b1fc7c 100644 --- a/internal/deployer/tracker.go +++ b/internal/deployer/tracker.go @@ -2,10 +2,15 @@ package deployer import ( "context" + "errors" "fmt" "sync" ) +// ErrDeployInProgress is returned for an action that cannot run because a +// deploy (or rollback, restart, pull) of the same app is still running. +var ErrDeployInProgress = errors.New("a deploy is already running for this app, try again when it finishes") + type OutputLine struct { Line string `json:"line"` Stream string `json:"stream"` // "stdout" or "stderr" diff --git a/internal/reconciler/busy_test.go b/internal/reconciler/busy_test.go new file mode 100644 index 00000000..93a44c7b --- /dev/null +++ b/internal/reconciler/busy_test.go @@ -0,0 +1,98 @@ +package reconciler + +import ( + "context" + "errors" + "testing" + + "github.com/vazra/simpledeploy/internal/compose" + "github.com/vazra/simpledeploy/internal/deployer" + "github.com/vazra/simpledeploy/internal/store" +) + +// newDeployerEnv is newTestEnv with a real deployer on a fake docker, so +// IsDeploying reads the same in-flight tracking as in production. +func newDeployerEnv(t *testing.T) (*Reconciler, *deployer.Deployer, *deployer.MockRunner, *store.Store, string) { + t.Helper() + r, _, st, appsDir := newTestEnv(t) + runner := &deployer.MockRunner{} + d, err := deployer.New(runner) + if err != nil { + t.Fatal(err) + } + r.deployer = d + return r, d, runner, st, appsDir +} + +// hold marks slug as having a deploy in flight until release is called. +func hold(t *testing.T, d *deployer.Deployer, slug string) (release func()) { + t.Helper() + if _, ok := d.Tracker.TrackWithLog(slug, func() {}); !ok { + t.Fatalf("%s already in flight", slug) + } + return func() { d.Tracker.DoneWithLog(slug, "deploy") } +} + +func TestRollbackWhileDeployingIsRefused(t *testing.T) { + t.Setenv("SIMPLEDEPLOY_DISABLE_PORT_LOOPBACK", "true") + r, d, runner, st, appsDir := newDeployerEnv(t) + _, ver := seedApp(t, st, appsDir, "app", "services:\n web:\n image: nginx:0.9\n") + release := hold(t, d, "app") + defer release() + + err := r.RollbackOne(context.Background(), "app", ver) + if !errors.Is(err, deployer.ErrDeployInProgress) { + t.Fatalf("err = %v, want ErrDeployInProgress", err) + } + if got := readCompose(t, appsDir, "app"); got != safeCompose { + t.Errorf("compose was overwritten: %q", got) + } + if runner.HasCall("docker", "up") { + t.Error("compose up ran during another deploy") + } +} + +// skipper reports tracked operations as skipped, as when another deploy +// takes the app between the reconciler's check and the deployer's. +type skipper struct{ *mockDeployer } + +func (skipper) RollbackDeploy(context.Context, *compose.AppConfig, int, *int64, ...deployer.RegistryAuth) deployer.DeployResult { + return deployer.DeployResult{Skipped: true} +} +func (skipper) Restart(context.Context, *compose.AppConfig) deployer.DeployResult { + return deployer.DeployResult{Skipped: true} +} +func (skipper) Pull(context.Context, *compose.AppConfig, []deployer.RegistryAuth) deployer.DeployResult { + return deployer.DeployResult{Skipped: true} +} + +func TestRollbackSkippedRestoresComposeFile(t *testing.T) { + t.Setenv("SIMPLEDEPLOY_DISABLE_PORT_LOOPBACK", "true") + r, mock, st, appsDir := newTestEnv(t) + r.deployer = skipper{mock} + _, ver := seedApp(t, st, appsDir, "app", "services:\n web:\n image: nginx:0.9\n") + + err := r.RollbackOne(context.Background(), "app", ver) + if !errors.Is(err, deployer.ErrDeployInProgress) { + t.Fatalf("err = %v, want ErrDeployInProgress", err) + } + if got := readCompose(t, appsDir, "app"); got != safeCompose { + t.Errorf("compose = %q, want the current file restored", got) + } + if evs, _ := st.ListDeployEvents("app"); len(evs) != 0 { + t.Errorf("deploy events = %+v, want none for a rollback that never ran", evs) + } +} + +func TestRestartAndPullSkippedReportBusy(t *testing.T) { + r, mock, st, appsDir := newTestEnv(t) + r.deployer = skipper{mock} + seedApp(t, st, appsDir, "app", safeCompose) + + if err := r.RestartOne(context.Background(), "app"); !errors.Is(err, deployer.ErrDeployInProgress) { + t.Errorf("RestartOne err = %v, want ErrDeployInProgress", err) + } + if err := r.PullOne(context.Background(), "app"); !errors.Is(err, deployer.ErrDeployInProgress) { + t.Errorf("PullOne err = %v, want ErrDeployInProgress", err) + } +} diff --git a/internal/reconciler/reconciler.go b/internal/reconciler/reconciler.go index 0dc6592d..a23b3bb3 100644 --- a/internal/reconciler/reconciler.go +++ b/internal/reconciler/reconciler.go @@ -345,7 +345,7 @@ func (r *Reconciler) RestartOne(ctx context.Context, slug string) error { } result := r.deployer.Restart(ctx, cfg) if result.Skipped { - return nil + return deployer.ErrDeployInProgress } action := "restart" status := "running" @@ -434,7 +434,7 @@ func (r *Reconciler) PullOne(ctx context.Context, slug string) error { } result := r.deployer.Pull(ctx, cfg, auths) if result.Skipped { - return nil + return deployer.ErrDeployInProgress } action := "pull" status := "running" @@ -609,6 +609,12 @@ func (r *Reconciler) RollbackOne(ctx context.Context, slug string, versionID int return &compose.ViolationError{Violations: v} } + // Never swap the file under a running deploy: it would be skipped + // below and leave this version on disk without running it. + if r.IsDeploying(slug) { + return deployer.ErrDeployInProgress + } + prev, prevErr := fsutil.ReadRegularFile(composePath) if err := fsutil.WriteFileAtomic(composePath, composeData, 0o600); err != nil { return fmt.Errorf("write compose: %w", err) } @@ -625,7 +631,14 @@ func (r *Reconciler) RollbackOne(ctx context.Context, slug string, versionID int cvID := versionID result := r.deployer.RollbackDeploy(ctx, cfg, ver.Version, &cvID, auths...) if result.Skipped { - return nil + // A deploy started after the check above. Put the current file + // back so the version that did not run is not deployed later. + if prevErr == nil { + if err := fsutil.WriteFileAtomic(composePath, prev, 0o600); err != nil { + log.Printf("[reconciler] restore %s compose after skipped rollback: %v", slug, err) + } + } + return deployer.ErrDeployInProgress } action := "rollback" From cbca31c6cd5539b38fae7504b834dc431c27210f Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 10:08:44 -0700 Subject: [PATCH 4/4] fix: retry a reconcile skipped behind a running deploy once it finishes --- docs/architecture/reconciler.md | 2 ++ internal/deployer/deployer.go | 18 +++++++++-- internal/reconciler/busy_test.go | 35 +++++++++++++++++++++ internal/reconciler/reconciler.go | 51 +++++++++++++++++++++++++++++-- 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/docs/architecture/reconciler.md b/docs/architecture/reconciler.md index ba39a350..b1dd8c3e 100644 --- a/docs/architecture/reconciler.md +++ b/docs/architecture/reconciler.md @@ -23,6 +23,8 @@ There is no internal periodic tick. If you want a heartbeat sweep, restart the p Steps 3 are run as goroutines under a `chan struct{}` semaphore of capacity **3**, so at most 3 deploys execute concurrently. Step 4 runs serially after the wait. +An app whose deploy is already running (an API deploy, rollback, restart or pull) is skipped in step 3, since that deploy may have read an older file. One `Reconcile()` is queued to run when it finishes (at most one per app); the hash check makes it a no-op when nothing changed. + ## Hash detection Migration 008 added `apps.compose_hash` as a `TEXT` column. After every successful deploy, `deployApp()` writes the SHA-256 of the compose file. On the next reconcile, file hash is recomputed and compared. Match -> skip. Mismatch -> deploy. Failure to read -> log and skip. Hashing reads through `fsutil.ReadRegularFile`, so a symlinked file never hashes. diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go index 93675f0d..003b61e8 100644 --- a/internal/deployer/deployer.go +++ b/internal/deployer/deployer.go @@ -74,12 +74,24 @@ func New(runner CommandRunner) (*Deployer, error) { return d, nil } -// runCmd uses RunStreaming when a DeployLog is available, otherwise falls back to d.runner.Run. +// runCmd streams output to the DeployLog when one is available and the +// runner is the real ExecRunner. Other runners (test fakes) go through Run, +// and their output is replayed into the log afterwards. func (d *Deployer) runCmd(ctx context.Context, dl *DeployLog, name string, args ...string) (string, string, error) { - if dl != nil { + if _, real := d.runner.(*ExecRunner); real && dl != nil { return RunStreaming(ctx, dl, name, args...) } - return d.runner.Run(ctx, name, args...) + stdout, stderr, err := d.runner.Run(ctx, name, args...) + if dl != nil { + for _, out := range []struct{ text, stream string }{{stdout, "stdout"}, {stderr, "stderr"}} { + for _, l := range strings.Split(strings.TrimRight(out.text, "\n"), "\n") { + if l != "" { + dl.Send(OutputLine{Line: l, Stream: out.stream}) + } + } + } + } + return stdout, stderr, err } func (d *Deployer) Deploy(ctx context.Context, app *compose.AppConfig, auths ...RegistryAuth) (result DeployResult) { diff --git a/internal/reconciler/busy_test.go b/internal/reconciler/busy_test.go index 93a44c7b..00c4a8c5 100644 --- a/internal/reconciler/busy_test.go +++ b/internal/reconciler/busy_test.go @@ -3,7 +3,10 @@ package reconciler import ( "context" "errors" + "os" + "path/filepath" "testing" + "time" "github.com/vazra/simpledeploy/internal/compose" "github.com/vazra/simpledeploy/internal/deployer" @@ -96,3 +99,35 @@ func TestRestartAndPullSkippedReportBusy(t *testing.T) { t.Errorf("PullOne err = %v, want ErrDeployInProgress", err) } } + +// A watcher reconcile skipped because another deploy of the app was running +// is retried once that deploy finishes, so the newer file gets deployed. +func TestSkippedReconcileRetriesAfterDeploy(t *testing.T) { + t.Setenv("SIMPLEDEPLOY_DISABLE_PORT_LOOPBACK", "true") + old := retryPoll + retryPoll = 10 * time.Millisecond + t.Cleanup(func() { retryPoll = old }) + + r, d, runner, st, appsDir := newDeployerEnv(t) + seedApp(t, st, appsDir, "app", safeCompose) + release := hold(t, d, "app") + if err := os.WriteFile(filepath.Join(appsDir, "app", "docker-compose.yml"), []byte("services:\n web:\n image: nginx:2\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + time.Sleep(50 * time.Millisecond) + if runner.HasCall("docker", "up") { + t.Fatal("compose up ran while another deploy was in flight") + } + + release() + deadline := time.Now().Add(5 * time.Second) + for !runner.HasCall("docker", "up") { + if time.Now().After(deadline) { + t.Fatal("skipped reconcile was not retried after the running deploy finished") + } + time.Sleep(20 * time.Millisecond) + } +} diff --git a/internal/reconciler/reconciler.go b/internal/reconciler/reconciler.go index a23b3bb3..d19616de 100644 --- a/internal/reconciler/reconciler.go +++ b/internal/reconciler/reconciler.go @@ -60,8 +60,18 @@ type Reconciler struct { syncer *configsync.Syncer // nil means configsync disabled audit *audit.Recorder // nil-safe bus *events.Bus // nil-safe + + retryMu sync.Mutex + retrying map[string]bool // slugs with a reconcile queued behind a running deploy } +// retryPoll is how often a reconcile queued behind a running deploy checks +// whether that deploy finished; retryMaxWait caps the wait. +var ( + retryPoll = time.Second + retryMaxWait = 30 * time.Minute +) + // scanResult is one pass over the apps directory. type scanResult struct { desired map[string]*compose.AppConfig @@ -167,8 +177,10 @@ func (r *Reconciler) Reconcile(ctx context.Context) error { // (handleDeploy spawns its own goroutine). Without this guard the // watcher-triggered reconcile races against the API goroutine, // double-dispatches docker compose for the same project, and - // orphans the WS subscribers watching the first deploy. + // orphans the WS subscribers watching the first deploy. That deploy + // may have read an older file, so check again once it finishes. if r.IsDeploying(slug) { + r.reconcileAfterDeploy(slug) continue } wg.Add(1) @@ -563,6 +575,37 @@ func (r *Reconciler) CancelOne(ctx context.Context, slug string) error { return r.deployer.Cancel(ctx, cfg) } +// reconcileAfterDeploy runs one Reconcile after slug's running deploy +// finishes. At most one is queued per slug; a Reconcile that skips again +// while it runs does not queue another, so this cannot loop. +func (r *Reconciler) reconcileAfterDeploy(slug string) { + r.retryMu.Lock() + if r.retrying[slug] { + r.retryMu.Unlock() + return + } + if r.retrying == nil { + r.retrying = map[string]bool{} + } + r.retrying[slug] = true + r.retryMu.Unlock() + + go func() { + defer func() { + r.retryMu.Lock() + delete(r.retrying, slug) + r.retryMu.Unlock() + }() + deadline := time.Now().Add(retryMaxWait) + for r.IsDeploying(slug) && time.Now().Before(deadline) { + time.Sleep(retryPoll) + } + if err := r.Reconcile(context.Background()); err != nil { + log.Printf("[reconciler] reconcile after skipped deploy of %s: %v", slug, err) + } + }() +} + func (r *Reconciler) IsDeploying(slug string) bool { if d, ok := r.deployer.(*deployer.Deployer); ok { return d.Tracker.IsDeploying(slug) @@ -861,8 +904,10 @@ func (r *Reconciler) deployApp(ctx context.Context, slug string, cfg *compose.Ap } result := r.deployer.Deploy(ctx, cfg, auths...) if result.Skipped { - // Another in-flight deploy for this slug is handling store updates - // and WS notifications; nothing more to do here. + // Another in-flight deploy for this slug handles store updates and + // WS notifications. The file may have changed after that deploy read + // it, so reconcile again once it finishes (a no-op when unchanged). + r.reconcileAfterDeploy(slug) return nil }