fix(api): don't burn wake cooldown when refused at capacity
A wake refused by the §9.1 running-server cap returns 503, but the per-server cooldown was recorded before the cap check ran. A player held because the cluster was momentarily full would then also have to wait out the wake cooldown once a slot freed, even though their refused wake never actually flipped desiredState. Split cooldownLimiter.allow into allowed (peek, no record) and record (commit). Both wake paths now consult allowed for the 429, then call record only after SetDesiredState succeeds — so neither a 503 at_capacity nor a SetDesiredState error consumes the cooldown. The split is safe against the running cap, which counts CRD truth via ListServers and is independent of the limiter.
This commit is contained in:
4 files changed
+69
-7
No files matched your search
+20
-5
@@ -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
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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()),
|
||||
|
||||
@@ -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"})
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user