fix: wire SNAPSHOT_INTERVAL to a periodic timer that triggers mem::snapshot-create - #1010
Conversation
|
@yingliang-zhang is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughWhen snapshots are enabled, the startup log now references ChangesSnapshot scheduling
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 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.
🧹 Nitpick comments (1)
src/index.ts (1)
359-361: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBoot log doesn't reflect whether the periodic timer actually started.
This
bootLogalways fires whensnapshotConfig.enabled, even ifsnapshotIntervalMswas0and the guard at Line 351 skipped creating the timer. Operators would see "Git snapshots: ... (every 0s)" implying periodic snapshots are running, when in fact only manual/API-triggered snapshots work — the exact gap this PR is meant to close.📝 Proposed fix to clarify boot log
- bootLog( - `Git snapshots: ${snapshotConfig.dir} (every ${snapshotConfig.interval}s)`, - ); + bootLog( + snapshotIntervalMs > 0 + ? `Git snapshots: ${snapshotConfig.dir} (every ${snapshotConfig.interval}s)` + : `Git snapshots: ${snapshotConfig.dir} (periodic timer disabled, SNAPSHOT_INTERVAL=0 — manual/API trigger only)`, + );🤖 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/index.ts` around lines 359 - 361, The boot log in the snapshot startup path is misleading because it always reports periodic Git snapshots even when the timer was not created. Update the logic around the snapshot initialization in src/index.ts so the `bootLog` tied to the snapshot setup only runs when `snapshotIntervalMs` is actually greater than zero and the interval timer is started, and use the existing snapshot-related symbols like `snapshotConfig` and `snapshotIntervalMs` to keep the message aligned with real behavior.
🤖 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.
Nitpick comments:
In `@src/index.ts`:
- Around line 359-361: The boot log in the snapshot startup path is misleading
because it always reports periodic Git snapshots even when the timer was not
created. Update the logic around the snapshot initialization in src/index.ts so
the `bootLog` tied to the snapshot setup only runs when `snapshotIntervalMs` is
actually greater than zero and the interval timer is started, and use the
existing snapshot-related symbols like `snapshotConfig` and `snapshotIntervalMs`
to keep the message aligned with real behavior.
|
Addressed the CodeRabbit nitpick in commit |
…timer Address CodeRabbit nitpick: the boot log always reported 'every Ns' even when the timer guard (snapshotIntervalMs > 0) skipped creating it, making SNAPSHOT_INTERVAL=0 misleadingly look like periodic snapshots were running. Signed-off-by: yingliang-zhang <zhangyingliang@outlook.com>
4b11197 to
3f3d93f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
163-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new explanatory comments.
The comments at Lines 163-164 and 357-361 explain implementation behavior. Remove them and keep the code self-describing through clear identifiers and structure.
As per coding guidelines: “Do not add comments explaining what code does; use clear naming instead.”
Also applies to: 357-361
🤖 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/index.ts` around lines 163 - 164, Remove the explanatory comments at the indicated locations, including the block around the environment-loading logic near the relevant initialization code, while preserving all surrounding behavior. Keep the implementation self-describing through its existing identifiers and structure without adding replacement comments.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/index.ts`:
- Around line 373-375: Declare and initialize snapshotIntervalMs in the snapshot
startup flow before the timer guard and boot log, deriving it from the
configured snapshot interval in milliseconds. Reuse this variable in the
existing conditional log while preserving the enabled and manual-trigger-only
messages.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 163-164: Remove the explanatory comments at the indicated
locations, including the block around the environment-loading logic near the
relevant initialization code, while preserving all surrounding behavior. Keep
the implementation self-describing through its existing identifiers and
structure without adding replacement comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| snapshotIntervalMs > 0 | ||
| ? `Git snapshots: ${snapshotConfig.dir} (every ${snapshotConfig.interval}s)` | ||
| : `Git snapshots: ${snapshotConfig.dir} (periodic timer disabled, SNAPSHOT_INTERVAL=0 — manual trigger only)`, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)src/index\.ts$|package\.json$' || true
echo "== relevant section =="
if [ -f src/index.ts ]; then
wc -l src/index.ts
sed -n '330,390p' src/index.ts | nl -ba -v330
fi
echo "== definitions/usages =="
rg -n "snapshotIntervalMs|snapshotConfig|snapshot" src/index.ts package.json || true
echo "== package module type =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({type:p.type, main:p.main, dependencies:p.dependencies || {}, devDependencies:p.devDependencies || {}}, null, 2))"
fiRepository: rohitg00/agentmemory
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)src/index\.ts$|package\.json$' || true
echo "== package module type =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({type:p.type, main:p.main, dependencies:p.dependencies || {}, devDependencies:p.devDependencies || {}}, null, 2))"
fi
echo "== relevant src/index.ts section and identifiers =="
if [ -f src/index.ts ]; then
wc -l src/index.ts
sed -n '320,400p' src/index.ts | nl -ba -v320
echo "== identifier occurrences =="
rg -n "snapshotIntervalMs|snapshotConfig|GIT_SNAPSHOT|SNAPSHOT|interval" src/index.ts || true
fiRepository: rohitg00/agentmemory
Length of output: 945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package name/context =="
sed -n '1,25p' src/index.ts
echo "== snapshot identifier occurrences =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/index.ts')
print(f'src/index.ts lines={len(p.read_text().splitlines())}')
for i,line in enumerate(p.read_text().splitlines(),1):
if 'snapshotIntervalMs' in line or 'snapshotConfig' in line or 'interval' in line and ('Snapshot' in line or 'SNAPSHOT' in line or 'git' in line.lower()):
print(f'{i}: {line}')
PY
echo "== lines 330-390 =="
python3 - <<'PY'
from pathlib import Path
lines=Path('src/index.ts').read_text().splitlines()
for i in range(330, min(len(lines),390)+1):
print(f'{i}: {lines[i-1]}')
PYRepository: rohitg00/agentmemory
Length of output: 4037
Declare snapshotIntervalMs before the snapshot boot log.
snapshotIntervalMs is not defined in src/index.ts, so TypeScript rejects this file and execution raises ReferenceError when snapshots are enabled. Add the millisecond value before the timer guard and reuse it in the log.
🤖 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/index.ts` around lines 373 - 375, Declare and initialize
snapshotIntervalMs in the snapshot startup flow before the timer guard and boot
log, deriving it from the configured snapshot interval in milliseconds. Reuse
this variable in the existing conditional log while preserving the enabled and
manual-trigger-only messages.
1 participant
Summary
The config value (default 3600s) was read and logged at boot —
Git snapshots: ... (every 3600s)— but nosetIntervaltimer was ever created. Periodic snapshots never fired automatically. Snapshots only happened when manually triggered via the API/MCP endpoint or thememory_snapshot_createMCP tool.This means that in a long-running daemon, if no one manually triggers a snapshot, data could be lost on crash/restart with only the last manual snapshot available for recovery.
Fix
Add a
setIntervalthat triggersmem::snapshot-createat the configured interval, following the exact same pattern as the existingauto-forget,lesson-decay,consolidation, andrecent-searches-sweeptimers:A guard (
snapshotIntervalMs > 0) prevents an infinite loop if someone explicitly setsSNAPSHOT_INTERVAL=0.How to verify
SNAPSHOT_ENABLED=trueandSNAPSHOT_INTERVAL=10in.envagentmemoryagentmemory mcpand triggermemory_snapshot_listperiodic snapshotcommit appears with a timestamp ~10s after bootdaemon.err.logforSnapshot createdinfo entriesTest results
npm run build— cleannpx vitest run test/snapshot.test.ts— 5/5 passedembedding-provider.test.tsunrelated to this change, confirmed by running tests on unmodifiedmain)Fixes #1006
Summary by CodeRabbit
New Features
Bug Fixes