diff --git a/studio/frontend/src/components/assistant-ui/code-token-cache.ts b/studio/frontend/src/components/assistant-ui/code-token-cache.ts index 69339cb13f..59d4d489a0 100644 --- a/studio/frontend/src/components/assistant-ui/code-token-cache.ts +++ b/studio/frontend/src/components/assistant-ui/code-token-cache.ts @@ -17,7 +17,8 @@ // storing frame N drops frames 1..N-1 of the same fence immediately. A // streaming reply occupies ONE entry while it streams, not one per window. // A size cap alone would not do this: the prefixes would sit in the cache -// until later replies pushed them out one at a time. +// until later replies pushed them out one at a time. It is ONE-DIRECTIONAL on +// purpose; see the comment at the eviction itself. // A CHARACTER CAP tokens cost roughly 17x their source in retained heap, so a budget in // characters of source is a budget in megabytes. A count cap alone would let // 64 large fences pin far more than 64 small ones. @@ -87,7 +88,20 @@ export function createTokenCache(options: TokenCacheOptions): TokenCache { // the rest, so this walk is safe. for (const [otherKey, other] of entries) { if (other.group !== group) continue; - if (code.startsWith(other.code) || other.code.startsWith(code)) { + // ONE DIRECTION ONLY: drop what the new code EXTENDS, never what extends it. + // + // Evicting both ways live-locks the app. Two fences can be on screen at once where one is + // a prefix of the other, which is what a page showing the same snippet at two lengths + // does. Storing the long one would evict the short one, the short one's next render would + // miss and evict the long one, and so on: a miss returns null, which schedules an + // asynchronous tokenisation, whose callback re-renders, which misses again. Measured on + // the pair below before this was one-directional: two misses per round, forever, with the + // cache stuck at one entry. It hung a CI job for thirty minutes. + // + // The direction kept here is the one the memory win needs. Every earlier frame of a + // GROWING fence is a prefix of the current one, so this still collapses a streamed reply + // onto one entry. The reverse case, a source that SHRINKS, is left to the size cap. + if (code.startsWith(other.code)) { drop(otherKey, other); } } diff --git a/studio/frontend/tests/code-token-cache.test.ts b/studio/frontend/tests/code-token-cache.test.ts index 84edebfb5d..889df8bdcc 100644 --- a/studio/frontend/tests/code-token-cache.test.ts +++ b/studio/frontend/tests/code-token-cache.test.ts @@ -61,6 +61,31 @@ test("a streamed fence occupies one entry, not one per refresh window", () => { } }); +test("two fences on screen at once, one extending the other, both stay cached", () => { + // The live-lock this exists to stop. A miss returns null to the renderer, which schedules an + // asynchronous tokenisation, whose callback re-renders and asks again. If storing the longer + // fence evicted the shorter one AND storing the shorter one evicted the longer, two fences on + // screen would miss forever and the page would never go idle. That hung a CI job for thirty + // minutes before eviction was made one-directional. + const cache = roomy(); + const short = "def f():\n return 1\n"; + const long = `${short}def g():\n return 2\n`; + let misses = 0; + for (let round = 0; round < 8; round += 1) { + for (const code of [short, long]) { + if (cache.get(GROUP, code) === null) { + misses += 1; + cache.set(GROUP, code, `tokens-${code.length}`); + } + } + } + // Three at most: each fence missing once, plus one re-fetch if the first store evicted it. + assert.ok(misses <= 3, `the cache never settled: ${misses} misses over 8 rounds`); + assert.equal(cache.stats().entries, 2); + assert.equal(cache.get(GROUP, short), `tokens-${short.length}`); + assert.equal(cache.get(GROUP, long), `tokens-${long.length}`); +}); + test("prefix eviction does not reach across groups", () => { const cache = roomy(); cache.set(OTHER_GROUP, "const a", "ts-tokens"); diff --git a/studio/frontend/tests/mutants/code-token-cache-mutants.mjs b/studio/frontend/tests/mutants/code-token-cache-mutants.mjs index 756b397128..dce50a8494 100644 --- a/studio/frontend/tests/mutants/code-token-cache-mutants.mjs +++ b/studio/frontend/tests/mutants/code-token-cache-mutants.mjs @@ -39,6 +39,14 @@ const MUTANTS = [ from: " if (code.startsWith(other.code) || other.code.startsWith(code)) {\n drop(otherKey, other);\n }", to: " if (false) {\n drop(otherKey, other);\n }", }, + { + // The shipped bug: evicting in BOTH directions live-locks two fences that are on screen at + // once when one extends the other. + name: "prefix eviction runs in both directions", + test: "two fences on screen at once, one extending the other, both stay cached", + from: " if (code.startsWith(other.code)) {", + to: " if (code.startsWith(other.code) || other.code.startsWith(code)) {", + }, { name: "prefix eviction ignores the group", test: "prefix eviction does not reach across groups", diff --git a/tests/studio/test_playwright_suites_run_in_ci.py b/tests/studio/test_playwright_suites_run_in_ci.py index 4b607b9750..bfa7181993 100644 --- a/tests/studio/test_playwright_suites_run_in_ci.py +++ b/tests/studio/test_playwright_suites_run_in_ci.py @@ -40,6 +40,19 @@ NOT_IN_CI = { # can go wrong silently, the verdict in harness_failures, is driven without a # browser by test_autoscroll_harness_contract.py, which CI does run. "playwright_thread_weight.py", + # A measurement harness rather than a gate: it prints the retained-heap slope per streamed + # code fence that #9228 was sized from, and sets no budget. It needs a forced GC between + # samples, so it is Chromium-with-CDP only, and the sizes that make the slope mean anything + # (six 32 KB fences per arm across four arms, plus a tick-rate ladder) cost tens of minutes. + # Run by hand when that slope needs re-measuring. What can go wrong silently in it, a settle + # predicate that reports "finished" while highlighting is still queued, is covered without a + # browser by studio/frontend/tests/code-token-cache.test.ts, which CI does run. + "playwright_shiki_retention.py", + # The same, for frame timing: it accumulates 40 streamed replies to separate the arms by + # hundreds of megabytes of retained heap before recording a single frame, which is minutes per + # arm, and it reports a comparison rather than gating on it. Its result was a NULL (the + # retained heap costs bytes, not frames), so there is no budget here to defend. + "playwright_shiki_jank.py", }