Replace atomic.Pointer for readbility

user/access_cache.go

@@ -3,105 +3,101 @@ package user
 import (
 	"regexp"
 	"strings"
-	"sync/atomic"
+	"sync"
 
 	"heckel.io/ntfy/v2/db"
 )
 
-// aclEntry mirrors one user_access row in the in-memory snapshot.
+// aclCache is an in-memory index over the entire user_access table.
+//
+// exact[username][escapedTopic] returns the matching entry in O(1) for the common
+// case where the requested topic appears verbatim in some rule. The key is the
+// stored form of the topic (i.e. with \_ escapes), so Lookup escapes incoming
+// topics through escapeUnderscore before probing.
+//
+// wildcard[username] is the linear-scan list of %-bearing rules for that user.
+// Walked per request; trivially small in practice. Wildcards are NOT u_everyone-
+// only -- any user can create them.
+type aclCache struct {
+	exact    map[string]map[string]aclEntry
+	wildcard map[string][]aclEntry
+	mu       sync.RWMutex // Protect exact and wildcard
+}
+
+// aclEntry mirrors one user_access row in the in-memory cache.
 //
 // topic is the raw stored value: it may contain \_ escapes (for literal underscores)
 // and % wildcards (translated from user-supplied *). For exact-match entries (no %)
-// matcher is nil and the entry is keyed by topic in aclSnapshot.exact. For wildcard
+// matcher is nil and the entry is keyed by topic in aclCache.exact. For wildcard
 // entries (with %) matcher is the pre-compiled regex equivalent of the LIKE pattern.
 type aclEntry struct {
 	topic   string
 	read    bool
 	write   bool
-	matcher *regexp.Regexp
-}
-
-// aclSnapshot is an immutable indexed form of the entire user_access table.
-//
-// exact[userName][escapedTopic] returns the matching entry in O(1) for the common
-// case where the requested topic appears verbatim in some rule. The key is the
-// stored form of the topic (i.e. with \_ escapes), so callers must pass topics
-// through escapeUnderscore before probing.
-//
-// wildcards[userName] is the linear scan list of %-bearing rules for that user.
-// Walked per request; trivially small in practice. Wildcards are NOT u_everyone-
-// only -- any user can create them.
-type aclSnapshot struct {
-	exact     map[string]map[string]aclEntry
-	wildcards map[string][]aclEntry
-}
-
-// aclCache holds the current snapshot behind an atomic pointer so that the hot
-// path (Lookup) is lock-free. reload builds a fresh snapshot off the request
-// path and atomically swaps the pointer; the old snapshot is GC'd once in-flight
-// Lookups release their references.
-//
-// A nil receiver behaves as if no snapshot were loaded -- Lookup returns
-// found=false, which the caller then resolves via DefaultAccess. This keeps
-// tests and edge cases (e.g. early-startup) safe.
-type aclCache struct {
-	snap atomic.Pointer[aclSnapshot]
+	matcher *regexp.Regexp //  Nil for exact entries
 }
 
 func newAccessCache() *aclCache {
-	return &aclCache{}
+	return &aclCache{
+		exact:    make(map[string]map[string]aclEntry),
+		wildcard: make(map[string][]aclEntry),
+	}
 }
 
-// reload runs the bulk-load query against the primary and atomically swaps in
-// a fresh snapshot. The primary is used (not ReadOnly) so a reload immediately
-// after an ACL mutation sees the freshly-written rows without replica lag.
+// reload runs the bulk-load query against the primary and swaps in freshly-built
+// exact and wildcard maps under the write lock. The primary is used (not
+// ReadOnly) so a reload immediately after an ACL mutation sees the freshly-
+// written rows without replica lag.
 func (c *aclCache) reload(d *db.DB, query string) error {
 	rows, err := d.Query(query)
 	if err != nil {
 		return err
 	}
 	defer rows.Close()
-	snap := &aclSnapshot{
-		exact:     make(map[string]map[string]aclEntry),
-		wildcards: make(map[string][]aclEntry),
-	}
+	exact := make(map[string]map[string]aclEntry)
+	wildcards := make(map[string][]aclEntry)
 	for rows.Next() {
-		var userName string
+		var username string
 		var entry aclEntry
-		if err := rows.Scan(&userName, &entry.topic, &entry.read, &entry.write); err != nil {
+		if err := rows.Scan(&username, &entry.topic, &entry.read, &entry.write); err != nil {
 			return err
 		}
 		if strings.Contains(entry.topic, "%") {
-			entry.matcher = compileLikeToRegex(entry.topic)
-			snap.wildcards[userName] = append(snap.wildcards[userName], entry)
-		} else {
-			if snap.exact[userName] == nil {
-				snap.exact[userName] = make(map[string]aclEntry)
+			re, err := compileLikeToRegex(entry.topic)
+			if err != nil {
+				return err
 			}
-			snap.exact[userName][entry.topic] = entry
+			entry.matcher = re
+			wildcards[username] = append(wildcards[username], entry)
+		} else {
+			if exact[username] == nil {
+				exact[username] = make(map[string]aclEntry)
+			}
+			exact[username][entry.topic] = entry
 		}
 	}
 	if err := rows.Err(); err != nil {
 		return err
 	}
-	c.snap.Store(snap)
+	c.mu.Lock()
+	c.exact = exact
+	c.wildcard = wildcards
+	c.mu.Unlock()
 	return nil
 }
 
 // Lookup returns the effective (read, write, found) permission for the given
 // (username, topic), preserving the priority ordering of the original SQL query:
-//   1. specific user beats Everyone
-//   2. longer pattern beats shorter (more specific wins)
-//   3. write beats read at equal length (write is "stronger")
+//  1. specific user beats Everyone
+//  2. longer pattern beats shorter (more specific wins)
+//  3. write beats read at equal length (write is "stronger")
 func (c *aclCache) Lookup(usernameOrEveryone, topic string) (read, write, found bool) {
 	if c == nil {
 		return false, false, false
 	}
-	snap := c.snap.Load()
-	if snap == nil {
-		return false, false, false
-	}
-	// Pre-compute the escaped form once: exact-match keys in the snapshot are
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+	// Pre-compute the escaped form once: exact-match keys in the cache are
 	// stored as toSQLWildcard would emit them (literal _ -> \_), so the
 	// incoming topic must be escaped the same way before map lookup.
 	escaped := escapeUnderscore(topic)
@@ -109,28 +105,28 @@ func (c *aclCache) Lookup(usernameOrEveryone, topic string) (read, write, found
 	// Specific user takes priority over Everyone. Skip the first lookup when
 	// the request is already anonymous to avoid scanning the same map twice.
 	if usernameOrEveryone != Everyone {
-		if e, ok := pickBest(snap, usernameOrEveryone, topic, escaped); ok {
+		if e, ok := c.pickBestLocked(usernameOrEveryone, topic, escaped); ok {
 			return e.read, e.write, true
 		}
 	}
-	if e, ok := pickBest(snap, Everyone, topic, escaped); ok {
+	if e, ok := c.pickBestLocked(Everyone, topic, escaped); ok {
 		return e.read, e.write, true
 	}
 	return false, false, false
 }
 
-// pickBest returns the highest-priority entry for a single user, combining the
-// exact-match O(1) probe with a linear scan over the (usually empty or tiny)
-// wildcard list. Priority within a user: longer pattern wins; write wins ties.
-func pickBest(snap *aclSnapshot, userName, topic, escaped string) (aclEntry, bool) {
+// pickBestLocked returns the highest-priority entry for a single user, combining
+// the exact-match O(1) probe with a linear scan over the (usually empty or tiny)
+// wildcard list. Caller must hold c.mu (RLock is sufficient).
+func (c *aclCache) pickBestLocked(username, topic, escaped string) (aclEntry, bool) {
 	var best aclEntry
 	var found bool
-	if m, ok := snap.exact[userName]; ok {
+	if m, ok := c.exact[username]; ok {
 		if e, ok := m[escaped]; ok {
 			best, found = e, true
 		}
 	}
-	for _, w := range snap.wildcards[userName] {
+	for _, w := range c.wildcard[username] {
 		if !w.matcher.MatchString(topic) {
 			continue
 		}
@@ -158,7 +154,7 @@ func better(a, b aclEntry) bool {
 // \_ is a literal underscore; no other backslashes occur. Topics themselves are
 // restricted to [A-Za-z0-9_-] (see AllowedTopic), so neither % nor stray
 // backslashes appear in user-supplied input.
-func compileLikeToRegex(pattern string) *regexp.Regexp {
+func compileLikeToRegex(pattern string) (*regexp.Regexp, error) {
 	var sb strings.Builder
 	sb.WriteString("^")
 	i := 0
@@ -176,5 +172,5 @@ func compileLikeToRegex(pattern string) *regexp.Regexp {
 		}
 	}
 	sb.WriteString("$")
-	return regexp.MustCompile(sb.String())
+	return regexp.Compile(sb.String())
 }

user/access_cache_test.go

@@ -1,6 +1,7 @@
 package user
 
 import (
+	"regexp"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -14,21 +15,21 @@ import (
 // forEachStoreBackend.
 
 func TestCompileLikeToRegex_Exact(t *testing.T) {
-	r := compileLikeToRegex("foo")
+	r := mustCompileLikeToRegex(t, "foo")
 	require.True(t, r.MatchString("foo"))
 	require.False(t, r.MatchString("foox"))
 	require.False(t, r.MatchString("xfoo"))
 }
 
 func TestCompileLikeToRegex_TrailingPercent(t *testing.T) {
-	r := compileLikeToRegex("up%")
+	r := mustCompileLikeToRegex(t, "up%")
 	require.True(t, r.MatchString("up"))
 	require.True(t, r.MatchString("up123"))
 	require.False(t, r.MatchString("xup"))
 }
 
 func TestCompileLikeToRegex_LeadingAndEmbeddedPercent(t *testing.T) {
-	r := compileLikeToRegex("%test%")
+	r := mustCompileLikeToRegex(t, "%test%")
 	require.True(t, r.MatchString("test"))
 	require.True(t, r.MatchString("mytest"))
 	require.True(t, r.MatchString("testxxx"))
@@ -39,7 +40,7 @@ func TestCompileLikeToRegex_LeadingAndEmbeddedPercent(t *testing.T) {
 func TestCompileLikeToRegex_EscapedUnderscore(t *testing.T) {
 	// "my\_topic" is the stored form of a literal "my_topic" -- the underscore
 	// must match itself, NOT act as a SQL one-character wildcard.
-	r := compileLikeToRegex(`my\_topic`)
+	r := mustCompileLikeToRegex(t, `my\_topic`)
 	require.True(t, r.MatchString("my_topic"))
 	require.False(t, r.MatchString("myXtopic"))
 	require.False(t, r.MatchString("mytopic"))
@@ -48,7 +49,7 @@ func TestCompileLikeToRegex_EscapedUnderscore(t *testing.T) {
 func TestCompileLikeToRegex_EscapedUnderscoreAdjacentToPercent(t *testing.T) {
 	// "nz\_vip\_%" is the stored form of "nz_vip_*" -- literal "nz_vip_" prefix
 	// followed by any suffix.
-	r := compileLikeToRegex(`nz\_vip\_%`)
+	r := mustCompileLikeToRegex(t, `nz\_vip\_%`)
 	require.True(t, r.MatchString("nz_vip_"))
 	require.True(t, r.MatchString("nz_vip_alpha"))
 	require.False(t, r.MatchString("nz_vipX"))
@@ -58,7 +59,7 @@ func TestCompileLikeToRegex_EscapedUnderscoreAdjacentToPercent(t *testing.T) {
 func TestCompileLikeToRegex_RegexMetaCharsInTopic(t *testing.T) {
 	// Topics in ntfy can include '-', which is benign, but make sure
 	// regex metacharacters in the pattern are escaped properly anyway.
-	r := compileLikeToRegex("foo-bar")
+	r := mustCompileLikeToRegex(t, "foo-bar")
 	require.True(t, r.MatchString("foo-bar"))
 	require.False(t, r.MatchString("foo.bar")) // would match if '-' leaked into a character class
 }
@@ -72,8 +73,9 @@ func TestACLCache_LookupOnNilReceiverSafe(t *testing.T) {
 }
 
 func TestACLCache_LookupBeforeReload(t *testing.T) {
-	// Before reload the snapshot pointer is nil. The cache treats this as
-	// "no rule found", which the caller resolves via DefaultAccess.
+	// A freshly-constructed cache has empty exact and wildcards maps. The
+	// cache treats this as "no rule found", which the caller resolves via
+	// DefaultAccess.
 	c := newAccessCache()
 	read, write, found := c.Lookup("phil", "mytopic")
 	require.False(t, found)
@@ -83,9 +85,9 @@ func TestACLCache_LookupBeforeReload(t *testing.T) {
 
 func TestACLCache_ExactMatchHit(t *testing.T) {
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: "phil", topic: "mytopic", read: true, write: true},
-	}))
+	})
 	read, write, found := c.Lookup("phil", "mytopic")
 	require.True(t, found)
 	require.True(t, read)
@@ -94,9 +96,9 @@ func TestACLCache_ExactMatchHit(t *testing.T) {
 
 func TestACLCache_ExactMatchMiss(t *testing.T) {
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: "phil", topic: "mytopic", read: true, write: true},
-	}))
+	})
 	_, _, found := c.Lookup("phil", "othertopic")
 	require.False(t, found)
 }
@@ -105,9 +107,9 @@ func TestACLCache_LiteralUnderscoreExactMatch(t *testing.T) {
 	// Stored as "my\_topic" (toSQLWildcard of "my_topic"). A literal underscore
 	// in the requested topic must match, while any other single char must not.
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: "phil", topic: `my\_topic`, read: true, write: false},
-	}))
+	})
 	read, write, found := c.Lookup("phil", "my_topic")
 	require.True(t, found)
 	require.True(t, read)
@@ -119,9 +121,9 @@ func TestACLCache_LiteralUnderscoreExactMatch(t *testing.T) {
 
 func TestACLCache_WildcardMatch(t *testing.T) {
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: Everyone, topic: "up%", read: false, write: true},
-	}))
+	})
 	read, write, found := c.Lookup("phil", "up42")
 	require.True(t, found)
 	require.False(t, read)
@@ -130,10 +132,10 @@ func TestACLCache_WildcardMatch(t *testing.T) {
 
 func TestACLCache_SpecificUserBeatsEveryone(t *testing.T) {
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: Everyone, topic: "mytopic", read: true, write: false},
 		{user: "phil", topic: "mytopic", read: false, write: false}, // deny-all for phil
-	}))
+	})
 	read, write, found := c.Lookup("phil", "mytopic")
 	require.True(t, found)
 	require.False(t, read)
@@ -142,9 +144,9 @@ func TestACLCache_SpecificUserBeatsEveryone(t *testing.T) {
 
 func TestACLCache_AnonymousReadsEveryone(t *testing.T) {
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: Everyone, topic: "announcements", read: true, write: false},
-	}))
+	})
 	read, write, found := c.Lookup(Everyone, "announcements")
 	require.True(t, found)
 	require.True(t, read)
@@ -155,10 +157,10 @@ func TestACLCache_LongerPatternWinsForSameUser(t *testing.T) {
 	// Both rules belong to the same user (Everyone). The more specific (longer)
 	// "mytopic%" should beat the catch-all "%".
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: Everyone, topic: "%", read: true, write: false},
 		{user: Everyone, topic: "mytopic%", read: true, write: true},
-	}))
+	})
 	read, write, found := c.Lookup(Everyone, "mytopicX")
 	require.True(t, found)
 	require.True(t, read)
@@ -167,36 +169,31 @@ func TestACLCache_LongerPatternWinsForSameUser(t *testing.T) {
 
 func TestACLCache_WriteBeatsReadAtEqualLength(t *testing.T) {
 	// Two wildcard rules of identical length for the same user. The write rule
-	// should win the tie-break.
+	// should win the tie-break. The two-rows-with-same-topic shape is
+	// impossible via real upsert (pkey would conflict), so we inject the entries
+	// directly into the cache's wildcard slice.
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
-		{user: Everyone, topic: "ab%", read: true, write: false},
-		{user: Everyone, topic: "ab%", read: false, write: true}, // synthesized; impossible via real upsert but exercises the tie-break
-	}))
-	// One of the two will be the surviving exact-key entry (map collision keeps last);
-	// but the wildcard slice is what we want to exercise. Inject two wildcard entries
-	// directly to force the tie-break path.
-	c.snap.Store(&aclSnapshot{
-		exact: map[string]map[string]aclEntry{},
-		wildcards: map[string][]aclEntry{
-			Everyone: {
-				{topic: "ab%", read: true, write: false, matcher: compileLikeToRegex("ab%")},
-				{topic: "ab%", read: false, write: true, matcher: compileLikeToRegex("ab%")},
-			},
+	c.mu.Lock()
+	c.exact = map[string]map[string]aclEntry{}
+	c.wildcard = map[string][]aclEntry{
+		Everyone: {
+			{topic: "ab%", read: true, write: false, matcher: mustCompileLikeToRegex(t, "ab%")},
+			{topic: "ab%", read: false, write: true, matcher: mustCompileLikeToRegex(t, "ab%")},
 		},
-	})
+	}
+	c.mu.Unlock()
 	_, write, found := c.Lookup(Everyone, "abc")
 	require.True(t, found)
 	require.True(t, write)
 }
 
 func TestACLCache_ConcurrentLookupAndReload(t *testing.T) {
-	// Atomic-pointer swap must be safe under concurrent reads. The race detector
+	// Lock-based swap must be safe under concurrent reads. The race detector
 	// catches any unsafe shared mutation.
 	c := newAccessCache()
-	c.snap.Store(buildSnapshot(t, []rawACLRow{
+	loadCache(t, c, []rawACLRow{
 		{user: Everyone, topic: "mytopic", read: true, write: true},
-	}))
+	})
 
 	var stop atomic.Bool
 	var wg sync.WaitGroup
@@ -210,17 +207,17 @@ func TestACLCache_ConcurrentLookupAndReload(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		for i := 0; i < 100; i++ {
-			c.snap.Store(buildSnapshot(t, []rawACLRow{
+			loadCache(t, c, []rawACLRow{
 				{user: Everyone, topic: "mytopic", read: i%2 == 0, write: i%2 == 1},
-			}))
+			})
 		}
 		stop.Store(true)
 	}()
 	wg.Wait()
 }
 
-// rawACLRow + buildSnapshot mirror the rows that reload would Scan from the DB
-// but avoid actually opening a DB for these unit tests.
+// rawACLRow models the rows that reload would Scan from the DB but avoids
+// actually opening a DB for these unit tests.
 type rawACLRow struct {
 	user  string
 	topic string
@@ -228,25 +225,35 @@ type rawACLRow struct {
 	write bool
 }
 
-func buildSnapshot(t *testing.T, rows []rawACLRow) *aclSnapshot {
+// loadCache writes the given rows into the cache under its write lock,
+// preserving the same exact/wildcard partitioning that reload would produce.
+func loadCache(t *testing.T, c *aclCache, rows []rawACLRow) {
 	t.Helper()
-	snap := &aclSnapshot{
-		exact:     make(map[string]map[string]aclEntry),
-		wildcards: make(map[string][]aclEntry),
-	}
+	exact := make(map[string]map[string]aclEntry)
+	wildcards := make(map[string][]aclEntry)
 	for _, r := range rows {
 		e := aclEntry{topic: r.topic, read: r.read, write: r.write}
 		if containsPercent(r.topic) {
-			e.matcher = compileLikeToRegex(r.topic)
-			snap.wildcards[r.user] = append(snap.wildcards[r.user], e)
+			e.matcher = mustCompileLikeToRegex(t, r.topic)
+			wildcards[r.user] = append(wildcards[r.user], e)
 		} else {
-			if snap.exact[r.user] == nil {
-				snap.exact[r.user] = make(map[string]aclEntry)
+			if exact[r.user] == nil {
+				exact[r.user] = make(map[string]aclEntry)
 			}
-			snap.exact[r.user][r.topic] = e
+			exact[r.user][r.topic] = e
 		}
 	}
-	return snap
+	c.mu.Lock()
+	c.exact = exact
+	c.wildcard = wildcards
+	c.mu.Unlock()
+}
+
+func mustCompileLikeToRegex(t *testing.T, pattern string) *regexp.Regexp {
+	t.Helper()
+	r, err := compileLikeToRegex(pattern)
+	require.NoError(t, err)
+	return r
 }
 
 func containsPercent(s string) bool {