Skip to content

fix(offload): kept-verbatim marks are global, so one session's expand exempts that content for every session #175

Description

@amiddavid

What happens

keptKey carries no session, so a kept-verbatim mark is global to the store:

// components/offload/state.go
func keptKey(ck string) string { return "cg:keep:" + ck }

func MarkKeptVerbatim(st store.Store, original string) {
	st.Put(keptKey(contentKey(original)), []byte{1})
}

func isKeptVerbatim(c *components.Ctx, ck string) bool {
	_, ok := c.Store.Get(keptKey(ck))   // no c.Session
	return ok
}

The mark's purpose is narrow and per-session: session A expanded this content, so re-compacting it there would just make A expand it again — a per-turn bounce loop. But the key has no session in it, so the exemption applies to every session sharing the store.

Why it costs money rather than correctness

skipReduce consults isKeptVerbatim for every offloader, so a mark written by one session makes that content permanently uncompactable for all of them. Nothing breaks: requests are correct, just larger.

The distribution is what makes it worth fixing rather than noting. Content that appears in many sessions is exactly the content most worth compacting, and it is also the content most likely to have been expanded by someone. So the leak lands preferentially on the highest-value candidates, it accumulates monotonically (marks are never cleared), and it is invisible: the offloader records kept_verbatim_after_expand, which is indistinguishable from the legitimate same-session case.

A shared-store deployment converges toward "the union of everything anyone ever expanded is exempt for everyone".

The fix, and why it is not a one-liner

Put the session in the key — cg:keep:<session>:<contentkey> — and thread it through MarkKeptVerbatim (which the proxy's expand loop calls with the session id already in hand) and isKeptVerbatim (which has c.Session). An empty session must be a no-op write, not a fallback to a global key, or the leak returns through the default path.

Two things make it more than a rename:

  1. Marks already on disk are under the old key shape and would all be ignored at once. A session mid-expand-loop when the new binary rolls would lose its exemption and start bouncing. So the read side wants to accept both shapes for a deprecation window, with only the new shape written.
  2. KeptVerbatim(st, original) is also read by the expand repair to charge a restored original to the dashboard exactly once. Whether that reader wants session scoping is a separate question — a global "this was already counted" may well be what it wants, since the double-count it prevents is per-store. Scoping both without asking would be assuming they are the same question.

Provenance

Found while re-cutting PR #80 onto main. That branch had already made these marks session-scoped, and its two tests for the property fail on main because the leak is real there:

  • TestKeptVerbatimDoesNotLeakAcrossSessions — session B must not inherit session A's exemption, and A must keep its own.
  • TestMarkKeptVerbatimIgnoresAnEmptySession — an empty session must write nothing.

Both are on the re-cut branch as t.Skip with their bodies preserved commented, pointing here. They were not adapted to the two-argument signature deliberately: an adapted body would compile and read as a real test while asserting nothing, because both sessions would share one global mark. Un-skip and restore them verbatim when the scoped key lands.

Filed separately rather than fixed inside PR #80 because it is a store key-format change touching every offloader, plus a migration decision — none of which is about co-reference.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions