diff --git a/internal/api/api.go b/internal/api/api.go index 60e48ef..38778fa 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -377,21 +377,36 @@ type cooldownLimiter struct { window time.Duration } -// allow reports whether name may wake now, recording the attempt when allowed. -func (c *cooldownLimiter) allow(name string, window time.Duration) bool { +// allowed reports whether name may wake now WITHOUT recording the attempt. A +// non-positive window disables the throttle. Splitting the check (allowed) from +// the commit (record) lets the wake path consult the cooldown for its 429 before +// a downstream gate — the §9.1 running-cap 503 — decides whether the wake will +// actually happen, so a wake refused at capacity never burns the per-server +// cooldown. +func (c *cooldownLimiter) allowed(name string, window time.Duration) bool { if window <= 0 { return true } c.mu.Lock() defer c.mu.Unlock() - t := c.now() - if last, ok := c.last[name]; ok && t.Sub(last) < window { + if last, ok := c.last[name]; ok && c.now().Sub(last) < window { return false } - c.last[name] = t return true } +// record starts name's cooldown at the current time. The wake path calls it only +// after the wake actually flips desiredState, so neither a 503 at_capacity nor a +// SetDesiredState error consumes the cooldown. allowed→record is deliberately not +// atomic: like the running-cap above, the cooldown is a soft throttle (a burst of +// truly concurrent wakes may each pass allowed before any records), which is +// harmless because SetDesiredState is idempotent. +func (c *cooldownLimiter) record(name string) { + c.mu.Lock() + defer c.mu.Unlock() + c.last[name] = c.now() +} + // ---- running-server cap ---- // withinRunningCap reports whether waking info's server is allowed under the diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 912dd24..f70c6f7 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -977,6 +977,44 @@ func TestWakeRunningCap(t *testing.T) { }) } +// TestWakeCooldownNotBurnedAtCapacity is the §9.1 regression guard for the +// cooldown/cap ordering: a wake the running-cap refuses with 503 must NOT start +// the per-server cooldown. Otherwise a player held because the cluster was +// momentarily full would, once a slot frees, still be made to wait out a 30s +// cooldown their refused wake never earned. With the clock frozen, a 503 followed +// by the same server waking the instant capacity frees must return 202, not 429. +func TestWakeCooldownNotBurnedAtCapacity(t *testing.T) { + cl := newFakeCluster() + target := &ServerInfo{Name: "survival", AutostartPolicy: "public", + DesiredState: string(v1alpha1.DesiredStopped)} + cl.byName["survival"] = target + cl.list = []ServerInfo{*target, {Name: "other", DesiredState: string(v1alpha1.DesiredRunning)}} + api := newTestAPI(newFakeRepo(), cl) + api.WakeCooldown = time.Minute + api.MaxRunningServers = 1 + api.External = staticExternal{p: &Principal{UserID: "u", Role: "user"}} + h := api.ExternalHandler() + + // The cluster is full (1 running == cap): the wake is refused with 503 and must + // leave the cooldown unstarted. + if w := do(h, "POST", "/api/v1/servers/survival/wake", "", nil); w.Code != http.StatusServiceUnavailable { + t.Fatalf("at-capacity wake code = %d, want 503", w.Code) + } + if _, set := cl.desired["survival"]; set { + t.Fatal("desiredState must not change when refused at capacity") + } + + // A slot frees (the other server is gone). The same server, same frozen clock, + // must now wake — a 429 here would prove the 503 had burned the cooldown. + cl.list = []ServerInfo{*target} + if w := do(h, "POST", "/api/v1/servers/survival/wake", "", nil); w.Code != http.StatusAccepted { + t.Fatalf("post-capacity wake code = %d, want 202 (the 503 must not burn the cooldown)", w.Code) + } + if cl.desired["survival"] != v1alpha1.DesiredRunning { + t.Fatalf("desired = %q, want Running", cl.desired["survival"]) + } +} + // ---- Zero-Trust admin boundary (§14) ---- func TestAdminBoundary(t *testing.T) { diff --git a/internal/api/handlers_internal.go b/internal/api/handlers_internal.go index 6f6cf99..a9c96c8 100644 --- a/internal/api/handlers_internal.go +++ b/internal/api/handlers_internal.go @@ -148,7 +148,7 @@ func (a *API) handleInternalWake(w http.ResponseWriter, r *http.Request) { writeError(w, r, err) return } - if !a.limiter().allow(name, a.WakeCooldown) { + if !a.limiter().allowed(name, a.WakeCooldown) { writeError(w, r, newError(http.StatusTooManyRequests, "cooldown", "wake is cooling down, retry shortly")) return } @@ -170,6 +170,10 @@ func (a *API) handleInternalWake(w http.ResponseWriter, r *http.Request) { writeError(w, r, err) return } + // Consume the shared per-server cooldown only after the wake flips, so a join + // the cap held with 503 (or a SetDesiredState error) leaves the cooldown + // untouched and the next join attempt is not also throttled. + a.limiter().record(name) _ = a.Repo.Audit(r.Context(), AuditEntry{ Actor: "velocity", Source: "internal", Action: "wake", ServerName: name, RequestID: requestIDFromContext(r.Context()), diff --git a/internal/api/handlers_user.go b/internal/api/handlers_user.go index 070bb11..93d3ed6 100644 --- a/internal/api/handlers_user.go +++ b/internal/api/handlers_user.go @@ -39,7 +39,7 @@ func (a *API) handleWake(w http.ResponseWriter, r *http.Request) { writeError(w, r, err) return } - if !a.limiter().allow(name, a.WakeCooldown) { + if !a.limiter().allowed(name, a.WakeCooldown) { writeError(w, r, newError(http.StatusTooManyRequests, "cooldown", "wake is cooling down, retry shortly")) return } @@ -60,6 +60,11 @@ func (a *API) handleWake(w http.ResponseWriter, r *http.Request) { writeError(w, r, err) return } + // The wake actually flipped, so consume the per-server cooldown only now: a 503 + // at_capacity or the SetDesiredState failure above must not burn it (a player + // held at capacity should retry the instant a slot frees, not wait out a + // cooldown their refused wake never earned). + a.limiter().record(name) a.audit(r, p.Email, "wake", name) writeJSON(w, http.StatusAccepted, map[string]any{"name": name, "desiredState": "Running"}) }