Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
openclaw-barnacle
Bot
added
extensions: memory-core
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 11:00 AM ET / 15:00 UTC. ClawSweeper reviewWhat this changesThe PR recognizes aged legacy Merge readiness⛔ Blocked until stronger real behavior proof is added - 7 items remain Keep open: the PR reaches unarchived legacy sidecars, but misses stale shadows after an earlier Doctor run archived the base file; it can also report deletion that did not complete. Priority: P2 Review scores
Verification
How this fits togetherMemory Core stores searchable agent memory in SQLite. Doctor migrates legacy sidecars to the current agent database and archives old files, while reindex cleanup removes stale shadow databases. flowchart LR
A[Legacy sidecar and shadows] --> B[Doctor discovery]
B --> C[Shadow cleanup]
C --> D[Sidecar migration]
D --> E[Archived legacy base]
D --> F[Current agent database]
Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +40, Tests +96. Total +136 across 5 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Use one Doctor-owned, strict age-gated legacy-shadow discovery path that also handles archived bases, retains lock safety, and reports only sets verified absent after deletion. Do we have a high-confidence way to reproduce the issue? Yes, from source: seed an archived Is this the best way to solve the issue? No; the added pre-archive sweep is in the right owner, but it cannot repair installations whose legacy base was archived earlier. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ef44eab5ed83. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Real-setup verification — tested at head Setup: isolated checkout ( After (head Before (merge-base Not covered: live gateway run (unit-level proof only). Verified by Sho (human) + Alpha (AI agent) on a real OpenClaw setup. Happy to re-run against a new head if the branch moves. |
|
@clawsweeper re-review Real, non-test proof: my own long-lived local ~/.openclaw/memory/ directory has 93 genuine leftover .tmp- orphan files from before the naming change. Called the real cleanupAgedMemoryReindexTempFiles directly (no vitest, no mocked fs) against a scratch copy of that real data — 0/93 removed pre-fix, 93/93 removed post-fix. Details in the PR body. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
I traced the actual reindex caller and upgrade path at head In shipped This needs a repair at the upgrade/doctor cleanup owner, with a full upgraded-state reproduction: seed the old directory layout, run the supported doctor/reindex flow, and verify that aged orphan files are reclaimed while active/young artifacts and the live database survive. Calling the exported helper with the old path manually bypasses the missing caller. I am leaving this open until that reachability and safe cleanup lifecycle are implemented and proven. No new-head runtime failure is claimed here; this finding is from the shipped producer, current path resolver, sole production caller, and migration source. |
….tmp- naming cleanupAgedMemoryReindexTempFiles only recognized the current .memory-reindex-<uuid> shadow-db prefix, introduced in 8b7269d. Any orphaned shadow database left over from before that change (named .tmp-<uuid>, the prior scheme) was invisible to the sweep and leaked forever, since a crashed or killed reindex never reaches the normal finally-block cleanup. Recognize both prefixes so the existing age-gated, lock-protected sweep picks up orphans regardless of which naming scheme wrote them.
… sweep Root cause found in review by steipete: the pattern widening this branch already added (recognizing the pre-8b7269d1978 .tmp-<uuid> naming alongside the current .memory-reindex-<uuid> one) was correct, but nothing in production ever calls cleanupAgedMemoryReindexTempFiles with the *legacy* path any more. src/agents/memory-search.ts's store.databasePath always resolves to the current per-agent openclaw-agent.sqlite path via resolveOpenClawAgentSqlitePath; the old stateDir/memory/<agentId>.sqlite default is never revisited by any runtime reindex caller once a deployment has upgraded past that change. Calling the exported helper directly with the old path (as the prior test did) proved the matcher works but bypassed the missing caller entirely -- it didn't prove any real upgraded install ever reaches it. The doctor migration for legacy memory sidecars (memorySidecarStateMigration in doctor-memory-sidecar.ts) is the one place that still knows the legacy path -- it already discovers stateDir/memory/<agentId>.sqlite to import and archive it. Added sweepLegacyMemorySidecarReindexOrphans, called at the start of each legacy-path group's processing, before archiveLegacyMemorySidecar renames the file away (cleanupAgedMemoryReindexTempFiles requires the dbPath it's given to still be a regular file). This makes the orphan sweep reachable from the one real caller that still visits the legacy path, independent of whether the sidecar's data import/archival itself succeeds. cleanupAgedMemoryReindexTempFiles now returns the shadow base names it removed (was void) so the migration can report what happened instead of just that the sweep ran. New regression test (doctor-memory-sidecar.test.ts, none existed for this migration before): seeds the real old directory layout -- a legacy sidecar placeholder plus aged and young pre-8b7269d1978-style orphan shadow sets -- and runs the actual memorySidecarStateMigration.migrateLegacyState doctor flow end to end, not just the isolated cleanup helper. Confirmed this fails on the pre-fix code (aged orphan file still present after the migration ran) and passes after this commit. Verification: doctor-memory-sidecar.test.ts + manager-db.test.ts 11/11; oxfmt/oxlint clean.
53c00be to
4aefde0
Compare
|
@steipete you're right -- fixed at the reachability boundary, not by re-testing the helper in isolation again.
New test (
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
clawsweeper
Bot
added
status: 📣 needs proof
…hival exact-listing check The legacy-orphan sweep (previous commit) now runs against every legacy path a migration run processes, including retry-marked ones -- and, like every other reindex-lock use in this codebase, leaves its .reindex-lock.sqlite coordination file on disk after release() (the lock mechanism is deliberately file-persistent; only the in-process SQLite transaction gets released, see manager-sqlite-lease.ts). That's a real, harmless, already-established side effect of a working sweep -- but this test's exact before/after directory-listing comparison didn't account for a new file category appearing, since nothing previously called cleanupAgedMemoryReindexTempFiles against a legacy (non-canonical) path. Excluded .reindex-lock.sqlite from both snapshots the test compares -- it's a coordination artifact orthogonal to what this test actually verifies (archival completeness of legacy sidecars). Caught on exact-head CI (checks-node-changed-extensions-config), not locally: my original local run only exercised doctor-memory-sidecar.test.ts and manager-db.test.ts directly, not the full extensions/memory-core suite this shares an invariant with. Verification: full extensions/memory-core suite, 100/102 files passing (1 unrelated pre-existing timezone-formatting failure in index.test.ts, confirmed present identically on unmodified main); doctor-contract-api.test.ts itself 63/63.
Update: fixed a real CI regression caught on exact-head, not locallyThe rebase's fix ran against every legacy path a migration processes, including retry-marked ones -- and, like every other reindex-lock use in this codebase, leaves its Excluded My original local proof only ran Verification: full @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…reindex-orphan migration test Matches other extension tests' convention (openclaw/plugin-sdk/test-env's useAutoCleanupTempDirTracker) instead of a manual mkdtemp/afterEach pair -- flagged by check:changed's temp-dir migration warning.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
….tmp- naming (openclaw#131568) Combined-deploy squash of openclaw#131568. See that PR for the full commit history, review discussion, and live-verification evidence.
|
Closing this for now. On my own deployment this saved real disk space — cleaned up the orphaned legacy I don't have the review bandwidth right now to rebase this past the conflict with We tried to get eyes on it — leaving it here for now. |
What Problem This Solves
cleanupAgedMemoryReindexTempFiles(added in 8b7269d, "fix(sqlite): preserve migrations and reindex safety") sweeps aged, crash-left shadow reindex databases so a killed/interrupted full reindex doesn't leak its temp.memory-reindex-<uuid>files forever. But it only recognizes that current naming scheme. Deployments that ran under the prior naming scheme (.tmp-<uuid>, used before 8b7269d, see f324f7e) can still carry orphaned shadow databases under the old name — those are invisible to the sweep'sresolveMemoryReindexBaseNameprefix match and leak forever, since a crashed reindex never reaches its normalfinally-block cleanup.Observed in the wild: a long-lived deployment that predates 8b7269d had accumulated 357 orphaned
main.sqlite.tmp-<uuid>shadow databases (plus-wal/-shmsidecars) totaling 4.7GB, none of which the existing 24h-aged cleanup ever touched because it only matched.memory-reindex-<uuid>.Why This Change Was Made
Root-caused while investigating sustained embedding-provider traffic on a live deployment. The reindex-safety fix in 8b7269d correctly stopped new leaks under the current naming scheme, but didn't provide a migration path for shadow files already orphaned under the scheme it replaced, so those pre-existing leaks were permanent.
User Impact
Any OpenClaw deployment that has been running memory-core across the 8b7269d boundary will have its existing orphaned
.tmp-<uuid>shadow reindex files (and their-wal/-shm/-journalsidecars) swept up by the normal age-gated, lock-protected cleanup the next time a reindex runs, instead of accumulating indefinitely. No behavior change for deployments that never ran the old naming scheme.Evidence
resolveMemoryReindexBaseNameinextensions/memory-core/src/memory/manager-db.tsnow checks both the current.memory-reindex-prefix and the legacy.tmp-prefix (same UUID validation, same suffix set) before treating a file as a reindex shadow orphan.extensions/memory-core/src/memory/manager-db.test.ts) that writes aged.tmp-<uuid>shadow files and assertscleanupAgedMemoryReindexTempFilesremoves them.oxfmt --checkandoxlintclean on both changed files.Real behavior proof: fails on pre-fix code
Reverted only
manager-db.ts(kept the new test) and ran the suite — the new test fails exactly as expected, because the pre-fix matcher never recognizes the legacy.tmp-<uuid>prefix:Real behavior proof: passes after the fix
Restored
manager-db.tsand re-ran the full suite (node scripts/run-vitest.mjs extensions/memory-core/src/memory/manager-db.test.ts --reporter=verbose):Update: real, non-test proof against a genuine upgraded state directory
Addresses ClawSweeper's finding that the existing evidence was test-transcript-only. Found real ammunition: my own long-lived local
~/.openclaw/memory/directory carries 93 genuine leftover.tmp-<uuid>files (31 distinct shadow databases x 3 suffixes each) from before the8b7269d1978rename, several months old -- exactly the real-world deployment this PR fixes, not a synthetic fixture.Copied that real directory to an isolated scratch location (never touched the live one) and called the actual exported
cleanupAgedMemoryReindexTempFilesdirectly from a standalone script -- no vitest, no mockedfs, the real function against real files:Pre-fix (checked out the parent commit's
manager-db.tsagainst the same real data):Post-fix (this PR's
manager-db.ts, fresh copy of the same real data):This is the "non-test Memory Core reindex against an upgraded state directory" the review asked for: a real deployment's actual accumulated legacy orphans, cleaned by the real production function, with a clean pre/post comparison on identical input.