From 27b67d41e8f89d320a13079543254a94e8d97be8 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:16:38 -0400 Subject: [PATCH 1/5] test(next-step): assert the deadline path instead of a 4s wall-clock bound (D-0492) --- cmd/commands/next_step_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cmd/commands/next_step_test.go b/cmd/commands/next_step_test.go index 62f6237f9..9238de9e5 100644 --- a/cmd/commands/next_step_test.go +++ b/cmd/commands/next_step_test.go @@ -153,14 +153,20 @@ func TestNextStep(t *testing.T) { t.Run(tc.stage+"_"+tc.mode, func(t *testing.T) { nextStepFixture(t, tc.stage) t.Setenv("NEXTSTEP_DOCKER", tc.mode) - start := time.Now() ctx, cancel := context.WithTimeout(context.Background(), nextStepTimeout) defer cancel() if got := nextStep(ctx); got != "Next: nself "+tc.want { t.Errorf("got %q, want %q", got, tc.want) } - if elapsed := time.Since(start); elapsed > 4*time.Second { - t.Errorf("detection took %v", elapsed) + // Assert the deadline path, not a wall-clock bound: a shared + // Windows runner took 9.5s here (D-0492). The slow stub must run + // into the deadline; every other case must finish inside it. + wantErr := error(nil) + if tc.mode == "slow" { + wantErr = context.DeadlineExceeded + } + if err := ctx.Err(); err != wantErr { + t.Errorf("ctx.Err() = %v, want %v", err, wantErr) } }) } From 5de3010d09ffcdf6bac7fa9f607206864c628f6b Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:16:38 -0400 Subject: [PATCH 2/5] fix(deploy): rolling progress line prints the real gate limit, up is bounded, interrupted up says how to recover (D-0469) --- .github/wiki/cmd-deploy.md | 14 ++- cmd/commands/deploy_rolling_up.go | 47 +++++++++ cmd/commands/deploy_rolling_up_test.go | 97 +++++++++++++++++++ cmd/commands/deploy_strategies.go | 19 ++-- cmd/commands/deploy_strategies_test.go | 5 +- internal/docker/compose_services.go | 23 +++-- internal/docker/compose_services_gate_test.go | 23 +++++ 7 files changed, 209 insertions(+), 19 deletions(-) create mode 100644 cmd/commands/deploy_rolling_up.go create mode 100644 cmd/commands/deploy_rolling_up_test.go diff --git a/.github/wiki/cmd-deploy.md b/.github/wiki/cmd-deploy.md index ef025f3fc..38fd17e7f 100644 --- a/.github/wiki/cmd-deploy.md +++ b/.github/wiki/cmd-deploy.md @@ -246,7 +246,7 @@ The registry marks the `--stream` flag `output: stream`, so MCP and HTTP keep th | Strategy | Status | Behavior | |---|---|---| -| `rolling` | **Default (v1.1.1)** | Per-service sequenced restart with health-gating. Each service waits up to 60s for `service_healthy` before the next restarts. | +| `rolling` | **Default (v1.1.1)** | Per-service sequenced restart with health-gating. Each service waits for `service_healthy` (60s, or the healthcheck's own declared timing when that is longer, up to 10m) before the next restarts. | | `blue-green / canary` | **Available via `--canary N` (Y17)** | Zero-downtime: green containers run alongside blue, Nginx shifts N% traffic to green during soak, then promotes to 100%. Requires `NSELF_FEATURE_BLUE_GREEN_DEPLOY=true`. | | `preview` | Not yet implemented | Falls back to rolling with an explicit warning. | @@ -335,7 +335,7 @@ fallback for plugin variables: a plugin variable must be in `.nself/compose.env` | Service kind | Waits for | Limit | On failure | |---|---|---|---| -| Declares a healthcheck | `healthy` | 60s | E251 (health check timeout), service named | +| Declares a healthcheck | `healthy` | 60s, or the healthcheck's own timing (`start_period` + retries × (`interval` + `timeout`)) when that is longer, capped at 10m | E251 (health check timeout), service named | | One-shot (`restart: "no"`, no healthcheck) | a new run that exits 0 | 60s | non-zero exit fails at once with E250; never exiting, or never starting again, is E251 | | Anything else | `running`, seen as the same run on two polls 2s apart | 30s | E251, service named | @@ -352,6 +352,14 @@ with E252, checked for every service before the first restart. If Docker cannot deploy stops with Docker's own message and no service code, because nothing is known about the service; an interrupt (ctrl-c) stops the same way. +**Bounded `up`, and an interrupted one.** The progress line prints the limit the gate really +applies, for example `healthy, max 105s`. `docker compose up -d --no-deps` itself is bounded at 15 +minutes per service (it can pull an image before it returns). If it does not finish, docker is +stopped and the deploy fails naming the service. A Ctrl-C during `up`, or docker exiting 130 before +nself saw the interrupt, ends the deploy as an interrupt, not a failure. `up` stops the old +container before it starts the new one, so the service may be left stopped: both messages end with +`Service may be stopped: run 'nself restart ' to bring it back`. + **What a failed deploy leaves behind.** Nothing rolls back. Services before the failed one already run the new config. The failed service is in whatever state `up` left it (running but not ready, restarting, or exited). Services after it are not touched and keep running the old config. Fix the cause and run @@ -397,7 +405,7 @@ serving until its replacement becomes healthy. pulled yet), so it prints the resolution rule instead of a concrete list — the real order is only known once the build for this deploy has run. -Use `--skip-health-check` to bypass the 60s gate for known-slow services like MeiliSearch or +Use `--skip-health-check` to bypass the health gate for known-slow services like MeiliSearch or Grafana. Set `HEALTHCHECK_TIMEOUT_` (e.g. `HEALTHCHECK_TIMEOUT_GRAFANA=120s`) to extend the per-service timeout. diff --git a/cmd/commands/deploy_rolling_up.go b/cmd/commands/deploy_rolling_up.go new file mode 100644 index 000000000..d30b8a197 --- /dev/null +++ b/cmd/commands/deploy_rolling_up.go @@ -0,0 +1,47 @@ +package commands + +// Purpose: the bounded `up -d --no-deps` step of the rolling restart (D-0469). +// Inputs: a rollingPlan and one service name. Outputs: nil, or an error that +// says whether docker timed out, was interrupted, or failed. +// Constraints: every docker call goes through internal/docker. + +import ( + "context" + "errors" + "fmt" + "os/exec" + "time" +) + +// rollingUpTimeout bounds one `up -d --no-deps` in the rolling path. It is +// generous because `up` can pull an image and recreate the container before it +// returns; the health gate has its own, shorter limit. A var so tests can shorten it. +var rollingUpTimeout = 15 * time.Minute + +// exitInterrupted is the status a docker child gives back when it saw SIGINT. +const exitInterrupted = 130 + +// rollingUp brings svc up under rollingUpTimeout. A daemon that hangs during +// `up` ends in a named error instead of hanging the deploy, and an interrupted +// `up` says the service may be left stopped and how to bring it back, because +// `up` stops the old container before it starts the new one. +func rollingUp(ctx context.Context, plan rollingPlan, svc string) error { + upCtx, cancel := context.WithTimeout(ctx, rollingUpTimeout) + defer cancel() + err := plan.compose.ComposeUpNoDeps(upCtx, plan.workdir, svc) + if err == nil { + return nil + } + hint := fmt.Sprintf("Service %s may be stopped: run 'nself restart %s' to bring it back", svc, svc) + var exit *exec.ExitError + switch { + case ctx.Err() != nil: + return fmt.Errorf("rolling restart interrupted while bringing up service %s: %w. %s", svc, ctx.Err(), hint) + case errors.As(err, &exit) && exit.ExitCode() == exitInterrupted: + // docker saw the Ctrl-C before nself's own handler cancelled ctx. + return fmt.Errorf("rolling restart interrupted while bringing up service %s: %w. %s", svc, context.Canceled, hint) + case upCtx.Err() != nil: + return fmt.Errorf("rolling restart: service %s did not finish 'up' within %s, so docker was stopped. %s", svc, rollingUpTimeout, hint) + } + return fmt.Errorf("rolling restart: service %s restart failed: %w\nRun 'nself logs %s' for details", svc, err, svc) +} diff --git a/cmd/commands/deploy_rolling_up_test.go b/cmd/commands/deploy_rolling_up_test.go new file mode 100644 index 000000000..6bbb0134f --- /dev/null +++ b/cmd/commands/deploy_rolling_up_test.go @@ -0,0 +1,97 @@ +package commands + +// deploy_rolling_up_test.go — the rolling `up` is bounded and its failures say +// what happened and how to recover (D-0469). Uses the fake docker of +// deploy_strategies_test.go; nothing here needs a daemon. + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/nself-org/cli/internal/docker" +) + +func TestRollingUp(t *testing.T) { + stack := func(t *testing.T, fixture string) *rollingStack { + t.Helper() + s := newRollingStack(t, []string{"postgres", "web"}, stackConfig) + s.setPs(t, "postgres", `[{"Name":"p-pg-1","State":"running","Health":"healthy"}]`) + if err := os.WriteFile(filepath.Join(s.fix, fixture), []byte("web"), 0o600); err != nil { + t.Fatal(err) + } + return s + } + + t.Run("a hung up ends in a timeout error that names the service and the way back", func(t *testing.T) { + s := stack(t, "uphang") + old := rollingUpTimeout + rollingUpTimeout = 300 * time.Millisecond + t.Cleanup(func() { rollingUpTimeout = old }) + start := time.Now() + _, err := runRollingRestart(context.Background(), s.dir, true) + if time.Since(start) > 30*time.Second { + t.Fatalf("deploy was not cut off: took %s", time.Since(start)) + } + if err == nil { + t.Fatal("want a timeout error, got nil") + } + for _, want := range []string{"service web did not finish 'up' within 300ms", "nself restart web"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q lacks %q", err, want) + } + } + if errors.Is(err, context.Canceled) { + t.Errorf("a timeout must not read as the user's interrupt: %v", err) + } + }) + + t.Run("docker exiting 130 before nself's handler runs reads as an interrupt", func(t *testing.T) { + s := stack(t, "up130") + _, err := runRollingRestart(context.Background(), s.dir, true) + if !errors.Is(err, context.Canceled) { + t.Fatalf("want an interrupt, got %v", err) + } + for _, want := range []string{"interrupted while bringing up service web", "nself restart web"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q lacks %q", err, want) + } + } + if strings.Contains(err.Error(), "restart failed") || errors.Is(err, docker.ErrStateRead) { + t.Errorf("an interrupt must not read as a failure: %v", err) + } + }) + + t.Run("a cancelled context prints the recovery line", func(t *testing.T) { + s := stack(t, "uphang") + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(300*time.Millisecond, cancel) + _, err := runRollingRestart(ctx, s.dir, true) + if !errors.Is(err, context.Canceled) || !strings.Contains(err.Error(), "nself restart web") { + t.Fatalf("want an interrupt with the recovery line, got %v", err) + } + }) +} + +func TestGateKindPrintsTheRealLimit(t *testing.T) { + for name, tc := range map[string]struct { + spec docker.ServiceSpec + want string + }{ + "healthcheck default": {docker.ServiceSpec{HasHealthcheck: true}, "healthy, max 60s"}, + "healthcheck slow": {docker.ServiceSpec{HasHealthcheck: true, HealthTimeout: 105 * time.Second}, "healthy, max 105s"}, + "healthcheck at the cap": {docker.ServiceSpec{HasHealthcheck: true, HealthTimeout: docker.MaxHealthTimeout}, "healthy, max 600s"}, + "one-shot": {docker.ServiceSpec{Restart: "no"}, "one-shot: exit 0, max 60s"}, + "plain": {docker.ServiceSpec{}, "running, max 30s"}, + } { + t.Run(name, func(t *testing.T) { + if got := gateKind(tc.spec); got != tc.want { + t.Errorf("gateKind = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/cmd/commands/deploy_strategies.go b/cmd/commands/deploy_strategies.go index 0e0146e57..136238482 100644 --- a/cmd/commands/deploy_strategies.go +++ b/cmd/commands/deploy_strategies.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "strings" + "time" "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/docker" @@ -66,7 +67,8 @@ type rollingPlan struct { // services last), never a hardcoded list: a fixed list restarted a service // that did not exist and aborted a real deploy half way (production incident // 2026-09-20). Each service is brought up with `up -d --no-deps` and then -// gated by kind: healthcheck declared -> healthy within 60s; restart "no" +// gated by kind: healthcheck declared -> healthy within 60s (longer when the +// healthcheck's own timing needs it, up to 10m); restart "no" // without a healthcheck (one-shot) -> a new run that exited 0; otherwise // running within 30s and still the same run a poll later. Any kind fails at // once on a container that exited non-zero or keeps restarting. The deploy @@ -114,9 +116,8 @@ func rollingRestart(ctx context.Context, plan rollingPlan) ([]deployStep, error) if err != nil { return addStep(ctx, steps, deployStep{Name: stepName, Status: "failed"}), gateError(svc, markErr(ctx, err)) } - if err := plan.compose.ComposeUpNoDeps(ctx, plan.workdir, svc); err != nil { - steps = addStep(ctx, steps, deployStep{Name: stepName, Status: "failed"}) - return steps, fmt.Errorf("rolling restart: service %s restart failed: %w\nRun 'nself logs %s' for details", svc, err, svc) + if err := rollingUp(ctx, plan, svc); err != nil { + return addStep(ctx, steps, deployStep{Name: stepName, Status: "failed"}), err } if !plan.jsonOut { fmt.Printf(" [waiting] Waiting for %s (%s)\n", svc, gateKind(spec)) @@ -181,15 +182,17 @@ func markErr(ctx context.Context, err error) error { return fmt.Errorf("%w: %w", docker.ErrStateRead, err) } -// gateKind names what the gate waits for, for the progress line. +// gateKind names what the gate waits for, for the progress line, with the +// limit HealthGate really applies (a slow healthcheck waits longer than 60s). func gateKind(sp docker.ServiceSpec) string { + limit := fmt.Sprintf("max %ds", int(docker.GateLimit(sp)/time.Second)) switch { case sp.OneShot(): - return "one-shot: exit 0, max 60s" + return "one-shot: exit 0, " + limit case sp.HasHealthcheck: - return "healthy, max 60s" + return "healthy, " + limit default: - return "running, max 30s" + return "running, " + limit } } diff --git a/cmd/commands/deploy_strategies_test.go b/cmd/commands/deploy_strategies_test.go index 713355a7c..8270b3c06 100644 --- a/cmd/commands/deploy_strategies_test.go +++ b/cmd/commands/deploy_strategies_test.go @@ -20,7 +20,7 @@ import ( // rollingFakeDocker answers the four docker compose calls the restart makes. // Fixtures live next to the log: services (config --services), config.json, -// ps..json, psfail (any `ps` exits 1), upfail (a service name whose `up` exits 1) and startfixed +// ps..json, psfail (any `ps` exits 1), upfail (a service name whose `up` exits 1), uphang, up130 and startfixed // (a constant `docker inspect` StartedAt, i.e. the container never restarted). const rollingFakeDocker = `#!/bin/sh D="$FAKE_DOCKER_DIR" @@ -38,6 +38,9 @@ config) up) for last; do :; done if [ -f "$D/upfail" ] && [ "$(cat "$D/upfail")" = "$last" ]; then echo "boom" >&2; exit 1; fi + # uphang names a service whose up never returns; up130 one whose up exits like an interrupted docker. + if [ -f "$D/uphang" ] && [ "$(cat "$D/uphang")" = "$last" ]; then exec sleep 60; fi + if [ -f "$D/up130" ] && [ "$(cat "$D/up130")" = "$last" ]; then exit 130; fi echo "$last" >> "$D/ups" ;; ps) if [ -f "$D/psfail" ]; then echo "Cannot connect to the Docker daemon" >&2; exit 1; fi diff --git a/internal/docker/compose_services.go b/internal/docker/compose_services.go index a7febf96f..8c4d136ac 100644 --- a/internal/docker/compose_services.go +++ b/internal/docker/compose_services.go @@ -128,6 +128,21 @@ func (SystemClock) Sleep(ctx context.Context, d time.Duration) error { // a source that leaves it empty still gets the two-poll check. type StateSource func(ctx context.Context) ([]ContainerState, error) +// GateLimit is how long HealthGate waits for spec: RunningTimeout for a plain +// service, HealthyTimeout for a one-shot, and for a healthcheck service the +// larger of HealthyTimeout and its declared HealthTimeout. Callers that print +// the wait (the deploy progress line) use it so they never disagree with the gate. +func GateLimit(spec ServiceSpec) time.Duration { + limit := RunningTimeout + if spec.HasHealthcheck || spec.OneShot() { + limit = HealthyTimeout + } + if spec.HasHealthcheck { + limit = max(limit, spec.HealthTimeout) + } + return limit +} + // HealthGate waits until spec's containers are ready for their kind (D17). // Any kind fails at once on a container that exited non-zero or is // restarting. Healthcheck declared -> every container healthy within @@ -144,13 +159,7 @@ func HealthGate(ctx context.Context, spec ServiceSpec, states StateSource, clock if clock == nil { clock = SystemClock{} } - limit := RunningTimeout - if spec.HasHealthcheck || spec.OneShot() { - limit = HealthyTimeout - } - if spec.HasHealthcheck { - limit = max(limit, spec.HealthTimeout) - } + limit := GateLimit(spec) deadline := clock.Now().Add(limit) var prev map[string]string for { diff --git a/internal/docker/compose_services_gate_test.go b/internal/docker/compose_services_gate_test.go index ff2b7e137..000842004 100644 --- a/internal/docker/compose_services_gate_test.go +++ b/internal/docker/compose_services_gate_test.go @@ -167,6 +167,29 @@ func TestHealthGateHealthTimeout(t *testing.T) { } } +// TestGateLimit pins the wait HealthGate applies per service kind; the deploy +// progress line prints this value (D-0469), so it must not drift from the gate. +func TestGateLimit(t *testing.T) { + for name, tc := range map[string]struct { + spec ServiceSpec + want time.Duration + }{ + "plain service": {ServiceSpec{Name: "web"}, RunningTimeout}, + "one-shot": {ServiceSpec{Name: "init", Restart: "no"}, HealthyTimeout}, + "healthcheck, no timing": {ServiceSpec{Name: "pg", HasHealthcheck: true}, HealthyTimeout}, + "healthcheck, short timing": {ServiceSpec{Name: "pg", HasHealthcheck: true, HealthTimeout: 10 * time.Second}, HealthyTimeout}, + "healthcheck, declared 105s": {ServiceSpec{Name: "pg", HasHealthcheck: true, HealthTimeout: 105 * time.Second}, 105 * time.Second}, + "healthcheck, capped at 10min": {ServiceSpec{Name: "pg", HasHealthcheck: true, HealthTimeout: MaxHealthTimeout}, MaxHealthTimeout}, + "timing without healthcheck": {ServiceSpec{Name: "web", HealthTimeout: 3 * time.Minute}, RunningTimeout}, + } { + t.Run(name, func(t *testing.T) { + if got := GateLimit(tc.spec); got != tc.want { + t.Errorf("GateLimit = %s, want %s", got, tc.want) + } + }) + } +} + func TestParseServiceSpecsHealthTiming(t *testing.T) { hcOf := func(hc string) string { return `{"services":{"s":{"healthcheck":` + hc + `}}}` } for name, tc := range map[string]struct { From 0398c24c2660051140f3b243e69038c634b8f011 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:16:38 -0400 Subject: [PATCH 3/5] fix(nginx): refuse $ in proxy targets the route model cannot represent (D-1139) --- .github/wiki/Nginx-Generation.md | 2 +- internal/nginx/routemodel/build.go | 7 +++++++ internal/nginx/routemodel/build_apidocs_test.go | 8 +++++++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/wiki/Nginx-Generation.md b/.github/wiki/Nginx-Generation.md index 4cbcd2a81..31a84f18b 100644 --- a/.github/wiki/Nginx-Generation.md +++ b/.github/wiki/Nginx-Generation.md @@ -36,7 +36,7 @@ removing it manually during migration. Each build writes `.nself/generated/routes.json`, version 1 of the proxy route contract. Its `_generated` key identifies the file as build output. The file describes the project and environment, HTTP defaults, the default server, rate zones, and every generated core, optional, custom service, frontend, and internal route. Route entries include their source, server names, TLS settings, security headers, blocked paths, upstream locations, rate limits, and any `shadowed_by` hand-managed file. Routes and zones have stable ordering. Hostnames, ports, and paths are included; credentials are excluded. -The docs site is listed as the core route `core:api-docs` (server name `docs.`, HTTPS only) when `API_DOCS_ENABLED` is not `false`. Its locations carry `static_file` (the file nginx serves with `alias`) and `headers_add` (the `add_header` set). Two more optional fields describe proxy targets: a location's `upstream` may carry `path` and `query`. The cli renders `set $up_x ; proxy_pass $up_x;`, so with a variable `proxy_pass` nginx sends the target's path and query instead of the request URI (the whole URI is replaced, not just the matched prefix). A consumer that cannot do the same must refuse such a route. A target with a fragment (`#`), user info, a space, or any non-ASCII byte is refused at build time. All four fields are omitted from the JSON when unset. +The docs site is listed as the core route `core:api-docs` (server name `docs.`, HTTPS only) when `API_DOCS_ENABLED` is not `false`. Its locations carry `static_file` (the file nginx serves with `alias`) and `headers_add` (the `add_header` set). Two more optional fields describe proxy targets: a location's `upstream` may carry `path` and `query`. The cli renders `set $up_x ; proxy_pass $up_x;`, so with a variable `proxy_pass` nginx sends the target's path and query instead of the request URI (the whole URI is replaced, not just the matched prefix). A consumer that cannot do the same must refuse such a route. A target with a fragment (`#`), user info, a space, any non-ASCII byte, or a `$` is refused at build time (nginx would expand a `$name` in the `set` value, so the model could not describe the real target; write `%24` for a literal dollar sign). All four fields are omitted from the JSON when unset. The nginx renderer reads this model. A route suppressed by a hand-managed `conf.d` file remains in the JSON with `shadowed_by` set, while its generated `nginx/sites/` file is omitted. Plugin snippets and hand-managed server blocks are in the same file; see [Plugin and hand-managed routes](#plugin-and-hand-managed-routes). diff --git a/internal/nginx/routemodel/build.go b/internal/nginx/routemodel/build.go index d675fb72f..ec5449e9a 100644 --- a/internal/nginx/routemodel/build.go +++ b/internal/nginx/routemodel/build.go @@ -217,6 +217,13 @@ func validateTarget(target string) error { return fmt.Errorf("target contains forbidden character %q", c) } } + // The generated conf is `set $up_x ; proxy_pass $up_x;`, and nginx + // expands every $name in a set value. A target like http://h/$uri would + // proxy to the request URI while the model recorded the literal "/$uri" + // (D-1139), so routes.json would disagree with the rendered conf. + if strings.Contains(target, "$") { + return fmt.Errorf("target contains '$': nginx would expand it as a variable, which the route model cannot represent (write %%24 for a literal dollar sign)") + } // nginx cannot use spaces, control or non-ASCII bytes in a proxy_pass // target, and the model cannot represent them. for i := 0; i < len(target); i++ { diff --git a/internal/nginx/routemodel/build_apidocs_test.go b/internal/nginx/routemodel/build_apidocs_test.go index 323fab4fc..7038d92f9 100644 --- a/internal/nginx/routemodel/build_apidocs_test.go +++ b/internal/nginx/routemodel/build_apidocs_test.go @@ -166,7 +166,8 @@ func TestMarshalOmitsNewFields(t *testing.T) { } // TestValidateTarget checks the targets nginx cannot use or the model cannot -// represent: fragments, user info, and any byte outside printable ASCII. +// represent: fragments, user info, nginx variables ($) and any byte outside +// printable ASCII. func TestValidateTarget(t *testing.T) { for _, tc := range []struct { target string @@ -185,6 +186,11 @@ func TestValidateTarget(t *testing.T) { {"http://h/café", "non-ASCII"}, {"http://h/\x7f", "non-ASCII"}, {"http://h;x", "forbidden character"}, + {"http://h/$uri", "variable"}, + {"http://h/p?a=$arg_b", "variable"}, + {"http://$host:8080", "variable"}, + {"http://h/${x}", "forbidden character"}, + {"http://h/cost%24", ""}, {"ftp://h", "scheme"}, {"http://", "host"}, } { From 8aeb6462b45028ee21a42d1e5c186156bbe73a2b Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:16:38 -0400 Subject: [PATCH 4/5] docs(test): correct why the registry golden still runs in a fresh process (D-0237) --- cmd/commands/registry_golden_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/commands/registry_golden_test.go b/cmd/commands/registry_golden_test.go index cc271e689..497b36444 100644 --- a/cmd/commands/registry_golden_test.go +++ b/cmd/commands/registry_golden_test.go @@ -33,12 +33,14 @@ const committedRegistry = "../../.github/command-registry.json" // compares the document without it. var generatedLine = regexp.MustCompile(`(?m)\A\{\n "_generated": "[^\n]*",\n`) -// goldenChild marks the child process that runs the golden check. Other tests -// in this package ResetFlags and redeclare flags on the real commands (for -// example env_target_test.go on `env target add`), so after them the live -// tree no longer matches what the shipped binary registers. The golden -// therefore runs the check in a fresh copy of this test binary, whose tree is -// exactly what init() registered, independent of test order. +// goldenChild marks the child process that runs the golden check. The env +// target tests no longer touch the shared tree (D-0237), but others still +// leave it changed: with TestCanonAccountResolution in the run, `admin` ends +// up in group "account" where the committed v1.5 file has "plugins". The +// golden therefore runs the check in a fresh copy of this test binary, whose +// tree is exactly what init() registered, independent of test order. Drop the +// wrapper only after that leak is fixed and the golden passes in-process in a +// full package run. const goldenChild = "NSELF_REGISTRY_GOLDEN_CHILD" // goldenSentinel is printed by the child only after the byte comparison From 0c28d73af89aabc56b816fd5b898a89a7bce8ed5 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 06:55:27 -0400 Subject: [PATCH 5/5] test(deploy): cancel the rolling-up test once web's up is hung, not after 300ms (D-0469) --- cmd/commands/deploy_rolling_up_test.go | 13 ++++++++++++- cmd/commands/deploy_strategies_test.go | 4 ++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/commands/deploy_rolling_up_test.go b/cmd/commands/deploy_rolling_up_test.go index 6bbb0134f..c42cfc557 100644 --- a/cmd/commands/deploy_rolling_up_test.go +++ b/cmd/commands/deploy_rolling_up_test.go @@ -69,7 +69,18 @@ func TestRollingUp(t *testing.T) { t.Run("a cancelled context prints the recovery line", func(t *testing.T) { s := stack(t, "uphang") ctx, cancel := context.WithCancel(context.Background()) - time.AfterFunc(300*time.Millisecond, cancel) + // Cancel once web's up is really hung, not after a fixed delay: on a loaded + // host 300ms can land while postgres is still being brought up. + go func() { + defer cancel() + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(filepath.Join(s.fix, "uphanging")); err == nil { + return + } + time.Sleep(10 * time.Millisecond) + } + }() _, err := runRollingRestart(ctx, s.dir, true) if !errors.Is(err, context.Canceled) || !strings.Contains(err.Error(), "nself restart web") { t.Fatalf("want an interrupt with the recovery line, got %v", err) diff --git a/cmd/commands/deploy_strategies_test.go b/cmd/commands/deploy_strategies_test.go index 8270b3c06..6d3e001d8 100644 --- a/cmd/commands/deploy_strategies_test.go +++ b/cmd/commands/deploy_strategies_test.go @@ -20,7 +20,7 @@ import ( // rollingFakeDocker answers the four docker compose calls the restart makes. // Fixtures live next to the log: services (config --services), config.json, -// ps..json, psfail (any `ps` exits 1), upfail (a service name whose `up` exits 1), uphang, up130 and startfixed +// ps..json, psfail (any `ps` exits 1), upfail (a service name whose `up` exits 1), uphang (which drops an uphanging marker once the hung up has started), up130 and startfixed // (a constant `docker inspect` StartedAt, i.e. the container never restarted). const rollingFakeDocker = `#!/bin/sh D="$FAKE_DOCKER_DIR" @@ -39,7 +39,7 @@ up) for last; do :; done if [ -f "$D/upfail" ] && [ "$(cat "$D/upfail")" = "$last" ]; then echo "boom" >&2; exit 1; fi # uphang names a service whose up never returns; up130 one whose up exits like an interrupted docker. - if [ -f "$D/uphang" ] && [ "$(cat "$D/uphang")" = "$last" ]; then exec sleep 60; fi + if [ -f "$D/uphang" ] && [ "$(cat "$D/uphang")" = "$last" ]; then : > "$D/uphanging"; exec sleep 60; fi if [ -f "$D/up130" ] && [ "$(cat "$D/up130")" = "$last" ]; then exit 130; fi echo "$last" >> "$D/ups" ;; ps)