fix(storage): stop session retention from evicting live sessions - #928
Merged
github-actions[bot] merged 8 commits intoJul 30, 2026
Merged
Conversation
Session keys are {StartTime.UnixNano()}_{ID}, and enforceSessionRetention
deleted the lowest keys. That evicts the LONGEST-LIVED session first, which
is exactly the session most likely to still be connected and working. A
client that stayed connected while 100 newer sessions were created vanished
from storage, went missing from the sessions API, and its later close/stat
updates had no record to find.
Choose victims by usefulness instead of key order:
1. closed sessions, stalest first - finished work, nothing will write to
these records again;
2. only if that frees too little, active sessions by last activity,
stalest first - an abandoned client goes before a working one.
Status is authoritative for liveness and activity only ranks within a tier,
so a connected-but-idle client outranks a closed session that churned more
recently; CloseInactiveSessions is what flips a truly dead session to
closed, and only then does it become evictable.
Tier 2 keeps the cap absolute. Refusing to evict active sessions at all
would let an all-active bucket (abandoned clients, a reconnect loop that
never closes cleanly) grow without bound, which is worse than the bug being
fixed.
Two further defects surfaced in the same function:
- bucket.Stats().KeyN does not count records Put earlier in the same write
transaction, so the bucket settled permanently at 101 records rather than
100. The scan now counts keys directly. This one is observable: the new
cap assertions failed with "expected 100, actual 101" before the change.
- Records were deleted while iterating the bucket's own cursor. bbolt
documents that as unsafe ("Changing data while traversing with a cursor
may cause it to be invalidated and return unexpected keys and/or
values"). A 2000-key probe did not manage to make it skip, so this is a
documented hazard rather than an observed failure - but sorting requires
collecting keys first regardless, which is also what
PruneExcessActivities in this package already does.
Retention also falls back to StartTime when LastActivity is unset, so
records predating that field do not all sort as year 1 and get evicted
ahead of genuinely stale sessions.
The suite is mutation-verified. An early version of the live-session test
passed with the status tier removed - its live session also had the
freshest activity, so the staleness ordering alone kept it - so the
connected-but-idle case was added to pin the tier itself.
Deploying mcpproxy-docs with
|
| Latest commit: |
c98c537
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e60f2fb9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-session-retention-evicts.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30522428451 --repo smart-mcp-proxy/mcpproxy-go
|
Cross-model review round 1 on PR #928. All four findings were verified against the code before being taken. Migration bypassed the cap (P2). CreateSession was the only caller of retention, but migrateLegacySessions is a second write path that can move an arbitrary number of records into the sessions bucket. A process that migrated and then only updated or closed existing sessions - never creating a new ID - would sit above the cap indefinitely. Retention now runs at database-open time, right after the migration, which also repairs a bucket left oversized by any older version for any reason. Selection is now bounded (P2). Instead of collecting and sorting every record, keep a heap of the best maxSessions seen so far and stream the rest into the victim list: O(n log maxSessions) time and O(maxSessions) ranking state, rather than O(n log n) and O(n). This matters on the one pass that can see a bucket far above the cap - the open-time trim above. Unreadable records now have their own tier (P2). The policy said they evict first; the code could not deliver it. An unmarshal failure left the zero value, which is indistinguishable from a VALID record whose status is not "active" and whose timestamps are unset, so the two shared a tier and a timestamp and key order decided between them - the usable record could be deleted while the corrupt one survived. Ranking is now an explicit tier (unreadable < closed < active) rather than a pair of derived booleans. The zero-activity test proved too little (P3). It only asserted that a zero-activity record survives, which also passes if the fallback is replaced by time.Now() - a substitution that would make every legacy record eternally the freshest thing in the bucket. It now pins both directions in one bucket: a zero-activity record with an ancient StartTime must be evicted, one with a recent StartTime must survive. New tests: migration leaves the bucket within the cap and spares the user login; an unreadable record is evicted ahead of a valid zero-valued record that sorts before it by key; a 500-record bucket trims to exactly the newest 100 in one call. All mutation-verified, including the time.Now() substitution named in the review.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Mutation testing found the bounded selection was under-tested: replacing the "candidate is worse than every survivor" short-circuit with a constant false passed the whole suite. Every existing test feeds records whose rank happens to ascend with their key, so the short-circuit branch was never taken - the displacement branch was always the correct one. Add the case that needs it: a fleet of long-lived active sessions at low keys, then a burst of closed sessions with newer start times at high keys. Every closed record ranks below every active one despite sorting after it, so the selection has to reject them outright rather than displace records it already decided to keep.
Cross-model review round 2 on PR #928. The heap bounded the SORT but not the memory. Every rejected or displaced candidate was appended to a victims slice, key and all, so an oversized bucket still cost O(n) state - on precisely the path the bounding was introduced for, the open-time repair of an old database. The comment claimed O(maxSessions) state; the code did not deliver it. Retention is now two passes: Pass 1 ranks and keeps only the heap of the best maxSessions records. Victims are counted, not collected - remembering them is what made state O(n). Pass 2 deletes everything that is not a survivor, in bounded batches, reading keys only. Deleting still cannot happen during traversal, so each batch is collected, the traversal abandoned, the batch deleted, and the scan resumed from the first key the batch did not reach. Peak state is O(maxSessions + retentionDeleteBatch) whatever the bucket size, and the expensive JSON decode still happens exactly once per record across both passes. Also pin what "runs on every open" actually means. The existing regression test only fed records in through the legacy migration, so moving the retention call back inside migrateLegacySessions would still have passed while silently dropping the repair of an already-oversized namespaced bucket. The new test seeds the namespaced bucket directly, closes, reopens, and asserts the trim - no legacy bucket anywhere in the picture. The far-above-the-limit test now uses 1200 records so the delete pass is split across several batches, which is the only way the resume path gets exercised, and asserts the survivors are exactly the newest 100 with no gaps - the shape a mis-resumed scan would break. It seeds in a single transaction, which also takes it from 4.5s to 0.06s.
… not Cross-model review round 3 on PR #928. The finding is correct and the claim was wrong for the third time on this branch. The 512-key batches bound the explicit victim-key slice. They do not bound peak memory for the trim. Every Bucket.Delete seeks to the key and calls Cursor.node(), which materialises the containing leaf page as an in-memory node cached in the bucket and retained until the transaction spills at commit. A trim touching P pages holds O(P) - in practice O(n) - however the victim keys are batched. Measured: a 100k-record trim holds ~90 MB inside the transaction (10k holds ~9 MB, linear as expected), of which the batched victim slice is ~27 KB. Batching bounds ~0.03% of the peak. Restating rather than re-engineering, deliberately. Delivering the bound means committing between batches, which trades away the property that migration and retention apply atomically under bbolt's exclusive file lock. That trade is not worth making for a bucket that has no known way to get large: enforceSessionRetention has been called from CreateSession since session tracking was introduced (it is present in the commit that added the feature), and now runs on every open too, so the bucket has never been uncapped. The realistic oversized case is the off-by-one that let it settle at 101 records, not 10^5. No behaviour change - comments only. The batching stays: it is correct, tested and does bound one real term, just not the dominant one.
Cross-model review round 3 confirmation pass, plus two corrections of my own. Comments only, no behaviour change. "No known way to occur" was too broad, and contradicted the comment two lines up: enforceSessionRetentionOnOpen promises to repair an oversized bucket whatever the reason, so claiming nothing can produce one cannot also be true. Direct bbolt manipulation, a database import or merge, or any other process writing the file can produce an arbitrarily large bucket. The claim is now scoped to supported in-process write paths, which is what the evidence actually establishes, and out-of-band writes are named as the excluded case that keeps the open-time repair justified. The in-process claim is now stated as the audit that supports it rather than as a bare assertion: only two writers add keys - CreateSession, with retention immediately after every insert, and the legacy migration, with retention immediately after at open. The other six writers Put on a key they already located by scanning and cannot grow the bucket. Also separate measured from computed. The ~90 MB was measured; the ~27 KB victim slice is arithmetic on retentionDeleteBatch. Both sat under one "Measured" lead-in, which is how a derived number ends up being trusted as an observed one.
…aviour TestEnforceSessionRetention_OnlyTouchesItsOwnBucket still claimed retention 'keeps the newest 100 by raw key order with no type check'. This PR removed exactly that: records are now decoded and ranked by usefulness. The test's real subject is containment — retention touches only the sessions bucket — so say that, and point at sessions_retention_test.go for eviction order.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arming auto-merge: QA PASS. The every-other-key skip is definitively fixed — before: 101 survivors as every other record; after: exactly 100 contiguous newest, with every survivor position asserted rather than counts. Batch/resume boundaries tested at 1, 511, 512, 513, 1023, 1024, 1025 victims with each evicted and surviving key checked individually. Five deliberate attempts to evict a live session all held, including a working session at the oldest key with freshest activity. Memory measurements independently corroborate the commit's ~90MB figure. E2E green (12m32s) now that the Landlock fix has landed.
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
… gained a status filter (#939) Semantic merge conflict, not a logic bug: #928 added sessions_retention_test.go calling GetRecentSessions(limit) while #930 added a status parameter for the tray glance's client filtering. Both PRs were green independently and broke main only once combined, because branch protection does not require a PR to be up to date with main before merging. Pass "" — the documented no-filter value — at the four call sites. The retention tests want every surviving record, so unfiltered is the correct argument, not merely the compiling one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
enforceSessionRetentionkeeps the 100 most recent sessions by deleting the oldest keys — and keys are{StartTime.UnixNano()}_{ID}, so eviction is ordered by start time with no regard for status. A long-lived active session is therefore deleted once 100 newer sessions exist:/api/v1/sessionsreports a connected, working client as absent, and later close/stat updates have no record to find.Bonus defect the reproduction exposed
The cap was never 100 — it was 101.
bucket.Stats().KeyNis page-level and does not reflect same-transaction writes, and retention runs insideCreateSession's transaction right after the newPut. The count was permanently one low. The pre-existing test missed it because it commits its records before calling retention.Fix — two-tier eviction
Victims are chosen by usefulness, not key order: closed sessions first, stalest first; then, only if that frees too little, active sessions by last activity, stalest first. Status is authoritative for liveness; activity only ranks within a tier.
Alternatives rejected on evidence, not taste:
TestEnforceSessionRetention_OnlyTouchesItsOwnBucket(130 all-active records must trim to 100) would correctly have gone red.An abandoned
activesession — a client that died without closing — is handled by tier 2: its stale last-activity sorts it ahead of the working session, so the cap stays absolute for any status mix. Retention logs a warning when forced into tier 2.Testing
Reproduced first: 3 failures against unmodified logic, the headline being
session not found: live-session— the connected client's record gone from storage entirely. 6 tests after the fix, plus the pre-existing retention/migration tests.Four mutants, each run alone against the full storage suite: remove the status tier → 1 fail; remove the zero-last-activity fallback → 1; restore
Stats().KeyN→ 4; invert staleness → 3. Nothing outside the new file failed under any mutation.Worth noting: the status-tier mutant initially passed everything, because the first live-session fixture also had the freshest activity, so staleness ordering rescued it and the tier was never exercised. A connected-but-idle case was added to pin it.
Gates
go build ./...and-tags serverOK ·go test ./internal/storage/... ./internal/runtime/... ./internal/httpapi/...all pass ·golangci-lint(v2, strict) 0 issues.