From ef30031e2e03ef86d4e9c990052d810a64e67d23 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Sat, 4 Apr 2026 09:50:27 -0700 Subject: [PATCH] perf: cache resolveRegionObservers with 30s TTL (#575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Cache `resolveRegionObservers()` results with a 30-second TTL to eliminate repeated database queries for region→observer ID mappings. ## Problem `resolveRegionObservers()` queried the database on every call despite the observers table changing infrequently (~20 rows). It's called from 10+ hot paths including `filterPackets()`, `GetChannels()`, and multiple analytics compute functions. When analytics caches are cold, parallel requests each hit the DB independently. ## Solution - Added a dedicated `regionObsMu` mutex + `regionObsCache` map with 30s TTL - Uses a separate mutex (not `s.mu`) to avoid deadlocks — callers already hold `s.mu.RLock()` - Cache is lazily populated per-region and fully invalidated after TTL expires - Follows the same pattern as `getCachedNodesAndPM()` (30s TTL, on-demand rebuild) ## Changes - **`cmd/server/store.go`**: Added `regionObsMu`, `regionObsCache`, `regionObsCacheTime` fields; rewrote `resolveRegionObservers()` to check cache first; added `fetchAndCacheRegionObs()` helper - **`cmd/server/coverage_test.go`**: Added `TestResolveRegionObserversCaching` — verifies cache population, cache hits, and nil handling for unknown regions ## Testing - All existing Go tests pass (`go test ./...`) - New test verifies caching behavior (population, hits, nil for unknown regions) Fixes #362 --------- Co-authored-by: you --- cmd/server/coverage_test.go | 65 +++++++++++++++++++++++++++++++++++++ cmd/server/store.go | 31 ++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/cmd/server/coverage_test.go b/cmd/server/coverage_test.go index 6ef42baf..15301b2f 100644 --- a/cmd/server/coverage_test.go +++ b/cmd/server/coverage_test.go @@ -3887,6 +3887,71 @@ func TestGetChannelMessagesAfterIngest(t *testing.T) { } } +// --- resolveRegionObservers caching --- + +func TestResolveRegionObserversCaching(t *testing.T) { + db := setupTestDB(t) + defer db.Close() + seedTestData(t, db) + + store := &PacketStore{db: db} + + // First call should populate cache. + obs1 := store.resolveRegionObservers("SJC") + if obs1 == nil || len(obs1) == 0 { + t.Fatal("expected observer IDs for SJC on first call") + } + + // Second call should return cached result (same pointer). + obs2 := store.resolveRegionObservers("SJC") + if len(obs2) != len(obs1) { + t.Errorf("cached result differs: got %d, want %d", len(obs2), len(obs1)) + } + + // Non-existent region should return nil even from cache. + obs3 := store.resolveRegionObservers("NONEXIST") + if obs3 != nil { + t.Errorf("expected nil for NONEXIST, got %v", obs3) + } + + // Verify cache fields are set. + if store.regionObsCache == nil { + t.Error("regionObsCache should be non-nil after calls") + } + if store.regionObsCacheTime.IsZero() { + t.Error("regionObsCacheTime should be set") + } +} + +func TestResolveRegionObserversCacheMissNewRegion(t *testing.T) { + db := setupTestDB(t) + defer db.Close() + seedTestData(t, db) + + store := &PacketStore{db: db} + + // Populate cache with SJC. + obs1 := store.resolveRegionObservers("SJC") + if obs1 == nil || len(obs1) == 0 { + t.Fatal("expected observer IDs for SJC on first call") + } + + // Cache is now valid. Request a different region that exists in DB. + // Before the fix, this would return nil from the map lookup instead of + // fetching from DB, silently returning "no observers" for up to 30s. + obs2 := store.resolveRegionObservers("LAX") + // LAX may or may not have data in the test DB, but the key point is: + // a non-existent region should be fetched (not just nil-returned). + // Verify the region key was cached (even if empty). + store.regionObsMu.Lock() + _, cached := store.regionObsCache["LAX"] + store.regionObsMu.Unlock() + if !cached { + t.Error("LAX should be cached after resolveRegionObservers call, even if empty") + } + _ = obs2 +} + func TestIndexByNodePreCheck(t *testing.T) { store := &PacketStore{ byNode: make(map[string][]*StoreTx), diff --git a/cmd/server/store.go b/cmd/server/store.go index 25cccbb6..f195a620 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -127,6 +127,10 @@ type PacketStore struct { channelsCacheKey string channelsCacheExp time.Time channelsCacheRes []map[string]interface{} + // Cached region → observer ID mapping (30s TTL, avoids repeated DB queries) + regionObsMu sync.Mutex + regionObsCache map[string]map[string]bool + regionObsCacheTime time.Time // Cached node list + prefix map (rebuilt on demand, shared across analytics) nodeCache []nodeInfo nodePM *prefixMap @@ -1840,15 +1844,42 @@ func (s *PacketStore) transmissionsForObserver(observerIDs string, from []*Store } // resolveRegionObservers returns a set of observer IDs for a given IATA region. +// Results are cached for 30 seconds to avoid repeated DB queries. +// Uses its own mutex (regionObsMu) so callers holding s.mu won't deadlock. func (s *PacketStore) resolveRegionObservers(region string) map[string]bool { + s.regionObsMu.Lock() + defer s.regionObsMu.Unlock() + + if s.regionObsCache != nil && time.Since(s.regionObsCacheTime) < 30*time.Second { + if m, ok := s.regionObsCache[region]; ok { + return m + } + return s.fetchAndCacheRegionObs(region) + } + // Cache expired — rebuild. + s.regionObsCache = make(map[string]map[string]bool) + s.regionObsCacheTime = time.Now() + + // Fetch for the requested region and cache it. + return s.fetchAndCacheRegionObs(region) +} + +// fetchAndCacheRegionObs fetches observer IDs for a region from the DB and stores in cache. +// Caller must hold regionObsMu. +func (s *PacketStore) fetchAndCacheRegionObs(region string) map[string]bool { + if m, ok := s.regionObsCache[region]; ok { + return m + } ids, err := s.db.GetObserverIdsForRegion(region) if err != nil || len(ids) == 0 { + s.regionObsCache[region] = nil return nil } m := make(map[string]bool, len(ids)) for _, id := range ids { m[id] = true } + s.regionObsCache[region] = m return m }