From f6ec78e2c8e084ce441c17cd48245d0b16ca8249 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 18 Aug 2026 05:01:13 +0200 Subject: [PATCH] Stop a slow host turning a login-guard test into a segfault (closes #186) --- internal/database/webhook_db_manager_test.go | 7 +- internal/middleware/loginguard.go | 32 ++- internal/middleware/loginguard_test.go | 200 +++++++++++++++---- 3 files changed, 195 insertions(+), 44 deletions(-) diff --git a/internal/database/webhook_db_manager_test.go b/internal/database/webhook_db_manager_test.go index 282d890..771f9e4 100644 --- a/internal/database/webhook_db_manager_test.go +++ b/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) } diff --git a/internal/middleware/loginguard.go b/internal/middleware/loginguard.go index 984a8d2..b9f6fe5 100644 --- a/internal/middleware/loginguard.go +++ b/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. diff --git a/internal/middleware/loginguard_test.go b/internal/middleware/loginguard_test.go index 452470a..fb2ac0c 100644 --- a/internal/middleware/loginguard_test.go +++ b/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 } }