fix: bound the audit log with AGENTMEMORY_AUDIT_MAX - #1176
Conversation
recordAudit has no retention: the audit scope grows for the life of the install. Because the KV adapter rewrites a whole scope on every set, each audit write costs more as the log gets longer, so the slowdown compounds on exactly the installs that have been running longest. A two-month-old install here held 14k rows in a 4.5MB blob that was rewritten on every audited operation. Keep the most recent AGENTMEMORY_AUDIT_MAX rows (default 5000; 0 restores the previous unbounded behaviour). The check runs every 100 writes so the common path stays a single set, which means the log is bounded at max + one interval rather than exactly max. A failed trim is logged and swallowed — the audited operation must not fail because pruning did. 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. |
📝 WalkthroughWalkthroughThe audit function now supports configurable retention. It defaults to 5,000 entries, prunes every 100 successful writes, removes oldest entries, supports unlimited retention with ChangesAudit retention
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant recordAudit
participant KVStore
participant Logger
recordAudit->>KVStore: Write audit entry
recordAudit->>KVStore: Prune oldest excess entries every 100 writes
KVStore-->>recordAudit: Return pruning result or failure
recordAudit->>Logger: Warn when pruning fails
recordAudit-->>recordAudit: Return successful audit entry
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/functions/audit.ts (1)
35-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation-explaining comments.
The new comments restate retention behavior and error handling. Use the existing names and control flow to express this behavior.
As per coding guidelines, “Do not add comments that explain what code does; use clear naming instead.”
Also applies to: 91-91
🤖 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/audit.ts` around lines 35 - 40, Remove the implementation-explaining comments near the audit retention logic, including the comment at the referenced secondary location. Preserve the existing code, names, and control flow without replacing the comments with new explanatory text.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/audit.ts`:
- Around line 63-66: Update the stale-entry cleanup loop in the audit trimming
flow to count only successful kv.delete calls, while tracking failures instead
of suppressing them. Warn through logger when any deletion fails, and report the
successful removal count in the existing audit log. Add a test covering a
rejected kv.delete and verifying the warning and accurate removal count.
- Around line 46-49: Update auditMax() to validate the entire
AGENTMEMORY_AUDIT_MAX value as a non-negative integer rather than relying on
parseInt(), rejecting partially numeric inputs such as hexadecimal or exponent
notation and returning DEFAULT_AUDIT_MAX for invalid values. Add tests covering
these partial numeric cases while preserving valid integer handling.
---
Nitpick comments:
In `@src/functions/audit.ts`:
- Around line 35-40: Remove the implementation-explaining comments near the
audit retention logic, including the comment at the referenced secondary
location. Preserve the existing code, names, and control flow without replacing
the comments with new explanatory text.
🪄 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: c1f35f18-d635-4b8b-a9ff-7b6bde6a5b88
📒 Files selected for processing (2)
src/functions/audit.tstest/audit-retention.test.ts
| function auditMax(): number { | ||
| const raw = parseInt(getEnvVar("AGENTMEMORY_AUDIT_MAX") || "", 10); | ||
| if (Number.isFinite(raw) && raw >= 0) return raw; | ||
| return DEFAULT_AUDIT_MAX; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'AGENTMEMORY_AUDIT_MAX|parseInt|auditMax' \
src/functions/audit.ts test/audit-retention.test.tsRepository: rohitg00/agentmemory
Length of output: 4712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- audit implementation ---'
sed -n '1,125p' src/functions/audit.ts
printf '%s\n' '--- retention tests ---'
sed -n '1,140p' test/audit-retention.test.ts
printf '%s\n' '--- getEnvVar implementation and usages ---'
rg -n -C 5 'function getEnvVar|const getEnvVar|export .*getEnvVar|getEnvVar\(' src
printf '%s\n' '--- JavaScript parseInt behavior ---'
node - <<'JS'
for (const value of ["0x10", "5e3", "12abc", " 150 ", "+5", "-1", ""]) {
const parsed = parseInt(value || "", 10);
console.log(JSON.stringify(value), parsed, Number.isFinite(parsed) && parsed >= 0);
}
JSRepository: rohitg00/agentmemory
Length of output: 25122
Reject partially numeric retention settings.
parseInt() interprets "0x10" as 0 and "5e3" as 5. These values can disable retention or delete audit entries earlier than intended. Validate the complete value as a non-negative integer and default invalid values to DEFAULT_AUDIT_MAX. Add tests for partial numeric values.
🤖 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/audit.ts` around lines 46 - 49, Update auditMax() to validate
the entire AGENTMEMORY_AUDIT_MAX value as a non-negative integer rather than
relying on parseInt(), rejecting partially numeric inputs such as hexadecimal or
exponent notation and returning DEFAULT_AUDIT_MAX for invalid values. Add tests
covering these partial numeric cases while preserving valid integer handling.
| for (const old of stale) { | ||
| await kv.delete(KV.audit, old.id).catch(() => {}); | ||
| } | ||
| logger.info("audit log trimmed", { removed: stale.length, kept: max }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report incomplete trims correctly.
Line 64 suppresses each delete failure. Line 66 then reports every stale entry as removed. If deletes fail, retention remains over the bound and monitoring receives a false success result.
Count successful deletes. Log a warning when one or more deletes fail. Report the actual removal count. Add a test where kv.delete() rejects.
🤖 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/audit.ts` around lines 63 - 66, Update the stale-entry cleanup
loop in the audit trimming flow to count only successful kv.delete calls, while
tracking failures instead of suppressing them. Warn through logger when any
deletion fails, and report the successful removal count in the existing audit
log. Add a test covering a rejected kv.delete and verifying the warning and
accurate removal count.
1 participant
Problem
recordAudithas no retention. Themem:auditscope grows for the life of the install, and because the KV adapter rewrites a whole scope on everyset, each audit write gets more expensive as the log gets longer — the slowdown compounds on exactly the installs that have been running longest.On a two-month-old install here that scope held ~14k rows in a 4.5MB blob, rewritten on every audited operation (every observe, compress, remember, slot write, eviction…).
Change
Keep the most recent
AGENTMEMORY_AUDIT_MAXrows, oldest dropped first.50000restores today's unbounded behaviour for anyone who wants the full logset; the log is therefore bounded atmax + one intervalrather than exactlymax, which the test asserts explicitly rather than pretending otherwiseVerification
test/audit-retention.test.ts(new, 4 cases): bounded near the max with the newest rows surviving and the oldest gone; no trimming under the bound;0stays unbounded;recordAuditstill returns its entry when the trim throws.Full suite green (1600 passed, 1 skipped).
Run the suite with an isolated
HOME— on a machine with a real~/.agentmemory/.env, config merging pulls the developer's keys and flags into 14 unrelated tests.Summary by CodeRabbit