Add opt-in scale-to-zero for idle services - #228
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “scale-to-zero” capability to kamal-proxy services by introducing an idle state machine that can stop write containers after inactivity and transparently wake them (including waiting for health readiness) on the next non-health-check request. This integrates Docker lifecycle management behind a small interface, persists sleep state, and exposes configuration via deploy/run flags plus documentation.
Changes:
- Introduces
IdleControllerand a Unix-socketDockerClientto stop/start containers and coordinate wake coalescing + readiness waiting. - Wires idle behavior into
Service/Router(state persistence, listing state, health-check behavior while sleeping, wake health-check restart). - Adds CLI/config/docs support for
--idle-timeout,--idle-wake-timeout, and--docker-socket.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the opt-in scale-to-zero behavior, Docker socket implications, and version negotiation. |
| internal/server/testing.go | Updates test server construction for the new NewRouter signature. |
| internal/server/target.go | Adds container-name access and health-check restart hooks used by wake behavior. |
| internal/server/service.go | Integrates IdleController into request flow, state persistence, and wake readiness waiting. |
| internal/server/service_test.go | Updates service construction and adds coverage for idle lifecycle initialization. |
| internal/server/router.go | Adds Docker client wiring, restores idle state correctly, and serializes state snapshots. |
| internal/server/router_test.go | Updates router construction for the new NewRouter signature. |
| internal/server/load_balancer.go | Adds PrepareForWake to restart health checks and re-arm healthy-wait state. |
| internal/server/load_balancer_test.go | Adds coverage for wake health-check restart behavior. |
| internal/server/idle_controller.go | New idle state machine with sleep/wake transitions, coalescing, and persistence hooks. |
| internal/server/idle_controller_test.go | New unit tests for idle sleep/wake/coalescing/restore behavior. |
| internal/server/health_check.go | Adds explicit Start() and refactors construction to support restart semantics. |
| internal/server/docker_client.go | New minimal Docker HTTP client over Unix socket with API version negotiation. |
| internal/server/docker_client_test.go | New tests for negotiation, fallback, concurrency, and error-body bounding. |
| internal/server/config.go | Adds DefaultDockerSocketPath and a config field for socket path. |
| internal/cmd/util.go | Adds getEnvString for string-valued flag defaults from env. |
| internal/cmd/run.go | Adds --docker-socket and passes it into NewRouter. |
| internal/cmd/deploy.go | Adds --idle-timeout and --idle-wake-timeout service options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/server/service.go:252
- Dispose assumes s.active is always non-nil and will panic if a Service is constructed before an active load balancer is installed (e.g., tests already cover this scenario). Guarding the call avoids a nil dereference during teardown/error paths.
s.active.Dispose()
if s.rollout != nil {
s.rollout.Dispose()
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/server/router.go:394
- When
os.Createfails, the error is returned without logging. Many call sites defersaveStateSnapshot()and ignore its return value, so state persistence failures can become silent. Logging the create error here avoids losing that signal.
f, err := os.Create(r.statePath)
if err != nil {
return err
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/server/idle_controller.go:139
- In BeginRequest, the timer is stopped but its channel is not drained. If the timer fires concurrently with the wake completion / ctx cancellation, leaving the value in timer.C can cause unnecessary retention and subtle races. Use the standard Stop+drain pattern in both the
<-doneand<-ctx.Done()branches.
timer := time.NewTimer(timeout)
select {
case <-done:
timer.Stop()
if state == IdleStateStopping {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/server/idle_controller.go:139
time.NewTimeris stopped without draining its channel. If the timer fires concurrently with selecting<-done, this can leave a value intimer.C, which is a common source of subtle leaks and flaky behavior in loops. Draintimer.CwhenStop()reports it already fired.
timer := time.NewTimer(timeout)
select {
case <-done:
timer.Stop()
if state == IdleStateStopping {
internal/server/idle_controller.go:150
- In the
ctx.Done()branch, the timer is stopped but not drained. As with the<-donebranch, a concurrent timer fire can leave a value buffered ontimer.C. Drain whenStop()returns false to avoid accumulating unread timer events.
return ErrIdleWakeTimeout
case <-ctx.Done():
timer.Stop()
return ctx.Err()
…o race fixes (#58) Partial #19: the IdleController state machine and the ContainerLifecycle seam. Not yet wired into Service or Router, so proxy behavior is unchanged. Also, independent of #19 and the reason this was worth merging now: - fix(test): TestRouter_GetCertificate_RegistryStillServesHostScopedServices deployed a TLS service with no certificate paths, so the fall-through past the registry reached a real autocert.Manager and asked Let's Encrypt PRODUCTION for a certificate, blocking for its full internal timeout. That one test was 300 of the internal/server suite's 320 seconds, and it violated .claude/rules/testing.md outright. The suite is back to 23s. - fix(server): Service.MarshalJSON read s.active/s.rollout with no serviceLock while UpdateLoadBalancer writes them under it -- confirmed with a DATA RACE before the fix. Target.BeginHealthChecks wrote t.stateConsumer outside the inflight lock while the prober goroutine read it. - docs: dash is this fork's main branch; upstream mergeability is not a design constraint. Four deliberate departures from basecamp#228, each a defect in it: a failed stop rolls back to active rather than marking the service asleep anyway; a failed wake backs off instead of retrying per request; BeginRequest takes one deadline for the whole call rather than a fresh timer per iteration; a generation counter supersedes any in-flight lifecycle goroutine on any transition. Known hole tracked in zoolutions/kamal#62: kamal deploy prunes exited containers, and a sleeping container is exited. Cold-wake latency tracked in #59. Refs #19
…mand (#65) The Service half of scale-to-zero (#19): the request gate, the options, the persisted state, and the CLI surface. The Router still does not install a container lifecycle, so nothing sleeps yet -- that is the last chunk. The flag is --sleep-after, NOT --idle-timeout. `run --idle-timeout` already exists for HTTP keep-alive (config.go, DefaultIdleTimeout = 60s), and two flags sharing one name with two unrelated meanings is an operator trap no compiler catches. --sleep-after also puts the flag, the persisted state name, and the log lines in one vocabulary. The gem needs to know this: kamal#68 assumed --idle-timeout would be the name. Gate placement is the load-bearing decision. It sits in serviceRequestWithTarget, after handlePausedAndStoppedRequests and before target selection. Every gate above it -- basic auth, IP allow list, rate limit, redirects, request deadline, the ACME HTTP handler -- therefore runs first, so a blocked, throttled, unauthenticated or redirected request can never spend a container start. Upstream basecamp#228 put this a layer up in ServeHTTP, where an anonymous client, a rate-limited flood, or an ACME HTTP-01 challenge each start containers. It is also above target selection, so the request handed on is byte-for-byte the one that arrived, body still unread. Health checks are answered, never held and never allowed to wake: an uptime monitor polling /up would pin a service awake forever, and holding the probe would make a load balancer in front evict a service that is sleeping correctly. They report unhealthy once a wake has actually failed, so monitoring stops being told everything is fine while every real request 503s. The 503 body carries none of the wake error. That error holds container references and up to four kilobytes of daemon output, and the response is reachable by anyone who can open a connection, so it is logged instead. Refs are recomputed in UpdateLoadBalancer, not just initialize. initialize runs before a load balancer exists on a first deploy, so refs were empty and a "wake" succeeded having started nothing -- caught by a test asserting the helper actually derived some. A redeploy pointing at new containers lands here too, which is exactly when the controller needs telling. Normalize only defaults a zero wake-timeout. Defaulting anything <= 0 swallowed a negative, making its validation unreachable; a negative is a typo and is now rejected rather than silently turned into 30s. UnmarshalJSON records the restored idle state but never builds a controller: the lifecycle is nil at that point, so one built there could reach StopContainer on a nil interface. SetContainerLifecycle creates it after the whole state file is decoded. A state file written before this feature has no idle_state key, parses to active, and re-marshals byte-identically since every new key is omitempty. Refs #19
|
Hi @kevinmcconnell, would you be able to review this PR when you have a chance? All feedback so far has been addressed, and the implementation has been validated with real Docker and a Rails application. The corresponding Kamal configuration is in basecamp/kamal#1916. I’d especially appreciate your feedback on whether the overall opt-in, Docker-socket-based approach fits the direction of kamal-proxy. I’m happy to rework or split the implementation if that would make it easier to review. |
|
Hi @kevinmcconnell, just following up on this review request. Please let me know if there’s anything I can do to help move the review forward. |
Summary
Add opt-in scale-to-zero for low-traffic services. When configured, kamal-proxy stops write containers after an idle timeout, starts them on the next application request, waits for health readiness, and then forwards the held request.
This continues the work started by @martijnenco in #197. Thank you for providing the implementation that made this possible.
Behavior
--idle-timeoutContainer operations are isolated behind the small
ContainerLifecycleinterface. The initial implementation connects directly to the Docker socket because it is the smallest opt-in approach. This grants powerful host-level Docker access and is documented explicitly. A restricted host-side start/stop service can be added later without changing the idle controller.The design and Docker socket tradeoff were discussed in #222. The corresponding Kamal PR, basecamp/kamal#1916, configures idle settings and mounts the Docker socket only when the feature is enabled.
Validation
go test ./...go test -race ./internal/server ./internal/cmdgo vet ./...git diff --check origin/main...HEADThe implementation was also tested with real Docker and a Rails application on a 2 GB Ubuntu VPS:
Detailed results and reproduction instructions: https://docs.komagata.org/6456