Skip to content

feat(server): idle controller and container lifecycle seam for scale-to-zero (partial) - #58

Merged
mhenrixon merged 6 commits into
dashfrom
feature/scale-to-zero
Jul 29, 2026
Merged

feat(server): idle controller and container lifecycle seam for scale-to-zero (partial)#58
mhenrixon merged 6 commits into
dashfrom
feature/scale-to-zero

Conversation

@mhenrixon

Copy link
Copy Markdown
Collaborator

Status: partial — foundation + two fixes. Not the whole feature.

Issue #19 is size:L and it is genuinely multi-day. This PR lands the state machine, the docs decision, and a CI fix found along the way. The wiring into Service/Router/CLI is not here, so nothing about proxy behavior changes yet. docs/plans/2026-07-29-scale-to-zero.md carries the rest.

Three independent commits, reviewable separately:

1. docs: dash is the main branch (644c741)

The docs told every workflow to root branches off main "to keep them upstream-PR-able". Per your call, 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.

Updated: CLAUDE.md branch map, .claude/rules/git-workflow.md, /lfg, /plan, /github-review-pr. /plan also gained a rule to verify a "port basecamp#N" issue still points at an open PR — two have gone stale under us (#63basecamp#225, basecamp#197basecamp#228).

2. fix(test): the suite was making a real Let's Encrypt call (1e36b89)

TestRouter_GetCertificate_RegistryStillServesHostScopedServicesmy own test, from #50 — deployed a TLS service with no certificate paths. createCertManager handed it an autocert.Manager; the test called GetCertificate, the registry declined as designed, and the fall-through reached autocert, which went to the real Let's Encrypt production directory and blocked for its full internal timeout.

internal/server suite
before 321s
after 23.4s

That one test was 300 of the 320 seconds. It also violated .claude/rules/testing.md outright — "no real ACME/Let's Encrypt calls … never hit the real endpoint" — so CI was making an outbound request to a rate-limited production endpoint on every run. Found while timing the suite for #19, unrelated to it otherwise; merge this one regardless of what you decide about the rest.

3. feat(server): the idle controller (a063390)

IdleController + the ContainerLifecycle seam. Ports the design of basecamp#228 — which supersedes the closed basecamp#197 the issue names — with four deliberate departures, each a defect in basecamp#228:

# basecamp#228 Here
1 A failed stop still marks the service asleep Rolls back to active. Otherwise an unmounted socket or a pruned container is a permanent outage: every later request held for the wake timeout then 503, for containers running perfectly
2 A failed wake retries per request Backs off, doubling to 30s. Otherwise one container start per inbound request, forever
3 Fresh timer per loop iteration One deadline for the whole call. Otherwise a request arriving during a stop waits the full timeout for the stop and again for the start — twice the documented bound
4 Per-wake token Generation counter, so any transition supersedes any in-flight lifecycle goroutine — a deploy landing mid-wake is not overwritten

Coalescing falls out of the mutex rather than extra machinery: startWakeLocked flips the state while still holding the lock, so 20 concurrent requests produce exactly one start.

Test plan

13 controller tests, -race at -count=3, no Docker and no network (fakeLifecycle counts atomic.Int64). Highlights:

  • TestIdleController_CoalescesConcurrentWakes — 20 goroutines, starts == 1
  • TestIdleController_StaysAwakeWhenContainersCannotBeStopped — the inverse of Add opt-in scale-to-zero for idle services basecamp/kamal-proxy#228's behavior
  • TestIdleController_DoesNotSleepWithRequestsInFlight — the WebSocket/SSE guarantee at unit level; one unreleased BeginRequest means no stop, ever
  • TestIdleController_BacksOffAfterAFailedWake — three requests, exactly one start attempt
  • TestIdleController_ResetSupersedesAnInFlightWake — the generation token
  • TestIdleState_NamesRoundTripstopping/waking fold to sleeping; unknown and empty → active
  • make test, go vet ./..., gofmt -l internal/ cmd/ clean; go test -race ./internal/server ./internal/cmd — 1143 pass

Deviations & judgment calls

  • The issue's premise was stale and the answer differed from last time. R5: Scale-to-zero idle services #19 says "port Add idle container support (scale-to-zero) basecamp/kamal-proxy#197 (open)". Add idle container support (scale-to-zero) basecamp/kamal-proxy#197 is closed, superseded by Add opt-in scale-to-zero for idle services basecamp/kamal-proxy#228. Unlike R4: On-demand TLS with ask endpoint #13 — where upstream had already merged and waiting was free — Add opt-in scale-to-zero for idle services basecamp/kamal-proxy#228 is only ready for review with no human maintainer review yet, so waiting has no known end. You chose to port.
  • Scope is partial and I am saying so rather than padding it. The remaining work is the Docker client, the Service request gate, Router preflight + restore, LoadBalancer suspend/resume, persistence, and CLI flags — roughly 25 more tests. The plan is committed so none of the reasoning has to be rediscovered.
  • Known hole, mitigated not fixed — this is the one to read. kamal deploy runs prune on every deploy, and Kamal::Commands::Prune#app_containers filters status=exited and pipes through tail -n +6 into docker rm. A sleeping container is exited. Past five stopped containers for a service, a sleeping one becomes a removal candidate; after that every wake 404s and the service 503s permanently. The deploy-time preflight cannot prevent it — it proves the reference resolves at deploy time. The backoff in this PR turns it from a silent retry storm into a visible failure. The real fix is gem-side in ../kamal (exclude proxy-managed containers from the prune filter, or pass a stable label selector) and needs its own issue.
  • The security tradeoff is real and inherited deliberately. Mounting /var/run/docker.sock into the internet-facing proxy is root-equivalent on the host; an RCE in kamal-proxy becomes host compromise. Opt-in, and only mounted when the feature is enabled. Upstream's maintainer judged that acceptable in discussion Passenger-style idle shutdown and on-demand wake-up for hosting many low-traffic applications basecamp/kamal-proxy#222, and the ContainerLifecycle interface is the seam where a restricted host-side start/stop service replaces it without the controller changing. Recorded in ROADMAP.md so the next reader meets it with the anchor.
  • Built in a worktree, not the main tree. The working tree had moved to feature/observability-batch with uncommitted work on R5: Observability batch (log format, OTel traceparent, metrics excludes) #20 — a parallel session. I did not touch it; .claude/worktrees/issue-19-scale-to-zero is where this was built.
  • Unmeasured: Add opt-in scale-to-zero for idle services basecamp/kamal-proxy#228 reports a 3.15s cold wake for a real Rails app on a VPS. This implementation has not been measured, and that number must not be repeated as if it had been.
  • Three pre-existing dash bugs the plan surfaced, none fixed here: WaitUntilHealthy reads waitForHealthyContext unlocked (load_balancer.go:153); BeginHealthChecks writes stateConsumer unsynchronized (target.go:285); Service.MarshalJSON reads s.active/s.rollout with no serviceLock. The third becomes far more reachable once sleep/wake persists state, so it must be fixed before the wiring lands.

Refs #19

…onstraint

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 (basecamp#197) is
closed, superseded by basecamp#228, whose Docker-socket-behind-a-lifecycle-interface
architecture the upstream maintainer blessed in discussion basecamp#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 basecamp#225, basecamp#197 to basecamp#228).
…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#228 -- which supersedes the closed
basecamp#197 the issue names -- adapted to dash, with four deliberate departures:

A failed stop rolls the service back to active. basecamp#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. basecamp#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
… 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.
…tine 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
@mhenrixon

Copy link
Copy Markdown
Collaborator Author

Follow-ups now tracked, and the third race is fixed in 67c1f69:

67c1f69 fixes two of the three races from the plan. The third, waitForHealthyContext, is deliberately not changed: it is written once in NewLoadBalancer before publication and never again, so it is not a race today. It becomes one the moment ResumeFromSleep re-arms it, and the lock belongs in that commit where the second writer appears.

@mhenrixon
mhenrixon marked this pull request as ready for review July 29, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant