From 644c741a8e5b3156edbbb31f56e63abe02917898 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 11:50:53 +0200 Subject: [PATCH 1/5] docs: make dash the main branch and drop upstream mergeability as a constraint The docs told every workflow to root feature branches off main "to keep them upstream-PR-able". That benefit is not one this fork wants, and the cost was concrete: GitHub resolves pull_request workflows from the head branch, and main's ci.yml triggers only on main, so a main-rooted PR into dash ran zero checks -- including golangci-lint, which is not installed locally. Those branches also conflicted on every fork-only file. dash is the main branch here. Branch off it, PR back into it, and design for dash rather than for what basecamp would accept. main stays as an upstream mirror to merge from, never a target. Same call as the ../kamal fork. Also refreshes the scale-to-zero roadmap anchor: the PR it named (#197) is closed, superseded by #228, whose Docker-socket-behind-a-lifecycle-interface architecture the upstream maintainer blessed in discussion #222. Records that mounting docker.sock into the internet-facing proxy is root-equivalent on the host, so the next reader meets that tradeoff with the anchor rather than after implementing. /plan now also has to check whether a "port basecamp#N" issue still points at an open PR -- two have gone stale under us already (#63 to #225, #197 to #228). --- .claude/commands/github-review-pr.md | 8 ++++---- .claude/commands/lfg.md | 4 ++-- .claude/commands/plan.md | 5 +++-- .claude/rules/git-workflow.md | 13 ++++++++++--- CLAUDE.md | 14 ++++++++++---- ROADMAP.md | 2 +- 6 files changed, 30 insertions(+), 16 deletions(-) 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 fa06cd8d..7ea273ff 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -46,7 +46,7 @@ Proxy-side roadmap for the dash fork. The cross-repo release sequencing, strateg | Weighted canary (`--target=b;weight=5`) | kamal#941, #8 | `LoadBalancer.nextTarget` (`load_balancer.go:216`) — currently pure round-robin | | 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 | -| 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) From a0633908e47243118cad45492a3d7e85d62d7e34 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 12:41:09 +0200 Subject: [PATCH 2/5] feat(server): idle controller and container lifecycle seam for scale-to-zero First half of scale-to-zero (#19): the state machine that decides when a service's containers should stop and start, and the interface that does it. No wiring into Service or Router yet, so nothing observable changes. Ports the design of basecamp/kamal-proxy#228 -- which supersedes the closed #197 the issue names -- adapted to dash, with four deliberate departures: A failed stop rolls the service back to active. #228 marked it asleep anyway, which turns an unmounted socket or a pruned container into a permanent outage: every later request held for the wake timeout and then failed, for containers that were running perfectly. A failed wake backs off, doubling to 30s. Without it a service whose container reference no longer resolves costs one container start per inbound request, forever. That case is not hypothetical -- see below. BeginRequest takes one deadline for the whole call. A per-iteration timer lets a request that arrived during a stop wait the full timeout for the stop and the full timeout again for the start, twice the bound the flag documents. A generation counter supersedes any in-flight lifecycle goroutine on any transition, so a deploy landing mid-wake is not overwritten by the outcome of the wake it replaced. #228 used a per-wake token that only covered wakes. Coalescing is a consequence of the mutex rather than extra machinery: startWakeLocked flips the state while still holding the lock, so twenty concurrent requests produce exactly one container start. Known hole, mitigated not fixed: `kamal deploy` runs prune on every deploy, and Kamal::Commands::Prune#app_containers filters `status=exited` into `docker rm`. A sleeping container is exited. Past five stopped containers for a service, a sleeping one becomes a removal candidate, after which every wake 404s. The backoff here turns that from a retry storm into a visible failure; the actual fix is gem-side in ../kamal and belongs in its own issue. Also lands docs/plans/2026-07-29-scale-to-zero.md, the full implementation plan, since the remaining wiring is several days of work and the reasoning behind the placement decisions should not have to be rediscovered. Refs #19 --- docs/plans/2026-07-29-scale-to-zero.md | 1173 +++++++++++++++++++++++ internal/server/container_lifecycle.go | 41 + internal/server/idle_controller.go | 532 ++++++++++ internal/server/idle_controller_test.go | 366 +++++++ 4 files changed, 2112 insertions(+) create mode 100644 docs/plans/2026-07-29-scale-to-zero.md create mode 100644 internal/server/container_lifecycle.go create mode 100644 internal/server/idle_controller.go create mode 100644 internal/server/idle_controller_test.go 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/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") +} From 1e36b894ba19fd2cfffdc96a53635e32b452f388 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 12:48:21 +0200 Subject: [PATCH 3/5] fix(test): stop the registry test asking Let's Encrypt production for a cert TestRouter_GetCertificate_RegistryStillServesHostScopedServices deployed a TLS service with no certificate paths, so createCertManager handed it an autocert.Manager. The test then called Router.GetCertificate, the registry declined as intended, and the fall-through reached autocert -- which went to the real Let's Encrypt production directory and blocked for its full internal timeout. That one test was 300 of the 320 seconds the internal/server suite took. With a static certificate instead, the fall-through lands on StaticCertManager and the suite is back to 23s. It also violated .claude/rules/testing.md outright: "no real ACME/Let's Encrypt calls ... never hit the real endpoint". Nothing in make test is supposed to need the network, and CI was making an outbound request to a rate-limited production ACME endpoint on every run. My own test, from #50. Found while timing the suite for #19. --- internal/server/tls_on_demand_registry_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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") From 67c1f695951df0307a0d2589977a0badf26db95d Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 13:06:45 +0200 Subject: [PATCH 4/5] fix(server): serialize the two fields a deploy and a background goroutine share Two unsynchronized field accesses, both pre-existing on dash and both made far more reachable by scale-to-zero, which persists state on every sleep and wake and re-arms health checks on every wake. Service.MarshalJSON read s.active and s.rollout with no lock, while UpdateLoadBalancer writes them under serviceLock. saveStateSnapshot marshals every service holding only the router's read lock, so a deploy landing during a state save tears the read. Confirmed with -race before the fix: WARNING: DATA RACE Service.UpdateLoadBalancer() Service.MarshalJSON() Lock order is routerLock then serviceLock, matching installLoadBalancer, so taking serviceLock here cannot invert. Target.BeginHealthChecks wrote t.stateConsumer outside withInflightLock while HealthCheckCompleted read it outside the lock from the prober's goroutine. RecheckHealth reaches this at runtime under --recheck-targets-on-restore. Both sides now go through the lock, with the consumer captured inside and called outside it so a state-change callback never runs while holding it. TestService_MarshalJSONIsSafeAgainstConcurrentDeploys is a genuine regression test -- it fails with a DATA RACE on the unfixed code. TestTarget_BeginHealthChecksIsSafeAgainstAnInFlightCheck is not: reverting the target.go fix does not make it trip, because the unsynchronized write and read sit either side of the same mutex and the window is too narrow to sample in a run of this length. It is labelled as a smoke test rather than left to imply a guarantee it does not give. A third finding from the same review is deliberately NOT changed: LoadBalancer.waitForHealthyContext is read unlocked at load_balancer.go:154, but it is written once in NewLoadBalancer before the value is published and never again, so it is not a race today. It becomes one the moment scale-to-zero re-arms it in ResumeFromSleep, and the lock belongs in that commit where the second writer appears. Refs #19 --- internal/server/concurrency_test.go | 109 ++++++++++++++++++++++++++++ internal/server/service.go | 8 ++ internal/server/target.go | 15 +++- 3 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 internal/server/concurrency_test.go 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/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) } } } From 390ef12205fa5d39c2c18c0fcd59cb59564dea01 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 13:27:58 +0200 Subject: [PATCH 5/5] chore: trigger CodeQL now that default setup is enabled