fix: evict lowest-value observations at the session cap instead of refusing writes - #1174
Open
sbynode-ux wants to merge 1 commit into
Open
fix: evict lowest-value observations at the session cap instead of refusing writes#1174sbynode-ux wants to merge 1 commit into
sbynode-ux wants to merge 1 commit into
Conversation
…fusing writes MAX_OBS_PER_SESSION currently makes mem::observe return "Session observation limit reached" once a session holds its cap, so every later observation in that session is dropped. The observations lost are the NEWEST ones in the LONGEST sessions, which is the part a memory system is least able to afford losing, and nothing is logged server-side, so a capped session looks identical to a quiet one. Evict instead: at the cap, drop the least valuable rows (lowest importance first, oldest breaking ties) to make room, clear them from the BM25 and vector indexes so search cannot return dead ids, and warn with the session id and the number evicted. The bound on how much a session stores is unchanged; only the choice of which observation is sacrificed changes. Three sessions in a real install had been silently pinned at exactly 500 rows for days before this was noticed. Signed-off-by: sbynode-ux <sby.node@gmail.com>
|
@sbynode-ux is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughObservation capture now evicts the least-important records when a session reaches its cap. Ties use record age. Successful evictions remove records from KV, BM25, and vector indexes. Missing importance scores use a default value, and individual deletion failures do not stop processing. ChangesSession observation-cap eviction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/functions/observe.ts (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the explanatory comment.
DEFAULT_EVICTION_IMPORTANCEalready describes the constant use.
As per coding guidelines, “Do not add comments that explain what code does; 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/observe.ts` around lines 19 - 20, Remove the explanatory comment immediately above DEFAULT_EVICTION_IMPORTANCE, leaving the constant declaration and its value unchanged.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/observe.ts`:
- Around line 157-166: Update the eviction loop in the observation write flow to
continue attempting additional victims until enough deletions succeed to make
room for the new observation. If the required eviction count cannot be achieved,
reject or skip the write before the persistence call around line 221, preserving
the maxObservationsPerSession cap; add a test covering failed deletions and
verifying no new observation is written.
- Around line 159-162: Update the automatic eviction flow around
getSearchIndex().remove and vectorIndexRemove to collect victim IDs and issue
one batched recordAudit() delete entry with details.evicted before deleting
records via kv.delete(). Preserve the existing eviction and index-removal
behavior while ensuring the audit is recorded before the bulk KV deletion.
In `@test/observe-session-cap.test.ts`:
- Around line 1-5: Update the module-level mocks in the test setup to mock
iii-sdk and provide TriggerAction.Void, ensuring mem::observe calls do not
execute the real trigger action. Preserve the existing mockSdk() and mockKV()
collaborator mocks.
---
Nitpick comments:
In `@src/functions/observe.ts`:
- Around line 19-20: Remove the explanatory comment immediately above
DEFAULT_EVICTION_IMPORTANCE, leaving the constant declaration and its value
unchanged.
🪄 Autofix
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: a9a1a1a9-40b5-4ef8-ac98-1ba000665856
📒 Files selected for processing (2)
src/functions/observe.tstest/observe-session-cap.test.ts
Comment on lines
+157
to
+166
| for (const victim of victims) { | ||
| try { | ||
| await kv.delete(KV.observations(payload.sessionId), victim.id); | ||
| evicted++; | ||
| getSearchIndex().remove(victim.id); | ||
| vectorIndexRemove(victim.id); | ||
| } catch { | ||
| // A failed delete just leaves the cap tight for this write; | ||
| // the next observation retries the eviction. | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not write when eviction did not make room.
A failed kv.delete() is swallowed, but Line 221 still writes the new observation. At the cap, this produces more than maxObservationsPerSession records. Continue through additional candidates until the required number of deletions succeeds. If that is not possible, reject the write or explicitly change the cap guarantee. Add a failed-delete test.
🤖 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/observe.ts` around lines 157 - 166, Update the eviction loop in
the observation write flow to continue attempting additional victims until
enough deletions succeed to make room for the new observation. If the required
eviction count cannot be achieved, reject or skip the write before the
persistence call around line 221, preserving the maxObservationsPerSession cap;
add a test covering failed deletions and verifying no new observation is
written.
Comment on lines
+159
to
+162
| await kv.delete(KV.observations(payload.sessionId), victim.id); | ||
| evicted++; | ||
| getSearchIndex().remove(victim.id); | ||
| vectorIndexRemove(victim.id); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\brecordAudit\s*\(' src testRepository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- observe imports and eviction path ---'
sed -n '1,35p' src/functions/observe.ts
sed -n '125,180p' src/functions/observe.ts
printf '%s\n' '--- audit implementation and guidance ---'
sed -n '1,90p' src/functions/audit.ts
printf '%s\n' '--- comparable eviction audit path ---'
sed -n '350,410p' src/functions/retention.ts
printf '%s\n' '--- observe references ---'
rg -n -C 3 'observe|retention-evict|recordAudit|kv\.delete' test src/functions/observe.ts src/functions/retention.tsRepository: rohitg00/agentmemory
Length of output: 46881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- audit entry type ---'
rg -n -A 28 'export interface AuditEntry|type AuditEntry' src/types.ts
printf '%s\n' '--- session-cap tests ---'
sed -n '1,175p' test/observe-session-cap.test.ts
printf '%s\n' '--- audit policy assertions ---'
rg -n -C 5 'audit coverage|details\.evicted|targetIds|BEFORE kv\.delete|bulk' src test/retention.test.tsRepository: rohitg00/agentmemory
Length of output: 24865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
observe = Path("src/functions/observe.ts").read_text()
audit = Path("src/functions/audit.ts").read_text()
delete_match = re.search(
r"for \(const victim of victims\) \{(?P<body>.*?)\n\s*\}\n\n\s*logger\.warn",
observe,
re.S,
)
if not delete_match:
raise SystemExit("Could not locate the session-cap eviction loop")
body = delete_match.group("body")
print("observe imports audit.ts:", bool(re.search(r'from ["\']\.\/audit\.js["\']', observe)))
print("recordAudit call in eviction loop:", bool(re.search(r"\brecordAudit\s*\(", body)))
print("kv.delete calls in eviction loop:", len(re.findall(r"\bkv\.delete\s*\(", body)))
print("evictedIds collection in observe:", "evictedIds" in observe)
print("batch details.evicted policy:", bool(re.search(r"details\.evicted", audit)))
print("policy requires audit before delete:", bool(re.search(r"recordAudit call\n.*?BEFORE kv\.delete", audit, re.S)))
PYRepository: rohitg00/agentmemory
Length of output: 384
Audit automatic observation evictions.
Add one batched delete audit for this bulk path with the victim IDs and details.evicted. Follow the audit policy and call recordAudit() before kv.delete().
🤖 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/observe.ts` around lines 159 - 162, Update the automatic
eviction flow around getSearchIndex().remove and vectorIndexRemove to collect
victim IDs and issue one batched recordAudit() delete entry with details.evicted
before deleting records via kv.delete(). Preserve the existing eviction and
index-removal behavior while ensuring the audit is recorded before the bulk KV
deletion.
Source: Coding guidelines
Comment on lines
+1
to
+5
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
|
|
||
| vi.mock("../src/logger.js", () => ({ | ||
| logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, | ||
| })); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/functions/observe.ts --items all
rg -n -C 2 'from "iii-sdk"|TriggerAction\.' src/functions/observe.ts
rg -n 'vi\.mock\(["'\'']iii-sdk' test/observe-session-cap.test.tsRepository: rohitg00/agentmemory
Length of output: 1906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test file ---'
cat -n test/observe-session-cap.test.ts
printf '%s\n' '--- function registration and trigger usage ---'
sed -n '1,380p' src/functions/observe.ts | cat -n | sed -n '1,380p'
printf '%s\n' '--- related test mock patterns ---'
rg -n -C 5 'vi\.mock\(["'\'']iii-sdk|TriggerAction\.Void|mockSdk|mockKV' test src package.json vitest.config.* 2>/dev/null || trueRepository: rohitg00/agentmemory
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- observe-session-cap.test.ts ---'
cat -n test/observe-session-cap.test.ts
printf '%s\n' '--- observe.ts imports, registration, and TriggerAction calls ---'
sed -n '1,60p' src/functions/observe.ts | cat -n
sed -n '180,225p' src/functions/observe.ts | cat -n
sed -n '245,280p' src/functions/observe.ts | cat -n
sed -n '315,350p' src/functions/observe.ts | cat -n
printf '%s\n' '--- iii-sdk mocks in the target test ---'
rg -n -C 3 'iii-sdk|TriggerAction|sdk\.trigger|registerObserveFunction|mockSdk|mockKV' test/observe-session-cap.test.tsRepository: rohitg00/agentmemory
Length of output: 16412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/functions/observe.ts").read_text()
test = Path("test/observe-session-cap.test.ts").read_text()
assert re.search(r'import\s*\{\s*TriggerAction\s*,\s*type\s+ISdk\s*\}\s*from\s*"iii-sdk"', source)
assert "action: TriggerAction.Void()" in source
assert re.search(r'function_id:\s*"stream::send"', source)
assert 'await import("../src/functions/observe.js")' in test
assert test.count('sdk.trigger("mem::observe"') == 1
assert test.count("observeOnce(sdk,") == 3
assert not re.search(r'vi\.mock\(\s*["\']iii-sdk["\']', test)
print("runtime iii-sdk import: present")
print("TriggerAction.Void call: present")
print("stream::send action: present")
print("dynamic observe module import: present")
print("mem::observe exercised by three tests")
print("module-level iii-sdk mock: absent")
PYRepository: rohitg00/agentmemory
Length of output: 363
Mock iii-sdk at module level.
observe.ts executes TriggerAction.Void() for every mem::observe call. Add vi.mock("iii-sdk") with TriggerAction.Void. Keep mockSdk() and mockKV() for handler collaborators.
🤖 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/observe-session-cap.test.ts` around lines 1 - 5, Update the module-level
mocks in the test setup to mock iii-sdk and provide TriggerAction.Void, ensuring
mem::observe calls do not execute the real trigger action. Preserve the existing
mockSdk() and mockKV() collaborator mocks.
Sources: Coding guidelines, Learnings
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
1 participant
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
Once a session reaches
MAX_OBS_PER_SESSION(default 500),mem::observereturnsSession observation limit reached (N)and every subsequent observation in that session is discarded. Two consequences:Change
At the cap, evict instead of refuse:
importance, oldesttimestampbreaking ties (uncompressed rows have no importance yet, so they are treated as 3)logger.warnwith the session id, the cap and the number evictedA failed delete is swallowed deliberately: the cap simply stays tight for that write and the next observation retries the eviction, which is strictly better than failing the capture.
Verification
test/observe-session-cap.test.ts(new, 3 cases):Full suite: 1599 passed, 1 skipped.
Unrelated finding while running the suite: on a machine that has a real
~/.agentmemory/.env, 14 tests fail (embedding-provider, fetch-timeout, auto-compress, context-slots, claude-bridge-path) because config loading merges that file, so the suite picks up the developer's own keys and flags. Everything passes with an isolatedHOME. Happy to open that as a separate issue if it is not already known.Summary by CodeRabbit
New Features
Bug Fixes