fix(api): join session summaries via kv.list instead of per-session kv.get fan-out#1101
fix(api): join session summaries via kv.list instead of per-session kv.get fan-out#1101Codejune wants to merge 1 commit into
Conversation
…v.get fan-out api::sessions fired one kv.get(KV.summaries, id) per session inside a single Promise.all. Once the session count grows into the hundreds the burst of concurrent kv calls is never serviced, the promises neither resolve nor reject, and GET /agentmemory/sessions hangs permanently while each attempt parks its in-flight kv invocations on the worker (active_invocations climbs without bound, heap exhaustion, degraded status). The viewer dashboard and CLI status then render 0 sessions and 0% token savings. Replace the fan-out with a single kv.list(KV.summaries) and an in-memory join keyed by sessionId, the same pattern mem::diagnose already uses. Fixes rohitg00#1100
|
@Codejune is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthrough
ChangesSession summary lookup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/triggers/api.ts`:
- Around line 858-860: Remove the catch(() => []) fallback from the KV.summaries
read in the summary mapping, so failures from kv.list<SessionSummary> propagate
or are converted into the API’s explicit 5xx error response. Preserve the
existing mapping of successful results to [summary.sessionId, summary] entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| (await kv.list<SessionSummary>(KV.summaries).catch(() => [])).map( | ||
| (summary): [string, SessionSummary] => [summary.sessionId, summary], | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently replace summary-store failures with an empty result.
catch(() => []) converts any KV.summaries read failure into a successful response where every session lacks its summary. This silently produces incomplete data and can make dashboards/CLI report incorrect token-savings information. Let the failure propagate or return an explicit 5xx response.
Proposed fix
const summariesById = new Map(
- (await kv.list<SessionSummary>(KV.summaries).catch(() => [])).map(
+ (await kv.list<SessionSummary>(KV.summaries)).map(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (await kv.list<SessionSummary>(KV.summaries).catch(() => [])).map( | |
| (summary): [string, SessionSummary] => [summary.sessionId, summary], | |
| ), | |
| (await kv.list<SessionSummary>(KV.summaries)).map( | |
| (summary): [string, SessionSummary] => [summary.sessionId, summary], | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/triggers/api.ts` around lines 858 - 860, Remove the catch(() => [])
fallback from the KV.summaries read in the summary mapping, so failures from
kv.list<SessionSummary> propagate or are converted into the API’s explicit 5xx
error response. Preserve the existing mapping of successful results to
[summary.sessionId, summary] entries.
Fixes #1100
Problem
api::sessionsfires onekv.get(KV.summaries, id)per session inside a singlePromise.all. Once the session count grows into the hundreds (615 in my store), the burst of concurrent kv calls is never serviced: the promises neither resolve nor reject (no timeout onkv.get, and.catchonly handles rejections), soGET /agentmemory/sessionshangs permanently (HTTP 000 after 180s+) and every attempt parks its in-flight kv invocations on the worker —active_invocationsclimbs without bound (327 → 1,403 while a dashboard was polling), heap hits ~94%, health goesdegraded. Dashboard andagentmemory statusthen show 0 sessions / 0% token savings.Full bisection evidence is in #1100 (zero-fanout filter responds in 0.17s, list-only endpoints respond in ~50ms, only the unbounded fan-out deadlocks).
Fix
Fetch summaries with a single
kv.list(KV.summaries)and join in memory bysessionId— the same patternmem::diagnosealready uses. One engine roundtrip instead of N.Verification
/agentmemory/sessions: 0.02–0.08s, HTTP 200, all 114 summaries attached (previously: permanent hang)agentmemory status:Sessions: 615, Observations: 9,526(previouslySessions: 0)active_invocationsstable in single digits while polling the dashboard (previously leaking ~600 per request)npx tsc --noEmit: no new diagnostics insrc/triggers/api.ts(the 4 pre-existing ones there are at lines 252/288/2454, untouched)npm test: 1,402 passed / 13 failed — the same 13 environment-dependent tests (embedding-provider key detection, auto-compress env gate, hook-project cwd basename, fs-watcher timing) fail identically on unmodifiedmainin my environment;test/mcp-standalone-proxy.test.ts, the only suite touching/agentmemory/sessions, passesSummary by CodeRabbit