You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found by integration review. This is the MaxCachedIdx fail-open that #40 deliberately scoped out (as "A3"), now with a measured trigger.
Problem
Reproduced with MaxEntries: 20 (pin cap = max/2 = 10): 40 frozen decisions from distinct sessions fill the pin budget. A new session's cg:len: key is then written unpinned, evicts under churn, and:
prevLen → 0 → MaxCachedIdx → -1 → Ctx.TailOnly(i) returns true for every index → offloaders mutate the cached prefix.
That is fail-open in the expensive direction: mutating already-cached content forces the provider to re-write the whole suffix at 11.5× the read price.
#40 pinned cg:len: precisely to prevent this. The pin works — until the budget is full, at which point the protection silently stops applying to new sessions.
Severity: lower than it looks — scope it honestly
The proxy chat path is not affected. It uses modes.Tracker (in-memory, defaultMaxSessions: 1000), which never consults cg:len:. Only /compact still routes through apply.BodyFull → the legacy prevLen(st, …) path.
So the exposure is offline replay and eval runs, not live traffic — a measurement-accuracy risk rather than a production cache regression. Worth stating plainly, because an offline eval that silently mutates cached prefixes produces numbers that do not describe what ships, and this project has already been burned twice by evals measuring a different component than production runs (/compact hard-coding the context window as unknown; internal/modelinfo never resolving a window at all).
Desired behavior
Pick one, and document why:
Make /compact use modes.Tracker too, so there is one boundary-derivation path and the legacy prevLen route disappears. Cleanest, and removes a whole class of eval-vs-production divergence.
Or invert the fail direction: when the boundary is genuinely unknown, treat it as fail-closed (TailOnly false — mutate nothing) rather than fail-open. fix(cache): slide the store TTL, pin frozen decisions, and repair lost ones #40's author noted prevLen resets to 0 deliberately, because after eviction we do not know what the provider cached — which is an argument for refusing to mutate, not for mutating everything.
Or reserve a pin slot per live session for cg:len: (2–4 bytes each) so the budget cannot exclude it.
components/component.go — Ctx.TailOnly, MaxCachedIdx (the < 0 ⇒ true-for-all branch).
store/store.go — Options.PinPrefixes, the max/2 pin cap, expired-first eviction.
modes/tracker.go — the in-memory boundary the chat path actually uses.
proxy/proxy.go — the /compact handler.
Testing plan
Reproduce: small MaxEntries, saturate the pin budget from N sessions, then assert a new session's /compact request does not mutate a message below the boundary.
Assert the chosen fail direction explicitly, with a test named for the invariant.
Confirm the chat path is unaffected by the same saturation (regression guard, so the two paths cannot silently diverge again).
Acceptance criteria
Pin-budget saturation cannot cause a cached-prefix mutation on any route.
A test that fails on today's code.
One documented boundary-derivation path, or an explicit note in docs/design.md on why two exist and how they differ.
Found by integration review. This is the
MaxCachedIdxfail-open that #40 deliberately scoped out (as "A3"), now with a measured trigger.Problem
Reproduced with
MaxEntries: 20(pin cap = max/2 = 10): 40 frozen decisions from distinct sessions fill the pin budget. A new session'scg:len:key is then written unpinned, evicts under churn, and:prevLen→ 0 →MaxCachedIdx→-1→Ctx.TailOnly(i)returns true for every index → offloaders mutate the cached prefix.That is fail-open in the expensive direction: mutating already-cached content forces the provider to re-write the whole suffix at 11.5× the read price.
#40 pinned
cg:len:precisely to prevent this. The pin works — until the budget is full, at which point the protection silently stops applying to new sessions.Severity: lower than it looks — scope it honestly
The proxy chat path is not affected. It uses
modes.Tracker(in-memory,defaultMaxSessions: 1000), which never consultscg:len:. Only/compactstill routes throughapply.BodyFull→ the legacyprevLen(st, …)path.So the exposure is offline replay and eval runs, not live traffic — a measurement-accuracy risk rather than a production cache regression. Worth stating plainly, because an offline eval that silently mutates cached prefixes produces numbers that do not describe what ships, and this project has already been burned twice by evals measuring a different component than production runs (
/compacthard-coding the context window as unknown;internal/modelinfonever resolving a window at all).Desired behavior
Pick one, and document why:
/compactusemodes.Trackertoo, so there is one boundary-derivation path and the legacyprevLenroute disappears. Cleanest, and removes a whole class of eval-vs-production divergence.TailOnlyfalse — mutate nothing) rather than fail-open. fix(cache): slide the store TTL, pin frozen decisions, and repair lost ones #40's author notedprevLenresets to 0 deliberately, because after eviction we do not know what the provider cached — which is an argument for refusing to mutate, not for mutating everything.cg:len:(2–4 bytes each) so the budget cannot exclude it.Relevant code
apply/apply.go—prevLen/putLen,maxCachedIdxderivation.components/component.go—Ctx.TailOnly,MaxCachedIdx(the< 0⇒ true-for-all branch).store/store.go—Options.PinPrefixes, the max/2 pin cap, expired-first eviction.modes/tracker.go— the in-memory boundary the chat path actually uses.proxy/proxy.go— the/compacthandler.Testing plan
MaxEntries, saturate the pin budget from N sessions, then assert a new session's/compactrequest does not mutate a message below the boundary.Acceptance criteria
docs/design.mdon why two exist and how they differ.go test -racegreen.