Unverified Commit 2a4a81b9 authored by Minseong Choi's avatar Minseong Choi 💬
Browse files

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.
parent c14ed170

internal/api/api.go

+20 −5
Changes for internal/api/api.go: 20 added lines, 5 removed lines.
Original line number Diff line number Diff line
@@ -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

internal/api/api_test.go

+38 −0
Changes for internal/api/api_test.go: 38 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -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) {

internal/api/handlers_internal.go

+5 −1
Changes for internal/api/handlers_internal.go: 5 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -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()),

internal/api/handlers_user.go

+6 −1
Changes for internal/api/handlers_user.go: 6 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -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"})

}