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..c0d6cf4 100644 --- a/internal/middleware/loginguard.go +++ b/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. diff --git a/internal/middleware/loginguard_test.go b/internal/middleware/loginguard_test.go index 452470a..08486f6 100644 --- a/internal/middleware/loginguard_test.go +++ b/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.