diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md index 93caca04..6586bbd1 100644 --- a/.claude/commands/github-review-pr.md +++ b/.claude/commands/github-review-pr.md @@ -77,10 +77,10 @@ gh pr view --repo mhenrixon/kamal-proxy --json mergeable,mergeStateS ### Which branch do you merge? (fork-specific — decide BEFORE merging) -The branch model (`.claude/rules/git-workflow.md`) forbids merging `dash` into an upstream-PR-able feature branch — fork-only cert code would contaminate it. So: +`dash` is this fork's main branch and feature branches root off it, so the answer is normally simple: -1. **First try `git merge origin/main`** — the sanctioned forward merge. Most conflicts against `dash` come from upstream code that reached `dash` via `main`; merging `main` forward resolves them without contaminating the branch. Re-check mergeability against `dash` afterwards. -2. **Still conflicting?** The overlap is fork-only code on `dash` (typically the cert overlap zone: `internal/cmd/run.go`, `internal/server/config.go`, `internal/server/router.go`, `internal/server/service.go`). If the branch is fork-internal (never intended for a basecamp PR), merging `origin/dash` is acceptable. If the branch is meant to stay upstream-PR-able, **stop and ask the user** — merging `dash` would be irreversible contamination. +1. **Merge `origin/dash`** — this is the sanctioned forward merge. Branches are no longer kept upstream-PR-able, so there is nothing to contaminate. +2. **Only reach for `git merge origin/main`** on an old branch that still roots off `main`, or when you specifically want upstream fixes that have not yet reached `dash`. Re-check mergeability against `dash` afterwards. Note `git rerere` is enabled: previously-seen conflicts auto-replay their resolutions — review what rerere staged before trusting it. @@ -106,7 +106,7 @@ Note `git rerere` is enabled: previously-seen conflicts auto-replay their resolu - The PR reports `MERGEABLE` (or the local `git merge-tree` check is clean), AND the merge commit (if one was needed) is pushed. - If the merge produced changes, CI is now re-running — that's expected; Phase A reads the fresh run. -- If a conflict cannot be resolved with confidence (both sides rewrote the same logic and the correct combination isn't decidable from the code, or resolving requires merging `dash` into an upstream-PR-able branch), **stop and ask the user** — a guessed resolution that compiles is worse than a question. +- If a conflict cannot be resolved with confidence (both sides rewrote the same logic and the correct combination isn't decidable from the code, or the correct combination is genuinely ambiguous), **stop and ask the user** — a guessed resolution that compiles is worse than a question. --- diff --git a/.claude/commands/lfg.md b/.claude/commands/lfg.md index 8d9603c2..a5d1a458 100644 --- a/.claude/commands/lfg.md +++ b/.claude/commands/lfg.md @@ -16,7 +16,7 @@ Execute a complete engineering workflow with verification at each phase, respect 1. Check the current branch: `git branch --show-current` 2. If NOT on `main`, switch: `git checkout main` 3. Sync with upstream (do not assume `origin/main` is current): `git fetch upstream --tags --prune && git merge --ff-only upstream/main && git push origin main` -4. Create feature branch **off `main`** (keeps it upstream-PR-able): `git checkout -b feature/{description}` (or `fix/{description}`, `issue-{number}-{brief-description}`) +4. Create feature branch **off `dash`** (`dash` is this fork's main branch): `git checkout -b feature/{description}` (or `fix/{description}`, `issue-{number}-{brief-description}`) 5. The branch merges **forward** into `dash` at PR time — never rebase it once pushed. See `.claude/rules/git-workflow.md` and `.claude/rules/upstream-sync.md`. --- @@ -327,7 +327,7 @@ The tests prove the CODE is right; this phase keeps the USER's mental model righ - [ ] `make test` passes - [ ] `go vet ./...` clean - [ ] Backwards compatibility maintained (state files, RPC contract, `kamal-proxy` naming untouched) -- [ ] Branch rooted off `main`, PR opened against `dash` +- [ ] Branch rooted off `dash`, PR opened against `dash` - [ ] PR created with description - [ ] PR body ends with `## Deviations & judgment calls` (from implementation-notes.md, since deleted) - [ ] Comprehension close-out delivered (decisions + three merge-gate questions) diff --git a/.claude/commands/plan.md b/.claude/commands/plan.md index 0fed0378..2638d326 100644 --- a/.claude/commands/plan.md +++ b/.claude/commands/plan.md @@ -22,7 +22,8 @@ You are the planning specialist for **dash-proxy**, the Go fork of `basecamp/kam - **Read-only for source code.** Never edit `.go` files, never commit, never create branches. The only file you may Write is a new plan markdown under `docs/plans/`. - **Never reproduce secrets** (ACME account keys, DNS provider API tokens, ghcr credentials) in the plan, even redacted ones you encounter while reading config or state files. - **Dedupe before creating an issue**: `gh issue list --search "" --repo mhenrixon/kamal-proxy` — if an existing issue covers this, extend it in your summary instead of duplicating. -- **Respect the fork boundary.** `main` is a fast-forward-only mirror of upstream — never plan work that lands there. Fork-only work (cert batching, wildcard DNS-01, anything not upstreamable) targets a feature branch rooted off `main`, merging forward into `dash`. If the change is generically useful and upstream-clean, say so — it may be worth a PR to `basecamp/kamal-proxy` instead of a fork-only patch. +- **Respect the fork boundary.** `dash` is this fork's main branch; plan work onto a feature branch rooted off `dash`, merging back into `dash`. `main` is a fast-forward-only mirror of upstream — never plan work that lands there. Upstream mergeability is **not** a constraint: design what is best for `dash` and diverge from basecamp where that is better. +- **Check upstream before porting.** When an issue says "port basecamp/kamal-proxy#N", verify that PR is still open and unmerged before planning a port — several have been superseded or merged since the issues were written (#63→#225, #197→#228). Diff against `upstream/main` first. ## Phase 1 — Investigate @@ -87,7 +88,7 @@ Use this structure for the issue body or markdown file. Every section is load-be ## Execution -Implement on a branch rooted off `main` (or the relevant feature branch — `san-certificate-batching` / `wildcard-certs` — if this extends fork-only cert work), PR against `dash`. +Implement on a branch rooted off `dash` (or the relevant feature branch — `san-certificate-batching` / `wildcard-certs` — if this extends fork-only cert work), PR against `dash`. ``` For GitHub issues: create with `gh issue create --repo mhenrixon/kamal-proxy --title "..." --body-file `. Write the body to a temp file first; do not use inline heredoc with `--body` (code fences get mangled by shell interpolation). diff --git a/.claude/rules/git-workflow.md b/.claude/rules/git-workflow.md index 2a0493be..579928fe 100644 --- a/.claude/rules/git-workflow.md +++ b/.claude/rules/git-workflow.md @@ -29,13 +29,20 @@ This is a fork, not a normal repo — branch roles are fixed. Full sync mechanic | Branch | Role | Can you commit here? | |---|---|---| +| `dash` | **This fork's main branch** — every feature lands here | Only via merge from feature branches | | `main` | Fast-forward-only mirror of `basecamp/kamal-proxy` | **NEVER** | -| `dash` | Long-lived integration + release branch | Only via merge from feature branches | | `san-certificate-batching` | SAN cert batching feature branch | Yes | | `wildcard-certs` | DNS-01 wildcard certs feature branch | Yes | -| `feature/*`, `fix/*` | New work | Yes — root off `main` | +| `feature/*`, `fix/*` | New work | Yes — root off `dash` | -**Root new feature branches off `main`, not `dash`** — this keeps them upstream-PR-able (basecamp can merge them without inheriting fork-only cert code). They merge *forward* into `dash`, never the reverse, and published branches are never rebased. +**Root new feature branches off `dash`.** `dash` is this fork's main branch, and upstream +mergeability is not a constraint on our design — we build what is best for `dash` and diverge +from basecamp where that is better. (Same call as the `../kamal` fork.) Rooting off `main` +instead produces PRs that run no CI and conflict on every fork-only file, which is why we +stopped doing it. + +`main` still exists so upstream fixes can be merged *forward* into `dash`. It is a source, +never a target. Published branches are never rebased. ## Branch Naming diff --git a/CLAUDE.md b/CLAUDE.md index 6c5397cf..1e060aaa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,10 +53,16 @@ Layer 0: unix socket + state files (~/.config/kamal-proxy, kamal-proxy.sock) ## Branch map -| Branch | Contents | Conflict surface vs main | +**`dash` is this fork's main branch.** New work branches off `dash` and PRs back into `dash`. +Upstream mergeability is **not** a design constraint — we take what is best for `dash` and +diverge where that is better. `main` still exists as an upstream mirror so we can keep pulling +basecamp's fixes forward, but nothing is shaped for their benefit. (Decided alongside the same +call in the `../kamal` fork.) + +| Branch | Contents | Conflict surface vs dash | |---|---|---| -| `main` | basecamp mirror, ff-only | — | -| `dash` | integration + release: publish workflow + cert features merged | — | +| `dash` | **the main branch** — all fork work lands here; publish workflow + cert features merged | — | +| `main` | basecamp mirror, ff-only — a source to merge *from*, never a target | — | | `san-certificate-batching` | SAN cert batching (`internal/server/san_cert_manager.go`), `--acme-email`/`--acme-directory` | run.go, config.go, router.go, go.mod | | `wildcard-certs` | DNS-01 wildcard certs (`internal/server/acme/`, cert registry), `--acme-dns-provider` etc. | run.go, config.go, router.go, go.mod | | `feat/loadbalancing` | SUPERSEDED — upstream absorbed multi-target LB natively (`load_balancer.go`, reader/writer split); the branch only retains a standalone `TargetPool` module. Not merged into `dash`; candidate for deletion. | — | @@ -77,7 +83,7 @@ Tag push (`vX.Y.Z.N`) → `.github/workflows/docker-publish.yml` → multi-arch | Command | Purpose | |---------|---------| -| `/lfg` | Full autonomous workflow: branch off `main` → understand → plan → TDD → verify → PR into `dash` | +| `/lfg` | Full autonomous workflow: branch off `dash` → understand → plan → TDD → verify → PR into `dash` | | `/plan` | Read-only planning → GitHub issue or `docs/plans/` markdown (execute with `/lfg`) | | `/architect` | Coordinate work across the cmd → RPC → server layers | | `/tdd` | Enforce RED → GREEN → REFACTOR with Go table-driven tests | diff --git a/ROADMAP.md b/ROADMAP.md index eb86f3d1..18233d25 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -47,7 +47,7 @@ Proxy-side roadmap for the dash fork. The cross-repo release sequencing, strateg | Redirect/rewrite rules | #35; kamal discussions #1214/#97 | DONE — `--redirect '=[;status=]'` and `--rewrite '='` on deploy (`internal/server/redirect_rules.go`). Both pieces of evidence only ask for www→apex, which `--canonical-host` already answers; what shipped is the path-level gap the issue names. Patterns are RE2 anchored to the whole path with `$1` expansion. Redirects fold into `redirectURLIfNeeded` alongside the TLS/canonical hop, so a path move on a TLS service costs the client one redirect, not two; a rule resolving to the request's own URL is dropped so a catch-all cannot loop. A relative replacement is always rebuilt as `scheme://host/...`, which is what stops a captured `//evil.com` from becoming a scheme-relative Location. Rewrites apply last in `serviceRequestWithTarget`, after the health-check exemptions and the allow list, and skip `isInternalRequest` — the TLS on-demand probe runs the same chain, and a catch-all pointing it at the app's index would approve a certificate for any host | | Compression (gzip/zstd/brotli) | rejected #19 | DONE — `--compress`, `--compress-min-length`, `--compress-content-type` on deploy (`internal/server/compression.go`, `compression_middleware.go`), wrapped outermost in `Service.createMiddleware`. Per-service, not per-target, because that is the only chain that sees proxy-written responses too. Encoding is chosen by the client's q values, ties by the order given. The decision is held until the body's size and type are known, which means a flush *before* any body must not settle it — `ReverseProxy` schedules exactly that after the headers of every unknown-length response (`flushInterval` returns -1), and it races the first write. `text/event-stream` is excluded outright, so the streaming bypass (`response_buffer_middleware.go:86`) never sees a compressed stream. The built-in error pages render above the router (`server.go:338`) and stay uncompressed; `--error-pages` ones do not | | Liveness endpoint for external monitors | #25 | DONE — `GET /.kamal-proxy/ping` → `200` (`internal/server/ping_handler.go`), mounted outermost and **unconditionally** in `Server.buildHandler`. It reuses the `/.kamal-proxy/` namespace but not `DynamicDomainManager.WrapHandler`, which only mounts when dynamic domains are configured — the endpoint has to answer on a proxy with zero services, which is the whole point. Outermost is what keeps it out of the access log: the logging middleware never sees it, at the cost of no request ID and no error page on that path. No readiness variant: `RestoreLastSavedState` runs before `Start` opens a listener and `BeginDrain` closes the listeners, so readiness here could only ever be `200` — the TCP state is the real signal. Not mounted on the metrics port, which is opt-in and IP-restricted | -| Scale-to-zero | port PR #197 (open) | `PauseController` states (`pause_controller.go`) are the natural base | +| Scale-to-zero | port PR #228 (open; supersedes the closed #197 the issue names) | `PauseController` states (`pause_controller.go`) are the natural base. Upstream maintainer blessed the architecture in discussion #222: opt-in direct Docker socket behind a `ContainerLifecycle` interface, so a restricted host-side start/stop service can replace it later. Mounting `docker.sock` into the internet-facing proxy is root-equivalent on the host — opt-in only, and only when the feature is enabled | | Observability batch — log format selection, OTel traceparent, metrics path excludes | #213 counter-proposal | `logging_middleware.go:81` (fixed JSON today); `request_id_middleware.go` | ## Implementation notes (apply to every feature) diff --git a/docs/plans/2026-07-29-scale-to-zero.md b/docs/plans/2026-07-29-scale-to-zero.md new file mode 100644 index 00000000..45c911f5 --- /dev/null +++ b/docs/plans/2026-07-29-scale-to-zero.md @@ -0,0 +1,1173 @@ +# Scale-to-zero idle services (issue #19) — implementation plan + +Produced 2026-07-29 by a scout/design/judge workflow against dash @ cad460e. Ports basecamp/kamal-proxy#228 (which supersedes the closed #197 issue #19 names). + +# Scale-to-zero on `dash` — decided implementation plan + +**Provenance note (read first):** the working tree is on `feature/observability-batch`, not `feature/scale-to-zero`, and is dirty (`log_format.go`, `trace_context.go`, `implementation-notes.md` untracked; `logging_middleware.go` modified). HEAD is `cad460e`. Branch off a clean `dash` before starting. Line numbers below are from HEAD as read. + +Every divergence from both input plans is anchored to a line I opened. + +--- + +## Decision summary + +| # | Question | Decision | +|---|---|---| +| 1 | IdleController vs PauseController | **Separate controller.** PauseController is the wrong shape; its broadcast idiom is copied. | +| 2 | Wake-hold position | `serviceRequestWithTarget`, immediately after `handlePausedAndStoppedRequests` (service.go:768). | +| 3 | Container identity | Derive from `targetURL.Hostname()`; **`--sleep-container` overrides**; deploy-time preflight proves it. Derivation is *not* reliable — see §3. | +| 4 | What blocks sleep | One signal: `IdleController.inflight`, released by a `defer` that cannot run until the stream closes. | +| 5 | Health checks vs stopped container | Single-target checks are **already stopped**, so nothing is marked unhealthy *and the wake has no readiness signal*. Fixed by `SuspendForSleep`/`ResumeFromSleep` + a locked `waitForHealthyContext` read. | +| 6 | Flags | `deploy --sleep-after` / `--wake-timeout` / `--sleep-container`; `run --docker-socket` (default **empty**). | +| 7 | Persistence | `ServiceOptions.{SleepAfter,WakeTimeout,SleepContainers}` + one `idle_state` string. Old files decode to `active`, feature off. | + +--- + +## 1. Separate `IdleController`, not an extended `PauseController` + +Both input plans said separate. I agree, and the reasons are verifiable rather than stylistic. `PauseController.Wait()` (pause_controller.go:108) is structurally wrong in four ways that cannot be patched without changing pause semantics for every existing user: + +1. **No side effect.** `Wait()` is a pure observer — it never causes the state to change. Sleep→wake requires the *first* waiter to fire `StartContainer`. Bolting that in would fire a network call from inside `getWaitState`'s `RLock` (pause_controller.go:133-142). +2. **No context.** `Wait()` takes no `context.Context` and never selects on `r.Context().Done()`. A client that hangs up mid-wake would leave a goroutine parked for the full timeout. Adding a parameter changes every pause call site. +3. **One enum, two orthogonal axes.** Pause is admin-driven; sleep is traffic-driven. "Paused *and* sleeping" is a real state that a single `State` field cannot hold, and `Resume()` (pause_controller.go:100) would silently clobber a sleep. +4. **`time.After(p.FailAfter)` per call** inside the read lock (pause_controller.go:138) — one never-stopped timer per in-flight request. + +**What I do copy:** the close-only broadcast channel (nothing is ever *sent*; `close()` is the broadcast, and waiters re-read state after waking — pause_controller.go:119-124), and the "re-drive the state machine on restore rather than trusting the serialized field" idiom from `UnmarshalJSON` (pause_controller.go:51-68). + +**Consequence I am taking deliberately:** the two controllers compose by *position*, not by shared state. Because the idle gate sits below the pause gate (§2), a paused or stopped service's requests never reach `BeginRequest` at all. That is why I ship no wake-side interlock — see §10 for the one interlock I do ship, and why. + +--- + +## 2. Where the wake-hold sits — and why not one layer up or down + +**Exact position:** `internal/server/service.go`, inside `serviceRequestWithTarget`, between `handlePausedAndStoppedRequests` (line 768) and `r = s.rewriteRequest(r)` (line 773). + +```go + if s.handlePausedAndStoppedRequests(w, r) { + return + } + + // After every gate above, so that no blocked, throttled, unauthenticated or + // redirected request can spend a container start -- upstream had none of + // those gates, which is why its own placement one layer up was safe there + // and is a denial-of-wallet vector here. Before target selection, so the + // request body is still unread when the hold begins. + handled, endIdleRequest := s.handleIdleRequest(w, r) + if endIdleRequest != nil { + defer endIdleRequest() + } + if handled { + return + } + + // Last, so that everything above -- the health check exemptions, the + // redirects, the allow list -- still sees the path the client asked for. + r = s.rewriteRequest(r) +``` + +**Why not one layer up** (`Service.ServeHTTP`, service.go:397 — where #228 put it). Everything in this list is *below* `ServeHTTP` on dash and *above* the chosen point, and each one is a concrete regression at the higher position: + +| Gate | Line | What #228's placement costs | +|---|---|---| +| `WithRequestDeadlineMiddleware` | service.go:695 | Wake escapes `--request-timeout` / `--path-request-timeout`. The existing comment promises the deadline "covers pause waits and target selection". | +| `certManager.HTTPHandler` | service.go:727 | An **ACME HTTP-01 challenge starts a container**. Per-service `autocert` (TLS-on-demand) has no root-level escape. | +| `WithErrorPageMiddleware` | service.go:707 | Wake-failure 503 renders the *root* error page (server.go:388), not the service's `--error-pages`. | +| `WithCompressionMiddleware` | service.go:720 | Wake-failure body uncompressed, unlike every other 503. | +| `rejectDisallowedIP` | service.go:740 | A blocked IP spends a `docker start`. | +| `handleRedirectsIfNeeded` | service.go:749 | A 301 that never reaches the target wakes a container. | +| `rejectRateLimited` | service.go:756 | A flood spends one `docker start` per burst. | +| `rejectUnauthenticated` | service.go:764 | **An anonymous client spends money.** | + +**Why not one layer down** (`startLoadBalancerRequest`, service.go:781). It holds `s.serviceLock.RLock()` for its whole body: + +```go +func (s *Service) startLoadBalancerRequest(w http.ResponseWriter, r *http.Request) func() { + s.serviceLock.RLock() + defer s.serviceLock.RUnlock() + ... +} +``` + +A 3-second wake there blocks `UpdateLoadBalancer`'s write lock (service.go:359) for 3 seconds — i.e. **every deploy stalls behind every wake.** + +**Why not further down still** (inside `LoadBalancer.StartRequest` or `serveWithRetries`). This is the trap: for the single-target service that scale-to-zero actually targets, `updateHealthyTargets` calls `lb.all.StopHealthChecks()` the moment the target first goes healthy (load_balancer.go:309-315, verified). The stopped container therefore stays `TargetStateHealthy`, `claimTarget` **succeeds**, and the request goes straight to a connection-refused 502 via `handleProxyError`. The selection-failure retry path (`ErrorNoHealthyTargets`) never fires. dash's retry machinery cannot be the hold. + +**Body safety.** The point of no return is `target.go:249`, `t.handlerForRequest(req).ServeHTTP(tw, req)` — where either `RequestBufferMiddleware` drains the body or `ReverseProxy` streams it. Everything above reads headers and URL only; `rewriteRequest` shallow-copies and shares the same `Body` (redirect_rules.go:139). `net/http` does not read a request body until the handler asks. So a chunked POST parked for 3 seconds is handed on byte-for-byte unread. **No buffering, no `TeeReader`, no `--buffer-requests` requirement.** + +--- + +## 3. Container identity — the honest answer + +**The proxy cannot reliably derive the container reference. It can derive it in the two common topologies and not in the others.** Neither input plan addressed this fully; both guarded only IP literals. + +What the proxy has is `host[:port]`, and nothing else. `hostRegex` is `^(\w[-_.\w+]+)(:\d+)?$` (target.go:27); `parseTargetSpec` strips `;weight=N` first (target_weight.go:31). + +| Topology | `--target` carries | `POST /containers/{ref}/start` | +|---|---|---| +| **Kamal** | 12-char **container short ID** — `docker container ls --filter name=… --quiet` (kamal `commands/base.rb:17`), joined with the port in `configuration/proxy.rb:146` | ✅ Docker accepts an unambiguous ID prefix | +| **Compose, container name** (this repo's `example/README.md` uses `--target example-web-1`) | container name | ✅ | +| **Compose, service alias** (`--target web:3000`) | a **network alias**, not a container name | ❌ **404** | +| Custom `--network-alias` | alias | ❌ 404 | +| IP literal (`10.0.0.5:3000` — `hostRegex` accepts it) | address | ❌ 404 | +| External DNS name | hostname | ❌ 404 | + +Case 3 is a *normal* way to write a Compose target, and it silently 404s. So: + +**Derivation (default):** + +```go +// ContainerRef returns the container this target's address names: its hostname, +// which is a container short ID in a Kamal deployment and a container name in +// the Compose example. It is a best guess -- a Compose service alias or a +// custom network alias resolves over Docker's DNS but names no container -- so +// --sleep-container exists to state the reference explicitly, and the deploy +// preflight is what turns a wrong guess into an error on the operator's +// terminal instead of a 503 an hour later. +// +// An IP literal is rejected outright: hostRegex accepts "10.0.0.5:3000", and no +// container runtime could ever act on it. Bracketed IPv6 never reaches here -- +// hostRegex rejects it before a Target is built. +func (t *Target) ContainerRef() (string, bool) { + host := t.targetURL.Hostname() + if net.ParseIP(host) != nil { + return "", false + } + return host, true +} +``` + +**Override (`--sleep-container`, repeatable).** When set it replaces the derived set entirely, so the "which target maps to which container" question never arises: + +```go +// SleepContainers names the containers to stop and start for --sleep-after, +// replacing what the proxy would infer from the target addresses. Set it when a +// target names a network alias rather than a container -- "--target web:3000" +// under Compose resolves over Docker's DNS but is not a container name. +// +// Unlike the inferred set this does not follow a redeploy, so under Kamal -- +// where the target is a per-release container id -- leave it unset and let the +// proxy infer. +SleepContainers []string `json:"sleep_containers,omitempty"` +``` + +```go +// containerRefsLocked names the containers behind this service's write targets +// across both slots. Read targets are replicas whose lifecycle the proxy does +// not own, so they are never stopped. +// +// Callers hold the service write lock: initialize() runs unlocked from +// UpdateOptions, so collecting refs there -- as upstream does -- races a +// concurrent redeploy mutating s.active. +func (s *Service) containerRefsLocked(options ServiceOptions) []string { + if len(options.SleepContainers) > 0 { + return slices.Clone(options.SleepContainers) + } + + refs := []string{} + for _, lb := range []*LoadBalancer{s.active, s.rollout} { + if lb == nil { + continue + } + for _, target := range lb.WriteTargets() { + if ref, ok := target.ContainerRef(); ok { + refs = append(refs, ref) + } + } + } + return refs +} +``` + +**Preflight closes the guessing gap.** In `Router.DeployService`, after `options.Validate()`: + +```go +// validateSleepConfiguration proves, in one round trip at deploy time, that the +// socket is mounted, the daemon answers, the API version negotiates, and every +// reference names a container this daemon knows. Upstream discovered all of +// that at the first idle timeout instead, marked the service asleep regardless, +// and then 503'd every request for a container that was running fine. +func (r *Router) validateSleepConfiguration(options ServiceOptions, targetURLs []string, targetOptions TargetOptions) error { + if options.SleepAfter <= 0 { + return nil + } + + if r.lifecycle == nil { + return ErrNoContainerLifecycle + } + + refs := options.SleepContainers + if len(refs) == 0 { + targets, err := NewTargetList(targetURLs, nil, targetOptions) + if err != nil { + return err + } + + for _, target := range targets { + ref, ok := target.ContainerRef() + if !ok { + return fmt.Errorf("%w: target %s is an address, not a container; name the container with --sleep-container", + ErrNotAContainerRef, target.Address()) + } + refs = append(refs, ref) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), containerPreflightTimeout) + defer cancel() + + for _, ref := range refs { + switch err := r.lifecycle.ContainerExists(ctx, ref); { + case err == nil: + + case errors.Is(err, ErrContainerNotFound): + return fmt.Errorf("%w: no container named %q; if the target names a network alias rather than a container, set --sleep-container", + ErrNotAContainerRef, ref) + + case errors.Is(err, ErrContainerInspectForbidden): + // A hardened socket proxy commonly allows POST .../start and + // .../stop while denying inspect. Refusing the deploy for that + // would lock out exactly the operators doing the right thing, so + // this is the one preflight failure that is a warning. + slog.Warn("Cannot verify container for --sleep-after; the socket denies inspect", + "container", ref, "error", err) + + default: + return fmt.Errorf("cannot manage container %q for --sleep-after: %w", ref, err) + } + } + + return nil +} +``` + +--- + +## 4. What prevents sleeping — one signal, three cases + +**The signal is `IdleController.inflight`.** There is exactly one, and it covers all three cases because of *where* its release is deferred. + +- **Incremented:** `IdleController.admit()`, under `c.lock`, when the state is `Active`. +- **Decremented:** `IdleController.EndRequest()`, via the `defer endIdleRequest()` in `serviceRequestWithTarget` (§2). +- **Read:** `IdleController.trySleep()`, under `c.lock`, as `c.inflight != 0`. + +| Case | Concrete mechanism | +|---|---| +| **Ordinary request** | `defer endIdleRequest()` runs when `serviceRequestWithTarget` returns — after `sendRequest()` (service.go:775-777). | +| **WebSocket** | `sendRequest()` → `Target.SendRequest` (target.go:240) → `ReverseProxy` handles the upgrade and **does not return until the hijacked connection closes**. `targetResponseWriter.Hijack` (target.go:649) flags it but does not release anything. So `endIdleRequest` cannot run for the life of the socket. | +| **SSE** | Identical: `ReverseProxy` streams `text/event-stream` with the stdlib's automatic `FlushInterval: -1` (no explicit interval set, target.go:371) and returns only on body close. | + +This is the same guarantee `Target.inflight` already gives `Drain` (target.go:252), reached without touching `Target`'s map or its lock. **No special-casing of WebSocket or SSE anywhere in this feature.** + +Deliberately *not* counted as activity — otherwise a 1 Hz uptime monitor pins a service awake forever: + +```go +if isInternalRequest(r) || s.targetOptions.IsHealthCheckRequest(r) { ... } +``` + +matching the exemption `rejectDisallowedIP`, `rejectRateLimited` and `rejectUnauthenticated` already use. `isInternalRequest` (service.go:73) catches the proxy's own TLS-on-demand probes. + +--- + +## 5. Health checks against a stopped container + +**Answer: it depends on target count, and the single-target case — the one scale-to-zero actually targets — is worse than "marked unhealthy".** + +| Shape | What happens when the container stops | Effect on wake | +|---|---|---| +| **Single target, default** | `updateHealthyTargets` called `lb.all.StopHealthChecks()` at first-healthy (load_balancer.go:309-315). **No probe runs.** Target stays `TargetStateHealthy`, stays in `lb.writers`. Requests dial a dead socket → `handleProxyError` → **502**, forever. | **Breaks wake, silently.** `markHealthy()` already fired, so `waitForHealthyContext` is already cancelled and `WaitUntilHealthy` returns `nil` **instantly** — the wake would report ready against a container that has not booted. | +| **Multi-target** | Probes run forever. First failure demotes `Healthy → Unhealthy` (target.go:330-334), target leaves the pool, `nextTarget` returns nil → **503**. Probes keep hammering a deliberately-stopped container every second. | Wake works, but noisy: `"Target health updated … unhealthy"` per interval per target for the whole sleep. | +| **Restored under `--recheck-targets-on-restore`** | `persistentHealthChecks` is pinned (load_balancer.go:180), so the single-target auto-stop never fires — behaves like multi-target. | Same noise, indefinitely. | + +**The fix — two `LoadBalancer` methods and one lock correction.** + +```go +// SuspendForSleep empties the pool and stops probing, so a container that is +// deliberately stopped is neither routed to nor dialled once a second. It is +// called before the containers go down: the idle controller holds arriving +// requests in Stopping while this runs, so an empty pool is never observable. +func (lb *LoadBalancer) SuspendForSleep() { + lb.all.StopHealthChecks() + + for _, target := range lb.all { + target.updateState(TargetStateUnhealthy) + } + + lb.lock.Lock() + defer lb.lock.Unlock() + + lb.writers = TargetList{} + lb.readers = TargetList{} +} + +// ResumeFromSleep puts the pool back in the state a fresh deployment starts in +// -- unverified, health-checked -- and re-arms WaitUntilHealthy so a caller can +// wait for the woken containers to actually answer. +// +// Re-arming is the whole point. A single-target pool stops probing at +// first-healthy and MarkAllHealthy assumes health on restore, so without this +// waitForHealthyContext is already cancelled and WaitUntilHealthy returns nil +// against a container that has not started. +// +// The previous context is replaced, not cancelled: WaitUntilHealthy reports any +// non-deadline cancellation as success, so cancelling would tell a concurrent +// waiter "healthy" at the exact moment every target was marked unverified. +func (lb *LoadBalancer) ResumeFromSleep() { + lb.lock.Lock() + lb.waitForHealthyContext, lb.markHealthy = context.WithCancel(context.Background()) + lb.lock.Unlock() + + for _, target := range lb.all { + target.updateState(TargetStateAdding) + } + + // RestartHealthChecks, not BeginHealthChecks: the latter assigns + // t.stateConsumer outside the inflight lock, which is only safe before a + // target serves anything. NewHealthCheck runs one immediate probe before it + // starts ticking, so readiness costs a round trip, not a check interval. + lb.all.RestartHealthChecks() +} +``` + +`TargetStateAdding` is the correct resume state: `HealthCheckCompleted(true)` promotes `Adding → Healthy` (target.go:317-322), and `HealthCheckCompleted(false)` only ever demotes `Healthy → Unhealthy` (target.go:330-334) — so a container that never comes up stays out of the pool rather than flapping. + +Readiness then arrives through machinery that already exists: `HealthCheckCompleted(true)` → `TargetStateChanged` → `updateHealthyTargets` → `healthyCount == len(lb.all)` → `markHealthy()` → the wake's `WaitUntilHealthy` returns. And the single-target `StopHealthChecks()` fires again, so a woken service returns to zero background probing. **A woken container is held to exactly the standard a freshly deployed one is** (`createLoadBalancer`, router.go:550). + +**Required companion fix** — `waitForHealthyContext` is now written at runtime, so the read at load_balancer.go:153 must take the lock or it is a data race that `-race` will catch: + +```go +func (lb *LoadBalancer) WaitUntilHealthy(timeout time.Duration) error { + lb.lock.Lock() + parent := lb.waitForHealthyContext + lb.lock.Unlock() + + ctx, cancel := context.WithTimeout(parent, timeout) + defer cancel() + + <-ctx.Done() + + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("%w (%s)", ErrorTargetFailedToBecomeHealthy, timeout) + } + + return nil +} +``` + +And in `target.go`, splitting `BeginHealthChecks` without touching the unsynchronized `stateConsumer` write: + +```go +func (t *Target) BeginHealthChecks(stateConsumer TargetStateConsumer) { + t.stateConsumer = stateConsumer + t.RestartHealthChecks() +} + +// RestartHealthChecks re-arms checking on a target whose checks were stopped, +// leaving the state consumer alone -- unlike BeginHealthChecks, which assigns it +// outside the inflight lock and is therefore only safe before a target serves. +func (t *Target) RestartHealthChecks() { + t.withInflightLock(func() { + if t.healthcheck != nil { + t.healthcheck.Close() + } + + t.healthcheck = NewHealthCheck( + t, + t.buildHealthCheckURL(), + t.options.HealthCheckConfig.Interval, + t.options.HealthCheckConfig.Timeout, + t.options.HealthCheckConfig.Host, + ) + }) +} +``` + +**`health_check.go` needs no change at all.** dash's `BeginHealthChecks` (target.go:284-301) already closes the previous healthcheck inside `withInflightLock`, which is the only thing #228's `newHealthCheck`/`Start()`/`sync.Once` split bought. + +One more guard, in `Service.RecheckTargetHealth` (service.go:577), so `--recheck-targets-on-restore` does not probe a sleeping service: + +```go + if s.idleController != nil && s.idleController.State() != IdleStateActive { + return + } +``` + +--- + +## 6. Flags + +### `kamal-proxy run` + +| Flag | Env | Default | Meaning | +|---|---|---|---| +| `--docker-socket` | `KAMAL_PROXY_DOCKER_SOCKET`, `DOCKER_SOCKET` | **`""` (disabled)** | Docker socket path. Validated in `preRun` to exist and be a socket. | + +```go +runCommand.cmd.Flags().StringVar(&globalConfig.DockerSocketPath, "docker-socket", getEnvString("DOCKER_SOCKET", ""), + "Path to the Docker socket, enabling --sleep-after for deployed services (default empty, disabled). Mounting this socket into an internet-facing proxy grants it root-equivalent control of the host") +``` + +Empty, not `/var/run/docker.sock`. #228's flag was a no-op against its own default while the client was constructed unconditionally — "opt-in" in name only. `ROADMAP.md:49` already commits to opt-in. + +`preRun` (which exists since `--min-tls`, run.go:68): + +```go + // Checked at boot, with the path in the message, rather than at the first + // idle timeout an hour later. + if globalConfig.DockerSocketPath != "" { + info, err := os.Stat(globalConfig.DockerSocketPath) + if err != nil { + return fmt.Errorf("docker-socket %q is not reachable: %w", globalConfig.DockerSocketPath, err) + } + if info.Mode()&os.ModeSocket == 0 { + return fmt.Errorf("docker-socket %q is not a socket", globalConfig.DockerSocketPath) + } + } +``` + +Wired with a **setter, not a constructor argument** — `NewRouter(statePath)` has 43 call sites, and `SetSANCertManager` / `SetDynamicDomainManager` / `SetCertificateRegistry` are the established idiom. Placed after `RestoreLastSavedState` so it reconciles restored services: + +```go + if globalConfig.DockerSocketPath != "" { + router.SetContainerLifecycle(server.NewDockerClient(globalConfig.DockerSocketPath)) + } +``` + +### `kamal-proxy deploy` + +| Flag | Default | Meaning | +|---|---|---| +| `--sleep-after` | `0` (never) | Stop this service's containers after this long without traffic. | +| `--wake-timeout` | `30s` | Max hold while they start and pass a health check. | +| `--sleep-container` | none (infer) | Container to stop/start, repeatable; replaces inference. | + +```go +deployCommand.cmd.Flags().DurationVar(&deployCommand.args.ServiceOptions.SleepAfter, "sleep-after", 0, + "Stop this service's target containers after this long with no traffic, and start them again on the next request (default 0, never). Requires the proxy to run with --docker-socket. Health checks and the proxy's own TLS probes are not traffic and never wake a sleeping service") +deployCommand.cmd.Flags().DurationVar(&deployCommand.args.ServiceOptions.WakeTimeout, "wake-timeout", server.DefaultWakeTimeout, + "Maximum time a request waits for a sleeping service's containers to start and pass a health check before failing with 503") +deployCommand.cmd.Flags().StringArrayVar(&deployCommand.args.ServiceOptions.SleepContainers, "sleep-container", nil, + "Container to stop and start for --sleep-after, replacing what the proxy infers from the target address. Needed when a target names a network alias rather than a container (may be specified multiple times)") +``` + +**Not `--idle-timeout`.** Verified collision: `run --idle-timeout` already exists for HTTP keep-alive (run.go:53 → `Config.IdleTimeout`, config.go:71, `DefaultIdleTimeout = 60s` at config.go:24). Two flags with one name and two unrelated meanings is an operator trap no compiler catches. `--sleep-after` also puts the flag, the persisted state name (`sleeping`), the `list` column and the log lines in one vocabulary. + +The cobra default is `DefaultWakeTimeout` so `--help` is honest, **and** `Normalize()` maps `0 → DefaultWakeTimeout` so restored state files and direct RPC callers get the same value: + +```go +func (so *ServiceOptions) Normalize() { + so.Hosts = NormalizeHosts(so.Hosts) + so.PathPrefixes = NormalizePathPrefixes(so.PathPrefixes) + so.Compression.Normalize() + + if so.SleepAfter > 0 && so.WakeTimeout <= 0 { + so.WakeTimeout = DefaultWakeTimeout + } +} +``` + +Validation joins the existing chain in `Validate()` (service.go:228-254), in `service_idle.go`: + +```go +func (so ServiceOptions) validateSleep() error { + if so.SleepAfter < 0 || so.WakeTimeout < 0 { + return fmt.Errorf("%w: sleep-after and wake-timeout cannot be negative", ErrServiceOptionsInvalid) + } + + if so.SleepAfter > 0 && so.TLSOnDemandURL != "" { + // An on-demand check asks the backend, at handshake time, whether a host + // may have a certificate. A sleeping backend cannot answer, and waking + // one would let any SNI on the internet start a container. + return fmt.Errorf("%w: sleep-after cannot be used with a TLS on-demand URL", ErrServiceOptionsInvalid) + } + + if so.SleepAfter <= 0 && len(so.SleepContainers) > 0 { + return fmt.Errorf("%w: sleep-container requires sleep-after", ErrServiceOptionsInvalid) + } + + return nil +} +``` + +### `kamal-proxy list` + +Compose into the existing `State` column. `ServiceDescription` is gob-encoded and router.go:70-72 mandates append-only; a new *value* in an existing string field is not a schema change, and a CLI built before this prints it verbatim. + +```go +// describeServiceState reports the one state an operator most needs to see. A +// paused or stopped service says so -- that is a human decision and it outranks +// anything traffic-driven -- and otherwise a sleeping or waking service says +// that rather than "running". +func describeServiceState(service *Service) string { + if pause := service.pauseController.GetState(); pause != PauseStateRunning { + return pause.String() + } + + if service.idleController != nil { + if idle := service.idleController.State(); idle != IdleStateActive { + return idle.String() + } + } + + return PauseStateRunning.String() +} +``` + +Column now reads `running | paused | stopped | sleeping | waking | stopping`. + +--- + +## 7. Persistence + +**Added to the state file — two places, nothing else.** + +1. **`ServiceOptions`** (persisted whole as `marshalledService.Options`, service.go:410), all `omitempty`: + +```go + SleepAfter time.Duration `json:"sleep_after,omitempty"` + WakeTimeout time.Duration `json:"wake_timeout,omitempty"` + SleepContainers []string `json:"sleep_containers,omitempty"` +``` + +2. **`marshalledService`** — one string, not #228's whole `IdleController` JSON codec: + +```go + // IdleState is written as a name rather than an enum's number: the state + // file outlives proxy versions and operators read it. Absent -- every state + // file written before scale-to-zero existed -- parses to active, which is + // also what SleepAfter == 0 produces, so the feature restores off. + IdleState string `json:"idle_state,omitempty"` +``` + +`MarshalJSON` writes `s.idleStateName()`; `UnmarshalJSON` sets `s.restoredIdleState = parseIdleState(ms.IdleState)` and **creates no controller**. `parseIdleState` folds `stopping` and `waking` down to `sleeping` on both write and read: a proxy that died mid-transition does not know whether the container moved, and waking from `sleeping` is safe precisely because `docker start` on a running container answers `304`, which the client treats as success. + +**A pre-existing state file decodes to:** no `sleep_after` → `SleepAfter == 0`; no `idle_state` → `""` → `IdleStateActive`; `configureIdleController` sees `SleepAfter <= 0` → `s.idleController` stays nil → `handleIdleRequest` returns immediately → **byte-for-byte today's behavior, and the re-marshalled file is byte-identical too** (every new key is `omitempty`). + +**Downgrade** (new file, older binary): `encoding/json` drops unknown fields, so the service restores with no idle awareness while its containers may be stopped and `MarkAllHealthy()` (service.go:481) claims otherwise → 502s until someone runs `docker start`. Documented limitation, not fixable from this side. + +**Why `UnmarshalJSON` must not build the controller.** It ends in `s.initialize(...)` (service.go:493) — at which point `s.lifecycle` is still nil, because the router can only inject it after decoding. #228 built the controller there and started its timer goroutine, so a persisted `Active` state with a small idle timeout could reach `StopContainer` on a **nil interface**. Here the controller is built by `SetContainerLifecycle`, which the router calls per service after restore: + +```go +// SetContainerLifecycle installs the runtime that starts and stops this +// service's containers and builds the idle controller around it. Restored +// services get theirs after the state file is decoded, which is exactly why +// UnmarshalJSON never creates one. +func (s *Service) SetContainerLifecycle(lifecycle ContainerLifecycle) { + s.lifecycle = lifecycle + s.configureIdleController(s.options) + + if s.idleController == nil || s.idleController.State() != IdleStateSleeping { + return + } + + // Restore assumed every target healthy (MarkAllHealthy, service.go:481). + // For a sleeping service that is a healthy pool pointing at a stopped + // container -- and under --recheck-targets-on-restore, a probe against it + // every second. Put it back the way sleeping left it. + s.suspendForSleep() +} +``` + +**When state is written.** `IdleController` calls `persist` on the `Active↔Sleeping` edges only — two `saveStateSnapshot` calls per full cycle — **synchronously from the controller's own goroutine**, never `go fn()` and never from a request goroutine. Both transitions already run off the request path (`trySleep` on the timer goroutine, `finishWake` on the wake goroutine), so no detachment is needed. `saveStateSnapshot` (router.go:610) marshals to `[]byte` and writes under `saveLock` via `writeFileAtomic` — **#228's file-leak fix and `stateLock` hunk are already superseded on dash; drop them.** + +**Companion fix required.** `saveStateSnapshot` marshals under `r.withReadLock`, but `Service.MarshalJSON` (service.go:425) reads `s.active` / `s.rollout` with **no `serviceLock`**. That is a pre-existing race, and persisting from a sleep/wake makes it far more reachable. Three-line fix, same lock order as everywhere else (`routerLock → serviceLock`, so no inversion): + +```go +func (s *Service) MarshalJSON() ([]byte, error) { + s.serviceLock.RLock() + defer s.serviceLock.RUnlock() + ... +} +``` + +--- + +## 8. New files + +`service.go` is **863 lines**, already over the 800-line ceiling in `.claude/rules/coding-style.md`. `router_test.go` is over it too. Nothing substantial goes in either. + +| File | Contents | ~lines | +|---|---|---| +| `internal/server/container_lifecycle.go` | `ContainerLifecycle` interface + sentinel errors — the swap seam | 45 | +| `internal/server/docker_client.go` | Socket-only Docker client, zero external deps | 195 | +| `internal/server/idle_controller.go` | State machine, coalescing, timer, backoff | 300 | +| `internal/server/service_idle.go` | The request gate, ref collection, suspend/resume, validation | 175 | + +Tests: `idle_controller_test.go`, `docker_client_test.go`, `service_idle_test.go`, `router_idle_test.go`. Load-balancer tests go into the existing `load_balancer_test.go` (285 lines, room to grow). + +Edits to existing files are surgical: `service.go` (+3 option fields, +4 struct fields, the gate call, `Dispose`, `UpdateLoadBalancer`, marshal/unmarshal, `RecheckTargetHealth` guard, `MarshalJSON` lock), `router.go` (`lifecycle` field, `SetContainerLifecycle`, preflight call, restore pass, `createOrUpdateService`, `describeServiceState`), `load_balancer.go` (2 methods + the `WaitUntilHealthy` lock), `target.go` (`ContainerRef`, `RestartHealthChecks`), `config.go`, `run.go`, `deploy.go`. + +--- + +## 9. `internal/server/idle_controller.go` — the state machine + +```go +// IdleState is where a service's containers are in the scale-to-zero cycle. +type IdleState int + +const ( + IdleStateActive IdleState = iota + IdleStateStopping + IdleStateSleeping + IdleStateWaking +) + +var ( + ErrWakeTimeout = errors.New("timed out waking containers") + ErrWakeFailed = errors.New("failed to wake containers") + ErrNoContainerLifecycle = errors.New("scale-to-zero requires the proxy to run with --docker-socket") + ErrNotAContainerRef = errors.New("target does not name a container") +) + +type IdleController struct { + name string + lifecycle ContainerLifecycle + + // suspend takes the targets out of the pool and stops probing them; resume + // puts them back and waits for them to answer. Supplied by the Service, so + // the controller never reaches into a load balancer or its locks. + suspend func() + resume func(timeout time.Duration) error + persist func() + + lock sync.Mutex + state IdleState + generation uint64 + refs []string + inflight int + lastRequest time.Time + sleepAfter time.Duration + wakeTimeout time.Duration + lastErr error + failures int + retryAfter time.Time + disabled bool + cancel context.CancelFunc + + // changed is closed on every transition and then replaced. A waiter parks on + // the channel it read under the lock and re-reads the state when it wakes, + // so no decision is ever made from a stale snapshot. Nothing is ever sent on + // it -- the close is the broadcast, exactly as in PauseController. + changed chan struct{} + + signalled chan struct{} // buffered 1; nudges the timer goroutine + closed chan struct{} + closeOnce sync.Once +} + +func (c *IdleController) setStateLocked(state IdleState) { + if c.state == state { + return + } + + c.state = state + c.generation++ + + close(c.changed) + c.changed = make(chan struct{}) +} +``` + +`generation` replaces #228's per-wake `wakeDone` token and is strictly more general: **any** transition invalidates **any** in-flight lifecycle goroutine, and waiters never read a cached error because they re-derive under the lock on the next loop iteration. + +### Transitions + +| From | To | Trigger | +|---|---|---| +| Active | Stopping | timer fires, guard passes under the lock | +| Stopping | Sleeping | every `StopContainer` returned nil | +| Stopping | **Active** | any `StopContainer` errored — rollback, see below | +| Sleeping | Waking | `admit()` sees `Sleeping` past the backoff window | +| Waking | Active | every `StartContainer` nil **and** `resume()` nil | +| Waking | Sleeping | any start error, readiness failure, or deadline | +| any | Active | `Reset(refs)` — new active load balancer installed | +| Stopping/Waking | Sleeping | serialization, both directions | + +### The hold and the coalescing + +```go +// BeginRequest admits a request, waking the service first if it is asleep. It +// blocks the calling request goroutine and never touches the request, which is +// why the gate sits above target selection: the *http.Request handed on +// afterwards is byte-for-byte the one that arrived, body unread. +func (c *IdleController) BeginRequest(ctx context.Context) error { + // One deadline for the whole call. Upstream allocated a fresh timer on every + // loop iteration, so a request arriving while the service was still stopping + // could wait the full wake timeout for the stop and the full wake timeout + // again for the start -- twice the documented bound. + deadline := time.Now().Add(c.WakeTimeout()) + + for { + changed, err := c.admit() + if changed == nil { + return err + } + + remaining := time.Until(deadline) + if remaining <= 0 { + return ErrWakeTimeout + } + + timer := time.NewTimer(remaining) + select { + case <-changed: + timer.Stop() + case <-timer.C: + return ErrWakeTimeout + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-c.closed: + timer.Stop() + return ErrWakeFailed + } + } +} + +// admit either takes a slot for the request, or hands back the channel to wait +// on for the next transition. A nil channel means the caller is done: admitted +// when err is nil, rejected otherwise. +func (c *IdleController) admit() (<-chan struct{}, error) { + c.lock.Lock() + defer c.lock.Unlock() + + switch c.state { + case IdleStateActive: + c.inflight++ + c.lastRequest = time.Now() + return nil, nil + + case IdleStateSleeping: + if time.Now().Before(c.retryAfter) { + // A wake failed recently. Fail now rather than hold for the full + // timeout and issue another doomed docker start: at request rate + // that is an unbounded retry storm against the daemon. + return nil, c.lastErr + } + c.startWakeLocked() + } + + return c.changed, nil +} + +func (c *IdleController) EndRequest() { + c.lock.Lock() + if c.inflight > 0 { + c.inflight-- + } + // Stamped on the way out as well as in, so an hour-long SSE stream counts as + // activity for that hour rather than for the instant it started. + c.lastRequest = time.Now() + c.lock.Unlock() + + c.signal() +} +``` + +**Coalescing is a consequence of the mutex, not of extra machinery.** `startWakeLocked` is reachable only from the `Sleeping` arm, and its first statement flips the state to `Waking` while still holding `c.lock`. Concurrent callers serialize on that mutex: the first starts the wake, every later one falls through to `return c.changed, nil` and parks on one channel. **Twenty concurrent requests, exactly one `StartContainer`.** + +```go +func (c *IdleController) startWakeLocked() { + c.setStateLocked(IdleStateWaking) + + generation := c.generation + refs := slices.Clone(c.refs) + deadline := time.Now().Add(c.wakeTimeout) + lifecycle, resume := c.lifecycle, c.resume + + ctx, cancel := context.WithDeadline(context.Background(), deadline) + c.cancel = cancel + + slog.Info("Waking service", "service", c.name, "containers", refs) + + go func() { + defer cancel() + + var err error + for _, ref := range refs { + // Starting an already-running container answers 304, which the + // client treats as success. That is what lets a proxy whose state + // file said "sleeping" for a container that never stopped heal + // itself on the very next request. + if startErr := lifecycle.StartContainer(ctx, ref); startErr != nil { + err = fmt.Errorf("%w: container %s: %w", ErrWakeFailed, ref, startErr) + break + } + } + + if err == nil && resume != nil { + // Started is not ready. Wait on whatever budget the starts left, + // not a fresh one. + if remaining := time.Until(deadline); remaining <= 0 { + err = ErrWakeTimeout + } else { + err = resume(remaining) + } + } + + c.finishWake(generation, err) + }() +} + +func (c *IdleController) finishWake(generation uint64, err error) { + c.lock.Lock() + + // A deploy or a close landed mid-wake and has already decided what the state + // should be. Do not overwrite it with the outcome of a wake it superseded. + if c.generation != generation { + c.lock.Unlock() + return + } + + c.cancel = nil + suspend, persist := c.suspend, c.persist + + if err == nil { + c.lastErr, c.failures, c.retryAfter = nil, 0, time.Time{} + c.lastRequest = time.Now() + c.setStateLocked(IdleStateActive) + } else { + c.lastErr = err + c.failures++ + c.retryAfter = time.Now().Add(wakeBackoff(c.failures)) + c.setStateLocked(IdleStateSleeping) + } + c.lock.Unlock() + + if err != nil { + slog.Error("Failed to wake service", "service", c.name, "error", err) + // Back out of the pool, so a container that came up but never answered + // is not probed once a second until somebody notices. + if suspend != nil { + suspend() + } + return + } + + slog.Info("Service awake", "service", c.name) + if persist != nil { + persist() + } +} + +// wakeBackoff spaces out attempts after a failed wake. Without it a service +// whose container reference no longer resolves costs one docker start per +// inbound request, forever. Same shape as the dynamic-domain quarantine. +func wakeBackoff(failures int) time.Duration { + return min(time.Second< 0 && len(c.refs) > 0 + c.lock.Unlock() + + if !eligible { + wait = time.Hour + } + wait = max(wait, 0) + + timer := time.NewTimer(wait) + select { + case <-timer.C: + c.trySleep() + case <-c.signalled: + timer.Stop() + case <-c.closed: + timer.Stop() + return + } + } +} +``` + +Go 1.26 — `timer.Stop()` alone is sufficient, no drain. + +--- + +## 10. Behavior matrix + +| Event | Result | +|---|---| +| **deploy while sleeping** | `createLoadBalancer` health-checks the **new** containers (router.go:550), `UpdateLoadBalancer` calls `Reset(refs)` under the service write lock → Active, backoff cleared. Held waiters are released and **loop back into `admit()`**, see Active, proceed. They are never told "the wake succeeded" — #228's `Reset` released them with a nil error, which is a lie under `deploy --force`, where nothing was health-checked. | +| **pause / stop while sleeping** | Requests never reach the gate: `handlePausedAndStoppedRequests` (service.go:768) runs first. So **no wake-side interlock is needed.** I *do* ship `Disable()`/`Enable()`, and it suppresses **sleeping only** — pause is a short window during a deploy, and letting the timer stop containers inside it turns a 2-second pause into a cold start on resume. `Enable()` resets `lastRequest` so a resumed service gets a full idle period. See §12 — this is a judgment call. | +| **wake timeout** | 503 through the service's own `--error-pages`, `Retry-After: 1`, backoff armed, containers left **running** so an operator has something to `docker logs`. | +| **Docker failure on start** | Same 503. `suspend()` re-runs so nothing probes a half-started container. Raw daemon error **logged, never rendered** (§11). | +| **Docker failure on stop** | Stays **Active**, `resume()` rolls the pool back, next attempt a full `--sleep-after` away. The §3 preflight makes this rare. | +| **health check never passes after wake** | `resume` → `WaitUntilHealthy` → `ErrorTargetFailedToBecomeHealthy` → wake fails → 503 + backoff. | +| **proxy restart mid-wake or mid-stop** | State serializes as `sleeping`. Next request issues `docker start` → `304` for whatever never stopped → `WaitUntilHealthy` passes on the first probe → Active. Self-healing. | +| **client hangs up mid-wake** | `BeginRequest` returns `ctx.Err()`; handler returns silently, no error page. The wake runs on a **detached** deadline context and completes for everyone else. | +| **two proxy generations during `kamal proxy reboot`** | Both may hold the service Active; the loser can `docker stop` a container the winner is serving. Inherent to two proxies against one daemon; the same window already exists for `--recheck-targets-on-restore`. Mitigated by `--drain-timeout` ordering, not by this code. | + +--- + +## 11. Request gate — `service_idle.go` + +```go +// handleIdleRequest holds the request until this service's containers are back. +// It returns whether the client has already been answered, and the function that +// releases the request's hold on the idle timer. +func (s *Service) handleIdleRequest(w http.ResponseWriter, r *http.Request) (bool, func()) { + controller := s.idleController + if controller == nil { + return false, nil + } + + // A TLS on-demand probe is synthesized inside the proxy and must never start + // a container. validateSleep refuses --sleep-after with an on-demand URL, so + // this only guards a state file written before that rule existed. + if isInternalRequest(r) { + return false, nil + } + + // A health check must never wake a service -- an uptime monitor polling /up + // would pin it awake forever -- and must be answered rather than held, or a + // downstream load balancer evicts a service that is sleeping correctly. + if s.targetOptions.IsHealthCheckRequest(r) { + return s.answerIdleHealthCheck(w, r, controller), nil + } + + if err := controller.BeginRequest(r.Context()); err != nil { + if errors.Is(err, context.Canceled) { + // The client hung up mid-wake. Nobody left to answer. + return true, nil + } + + // Logged, not rendered. The underlying error carries container + // references and up to four kilobytes of daemon output, and this + // response is reachable by anyone who can open a connection. + slog.Error("Rejecting request: service did not wake", + "service", s.name, "path", r.URL.Path, "error", err) + w.Header().Set("Retry-After", "1") + SetErrorResponse(w, r, http.StatusServiceUnavailable, nil) + return true, nil + } + + return false, controller.EndRequest +} + +// answerIdleHealthCheck answers for a sleeping service without waking it. It +// reports healthy while the sleep is working as intended and stops the moment a +// wake has actually failed -- upstream returned 200 unconditionally, so a +// service that could no longer start reported green to its monitoring forever +// while 503ing every real request. +func (s *Service) answerIdleHealthCheck(w http.ResponseWriter, r *http.Request, controller *IdleController) bool { + if controller.State() == IdleStateActive { + return false + } + + if err := controller.LastWakeError(); err != nil { + slog.Warn("Reporting unhealthy: last wake failed", "service", s.name, "error", err) + SetErrorResponse(w, r, http.StatusServiceUnavailable, nil) + return true + } + + w.WriteHeader(http.StatusOK) + return true +} +``` + +Controller construction writes `s.idleController` **once** per `Service` lifetime and reconfigures in place thereafter, so the request path's unlocked read is safe by construction: + +```go +func (s *Service) configureIdleController(options ServiceOptions) { + if options.SleepAfter <= 0 { + if s.idleController != nil { + s.idleController.Configure(0, 0, nil) + } + return + } + + if s.idleController == nil { + if s.lifecycle == nil { + // DeployService refuses --sleep-after without a lifecycle, so this is + // a restored service whose lifecycle arrives later, from + // SetContainerLifecycle. Building the controller now would start a + // timer that could reach StopContainer on a nil interface. + return + } + s.idleController = NewIdleController( + s.name, s.lifecycle, s.suspendForSleep, s.resumeFromSleep, s.persistState) + s.idleController.Restore(s.restoredIdleState) + } + + s.idleController.Configure(options.SleepAfter, options.WakeTimeout, s.containerRefs(options)) +} +``` + +Called as the last line of `initialize` (service.go:571). `Dispose` gains `if s.idleController != nil { s.idleController.Close() }`. + +**`internal/cmd/util.go` needs no change** — `getEnvString` already exists at util.go:79, so #228's hunk is a duplicate declaration. + +--- + +## 12. TDD list, in write order + +Each entry: the test, then the one behavior it proves. + +**Controller — `idle_controller_test.go`, `fakeLifecycle` counting `atomic.Int64`, no HTTP** + +1. `TestIdleState_NamesRoundTrip` — `stopping`/`waking` parse back to `sleeping`; unknown → `active`. *Locks the persisted vocabulary before anything writes it.* +2. `TestIdleController_AdmitsRequestsWhileActive` — `BeginRequest` returns nil, zero lifecycle calls. *The feature costs nothing when awake.* +3. `TestIdleController_SleepsAfterIdlePeriod` — one `StopContainer` per ref, `suspend` called, state `sleeping`, `persist` once. +4. `TestIdleController_DoesNotSleepWithRequestsInFlight` — `BeginRequest` without `EndRequest` ⇒ no stop, ever. *The WebSocket/SSE guarantee at unit level.* +5. `TestIdleController_WakesOnTheNextRequest` — one `StartContainer`, `resume` called with a positive budget, state `active`. +6. `TestIdleController_CoalescesConcurrentWakes` — 20 goroutines, `starts.Load() == 1`, all 20 return nil. *The headline property; the one thing that must never regress.* +7. `TestIdleController_BoundsTheWaitAcrossAStopAndAWake` — a request arriving during `Stopping` with a blocking stop returns by `wakeTimeout`, not 2×. *Proves the single-deadline fix.* +8. `TestIdleController_ClientCancellationReleasesTheWaiterNotTheWake` — cancelled ctx returns `context.Canceled` while the wake completes; a later call finds `active`. +9. `TestIdleController_BacksOffAfterAFailedWake` — three sequential `BeginRequest`s against a failing lifecycle ⇒ exactly one `StartContainer`; calls 2 and 3 return the cached error immediately. *Proves no request-rate retry storm.* +10. `TestIdleController_StaysAwakeWhenContainersCannotBeStopped` — stop errors ⇒ state `active`, `resume` called, no persist. ***The most important test in the suite** — the exact inverse of #228's `TestIdleControllerRecoversFromPartialStopFailure`.* +11. `TestIdleController_ResetSupersedesAnInFlightWake` — `Reset` during a slow wake; waiters proceed, and the wake's late `finishWake` does not move the state. *The generation token.* +12. `TestIdleController_DisableSuppressesSleepOnly` — disabled + idle ⇒ no stop; a request still wakes a sleeping service. +13. `TestIdleController_PersistsOnlySleepAndWakeEdges` — persist count is exactly 2 per cycle. + +**Docker client — `docker_client_test.go`, `httptest` over a real unix listener** + +14. `TestDockerClient_NegotiatesAndUsesVersionedPaths` — `POST /v1.44/containers/web-1/start`, name path-escaped. *The wire contract.* +15. `TestDockerClient_TreatsNotModifiedAsSuccess` — `304` is not an error. *A coalesced wake against a running container succeeds.* +16. `TestDockerClient_FallsBackWhenVersionIsUnavailable` — transport error and non-2xx both yield `1.41`. *Socket proxies without `/version` still work.* +17. `TestDockerClient_DoesNotCacheANegotiationFailure` — call twice, second succeeds. *Fixes #228's permanently-poisoned cache.* +18. `TestDockerClient_TruncatesLongErrorBodies` and `_ReadsALargeVersionPayload` — 4 KB error cap; `/version` gets its own much larger limit. *A plugin-heavy host does not truncate into `unexpected EOF`.* +19. `TestDockerClient_ClassifiesMissingAndForbidden` — 404 → `ErrContainerNotFound`, 403 → `ErrContainerInspectForbidden`. *Drives the preflight's warn-vs-fail split.* + +**Target and load balancer** + +20. `TestTarget_ContainerRefRejectsAddresses` — table: `web-1`→ok, `3f2a1b9c4d5e`→ok, `web-1:3000`→`web-1`, `web-1:3000;weight=5`→`web-1`, `10.0.0.5:3000`→false. +21. `TestLoadBalancer_SuspendForSleepEmptiesThePoolAndStopsProbing` — `HealthyTargets()` empty; killing the backend produces no further state churn. +22. `TestLoadBalancer_ResumeFromSleepRearmsWaitUntilHealthy` — **against a single-target pool**, so the checks were already stopped at first-healthy: suspend, resume, `WaitUntilHealthy(time.Second)` returns nil only once the backend actually answers. *Proves the exact defect in §5 — without `ResumeFromSleep` this returns nil instantly.* +23. `TestLoadBalancer_ResumeFromSleepDoesNotReleaseAPreviousWaiter` — a `WaitUntilHealthy` already parked is **not** released by the resume. *The #228 booby trap, avoided.* + +**Service — `service_idle_test.go`** + +24. `TestServiceOptions_ValidateSleep` — table: negative durations, `--tls-on-demand-url`, `--sleep-container` without `--sleep-after`. +25. `TestService_SleepingServiceWakesAndForwardsAChunkedBody` — POST with `Transfer-Encoding: chunked` and no `Content-Length`; backend receives every byte. ***The headline behavioral test.*** +26. `TestService_BlockedRequestsDoNotWake` — table over `--allow-ip`, `--basic-auth`, `--rate-limit`, canonical-host redirect: each returns its own status with `starts.Load() == 0`. ***Proves the placement decision; the reason this is a port and not a `git apply`.*** +27. `TestService_HealthCheckDoesNotWake` / `TestService_HealthCheckReportsUnhealthyAfterAFailedWake` — the two halves of the monitoring-honesty fix. +28. `TestService_StreamingResponsePreventsSleep` — an SSE handler held open past `--sleep-after` ⇒ no stop; closing it lets the next tick sleep. *End-to-end §4.* +29. `TestService_WakeFailureRendersTheServicesErrorPage` — custom `--error-pages` 503 body, **and assert the container reference does not appear in it**. +30. `TestService_PausedServiceIsNeverWokenByTraffic`. +31. `TestService_SleepStateSurvivesAMarshalRoundTrip` / `TestService_StateFileWithoutIdleStateRestoresAwake` — forward and backward compatibility. + +**Router — `router_idle_test.go`** + +32. `TestRouter_DeployRejectsSleepAfterWithoutADockerSocket` — `ErrNoContainerLifecycle`. +33. `TestRouter_DeployRejectsAnUnknownContainer` / `_RejectsAnAddressTarget` — both error messages name `--sleep-container`. +34. `TestRouter_DeployWarnsButProceedsWhenInspectIsForbidden` — the hardened-socket-proxy path. +35. `TestRouter_SleepContainerOverridesInference` — `--sleep-container` set ⇒ target hostnames never consulted. +36. `TestRouter_RestoreSuspendsASleepingServicesTargets` — `HealthyTargets()` empty after restore despite `MarkAllHealthy`. +37. `TestRouter_ListShowsSleepingAndPrefersPaused`. + +**CLI** + +38. `TestDeployCommand_SleepFlags` (defaults `0` / `30s` / nil), `TestRunCommand_DockerSocketFlagDefaultsToDisabled`, `_RejectsAMissingSocket`. + +Then: `gofmt -l internal/ cmd/`, `make test`, and **`go test -race ./...` is mandatory** — this touches `Router`, `LoadBalancer` and `Target`. + +No `make bench` gate: `BeginRequest` on an active service is one uncontended mutex acquire. I would still add `BenchmarkService_ServeHTTPWithIdleControllerActive` against the same service without one, since the gate is on every request of a sleep-enabled service. + +--- + +## 13. What I am NOT confident about + +**1. The prune interaction is unfixed and unfixable from this side.** Verified: `kamal deploy` invokes `kamal:cli:prune:all` on every run (`kamal/lib/kamal/cli/main.rb:58`), and `Kamal::Commands::Prune#app_containers` (`prune.rb:16-21`) pipes `docker ps -q -a --filter status=exited …` through `tail -n +6` into `docker rm`. **A sleeping container is `exited`.** With more than five stopped containers for a service, a sleeping one is a `docker rm` candidate — after which the persisted reference names nothing, every wake 404s, backoff climbs to 30s, and the service 503s permanently. The deploy preflight proves the ref resolves *at deploy time* and cannot prevent this. The backoff (test 9) and the honest health check (test 27) turn it from a silent outage into a visible one; they are mitigation, not a fix. **The real fix is gem-side** — exclude proxy-managed containers from the prune filter, or have the gem pass a stable label selector. It belongs in `../kamal` as a follow-up and must be named in the PR body and `ROADMAP.md`, as the PROXY-protocol and weighted-target work did. + +**2. The preflight is a new way for a deploy to fail.** In Kamal the target ID and the proxy's socket are the same host and daemon, so it holds. In a DinD, remote-daemon, or split-daemon socket-proxy topology, `ContainerExists` 404s and **every `--sleep-after` deploy fails loudly** — better than #228's silent outage, but new, and it fails on the RPC path where a hung socket costs `containerPreflightTimeout`. The 403 warn-path covers hardened socket proxies; it does not cover wrong-daemon. I considered a `--skip-sleep-preflight` escape hatch and left it out as YAGNI. **Revisit if anyone reports it.** + +**3. `Disable()` on pause is a judgment call, not a derivation.** Plan 2 argued for shipping nothing (the pause gate makes the wake half unreachable — true, verified). I ship the sleep half because a pause during a deploy that lets containers stop turns a 2-second window into a cold start. But `kamal-proxy stop` arguably *should* stop containers. I chose "suppress sleeping in both paused and stopped" for uniformity. **Low confidence; cheap to flip; make it a review question.** + +**4. `Service.MarshalJSON` taking `serviceLock.RLock()` is a pre-existing-race fix bundled into a feature PR.** Lock order is safe (`routerLock → serviceLock`, matching `installLoadBalancer`), and I verified no inversion. But it is a behavior change to the save path that this feature merely makes reachable, and a reviewer may reasonably want it split into its own commit. **I would land it as a separate first commit on the same branch.** + +**5. `RestartHealthChecks` does not fix the pre-existing `stateConsumer` race**, it only avoids widening it. `BeginHealthChecks` writes `t.stateConsumer` outside `withInflightLock` (target.go:285) while a live healthcheck goroutine can read it in `HealthCheckCompleted` (target.go:340). `RecheckHealth` already reaches this at runtime. My design routes the new wake path around it rather than through it — but `-race` may surface the existing one via `--recheck-targets-on-restore` tests once more of this code runs concurrently. **If it does, the fix is moving the assignment inside the lock, and it is in scope.** + +**6. Unmeasured:** `resume()` bounds a wake by a health-check round trip, so the cold-start figure depends entirely on the app. #228 measured 3.15s for a real Rails boot on a VPS; I have not reproduced that here and **the PR must not repeat the number as if this implementation had been measured.** \ No newline at end of file diff --git a/internal/server/concurrency_test.go b/internal/server/concurrency_test.go new file mode 100644 index 00000000..e9314fe5 --- /dev/null +++ b/internal/server/concurrency_test.go @@ -0,0 +1,109 @@ +package server + +import ( + "encoding/json" + "net/http" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +// Saving routing state marshals every service while deploys are replacing their +// load balancers. Both of these run today -- saveStateSnapshot holds only the +// router's read lock, and MarshalJSON read s.active and s.rollout without the +// service lock that UpdateLoadBalancer writes them under. +// +// Only -race fails on the unfixed code; without it the read is simply torn and +// silent, which is why this test asserts nothing beyond "it ran". +func TestService_MarshalJSONIsSafeAgainstConcurrentDeploys(t *testing.T) { + service, err := NewService("racy", defaultServiceOptions, defaultTargetOptions, nil) + require.NoError(t, err) + + service.UpdateLoadBalancer(testLoadBalancerWithHandlers(t, func(w http.ResponseWriter, r *http.Request) {}), TargetSlotActive) + + var deploys sync.WaitGroup + done := make(chan struct{}) + + deploys.Add(1) + go func() { + defer deploys.Done() + for { + select { + case <-done: + return + default: + lb := testLoadBalancerWithHandlers(t, func(w http.ResponseWriter, r *http.Request) {}) + if replaced := service.UpdateLoadBalancer(lb, TargetSlotActive); replaced != nil { + replaced.Dispose() + } + } + } + }() + + for range 200 { + _, err := json.Marshal(service) + require.NoError(t, err) + } + + close(done) + deploys.Wait() +} + +// --recheck-targets-on-restore calls RecheckHealth, which calls BeginHealthChecks +// on a target whose previous health check goroutine may still be running. That +// goroutine reads t.stateConsumer from HealthCheckCompleted while +// BeginHealthChecks writes it, and neither side held the inflight lock. +// +// Honest limitation: unlike the MarshalJSON test above, this one did NOT +// reproduce the race on the unfixed code -- the unsynchronized write and read +// sit either side of the same mutex, so the window is narrow enough that the +// detector did not sample it in a run of this length. It exercises both sides +// concurrently and would catch a coarser regression, but treat it as a smoke +// test, not proof. The fix stands on the code: a field written by one goroutine +// and read by another, now both under the inflight lock. +func TestTarget_BeginHealthChecksIsSafeAgainstAnInFlightCheck(t *testing.T) { + target := testTarget(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + t.Cleanup(target.StopHealthChecks) + + consumer := &countingStateConsumer{} + + var wg sync.WaitGroup + done := make(chan struct{}) + + // Drive health check completions directly: the real prober's interval is a + // second, far too slow to collide inside a test. + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-done: + return + default: + target.HealthCheckCompleted(true) + target.HealthCheckCompleted(false) + } + } + }() + + for range 100 { + target.BeginHealthChecks(consumer) + } + + close(done) + wg.Wait() +} + +type countingStateConsumer struct { + mu sync.Mutex + changes int +} + +func (c *countingStateConsumer) TargetStateChanged(target *Target) { + c.mu.Lock() + defer c.mu.Unlock() + c.changes++ +} diff --git a/internal/server/container_lifecycle.go b/internal/server/container_lifecycle.go new file mode 100644 index 00000000..9db8afcc --- /dev/null +++ b/internal/server/container_lifecycle.go @@ -0,0 +1,41 @@ +package server + +import ( + "context" + "errors" +) + +var ( + // ErrContainerNotFound means the runtime does not know the reference. For a + // wake that is usually a container the deploy-time preflight saw and + // something removed since -- `kamal deploy` prunes exited containers, and a + // sleeping container is exited. + ErrContainerNotFound = errors.New("container not found") + + // ErrContainerInspectForbidden means the socket answered, but refused to + // describe the container. Hardened socket proxies commonly allow start and + // stop while denying inspect, so the deploy preflight warns on this rather + // than failing -- refusing would lock out the operators doing the right thing. + ErrContainerInspectForbidden = errors.New("container inspection forbidden") +) + +// ContainerLifecycle starts and stops the containers behind a service. It is the +// seam that keeps scale-to-zero independent of how that happens: the shipped +// implementation talks to the Docker socket directly, which is the smallest +// opt-in approach and also grants the proxy root-equivalent access to the host. +// A restricted host-side start/stop service can replace it without the idle +// controller changing at all. +type ContainerLifecycle interface { + // StartContainer starts the container, returning nil if it is already + // running. That is what lets a proxy whose state file said "sleeping" for a + // container that never stopped heal itself on the next request. + StartContainer(ctx context.Context, ref string) error + + // StopContainer stops the container, returning nil if it is already stopped. + StopContainer(ctx context.Context, ref string) error + + // ContainerExists reports whether the reference names a container this + // runtime knows, so a deploy can reject a reference that would only fail + // hours later at the first idle timeout. + ContainerExists(ctx context.Context, ref string) error +} diff --git a/internal/server/idle_controller.go b/internal/server/idle_controller.go new file mode 100644 index 00000000..fd97c6a2 --- /dev/null +++ b/internal/server/idle_controller.go @@ -0,0 +1,532 @@ +package server + +import ( + "context" + "errors" + "fmt" + "log/slog" + "slices" + "sync" + "time" +) + +const ( + // DefaultWakeTimeout bounds the whole hold a request may experience: the + // stop it may have arrived during, the container start, and the wait for the + // app to answer a health check. + DefaultWakeTimeout = 30 * time.Second + + // containerStopTimeout is the budget for putting a service to sleep. No + // request waits on it, so it does not share the wake timeout. Docker's own + // stop is SIGTERM then SIGKILL after 10s. + containerStopTimeout = 30 * time.Second + + // maxWakeBackoff caps the pause between attempts after a wake fails, so a + // service whose container reference no longer resolves settles at one start + // attempt per interval instead of one per request. + maxWakeBackoff = 30 * time.Second + + idleTimerIdlePark = time.Hour +) + +var ( + ErrWakeTimeout = errors.New("timed out waking containers") + ErrWakeFailed = errors.New("failed to wake containers") + ErrNoContainerLifecycle = errors.New("scale-to-zero requires the proxy to run with --docker-socket") + ErrNotAContainerRef = errors.New("target does not name a container") +) + +// IdleState is where a service's containers are in the scale-to-zero cycle. +type IdleState int + +const ( + IdleStateActive IdleState = iota + IdleStateStopping + IdleStateSleeping + IdleStateWaking +) + +func (s IdleState) String() string { + switch s { + case IdleStateStopping: + return "stopping" + case IdleStateSleeping: + return "sleeping" + case IdleStateWaking: + return "waking" + default: + return "active" + } +} + +// ParseIdleState reads the name persisted in the state file. The transient +// states fold down to sleeping in both directions: a proxy that died mid +// transition cannot know whether the container moved, and waking from sleeping +// is the safe assumption because starting an already-running container succeeds. +// Anything unrecognised -- including the empty string every state file written +// before this feature existed carries -- restores active. +func ParseIdleState(name string) IdleState { + switch name { + case "sleeping", "stopping", "waking": + return IdleStateSleeping + default: + return IdleStateActive + } +} + +// IdleControllerConfig is everything the controller needs from its Service. The +// suspend/resume/persist hooks are supplied rather than reached for, so the +// controller never touches a load balancer, a target, or their locks. +type IdleControllerConfig struct { + Name string + Lifecycle ContainerLifecycle + Refs []string + SleepAfter time.Duration + WakeTimeout time.Duration + + // Suspend takes the targets out of the pool and stops probing them. + Suspend func() + // Resume puts them back and waits, within the given budget, for one to answer. + Resume func(timeout time.Duration) error + // Persist writes the routing state, so a sleeping service is still sleeping + // after a proxy restart. + Persist func() +} + +// IdleController stops a service's containers once it has been idle for +// SleepAfter, and starts them again on the next request that needs them. +type IdleController struct { + name string + lifecycle ContainerLifecycle + + suspend func() + resume func(timeout time.Duration) error + persist func() + + lock sync.Mutex + state IdleState + generation uint64 + refs []string + inflight int + lastRequest time.Time + sleepAfter time.Duration + wakeTimeout time.Duration + lastErr error + failures int + retryAfter time.Time + disabled bool + cancel context.CancelFunc + + // changed is closed on every transition and then replaced. A waiter reads it + // under the lock, parks on it, and re-reads the state when it wakes, so no + // decision is ever made from a stale snapshot. Nothing is ever sent on it -- + // the close is the broadcast, the same idiom PauseController uses. + changed chan struct{} + + signalled chan struct{} + closed chan struct{} + closeOnce sync.Once +} + +func NewIdleController(config IdleControllerConfig) *IdleController { + wakeTimeout := config.WakeTimeout + if wakeTimeout <= 0 { + wakeTimeout = DefaultWakeTimeout + } + + controller := &IdleController{ + name: config.Name, + lifecycle: config.Lifecycle, + suspend: config.Suspend, + resume: config.Resume, + persist: config.Persist, + refs: slices.Clone(config.Refs), + sleepAfter: config.SleepAfter, + wakeTimeout: wakeTimeout, + lastRequest: time.Now(), + changed: make(chan struct{}), + signalled: make(chan struct{}, 1), + closed: make(chan struct{}), + } + + go controller.run() + + return controller +} + +func (c *IdleController) State() IdleState { + c.lock.Lock() + defer c.lock.Unlock() + return c.state +} + +// RestoreSleeping puts a controller straight into sleeping, for a service whose +// state file said its containers were down. It does not touch the containers -- +// they are already stopped, and the next request is what starts them. +func (c *IdleController) RestoreSleeping() { + c.lock.Lock() + defer c.lock.Unlock() + + if c.state == IdleStateActive { + c.setStateLocked(IdleStateSleeping) + } +} + +// Close stops the timer goroutine and releases every parked waiter. An in-flight +// container operation is cancelled rather than waited on. +func (c *IdleController) Close() { + c.closeOnce.Do(func() { + c.lock.Lock() + cancel := c.cancel + c.cancel = nil + c.lock.Unlock() + + if cancel != nil { + cancel() + } + close(c.closed) + }) +} + +// Disable suppresses sleeping without affecting waking, for a service that has +// been paused or stopped. A paused service that let its containers stop would +// turn the resume into a cold start. +func (c *IdleController) Disable() { + c.lock.Lock() + c.disabled = true + c.lock.Unlock() + + c.signal() +} + +func (c *IdleController) Enable() { + c.lock.Lock() + c.disabled = false + c.lastRequest = time.Now() + c.lock.Unlock() + + c.signal() +} + +// Reset points the controller at a new set of containers and declares the +// service awake, which is what a redeploy means. Bumping the generation is what +// stops a lifecycle goroutine started before the deploy from writing its outcome +// over the state the deploy just established. +func (c *IdleController) Reset(refs []string) { + c.lock.Lock() + c.refs = slices.Clone(refs) + c.lastRequest = time.Now() + c.lastErr, c.failures, c.retryAfter = nil, 0, time.Time{} + + cancel := c.cancel + c.cancel = nil + + if c.state == IdleStateActive { + // Still bump the generation: a wake or a stop may be in flight even + // though the state has already settled back to active. + c.generation++ + } else { + c.setStateLocked(IdleStateActive) + } + c.lock.Unlock() + + if cancel != nil { + cancel() + } + c.signal() +} + +// BeginRequest admits a request, waking the service first if it is asleep. It +// blocks the calling request goroutine and never touches the request itself, +// which is why the gate sits above target selection: the request handed on +// afterwards is byte-for-byte the one that arrived, its body still unread. +// +// Every admitted request must be paired with EndRequest. +func (c *IdleController) BeginRequest(ctx context.Context) error { + // One deadline for the whole call. A per-iteration timer would let a request + // that arrived while the service was still stopping wait the full timeout for + // the stop and the full timeout again for the start -- twice the bound the + // flag documents. + deadline := time.Now().Add(c.WakeTimeout()) + + for { + changed, err := c.admit() + if changed == nil { + return err + } + + remaining := time.Until(deadline) + if remaining <= 0 { + return ErrWakeTimeout + } + + timer := time.NewTimer(remaining) + select { + case <-changed: + timer.Stop() + case <-timer.C: + return ErrWakeTimeout + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-c.closed: + timer.Stop() + return ErrWakeFailed + } + } +} + +// admit either takes a slot for the request, or hands back the channel to wait +// on for the next transition. A nil channel means the caller is finished: +// admitted when err is nil, rejected otherwise. +func (c *IdleController) admit() (<-chan struct{}, error) { + c.lock.Lock() + defer c.lock.Unlock() + + switch c.state { + case IdleStateActive: + c.inflight++ + c.lastRequest = time.Now() + return nil, nil + + case IdleStateSleeping: + if time.Now().Before(c.retryAfter) { + // A wake failed recently. Fail now rather than hold for the full + // timeout and issue another doomed start: at request rate that is an + // unbounded retry storm against the container runtime. + return nil, c.lastErr + } + c.startWakeLocked() + } + + return c.changed, nil +} + +func (c *IdleController) EndRequest() { + c.lock.Lock() + if c.inflight > 0 { + c.inflight-- + } + // Stamped on the way out as well as in, so an hour-long stream counts as + // activity for that hour rather than for the instant it started. + c.lastRequest = time.Now() + c.lock.Unlock() + + c.signal() +} + +func (c *IdleController) WakeTimeout() time.Duration { + c.lock.Lock() + defer c.lock.Unlock() + return c.wakeTimeout +} + +func (c *IdleController) setStateLocked(state IdleState) { + if c.state == state { + return + } + + c.state = state + c.generation++ + + close(c.changed) + c.changed = make(chan struct{}) +} + +func (c *IdleController) signal() { + select { + case c.signalled <- struct{}{}: + default: + } +} + +// startWakeLocked is reachable only from the sleeping arm of admit, and flips the +// state while still holding the lock. Concurrent callers therefore serialize on +// that mutex: the first starts the wake, every later one parks on the same +// channel. The coalescing is a consequence of the mutex, not extra machinery. +func (c *IdleController) startWakeLocked() { + c.setStateLocked(IdleStateWaking) + + generation := c.generation + refs := slices.Clone(c.refs) + deadline := time.Now().Add(c.wakeTimeout) + lifecycle, resume := c.lifecycle, c.resume + + ctx, cancel := context.WithDeadline(context.Background(), deadline) + c.cancel = cancel + + slog.Info("Waking service", "service", c.name, "containers", refs) + + go func() { + defer cancel() + + var err error + for _, ref := range refs { + if startErr := lifecycle.StartContainer(ctx, ref); startErr != nil { + err = fmt.Errorf("%w: container %s: %w", ErrWakeFailed, ref, startErr) + break + } + } + + if err == nil && resume != nil { + // Started is not ready. Wait on whatever budget the starts left, not + // a fresh one. + if remaining := time.Until(deadline); remaining <= 0 { + err = ErrWakeTimeout + } else { + err = resume(remaining) + } + } + + c.finishWake(generation, err) + }() +} + +func (c *IdleController) finishWake(generation uint64, err error) { + c.lock.Lock() + + // A deploy or a close landed mid-wake and has already decided what the state + // should be. Do not overwrite it with the outcome of a wake it superseded. + if c.generation != generation { + c.lock.Unlock() + return + } + + c.cancel = nil + suspend, persist := c.suspend, c.persist + + if err == nil { + c.lastErr, c.failures, c.retryAfter = nil, 0, time.Time{} + c.lastRequest = time.Now() + c.setStateLocked(IdleStateActive) + } else { + c.lastErr = err + c.failures++ + c.retryAfter = time.Now().Add(wakeBackoff(c.failures)) + c.setStateLocked(IdleStateSleeping) + } + c.lock.Unlock() + + if err != nil { + slog.Error("Failed to wake service", "service", c.name, "error", err) + // Back out of the pool, so a container that came up but never answered is + // not probed once a second until somebody notices. + if suspend != nil { + suspend() + } + return + } + + slog.Info("Service awake", "service", c.name) + if persist != nil { + persist() + } +} + +// wakeBackoff spaces out attempts after a failed wake. Without it a service whose +// container reference no longer resolves costs one start per inbound request, +// forever. +func wakeBackoff(failures int) time.Duration { + return min(time.Second< 0 && len(c.refs) > 0 + c.lock.Unlock() + + if !eligible { + wait = idleTimerIdlePark + } + wait = max(wait, 0) + + timer := time.NewTimer(wait) + select { + case <-timer.C: + c.trySleep() + case <-c.signalled: + timer.Stop() + case <-c.closed: + timer.Stop() + return + } + } +} diff --git a/internal/server/idle_controller_test.go b/internal/server/idle_controller_test.go new file mode 100644 index 00000000..d04bd30e --- /dev/null +++ b/internal/server/idle_controller_test.go @@ -0,0 +1,366 @@ +package server + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeLifecycle counts container operations without touching Docker. Every hook +// is optional; the zero value succeeds instantly. +type fakeLifecycle struct { + starts atomic.Int64 + stops atomic.Int64 + + startFunc func(ctx context.Context, ref string) error + stopFunc func(ctx context.Context, ref string) error +} + +func (f *fakeLifecycle) StartContainer(ctx context.Context, ref string) error { + f.starts.Add(1) + if f.startFunc != nil { + return f.startFunc(ctx, ref) + } + return nil +} + +func (f *fakeLifecycle) StopContainer(ctx context.Context, ref string) error { + f.stops.Add(1) + if f.stopFunc != nil { + return f.stopFunc(ctx, ref) + } + return nil +} + +func (f *fakeLifecycle) ContainerExists(ctx context.Context, ref string) error { + return nil +} + +type idleHooks struct { + mu sync.Mutex + suspends int + resumes int + persists int + resumeArg time.Duration + + resumeFunc func(timeout time.Duration) error +} + +func (h *idleHooks) counts() (suspends, resumes, persists int) { + h.mu.Lock() + defer h.mu.Unlock() + return h.suspends, h.resumes, h.persists +} + +func testIdleController(t *testing.T, lifecycle ContainerLifecycle, sleepAfter time.Duration) (*IdleController, *idleHooks) { + t.Helper() + + hooks := &idleHooks{} + + controller := NewIdleController(IdleControllerConfig{ + Name: "test", + Lifecycle: lifecycle, + Refs: []string{"web-1"}, + SleepAfter: sleepAfter, + WakeTimeout: time.Second, + Suspend: func() { + hooks.mu.Lock() + hooks.suspends++ + hooks.mu.Unlock() + }, + Resume: func(timeout time.Duration) error { + hooks.mu.Lock() + hooks.resumes++ + hooks.resumeArg = timeout + resumeFunc := hooks.resumeFunc + hooks.mu.Unlock() + + if resumeFunc != nil { + return resumeFunc(timeout) + } + return nil + }, + Persist: func() { + hooks.mu.Lock() + hooks.persists++ + hooks.mu.Unlock() + }, + }) + + t.Cleanup(controller.Close) + return controller, hooks +} + +// waitForState polls until the controller reaches want, so tests never sleep for +// a fixed duration and never race a transition that happens on another goroutine. +func waitForState(t *testing.T, controller *IdleController, want IdleState) { + t.Helper() + + require.Eventually(t, func() bool { + return controller.State() == want + }, 2*time.Second, time.Millisecond, "expected state %s, got %s", want, controller.State()) +} + +func TestIdleState_NamesRoundTrip(t *testing.T) { + tests := []struct { + name string + expected IdleState + }{ + {name: "active", expected: IdleStateActive}, + {name: "sleeping", expected: IdleStateSleeping}, + + // A proxy that died mid-transition cannot know whether the container + // moved. Both fold to sleeping, which is safe to wake from: starting an + // already-running container answers 304. + {name: "stopping", expected: IdleStateSleeping}, + {name: "waking", expected: IdleStateSleeping}, + + // Every state file written before scale-to-zero existed. + {name: "", expected: IdleStateActive}, + {name: "nonsense", expected: IdleStateActive}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, ParseIdleState(tt.name)) + }) + } + + assert.Equal(t, "active", IdleStateActive.String()) + assert.Equal(t, "sleeping", IdleStateSleeping.String()) +} + +func TestIdleController_AdmitsRequestsWhileActive(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, hooks := testIdleController(t, lifecycle, 0) + + require.NoError(t, controller.BeginRequest(context.Background())) + controller.EndRequest() + + assert.Zero(t, lifecycle.starts.Load(), "an awake service must not start anything") + assert.Zero(t, lifecycle.stops.Load()) + + suspends, resumes, persists := hooks.counts() + assert.Zero(t, suspends) + assert.Zero(t, resumes) + assert.Zero(t, persists) +} + +func TestIdleController_SleepsAfterIdlePeriod(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, hooks := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + assert.Equal(t, int64(1), lifecycle.stops.Load(), "one stop per container reference") + + suspends, _, persists := hooks.counts() + assert.Equal(t, 1, suspends, "targets leave the pool before the container goes down") + assert.Equal(t, 1, persists, "sleeping is a persisted edge") +} + +func TestIdleController_DoesNotSleepWithRequestsInFlight(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + // Never released -- this is what a WebSocket or an SSE stream looks like to + // the controller, and it is the whole guarantee that neither needs + // special-casing anywhere in the feature. + require.NoError(t, controller.BeginRequest(context.Background())) + + time.Sleep(80 * time.Millisecond) + + assert.Equal(t, IdleStateActive, controller.State()) + assert.Zero(t, lifecycle.stops.Load(), "a service with an open stream must never sleep") +} + +func TestIdleController_WakesOnTheNextRequest(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, hooks := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + require.NoError(t, controller.BeginRequest(context.Background())) + defer controller.EndRequest() + + assert.Equal(t, IdleStateActive, controller.State()) + assert.Equal(t, int64(1), lifecycle.starts.Load()) + + hooks.mu.Lock() + resumeArg := hooks.resumeArg + hooks.mu.Unlock() + assert.Positive(t, resumeArg, "resume gets whatever budget the starts left, not a fresh one") +} + +func TestIdleController_CoalescesConcurrentWakes(t *testing.T) { + lifecycle := &fakeLifecycle{ + startFunc: func(ctx context.Context, ref string) error { + time.Sleep(20 * time.Millisecond) + return nil + }, + } + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + const concurrency = 20 + var wg sync.WaitGroup + errs := make([]error, concurrency) + + for i := range concurrency { + wg.Add(1) + go func() { + defer wg.Done() + errs[i] = controller.BeginRequest(context.Background()) + if errs[i] == nil { + controller.EndRequest() + } + }() + } + wg.Wait() + + for i, err := range errs { + assert.NoError(t, err, "waiter %d", i) + } + assert.Equal(t, int64(1), lifecycle.starts.Load(), + "twenty concurrent requests must coalesce into exactly one container start") +} + +func TestIdleController_ClientCancellationReleasesTheWaiterNotTheWake(t *testing.T) { + lifecycle := &fakeLifecycle{ + startFunc: func(ctx context.Context, ref string) error { + time.Sleep(50 * time.Millisecond) + return nil + }, + } + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(10 * time.Millisecond) + cancel() + }() + + err := controller.BeginRequest(ctx) + require.ErrorIs(t, err, context.Canceled, "the client hanging up releases its own waiter") + + // The wake it triggered still completes for everyone else. + waitForState(t, controller, IdleStateActive) + assert.Equal(t, int64(1), lifecycle.starts.Load()) +} + +func TestIdleController_BacksOffAfterAFailedWake(t *testing.T) { + wakeErr := errors.New("no such container") + lifecycle := &fakeLifecycle{ + startFunc: func(ctx context.Context, ref string) error { return wakeErr }, + } + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + first := controller.BeginRequest(context.Background()) + require.Error(t, first) + + // Calls 2 and 3 must return the cached error immediately rather than issue + // another doomed start -- at request rate that is a retry storm. + for range 2 { + start := time.Now() + err := controller.BeginRequest(context.Background()) + require.Error(t, err) + assert.Less(t, time.Since(start), 100*time.Millisecond, "backoff must fail fast, not hold") + } + + assert.Equal(t, int64(1), lifecycle.starts.Load(), "exactly one start attempt across three requests") +} + +// The exact inverse of upstream #228, which marked the service asleep even when +// the stop failed -- turning an unmounted socket or a pruned container into a +// permanent outage for containers that were running perfectly. +func TestIdleController_StaysAwakeWhenContainersCannotBeStopped(t *testing.T) { + lifecycle := &fakeLifecycle{ + stopFunc: func(ctx context.Context, ref string) error { return errors.New("permission denied") }, + } + controller, hooks := testIdleController(t, lifecycle, 10*time.Millisecond) + + require.Eventually(t, func() bool { + return lifecycle.stops.Load() > 0 + }, 2*time.Second, time.Millisecond) + + waitForState(t, controller, IdleStateActive) + + _, resumes, persists := hooks.counts() + assert.Positive(t, resumes, "targets go back in the pool so health checks can sort out reality") + assert.Zero(t, persists, "a rolled-back sleep is not a persisted edge") +} + +func TestIdleController_DisableSuppressesSleepOnly(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + controller.Disable() + time.Sleep(60 * time.Millisecond) + + assert.Equal(t, IdleStateActive, controller.State()) + assert.Zero(t, lifecycle.stops.Load(), "a paused service must not sleep") + + // But a service that is already asleep still wakes on traffic. + controller.Enable() + waitForState(t, controller, IdleStateSleeping) + controller.Disable() + + require.NoError(t, controller.BeginRequest(context.Background())) + controller.EndRequest() + assert.Equal(t, IdleStateActive, controller.State()) + assert.Equal(t, int64(1), lifecycle.starts.Load()) +} + +func TestIdleController_ResetSupersedesAnInFlightWake(t *testing.T) { + release := make(chan struct{}) + lifecycle := &fakeLifecycle{ + startFunc: func(ctx context.Context, ref string) error { + <-release + return nil + }, + } + controller, _ := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + + done := make(chan error, 1) + go func() { done <- controller.BeginRequest(context.Background()) }() + + waitForState(t, controller, IdleStateWaking) + + // A redeploy lands mid-wake and decides the state itself. + controller.Reset([]string{"web-2"}) + assert.Equal(t, IdleStateActive, controller.State()) + + require.NoError(t, <-done, "the parked waiter proceeds once the deploy makes the service active") + + close(release) + + // The superseded wake's late completion must not move the state back. + time.Sleep(50 * time.Millisecond) + assert.Equal(t, IdleStateActive, controller.State()) +} + +func TestIdleController_PersistsOnlySleepAndWakeEdges(t *testing.T) { + lifecycle := &fakeLifecycle{} + controller, hooks := testIdleController(t, lifecycle, 10*time.Millisecond) + + waitForState(t, controller, IdleStateSleeping) + require.NoError(t, controller.BeginRequest(context.Background())) + controller.EndRequest() + waitForState(t, controller, IdleStateActive) + + _, _, persists := hooks.counts() + assert.Equal(t, 2, persists, "exactly one persist per edge, two per full cycle") +} diff --git a/internal/server/service.go b/internal/server/service.go index c7a971d8..24f1a626 100644 --- a/internal/server/service.go +++ b/internal/server/service.go @@ -423,6 +423,14 @@ type marshalledService struct { } func (s *Service) MarshalJSON() ([]byte, error) { + // Saving state marshals every service while deploys may be replacing their + // load balancers: saveStateSnapshot holds only the router's read lock, and + // UpdateLoadBalancer writes s.active and s.rollout under this one. Lock order + // is routerLock then serviceLock, matching installLoadBalancer, so this + // cannot invert. + s.serviceLock.RLock() + defer s.serviceLock.RUnlock() + // Specs rather than Names, so that a target's weight survives a restart. It // renders as a bare address unless a weight was actually set, so unweighted // state files stay exactly what they have always been. diff --git a/internal/server/target.go b/internal/server/target.go index 33313b10..f9a54687 100644 --- a/internal/server/target.go +++ b/internal/server/target.go @@ -282,9 +282,11 @@ WAIT_FOR_REQUESTS_TO_COMPLETE: } func (t *Target) BeginHealthChecks(stateConsumer TargetStateConsumer) { - t.stateConsumer = stateConsumer - t.withInflightLock(func() { + // Inside the lock: RecheckHealth reaches here while the previous prober's + // goroutine can still be in HealthCheckCompleted reading this field. + t.stateConsumer = stateConsumer + if t.healthcheck != nil { t.healthcheck.Close() } @@ -313,6 +315,7 @@ func (t *Target) StopHealthChecks() { func (t *Target) HealthCheckCompleted(success bool) { var previousState, newState TargetState + var stateConsumer TargetStateConsumer t.withInflightLock(func() { previousState = t.state @@ -333,13 +336,17 @@ func (t *Target) HealthCheckCompleted(success bool) { } newState = t.state + + // Read under the lock and used outside it: BeginHealthChecks writes this + // field, and RecheckHealth calls it while this goroutine may be running. + stateConsumer = t.stateConsumer }) if newState != previousState { slog.Info("Target health updated", "target", t.Address(), "state", newState.String(), "was", previousState.String()) - if t.stateConsumer != nil { - t.stateConsumer.TargetStateChanged(t) + if stateConsumer != nil { + stateConsumer.TargetStateChanged(t) } } } diff --git a/internal/server/tls_on_demand_registry_test.go b/internal/server/tls_on_demand_registry_test.go index af2e53fc..e2e622a1 100644 --- a/internal/server/tls_on_demand_registry_test.go +++ b/internal/server/tls_on_demand_registry_test.go @@ -151,16 +151,24 @@ func TestRouter_GetCertificate_RegistryStillServesHostScopedServices(t *testing. _, target := testBackend(t, "first", http.StatusOK) + // A static certificate, so that the fall-through past the registry lands on + // StaticCertManager. Left to autocert, this test asked Let's Encrypt + // PRODUCTION for a certificate for app.example.com and blocked for its full + // five-minute internal timeout -- on its own, 300 of the suite's 320 seconds. + certPath, keyPath := prepareTestCertificateFiles(t) + serviceOptions := defaultServiceOptions serviceOptions.TLSEnabled = true serviceOptions.ACMECachePath = t.TempDir() serviceOptions.Hosts = []string{"app.example.com"} + serviceOptions.TLSCertificatePath = certPath + serviceOptions.TLSPrivateKeyPath = keyPath require.NoError(t, router.DeployService("hostscoped", []string{target}, defaultEmptyReaders, serviceOptions, defaultTargetOptions, defaultDeploymentOptions)) _, err := router.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) - assert.Error(t, err, "no provisioning method is wired up in this test") + assert.NoError(t, err, "the service's own certificate serves the request") assert.NotContains(t, registry.pendingDomains, "sibling.example.com", "registry should have batched the pending sibling while provisioning")