fix(memory): harden builtin sync and reindex semantics#55497
Conversation
Greptile SummaryThis PR hardens the three distinct memory-sync contracts (background best-effort incremental sync, targeted post-compaction session refresh, and atomic full reindex) that were previously sharing the same fail-fast control flow. The key behavioural improvements are: background sync now keeps the last good searchable state for retryable per-file local failures and marks only those files dirty for retry; targeted session refresh now surfaces an explicit failure instead of silently partial-succeeding; full reindex is now fully atomic via a typed Confidence Score: 4/5Safe to merge — the PR is a clear net improvement over the previous fail-fast behaviour, all changed paths have new targeted tests, and the two issues found are both P2 edge cases that do not affect the primary sync/reindex paths under normal operation. The logic is well-reasoned, the new invariants are all covered by tests, and the schema change is additive with lazy backfill. The one real logic concern (unprocessed session files not entering sessionsDirtyFiles after a provider-error mid-reindex) requires a very specific concurrent timing window (session transcript written during a failed force-reindex), and the impact is limited to stale index entries until the user's next force reindex. The withIndexWriteSavepoint async risk has no current callers. Neither issue is a regression relative to the old behaviour. Score is 4 rather than 5 because the provider-error dirty-set gap is a concrete (if unlikely) correctness hole that is straightforward to close. src/memory/manager-sync-ops.ts — specifically restoreRetryStateAfterReindexRollback and withIndexWriteSavepoint.
|
| Filename | Overview |
|---|---|
| src/memory/manager-sync-ops.ts | Core of this PR — adds typed MemorySyncError, per-file savepoints, rollback-state restoration, identity-key-based stale-prune guards, and separates incremental vs. full-reindex commit semantics. Two issues found: withIndexWriteSavepoint has no type guard against accidental async callers, and partially-processed session files after a provider-error-interrupted reindex can escape the dirty-file retry set. |
| src/memory/manager-embedding-ops.ts | Wraps all embedding-cache DB operations in asLocalSyncError, moves per-batch upsertEmbeddingCache inside the loop (with toCache.length = 0 flush) so successful batches are durable before the next batch runs, and wraps multi-table chunk/file writes in withIndexWriteSavepoint. Straightforward and correct. |
| src/memory/internal.ts | Adds identityKey field to MemoryFileEntry and exports buildFileIdentityKey (dev:ino string). Additive, backward-compatible change; existing rows back-fill lazily via the schema migration path. |
| src/memory/memory-schema.ts | Adds identity_key TEXT column to the files table (new schema + ensureColumn backfill) and a corresponding index. Additive and non-breaking for existing databases. |
| src/memory/session-files.ts | Adds identityKey field to SessionFileEntry using the same buildFileIdentityKey helper. Minimal and consistent with the memory-file change. |
| src/memory/index.test.ts | Splits the single targeted-sync test into two focused tests: one for dirty-session preservation and one for explicit failure surfacing on targeted refresh failures. Imports MemorySyncError and adds embedBatchOverride hook for future fault-injection. |
| src/memory/manager.atomic-reindex.test.ts | Replaces the single regression test with five focused tests covering: safe reindex failure preservation, unsafe fast-mode rollback, memory dirty state after session-caused rollback, all session files re-queued after a partial rollback, and live dirty-state updates surviving a rollback. Good coverage of the new invariants. |
| src/memory/manager.embedding-batches.test.ts | Adds a test verifying that successful batch embeddings cached before a later batch fails are reused on retry (only 1 extra provider call after the initial 2). Directly locks in the per-batch persistence fix. |
| src/memory/test-embeddings-mock.ts | Adds ssrfPolicy to the OpenAI mock using buildRemoteBaseUrlPolicy, aligning the mock with the shape expected by the real remote HTTP layer. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/memory/manager-sync-ops.ts
Line: 340-356
Comment:
**`withIndexWriteSavepoint` silently breaks with async actions**
The method signature `protected withIndexWriteSavepoint<T>(action: () => T): T` accepts async lambdas without a type error — TypeScript infers `T = Promise<void>` — but the savepoint semantics are synchronous-only. When an async lambda is passed, `RELEASE SAVEPOINT` fires as soon as the `Promise` is returned (before any async writes complete), making the savepoint a no-op as a write boundary.
All current callers pass synchronous lambdas, so there is no current bug. But the absence of a compile-time guard means a future caller could accidentally pass `async () => { ... }` and get silently incorrect behaviour without any warning.
A minimal safeguard is a JSDoc comment. A stronger approach is a type-level constraint:
```ts
protected withIndexWriteSavepoint<T>(action: () => Exclude<T, Promise<unknown>>): T {
```
or at minimum:
```ts
/**
* SYNC-ONLY: `action` must be a synchronous callback.
* Passing an async lambda will release the savepoint before writes complete.
*/
protected withIndexWriteSavepoint<T>(action: () => T): T {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/memory/manager-sync-ops.ts
Line: 948-966
Comment:
**Unprocessed session files may be silently skipped on the next incremental sync after a provider-error-interrupted full reindex**
When `runWithConcurrency` terminates early because a provider error is thrown mid-way through session sync, `syncedFiles` captures only the tasks that completed before the concurrency slot stopped. Session files that were never dispatched are in neither `syncedFiles` nor `failedFiles`.
In `restoreRetryStateAfterReindexRollback`, only `syncedFiles` and `failedFiles` are added to `sessionsDirtyFiles`:
```ts
this.markSessionFilesDirty(params.sessionResult.syncedFiles);
this.markSessionFilesDirty(params.sessionResult.failedFiles);
```
This leaves the unprocessed files absent from `sessionsDirtyFiles`. On the next incremental (non-forced) sync, `indexAll` is `false` (because `sessionsDirtyFiles.size > 0`), so only files present in `sessionsDirtyFiles` are processed — the unprocessed files are silently skipped.
For most cases this is harmless because the DB was rolled back and hash comparison would skip unchanged files anyway. However, if a session transcript was appended to (by a concurrent agent session) during the failed reindex window and was not among the processed files, it will remain stale in the index until the user runs another force reindex.
A conservative fix would be to clear `sessionsDirtyFiles` (letting `indexAll = true`) instead of populating it with partially-complete work when the reindex was aborted due to a provider error:
```ts
private restoreRetryStateAfterReindexRollback(params: {
memoryReindexStarted?: boolean;
sessionReindexStarted?: boolean;
sessionResult?: SessionFileSyncResult;
}) {
if (params.memoryReindexStarted) {
this.dirty = true;
}
if (params.sessionReindexStarted) {
this.sessionsDirty = true;
// If reindex was aborted by a provider error, sessionResult.fatalProviderError is set.
// In that case, we cannot trust the partial syncedFiles set to cover all dirty work,
// so clear sessionsDirtyFiles so the next sync uses indexAll=true.
if (params.sessionResult?.fatalProviderError) {
this.sessionsDirtyFiles.clear();
return;
}
this.markSessionFilesDirty(params.sessionResult?.syncedFiles);
this.markSessionFilesDirty(params.sessionResult?.failedFiles);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): harden builtin sync and rei..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Superseded by #59137. The original branch targeted the pre-plugin memory layout under |
1 participant
Summary
src/memory/*implementation plus targeted/manual repros.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
syncMemoryFiles()/syncSessionFiles()decide not only whether a file is indexed, but also whether stale rows are pruned, whether targeted session refresh counts as complete, whether a full reindex may commit, and which dirty/session-dirty work must be restored after rollback. At the same time,indexFile()performs multi-table delete/rewrite work (files,chunks, vector rows, FTS rows) without a per-file atomic boundary, so "continue on error" is only safe if partial writes cannot leak through.git blame, prior PR, issue, or refactor if known): this follows the same reliability thread as #1151 (atomic indexing), #10324 (multi-write paths lacking transaction wrappers), #10863 (dirty-state semantics), #25561 (mode-aware post-compaction session reindexing), #31961 (continue on embedding errors), and #42578 (per-batch cache persistence). It also extends the provider error taxonomy direction already accepted in #39377.Batch: disabled. It is also not just a retry-pattern issue, because retry-only changes do not fix partial-write hazards, rollback-state loss, or targeted refresh false positives.Regression Test Plan (if applicable)
src/memory/index.test.tssrc/memory/manager.atomic-reindex.test.tssrc/memory/manager.embedding-batches.test.tsUser-visible / Behavior Changes
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
provider: openai,model: BAAI/bge-m3)memorySearch.provider: "openai"memorySearch.model: "BAAI/bge-m3"memorySearch.extraPaths: [<Obsidian vault>]memorySearch.cache.enabled: truememorySearch.remote.batch.enabled: falseSteps
memorySearch.extraPathscorpus and a rate-limited remote embedding provider.openclaw memory index --force.429during indexing.openclaw memory status.openclaw memory index.Expected
Actual
Before this change:
openclaw memory index --forcefailed with a provider429.openclaw memory statusshowedIndexed: 0/2343 files · 0 chunksand an empty embedding cache.After this change and rebuild:
openclaw memory index --forcecompleted substantial work instead of failing the whole run at the first provider interruption.openclaw memory statusshowedIndexed: 2331/2343 files · 9726 chunksandEmbedding cache: enabled (8913 entries).openclaw memory indexcompleted the remaining files toIndexed: 2343/2343 files · 10033 chunks.Evidence
Attach at least one:
CLI before
CLI after rebuild with this patch
Local targeted tests
pnpm test -- src/memory/manager.atomic-reindex.test.ts src/memory/index.test.ts src/memory/manager.embedding-batches.test.tsHuman Verification (required)
What you personally verified (not just CI), and how:
Verified
openclaw memory index --forcepreviously failed on a real rate-limited corpus withIndexed: 0/....Verified the patched build completed substantial force-reindex work without discarding healthy-file progress.
Verified
openclaw memory statusshowed substantial indexed file/chunk counts and a populated embedding cache after the patched run.Verified a follow-up incremental
openclaw memory indexcompleted the remaining files.Verified the new focused regression tests pass locally.
Edge cases checked:
What you did not verify:
Review Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
src/memory/manager-sync-ops.tssrc/memory/manager-embedding-ops.tssrc/memory/internal.tssrc/memory/session-files.tssrc/memory/memory-schema.tsRisks and Mitigations
identity_keyexpands the builtin memory schema and stale-prune logic.