fix(memory): readonly sync recovery by rodrigouroz · Pull Request #25799 · openclaw/openclaw

rodrigouroz · 2026-02-24T20:49:52Z

Summary

Describe the problem and fix in 2–5 bullets:

Problem: memory sync could get stuck on attempt to write a readonly database and keep failing until gateway restart.
Why it matters: session-delta/search memory indexing degrades in long-running gateway sessions.
What changed: added in-flight dedup for concurrent MemoryIndexManager.get() creation and added one-shot readonly recovery in sync() (reopen sqlite handle + retry once).
What did NOT change (scope boundary): no schema changes, no config/env changes, no provider behavior/API contract changes.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #

User-visible / Behavior Changes

Gateway can self-recover memory sync when a stale sqlite handle hits readonly after atomic reindex swap.
Concurrent first-use manager requests for the same agent/config now share one initialization path.

Security Impact (required)

New permissions/capabilities? (Yes/No) No
Secrets/tokens handling changed? (Yes/No) No
New/changed network calls? (Yes/No) No
Command/tool execution surface changed? (Yes/No) No
Data access scope changed? (Yes/No) No
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: Node + pnpm
Model/provider: mocked embeddings in unit tests
Integration/channel (if any): N/A
Relevant config (redacted): default sqlite memory index path

Steps

Trigger concurrent manager creation calls for the same cache key.
Force one sync failure with readonly sqlite error.
Run sync again.

Expected

Concurrent creation returns a single manager instance.
Readonly sync error triggers reopen and a single retry, then succeeds.

Actual

Verified with new tests below.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Passing commands:

pnpm vitest run src/memory/manager.get-concurrency.test.ts src/memory/manager.readonly-recovery.test.ts
pnpm vitest run src/memory/manager.atomic-reindex.test.ts src/memory/manager.sync-errors-do-not-crash.test.ts
pnpm check

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: singleton dedup under concurrent get(), readonly self-heal retry path, non-readonly errors do not retry.
Edge cases checked: retry happens only on readonly-class errors and only once.
What you did not verify: full live channel E2E run on this branch.

Compatibility / Migration

Backward compatible? (Yes/No) Yes
Config/env changes? (Yes/No) No
Migration needed? (Yes/No) No
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert commit Memory: dedupe manager creation and recover readonly sync.
Files/config to restore: src/memory/manager.ts.
Known bad symptoms reviewers should watch for: unexpected retry behavior on non-readonly sync failures.

Risks and Mitigations

Risk: readonly recovery could hide persistent underlying filesystem issues.
- Mitigation: retry is one-shot and only for readonly-class errors; subsequent failures still surface.
Risk: in-flight dedup map could leak pending entries on errors.
- Mitigation: pending key is always cleaned in finally.

Greptile Summary

Adds two targeted fixes to memory sync reliability: (1) deduplicates concurrent MemoryIndexManager.get() calls to prevent multiple initialization paths racing to create the same manager, and (2) adds one-shot readonly recovery in sync() that reopens stale SQLite handles and retries once after atomic reindex swaps.

Both changes are well-scoped, backward-compatible, and address real production failure modes (concurrent gateway init and readonly handle errors after atomic reindex). The implementation correctly:

Uses a pending promise map that cleans up in finally blocks to prevent leaks
Limits retry to readonly-class errors only and retries exactly once
Resets all vector/FTS state during recovery to ensure clean reinitialization
Adds comprehensive test coverage for both dedup and recovery paths

The changelog entry accurately describes the user-visible impact and matches the commit history.

Confidence Score: 5/5

This PR is safe to merge with minimal risk
The changes are narrowly scoped to fix specific failure modes with clear mitigations: (1) concurrent manager creation dedup uses standard async pattern with proper cleanup, (2) readonly recovery is limited to one retry on readonly-class errors only. Both fixes have comprehensive test coverage, maintain backward compatibility, and follow established patterns in the codebase. No schema changes, no config changes, no external API changes.
No files require special attention

_{Last reviewed commit: 2d2d1b9}

Takhoffman · 2026-02-27T18:26:57Z

PR #25799 - fix(memory): readonly sync recovery (#25799)

Merged via squash.

Merge commit: 1867611
Verified:
- pnpm build
- pnpm check
- pnpm test:macmini (fails in this environment at src/daemon/launchd.integration.test.ts beforeAll hook timeout; merged with explicit Tak override)
Changes made:
- CHANGELOG.md
- src/memory/manager.ts
- src/memory/manager.readonly-recovery.test.ts
- src/memory/manager.get-concurrency.test.ts
Why these changes were made:
- Preserve the original memory readonly-recovery fix while adding mitigation hardening before landing: broaden readonly error-shape detection and add readonly-recovery telemetry (attempts/successes/failures/lastError), with targeted tests for code-based readonly errors.
Changelog: CHANGELOG.md updated=true required=true opt_out=false

Thanks @rodrigouroz!

Verified: - pnpm build - pnpm check - pnpm test:macmini (fails in this environment at src/daemon/launchd.integration.test.ts beforeAll hook timeout; merged with Tak override) Co-authored-by: rodrigouroz <384037+rodrigouroz@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>

Verified: - pnpm build - pnpm check - pnpm test:macmini (fails in this environment at src/daemon/launchd.integration.test.ts beforeAll hook timeout; merged with Tak override) Co-authored-by: rodrigouroz <384037+rodrigouroz@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> (cherry picked from commit f446d90)

Verified: - pnpm build - pnpm check - pnpm test:macmini (fails in this environment at src/daemon/launchd.integration.test.ts beforeAll hook timeout; merged with Tak override) Co-authored-by: rodrigouroz <384037+rodrigouroz@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>

openclaw-barnacle bot added size: M experienced-contributor labels Feb 24, 2026

rodrigouroz force-pushed the fix/memory-readonly-sync-recovery branch 2 times, most recently from 2d2d1b9 to 9c3e5f8 Compare February 26, 2026 17:28

rodrigouroz and others added 3 commits February 27, 2026 12:24

Memory: dedupe manager creation and recover readonly sync

e058e15

Changelog: note readonly memory sync recovery fix

36174c7

Memory: harden readonly recovery detection and telemetry

934b24e

Takhoffman force-pushed the fix/memory-readonly-sync-recovery branch from 00ffcbf to 934b24e Compare February 27, 2026 18:26

Takhoffman merged commit 1867611 into openclaw:main Feb 27, 2026
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(memory): readonly sync recovery#25799

fix(memory): readonly sync recovery#25799
Takhoffman merged 3 commits intoopenclaw:mainfrom
rodrigouroz:fix/memory-readonly-sync-recovery

rodrigouroz commented Feb 24, 2026 •

edited by greptile-apps bot

Loading

Uh oh!

Uh oh!

Takhoffman commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rodrigouroz commented Feb 24, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Takhoffman commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rodrigouroz commented Feb 24, 2026 •

edited by greptile-apps bot

Loading