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

internal/middleware/loginguard.go

@@ -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.

internal/middleware/loginguard_test.go

@@ -29,6 +29,15 @@ const (
 
 	guardClient = "198.51.100.7"
 	guardUser   = "admin"
+
+	// racePasses is how many times a both-cases-ready select race is
+	// run. A pass can only go the wrong way once the zero-duration
+	// timer has fired, so the per-pass detection probability is
+	// somewhere below 1/2 rather than exactly it; the bound that
+	// matters is that passes are independent, so a regression that
+	// survives is exponentially unlikely in N. The test still waits
+	// on nothing.
+	racePasses = 1000
 )
 
 // newGuard builds a guard with production-shaped defaults and the
@@ -224,21 +233,71 @@ 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)
 
 	var (
-		mu      sync.Mutex
-		inside  int
-		highest int
-		wg      sync.WaitGroup
+		mu       sync.Mutex
+		inside   int
+		highest  int
+		wg       sync.WaitGroup
+		recorded sync.WaitGroup
+		once     sync.Once
 	)
 
+	// Slot holders rendezvous instead of sleeping, and they hold until
+	// every worker has been answered. A sleep only makes overlap
+	// likely — on a host loaded enough to deschedule a goroutine for
+	// longer than the sleep the workers serialise and the maximum
+	// observed comes back as 1 — so the rendezvous is what makes the
+	// overlap a fact rather than a race won.
+	//
+	// The barrier must not open at the concurrency-th holder, which
+	// would fix the lower bound at the cost of the upper one this test
+	// exists to enforce: holders would leave as soon as the count
+	// reached concurrency, so a guard admitting extra requests would
+	// let them arrive after the first holders had already left and
+	// highest would report concurrency however many were really let
+	// in. It opens instead once every worker's acquire has returned
+	// and any slot it won has been counted, so under a broken guard
+	// every admitted worker is inside simultaneously and highest is
+	// the true maximum. Under a correct guard the refused workers
+	// return within the guard's own wait, which decides nothing beyond
+	// how long that takes.
+	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 only way to reach it is a worker that never
+	// returns from acquire at all. It is here so that such a wedge
+	// fails legibly on the assertion below instead of hanging until
+	// the package test timeout.
+	abandon := time.AfterFunc(rendezvousDeadlock, closeOverlapped)
+	defer abandon.Stop()
+
+	recorded.Add(workers)
+
+	go func() {
+		recorded.Wait()
+		closeOverlapped()
+	}()
+
 	for range workers {
 		wg.Go(func() {
 			release, ok := g.AcquireForTest(context.Background())
 			if !ok {
+				recorded.Done()
+
 				return
 			}
 
@@ -253,9 +312,12 @@ func TestLoginGuard_SemaphoreBoundsConcurrentVerifications(
 
 			mu.Unlock()
 
-			// Hold the slot long enough that the other workers are
-			// certainly contending for it.
-			time.Sleep(10 * time.Millisecond)
+			// Counted before signalling, so the barrier can never open
+			// while an admitted worker is still on its way to being
+			// counted.
+			recorded.Done()
+
+			<-overlapped
 
 			mu.Lock()
 			inside--
@@ -278,6 +340,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 +375,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 +503,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,22 +535,17 @@ func TestLoginGuard_ShedsPastTheQueueCap(t *testing.T) {
 	defer release()
 	defer fillQueue(t, g, maxWaiters)()
 
-	got := probeQueueCap(g, probeWait)
+	granted, answered := probeQueueCap(g, probePatience)
 
-	require.NotNil(
-		t, got,
+	require.True(
+		t, answered,
 		"a request arriving past the queue cap is still waiting to "+
 			"be queued; it must have been shed",
 	)
 	assert.False(
-		t, got.ok,
+		t, granted,
 		"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 +575,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,41 +592,38 @@ 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. It reports, in
+// order, whether the call was granted a slot and whether it was
+// answered at all within wait; a call that never returned reports
+// false for both.
 //
 // 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, 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 {
 	case result := <-probed:
-		return &result
+		return result, true
 	case <-time.After(wait):
-		return nil
+		return false, false
 	}
 }