perf(store): add opt-in TTL cache to getStatus for polling callers#4
perf(store): add opt-in TTL cache to getStatus for polling callers#4lukeboyett wants to merge 4 commits intofix/db-transaction-typefrom
Conversation
On large indexes (millions of rows, tens of GB of SQLite) the
`COUNT(DISTINCT d.hash)` scan inside `getHashesNeedingEmbedding`
dominates `getStatus()` cost. For long-lived processes that hit this
path repeatedly — upstream's own MCP server calls `store.getStatus()`
on status/stats requests, and external HTTP/daemon wrappers do too —
every poll pays the full cost even though the result barely changes
between calls.
Default behavior is unchanged. The `Store.getStatus` signature now
takes an optional `{ ttlMs }`; callers that want fresh results pass
nothing (or `ttlMs: 0`). Callers that know they poll can pass
`ttlMs: 5000` or similar to reuse a recent snapshot. Cache scope is
per-Store instance, not module-global, so multiple stores don't
cross-contaminate. A call that omits `ttlMs` (or passes 0) clears the
cached value so the next polling-mode call re-populates it.
No call sites are changed in this PR — the cache is opt-in. MCP,
CLI `qmd status`, and SDK callers keep their existing fresh-each-call
semantics until they explicitly opt in.
Test (`test/status-cache.test.ts`): uses vitest fake timers and a
spy on `db.prepare` to assert
- default calls re-query every time,
- `ttlMs > 0` skips re-query within the window,
- `ttlMs=0` behaves identically to no options,
- a fresh (no-options) call invalidates the cache so the next
polling call re-populates.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e9d479db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ttl > 0 && statusCache && statusCache.expiresAt > now) { | ||
| return statusCache.value; |
There was a problem hiding this comment.
getStatus returns the cached snapshot whenever statusCache.expiresAt > now, but that expiration was set by the previous caller’s TTL. If one caller populates the cache with a long TTL (e.g., 60s) and a later caller asks for a shorter TTL (e.g., 1s), this branch still serves data older than the later caller requested. This makes ttlMs non-authoritative in mixed-caller scenarios and can return unexpectedly stale status from a shared Store instance.
Useful? React with 👍 / 👎.
Codex P2 on #4: the original implementation stored the cache entry as `expiresAt = fetchedAt + ttlMs` using the *first* caller's TTL. If a later caller passed a shorter TTL, the cache was still considered valid as long as the original expiry had not passed, so the second caller could silently receive data older than their requested window. Fix: - Store `fetchedAt` (the snapshot's timestamp) rather than `expiresAt`. - On each call compute `now - fetchedAt` and compare against *this* caller's `ttlMs`. Each caller's freshness contract is now honored regardless of what a previous caller asked for. Test: - New `each caller's ttlMs is authoritative against the data's age` covers the mixed-TTL scenario: caller A (60s TTL) seeds the cache, caller B (1s TTL) two seconds later must re-fetch, caller C (60s TTL) immediately after B reuses B's fresh snapshot.
|
Addressed in 0f31594. P2 "Respect per-call ttlMs when reusing cached status" — valid catch. The cache now stores New test @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f31594436
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const now = Date.now(); | ||
| if (ttl > 0 && statusCache && (now - statusCache.fetchedAt) < ttl) { | ||
| return statusCache.value; | ||
| } | ||
| const fresh = getStatus(db); | ||
| statusCache = ttl > 0 ? { value: fresh, fetchedAt: now } : null; |
There was a problem hiding this comment.
getStatus captures now before executing the expensive DB query and then stores that pre-query timestamp in statusCache.fetchedAt. When the query itself takes a meaningful amount of time (the exact large-index case this cache targets), the cached snapshot is immediately considered older than it really is, which shortens or can entirely negate the caller’s ttlMs window (e.g., query time >= TTL causes a miss on the very next call). Stamping fetchedAt after getStatus(db) returns keeps TTL behavior aligned with the actual data age.
Useful? React with 👍 / 👎.
Codex P2 on #4 round 2: the original fix captured `now = Date.now()` before running the expensive `getStatus(db)` query and then wrote that pre-query timestamp into `statusCache.fetchedAt`. The large- index case that motivates this cache is exactly where the query takes meaningful time, so the cached snapshot is aged by the query's duration — a follow-up call with a short TTL can get a cache miss on the very next invocation. Fix: capture `Date.now()` a second time after `getStatus(db)` returns and stamp with the post-query value. The cache check (`Date.now() - fetchedAt < ttl`) is unchanged, so each call still compares the snapshot's age against the caller's TTL. Test: `fetchedAt is stamped after the query completes, not before` uses a monotonic Date.now() spy that advances 100ms per call. After the first `getStatus({ ttlMs: 10000 })` the stamp is strictly greater than 0 (the spy's starting clock). An immediate follow-up with `ttlMs: 200` sees an observed age of one tick (100ms) and reuses the cached snapshot. A buggy pre-query stamp of 0 would push the observed age above 200ms and cause an unintended fresh query.
|
Addressed in 2f384da. P2 "Record fetchedAt after status query completes" — legit. Previously I captured New test @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f384dad24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let store: Store; | ||
|
|
||
| beforeAll(() => { | ||
| enableProductionMode(); |
There was a problem hiding this comment.
Calling enableProductionMode() here flips a module-global flag and this suite never restores it, so later tests in the same Vitest process can observe production mode unexpectedly. In particular, tests that rely on test-mode behavior (like the createStore()-without-path guard) become order-dependent and may fail when this file runs first or is run alongside them. Please reset with _resetProductionModeForTesting() in teardown (or otherwise isolate this state) to avoid cross-test contamination.
Useful? React with 👍 / 👎.
Codex P2 on #4 round 3: `enableProductionMode()` flips a module- global flag. The new `test/status-cache.test.ts` suite enabled it in `beforeAll` but never restored, leaking the flag into any later test file running in the same vitest process (the repo sets `fileParallelism: false`, so state carries over linearly). Tests that rely on the test-mode `createStore()`-without-path guard would become order-dependent. Fix: call `_resetProductionModeForTesting()` in `afterAll` to restore the pre-suite flag value. Matches the pattern in the existing store/MCP suites.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Adds an opt-in TTL cache to
Store.getStatus()for callers that poll it. Default behavior is unchanged; every existing call site keeps fresh-each-call semantics.Motivation
store.getStatus()runs aCOUNT(DISTINCT d.hash)scan (viagetHashesNeedingEmbedding) plus aGROUP BY collectionand a total-doc count. On small indexes this is fast; on large ones (millions of documents, tens of GB of SQLite) each call is expensive.Long-lived processes that repeatedly hit this path pay the full cost every time:
store.getStatus()from status/stats requests (src/mcp/server.ts:110,:515).For these pollers the result barely changes between calls. A short TTL cache is a large wall-clock win; for interactive CLI use the cache is unused and behavior is identical.
What's in it
Store.getStatusaccepts an optional{ ttlMs }parameter.ttlMs > 0, the result is cached per-Storeinstance; repeat calls within the window return the cached snapshot.ttlMsis omitted or0, the call runs fresh and clears any previously-cached value, so interleaving fresh + cached calls stays coherent.Tests
test/status-cache.test.ts:vi.spyOn(db, "prepare")call count increases on each callttlMs > 0skips re-query within the window (fake timers), then re-queries once the window expiresttlMs = 0behaves identically to no optionsNotes
fix/db-transaction-typebecause a cleanmaincurrently fails to build; that baseline build fix is the parent PR.