Skip to content

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

Closed
lin-hongkuan wants to merge 1 commit into
openclaw:mainfrom
lin-hongkuan:lhk/fix-memory-close-errors-96766
Closed

fix(memory): report close-time pending work failures#100308
lin-hongkuan wants to merge 1 commit into
openclaw:mainfrom
lin-hongkuan:lhk/fix-memory-close-errors-96766

Conversation

@lin-hongkuan

@lin-hongkuan lin-hongkuan commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes memory manager close-time diagnostics so rejected pending sync/provider initialization work is no longer swallowed silently.

Issue/Motivation

Fixes #96766.

awaitPendingManagerWork previously caught and discarded errors from both pendingSync and pendingProviderInit. If shutdown races with a failed memory sync, SQLite/filesystem failure, or provider initialization failure, the close path could hide the failure completely.

Changes

  • Adds an optional onError callback to awaitPendingManagerWork and invokes it for rejected pending sync/provider initialization promises.
  • Wires MemoryIndexManager.close() to log close-time pending-work failures with the existing formatErrorMessage helper.
  • Adds regression coverage for rejected sync, rejected provider initialization, and successful pending work.

Testing

  • node scripts/run-vitest.mjs run extensions/memory-core/src/memory/manager.async-search.test.ts — passed (6 tests)
  • pnpm exec oxlint extensions/memory-core/src/memory/manager-async-state.ts extensions/memory-core/src/memory/manager.ts extensions/memory-core/src/memory/manager.async-search.test.ts — 0 warnings/errors
  • pnpm exec oxfmt --check --threads=1 extensions/memory-core/src/memory/manager-async-state.ts extensions/memory-core/src/memory/manager.ts extensions/memory-core/src/memory/manager.async-search.test.ts — passed
  • git diff --check — passed
  • Diff secret scan — 0 suspicious added secrets

What Problem This Solves

Close-time memory manager failures were invisible: awaitPendingManagerWork used empty catch {} blocks for both pending sync and provider initialization. That made shutdown-time sync/provider failures indistinguishable from clean close and could hide data-loss or provider-init failures.

Evidence

  • Source verification on current main: extensions/memory-core/src/memory/manager-async-state.ts still had empty catch {} blocks for pendingSync and pendingProviderInit before this patch.
  • Regression tests now assert rejected pending sync and provider initialization promises call onError, while resolved promises do not.
  • Local validation passed:
    • node scripts/run-vitest.mjs run extensions/memory-core/src/memory/manager.async-search.test.ts → 1 file passed, 6 tests passed
    • pnpm exec oxlint ... → Found 0 warnings and 0 errors
    • pnpm exec oxfmt --check --threads=1 ... → All matched files use the correct format
    • git diff --check → clean
    • Diff secret scan → 0 suspicious added secrets

No live SQLite/disk-full shutdown fault was forced; the regression exercises the deterministic promise-rejection paths described by the issue.

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 7:53 AM ET / 11:53 UTC.

Summary
The branch adds an optional error callback to awaitPendingManagerWork, logs pending sync/provider-init failures during MemoryIndexManager.close(), and adds regression tests for rejected and resolved pending work.

PR surface: Source +14, Tests +36. Total +50 across 3 files.

Reproducibility: yes. at source level: current main's awaitPendingManagerWork has empty catches for both pendingSync and pendingProviderInit, and MemoryIndexManager.close() calls it for both paths. I did not run a live SQLite/provider shutdown failure in this read-only review.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: vector/embedding metadata: extensions/memory-core/src/memory/manager-async-state.ts, vector/embedding metadata: extensions/memory-core/src/memory/manager.async-search.test.ts, vector/embedding metadata: extensions/memory-core/src/memory/manager.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96766
Summary: This PR is the candidate fix for the open memory-core issue reporting silent close-time pending sync/provider-init error swallowing.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted real behavior proof, such as terminal output or logs showing memory close: pending manager work failed after inducing a pending sync or provider-init rejection in a real OpenClaw memory manager close path.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides unit-test, lint, format, and diff-check evidence only; before merge, the contributor should add redacted live output, logs, or a terminal transcript showing the close-time warning in a real manager shutdown path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merge-readiness proof gap: the PR body provides focused tests and local check output, but no redacted live close/log transcript showing the warning during a real manager shutdown path.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused logging helper change after the contributor adds redacted real shutdown or close-time log proof that the warning is emitted without changing close ordering.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No ClawSweeper repair job is needed because I found no concrete code defect; the remaining merge gate is contributor-supplied real behavior proof for the close-time logging path.

Security
Cleared: The diff touches only memory-core TypeScript logic and tests; it does not change dependencies, workflows, lockfiles, secrets, permissions, or external code execution surfaces.

Review details

Best possible solution:

Land the focused logging helper change after the contributor adds redacted real shutdown or close-time log proof that the warning is emitted without changing close ordering.

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

Yes at source level: current main's awaitPendingManagerWork has empty catches for both pendingSync and pendingProviderInit, and MemoryIndexManager.close() calls it for both paths. I did not run a live SQLite/provider shutdown failure in this read-only review.

Is this the best way to solve the issue?

Yes: adding an optional helper callback and wiring the close path to log with the existing formatErrorMessage helper is the narrow maintainable fix. Throwing these close-time errors would be a broader behavior change than the linked bug requires.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 0fc29969d960.

Label changes

Label changes:

  • add P2: This PR fixes a bounded memory-core reliability bug that can hide close-time sync/provider failures, with no evidence of a widespread active outage.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides unit-test, lint, format, and diff-check evidence only; before merge, the contributor should add redacted live output, logs, or a terminal transcript showing the close-time warning in a real manager shutdown path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This PR fixes a bounded memory-core reliability bug that can hide close-time sync/provider failures, with no evidence of a widespread active outage.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides unit-test, lint, format, and diff-check evidence only; before merge, the contributor should add redacted live output, logs, or a terminal transcript showing the close-time warning in a real manager shutdown path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +14, Tests +36. Total +50 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 18 4 +14
Tests 1 36 0 +36
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 54 4 +50

What I checked:

  • Repository policy applied: Root and extensions policy were read fully; the review applied the bundled-plugin boundary and deep PR-review guidance for this memory-core change. (AGENTS.md:1, 0fc29969d960)
  • Current main source reproduction: Current main still catches both rejected pending close-time promises with empty catch {} blocks in awaitPendingManagerWork. (extensions/memory-core/src/memory/manager-async-state.ts:19, 0fc29969d960)
  • Close path caller: MemoryIndexManager.close() snapshots pending provider initialization and uses awaitPendingManagerWork for both pending provider init and the current sync before database teardown. (extensions/memory-core/src/memory/manager.ts:1298, 0fc29969d960)
  • Adjacent error-reporting pattern: The nearby search-triggered async sync path already reports rejected sync work through an onError callback, matching the PR's chosen helper shape. (extensions/memory-core/src/memory/manager.ts:652, 0fc29969d960)
  • PR diff reviewed: The PR head adds onError handling to the helper, wires close-time logging with formatErrorMessage, and adds focused tests for rejected sync, rejected provider init, and successful pending work. (extensions/memory-core/src/memory/manager-async-state.ts:19, c17735563d9e)
  • Canonical linked issue: The linked open issue reports the same empty close-time catches and asks for visible logging or error reporting; its prior ClawSweeper review marked the bug source-reproducible and queueable.

Likely related people:

  • steipete: History shows awaitPendingManagerWork was extracted in Peter Steinberger's memory manager helper refactor and the current helper lines are carried by his recent main commit. (role: introduced helper and recent area contributor; confidence: high; commits: 3921bb2df672, 009b00f7bad1; files: extensions/memory-core/src/memory/manager-async-state.ts, extensions/memory-core/src/memory/manager.ts)
  • spacegeologist: Merged PR history shows Zee Zheng authored the recent memory close sync/provider lifecycle fix that introduced related close-ordering behavior around the same manager close path. (role: recent close lifecycle contributor; confidence: high; commits: a26fbba46e64, e9823023f4fc; files: extensions/memory-core/src/memory/manager.ts, extensions/memory-core/src/memory/index.test.ts)
  • lukeboyett: Merged PR history shows Luke Boyett worked on adjacent memory manager lifecycle and watcher cleanup code that overlaps shutdown behavior but not this exact diagnostic path. (role: adjacent memory lifecycle contributor; confidence: medium; commits: 9e43d0327fc8; files: extensions/memory-core/src/memory/manager.ts, extensions/memory-core/src/memory/manager-sync-ops.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 clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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 Jul 5, 2026
@steipete steipete self-assigned this Jul 5, 2026
This was referenced Jul 5, 2026
Closed
Closed
Merged
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Thank you — this fix landed through maintainer batch PR #100399 in commit b22c36f, with contributor credit and changelog thanks preserved. Closing this source PR as superseded.

@steipete steipete closed this Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

extensions: memory-core Extension: memory-core P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

awaitPendingManagerWork silently swallows all errors from pending sync/provider init on close

2 participants

@lin-hongkuan @steipete