fix(memory-core): write meta after sync when identity is missing with existing chunks#90395
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 6, 2026, 6:14 AM ET / 10:14 UTC. Summary PR surface: Source +21, Tests +95. Total +116 across 2 files. Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main and v2026.6.1 return dirty when metadata is missing but chunks exist, leaving search with missing identity. The PR body also supplies real SQLite terminal proof that the patched sync recovers the index. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow missing-meta recovery only after maintainers accept the provider-unavailable pause semantics and keep the concurrent reindex and stale-handle fixes scoped to their canonical PRs. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: current main and v2026.6.1 return dirty when metadata is missing but chunks exist, leaving search with missing identity. The PR body also supplies real SQLite terminal proof that the patched sync recovers the index. Is this the best way to solve the issue? Yes, this is the best narrow fix for the missing-meta dead loop because it reuses verified full reindex instead of writing metadata over unverified chunks. The provider-unavailable pause and related PR ordering still need maintainer acceptance before merge. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against f4a5e5762e27. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +21, Tests +95. Total +116 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Thanks for working on this. I reported a possibly related memory_search issue here: I’m adding this clarification because PR #90395 looks related to the same memory-index failure cluster, but I’m not sure it addresses the specific behavior I observed in #90361. My observed/local issue was not only “metadata missing while chunks exist.” The concerning path was:
So from my perspective, the safer fix is probably not just “write metadata if missing but chunks exist.” That may repair one dead-loop symptom, but it seems risky if the existing chunks are stale, deleted, from a different provider/scope/chunking config, or otherwise unverified. Expected behavior, in plain terms:
I agree with the ClawSweeper concern that stamping fresh metadata onto unverified existing chunks could make stale/out-of-scope memory searchable. I also agree deterministic race coverage would be ideal: pause safe reindex after temp DB exposure, then perform concurrent search/status reads. I’m avoiding more stress testing on my live install for now because it is stable after a local mitigation, but I can provide screenshots/logs if the issue recurs. One reason I think this is important: this failure can be quiet from the user’s perspective. If the agent does not explicitly surface the memory_search unavailable result, the user may only experience degraded recall and assume the agent forgot or answered weakly. |
9fd8ef7 to
29da937
Compare
29da937 to
17ac669
Compare
|
Thanks for the context on #90361 — that is helpful for understanding the broader memory-index failure cluster. This PR (#90395) is deliberately scoped to the single dead-loop path in : when identity is missing but prevents a full reindex, the sync returns early without writing meta. The fix adds the missing call in that specific early-return branch, matching the existing meta write pattern already used by / . The race condition you described in #90361 — where a search-triggered async sync/reindex begins before the current search completes, and safe reindex temporarily exposes a temp DB / missing metadata — sounds like a separate timing/atomicity issue in the search→sync→reindex pipeline. That one would likely need a dedicated fix that coordinates the search lifecycle with the reindex lifecycle, which is outside the scope of this change. If #90361 still reproduces after this fix lands, a follow-up PR targeting the search/reindex interleaving specifically would be the right next step. |
17ac669 to
0535a64
Compare
|
Updated PR body with real behavior proof from standalone script exercising the actual memory database code path ( |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Updated the fix to trigger a verified full reindex instead of writing unverified meta. The reindex path ( Changes in this push:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Added semantic-index guard: when a specific embedding provider (e.g. openai) is configured but unavailable, the recovery path leaves the index dirty/paused instead of forcing an FTS-only reindex that would wipe vector embeddings. For Test coverage (10 FTS-only tests, 3 recovery scenarios):
All 49 memory tests pass across 5 test files. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Added live memory-search proof using
The proof uses the actual memory-core module ( All 49 memory tests pass (10 FTS-only + 39 broader suite). @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
When identity is missing but indexed chunks exist, force a verified full reindex via runSafeReindex/runUnsafeReindex instead of returning with a dirty/paused index. The reindex path prunes stale chunks from deleted paths, old providers, or old scopes before writing meta, so the index does not become searchable with unverified memory rows. Guard: when a specific embedding provider is configured but currently unavailable, the recovery path leaves the index dirty/paused instead of forcing an FTS-only reindex that would wipe existing vector embeddings. For auto/none/unset providers or when the provider is available, the verified full reindex proceeds safely. Adds three regression tests: - triggers verified reindex when identity missing + chunks exist - prunes stale chunks from deleted paths during recovery - preserves index when specific provider is unavailable Closes openclaw#90338 Co-Authored-By: Claude Opus 4.8 <[email protected]>
019865b to
be3db39
Compare
|
Superseded by #91897, merged as 1a2eb74. The landed fix keeps the missing-identity self-heal fail-closed when any semantic chunks exist, while allowing automatic recovery only when all stored chunks are explicitly Closing this duplicate in favor of the landed canonical implementation. The separate live-process DB inode-swap problem described in #91167 remains open and is not claimed fixed by this merge. |
Summary
memory_index_meta_v1meta table entry whenindexIdentity.status !== "valid"and!needsFullReindex. Allmemory_searchcalls return "index metadata is missing" with 0 results.runSync()detects identity is missing but existing chunks prevent a full reindex, the fix forces a verified full reindex viarunSafeReindex/runUnsafeReindex. The reindex path prunes stale chunks from deleted paths, old providers, or old scopes before writing meta.Design note on stale-row safety
The recovery path triggers a full reindex rather than stamping fresh metadata onto unverified chunks. The reindex path (
runSafeReindex/runUnsafeReindex) scans current sources, indexes only matching files, prunes stale rows from deleted paths, and writes meta only after verification.Guard against semantic-index downgrade: When a specific embedding provider (e.g.
openai) is configured but currently unavailable, the recovery path leaves the index dirty/paused instead of forcing a reindex. Without this guard, a provider outage would cause the recovery path to silently rebuild an existing semantic/vector index as FTS-only, permanently losing vector embeddings. Forauto/none/unset providers or when the provider is available, the verified full reindex proceeds safely.Change Type
Scope
Linked Issues
Real behavior proof
Behavior addressed: Memory index meta never written when auto-sync finds identity missing with existing chunks (#90338). Every sync cycle indexes files, creates chunks, returns early without writing meta — dead loop. The fix triggers a verified full reindex in this recovery path.
Real environment tested: Node v22.22.0, Linux x86_64, OpenClaw 2026.6.1, memory-core builtin backend with FTS-only provider.
Exact steps or command run after this patch:
Step 1 — Manager-level tests (10 FTS-only tests, 3 recovery scenarios):
Step 2 — Full memory test suite (5 files, 49 tests):
Step 3 — Live memory-search proof using
MemoryIndexManagerwith real SQLite (console output):Evidence after fix (terminal capture — full recovery path):
Observed result after fix:
sync()without force enters the recovery path, triggers verified full reindexmemory_searchreturns results again, dead loop permanently brokenWhat was not tested: Live OpenClaw gateway with real embedding provider (e.g. OpenAI embeddings). The related safe-reindex race (#90453) and stale-handle fix (#90727) are tracked separately.
Co-Authored-By: Claude Opus 4.8 [email protected]