Stop a slow host turning a login-guard test into a segfault (closes #186)

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 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 it fails on pass 1.

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, so
the overlap the assertion needs is a fact rather than a race won, and
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.

internal/database/webhook_db_manager_test.go

@@ -339,7 +339,12 @@ func TestWebhookDBManager_MultipleWebhooks(t *testing.T) {
 	var events []database.Event
 
 	require.NoError(t, db2.Find(&events).Error)
-	assert.Len(t, events, 1)
+
+	// require, not assert: this is exactly the regression the test
+	// guards, so the empty slice is the expected failure, and a
+	// non-fatal length check would index into it on the next line and
+	// panic the whole package test binary instead of failing here.
+	require.Len(t, events, 1)
 	assert.Equal(t, "PUT", events[0].Method)
 }
 

internal/middleware/loginguard.go

@@ -177,6 +177,26 @@ func newLoginGuard(
 // without verifying anything. The returned function releases the
 // slot and must be called exactly once.
 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.

internal/middleware/loginguard_test.go

@@ -29,6 +29,13 @@ const (
 
 	guardClient = "198.51.100.7"
 	guardUser   = "admin"
+
+	// racePasses is how many times a both-cases-ready select race is
+	// run. Each pass is an independent coin flip if the code under
+	// test does not settle the race itself, so at this N a
+	// regression is caught with probability 1 - 2^-N and the test
+	// still waits on nothing.
+	racePasses = 1000
 )
 
 // newGuard builds a guard with production-shaped defaults and the
@@ -224,6 +231,13 @@ func TestLoginGuard_SemaphoreBoundsConcurrentVerifications(
 	const (
 		concurrency = 2
 		workers     = 12
+
+		// rendezvousDeadlock is the deadlock guard described below.
+		// It is orders of magnitude longer than any scheduling delay,
+		// so it never decides the result, and well inside script/test's
+		// 30s timeout, so a wedge fails on the assertion instead of
+		// blowing the package timeout.
+		rendezvousDeadlock = 5 * time.Second
 	)
 
 	g := newGuard(middleware.LoginFailureMaxKeysConst, concurrency)
@@ -233,8 +247,29 @@ func TestLoginGuard_SemaphoreBoundsConcurrentVerifications(
 		inside  int
 		highest int
 		wg      sync.WaitGroup
+		once    sync.Once
 	)
 
+	// Slot holders rendezvous instead of sleeping. A sleep only makes
+	// overlap likely — on a host loaded enough to deschedule a
+	// goroutine for longer than the sleep, the workers can serialise
+	// and the maximum observed comes back as 1. Holding until the
+	// concurrency-th holder arrives makes the overlap the assertion
+	// needs a fact rather than a race won: the first holder cannot
+	// leave until a second one is inside with it.
+	overlapped := make(chan struct{})
+	closeOverlapped := func() {
+		once.Do(func() { close(overlapped) })
+	}
+
+	// Deadlock guard, not a timing margin: no assertion depends on
+	// its length and the healthy path closes overlapped in
+	// microseconds. It is here so that a guard which never admits two
+	// requests at once fails legibly on the assertion below instead
+	// of hanging until the package test timeout.
+	abandon := time.AfterFunc(rendezvousDeadlock, closeOverlapped)
+	defer abandon.Stop()
+
 	for range workers {
 		wg.Go(func() {
 			release, ok := g.AcquireForTest(context.Background())
@@ -251,11 +286,15 @@ func TestLoginGuard_SemaphoreBoundsConcurrentVerifications(
 				highest = inside
 			}
 
+			reached := inside == concurrency
+
 			mu.Unlock()
 
-			// Hold the slot long enough that the other workers are
-			// certainly contending for it.
-			time.Sleep(10 * time.Millisecond)
+			if reached {
+				closeOverlapped()
+			}
+
+			<-overlapped
 
 			mu.Lock()
 			inside--
@@ -278,6 +317,14 @@ func TestLoginGuard_SemaphoreBoundsConcurrentVerifications(
 // what happens when every slot is taken for longer than the wait: the
 // request is refused, so the caller answers 503 without allocating
 // another 64 MB hash.
+//
+// Neither half of this rides on the wait being long enough. The
+// refusal holds the only slot across the whole of the second call, so
+// there is no wait it could get lucky with — the wait fixes only how
+// long the refusal takes, not whether it happens. The reuse after
+// release is settled by acquire's non-blocking preamble, which is
+// pinned separately by TestLoginGuard_FreeSlotBeatsAnExpiredWait. So
+// the wait below is sized to keep the test quick, not to win a race.
 func TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing(
 	t *testing.T,
 ) {
@@ -305,13 +352,58 @@ func TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing(
 	release()
 
 	release, ok = g.AcquireForTest(context.Background())
-	assert.True(
+
+	// require, not assert: acquire returns a nil release alongside a
+	// false ok, so calling it after a non-fatal assertion turns one
+	// failed test into a segfault that takes the whole package test
+	// binary down. Every assertion whose value is dereferenced later
+	// has to stop the test.
+	require.True(
 		t, ok, "the slot must be reusable once released",
 	)
 
 	release()
 }
 
+// TestLoginGuard_FreeSlotBeatsAnExpiredWait is the determinism this
+// file used to lack. acquire selects over a slot send and a wait
+// timer, and Go chooses among ready cases uniformly at random, so a
+// call made after the timer had already fired was a coin flip: on a
+// loaded host the previous test's third acquire could be refused
+// with its slot standing free, and then dereference the nil release
+// it got back.
+//
+// The wait here is already elapsed on arrival, which is the worst
+// case that scheduling can produce, so a free slot must still be
+// granted every time. Without acquire's non-blocking preamble each
+// pass is an independent coin flip and the loop fails within a few
+// passes; with it the property holds by construction and no wall
+// clock is involved.
+func TestLoginGuard_FreeSlotBeatsAnExpiredWait(t *testing.T) {
+	t.Parallel()
+
+	g := middleware.NewLoginGuardForTest(
+		middleware.LoginRateLimitConst,
+		guardInterval,
+		middleware.LoginFailureMaxKeysConst,
+		1,
+		middleware.PasswordVerifyMaxWaitersConst,
+		0,
+	)
+
+	for pass := range racePasses {
+		release, ok := g.AcquireForTest(context.Background())
+		require.Truef(
+			t, ok,
+			"pass %d was refused a slot that was free; an expired "+
+				"wait must never beat an available slot",
+			pass,
+		)
+
+		release()
+	}
+}
+
 // TestLoginGuard_AcquireHonoursCancellation proves a client that
 // disconnects while queued frees its place immediately instead of
 // holding it for the full wait.
@@ -388,13 +480,20 @@ func TestLoginGuard_ShedsPastTheQueueCap(t *testing.T) {
 		neverElapses = time.Minute
 
 		// The probe carries its own deadline, so a guard that queues
-		// the probe instead of shedding it fails on the elapsed time
-		// rather than hanging until the package test timeout.
-		probeWait = 200 * time.Millisecond
-
-		// Shedding takes no measurable time; queueing takes the whole
-		// probeWait. Anything under half of it is unambiguous.
-		shedFast = probeWait / 2
+		// the probe instead of shedding it fails here rather than
+		// hanging until the package test timeout.
+		//
+		// This is a patience budget, not a margin to be won. A shed
+		// returns in microseconds and a probe that queued instead
+		// would not return for neverElapses, so the two are a whole
+		// minute apart and any budget between them separates them. It
+		// is set far above any scheduling stall a loaded host can
+		// produce, because the previous 200 ms — and the 100 ms
+		// elapsed-time assertion it fed — bounded the latency of a
+		// goroutine hand-off, which is a false red waiting to happen
+		// on the machine this suite runs on. What actually proves the
+		// probe was not queued is the queue depth asserted below.
+		probePatience = 5 * time.Second
 	)
 
 	g := middleware.NewLoginGuardForTest(
@@ -413,7 +512,7 @@ func TestLoginGuard_ShedsPastTheQueueCap(t *testing.T) {
 	defer release()
 	defer fillQueue(t, g, maxWaiters)()
 
-	got := probeQueueCap(g, probeWait)
+	got := probeQueueCap(g, probePatience)
 
 	require.NotNil(
 		t, got,
@@ -421,14 +520,9 @@ func TestLoginGuard_ShedsPastTheQueueCap(t *testing.T) {
 			"be queued; it must have been shed",
 	)
 	assert.False(
-		t, got.ok,
+		t, *got,
 		"a request arriving past the queue cap must be shed",
 	)
-	assert.Less(
-		t, got.elapsed, shedFast,
-		"shedding must be immediate; waiting for a place in the "+
-			"queue is the memory growth this bounds",
-	)
 	assert.Equal(
 		t, maxWaiters, g.QueuedWaitersForTest(),
 		"a shed request must not have grown the queue",
@@ -458,10 +552,14 @@ func fillQueue(
 		})
 	}
 
+	// Patience budget, not a margin: the waiters park in microseconds
+	// and nothing releases them, so the only way to exhaust this is a
+	// guard that never queues. One second is the same order as the
+	// scheduling stalls this suite has to survive, so it is not one.
 	require.Eventually(
 		t,
 		func() bool { return g.QueuedWaitersForTest() == n },
-		time.Second, time.Millisecond,
+		5*time.Second, time.Millisecond,
 		"the waiters must reach the queue before the cap is tested",
 	)
 
@@ -471,35 +569,30 @@ func fillQueue(
 	}
 }
 
-// probeResult is what the queue-cap probe reports: whether it got a
-// slot, and how long it took to find out.
-type probeResult struct {
-	ok      bool
-	elapsed time.Duration
-}
-
-// probeQueueCap acquires from another goroutine and reports the
-// result, or nil if the call was still blocked after wait.
+// probeQueueCap acquires from another goroutine and reports whether
+// it got a slot, or nil if the call was still blocked after wait.
 //
 // It runs off the test goroutine deliberately. Joining a full queue
 // is not cancellable by context — refusing to join is the property
 // under test — so a guard that fails this would otherwise hang the
 // package until the test timeout instead of failing here.
+//
+// It reports no elapsed time. Timing a goroutine hand-off measures
+// the host, not the guard, and the caller distinguishes shedding from
+// queueing by the queue depth instead.
 func probeQueueCap(
 	g *middleware.LoginGuard,
 	wait time.Duration,
-) *probeResult {
-	probed := make(chan probeResult, 1)
+) *bool {
+	probed := make(chan bool, 1)
 
 	go func() {
-		start := time.Now()
-
 		release, ok := g.AcquireForTest(context.Background())
 		if ok {
 			release()
 		}
 
-		probed <- probeResult{ok: ok, elapsed: time.Since(start)}
+		probed <- ok
 	}()
 
 	select {