fix: dedupe double-triggered summaries and unchanged-input re-processing on session stop - #1061
fix: dedupe double-triggered summaries and unchanged-input re-processing on session stop#1061yulin0629 wants to merge 6 commits into
Conversation
|
@yulin0629 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
A single stop-hook invocation used to run mem::summarize twice: once via the direct POST /agentmemory/summarize, and once via /agentmemory/session/end -> event::session::stopped. Each run re-summarized the entire session from scratch, so every session stop cost 2x full-history LLM calls (observed: duplicate ~230KB requests per stop, growing as the session accumulated observations). - stop hook: drop the direct summarize call; session/end already fans out to the summarize path. - mem::summarize: serialize per-session runs with withKeyedLock (same pattern as mem::observe) and short-circuit when the stored summary's observationCount already matches — observations are append-only, so an unchanged count means the LLM input would be identical.
481611b to
d5741ae
Compare
📝 WalkthroughWalkthroughShutdown flows now rely on session-end handling for summarization. The summarize handler serializes work per session, skips unchanged observation sets, and handles lock timeouts. Graph extraction also skips unchanged inputs. Reflection uses bounded graph snapshots, and health thresholds use the V8 heap limit. ChangesSession-stop deduplication
Reflection scalability
Heap-aware health thresholds
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StopFlow
participant SessionEnd
participant SummarizeHandler
participant GraphTrigger
participant KV
participant LLMProvider
StopFlow->>SessionEnd: POST /agentmemory/session/end
SessionEnd->>SummarizeHandler: Trigger mem::summarize
SummarizeHandler->>KV: Compare inputFingerprint
alt Fingerprint differs
SummarizeHandler->>LLMProvider: Generate summary
LLMProvider-->>SummarizeHandler: Return summary
SummarizeHandler->>KV: Persist summary and fingerprint
else Fingerprint matches
KV-->>SummarizeHandler: Skip LLM workflow
end
SessionEnd->>GraphTrigger: Handle event::session::stopped
GraphTrigger->>KV: Compare graph extraction fingerprint
alt Fingerprint differs
GraphTrigger->>KV: Persist fingerprint
GraphTrigger->>LLMProvider: Trigger mem::graph-extract
else Fingerprint matches
KV-->>GraphTrigger: Skip graph extraction
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses redundant, double-triggered session summarization by removing an extra stop-hook summarize call and adding a per-session in-flight guard plus an “unchanged input” short-circuit in mem::summarize, with accompanying tests to cover the race/skip behavior.
Changes:
- Remove the direct
/agentmemory/summarizecall from stop hooks so stop only triggers/agentmemory/session/end(which already fans out to summarization). - Serialize
mem::summarizeper session viawithKeyedLockand add a skip path when the stored summary appears up-to-date. - Add tests covering the skip path, concurrency collapse, and re-summarize-on-growth behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/functions/summarize.ts |
Adds per-session keyed lock and an “up-to-date” short-circuit to avoid duplicate LLM work. |
src/hooks/stop.ts |
Removes the extra summarize request, relying on session-end fanout. |
plugin/scripts/stop.mjs |
Mirrors stop hook behavior change in the packaged script output. |
test/summarize.test.ts |
Adds tests for the new dedup/locking behavior. |
Comments suppressed due to low confidence (1)
src/hooks/stop.ts:49
- This hook now only fires a single HTTP request (
/agentmemory/session/end), but the forced-exit timer is still 1500ms. Since an unawaitedfetch()keeps the event loop alive, this can unnecessarily delay hook completion vs. other single-request hooks insrc/hooks/which use 500ms.
fetch(`${REST_URL}/agentmemory/session/end`, {
method: "POST",
headers: authHeaders(),
body: JSON.stringify({ sessionId }),
signal: AbortSignal.timeout(5000),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const existing = await kv.get<SessionSummary>( | ||
| KV.summaries, | ||
| sessionId, | ||
| }); | ||
| return { | ||
| success: false, | ||
| error: "no_provider", | ||
| reason: | ||
| "No LLM provider key set; Summarize is a no-op. Set ANTHROPIC_API_KEY (or GEMINI/OPENROUTER/MINIMAX) in ~/.agentmemory/.env to enable.", | ||
| }; | ||
| } | ||
| ); | ||
| if (existing && existing.observationCount === compressed.length) { |
| @@ -38,7 +32,7 @@ async function main() { | |||
| setTimeout(() => process.exit(0), 1500).unref(); | |||
| } | |||
![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/functions/summarize.ts`:
- Around line 244-282: The session lock in the summarize handler can remain held
indefinitely while produceSummaryXml runs. Bound the provider call used by
produceSummaryXml with the existing timeout mechanism, covering Anthropic and
agent-sdk as well as the already bounded providers, so hung summarization
releases withKeyedLock; alternatively, move the existing observation-count
up-to-date check before lock acquisition only if it remains race-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7c79df3-c4c1-4e44-bfd0-4f7bd40b4182
📒 Files selected for processing (4)
plugin/scripts/stop.mjssrc/functions/summarize.tssrc/hooks/stop.tstest/summarize.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/summarize.test.ts (1)
548-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the "permanently hung" case; consider also covering "eventually resolves after timeout".
This test only proves the lock frees when the provider never settles. It doesn't exercise a provider that resolves shortly after the lock timeout fires — which is the scenario that can produce a stale overwrite of a newer summary (see the critical-issue comment on
src/functions/summarize.tsLines 68-91,473-475). Once that's fixed, a regression test resolving the hung provider after the timeout and asserting the later summary isn't clobbered would be valuable.🤖 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 `@test/summarize.test.ts` around lines 548 - 573, Extend the timeout coverage around the handler setup and provider summarize flow to use a provider promise that resolves after SUMMARIZE_LOCK_TIMEOUT_MS, then complete a newer follow-up summary before resolving the timed-out run. Assert that the late result does not overwrite the newer summary, while preserving the existing timeout and lock-release assertions.src/functions/summarize.ts (1)
306-328: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndependent kv operations can run in parallel.
kv.list(KV.observations(sessionId))(Line 306-308) and the laterkv.get(KV.summaries, sessionId)(Line 325-328) don't depend on each other and could be issued together viaPromise.all. Likewisekv.set(KV.summaries, ...)(Line 434) andsafeAudit(...)(Line 435-438) write to independent keys and don't need to be sequential.♻️ Proposed parallelization
- const observations = await kv.list<CompressedObservation>( - KV.observations(sessionId), - ); + const [observations, existingBeforeRun] = await Promise.all([ + kv.list<CompressedObservation>(KV.observations(sessionId)), + kv.get<SessionSummary>(KV.summaries, sessionId), + ]); const compressed = observations.filter((o) => o.title); if (compressed.length === 0) { ... } const inputFingerprint = computeInputFingerprint(compressed); - const existing = await kv.get<SessionSummary>( - KV.summaries, - sessionId, - ); + const existing = existingBeforeRun; if (existing && existing.inputFingerprint === inputFingerprint) {summary.inputFingerprint = inputFingerprint; - await kv.set(KV.summaries, sessionId, summary); - await safeAudit(kv, "compress", "mem::summarize", [sessionId], { - title: summary.title, - observationCount: compressed.length, - }); + await Promise.all([ + kv.set(KV.summaries, sessionId, summary), + safeAudit(kv, "compress", "mem::summarize", [sessionId], { + title: summary.title, + observationCount: compressed.length, + }), + ]);As per coding guidelines: "Use parallel operations where possible with
Promise.allfor independent kv operations/reads."Also applies to: 433-438
🤖 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/functions/summarize.ts` around lines 306 - 328, Update the summarize flow around the observations read and existing summary lookup to issue the independent kv.list and kv.get operations together with Promise.all, while preserving the existing no-observations and fingerprint logic. Near the summary persistence block, likewise run kv.set and safeAudit concurrently with Promise.all, ensuring both operations still complete and retain their current arguments and behavior.Source: Coding guidelines
🤖 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/functions/summarize.ts`:
- Around line 68-91: The timeout path must prevent a still-running runSummarize
from persisting stale output after the session lock is released. Add a
supersession/timeout check immediately before the KV summary write and safeAudit
calls, using the existing session/run state, and skip persistence when the
timeout has won; ensure the check also handles providers that resolve after
timeout while preserving normal successful persistence.
---
Nitpick comments:
In `@src/functions/summarize.ts`:
- Around line 306-328: Update the summarize flow around the observations read
and existing summary lookup to issue the independent kv.list and kv.get
operations together with Promise.all, while preserving the existing
no-observations and fingerprint logic. Near the summary persistence block,
likewise run kv.set and safeAudit concurrently with Promise.all, ensuring both
operations still complete and retain their current arguments and behavior.
In `@test/summarize.test.ts`:
- Around line 548-573: Extend the timeout coverage around the handler setup and
provider summarize flow to use a provider promise that resolves after
SUMMARIZE_LOCK_TIMEOUT_MS, then complete a newer follow-up summary before
resolving the timed-out run. Assert that the late result does not overwrite the
newer summary, while preserving the existing timeout and lock-release
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c802e413-58f5-4fb1-8c2d-cdcabc24b2ab
📒 Files selected for processing (5)
plugin/scripts/stop.mjssrc/functions/summarize.tssrc/hooks/stop.tssrc/types.tstest/summarize.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/stop.ts
- plugin/scripts/stop.mjs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/summarize.test.ts (1)
548-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the "permanently hung" case; consider also covering "eventually resolves after timeout".
This test only proves the lock frees when the provider never settles. It doesn't exercise a provider that resolves shortly after the lock timeout fires — which is the scenario that can produce a stale overwrite of a newer summary (see the critical-issue comment on
src/functions/summarize.tsLines 68-91,473-475). Once that's fixed, a regression test resolving the hung provider after the timeout and asserting the later summary isn't clobbered would be valuable.🤖 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 `@test/summarize.test.ts` around lines 548 - 573, Extend the timeout coverage around the handler setup and provider summarize flow to use a provider promise that resolves after SUMMARIZE_LOCK_TIMEOUT_MS, then complete a newer follow-up summary before resolving the timed-out run. Assert that the late result does not overwrite the newer summary, while preserving the existing timeout and lock-release assertions.src/functions/summarize.ts (1)
306-328: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndependent kv operations can run in parallel.
kv.list(KV.observations(sessionId))(Line 306-308) and the laterkv.get(KV.summaries, sessionId)(Line 325-328) don't depend on each other and could be issued together viaPromise.all. Likewisekv.set(KV.summaries, ...)(Line 434) andsafeAudit(...)(Line 435-438) write to independent keys and don't need to be sequential.♻️ Proposed parallelization
- const observations = await kv.list<CompressedObservation>( - KV.observations(sessionId), - ); + const [observations, existingBeforeRun] = await Promise.all([ + kv.list<CompressedObservation>(KV.observations(sessionId)), + kv.get<SessionSummary>(KV.summaries, sessionId), + ]); const compressed = observations.filter((o) => o.title); if (compressed.length === 0) { ... } const inputFingerprint = computeInputFingerprint(compressed); - const existing = await kv.get<SessionSummary>( - KV.summaries, - sessionId, - ); + const existing = existingBeforeRun; if (existing && existing.inputFingerprint === inputFingerprint) {summary.inputFingerprint = inputFingerprint; - await kv.set(KV.summaries, sessionId, summary); - await safeAudit(kv, "compress", "mem::summarize", [sessionId], { - title: summary.title, - observationCount: compressed.length, - }); + await Promise.all([ + kv.set(KV.summaries, sessionId, summary), + safeAudit(kv, "compress", "mem::summarize", [sessionId], { + title: summary.title, + observationCount: compressed.length, + }), + ]);As per coding guidelines: "Use parallel operations where possible with
Promise.allfor independent kv operations/reads."Also applies to: 433-438
🤖 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/functions/summarize.ts` around lines 306 - 328, Update the summarize flow around the observations read and existing summary lookup to issue the independent kv.list and kv.get operations together with Promise.all, while preserving the existing no-observations and fingerprint logic. Near the summary persistence block, likewise run kv.set and safeAudit concurrently with Promise.all, ensuring both operations still complete and retain their current arguments and behavior.Source: Coding guidelines
🤖 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/functions/summarize.ts`:
- Around line 68-91: The timeout path must prevent a still-running runSummarize
from persisting stale output after the session lock is released. Add a
supersession/timeout check immediately before the KV summary write and safeAudit
calls, using the existing session/run state, and skip persistence when the
timeout has won; ensure the check also handles providers that resolve after
timeout while preserving normal successful persistence.
---
Nitpick comments:
In `@src/functions/summarize.ts`:
- Around line 306-328: Update the summarize flow around the observations read
and existing summary lookup to issue the independent kv.list and kv.get
operations together with Promise.all, while preserving the existing
no-observations and fingerprint logic. Near the summary persistence block,
likewise run kv.set and safeAudit concurrently with Promise.all, ensuring both
operations still complete and retain their current arguments and behavior.
In `@test/summarize.test.ts`:
- Around line 548-573: Extend the timeout coverage around the handler setup and
provider summarize flow to use a provider promise that resolves after
SUMMARIZE_LOCK_TIMEOUT_MS, then complete a newer follow-up summary before
resolving the timed-out run. Assert that the late result does not overwrite the
newer summary, while preserving the existing timeout and lock-release
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c802e413-58f5-4fb1-8c2d-cdcabc24b2ab
📒 Files selected for processing (5)
plugin/scripts/stop.mjssrc/functions/summarize.tssrc/hooks/stop.tssrc/types.tstest/summarize.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/stop.ts
- plugin/scripts/stop.mjs
🛑 Comments failed to post (1)
src/functions/summarize.ts (1)
68-91: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -B3 -A15 'withKeyedLock' src -g'*.ts' rg -n 'Promise\.race' src -g'*.ts'Repository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files first ast-grep outline src/functions/summarize.ts --view expanded printf '\n--- keyed-mutex ---\n' ast-grep outline src/state/keyed-mutex.ts --view expanded printf '\n--- summarize.ts key slices ---\n' sed -n '1,220p' src/functions/summarize.ts | cat -n printf '\n--- keyed-mutex.ts ---\n' sed -n '1,220p' src/state/keyed-mutex.ts | cat -n printf '\n--- summarize tests around timeout ---\n' sed -n '520,620p' src/functions/summarize.ts | cat -nRepository: rohitg00/agentmemory
Length of output: 10917
🏁 Script executed:
#!/bin/bash set -euo pipefail # Focus on summarize persistence and any freshness checks rg -n -A6 -B6 'kv\.set\(KV\.summaries|inputFingerprint|summarize_timeout|withKeyedLock|raceLockTimeout|runSummarize' src/functions/summarize.tsRepository: rohitg00/agentmemory
Length of output: 158
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the exact file size before reading more if needed wc -l src/functions/summarize.ts src/state/keyed-mutex.tsRepository: rohitg00/agentmemory
Length of output: 234
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the parts that define the timeout/lock behavior sed -n '1,220p' src/functions/summarize.ts | nl -ba | sed -n '1,220p' printf '\n---\n' sed -n '1,220p' src/state/keyed-mutex.ts | nl -ba | sed -n '1,220p'Repository: rohitg00/agentmemory
Length of output: 198
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for summary writes and freshness checks in the repository rg -n -A4 -B4 'KV\.summaries|inputFingerprint|summarize_timeout|summarize|SessionSummary' src/functions src/state src/types -g'*.ts'Repository: rohitg00/agentmemory
Length of output: 17669
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show summarize.ts around the timeout and persistence paths only nl -ba src/functions/summarize.ts | sed -n '1,240p'Repository: rohitg00/agentmemory
Length of output: 198
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show keyed mutex implementation nl -ba src/state/keyed-mutex.ts | sed -n '1,220p'Repository: rohitg00/agentmemory
Length of output: 198
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the summarize implementation around the write path. sed -n '220,478p' src/functions/summarize.ts | cat -n printf '\n--- tests around summarize timeout ---\n' rg -n -A20 -B10 'summarize_timeout|lock timeout|raceLockTimeout|mem::summarize|runSummarize' src -g'*.ts'Repository: rohitg00/agentmemory
Length of output: 19396
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the summarize implementation around the write path using cat -n. sed -n '220,478p' src/functions/summarize.ts | cat -n printf '\n--- nearby tests/mentions ---\n' rg -n -A8 -B8 'summarize_timeout|raceLockTimeout|runSummarize|mem::summarize' src -g'*.ts'Repository: rohitg00/agentmemory
Length of output: 16092
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'kv\.set|kv\.delete|recordAudit|safeAudit|metricsStore|KV\.summaries|inputFingerprint|createdAt|updatedAt|sessionId' src/functions/summarize.tsRepository: rohitg00/agentmemory
Length of output: 215
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -A20 -B20 'registerSummarizeFunction|mem::summarize|summarize\(' src -g'*.ts'Repository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the bottom half of summarize.ts directly. sed -n '280,478p' src/functions/summarize.ts | cat -nRepository: rohitg00/agentmemory
Length of output: 8703
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '280,478p' src/functions/summarize.ts | cat -nRepository: rohitg00/agentmemory
Length of output: 8703
🏁 Script executed:
#!/bin/bash set -euo pipefail grep -nE 'summaries|inputFingerprint|safeAudit|metricsStore|kv\.set|kv\.get|recordAudit|SessionSummary' src/functions/summarize.tsRepository: rohitg00/agentmemory
Length of output: 212
Prevent timed-out summarize runs from overwriting newer summaries
Promise.raceonly ends the caller; it doesn’t cancelrunSummarize(). Once the timeout branch wins, the lock is released and the old run can still reachkv.set(KV.summaries, sessionId, summary)/safeAudit, overwriting a newer result. Add a supersession check right before persist (or abort the provider call on timeout) and cover the case where the provider resolves after the timeout.🤖 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/functions/summarize.ts` around lines 68 - 91, The timeout path must prevent a still-running runSummarize from persisting stale output after the session lock is released. Add a supersession/timeout check immediately before the KV summary write and safeAudit calls, using the existing session/run state, and skip persistence when the timeout has won; ensure the check also handles providers that resolve after timeout while preserving normal successful persistence.
… exit timer - Replace the observationCount-based skip with an input fingerprint (sha256 over ordered observation ids + timestamps). Count alone is not safe: evict/auto-forget can delete observations, so the set can change while the count returns to a previous value. Old summaries have no fingerprint and simply re-summarize once. - Bound how long one run may hold the per-session lock (SUMMARIZE_LOCK_TIMEOUT_MS, default 5 min): anthropic/agent-sdk providers have no request timeout, and a hung call must not block every later summarize for that session. - Align the stop hook forced-exit timer with other single-request hooks (1500ms -> 500ms) now that it only sends one request.
…hanged The session-stopped handler sent the session's full compressed observation set to mem::graph-extract on every stop. Sessions that stop repeatedly without new activity (heartbeat-style loops) re-extracted the identical set each time — for a 500-observation session that is a ~50K-token LLM call per stop. Fingerprint the set (same helper as the summarize skip) and store it per session; trigger extraction only when it changed.
b531494 to
b6a002b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/triggers/events.ts (1)
66-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize independent KV reads using
Promise.all.As per coding guidelines, use parallel operations where possible with
Promise.allfor independent kv writes/reads. You can fetch the previous fingerprint state concurrently with the observations list to save a network round trip.⚡ Proposed refactor
- const observations = await kv.list<CompressedObservation>( - KV.observations(data.sessionId), - ); + const [observations, prev] = await Promise.all([ + kv.list<CompressedObservation>(KV.observations(data.sessionId)), + kv.get<{ fingerprint: string }>(KV.graphExtractState, data.sessionId).catch(() => null), + ]); const compressed = observations.filter((o) => o.title); if (compressed.length > 0) { // Sessions that stop repeatedly without new observations (e.g. // heartbeat loops) would re-extract the identical set every time; // skip when the fingerprint of the set is unchanged. const fingerprint = computeInputFingerprint(compressed); - const prev = await kv - .get<{ fingerprint: string }>( - KV.graphExtractState, - data.sessionId, - ) - .catch(() => null);🤖 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/events.ts` around lines 66 - 80, Update the event handling flow around KV.observations and KV.graphExtractState to start both independent reads concurrently with Promise.all, then derive compressed observations and the previous fingerprint state from the results while preserving the existing filtering and fallback behavior.Source: Coding guidelines
🤖 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 `@test/session-end-triggers-graph.test.ts`:
- Around line 94-118: Replace the source-text regex tests in the
“event::session::stopped graph-extract dedup” suite with runtime tests that
invoke the handler registered by registerEventTriggers using local mock sdk and
kv objects. Mock sdk.trigger, kv.get, kv.set, and kv.list, then assert that the
compressed fingerprint is persisted, unchanged fingerprints skip extraction, and
mem::graph-extract triggers only for new fingerprints.
---
Nitpick comments:
In `@src/triggers/events.ts`:
- Around line 66-80: Update the event handling flow around KV.observations and
KV.graphExtractState to start both independent reads concurrently with
Promise.all, then derive compressed observations and the previous fingerprint
state from the results while preserving the existing filtering and fallback
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1db93c90-2e2b-47cf-a727-747ab884f1dc
📒 Files selected for processing (9)
plugin/scripts/stop.mjssrc/functions/input-fingerprint.tssrc/functions/summarize.tssrc/hooks/stop.tssrc/state/schema.tssrc/triggers/events.tssrc/types.tstest/session-end-triggers-graph.test.tstest/summarize.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/types.ts
- src/hooks/stop.ts
- src/functions/summarize.ts
- test/summarize.test.ts
- plugin/scripts/stop.mjs
|
|
||
| // Sessions that stop repeatedly without new observations (e.g. heartbeat | ||
| // loops) used to re-extract the identical observation set on every stop. | ||
| // The handler now fingerprints the set and skips when unchanged. | ||
| describe("event::session::stopped graph-extract dedup", () => { | ||
| const events = readFileSync("src/triggers/events.ts", "utf-8"); | ||
|
|
||
| it("fingerprints the compressed set before triggering mem::graph-extract", () => { | ||
| expect(events).toMatch( | ||
| /const fingerprint = computeInputFingerprint\(compressed\);[\s\S]*?function_id:\s*"mem::graph-extract"/, | ||
| ); | ||
| }); | ||
|
|
||
| it("skips extraction when the stored fingerprint matches", () => { | ||
| expect(events).toMatch( | ||
| /prev\.fingerprint === fingerprint[\s\S]*?Graph extraction skipped/, | ||
| ); | ||
| }); | ||
|
|
||
| it("persists the fingerprint under KV.graphExtractState before triggering", () => { | ||
| expect(events).toMatch( | ||
| /kv\.set\(KV\.graphExtractState,\s*data\.sessionId,\s*\{\s*fingerprint,[\s\S]*?\}\);[\s\S]*?function_id:\s*"mem::graph-extract"/, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Refactor test to execute runtime behavior using mocks.
As per coding guidelines, test files must use the mock pattern with mock implementations of sdk.trigger, kv.get, kv.set, and kv.list. Testing logic by reading the source file as a string and matching regular expressions against it is highly brittle and fails to verify actual runtime execution.
Based on learnings, since the registerEventTriggers function accepts sdk and kv directly as arguments, you can pass local mock objects (e.g., mockSdk()/mockKV()) to invoke the target handler and assert on the executed state changes and triggers instead of attempting to mock the module.
🤖 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 `@test/session-end-triggers-graph.test.ts` around lines 94 - 118, Replace the
source-text regex tests in the “event::session::stopped graph-extract dedup”
suite with runtime tests that invoke the handler registered by
registerEventTriggers using local mock sdk and kv objects. Mock sdk.trigger,
kv.get, kv.set, and kv.list, then assert that the compressed fingerprint is
persisted, unchanged fingerprints skip extraction, and mem::graph-extract
triggers only for new fingerprints.
Sources: Coding guidelines, Learnings
yulin0629
changed the title
…input re-processing on session stop
`kv.list(KV.graphNodes)` stopped completing once mem:graph:nodes.bin grew past 100 MiB (26K nodes, 3.5M sourceObservationIds). The failure was swallowed by `.catch(() => [])`, so graph clustering silently returned no clusters and reflect fell back to lexical Jaccard clustering, which built a cluster matching 6497 facts and a ~400k-token prompt. Every run then exceeded the 180s invocation timeout, disabling insight generation. Reflect now reads the rohitg00#814 top-degree snapshot via a single kv.get, and cluster inputs are capped at 50 items per category so the fallback path can no longer produce an unbounded prompt or unbounded sourceMemoryIds. The remaining kv.list calls log a warning instead of degrading silently. Verified in production: 42s, usedFallback=false, 5 new insights, where every prior run timed out at 180s. AI-Agent: Claude Code AI-Session-IDs: 8affe40e-9240-4076-96a8-c71d01a59bdc
…d heap memPercent divided heapUsed by heapTotal, but heapTotal is the heap V8 has committed so far, which it deliberately keeps near-full and grows on demand. A healthy process therefore reports ~100%, and since the RSS floor (512MB) is trivially cleared by any real workload, the server pinned itself at status=critical and /agentmemory/health returned 503 indefinitely. Observed on the Oracle host: 588MB used of a 603MB committed heap = 97%, while --max-old-space-size=6144 puts real utilisation under 10%. That makes health useless as a liveness signal — a watchdog keyed on it would restart a perfectly healthy server forever. The snapshot now carries heapLimit from v8.getHeapStatistics(), and the ratio uses it when present. heapLimit is optional so snapshots persisted before this change still evaluate against heapTotal as they did before. The RSS floor added for issue rohitg00#158 was a band-aid over this same defect; it is left in place as a second guard. AI-Agent: Claude Code AI-Session-IDs: 8affe40e-9240-4076-96a8-c71d01a59bdc
AI-Agent: Claude Code AI-Session-IDs: 8affe40e-9240-4076-96a8-c71d01a59bdc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/functions/reflect.ts (1)
191-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation-detail comments.
Lines 191-197 restate the graph-loading implementation. Use clear names and keep only comments that explain non-obvious constraints.
As per coding guidelines, "In TypeScript source code, avoid code comments explaining WHAT — use clear naming instead."
🤖 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/functions/reflect.ts` around lines 191 - 197, Remove the implementation-detail comment above the graph-node loading logic in reflect, leaving the code behavior unchanged. Use the existing symbol names and structure to convey the data source and fallback behavior; retain comments only if they document a non-obvious constraint.Source: Coding guidelines
src/health/thresholds.ts (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the explanatory comment.
heapCeiling,heapLimit, andheapTotalalready identify the values. Keep only a short rationale, or remove the comment block.Proposed change
- // heapTotal is the heap V8 has *committed*, which it keeps near-full by design - // and grows on demand — measuring against it reports ~100% on a perfectly - // healthy process. Measure against the real ceiling (--max-old-space-size) - // when the snapshot carries it; older persisted snapshots do not. + // Use the V8 ceiling to avoid false high-memory alerts.As per coding guidelines,
src/**/*.tsmust avoid code comments explaining WHAT and use clear naming instead.🤖 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/health/thresholds.ts` around lines 62 - 65, Shorten or remove the explanatory comment above the heap ceiling calculation, keeping at most a brief rationale for preferring the real V8 heap limit over heapTotal. Leave the existing heapCeiling, heapLimit, and heapTotal logic unchanged.Source: Coding guidelines
test/health-thresholds.test.ts (1)
83-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for snapshots without
heapLimit.Both tests set
heapLimit, so they do not exercise the fallback insrc/health/thresholds.tsLines 66-69. Add a case withheapLimitomitted and usage above the configured threshold. This protects legacy persisted snapshots.Proposed test
+ it("falls back to heapTotal when heapLimit is absent", () => { + const s = snap({ + memory: { + heapUsed: 95, + heapTotal: 100, + rss: 1, + external: 0, + }, + }); + const result = evaluateHealth(s, { + memoryWarnPercent: 80, + memoryCriticalPercent: 90, + memoryRssFloorBytes: 0, + }); + expect(result.status).toBe("critical"); + expect(result.alerts.some((a) => a.startsWith("memory_critical_"))).toBe( + true, + ); + });🤖 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 `@test/health-thresholds.test.ts` around lines 83 - 113, Add a test in the health threshold suite that omits memory.heapLimit from the snapshot and sets heap usage above the configured fallback threshold, then assert evaluateHealth returns the expected critical status and memory alert. Keep the existing heapLimit-based tests unchanged and ensure the new case covers legacy persisted snapshots.
🤖 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/functions/reflect.ts`:
- Around line 198-205: Update the reflection data-loading flow around
Promise.all to derive graphEdges from snapshot?.topEdges ?? [] instead of
calling listOrEmpty on KV.graphEdges, while preserving the existing snapshot
node and memory loading behavior. Extend the relevant reflect test to verify
that mem:graph:edges is not enumerated.
---
Nitpick comments:
In `@src/functions/reflect.ts`:
- Around line 191-197: Remove the implementation-detail comment above the
graph-node loading logic in reflect, leaving the code behavior unchanged. Use
the existing symbol names and structure to convey the data source and fallback
behavior; retain comments only if they document a non-obvious constraint.
In `@src/health/thresholds.ts`:
- Around line 62-65: Shorten or remove the explanatory comment above the heap
ceiling calculation, keeping at most a brief rationale for preferring the real
V8 heap limit over heapTotal. Leave the existing heapCeiling, heapLimit, and
heapTotal logic unchanged.
In `@test/health-thresholds.test.ts`:
- Around line 83-113: Add a test in the health threshold suite that omits
memory.heapLimit from the snapshot and sets heap usage above the configured
fallback threshold, then assert evaluateHealth returns the expected critical
status and memory alert. Keep the existing heapLimit-based tests unchanged and
ensure the new case covers legacy persisted snapshots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c1d9435-6cdb-4fa3-974f-7d4636edf8cb
📒 Files selected for processing (8)
CHANGELOG.mdsrc/functions/graph.tssrc/functions/reflect.tssrc/health/monitor.tssrc/health/thresholds.tssrc/types.tstest/health-thresholds.test.tstest/reflect.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
| const [snapshot, graphEdges, semanticMemories, lessons, crystals] = | ||
| await Promise.all([ | ||
| kv.list<GraphNode>(KV.graphNodes).catch(() => []), | ||
| kv.list<GraphEdge>(KV.graphEdges).catch(() => []), | ||
| kv.list<SemanticMemory>(KV.semantic).catch(() => []), | ||
| kv.list<Lesson>(KV.lessons).catch(() => []), | ||
| kv.list<Crystal>(KV.crystals).catch(() => []), | ||
| readSnapshot(kv), | ||
| listOrEmpty<GraphEdge>(KV.graphEdges), | ||
| listOrEmpty<SemanticMemory>(KV.semantic), | ||
| listOrEmpty<Lesson>(KV.lessons), | ||
| listOrEmpty<Crystal>(KV.crystals), | ||
| ]); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Use snapshot edges for reflection.
Line 201 still enumerates every graph edge. A large KV.graphEdges scope can consume the invocation budget before listOrEmpty catches a failure. Reflection then has snapshot nodes but no edges, so it unnecessarily uses lexical clustering.
Use snapshot?.topEdges ?? []. GraphSnapshot.topEdges is the bounded edge set that corresponds to the snapshot node set. Extend test/reflect.test.ts to assert that mem:graph:edges is not enumerated.
Proposed fix
- const [snapshot, graphEdges, semanticMemories, lessons, crystals] =
+ const [snapshot, semanticMemories, lessons, crystals] =
await Promise.all([
readSnapshot(kv),
- listOrEmpty<GraphEdge>(KV.graphEdges),
listOrEmpty<SemanticMemory>(KV.semantic),
listOrEmpty<Lesson>(KV.lessons),
listOrEmpty<Crystal>(KV.crystals),
]);
+ const graphEdges = snapshot?.topEdges ?? [];📝 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.
| const [snapshot, graphEdges, semanticMemories, lessons, crystals] = | |
| await Promise.all([ | |
| kv.list<GraphNode>(KV.graphNodes).catch(() => []), | |
| kv.list<GraphEdge>(KV.graphEdges).catch(() => []), | |
| kv.list<SemanticMemory>(KV.semantic).catch(() => []), | |
| kv.list<Lesson>(KV.lessons).catch(() => []), | |
| kv.list<Crystal>(KV.crystals).catch(() => []), | |
| readSnapshot(kv), | |
| listOrEmpty<GraphEdge>(KV.graphEdges), | |
| listOrEmpty<SemanticMemory>(KV.semantic), | |
| listOrEmpty<Lesson>(KV.lessons), | |
| listOrEmpty<Crystal>(KV.crystals), | |
| ]); | |
| const [snapshot, semanticMemories, lessons, crystals] = | |
| await Promise.all([ | |
| readSnapshot(kv), | |
| listOrEmpty<SemanticMemory>(KV.semantic), | |
| listOrEmpty<Lesson>(KV.lessons), | |
| listOrEmpty<Crystal>(KV.crystals), | |
| ]); | |
| const graphEdges = snapshot?.topEdges ?? []; |
🤖 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/functions/reflect.ts` around lines 198 - 205, Update the reflection
data-loading flow around Promise.all to derive graphEdges from
snapshot?.topEdges ?? [] instead of calling listOrEmpty on KV.graphEdges, while
preserving the existing snapshot node and memory loading behavior. Extend the
relevant reflect test to verify that mem:graph:edges is not enumerated.
|
Rebased this onto current Three conflicts, all mechanical:
Verified the summarize guard live against a self-hosted 0.9.29 (Anthropic provider, session pinned at
That second case is the one a pure time-window dedup (#572) can't close, which matches your reasoning in the description — worth keeping the lock rather than folding this into a freshness window. One behavioural note worth a changelog line: summaries written before |
Fixes #1062. Fixes #1063.
Problem
Every session stop currently runs
mem::summarizetwice, and each run re-summarizes the whole session from scratch.The stop hook fires two requests:
POST /agentmemory/summarize→api::summarize→mem::summarizePOST /agentmemory/session/end→api::session::end→ fans outevent::session::stopped, whose handler callsmem::summarizeagain (src/triggers/events.ts)mem::summarizehas no lock, no in-flight guard, and no unchanged-input short-circuit (unlikemem::observe, which already usesDedupMap+withKeyedLock), so both runs do the full LLM work.Observed in production (self-hosted, v0.9.27): every session stop produced two byte-identical summarize requests at the provider (same second, same body size), growing each cycle as the session accumulated observations — e.g. a heartbeat-style session that stopped every 30 minutes reached duplicate ~230 KB (~110K input tokens) requests per cycle:
Fix
/agentmemory/summarizecall —/agentmemory/session/endalready fans out to the summarize path. One stop, one summarize.mem::summarize: serialize per-session runs withwithKeyedLock(same pattern asmem::observe), and short-circuit when the stored summary'sobservationCountalready matches the current compressed count. Observations are append-only, so an unchanged count means the LLM input would be byte-identical to the previous run — re-running cannot produce new information. This also stops full re-summarization of long-lived sessions that stop repeatedly without new activity.Relation to #572
#572 adds a time-window dedup (
SUMMARIZE_DEDUP_WINDOW_MS, default 90 s). That helps, but:The two changes are complementary: count-based skip covers "nothing new" regardless of elapsed time, and a time window could still be layered on top for sessions that keep accumulating low-value observations.
Also: graph extraction has the same shape (#1063)
The session-stopped handler sent the session's full compressed observation set to
mem::graph-extracton every stop with no change detection — for a 500-observation heartbeat session that was a ~50K-token LLM call every 30 minutes with identical input. The last commit reuses the same fingerprint helper: store the set's fingerprint per session (KV.graphExtractState) and trigger extraction only when it changed.Tests
Three new cases in
test/summarize.test.ts:skipped: true, no second LLM callnpm test: no new failures (the 4 pre-existing failures inauto-compress.test.ts/embedding-provider.test.tsreproduce on a clean checkout and come from environment leakage, unrelated to this change).Summary by CodeRabbit