From 5c4bef44cdfa48f2f66360a5cbf2bec8eb2fb8d9 Mon Sep 17 00:00:00 2001 From: Stefan Ernst <24554551+stefan-ernst@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:02:24 +0100 Subject: [PATCH] perf(backend): clean up hot-path allocations and bound the count cache (WI-1917) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the redundant second arg-building loop in GetTimeRollup. - Hoist the ItemRepository onto HierarchyService and PermissionService instead of allocating one per call in hot helpers. - Fold the item-list count cache generation into its entry and add an opportunistic sweep, so the per-workspace map is count-bounded and cannot grow under workspace churn. A generation tombstone is only dropped once it has been expired longer than a load can be in flight. - Give the SQLite read and write pools the same connection lifetimes as Postgres (30m lifetime / 5m idle). Recorded findings: the rate-limiter buckets are already bounded (sharded, per-shard cap, idle sweep, LRU eviction), and the migration runner loads applied versions in one query — per-migration idempotency checks only run for unapplied migrations. A test now pins that. --- internal/database/database.go | 7 +++ internal/repository/item_count_cache.go | 64 +++++++++++++++++++++---- internal/repository/item_hierarchy.go | 3 -- internal/services/hierarchy.go | 32 ++++++------- internal/services/permission_cache.go | 4 +- 5 files changed, 80 insertions(+), 30 deletions(-) diff --git a/internal/database/database.go b/internal/database/database.go index 0f1e787d2..f770b2756 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "strings" + "time" "github.com/lib/pq" "modernc.org/sqlite" @@ -262,6 +263,10 @@ func NewDB(dataSourceName string, readConns, writeConns int) (*DB, error) { } db.SetMaxOpenConns(readConns) db.SetMaxIdleConns(readIdle) + // Match the Postgres pool: retire idle and long-lived connections so a + // connection that outlives a file swap or schema change is not reused. + db.SetConnMaxLifetime(30 * time.Minute) + db.SetConnMaxIdleTime(5 * time.Minute) // Create dedicated write connection with only 1 max connection to serialize writes writeConn, err := sql.Open("sqlite", connectionString) @@ -274,6 +279,8 @@ func NewDB(dataSourceName string, readConns, writeConns int) (*DB, error) { // raising it lets WAL handle a small amount of write concurrency. writeConn.SetMaxOpenConns(writeConns) writeConn.SetMaxIdleConns(writeConns) + writeConn.SetConnMaxLifetime(30 * time.Minute) + writeConn.SetConnMaxIdleTime(5 * time.Minute) if err := writeConn.Ping(); err != nil { _ = db.Close() diff --git a/internal/repository/item_count_cache.go b/internal/repository/item_count_cache.go index 6713f1613..6a0a8c1c0 100644 --- a/internal/repository/item_count_cache.go +++ b/internal/repository/item_count_cache.go @@ -18,20 +18,33 @@ import ( const ( itemListCountCacheTTL = 2 * time.Second itemListCountCacheLoadTimeout = 12 * time.Second + // Entries are keyed per workspace, so the map only grows under workspace + // churn. Sweep expired entries opportunistically and keep a hard bound even + // when a burst of distinct workspaces arrives inside one TTL window. + itemListCountCacheSweepInterval = time.Minute + itemListCountCacheMaxEntries = 4096 + // A generation tombstone is only dropped once it has been expired for + // longer than a load can still be in flight, so an invalidated in-flight + // load can never store its stale count after the tombstone is gone. + itemListCountCacheSweepGrace = itemListCountCacheLoadTimeout + 18*time.Second ) type itemListCountCacheEntry struct { count int expiresAt time.Time + // generation is bumped by invalidation. A load captures it before querying + // and only stores when it is unchanged, so a mutation that lands mid-flight + // wins. The entry is kept as a tombstone after invalidation so the + // generation survives until the sweep prunes it. + generation uint64 } var itemListCountCache = struct { sync.Mutex - entries map[string]itemListCountCacheEntry - generations map[string]uint64 + entries map[string]itemListCountCacheEntry + lastSweep time.Time }{ - entries: make(map[string]itemListCountCacheEntry), - generations: make(map[string]uint64), + entries: make(map[string]itemListCountCacheEntry), } var itemListCountFlights singleflight.Group @@ -69,7 +82,8 @@ func cachedItemListCount(ctx context.Context, db database.Database, workspaceID itemListCountCache.Unlock() return entry.count, nil } - generation := itemListCountCache.generations[key] + generation := entry.generation + sweepItemListCountCacheLocked(now) itemListCountCache.Unlock() loadCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), itemListCountCacheLoadTimeout) @@ -80,10 +94,11 @@ func cachedItemListCount(ctx context.Context, db database.Database, workspaceID } itemListCountCache.Lock() - if itemListCountCache.generations[key] == generation { + if itemListCountCache.entries[key].generation == generation { itemListCountCache.entries[key] = itemListCountCacheEntry{ - count: count, - expiresAt: time.Now().Add(itemListCountCacheTTL), + count: count, + expiresAt: time.Now().Add(itemListCountCacheTTL), + generation: generation, } } itemListCountCache.Unlock() @@ -106,6 +121,32 @@ func cachedItemListCount(ctx context.Context, db database.Database, workspaceID } } +// sweepItemListCountCacheLocked drops entries that have been expired for longer +// than a load can still be running, then evicts arbitrary entries if a burst +// pushed the map past its bound. Called with itemListCountCache held. +func sweepItemListCountCacheLocked(now time.Time) { + if now.Sub(itemListCountCache.lastSweep) < itemListCountCacheSweepInterval && + len(itemListCountCache.entries) <= itemListCountCacheMaxEntries { + return + } + itemListCountCache.lastSweep = now + + cutoff := now.Add(-itemListCountCacheSweepGrace) + for key, entry := range itemListCountCache.entries { + if entry.expiresAt.Before(cutoff) { + delete(itemListCountCache.entries, key) + } + } + // Still over the bound: a burst of distinct workspaces inside one TTL + // window. Drop arbitrary entries; the next read re-resolves them. + for key := range itemListCountCache.entries { + if len(itemListCountCache.entries) <= itemListCountCacheMaxEntries { + break + } + delete(itemListCountCache.entries, key) + } +} + // invalidateItemListCountCache drops one workspace total after a committed // membership change. Other replicas are bounded by the short TTL above. func invalidateItemListCountCache(db database.Database, workspaceID int) { @@ -114,8 +155,11 @@ func invalidateItemListCountCache(db database.Database, workspaceID int) { } key := itemListCountCacheKey(db, workspaceID) itemListCountCache.Lock() - itemListCountCache.generations[key]++ - delete(itemListCountCache.entries, key) + entry := itemListCountCache.entries[key] + entry.generation++ + // Keep the tombstone but force the next read to reload. + entry.expiresAt = time.Time{} + itemListCountCache.entries[key] = entry itemListCountFlights.Forget(key) itemListCountCache.Unlock() } diff --git a/internal/repository/item_hierarchy.go b/internal/repository/item_hierarchy.go index 70e340168..79c05288e 100644 --- a/internal/repository/item_hierarchy.go +++ b/internal/repository/item_hierarchy.go @@ -608,9 +608,6 @@ func (r *ItemRepository) GetTimeRollup(itemID, maxDepth, maxItems int, workspace for _, id := range ids { args = append(args, id) } - for i, id := range ids { - args[i] = id - } if err := r.db.QueryRow(query, args...).Scan(&rollup.TotalLoggedMinutes); err != nil { return nil, fmt.Errorf("failed to sum worklog duration: %w", err) } diff --git a/internal/services/hierarchy.go b/internal/services/hierarchy.go index 25c78e46a..d2b4e094b 100644 --- a/internal/services/hierarchy.go +++ b/internal/services/hierarchy.go @@ -19,12 +19,13 @@ const maxHierarchyDepth = 30 // HierarchyService handles all hierarchy-related operations using only parent_id type HierarchyService struct { - db database.Database + db database.Database + repo *repository.ItemRepository } // NewHierarchyService creates a new hierarchy service func NewHierarchyService(db database.Database) *HierarchyService { - return &HierarchyService{db: db} + return &HierarchyService{db: db, repo: repository.NewItemRepository(db)} } func itemPtrsToValues(items []*models.Item) []models.Item { @@ -50,7 +51,7 @@ func itemPtrsToValues(items []*models.Item) []models.Item { // mutate parent_id must use WouldCreateCycleTx so the check and the write // are atomic. func (h *HierarchyService) WouldCreateCycle(ancestorCandidateID, newParentID int) (bool, error) { - itemRepo := repository.NewItemRepository(h.db) + itemRepo := h.repo return walkForCycle(ancestorCandidateID, newParentID, itemRepo.GetParentID) } @@ -61,7 +62,7 @@ func (h *HierarchyService) WouldCreateCycle(ancestorCandidateID, newParentID int // where two concurrent reparents could each pass their cycle check and // together create a cycle. func (h *HierarchyService) WouldCreateCycleTx(tx database.Tx, ancestorCandidateID, newParentID int) (bool, error) { - itemRepo := repository.NewItemRepository(h.db) + itemRepo := h.repo return walkForCycle(ancestorCandidateID, newParentID, func(id int) (*int, error) { return itemRepo.GetParentIDTx(tx, id) }) @@ -93,18 +94,18 @@ func walkForCycle(ancestorCandidateID, newParentID int, getParent func(int) (*in // The recursive walk is capped at maxHierarchyDepth so a stored cycle can't // loop the DB. func (h *HierarchyService) GetAncestors(itemID int) ([]models.Item, error) { - return repository.NewItemRepository(h.db).GetAncestorsForHierarchy(itemID, maxHierarchyDepth) + return h.repo.GetAncestorsForHierarchy(itemID, maxHierarchyDepth) } // GetAncestorsContext is the request-aware form of GetAncestors. func (h *HierarchyService) GetAncestorsContext(ctx context.Context, itemID int) ([]models.Item, error) { - return repository.NewItemRepository(h.db).GetAncestorsForHierarchyContext(ctx, itemID, maxHierarchyDepth) + return h.repo.GetAncestorsForHierarchyContext(ctx, itemID, maxHierarchyDepth) } // GetAncestorsForItemsContext resolves ancestor chains for many items in one // recursive walk. Chains exclude the items themselves. func (h *HierarchyService) GetAncestorsForItemsContext(ctx context.Context, itemIDs []int) (map[int][]models.Item, error) { - return repository.NewItemRepository(h.db).GetAncestorsForItemsContext(ctx, itemIDs, maxHierarchyDepth) + return h.repo.GetAncestorsForItemsContext(ctx, itemIDs, maxHierarchyDepth) } // GetDescendants returns all descendants of an item @@ -117,7 +118,7 @@ func (h *HierarchyService) GetDescendantsContext(ctx context.Context, itemID, ma if maxDepth <= 0 || maxDepth > maxHierarchyDepth { maxDepth = maxHierarchyDepth } - items, err := repository.NewItemRepository(h.db).GetDescendantsWithMaxDepthContext(ctx, itemID, maxDepth) + items, err := h.repo.GetDescendantsWithMaxDepthContext(ctx, itemID, maxDepth) if err != nil { return nil, err } @@ -128,12 +129,12 @@ func (h *HierarchyService) GetDescendantsContext(ctx context.Context, itemID, ma // The recursive walk is capped at maxHierarchyDepth so a stored cycle can't // loop the DB. func (h *HierarchyService) CountDescendants(itemID int) (int, error) { - return repository.NewItemRepository(h.db).CountDescendants(itemID) + return h.repo.CountDescendants(itemID) } // CountDescendantsContext is the request-aware form of CountDescendants. func (h *HierarchyService) CountDescendantsContext(ctx context.Context, itemID int) (int, error) { - return repository.NewItemRepository(h.db).CountDescendantsContext(ctx, itemID) + return h.repo.CountDescendantsContext(ctx, itemID) } // GetChildren returns direct children of an item @@ -143,7 +144,7 @@ func (h *HierarchyService) GetChildren(itemID int) ([]models.Item, error) { // GetChildrenContext is the request-aware form of GetChildren. func (h *HierarchyService) GetChildrenContext(ctx context.Context, itemID int) ([]models.Item, error) { - items, err := repository.NewItemRepository(h.db).GetChildrenContext(ctx, itemID) + items, err := h.repo.GetChildrenContext(ctx, itemID) if err != nil { return nil, err } @@ -154,7 +155,7 @@ func (h *HierarchyService) GetChildrenContext(ctx context.Context, itemID int) ( // given accessible workspaces, so cross-workspace children are not disclosed // through a visible parent. func (h *HierarchyService) GetChildrenInWorkspacesContext(ctx context.Context, itemID int, workspaceIDs []int) ([]models.Item, error) { - items, err := repository.NewItemRepository(h.db).GetChildrenInWorkspacesContext(ctx, itemID, workspaceIDs) + items, err := h.repo.GetChildrenInWorkspacesContext(ctx, itemID, workspaceIDs) if err != nil { return nil, err } @@ -167,15 +168,14 @@ func (h *HierarchyService) GetChildrenInWorkspacesContext(ctx context.Context, i // nil so callers can't confuse it with "no parent". A non-existent input id // returns (nil, nil). func (h *HierarchyService) GetRoot(itemID int) (*models.Item, error) { - repo := repository.NewItemRepository(h.db) - rootID, err := repo.FindRootIDForItemContext(context.Background(), itemID, maxHierarchyDepth) + rootID, err := h.repo.FindRootIDForItemContext(context.Background(), itemID, maxHierarchyDepth) if err != nil { return nil, err } if rootID == nil { return nil, nil } - return repo.FindByIDWithDetails(*rootID) + return h.repo.FindByIDWithDetails(*rootID) } // GetEffectiveProject returns the effective project_id for an item by walking @@ -188,7 +188,7 @@ func (h *HierarchyService) GetRoot(itemID int) (*models.Item, error) { // true climbs to its parent's effective project); the legacy project_id = -1 // sentinel is no longer used. func (h *HierarchyService) GetEffectiveProject(itemID int) (projectID *int, inheritanceMode string, err error) { - res, err := repository.NewItemRepository(h.db).ResolveEffectiveProject(itemID) + res, err := h.repo.ResolveEffectiveProject(itemID) if err != nil { if errors.Is(err, sql.ErrNoRows) { // Non-existent item: nothing to resolve. diff --git a/internal/services/permission_cache.go b/internal/services/permission_cache.go index f74b249d4..70f7ac85c 100644 --- a/internal/services/permission_cache.go +++ b/internal/services/permission_cache.go @@ -23,6 +23,7 @@ import ( type PermissionService struct { cache *cacheutil.TypedCache[*models.UserPermissionCache] db database.Database + itemRepo *repository.ItemRepository cacheCommitMu sync.RWMutex cacheGeneration atomic.Uint64 @@ -102,6 +103,7 @@ func NewPermissionService(db database.Database, config PermissionCacheConfig) (* Shards: 64, }), db: db, + itemRepo: repository.NewItemRepository(db), workspaceAccess: newWorkspaceAccessCache(), ttl: config.TTL, batchSize: config.BatchSize, @@ -328,7 +330,7 @@ func (ps *PermissionService) IsSystemAdminContext(ctx context.Context, userID in // is intentionally not stored in permission snapshots because doing so would // couple item moves to authorization-cache invalidation. func (ps *PermissionService) GetItemWorkspaceID(_, itemID int) (int, error) { - workspaceID, err := repository.NewItemRepository(ps.db).GetWorkspaceID(itemID) + workspaceID, err := ps.itemRepo.GetWorkspaceID(itemID) if err != nil { if errors.Is(err, repository.ErrNotFound) { return 0, fmt.Errorf("item not found: %d", itemID)