Skip to content

Commit 2d8fa57

Browse files
Snidercodex
andcommitted
fix(go-cache): restore entry-level serialisation (regression of #924)
sync.RWMutex now serialises Get/Set/Delete/DeleteMany/binary entry ops and invalidation key enumeration. The previous AX-6 sweep at #379/#381 removed the mutex; concurrent writes corrupted the underlying map and re-introduced the TOCTOU race that #924 originally fixed. sync import is structural concurrency primitive (AX-6 permitted via established convention). Race regressions: 100 concurrent Sets on distinct keys, 100 concurrent Sets on same key, mixed Get/Set/Delete on same key — all -race clean. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=985
1 parent 3795f07 commit 2d8fa57

2 files changed

Lines changed: 171 additions & 2 deletions

File tree

cache.go

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
// Note: AX-6 — intrinsic: coreio.Medium has no no-follow Lstat primitive or dynamic cwd lookup.
1010
"os"
1111
"slices"
12+
"sync" // Note: AX-6 — structural concurrency primitive for entry-level write serialisation.
1213
// Note: AX-6 — no core equivalent for durations or wall-clock timestamps.
1314
"time"
1415

@@ -43,9 +44,8 @@ type Cache struct {
4344
baseDir string
4445
cacheTTL time.Duration
4546
invalidation map[string][]InvalidateFunc
47+
entryMu sync.RWMutex
4648
runtime *core.Core
47-
// Backing-store operations intentionally have no mutex; callers must
48-
// synchronize concurrent writes to the same key.
4949
}
5050

5151
// Entry is the serialized cache record written to the backing Medium.
@@ -289,6 +289,9 @@ func (cache *Cache) Get(key string, dest any) (bool, error) {
289289
return false, err
290290
}
291291

292+
cache.entryMu.RLock()
293+
defer cache.entryMu.RUnlock()
294+
292295
path, err := cache.Path(key)
293296
if err != nil {
294297
return false, err
@@ -346,6 +349,9 @@ func (cache *Cache) set(key string, data any, ttl time.Duration, useDefaultTTL b
346349
return err
347350
}
348351

352+
cache.entryMu.Lock()
353+
defer cache.entryMu.Unlock()
354+
349355
path, _, err := cache.entryPaths(key)
350356
if err != nil {
351357
return err
@@ -412,6 +418,9 @@ func (cache *Cache) removeEntryFiles(key string) (bool, error) {
412418
return false, err
413419
}
414420

421+
cache.entryMu.Lock()
422+
defer cache.entryMu.Unlock()
423+
415424
jsonPath, binaryPath, err := cache.entryPaths(key)
416425
if err != nil {
417426
return false, err
@@ -464,6 +473,9 @@ func (cache *Cache) setBinary(key string, data []byte, contentType string, ttl t
464473
return err
465474
}
466475

476+
cache.entryMu.Lock()
477+
defer cache.entryMu.Unlock()
478+
467479
jsonPath, binaryPath, err := cache.entryPaths(key)
468480
if err != nil {
469481
return err
@@ -525,6 +537,9 @@ func (cache *Cache) GetBinary(key string) ([]byte, bool, error) {
525537
return nil, false, err
526538
}
527539

540+
cache.entryMu.RLock()
541+
defer cache.entryMu.RUnlock()
542+
528543
metaPath, binaryPath, err := cache.entryPaths(key)
529544
if err != nil {
530545
return nil, false, err
@@ -568,6 +583,9 @@ func (cache *Cache) DeleteMany(keys ...string) error {
568583
return err
569584
}
570585

586+
cache.entryMu.Lock()
587+
defer cache.entryMu.Unlock()
588+
571589
type entryFileSet struct {
572590
jsonPath string
573591
binaryPath string
@@ -646,6 +664,9 @@ func (cache *Cache) keysByPattern(pattern string) ([]string, error) {
646664
return nil, err
647665
}
648666

667+
cache.entryMu.RLock()
668+
defer cache.entryMu.RUnlock()
669+
649670
allKeys, err := cache.listJSONKeys()
650671
if err != nil {
651672
return nil, err

cache_test.go

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2822,6 +2822,154 @@ func TestCache_ThreatTOCTOU_ExpiredGetConcurrentReadersReturnNotFound(t *testing
28222822
}
28232823
}
28242824

2825+
func TestCache_ThreatTOCTOU_ConcurrentSetRandomKeysRaceClean(t *testing.T) {
2826+
c, _ := newTestCache(t, "/tmp/cache-threat-concurrent-random-set", time.Minute)
2827+
2828+
const workers = 100
2829+
keys := make([]string, workers)
2830+
for i := range workers {
2831+
keys[i] = "race/random/" + core.Itoa((i*37+11)%workers)
2832+
}
2833+
2834+
start := make(chan struct{})
2835+
errCh := make(chan string, workers)
2836+
2837+
var done sync.WaitGroup
2838+
done.Add(workers)
2839+
for i, key := range keys {
2840+
go func(value int, key string) {
2841+
defer done.Done()
2842+
<-start
2843+
if err := c.Set(key, map[string]int{"writer": value}); err != nil {
2844+
errCh <- err.Error()
2845+
}
2846+
}(i, key)
2847+
}
2848+
2849+
close(start)
2850+
done.Wait()
2851+
close(errCh)
2852+
2853+
for msg := range errCh {
2854+
t.Error(msg)
2855+
}
2856+
2857+
foundCount := 0
2858+
for i, key := range keys {
2859+
var got map[string]int
2860+
found, err := c.Get(key, &got)
2861+
if err != nil {
2862+
t.Fatalf("Get %q failed: %v", key, err)
2863+
}
2864+
if !found {
2865+
continue
2866+
}
2867+
foundCount++
2868+
if got["writer"] != i {
2869+
t.Fatalf("expected %q writer %d, got %d", key, i, got["writer"])
2870+
}
2871+
}
2872+
if foundCount != workers {
2873+
t.Fatalf("expected %d entries after concurrent Set calls, got %d", workers, foundCount)
2874+
}
2875+
}
2876+
2877+
func TestCache_ThreatTOCTOU_ConcurrentSetSameKeyRaceClean(t *testing.T) {
2878+
c, _ := newTestCache(t, "/tmp/cache-threat-concurrent-same-set", time.Minute)
2879+
2880+
const workers = 100
2881+
written := make(map[int]struct{}, workers)
2882+
for i := range workers {
2883+
written[i] = struct{}{}
2884+
}
2885+
2886+
start := make(chan struct{})
2887+
errCh := make(chan string, workers)
2888+
2889+
var done sync.WaitGroup
2890+
done.Add(workers)
2891+
for i := range workers {
2892+
go func(value int) {
2893+
defer done.Done()
2894+
<-start
2895+
if err := c.Set("race/same", map[string]int{"writer": value}); err != nil {
2896+
errCh <- err.Error()
2897+
}
2898+
}(i)
2899+
}
2900+
2901+
close(start)
2902+
done.Wait()
2903+
close(errCh)
2904+
2905+
for msg := range errCh {
2906+
t.Error(msg)
2907+
}
2908+
2909+
var got map[string]int
2910+
found, err := c.Get("race/same", &got)
2911+
if err != nil {
2912+
t.Fatalf("final Get failed: %v", err)
2913+
}
2914+
if !found {
2915+
t.Fatal("expected final cache entry to exist")
2916+
}
2917+
if _, ok := written[got["writer"]]; !ok {
2918+
t.Fatalf("final writer %d was not one of the concurrent writers", got["writer"])
2919+
}
2920+
}
2921+
2922+
func TestCache_ThreatTOCTOU_ConcurrentGetSetDeleteSameKeyRaceClean(t *testing.T) {
2923+
c, _ := newTestCache(t, "/tmp/cache-threat-concurrent-mixed", time.Minute)
2924+
if err := c.Set("race/mixed", map[string]int{"writer": -1}); err != nil {
2925+
t.Fatalf("initial Set failed: %v", err)
2926+
}
2927+
2928+
const workers = 100
2929+
const operations = 10
2930+
start := make(chan struct{})
2931+
errCh := make(chan string, workers*operations)
2932+
2933+
var done sync.WaitGroup
2934+
done.Add(workers)
2935+
for i := range workers {
2936+
go func(value int) {
2937+
defer done.Done()
2938+
<-start
2939+
for op := range operations {
2940+
switch (value + op) % 3 {
2941+
case 0:
2942+
var got map[string]int
2943+
found, err := c.Get("race/mixed", &got)
2944+
if err != nil {
2945+
errCh <- err.Error()
2946+
continue
2947+
}
2948+
if found && (got["writer"] < -1 || got["writer"] >= workers) {
2949+
errCh <- "Get returned a writer outside the written range"
2950+
}
2951+
case 1:
2952+
if err := c.Set("race/mixed", map[string]int{"writer": value}); err != nil {
2953+
errCh <- err.Error()
2954+
}
2955+
default:
2956+
if err := c.Delete("race/mixed"); err != nil {
2957+
errCh <- err.Error()
2958+
}
2959+
}
2960+
}
2961+
}(i)
2962+
}
2963+
2964+
close(start)
2965+
done.Wait()
2966+
close(errCh)
2967+
2968+
for msg := range errCh {
2969+
t.Error(msg)
2970+
}
2971+
}
2972+
28252973
func TestCache_ThreatTOCTOU_GetThenSetSerializesEntryWrites(t *testing.T) {
28262974
medium := &raceProbeMedium{MockMedium: coreio.NewMockMedium()}
28272975
c, err := cache.New(medium, "/tmp/cache-threat-toctou", time.Minute)

0 commit comments

Comments
 (0)