Files
at-container-registry/pkg/auth/denial_counter_test.go
T
Evan JarrettandClaude Opus 5 985ebd3a5f auth: make the crew denial counter atomic
cacheDenial read denial_count, incremented it in Go, and wrote the result back.
Two overlapping denials for the same (hold, user) both read the same value and
both wrote the same value, so one increment vanished. The effect is that the
backoff ladder advances more slowly than configured, which means a denied client
keeps hammering the hold's PDS for longer than intended. Already reachable
across goroutines on one instance; routine with several behind a load balancer.

It is now a single INSERT ... ON CONFLICT DO UPDATE that increments in place.
next_retry_at moved into SQL as well, derived from the count the same statement
is producing, rather than computed in Go from a count that may already be stale
by the time the write lands. The CASE ladder is generated from
dbBackoffDurations so configuration still drives the backoff, and no request
data reaches the string.

The measured difference, with 20 concurrent denials: the old code recorded 12
where it should have recorded 21, losing 9. The new code loses none.

The surviving SELECT only picks a branch (first denial goes to memory only), so
a stale answer costs at most one skipped or one extra write, never a count.

Two implementation notes. datetime() truncates to whole seconds and the backoff
ladder is sub-second in tests, so timestamps use
strftime('%Y-%m-%dT%H:%M:%fZ', ...) instead; libSQL normalizes that to RFC 3339
and it scans back into time.Time with the right instant, which was verified
before relying on it. And a one-rung ladder emits a bare number rather than a
CASE, because "CASE ELSE x END" with no WHEN arm is a syntax error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-11 21:33:07 -05:00

195 lines
5.6 KiB
Go

package auth
import (
"database/sql"
"path/filepath"
"sync"
"testing"
"time"
"atcr.io/pkg/appview/db"
)
// concurrentTestDB returns a file-backed database.
//
// It must not be ":memory:" like setupTestDB: go-libsql gives each connection to
// an in-memory DSN its own private database, so concurrent goroutines would each
// see a different (empty) one and the test would prove nothing.
func concurrentTestDB(t *testing.T) *sql.DB {
t.Helper()
testDB, err := db.InitDB(filepath.Join(t.TempDir(), "auth.db"), db.LibsqlConfig{})
if err != nil {
t.Fatalf("InitDB: %v", err)
}
t.Cleanup(func() { testDB.Close() })
return testDB
}
// TestCacheDenialConcurrentIncrementsAreNotLost is the regression test for the
// read-modify-write in cacheDenial.
//
// The old implementation read denial_count, incremented it in Go, and wrote the
// result back. Two overlapping denials for the same (hold, user) both read the
// same value and both wrote the same value, so one increment vanished. The
// visible effect is that the backoff ladder advances more slowly than
// configured, meaning a denied client keeps hammering the hold's PDS for longer
// than intended.
//
// This was already reachable across goroutines on one instance and becomes
// routine with several instances behind a load balancer.
func TestCacheDenialConcurrentIncrementsAreNotLost(t *testing.T) {
testDB := concurrentTestDB(t)
remote := NewRemoteHoldAuthorizerWithBackoffs(
testDB, false,
time.Hour, // firstDenialBackoff
time.Hour, // cleanupInterval
time.Hour, // cleanupGracePeriod
[]time.Duration{time.Hour},
).(*RemoteHoldAuthorizer)
defer close(remote.stopCleanup)
const (
holdDID = "did:web:hold01.atcr.io"
userDID = "did:plc:user1"
)
// Two calls to get past the in-memory-only first denial and create the row.
if err := remote.cacheDenial(holdDID, userDID); err != nil {
t.Fatalf("first denial: %v", err)
}
if err := remote.cacheDenial(holdDID, userDID); err != nil {
t.Fatalf("second denial: %v", err)
}
before := denialCount(t, testDB, holdDID, userDID)
if before != 1 {
t.Fatalf("expected the first persisted denial to be count 1, got %d", before)
}
const concurrent = 20
var wg sync.WaitGroup
errs := make(chan error, concurrent)
start := make(chan struct{})
for range concurrent {
wg.Go(func() {
<-start
if err := remote.cacheDenial(holdDID, userDID); err != nil {
errs <- err
}
})
}
close(start)
wg.Wait()
close(errs)
for err := range errs {
t.Errorf("cacheDenial: %v", err)
}
got := denialCount(t, testDB, holdDID, userDID)
want := before + concurrent
if got != want {
t.Errorf("denial_count = %d, want %d: %d increments were lost to the read-modify-write",
got, want, want-got)
}
}
// TestCacheDenialBackoffMatchesLadder checks that next_retry_at, now computed in
// SQL from the count the same statement produces, still lands where
// getBackoffDuration says it should.
func TestCacheDenialBackoffMatchesLadder(t *testing.T) {
testDB := concurrentTestDB(t)
ladder := []time.Duration{2 * time.Second, 30 * time.Second, 5 * time.Minute}
remote := NewRemoteHoldAuthorizerWithBackoffs(
testDB, false, time.Hour, time.Hour, time.Hour, ladder,
).(*RemoteHoldAuthorizer)
defer close(remote.stopCleanup)
const (
holdDID = "did:web:hold01.atcr.io"
userDID = "did:plc:user1"
)
// First denial is in-memory only and creates no row.
if err := remote.cacheDenial(holdDID, userDID); err != nil {
t.Fatalf("first denial: %v", err)
}
// Each subsequent denial advances one rung, clamping at the last.
wantByCount := map[int]time.Duration{
1: ladder[0],
2: ladder[1],
3: ladder[2],
4: ladder[2], // clamped
5: ladder[2],
}
for count := 1; count <= 5; count++ {
issued := time.Now()
if err := remote.cacheDenial(holdDID, userDID); err != nil {
t.Fatalf("denial %d: %v", count, err)
}
var gotCount int
var nextRetry time.Time
err := testDB.QueryRow(
`SELECT denial_count, next_retry_at FROM hold_crew_denials WHERE hold_did = ? AND user_did = ?`,
holdDID, userDID,
).Scan(&gotCount, &nextRetry)
if err != nil {
t.Fatalf("read denial %d: %v", count, err)
}
if gotCount != count {
t.Fatalf("denial_count = %d, want %d", gotCount, count)
}
want := wantByCount[count]
actual := nextRetry.Sub(issued)
// Generous tolerance: the value is SQLite's clock, not Go's, and the
// column stores milliseconds.
if actual < want-2*time.Second || actual > want+2*time.Second {
t.Errorf("count %d: next_retry_at is %v out, want about %v", count, actual, want)
}
}
}
// TestCacheDenialBlocksAfterPersisting ties the new write back to the read path:
// a persisted denial must actually block.
func TestCacheDenialBlocksAfterPersisting(t *testing.T) {
testDB := concurrentTestDB(t)
remote := NewRemoteHoldAuthorizerWithBackoffs(
testDB, false, time.Hour, time.Hour, time.Hour,
[]time.Duration{time.Hour},
).(*RemoteHoldAuthorizer)
defer close(remote.stopCleanup)
const (
holdDID = "did:web:hold01.atcr.io"
userDID = "did:plc:user1"
)
_ = remote.cacheDenial(holdDID, userDID) // in-memory
_ = remote.cacheDenial(holdDID, userDID) // persisted
blocked, err := remote.isBlockedByDenialBackoff(holdDID, userDID)
if err != nil {
t.Fatalf("isBlockedByDenialBackoff: %v", err)
}
if !blocked {
t.Error("expected the user to be blocked by the persisted backoff")
}
}
func denialCount(t *testing.T, database *sql.DB, holdDID, userDID string) int {
t.Helper()
var n int
err := database.QueryRow(
`SELECT denial_count FROM hold_crew_denials WHERE hold_did = ? AND user_did = ?`,
holdDID, userDID,
).Scan(&n)
if err != nil {
t.Fatalf("read denial_count: %v", err)
}
return n
}