Studio: make token cache prefix eviction one-directional so two live fences cannot live-lock

This commit is contained in:
danielhanchen 2026-08-18 21:53:39 +00:00
parent c987dd53a0
commit d0ea96d85e
4 changed files with 62 additions and 2 deletions

View file

@ -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<T>(options: TokenCacheOptions): TokenCache<T> {
// 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);
}
}

View file

@ -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");

View file

@ -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",

View file

@ -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",
}