docs(cache): threat-model audit Section 3 (TOCTOU + invalidation race) (#924 Cerberus)

Filled Section 3 of threats.md with Question/Finding/Severity/Repro test/Fix
for all three concerns:
1. Invalidate vs OnInvalidate concurrent registration
2. TTL expiry race on concurrent Get
3. Get-then-Set caller-side TOCTOU

Added threat-named race-stress tests in cache_test.go covering invalidation
snapshot, concurrent expired Get, and get-then-set race probe. Tests pass
under -race -count=10. No cache.go hardening needed — analysis showed
existing locking is correct.

Closes tasks.lthn.sh/view.php?id=924

Co-authored-by: Codex <noreply@openai.com>

Author: Snider

cache_test.go

@@ -17,6 +17,7 @@ import (
 	"os"
 	"runtime"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -2684,6 +2685,143 @@ func TestCache_ThreatPathTraversal_HTTPCacheUsesHashedRequestStorageKeys(t *test
 	}
 }
 
+func TestCache_ThreatTOCTOU_InvalidateOnInvalidateRegistrationIsSnapshotRaceClean(t *testing.T) {
+	c, _ := newTestCache(t, "/tmp/cache-threat-invalidate-snapshot", time.Minute)
+
+	if err := c.Set("victim", "old"); err != nil {
+		t.Fatalf("Set victim failed: %v", err)
+	}
+	if err := c.Set("late", "new"); err != nil {
+		t.Fatalf("Set late failed: %v", err)
+	}
+
+	var registerOnce sync.Once
+	var lateCalls int64
+	c.OnInvalidate("reload", func(trigger string) []string {
+		registerOnce.Do(func() {
+			c.OnInvalidate(trigger, func(string) []string {
+				atomic.AddInt64(&lateCalls, 1)
+				return []string{"late"}
+			})
+		})
+		runtime.Gosched()
+		return []string{"victim"}
+	})
+
+	deleted, err := c.Invalidate("reload")
+	if err != nil {
+		t.Fatalf("Invalidate failed: %v", err)
+	}
+	if deleted != 1 {
+		t.Fatalf("expected first invalidation to delete snapshot callback match only, got %d", deleted)
+	}
+	if got := atomic.LoadInt64(&lateCalls); got != 0 {
+		t.Fatalf("newly registered callback should not run in same invalidation pass, got %d calls", got)
+	}
+
+	deleted, err = c.Invalidate("reload")
+	if err != nil {
+		t.Fatalf("second Invalidate failed: %v", err)
+	}
+	if deleted != 1 {
+		t.Fatalf("expected second invalidation to delete late callback match, got %d", deleted)
+	}
+	if got := atomic.LoadInt64(&lateCalls); got != 1 {
+		t.Fatalf("expected late callback to run once on next invalidation pass, got %d calls", got)
+	}
+}
+
+func TestCache_ThreatTOCTOU_InvalidateConcurrentRegistrationRaceClean(t *testing.T) {
+	c, _ := newTestCache(t, "/tmp/cache-threat-invalidate-race", time.Minute)
+	c.OnInvalidate("reload", func(string) []string {
+		runtime.Gosched()
+		return nil
+	})
+
+	const workers = 16
+	const registrationsPerWorker = 16
+	start := make(chan struct{})
+	errCh := make(chan error, workers)
+
+	var done sync.WaitGroup
+	done.Add(workers * 2)
+	for range workers {
+		go func() {
+			defer done.Done()
+			<-start
+			for range registrationsPerWorker {
+				c.OnInvalidate("reload", func(string) []string {
+					runtime.Gosched()
+					return nil
+				})
+			}
+		}()
+	}
+	for range workers {
+		go func() {
+			defer done.Done()
+			<-start
+			for range registrationsPerWorker {
+				if _, err := c.Invalidate("reload"); err != nil {
+					errCh <- err
+					return
+				}
+			}
+		}()
+	}
+
+	close(start)
+	done.Wait()
+	close(errCh)
+
+	for err := range errCh {
+		t.Errorf("Invalidate failed: %v", err)
+	}
+}
+
+func TestCache_ThreatTOCTOU_ExpiredGetConcurrentReadersReturnNotFound(t *testing.T) {
+	c, _ := newTestCache(t, "/tmp/cache-threat-expired-get", time.Minute)
+	if err := c.SetWithTTL("ttl/race", map[string]string{"state": "expired"}, time.Nanosecond); err != nil {
+		t.Fatalf("SetWithTTL failed: %v", err)
+	}
+	time.Sleep(2 * time.Millisecond)
+
+	const readers = 64
+	start := make(chan struct{})
+	errCh := make(chan string, readers)
+
+	var done sync.WaitGroup
+	done.Add(readers)
+	for range readers {
+		go func() {
+			defer done.Done()
+			<-start
+
+			got := map[string]string{"state": "sentinel"}
+			found, err := c.Get("ttl/race", &got)
+			if err != nil {
+				errCh <- err.Error()
+				return
+			}
+			if found {
+				errCh <- "expected expired Get to return found=false"
+				return
+			}
+			if got["state"] != "sentinel" {
+				errCh <- "expired Get unmarshaled stale data into destination"
+			}
+		}()
+	}
+
+	close(start)
+	done.Wait()
+	close(errCh)
+
+	for msg := range errCh {
+		t.Error(msg)
+	}
+}
+
 func TestCache_ThreatTOCTOU_GetThenSetSerializesEntryWrites(t *testing.T) {
 	medium := &raceProbeMedium{MockMedium: coreio.NewMockMedium()}
 	c, err := cache.New(medium, "/tmp/cache-threat-toctou", time.Minute)

threats.md

@@ -74,26 +74,38 @@ Status: Complete
 
 Question: What lock is held while `Invalidate` walks callbacks, and can `OnInvalidate` append to the same trigger while that walk is in progress?
 
-Finding: No map-walk race found. `Cache.mu` is the lock protecting the invalidation callback map (`cache.go:50`). `OnInvalidate` takes the write lock before appending to `cache.invalidation[trigger]` (`cache.go:696`, `cache.go:698`). `Invalidate` takes the read lock only long enough to copy the trigger's callback slice, then releases the lock before executing callbacks and deleting entries (`cache.go:709`, `cache.go:710`, `cache.go:711`, `cache.go:713`). A callback that registers more invalidations therefore cannot mutate the map while it is being read. The newly registered callback is not included in the already-snapshotted invalidation pass, which is acceptable snapshot semantics. No `delete(c.invalidation, trigger)` call exists in the reviewed cache implementation.
+Finding: No map-walk race found. `Cache.mu` is the lock protecting the invalidation callback map (`cache.go:56`). `OnInvalidate` takes the write lock before appending to `cache.invalidation[trigger]` (`cache.go:818`, `cache.go:820`). `Invalidate` takes the read lock only long enough to copy the trigger's callback slice, then releases the lock before executing callbacks and deleting entries (`cache.go:831`, `cache.go:832`, `cache.go:833`, `cache.go:835`). A callback that registers more invalidations therefore cannot mutate the map while it is being read, and it does not deadlock by trying to acquire the write lock from inside the callback. The newly registered callback is not included in the already-snapshotted invalidation pass, which is acceptable snapshot semantics. No `delete(cache.invalidation, trigger)` call exists in the reviewed cache implementation.
 
 Severity: None.
 
+Repro test: `TestCache_ThreatTOCTOU_InvalidateOnInvalidateRegistrationIsSnapshotRaceClean` and `TestCache_ThreatTOCTOU_InvalidateConcurrentRegistrationRaceClean` (`cache_test.go:2688`, `cache_test.go:2734`).
+
+Fix: No code change required. The existing callback snapshot under `Cache.mu` is the intended mitigation; the added tests pin the race-clean and snapshot semantics.
+
 ### 3.2 TTL expiry race on Get
 
 Status: Complete
 
 Question: Can two concurrent readers of a freshly expired entry return expired data, or does one reader delete/alter state out from under the other?
 
-Finding: No unsafe TTL expiry race found. `Get` reads the entry under the entry read lock, unmarshals the cache envelope, checks `time.Now().After(entry.ExpiresAt)`, and returns `found=false` before unmarshalling cached data into the caller's destination (`cache.go:178`, `cache.go:186`, `cache.go:195`, `cache.go:200`, `cache.go:201`, `cache.go:204`). `GetBinary` follows the same metadata-first expiry check and returns `found=false` before reading the payload body (`cache.go:439`, `cache.go:445`, `cache.go:446`, `cache.go:449`). Expired reads do not delete files, so two readers can both lose and safely return not-found; neither path returns expired data after observing the expiry check.
+Finding: No unsafe TTL expiry race found. `Get` reads the entry under the entry read lock, unmarshals the cache envelope, checks `time.Now().After(entry.ExpiresAt)`, and returns `found=false` before unmarshalling cached data into the caller's destination (`cache.go:300`, `cache.go:317`, `cache.go:322`, `cache.go:323`, `cache.go:326`). `GetBinary` follows the same metadata-first expiry check and returns `found=false` before reading the payload body (`cache.go:545`, `cache.go:562`, `cache.go:567`, `cache.go:568`, `cache.go:571`). Expired reads do not delete files, so two readers can both lose and safely return not-found; neither path returns expired data after observing the expiry check.
 
 Severity: None.
 
+Repro test: `TestCache_ThreatTOCTOU_ExpiredGetConcurrentReadersReturnNotFound` (`cache_test.go:2782`).
+
+Fix: No code change required. The current metadata-first expiry check and non-mutating expired-read behavior are safe for concurrent readers.
+
 ### 3.3 Get-then-Set caller-site TOCTOU
 
 Status: Complete
 
 Question: If two consumers both observe `Get` as missing or expired and then both call `Set`, does `Cache.mu` serialize the writes, or is this just last-writer-wins cache behavior?
 
-Finding: Yes, fixed. `Cache.mu` protects invalidation callback registration and snapshotting, while `entryMu` now serializes cache entry I/O separately (`cache.go:50`, `cache.go:51`). `Get` and `GetBinary` take `entryMu.RLock` while reading entries (`cache.go:178`, `cache.go:423`). `Set` and `SetBinary` take `entryMu.Lock` across path resolution, rollback snapshot, and writes (`cache.go:238`, `cache.go:246`, `cache.go:279`, `cache.go:280`, `cache.go:360`, `cache.go:368`, `cache.go:401`, `cache.go:407`). Delete paths are also serialized: single-key removal locks before deleting metadata and binary sidecars, `DeleteMany` locks across its batch, and invalidation pattern listing takes the entry read lock while walking keys (`cache.go:306`, `cache.go:315`, `cache.go:323`, `cache.go:469`, `cache.go:486`, `cache.go:550`, `cache.go:553`). Pure cache freshness remains last-writer-wins, but callers no longer need the backing `coreio.Medium` to tolerate overlapping entry operations. Regression coverage: `TestCache_ThreatTOCTOU_GetThenSetSerializesEntryWrites` (`cache_test.go:2668`).
+Finding: Yes, fixed. `Cache.mu` protects invalidation callback registration and snapshotting, while `entryMu` serializes cache entry I/O separately (`cache.go:56`, `cache.go:57`). `Get` and `GetBinary` take `entryMu.RLock` while reading entries (`cache.go:300`, `cache.go:545`). `Set` and `SetBinary` take `entryMu.Lock` across path resolution, rollback snapshot, and writes (`cache.go:360`, `cache.go:368`, `cache.go:401`, `cache.go:482`, `cache.go:490`, `cache.go:494`, `cache.go:523`, `cache.go:529`). Delete paths are also serialized: single-key removal locks before deleting metadata and binary sidecars, `DeleteMany` locks across its batch, and invalidation pattern listing takes the entry read lock while walking keys (`cache.go:428`, `cache.go:431`, `cache.go:591`, `cache.go:667`, `cache.go:672`, `cache.go:675`). Pure cache freshness remains last-writer-wins, but callers no longer need the backing `coreio.Medium` to tolerate overlapping entry operations.
 
 Severity: Medium before fix; mitigated by entry-level serialization.
+
+Repro test: `TestCache_ThreatTOCTOU_GetThenSetSerializesEntryWrites` (`cache_test.go:2825`).
+
+Fix: Use `entryMu` for cache entry I/O so concurrent caller-side `Get`-then-`Set` misses cannot overlap backing-medium writes. This preserves last-writer-wins cache semantics while removing the lower-level I/O race.