[codex] Fix memory close sync race by spacegeologist · Pull Request #86765 · openclaw/openclaw

spacegeologist · 2026-05-26T05:22:57Z

Summary

Register MemoryIndexManager.sync() as in-flight before awaiting provider initialization so close() can see syncs that are blocked inside provider setup.
Delay provider and SQLite teardown until the current sync has settled, while still tracking provider references seen during shutdown so fallback swaps can be closed safely.
Update memory-core regression coverage for both existing pending-sync shutdown and the provider-initialization interleaving.

Root Cause

close() captured this.syncing before awaiting provider initialization and watcher teardown. A sync that was already started but still awaiting ensureProviderInitialized() could attach this.syncing after that stale snapshot, letting close() proceed to close provider and database resources while the sync continued.

Real Behavior Proof

Behavior addressed: MemoryIndexManager.close() no longer closes embedding providers or the SQLite database before already-started sync work settles, including syncs blocked in provider initialization.

Real environment tested: Local sparse checkout on macOS arm64 with Node v24.13.1 and installed workspace dependencies. The proof runs a standalone Node runtime script against the real MemoryIndexManager, real SQLite index, and the public memory embedding provider registry. The script creates a temporary workspace and memory file, starts sync({ force: true }), blocks provider creation, calls close(), then releases provider init and holds the real embedBatch() call before allowing sync to finish.

Exact steps or command run after this patch:

NODE_NO_WARNINGS=1 node --import tsx /private/tmp/openclaw-86765-proof.mjs

Evidence after fix:

openclaw#86765 runtime proof
node: v24.13.1
platform: darwin arm64
workspace: /var/folders/qm/d7_ghh9j1019scbft187ry000000gn/T/openclaw-86765-proof-511y6J
scenario: sync blocked during provider init, close() called, provider init released, sync held inside real embedBatch
during blocked sync:
  close settled: false
  provider close calls: 0
  embedBatch calls: 1
  sqlite SELECT 1: ok
after releasing sync:
  close error: none
  provider close calls: 1
  sqlite after close: database is not open
events: provider:create:entered -> provider:create:resolved -> provider:embedBatch:start:1 -> provider:embedBatch:released -> provider:close:1
PASS: close waited for the in-flight sync before closing provider/database resources

Observed result after fix: While sync was blocked inside the real embedBatch() path, close() had not settled, the embedding provider had not been closed, and SQLite still answered SELECT 1. After releasing sync, close() completed without error, the provider closed exactly once, and SQLite reported database is not open, which shows resource teardown happened after the in-flight sync settled.

What was not tested: The proof uses a local registered provider adapter to make the race deterministic; it does not call external embedding APIs. Full repository lint/check was not run locally. The repo run-oxlint.mjs wrapper tries to prepare extension package boundary artifacts for unrelated sparse-checkout extensions, so the PR uses direct touched-file oxfmt/oxlint plus the relevant memory tests.

Verification

NODE_NO_WARNINGS=1 node --import tsx /private/tmp/openclaw-86765-proof.mjs - passed, runtime proof above
node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-memory.config.ts --configLoader runner extensions/memory-core/src/memory/index.test.ts extensions/memory-core/src/memory/manager.async-search.test.ts extensions/memory-core/src/memory/manager.readonly-recovery.test.ts - passed, 3 files / 32 tests
./node_modules/.bin/oxfmt --check extensions/memory-core/src/memory/manager.ts extensions/memory-core/src/memory/index.test.ts - passed
./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/memory-core/src/memory/manager.ts extensions/memory-core/src/memory/index.test.ts - passed
git diff --check origin/main...HEAD - passed

clawsweeper · 2026-05-26T05:25:26Z

Codex review: needs maintainer review before merge. Reviewed May 26, 2026, 9:21 PM ET / 01:21 UTC.

Summary
The PR changes memory-core sync/close ordering so sync work is registered before provider initialization and provider/SQLite teardown waits for tracked sync/provider shutdown, with focused regression tests.

PR surface: Source +31, Tests +62. Total +93 across 2 files.

Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main registers this.syncing only after provider initialization, while close snapshots pendingSync before await points. I did not run tests in this read-only review, but the PR adds deterministic coverage for the interleaving.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge
No repair lane is needed because the reviewed merge result has supplied real runtime proof and no actionable patch defect; the next action is ordinary maintainer and CI review.

Security
Cleared: No concrete security or supply-chain concern was found in this memory-core implementation/test diff.

Review details

Best possible solution:

Land the narrow memory-core fix after normal maintainer and CI review, then close #86702 if the merge is accepted as the canonical fix.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a high-confidence reproduction path: current main registers this.syncing only after provider initialization, while close snapshots pendingSync before await points. I did not run tests in this read-only review, but the PR adds deterministic coverage for the interleaving.

Is this the best way to solve the issue?

Yes, the merge-result approach is a narrow fix: register in-flight sync work before provider initialization, then delay provider and database teardown until the current sync has settled. It preserves the current native watcher close path in the merge result.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against f6599ede0d5b.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix terminal runtime proof exercising the real manager and SQLite path with a gated registered provider, showing teardown waits until the sync is released.
add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

P2: This is a normal-priority memory-core race fix with limited blast radius and focused regression coverage.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies after-fix terminal runtime proof exercising the real manager and SQLite path with a gated registered provider, showing teardown waits until the sync is released.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix terminal runtime proof exercising the real manager and SQLite path with a gated registered provider, showing teardown waits until the sync is released.

Evidence reviewed

PR surface:

Source +31, Tests +62. Total +93 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	38	7	+31
Tests	1	71	9	+62
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	109	16	+93

What I checked:

Current-main race source: On current main, sync awaits provider initialization before assigning this.syncing, so close can observe no pending sync while provider initialization is still in progress. (extensions/memory-core/src/memory/manager.ts:741, f6599ede0d5b)
Current-main early provider close: On current main, close snapshots pendingSync before await points and closes the current provider before waiting for the captured pending sync. (extensions/memory-core/src/memory/manager.ts:1028, f6599ede0d5b)
Merge-result fix shape: The merge result assigns this.syncing before ensureProviderInitialized and close re-reads the current sync after provider initialization before draining tracked providers. (extensions/memory-core/src/memory/manager.ts:747, c3b14a2a0077)
Regression coverage: The merge result includes tests for both pending-sync provider close ordering and the provider-initialization interleaving that attaches this.syncing after close starts. (extensions/memory-core/src/memory/index.test.ts:408, c3b14a2a0077)
Real behavior proof supplied: The PR body includes after-fix terminal output from a standalone Node runtime script using the real MemoryIndexManager, real SQLite index, and registered provider adapter; close stayed pending until the gated sync released, then provider and DB teardown completed. (ec4263303c86)
Security surface: The reviewed merge-result diff only changes memory-core TypeScript implementation and tests; it does not touch workflows, dependencies, lockfiles, package scripts, publishing metadata, or secret handling. (c3b14a2a0077)

Likely related people:

steipete: Authored the follow-up PR commit that refactors and clarifies memory-core provider draining during close, and is assigned on the PR timeline. (role: recent area contributor; confidence: high; commits: ec4263303c86; files: extensions/memory-core/src/memory/manager.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

clawsweeper · 2026-05-26T05:29:25Z

ClawSweeper PR egg

✨ Hatched: 🌱 uncommon Frosted Clawlet

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

Merged PRs are hatchable.
Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🌱 uncommon.
Trait: sleeps inside passing CI.
Image traits: location diff observatory; accessory green check lantern; palette coral, mint, and warm cream; mood celebratory; pose curling around a status light; shell soft velvet shell; lighting moonlit rim light; background delicate sparkle particles.
Share on X: post this hatch
Copy: My PR egg hatched a 🌱 uncommon Frosted Clawlet in ClawSweeper.

What is this egg doing here?

Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

spacegeologist · 2026-05-26T05:59:15Z

@clawsweeper re-review

I added a standalone runtime proof to the PR body. It runs the real MemoryIndexManager + SQLite path with a gated registered embedding provider and shows close() stays pending, provider close calls remain 0, and SQLite SELECT 1 succeeds while sync is held; after releasing sync, provider closes once and SQLite is closed.

clawsweeper · 2026-05-26T05:59:17Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26435181710
Updated: 2026-05-26T06:06:28.501Z

steipete · 2026-05-27T01:21:45Z

Verification for head ec42633:

Behavior addressed: memory close now drains providers through one explicit helper path, preserving the provider-created-during-shutdown retry while removing the duplicated close call sequence.
Real environment tested: local macOS checkout plus GitHub Actions CI.
Exact steps or command run after this patch:

./node_modules/.bin/oxfmt --check extensions/memory-core/src/memory/manager.ts
./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/memory-core/src/memory/manager.ts
git diff --check
node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-memory.config.ts --configLoader runner extensions/memory-core/src/memory/index.test.ts
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
gh --live run rerun 26484743893 --repo openclaw/openclaw --failed
node scripts/run-vitest.mjs run --config test/vitest/vitest.runtime-config.config.ts src/config/sessions.cache.test.ts --reporter=verbose
Evidence after fix: memory-core targeted Vitest passed 20 tests; autoreview reported clean with no accepted/actionable findings; targeted local runtime-config session cache repro passed 27 tests.
Observed result after fix: PR CI run 26484743893 is green except checks-node-core-runtime-infra-process, which failed twice on src/config/sessions.cache.test.ts JSON.parse spy assertions. Main CI run 26484725201 at 7c432d2 failed the same job and same assertions, so this is pre-existing main breakage outside this PR's touched surface.
What was not tested: no full local pnpm check/test; no Testbox run. GitHub Actions full CI was used, with the unrelated main-broken runtime-config job noted above.

fix(memory-core): wait for sync before closing resources

a26fbba

openclaw-barnacle Bot added extensions: memory-core Extension: memory-core size: S triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. labels May 26, 2026

clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels May 26, 2026

spacegeologist marked this pull request as ready for review May 26, 2026 05:31

clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels May 26, 2026

openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. labels May 26, 2026

steipete self-assigned this May 27, 2026

refactor(memory-core): clarify close provider draining

ec42633

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 27, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 27, 2026

steipete merged commit e982302 into openclaw:main May 27, 2026
160 of 162 checks passed

clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels May 27, 2026

brokemac79 mentioned this pull request May 27, 2026

MemoryIndexManager.close() races with in-flight sync — provider/DB closed before sync settles #86702

Closed

clawsweeper Bot mentioned this pull request Jul 5, 2026

fix(memory): report close-time pending work failures #100308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[codex] Fix memory close sync race#86765

[codex] Fix memory close sync race#86765
steipete merged 2 commits into
openclaw:mainfrom
spacegeologist:codex/memory-close-sync-race

spacegeologist commented May 26, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 26, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 26, 2026 •

edited

Loading

Uh oh!

spacegeologist commented May 26, 2026

Uh oh!

clawsweeper Bot commented May 26, 2026 •

edited

Loading

Uh oh!

steipete commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

spacegeologist commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Real Behavior Proof

Verification

Uh oh!

clawsweeper Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clawsweeper Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hatch command

Uh oh!

spacegeologist commented May 26, 2026

Uh oh!

clawsweeper Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spacegeologist commented May 26, 2026 •

edited

Loading

clawsweeper Bot commented May 26, 2026 •

edited

Loading

clawsweeper Bot commented May 26, 2026 •

edited

Loading

clawsweeper Bot commented May 26, 2026 •

edited

Loading