diff --git a/CACHE_AUDIT.md b/CACHE_AUDIT.md deleted file mode 100644 index 9343490a..00000000 --- a/CACHE_AUDIT.md +++ /dev/null @@ -1,300 +0,0 @@ -# Storage Cache Usage Audit - -**Date:** 2025-10-27 -**Purpose:** Document all dependencies on the daemon storage cache before removal -**Related:** bd-30 (Audit Current Cache Usage), bd-29 (Remove Daemon Storage Cache epic) - ---- - -## Summary - -The daemon storage cache (`storageCache map[string]*StorageCacheEntry`) is designed for multi-repository routing but only ever caches **one repository** in practice (local daemon model). All code paths can safely use `s.storage` directly instead of `getStorageForRequest()`. - ---- - -## Key Findings - -### 1. getStorageForRequest() Callers - -**Total calls:** 17 production calls + 11 test calls - -**Production files:** -- `internal/rpc/server_issues_epics.go` - **8 calls** (CreateIssue, UpdateIssue, CloseIssue, ReopenIssue, GetIssue, ListIssues, DeleteIssue, CreateEpic) -- `internal/rpc/server_labels_deps_comments.go` - **4 calls** (AddLabel, RemoveLabel, AddDependency, AddComment) -- `internal/rpc/server_export_import_auto.go` - **2 calls** (AutoExport, AutoImport) -- `internal/rpc/server_compact.go` - **2 calls** (CompactIssue, GetDeletedIssue) -- `internal/rpc/server_routing_validation_diagnostics.go` - **1 call** (ValidateDatabase) - -**Test file:** -- `internal/rpc/server_eviction_test.go` - **11 calls** (cache eviction tests only) - -**Pattern (all identical):** -```go -store, err := s.getStorageForRequest(req) -if err != nil { - return Response{Success: false, Error: err.Error()} -} -// ... use store ... -``` - -**Replacement strategy:** Replace with `store := s.storage` and remove error handling. - ---- - -### 2. req.Cwd Usage - -**Purpose:** `req.Cwd` is set by RPC client for database discovery in multi-repo routing. - -**Code paths:** - -1. **RPC client (`internal/rpc/client.go`):** - - Sets `req.Cwd` in request construction (line not shown in excerpt, but inferred from server usage) - - Client debug output references `CacheSize` from health check (line 78) - -2. **Server cache lookup (`internal/rpc/server_cache_storage.go`):** - ```go - // If no cwd specified, use default storage - if req.Cwd == "" { - return s.storage, nil - } - ``` - - When `req.Cwd` is set, uses `findDatabaseForCwd()` to locate `.beads/*.db` - - Canonicalizes to repo root as cache key - - Checks cache with mtime-based invalidation (lines 162-256) - -3. **Validation endpoint (`server_routing_validation_diagnostics.go`):** - - Uses `req.Cwd` for database validation (line 84-86) - - Only validator that inspects `req.Cwd` outside of cache - -4. **Comment forwarding (`server_labels_deps_comments.go`):** - - Passes `req.Cwd` through when forwarding AddComment to hooks (line 179) - - Preserves context for downstream handlers - -**Observation:** In local daemon model, `req.Cwd` is **always empty or always matches the daemon's workspace**. The daemon only serves one repository, so routing logic is unused. - ---- - -### 3. MCP Server Multi-Repo Strategy - -**Files examined:** -- `integrations/beads-mcp/` (searched for `req.Cwd` - no results) - -**Findings:** -- MCP server does **not** use `req.Cwd` routing -- MCP follows **recommended architecture**: separate daemon per repository -- Each MCP workspace connects to its own local daemon via socket -- No shared daemon across projects - -**Implication:** MCP will **not** be affected by cache removal. Each daemon already serves only one repo. - ---- - -### 4. Cache-Dependent Tests - -**File:** `internal/rpc/server_eviction_test.go` - -**Tests to DELETE (entire file):** -- `TestStorageCacheEviction_TTL` - TTL-based eviction -- `TestStorageCacheEviction_LRU` - LRU eviction when over max size -- `TestStorageCacheEviction_MemoryPressure` - Memory pressure eviction -- `TestStorageCacheEviction_MtimeInvalidation` - Stale DB detection -- `TestConcurrentCacheAccess` - Concurrent cache access -- `TestSubdirectoryDatabaseLookup` - Subdirectory canonicalization -- `TestManualCacheEviction` - Manual evict API -- `TestCacheSizeLimit` - Cache size enforcement -- `TestNegativeTTL` - Negative TTL config - -**File:** `internal/rpc/limits_test.go` - -**Updates needed:** -- Remove assertions like `len(srv.storageCache)` (exact lines not shown, needs inspection) -- Remove manual cache population (if any) - ---- - -### 5. Environment Variables - -**Cache-related env vars (to deprecate):** - -1. **`BEADS_DAEMON_MAX_CACHE_SIZE`** - - Default: 50 - - Used in: `server_core.go:63` - - Tested in: `server_eviction_test.go:238` - -2. **`BEADS_DAEMON_CACHE_TTL`** - - Default: 30 minutes - - Used in: `server_core.go:72` - - Tested in: `server_eviction_test.go:239, 423` - -3. **`BEADS_DAEMON_MEMORY_THRESHOLD_MB`** - - Default: 500 MB - - Used in: `server_cache_storage.go:47` - - Triggers aggressive eviction - -**Other env vars (keep):** -- `BEADS_DAEMON_MAX_CONNS` (connection limiting) -- `BEADS_DAEMON_REQUEST_TIMEOUT` (request timeout) - ---- - -### 6. Cache-Related Server Fields - -**In `internal/rpc/server_core.go` (Server struct):** - -**Fields to REMOVE:** -```go -storageCache map[string]*StorageCacheEntry // line 36 -cacheMu sync.RWMutex // line 37 -maxCacheSize int // line 38 -cacheTTL time.Duration // line 39 -cleanupTicker *time.Ticker // line 40 -cacheHits int64 // line 44 -cacheMisses int64 // line 45 -``` - -**Fields to KEEP:** -```go -storage storage.Storage // line 28 - default storage (use this!) -// ... all other fields ... -``` - ---- - -### 7. Cache Functions in server_cache_storage.go - -**All functions in this file will be deleted:** - -1. **`runCleanupLoop()`** (lines 23-37) - - Periodic eviction ticker goroutine - - Called from `Start()` - -2. **`checkMemoryPressure()`** (lines 39-61) - - Memory monitoring and aggressive eviction trigger - - Called from cleanup loop - -3. **`aggressiveEviction()`** (lines 63-103) - - Evicts 50% of cache under memory pressure - - LRU-based selection - -4. **`evictStaleStorage()`** (lines 105-155) - - TTL-based eviction - - LRU eviction for size enforcement - - Called periodically and after new cache entries - -5. **`getStorageForRequest(req *Request)`** (lines 157-256) - - Main cache lookup and routing logic - - mtime-based invalidation - - **17 production callers** (see section 1) - -6. **`findDatabaseForCwd(cwd string)`** (lines 258-286) - - Walks up directory tree to find `.beads/*.db` - - Used only by `getStorageForRequest()` - - Also used by validation endpoint (line 86 of diagnostics) - -**Note:** `findDatabaseForCwd()` is also called from `server_routing_validation_diagnostics.go:86` for database path validation. We need to decide if that endpoint should be removed or reimplemented. - ---- - -### 8. Metrics/Health Impact - -**Health endpoint fields to remove:** -- `cache_size` (line 77-78 in client.go debug output references this) - -**Metrics endpoint fields to remove:** -- `cache_size` -- `cache_hits` (field line 44) -- `cache_misses` (field line 45) - -**Files to update:** -- `internal/rpc/server_routing_validation_diagnostics.go` - Health and Metrics handlers -- `internal/rpc/metrics.go` - Metrics struct - ---- - -### 9. Cache Initialization and Lifecycle - -**Initialization (`server_core.go:NewServer()`):** -- Line 98: `storageCache: make(map[string]*StorageCacheEntry)` -- Lines 99-100: `maxCacheSize`, `cacheTTL` config -- **Remove:** Cache map creation and config parsing (lines 62-76, 98-100) - -**Start lifecycle (`server_core.go:Start()` - not shown in excerpt):** -- Starts `runCleanupLoop()` goroutine for periodic eviction -- **Remove:** Cleanup loop goroutine launch - -**Stop lifecycle (`server_core.go:Stop()` - not shown in excerpt):** -- Stops cleanup ticker -- Closes all cached storage entries -- **Simplify:** Just close `s.storage` directly - ---- - -## Migration Strategy - -### Phase 1: Remove Cache Code (bd-31, bd-32, bd-33) -1. Remove cache fields from `Server` struct -2. Replace `getStorageForRequest(req)` with `s.storage` in all 17 callers -3. Remove error handling for storage lookup failures -4. Delete `server_cache_storage.go` entirely - -### Phase 2: Clean Up Tests (bd-34) -1. Delete `server_eviction_test.go` (entire file) -2. Update `limits_test.go` to remove cache assertions - -### Phase 3: Update Metrics (bd-35) -1. Remove `cache_size`, `cache_hits`, `cache_misses` from health/metrics endpoints -2. Remove cache-related fields from `Metrics` struct - -### Phase 4: Documentation (bd-36) -1. Remove cache env var docs from ADVANCED.md, commands/daemons.md -2. Add CHANGELOG entry documenting removal - ---- - -## Risk Assessment - -### ✅ Safe to Remove -- **Multi-repo routing:** Unused in local daemon model (always 1 repo) -- **Cache eviction logic:** Unnecessary for single storage instance -- **mtime invalidation:** Irrelevant for persistent `s.storage` connection -- **LRU/TTL/memory pressure:** Over-engineered for 1 cached entry - -### ⚠️ Needs Verification -- **MCP server:** Confirmed using separate daemons (no `req.Cwd` usage found) -- **Database validation:** `ValidateDatabase` endpoint uses `findDatabaseForCwd()` - may need reimplementation or removal -- **Comment forwarding:** Passes `req.Cwd` to hooks - verify hooks don't rely on it - -### ✅ No Breaking Changes Expected -- Local daemon already behaves as if cache size = 1 -- All requests route to same `s.storage` instance -- Performance identical (no cache overhead to remove) -- Memory usage will improve (no cache structs) - ---- - -## Acceptance Criteria Checklist - -- [x] **Document all callers of `getStorageForRequest()`** - 17 production, 11 test -- [x] **Verify `req.Cwd` is only for database discovery** - Confirmed routing-only purpose -- [x] **Confirm MCP doesn't rely on multi-repo cache** - MCP uses separate daemons -- [x] **List tests assuming multi-repo routing** - `server_eviction_test.go` (entire file) -- [x] **Review cache env vars** - 3 cache vars, 2 non-cache vars to keep -- [x] **Document cache dependencies** - All code paths identified above - ---- - -## Next Steps - -1. **bd-31**: Remove cache fields from `Server` struct -2. **bd-32**: Replace `getStorageForRequest()` with `s.storage` (17 call sites) -3. **bd-33**: Delete `server_cache_storage.go` -4. **bd-34**: Delete `server_eviction_test.go`, update `limits_test.go` -5. **bd-35**: Remove cache metrics from health/metrics endpoints -6. **bd-36**: Update documentation and CHANGELOG - -**Special consideration:** `ValidateDatabase` endpoint in `server_routing_validation_diagnostics.go` uses `findDatabaseForCwd()` outside of cache. Needs decision: -- Option A: Remove validation endpoint (may be unused) -- Option B: Inline database discovery logic in validator -- Option C: Keep `findDatabaseForCwd()` as standalone helper - -**Recommendation:** Verify if `ValidateDatabase` is used, then choose Option A or B.