Skip to content

fix(store): pin-budget exhaustion can reopen the TailOnly fail-open on /compact #47

Description

@OsherElhadad

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-1Ctx.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.

Relevant code

  • apply/apply.goprevLen/putLen, maxCachedIdx derivation.
  • components/component.goCtx.TailOnly, MaxCachedIdx (the < 0 ⇒ true-for-all branch).
  • store/store.goOptions.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.
  • go test -race green.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    Status
    New/ToDo

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions