fix(memory): refresh rebuilt index handles#90727
Conversation
|
Thanks for the context here. I did a careful shell check against current Close as implemented on current main: the whole-file memory index replacement that made live managers keep stale rebuilt-index handles has been replaced by per-agent SQLite storage plus transactional memory-table publication, so the branch's central handle-refresh fix is no longer needed. So I’m closing this as already implemented rather than keeping a duplicate issue open. Review detailsBest possible solution: Keep the current per-agent SQLite/table-publication architecture and continue the broader safe-reindex concurrency work in its canonical issue/PR instead of merging this obsolete handle-refresh branch. Do we have a high-confidence way to reproduce the issue? Yes for the original reported branch scenario: the PR discussion supplied live before/after output, and source inspection shows the old whole-file rebuild path could orphan a running manager's open DB handle; current main no longer uses that path. Is this the best way to solve the issue? No, this PR is no longer the best fix: current main removes the underlying file-swap handle problem by publishing rebuilt memory tables into the existing agent DB, while the remaining in-flight reindex/read race belongs to the separate canonical work. Security review: Security review cleared: The branch touches memory-core runtime and tests only; it does not change secrets, auth, dependencies, workflows, package resolution, or external code execution paths. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against a42a1af942b2; fix evidence: commit 8b7269d19782, main fix timestamp 2026-06-18T17:20:06Z. |
|
Updated the PR body with direct injected-tool proof for the P1 ClawSweeper ask. Proof sequence now included:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
kagura-agent
left a comment
There was a problem hiding this comment.
Review from an affected user (OpenClaw 2026.6.1, 12 agents, all with missing meta)
✅ What this PR gets right
1. lastMetaSerialized = null before temp DB switch (manager-sync-ops.ts)
This is the primary fix. The root cause is that writeMeta() skips the INSERT when the serialized value matches the cached lastMetaSerialized from the original DB, but after this.db is swapped to the temp DB, the cache still holds the old value. Clearing it ensures the meta row is always written to the fresh temp DB. The save/restore in originalState is also correct for the error recovery path.
2. refreshDatabaseHandleIfPathChanged() with inode tracking (manager.ts)
This solves the second failure mode: CLI rebuilds the index (atomic file swap), but the gateway process keeps an open handle to the old (now-deleted) inode. Using dev + ino + size + mtimeMs to detect file replacement and reopen is a solid approach. Calling it at three points (search, sync, refreshIndexIdentityDirty) provides good coverage.
3. Moving async search sync after index identity validation
Smart change. The old position could trigger a background sync that itself hits the missing-meta state, creating noise. Deferring until after validation avoids self-induced dirty-state work.
🤔 Minor observations (not blocking)
1. statSync on the hot path
refreshDatabaseHandleIfPathChanged() calls statSync() on every search and sync. For most installations this is negligible (stat is fast on local fs, and the DB path is cached by the OS). But if anyone runs memory on a network mount, this could add latency. A debounce or TTL cache on the stat result might be worth considering — though I'd defer to maintainer judgment on whether the complexity is warranted.
2. The identity check uses size + mtimeMs alongside dev + ino
Since the atomic swap uses rename (which changes inode), dev + ino alone would be sufficient to detect a swap. Including size + mtimeMs adds robustness for edge cases where the file is modified in-place (e.g., WAL checkpoint changing size), but it could also cause unnecessary reopens during normal WAL activity. In practice this probably doesn't matter because the guard if (this.closed || this.syncing !== null) prevents reopens during active operations.
3. Test coverage looks good
Both test cases (missing meta + valid meta with content change) cover the key scenarios. The test that deletes the meta row and then rebuilds via a separate CLI manager mirrors the real-world failure path exactly.
Verdict
This PR correctly addresses the two root causes I independently identified through DB inspection and source analysis on my own installation. The lastMetaSerialized cache bug is the primary issue; the stale file handle is the secondary one. Both fixes are minimal and well-scoped. 👍
|
Supplemental affected-install field sequence from an unpatched OpenClaw This is not branch validation of this PR. It is diagnostic evidence that the same stale live runtime / rebuilt-index split still appears on my install, and that a gateway/runtime restart clears the stale metadata state. Environment:
Pre-repair baseline: Hydrate command: Wrapped command: Hydrate result: Immediately after hydrate, CLI/helper state became valid: But the already-running live runtime still returned the old failure: After a gateway-only restart, helper status was still healthy/valid: Post-restart live memory search no longer returned A second confirmation canary succeeded through the builtin OpenAI-backed memory path: Interpretation:
This matches the stale live SQLite handle/cache class this PR is addressing, distinct from the OpenRouter/openai-compatible provider-key reports being discussed elsewhere. |
|
ClawSweeper applied the proposed close for this PR.
|
Summary
index metadata is missingafter a separate CLI/status manager has rebuilt the durable SQLite index successfully.memory_index_meta_v1into the fresh temp DB even when the previous DB had the same serialized metadata cached.Linked context
Closes #90414
Related #90361
Related #90338
Related #90395
Related #90453
Requested by an affected owner/user during live troubleshooting of a local OpenClaw install.
Real behavior proof (required for external PRs)
memory_search/openclaw.memory_searchcould reportindex metadata is missingeven though the durable disk index was healthy and CLI/fresh-manager search worked.2026.6.2-beta.1 (f03f57a), Nodev25.6.1, builtin memory index, installed global npm runtime./usr/local/lib/node_modules/openclaw/dist/manager-B5STWg-k.js.node --check /usr/local/lib/node_modules/openclaw/dist/manager-B5STWg-k.js.openclaw gateway stopandopenclaw gateway start.openclaw gateway call tools.invoke --json --params '{"name":"memory_search","args":{"query":"openclaw memory_search metadata","maxResults":1,"corpus":"memory"},"agentId":"main","sessionKey":"agent:main:main","idempotencyKey":"memory-pr-proof-warm-2"}' --timeout 30000openclaw memory index --agent main --forceopenclaw memory status --agent main --index --jsonopenclaw gateway call tools.invoke --json --params '{"name":"memory_search","args":{"query":"openclaw memory_search metadata","maxResults":1,"corpus":"memory"},"agentId":"main","sessionKey":"agent:main:main","idempotencyKey":"memory-pr-proof-after-rebuild"}' --timeout 30000memory_searchtool returnedok: truewith provider/model/debug hit details instead ofdisabled=true,unavailable=true, orerror="index metadata is missing".tools.invokegateway path exercises the same gateway-visible injected tool dispatcher without depending on model tool-choice behavior.index metadata is missingafter gateway restart. One matching memory record from2026-06-05 11:54 CDTdocuments a prior liveopenclaw.memory_searchcall returningdisabled=true,unavailable=true,error="index metadata is missing"while CLI status showedindexIdentity.status=valid, vector/FTS ready, and embedding probe OK.Tests and validation
Focused commands run after rebasing onto current
origin/main:Regression coverage added:
index.test.tsnow covers a live manager reopening after another manager atomically rebuilds an index that had missing metadata.index.test.tsalso covers a live manager reopening after another manager atomically rebuilds a valid index with changed content, proving the stale handle is not kept after file replacement.What failed before this fix:
runSafeReindex()could switch to a fresh temp DB whilelastMetaSerializedstill matched the previous DB, causingwriteMeta()to skip writing metadata into the new DB.main.sqlite, so status/search could keep seeing missing or stale metadata while disk and CLI were healthy.Risk checklist
Did user-visible behavior change? (
Yes/No): Yes, memory search should recover from externally rebuilt indexes instead of staying paused on stale live manager state.Did config, environment, or migration behavior change? (
Yes/No): No.Did security, auth, secrets, network, or tool execution behavior change? (
Yes/No): No.What is the highest-risk area?
How is that risk mitigated?
memory_searchcall succeeding after an external CLI full rebuild.Current review state
What is the next action?
What is still waiting on author, maintainer, CI, or external proof?
Which bot or reviewer comments were addressed?
memory_searchcall after an external CLI rebuild showing the missing-metadata error is gone.