mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 16:02:45 +00:00
Neil
a3512fcd69
The LRU cap added in 20fb0b3f6e bounded memory by discarding a safety property.
Reported as a review concern, reproduced before believing it, and confirmed:
`sessionTabsPublicationEpochHistoryByWorktree` is deliberately RETAINED after a
worktree's live record is dropped — it is the tombstone fence that stops a
sibling stream's late frame from a retired publisher being applied. A pure LRU
evicts exactly those tombstones, by construction: a still-publishing worktree
renotes its epoch on every accepted frame, so the eviction victim is always a
fence.
Absence is fail-OPEN on both read paths. `hasRetiredValue` answers false for a
missing entry, so `isRetiredSessionTabsPublicationEpoch` cannot tell "never seen"
from "fenced and forgotten" — and `recordReceivedWebSessionTabsSnapshot` then
RE-NOTES the evicted epoch as current, resurrecting the publisher the fence
retired.
MEASURED, not argued. Fence a worktree, note 512+ others, deliver the retired
publisher's late frame: with the tombstone retained it is rejected; after
eviction it is ACCEPTED.
The first version of that repro passed and was wrong. It gave each worktree its
own runtime id, so the retired RUNTIME-ID fence rejected the frame and the epoch
fence was never consulted — a test green for an unrelated reason. A real sibling
stream on one environment shares the runtime id; with one shared id throughout,
the acceptance reproduces. Recorded here because the fixture detail is the whole
difference between a passing test and a real one.
THE FIX: the cap yields to the fence rather than the other way round. Each entry
carries `notedAt`, and eviction stops at the first entry younger than
SESSION_TABS_PUBLICATION_FENCE_RETENTION_MS. The map may exceed 512 while every
entry is still young; memory is then bounded by worktree churn WITHIN the
retention window instead of by count, which is the bound that can be held
without discarding a live fence.
512 IS NOT THE JUSTIFIED NUMBER, and it was right to challenge it — it was a
memory target with nothing behind it. The justified number is the retention
window: 4x the tab RPC budget. A frame in flight longer than that budget has
already been abandoned by the transport, so a fence older than 4x it has outlived
anything it could fence against. The count cap is now only the memory target, and
it is the one that gives way.
Both growth tests are updated to advance the clock, which also makes them
faithful: a long-running session is long in TIME, and a count-only fixture
measures the map at an instant where nothing is evictable and no bound can hold
without dropping a live fence.
Also writes the structural-assertion lesson into the teardown test's header
rather than leaving it in a commit message: the original perf assertion was a
wall-clock ratio that flaked at 22.3x against a 20x bound, and was replaced with
a `.keys()` spy, because the enumeration IS the cost. A duration assertion fails
for reasons unrelated to the property it protects and gets retried away, taking
the real regression with it.
Mutation: removing the retention guard (back to pure LRU) kills exactly the
"keeps a young fence past the cap" assertion, and leaves every bound assertion
passing — the bound tests and the fence test are cleanly separated.
Verification: 190 files / 1595 tests pass, log grepped = 0. pnpm tc and oxlint
clean.