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 3 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.
yulin0629
force-pushed
the
fix/summarize-double-trigger
branch
from
481611b to
d5741ae
Compare
📝 WalkthroughWalkthroughShutdown flows now rely on session-end handling for summarization. The summarize handler serializes work per session, skips identical observation sets using fingerprints, handles lock timeouts, and reruns when observations change. Session-stop graph extraction also skips unchanged observation sets. ChangesSession-stop deduplication
Estimated code review effort: 3 (Moderate) | ~30 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: Return existing summary
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 PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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.
yulin0629
force-pushed
the
fix/summarize-double-trigger
branch
from
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
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