Worked review 4: a generic cache with TTL

Cache[K, V] is a small generic, time-limited cache — Set stores a value with an expiry, Get should return false once that expiry has passed. It compiles, go vet says nothing, golangci-lint run says nothing, and it runs. It has 4 real bugs, one of which is a genuine fatal error under real concurrent use.

The program

Run ▸ Open in Playground

package main

import (
  "fmt"
  "time"
)

type entry[V any] struct {
  value V
  exp   time.Time
}

type Cache[K comparable, V any] struct {
  data map[K]entry[V]
  ttl  time.Duration
}

func New[K comparable, V any](ttl time.Duration) Cache[K, V] {
  return Cache[K, V]{data: make(map[K]entry[V]), ttl: ttl}
}

func (c Cache[K, V]) Set(key K, value V) {
  c.data[key] = entry[V]{value: value, exp: time.Now().Add(c.ttl)}
}

func (c Cache[K, V]) Get(key K) (V, bool) {
  e, ok := c.data[key]
  if !ok {
    var zero V
    return zero, false
  }
  if time.Now().After(e.exp) {
    delete(c.data, key)
  }
  return e.value, true
}

func main() {
  c := New[string, int](50 * time.Millisecond)
  c.Set("a", 1)

  v, ok := c.Get("a")
  fmt.Println("immediately:", v, ok)

  time.Sleep(100 * time.Millisecond)
  v, ok = c.Get("a")
  fmt.Println("after ttl:", v, ok)

  for i := 0; i < 5; i++ {
    c.Set(fmt.Sprintf("k%d", i), i)
  }
  time.Sleep(100 * time.Millisecond)
  fmt.Println("entries still in map after all expired, none re-Get:", len(c.data))

  c2 := c
  c2.Set("shared", 42)
  sharedVal, sharedOK := c.Get("shared")
  fmt.Println("c (unrelated-looking copy) also sees c2's write:", sharedVal, sharedOK)
}

Find the bugs

c.Get("a") after the TTL has already passed still reports true. Two supposedly independent-looking cache variables see each other’s writes. Try to name at least three of the four bugs before opening the answer.

Reveal the 4 bugs

1. Get deletes the expired entry but still returns it as if it were valid. if time.Now().After(e.exp) { delete(c.data, key) } runs, and then execution falls straight through to the unconditional return e.value, true below it — there is no return zero, false on the expired path. Real output: after ttl: 1 true, when it should be 0 false. The entry is even correctly removed from the map (delete on a shared map reference works fine through a value receiver) — but the stale value ships anyway on the way out.
2. Neither Set nor Get synchronizes access to the map. There is no sync.Mutex anywhere in Cache. Run four goroutines calling Set/Get on the same cache concurrently and Go’s runtime — not just the race detector — catches it directly:

   fatal error: concurrent map writes

   This is not a -race-only finding; unsynchronized concurrent map writes crash the process outright, with or without the race detector attached (-race additionally reports the exact two goroutines and lines involved).
3. Expired entries are never removed unless something happens to call Get on that exact key again. The demo sets five keys, lets all of them expire, and never touches any of them again with Get — len(c.data) reports all 5 still present. A cache used as Set-mostly (write a value, look it up rarely or never again by that exact key) grows without bound; nothing ever sweeps it.
4. New returns Cache[K, V] by value with value-receiver methods, so a plain assignment silently aliases instead of copying. c2 := c looks like an independent copy — Cache is a plain struct, after all — but Cache’s only real state is a map, and maps are reference types. c2.Set("shared", 42) is visible through c too: c (unrelated-looking copy) also sees c2's write: 42 true. Nothing here is technically incorrect Go — it is a value type whose “value” semantics are deceptive because of what it contains.

The corrected program

Run ▸ Open in Playground

package main

import (
  "fmt"
  "sync"
  "time"
)

type entry[V any] struct {
  value V
  exp   time.Time
}

type Cache[K comparable, V any] struct {
  mu   sync.Mutex // fixed: guards data against concurrent Set/Get
  data map[K]entry[V]
  ttl  time.Duration
  done chan struct{}
}

func New[K comparable, V any](ttl time.Duration) *Cache[K, V] { // fixed: pointer — copies now share explicitly, on purpose
  c := &Cache[K, V]{data: make(map[K]entry[V]), ttl: ttl, done: make(chan struct{})}
  go c.janitor(ttl) // fixed: proactively evict expired entries, not just on access
  return c
}

func (c *Cache[K, V]) janitor(interval time.Duration) {
  t := time.NewTicker(interval)
  defer t.Stop()
  for {
    select {
    case <-t.C:
      c.mu.Lock()
      for k, e := range c.data {
        if time.Now().After(e.exp) {
          delete(c.data, k)
        }
      }
      c.mu.Unlock()
    case <-c.done:
      return
    }
  }
}

func (c *Cache[K, V]) Close() { close(c.done) } // fixed: caller can stop the janitor goroutine

func (c *Cache[K, V]) Set(key K, value V) {
  c.mu.Lock()
  defer c.mu.Unlock()
  c.data[key] = entry[V]{value: value, exp: time.Now().Add(c.ttl)}
}

func (c *Cache[K, V]) Get(key K) (V, bool) {
  c.mu.Lock()
  defer c.mu.Unlock()
  e, ok := c.data[key]
  if !ok || time.Now().After(e.exp) { // fixed: expired counts as not found
    var zero V
    return zero, false
  }
  return e.value, true
}

func main() {
  c := New[string, int](50 * time.Millisecond)
  defer c.Close()
  c.Set("a", 1)

  v, ok := c.Get("a")
  fmt.Println("immediately:", v, ok)

  time.Sleep(100 * time.Millisecond)
  v, ok = c.Get("a")
  fmt.Println("after ttl:", v, ok)

  for i := 0; i < 5; i++ {
    c.Set(fmt.Sprintf("k%d", i), i)
  }
  time.Sleep(200 * time.Millisecond) // give the janitor a couple of ticks
  fmt.Println("entries left after janitor sweep:", len(c.data))

  c2 := c // fixed: this is now an explicit pointer copy — no surprise aliasing of a value type
  c2.Set("shared", 42)
  sharedVal, sharedOK := c.Get("shared")
  fmt.Println("c and c2 are the same *Cache on purpose:", sharedVal, sharedOK)
}

Run the corrected version: after ttl: 0 false, entries left after janitor sweep: 0 (a background janitor goroutine, stoppable via Close, sweeps expired entries on a ticker), and c and c2 are the same *Cache on purpose: 42 true — the aliasing is unchanged, but it is no longer a silent surprise: *Cache[K, V] documents in its type that copies share state, the same way every other pointer in Go does. Stress-testing this version with four goroutines calling Set/Get concurrently under go run -race reports no race and no fatal error — the version above crashed with fatal error: concurrent map writes under the identical load.

🟦 A generic type is not automatically a safe type

Every bug here would exist identically in a non-generic map[string]int-based cache — generics changed nothing about the need for a mutex, a correct expiry check, or pointer semantics. Don’t let a type parameter’s novelty distract from the same review checklist that applies to any stateful type. See Generics and The sync package.

Get calls delete(c.data, key) when an entry has expired, so the entry really is removed from the map. Why does the buggy version still report the value as found?

delete() doesn't actually work on generic map types The function falls through to the unconditional return e.value, true after the delete, instead of returning the zero value and false on the expired path time.Now().After(e.exp) always returns false The value is cached a second time somewhere else

Four goroutines call Set/Get on the same unsynchronized Cache concurrently. What actually happens, verified on this exact program?

Nothing — Go maps handle concurrent access safely by default A fatal error: concurrent map writes crash — not just a -race warning, an unrecoverable runtime crash even without the race detector The program silently returns wrong values but never crashes It only fails if the map has more than 1000 entries

In the buggy version, c2 := c followed by c2.Set(...) is visible through c too. Why does this surprise a reader more than the same thing happening with, say, *MyStruct?

It shouldn't surprise anyone — pointers and value types behave identically in Go Cache[K, V] looks like an ordinary value type (no asterisk, a plain struct), so c2 := c reads like an independent copy — the aliasing is hidden inside the map field rather than being visible in the type itself Generic types cannot be assigned with := This behavior only happens with comparable key types

The fixed version adds a janitor goroutine started inside New and a Close method. Why does Close matter here, beyond just 'good practice'?

It doesn't matter — the janitor goroutine would stop on its own Without calling Close, the janitor goroutine loops on its ticker forever, holding the mutex periodically and never exiting — exactly the goroutine-leak pattern from the AI Bug Catalog, just introduced by the fix itself Close is required by the Go compiler for any type with a background goroutine Close deletes all cache entries immediately

Submit

Mark as complete