Refactor refreshStatsCache: lazy on-demand caching with TTL

[Joxx0r]

Summary

• Replace the 30s setInterval timer in refreshStatsCache() with a lazy getStatsCache() function that computes stats on-demand with a 30s TTL — no background work when idle
• Decouple computeInheritanceDepth() from stats caching into a separate debounced post-ingest trigger (5s debounce after last ingest batch)
• Simplify /stats and /summary endpoints to use the unified lazy cache

Closes #57

Test plan

• All 55 existing tests pass (npm test)
• /stats and /summary return valid data on first call (lazy compute)
• Two rapid /stats calls return cached data (TTL prevents recomputation)
• After ingest, depth recomputation fires ~5s later (check logs)
• No lingering timers after service shutdown

🤖 Generated with Claude Code

[claude]

Review: Refactor refreshStatsCache — lazy on-demand caching with TTL

Overall this is a clean, well-motivated refactor. The RETURNING-based upsertAssetBatch optimization and the consolidation of getAssetStats() into getStats() are both good improvements. Passes all 55 existing tests, CI green. A few things to look at:

---

Bug: getStatsCache() can return null on first call if refreshStatsCache() fails

function getStatsCache() {
  const now = Date.now();
  if (statsCache && (now - statsCacheTime) < STATS_TTL_MS) {
    return statsCache;
  }
  refreshStatsCache();   // catches errors internally, statsCache stays null
  statsCacheTime = now;
  return statsCache;     // may be null
}

refreshStatsCache() swallows errors and leaves statsCache = null. If the DB is unavailable on the very first /stats or /summary request, res.json(null) gets sent as a 200 — clients will see null instead of an error response.

The old setInterval had the same cold-start gap, but since the interval didn't call refreshStatsCache() at startup either, this isn't a regression — both paths have a window. Still worth hardening with a null guard in the endpoints:

app.get('/stats', (req, res) => {
  try {
    const data = getStatsCache();
    if (!data) return res.status(503).json({ error: 'Stats not yet available' });
    res.json(data);
  } catch (err) { ... }
});

---

Minor: app._depthDebounceTimer goes stale after the timer fires

Inside scheduleDepthRecompute():

depthDebounceTimer = setTimeout(() => {
  depthDebounceTimer = null;   // closure var cleared
  ...
}, DEPTH_DEBOUNCE_MS);
app._depthDebounceTimer = depthDebounceTimer;  // set to the live timer ID

After the callback runs, depthDebounceTimer (closure) is null, but app._depthDebounceTimer still holds the expired timer ID. clearTimeout on an already-fired timer is harmless, so the after() hook in the test works fine — but the exposed handle is misleading. Easy fix:

depthDebounceTimer = setTimeout(() => {
  depthDebounceTimer = null;
  app._depthDebounceTimer = null;  // keep in sync
  ...
}, DEPTH_DEBOUNCE_MS);

---

Test coverage gaps

The three unchecked items in the test plan are all observable behaviours that tests could cover without much effort:

• TTL cache hit: call /stats twice within 30s, assert refreshStatsCache isn't invoked twice (e.g. spy / mock Date.now to stay within TTL). This is the core invariant of the refactor.
• Timer debounce: ingest a batch, assert scheduleDepthRecompute schedules exactly one timeout, not N.
• No lingering timers: already covered by the after() cleanup, but an explicit test that app._depthDebounceTimer is null after the timer fires would be worthwhile.

Not blocking, but given that the motivation for this PR is "no background work when idle", the TTL behaviour in particular deserves an automated assertion.

---

Positive notes

• RETURNING in upsertAssetBatch is a genuine win — eliminates the follow-up batch SELECT entirely.
• Removing the setInterval eliminates idle CPU and makes the service easier to reason about during shutdown.
• scheduleDepthRecompute() decoupling from the stats cache is the right call — depth recomputation is write-path work; stats reads shouldn't trigger it.
• statsCacheTime = 0 after trigram build to force the next request to recompute is correct.

[claude]

Requesting changes on two points before merging: the null-return path in getStatsCache() (sends a 200 with null body when the DB is unavailable at cold-start), and the stale app._depthDebounceTimer reference after the debounce fires. Both are small fixes. Happy to approve once those are addressed — the overall direction is good.

[claude]

Both CHANGES_REQUESTED issues from the previous review are resolved in commit 548c38a:

1. Null guard: /stats and /summary now return 503 when getStatsCache() returns null — correct fix.
2. Stale timer ref: app._depthDebounceTimer = null is now cleared inside the debounce callback.

Minor nit (not blocking): statsCacheTime = now in getStatsCache() is set unconditionally even on failure. Not a correctness issue — the TTL check requires statsCache to be truthy, so null-cache calls always retry regardless. Just slightly misleading code.

Test coverage for the TTL hit behavior is still absent, but that was already called non-blocking. Could be a follow-up.

CI green, logic sound. Approving.