Skip to content

Key the timeline response cache by request shape, not maxSeq - #2218

Open
SawyerHood wants to merge 1 commit into
mainfrom
fix/2066-timeline-cache-per-shape
Open

Key the timeline response cache by request shape, not maxSeq#2218
SawyerHood wants to merge 1 commit into
mainfrom
fix/2066-timeline-cache-per-shape

Conversation

@SawyerHood

Copy link
Copy Markdown
Collaborator

What was wrong

createThreadTimelineCache keyed entries by ${maxSeq}|${paramsKey}. A thread's maxSeq is monotonic (getLatestThreadSequence = MAX(sequence)), so the moment an event is appended the previous entry for the same request window can never be looked up again, yet it stays strongly referenced until 127 more entries push it out of the LRU. During a streaming turn the web client refetches the same window after every event batch, so a single active thread fills all 128 slots with dead revisions within seconds. Reported by @Yazington in #2066; investigation report: https://get-bb.github.io/reports/issues/2066.html (on a seeded 9,001-event thread, 100 append+refetch rounds grew the post-GC heap by 19-22 MB on main vs ~3.6 MB with per-shape retention).

What changed

  • apps/server/src/services/threads/timeline-cache.ts: the cache is now keyed by paramsKey (request shape) and stores { maxSeq, value }. A hit requires an equal maxSeq. A miss deletes the slot before the row-cap check and re-inserts as most-recently-used, so an oversized newer revision (the streaming expanded-turn case) also drops the stale cached one rather than leaving it pinned. getOrBuild takes { paramsKey, maxSeq }.
  • apps/server/src/routes/threads/data.ts: computes paramsKey once and passes it to both the response cache and the latest-rows delta cache.
  • buildThreadTimelineCacheKey and the maxSeq field of ThreadTimelineCacheKeyArgs are deleted; nothing else used them. buildThreadTimelineParamsKey now takes the args type directly instead of Omit<..., "maxSeq">.
  • No wire change (server-internal only), no CLI/doc surface.

Relation to #2067 (draft, also by @Yazington, same root cause and same per-shape idea; credit to them for the diagnosis and the approach). This PR differs in that: (1) it stores the bare maxSeq number instead of a revisionKey string that re-embeds the whole params key, which is redundant once the map is already keyed by shape; (2) it removes the now-unused buildThreadTimelineCacheKey helper and maxSeq key field instead of keeping them; (3) it ships the tests the #2067 body describes but does not contain: a signature-independent route-level regression plus unit coverage for "oversized replacement evicts the stale revision" and "older-maxSeq request never receives the newer value"; (4) it is based on current main (#2067 is ~68 commits behind).

How you verified

  • New route test apps/server/test/public/public-thread-timeline-cache-retention.test.ts drives the real GET /api/v1/threads/:id/timeline against in-memory SQLite through 150 append+refetch rounds with the same request shape and asserts cache.size === 1. It only wraps the cache factory to read .size; it does not name the getOrBuild signature, so it runs unchanged before and after. Before the fix: AssertionError: expected 128 to be 1. After: passes.
  • apps/server/test/services/threads/timeline-cache.test.ts adapted to the new signature, with added cases: new maxSeq replaces the prior revision (size stays 1), an over-cap replacement drops the cached revision, an older-maxSeq request is never served the newer value, separate shapes are retained independently, LRU eviction still works.
  • pnpm exec turbo run test --filter=@bb/server: 199 files / 1901 tests passed.
  • pnpm exec turbo run typecheck lint --filter=@bb/server: clean.

Fixes #2066

AGENT GENERATED: by Claude Opus 5

The thread timeline cache keyed entries by `${maxSeq}|${paramsKey}`. A
thread's maxSeq only increases, so every appended event produced a new key
and left the previous revision of the same request window resident in the
128-entry LRU even though it could never be looked up again. During a
streaming turn the client refetches the same window after every event
batch, so one active thread filled the cache with dead revisions.

Key the cache by paramsKey and store `{ maxSeq, value }`. A hit requires an
equal maxSeq; a miss deletes the slot before the row-cap check (so an
oversized newer revision also drops the stale one) and re-inserts as MRU.
buildThreadTimelineCacheKey and the maxSeq field of
ThreadTimelineCacheKeyArgs had no other users and are removed.

Adds a route-level regression test that drives GET /threads/:id/timeline
through 150 append+refetch rounds and asserts cache.size is 1 (was 128).

Fixes #2066

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timeline cache retains obsolete revisions for the same request shape

1 participant