fix(store): give rewind payloads their own shorter horizon, and count the reclamations a replay absorbs

[amiddavid]

Closes #190.

The gap, and why the issue said it could not be decided yet

#188 bounded the rewind reserve but left a slot released only by the TTL, with both namespaces sharing one. The store is a single process-wide instance, so a busy period could hold the reserve saturated for the whole of ttl_seconds — ~2.8 h at the default — and while it is saturated every new removal is refused for every session at once. Savings fall to zero long after the load that caused it is gone.

The issue listed five options and said picking without a re-run of iteration 024 would be another guess at a default. The re-run has not happened and, per the validation session, will not arrive first — iteration 024 ran on a commit that predates #188, so stash_refused has never been observed at all, and the issue's own gating plan is circular (the re-run needs #190 either chosen or deliberately left unfixed and instrumented).

The choice turns out to be decidable from the code, because the issue's stated objection to option 3 is not what the tree does.

The mechanism the issue did not account for

A payload, unlike a frozen decision, is re-derivable from the request in flight:

• A frozen decision is the replacement bytes the provider already cached. Nothing else holds them; losing one flips an already-cached message and re-writes the suffix at ~11.5x the read price. It has to survive a whole long-horizon task including idle gaps — which is what DefaultTTL is sized for.
• A payload is a copy of content the agent re-sends every turn. Every offloader replays its frozen decision on every turn regardless of the cache-tail gate — it must, or the message reverts full→compacted→full and churns the KV cache (mask.go:79, "Reapply a previously-frozen mask on EVERY turn") — and that replay calls commitRefresh, which PutStashes the payload again from the message text it just read.

So a live marker's payload has its deadline slid every turn, and one already reclaimed is re-created on the request path — before the request goes upstream, and therefore before any expand call in the response could ask for it.

payload TTL 1800s  |  decision TTL 10000s

turn N   : marker replayed -> PutStash refreshes payload (expiry slides)
idle 45m : payload swept, decision survives (pinned, 10000s)
turn N+1 : reapplyFrozen -> commitRefresh -> PutStash RE-CREATES payload
           ...then the request goes upstream, expand resolves. OK.

abandoned session: nothing replays -> reclaimed after 30m, not 2.8h

The horizon a payload needs is one inter-turn gap, not one session. That makes this option 3 arrived at by mechanism rather than by guess — and it introduces no irreversibility (option 1), needs no arbitrary share number (option 2, which #188 declined for exactly that reason), and needs no session-end signal that does not exist (option 4).

What changed

• store.Options.StashTTLSeconds (stash_ttl_seconds), DefaultStashTTL = 1800s. Not a fresh guess: 1800s is the value DefaultTTL's own comment records as too short for a frozen decision (headroom's CCR store), reused in the one namespace whose horizon it does fit.
• Capped at ttl_seconds. A payload outliving the decision that names its marker is memory nothing can ever read, so shortening ttl_seconds gets the shorter of the two rather than a reserve held open by dead payloads — which would be this same saturation arrived at by config.
• Memory.ttlFor picks the horizon per entry, keyed on the stash flag rather than the key (a payload's key is a bare content hash the store cannot recognise — see Stasher). Applied at every write and at Get's sliding refresh, Put included.
• stash_revived / cg_stash_revived_total: payloads written again under a key the TTL had taken. stash_expired reported two outcomes at once — a payload nobody wanted, and a payload an outstanding marker still needs — and those call for opposite responses. Same ambiguous-counter shape #188 split stash_refused/stash_missing over, and that proxy: an unrecognised usage dialect is indistinguishable from no usage, and degraded silently for 4,015 requests #200 states as a general rule.

One help text that named the wrong remedy

cg_stash_missing_total, /stats and two docs pages all said the fix for a dangling marker is ttl_seconds. A replay re-stashes the payload it re-derived, so a marker only dangles when that write was also refused — the remedy is the reserve first (max_entries / stash_max_bytes), and stash_ttl_seconds only if stash_expired is what is taking the payloads. That was already incomplete before this change; corrected here because this change alters what expiry means, so leaving it would have made it wrong rather than merely partial.

The exposure this leaves

A turn that runs no pipeline performs no refresh — an x-context-guru-bypass request, or the agent-compaction bypass — so an unbroken run of bypassed turns longer than stash_ttl_seconds could outlive a payload whose marker is still live. Both are single-request events in practice, and if it happens the outcome is the already-reported one (stash_missing), not a silent loss. Documented at docs/reference/config.md.

Verification

gofmt -l . clean, go vet ./... clean, go test ./... all packages pass, go test -race clean over store, components/offload, proxy, metrics, dash, cmd (Go 1.26.4, eval box).

Six new tests, each revert-verified to fail without the change:

Test	Reverted →
store.TestPayloadsExpireSoonerThanTheDecisionsThatNameThem	"the reserve released no slot at the payload horizon (2h46m40s)"
store.TestAReclaimedPayloadRewrittenByAReplayIsCountedAsRevived	"the payload outlived stash_ttl_seconds"
store.TestThePayloadHorizonNeverOutlivesTheDecisionHorizon	"stash_ttl_seconds must stay configurable, got 2h46m40s"
store.TestDefaultPayloadHorizonIsWellInsideTheDecisionHorizon	guards the constants' relationship
store.TestBecomingAPayloadLowersTheSweepBound	"the sweep skipped an expired payload because nextExpiry still held the long horizon"
offload.TestAReclaimedPayloadIsReDerivedBeforeItsMarkerGoesUpstream	the load-bearing one — see below
offload.TestAReclaimedPayloadThatCannotBeReStashedIsReportedMissing	"the payload outlived stash_ttl_seconds"

The re-derive test was verified against a second mutation, not only the shared-horizon revert, because that revert only proves the reclamation happened. With commitRefresh changed to read the payload instead of re-stashing it, the test fails on its own subject — "the marker on the wire does NOT resolve after its payload was reclaimed". Without that second check the assertion could have been passing on a payload that never left. If this test ever fails, DefaultStashTTL goes back to DefaultTTL and not the other way round.

One bug this change introduced and its own new test caught: when an entry becomes a payload its deadline moves earlier (ttl → stashTTL), and nextExpiry — the lower bound that lets sweepExpired skip a pass — was not lowered to match. A bound left above an entry's real expiry makes the sweep return early, so that reserve slot is never reclaimed by any TTL: the exact permanent saturation this PR is about, reintroduced by the fix for it. TestBecomingAPayloadLowersTheSweepBound is sized so the become-a-payload write is the only thing that can lower the bound, and noteExpiry's comment no longer claims a plain write's deadline is "the latest of any live entry", which two horizons make false.

Two contract tests updated as they are designed to require: TestEverySnapshotFieldIsExportedOrExempt (StashRevived is sourced from StashStats() like its four siblings, so it is listed in notExportedWhy for that reason) and TestStatsShapeIsUnchanged (stash_revived added to the reviewed contract, in alphabetical position).

Not in this change, deliberately

Option 5 from the issue's comment — gating extract_llm's exploring() on reserve health. It bounds a different cost (repeated model calls during saturation) in a different package and needs its own decision about the threshold; this change reduces the duration of the episode it is about. Left on the issue.

What the next run should read

stash_revived against stash_expired, with stash_missing. Revived tracking expired means the shorter horizon is being absorbed by the per-turn re-stash exactly as argued above; expired climbing while revived stays flat is also fine (sessions that never came back — the reclamation this exists to do); stash_missing moving is the one that says the reserve is genuinely undersized. That is the measurement #190 asked for, and it now arrives on the first run rather than needing an instrumented arm.

🤖 Generated with Claude Code

[amiddavid]

Reviewed the mechanism first, as you asked. The load-bearing claim survives for the component the test drives (mask via reapplyFrozen), and I could not break it — the re-derivation really does happen on the request path, commitRefresh really is unrefusable, and TestAReclaimedPayloadIsReDerivedBeforeItsMarkerGoesUpstream is non-vacuous (the expand.Resolve before turn 2 is what forces the reclamation, and the second mutation you describe is the right one).

What I think is wrong is the universal quantifier: "every offloader replays its frozen decision on every turn" is not true of every offloader, and the two ways it fails have different failure shapes — one of which is not the stash_missing your exposure paragraph promises. Both are in the comments/docs rather than the code, so neither is a revert; the second one I would want stated before this merges, because the docs table is what an operator will reason from.

Answers to your two specific questions are inline at ttlFor (a) and at the cap (b). Short version: (a) I audited all five e.expires = sites and believe the fix is complete — but the completeness rests on stashTTL <= ttl, so I would make it structural rather than audited. (b) the cap is right, but it is silent and /config reports the pre-cap value.

Nothing here blocks on a re-run, and the stash_revived/stash_expired split is the right shape.

[amiddavid]

The quantifier is too strong: a component's replay phase can be skipped by a gate that sits ahead of it.

reapplyFrozen-based offloaders (mask, cmdfilter, collapse, failed_run, skeleton, readlifecycle, agentdiet) do behave as this comment says. Two do not:

• summarize.Offload returns at summarize.go:157 on !s.trigger.Fires(...) || end <= start, and again at :162/:165 when no model client resolves — all before tryReuse, which is where its commitRefresh pair lives. A turn with no cheap-model credential refreshes none of summarize's span payloads.
• extract_llm.Offload returns at extract_llm.go:659 (no_goal_keywords) before Phase 1 at :827, which is the "reapply frozen compactions on every turn" block.

At 10,000s these gaps were almost always absorbed. At 1,800s a 30-minute run of them is not exotic, and one of them is not a single-request event the way x-context-guru-bypass is: the agent's own compaction shrinks the incoming request, which can drop it back under Trigger.MinRequestTokens (3,000 by default) for several consecutive turns while markers are still live in the compacted transcript — the interaction #67 just landed for. That makes summarize's trigger skip recurring, which is exactly the shape the exposure paragraph at :315 assumes does not exist.

Two ways out, and I don't think it matters much which: hoist the replay/refresh phase above those gates (a refresh is cheap and unrefusable, so there is no reason it should be downstream of a model gate), or narrow this comment and the docs/reference/config.md exposure list to name per-component skips. I'd prefer the hoist for summarize's model == nil case specifically, since "the cheap model is down" is a state that persists for many turns by nature.

[amiddavid]

"the outcome is the already-reported one — stash_missing" is component-dependent, and for four offloaders it is the other counter plus a silent flip.

dedup.go:70, extract.go:82, linecap.go:146 and smartcrush.go:88 call commitMark and have no reapplyFrozen/commitRefresh path at all (git grep -n 'commitMark(\|reapplyFrozen(' components/offload/*.go). Their per-turn re-stash therefore goes through commitMark, which is the refusable half of the pair — skipReduce is false on a re-sent original, so they redo the transformation every turn and land on PutStash via commitMark.

While the payload is live that write is PutStash's refresh branch, which is retained unconditionally. Once the payload has been reclaimed it is a new stash, and a new stash into a saturated reserve is refused → continue → the message goes upstream verbatim after earlier turns sent it compacted. That is a full-suffix cache write, and it is booked as stash_refused, whose operator-facing text in promexport.go and docs/reference/routes.md promises "the content was left verbatim and nothing became irreversible". True as far as reversibility goes, and misleading about the cost actually paid.

This is reachable at 10,000s too, so it is not a regression this PR introduces — but shortening the horizon shortens the distance to it by 5.5x, and the paragraph above tells a reader the worst case is a counter that in this path never moves. Worth a sentence here and a row in the docs/reference/config.md table.

[amiddavid]

(a) — I audited it and believe you got all of them; here is the audit so it is on the record, plus a way to stop relying on one.

Every site that assigns e.expires:

site	direction	noted?
PutStash refresh, :576	can move earlier (ttl → stashTTL)	yes, your fix
PutStash new, :593	new entry	yes, :610
Put refresh, :691	later only	not needed
Put new, :702	new entry	yes, :713
Get slide, :801	later only	not needed

The two unnoted ones are safe because stashTTL <= ttl is enforced in NewMemory and now is monotonic: now₁ + stashTTL > now₀ + stashTTL for any refresh, and no path ever clears e.stash, so an entry's horizon never lengthens mid-life either. So no sibling — but note that the correctness of :691 and :801 is now a consequence of the answer to your question (b). If the cap ever goes, someone has to redo this table.

Concrete suggestion: fold it into one place, e.g. func (m *Memory) setExpiry(e *entry) { e.expires = m.now().Add(m.ttlFor(e)); m.noteExpiry(e.expires) }, and use it at all five sites. noteExpiry on a deadline that only moves later is a no-op by construction, so the unconditional version costs one comparison and makes a sixth write site unable to get this wrong — which is the same completeness argument #198 is open about for the gateExempt table.

[amiddavid]

(b) — keep the cap. Two smaller things about it.

The cap is the right default, and the reason given (a payload nothing can name is a slot held for nothing) is the one that matters. I would not add an escape hatch: an operator who wants payloads to outlive decisions is asking for the #190 saturation by configuration, and there is no workload argument for it that isn't better served by raising ttl_seconds.

Two notes:

1. The justification is slightly overstated. "A payload outliving the decision that names its marker is memory nothing can ever read" — the model can call expand on a marker it read in an earlier turn's context even after the frozen decision has expired and that message has gone upstream verbatim, because the marker is in the conversation the model is reasoning over, not only in the request the proxy just built. That path is rare and short-lived, and it does not change the conclusion; I'd just not lean on "ever".

2. The cap is applied silently, and /config reports the pre-cap value. cmd/context-guru-proxy/main.go:921 publishes cfg.Store.StashTTLSeconds, so stash_ttl_seconds: 20000 with ttl_seconds: 10000 shows 20000 on /config and the dashboard while the store uses 10000. That is the same silent-divergence shape fix(proxy): tell an unreadable usage block apart from a provider that reported none, and record the shape once #205 is about, in the config surface. Either report the effective value (the store would need to expose it) or log once at startup when the cap bites. stash_ttl_seconds: 0 reporting 0 rather than 1800 is the same pre-existing pattern as ttl_seconds, so I'd leave that alone unless you want to fix the family.

[amiddavid]

A zero on this pair is not evidence, and "the measurement arrives on the first run" needs that caveat.

sweepExpired is called only from StashRoom (store.go:522), PutStash's pre-refusal path (:586), and evictOldest (:889) — i.e. only when the reserve, the shared exempt budget, or the entry cap is already binding. In a run that never saturates, expired payloads are simply never swept: PutStash's refresh branch does not check expiry, so an expired-but-unswept entry is resurrected in place (your own test comment at payload_rederive_test.go:66 says exactly this) and neither stash_expired nor stash_revived moves.

That is the correct behaviour — the slot is released at the moment the slot is wanted — but it means the PR's closing claim that the measurement "now arrives on the first run rather than needing an instrumented arm" holds only for a run that actually saturates the reserve. That is the same precondition iteration 024 failed to meet for stash_refused, which is how #190 ended up undecidable from data in the first place. Worth saying in "what the next run should read": stash_expired = stash_revived = 0 means the reserve never bound, not the horizon is working, and the thing that distinguishes those is stash_refused / StashLive against Capacity.

[amiddavid]

48e0f09 addresses all five. Re-read the whole store diff rather than just the fixup, and I have no objection left — LGTM from me (GitHub will not let this account file a formal approval on its own PR, so read this as one).

Specifically: setExpiry is now the only thing that stamps a deadline, including the two new-entry sites, which is what makes the invariant structural instead of an audit — and moving Put's construction to a bare &entry{} so there is no second place a deadline can come from is more than I asked for and the right call. EffectiveStashTTLSeconds deriving from NewMemory rather than re-implementing the rule is the correct shape. The narrowed re-derivation claim reads as the honest version now, and your bound on it — a skipped component splices nothing, so nothing dangles while the skip lasts, and the exposure is only that its next firing may find the payload gone and the reserve full at the same moment — is a better statement than the finding I raised. Declining to hoist summarize's replay above its model gate is right on this PR: it is a behaviour change in a component with its own test burden, and it is noted rather than buried.

The metrics.Snapshot paragraph on both-counters-at-zero is the change I care most about, because it is the difference between the next run producing an answer and producing another undecidable issue.

Two notes below, neither blocking and neither worth another round.

[amiddavid]

Non-blocking: this loop cannot fail. EffectiveStashTTLSeconds(o) is defined as int(NewMemory(o).stashTTL / time.Second), so the assertion is A == A for every shape in the table — it will keep passing through any change to the resolution rule, including a wrong one. The three literal assertions above it are the ones carrying this test.

Not an argument for deleting it: a future refactor that re-implements the rule inside the helper would start being caught, which is presumably the intent. But the drift this PR actually fixed is between effectiveConfig's published value and the store, and the loop does not span that — cmd/context-guru-proxy/main.go could go back to cfg.Store.StashTTLSeconds tomorrow and every assertion in this file would still pass. A test that fails on the regression has to read the map effectiveConfig returns (got["store"].(map[string]any)["stash_ttl_seconds"]) for Options where the cap bites, which is one assertion in the cmd package.

[amiddavid]

Trivial, for the record rather than for action: this builds a whole Memory — list plus three maps — to read one duration. Fine as it stands, because effectiveConfig is called once at startup (main.go:289) and the coupling to NewMemory is the property you wanted. Worth knowing only so nobody later moves the caller onto a request path, or gives NewMemory a side effect (a goroutine, a file handle) without noticing that this reads it. If either looks likely, lifting the two lines of resolution into a resolveStashTTL(o Options) time.Duration that both NewMemory and this call keeps the no-drift property without the allocation.

[amiddavid]

e07c7af closes both notes, and the second one better than I asked. LGTM, nothing outstanding (this account cannot file a formal approval on its own PR).

The new cmd/context-guru-proxy.TestConfigPublishesTheEffectivePayloadHorizon does span the drift, including in the part I would have called decorative: got comes from the map effectiveConfig builds and store.EffectiveStashTTLSeconds(cfg.Store) from the helper, so if main.go ever goes back to publishing the raw field those two diverge and it fails — it is not the A == A the removed loop was. The literal tc.want rows cover the defaults case on top of that, which is the one a same-function comparison could not have caught. Revert-verifying it against main.go publishing the raw field is the right check for a test whose whole purpose is to fail on that regression.

Removing the store-side loop with the note rather than silently is what makes it not come back. Agreed on leaving EffectiveStashTTLSeconds building a Memory.

One thing worth keeping from this exchange, since you said you were writing it into your notes: a test in the wrong package to fail and a test that is true by construction are the same defect wearing different clothes — both read as coverage from the outside, and neither can distinguish a working implementation from a broken one. The tell is the same in both cases: ask what edit would make it fail, and if the answer is "an edit to the assertion itself", it is not a test.