Stop a slow host turning a login-guard test into a segfault (closes #186)
All checks were successful
check / check (push) Successful in 2m50s

CI run 232 failed
TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing and then
took the whole internal/middleware test binary down with a SIGSEGV, on
a commit whose own gates were green. acquire returns (nil, false) on
every refusal path, the assertion on ok was non-fatal, and the next
line called the nil release. Two defects sit behind that, and the
second one is not confined to the test.

acquire selected over a slot send and an already-armed wait timer. Go
picks among ready cases uniformly at random, so a process descheduled
for longer than the wait sheds a request with slots standing free —
under load, which is exactly when shedding a login is least
defensible. A non-blocking preamble now takes a free slot before any
timer is armed, the same shape lifecycle.waitDone uses to settle its
own both-ready race. It cannot let a late arrival barge past a queued
waiter: a waiter can only be parked on a full buffer, and a release
refills that buffer from the head of the send queue under the channel
lock, so the preamble's send fails whenever anyone is waiting. Placing
it ahead of the queue admission also stops a request that never waits
from occupying a waiter's place.

The preamble does not consult ctx, so an already-cancelled request
with a free slot is now granted it rather than refused about half the
time. That is deliberate and matches waitDone; acquire's doc said
otherwise and has been corrected to describe what the code does.

The test's third acquire is therefore settled by construction rather
than by the wait being long enough, and
TestLoginGuard_FreeSlotBeatsAnExpiredWait pins that: 1000 passes with
a wait already elapsed on arrival, the worst case scheduling can
produce. Without the preamble each pass can go the wrong way and the
loop fails within a few passes; measured on the reverted-preamble
mutation, pass 2.

Assertions whose value is dereferenced afterwards are require, not
assert. A sweep of every test file found one sibling of the same
shape: webhook_db_manager_test.go checked a slice length non-fatally
and indexed it on the next line, so the regression it guards would
have surfaced as an index-out-of-range panic through
internal/database rather than as a failing test.

TestLoginGuard_SemaphoreBoundsConcurrentVerifications used a 10 ms
sleep to make two workers overlap and asserted the observed maximum
was exactly two. A sleep only makes overlap likely; on a host that can
deschedule a goroutine for longer than the sleep the workers serialise
and the maximum comes back as one. The slot holders now rendezvous
instead, and the barrier opens only once every worker's acquire has
returned and any slot it won has been counted — not at the
concurrency-th holder, which would fix the lower bound while
destroying the upper one the test exists to enforce, since holders
would leave before an over-admitting guard's extra workers had been
observed. The test no longer sleeps at all.

The queue-cap test in the same file held the two tightest wall-clock
margins in the repo: it asserted a shed request returned in under
100 ms, timed across a goroutine hand-off, and gave the probe 200 ms
to return at all. Both bounded host latency rather than guard
behaviour. The elapsed-time assertion is gone, since a shed request is
told from a queued one by the queue depth, which is a state fact; the
probe's budget is now five seconds against a queue wait of a minute,
which only a guard that queues can exhaust. Mutating the queue
admission back to a blocking send still fails the test.
This commit is contained in:
2026-08-18 01:41:36 +00:00
committed by sneak
parent 9313b0fb41
commit f932a86e8d
3 changed files with 195 additions and 44 deletions

View File

@@ -173,10 +173,38 @@ func newLoginGuard(
// acquire reserves a verification slot, waiting up to the guard's
// wait for one. It reports false when the queue of waiters is
// already full, when no slot became available in time, or when the
// request was cancelled first; the caller must then answer 503
// without verifying anything. The returned function releases the
// request was cancelled while waiting; the caller must then answer
// 503 without verifying anything. The returned function releases the
// slot and must be called exactly once.
//
// ctx is consulted only once the request has to wait: a slot that is
// free on arrival is handed out without looking at it, so an
// already-cancelled request can be granted one. That is deliberate
// and matches lifecycle.waitDone — the caller abandons the work on
// its own ctx and releases the slot immediately, so nothing is spent
// on it, and refusing instead would mean shedding a request with
// capacity standing free.
func (g *loginGuard) acquire(ctx context.Context) (func(), bool) {
// A free slot is taken before any timer is armed, and before a
// queue place is claimed: a request that never waits is not a
// waiter. Without this preamble the bounded select below can find
// its slot send and an already-expired timer ready at the same
// time, and Go picks among ready cases uniformly at random — so a
// process descheduled for longer than the wait sheds a request
// with slots standing free, which is precisely when shedding is
// least defensible.
//
// This cannot let a late arrival barge past a queued waiter. A
// waiter can only be parked on a FULL buffer, and a release
// refills that buffer from the head of the send queue under the
// channel lock, so the buffer never appears non-full while anyone
// is parked and this send fails whenever there is a waiter.
select {
case g.slots <- struct{}{}:
return func() { <-g.slots }, true
default:
}
// Shedding past the queue depth is what keeps waiting memory
// bounded; the wait alone only bounds how long one waiter holds
// its parsed form, not how many hold one at once.