Code Review: pkg/cache — Caching Layer (Roadmap 3.1)

Overall this is clean, well-structured code with good test coverage. The interface design is simple and the LRU+TTL memory implementation is solid. A few issues worth addressing before merging:

Bugs

1. MemoryCache.Get — potential LRU inconsistency after concurrent delete (memory.go:56-66)

Between releasing the RLock and re-acquiring the write lock for MoveToFront, another goroutine can delete the element. Go's container/list.MoveToFront is a no-op if the element has been removed (e.list == nil), so there's no panic — but a concurrent Set of the same key could result in the new element never being promoted. Minor LRU inaccuracy, but worth a note.

The simplest fix: check whether the element is still in c.items after acquiring the write lock before calling MoveToFront.

c.mu.Lock()
if _, ok := c.items[key]; ok {
    c.order.MoveToFront(elem)
}
c.mu.Unlock()

2. Close() panics on second call (memory.go:121, sqlite.go:73)

close(c.stop) panics if called twice. In test code with defer c.Close() this is safe, but any caller who accidentally calls Close twice will get a hard panic. A simple sync.Once guard would prevent this:

func (c *MemoryCache) Close() error {
    c.closeOnce.Do(func() { close(c.stop) })
    return nil
}

3. SQLite time comparison fragility (sqlite.go:48)

`SELECT value FROM _cache WHERE key = ? AND expires_at > ?`, key, time.Now(),

The behavior of comparing DATETIME columns with a Go time.Time depends on how the driver serializes it. With modernc.org/sqlite, this works, but it's implicit. Using time.Now().UTC().Format(time.RFC3339) makes the format explicit and portable across drivers.

4. Unhandled error in cleanup goroutine (sqlite.go:84)

c.db.Exec(`DELETE FROM _cache WHERE expires_at < ?`, time.Now())

The error is silently discarded. Even if it's a background task, logging the error (or using a passed-in logger) would help with observability in production use.

Design / Convention Issues

5. Closeable should be named Closer (cache.go:22)

Go convention (from io.Closer) uses the -er suffix for single-method interfaces: Closer, not Closeable. The interface should either be named Closer or simply embed io.Closer:

type Closer interface {
    Close() error
}

6. Context parameter ordering in GetJSON/SetJSON (cache.go:26, 40)

Go convention is ctx context.Context as the first parameter. The current signatures put the cache before context, which is non-idiomatic and will likely trigger staticcheck SA1012 or similar linters:

// Current (non-idiomatic)
func GetJSON[T any](c Cache, ctx context.Context, key string) (T, bool, error)

// Idiomatic
func GetJSON[T any](ctx context.Context, c Cache, key string) (T, bool, error)

7. Missing index on expires_at in SQLite schema (sqlite.go:24-30)

The cleanup query DELETE FROM _cache WHERE expires_at < ? and the Get query both filter on expires_at. Without an index, these do a full table scan. For any non-trivial cache size this will be slow:

CREATE INDEX IF NOT EXISTS _cache_expires_at ON _cache (expires_at)

8. HTTPCache applies to all HTTP methods (http.go:10)

Setting Cache-Control: public on POST/PUT/DELETE responses is semantically wrong and could confuse CDNs or proxies. The middleware should restrict to safe methods:

return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    if r.Method == http.MethodGet || r.Method == http.MethodHead {
        w.Header().Set("Cache-Control", value)
    }
    next.ServeHTTP(w, r)
})

Test Coverage Gaps

9. TTL tests use time.Sleep — flaky in CI (memory_test.go:66, sqlite_test.go:69)

time.Sleep(100 * time.Millisecond) after a 50ms TTL is a common source of CI flakiness under load. Consider a slightly larger margin (e.g., 200ms sleep, 50ms TTL) or use testing.Short() to skip in short mode.

10. No concurrent access test for SQLiteCache

MemoryCache has a good concurrency test, but SQLiteCache has none. SQLite's default WAL mode handles concurrent reads fine, but concurrent writes can produce SQLITE_BUSY errors without proper retry logic. A basic concurrent test (similar to TestMemoryCache_ConcurrentAccess) would catch driver-level issues.

11. No test for double-Close()

Given the panic on double-close described above, a test that calls Close() twice would pin the expected behavior.

Minor

• errors.Is(err, sql.ErrNoRows) is preferred over err == sql.ErrNoRows in sqlite.go:48 for wrapped error compatibility.
• NewSQLiteCache starts the cleanup goroutine before returning the cache — if NewSQLiteCache is called with a cleanupInterval of 0, the default kicks in but a zero-value cleanupInterval from the caller could also mean "no cleanup". Worth documenting the semantics clearly.

Summary: Fix the Close() double-call panic and the context parameter ordering (both are breaking correctness or API convention issues). The index and method-filtering for HTTPCache are the most impactful improvements for production use. Everything else is polish.