Skip to content

fix(memory-core): skip forced sync for healthy zero-hit search#95595

Closed
Lestat569769 wants to merge 1 commit into
openclaw:mainfrom
Lestat569769:fix/memory-core-healthy-zero-hit
Closed

fix(memory-core): skip forced sync for healthy zero-hit search#95595
Lestat569769 wants to merge 1 commit into
openclaw:mainfrom
Lestat569769:fix/memory-core-healthy-zero-hit

Conversation

@Lestat569769

@Lestat569769 Lestat569769 commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown

What Problem This Solves

A valid memory search can return zero hits while the index is marked dirty even though the index is otherwise healthy. Before this fix, the tool wrapper treated that dirty-only zero-hit as a reason to call manager.sync({ reason: "search", force: true }) and wait inside the tool call. On hosts where the forced sync does not complete quickly, memory_search can hang until the tool timeout even though the correct user-facing answer is simply an empty result set.

This patch keeps the recall-preserving retry for non-QMD dirty indexes while preventing the QMD healthy dirty-only zero-hit path from forcing a blocking sync. Missing, invalid, empty, or unavailable index states still force sync.

Summary

  • skip blocking forced sync for healthy QMD zero-hit search results, including dirty-only QMD status
  • preserve dirty zero-hit forced-sync retry for non-QMD or unknown backends
  • keep forced sync for missing, invalid, empty, vector-unavailable, or FTS-unavailable memory states
  • treat disabled vector/FTS modes as healthy instead of unavailable
  • add regression coverage for QMD dirty healthy zero-hit no-sync and non-QMD dirty zero-hit retry behavior

Evidence

Focused local verification on branch head 925495a4:

git diff --check
node scripts/run-vitest.mjs extensions/memory-core/src/tools.test.ts

Result:

[test] starting test/vitest/vitest.extension-memory.config.ts

 RUN  v4.1.8 /home/lestat/openclaw-fresh/workspace/external/openclaw-source-preservation

 Test Files  1 passed (1)
      Tests  19 passed (19)
   Start at  20:29:09
   Duration  4.21s (transform 2.03s, setup 283ms, import 3.64s, tests 101ms, environment 0ms)

[test] passed 1 Vitest shard in 19.33s

Regression coverage now includes:

  • shouldForceSyncAfterZeroResults returns healthy_zero_hit for QMD dirty healthy zero-hit status
  • shouldForceSyncAfterZeroResults returns dirty_index for builtin or unknown dirty healthy zero-hit status
  • returns a healthy zero-hit without forcing sync when the index is ready asserts one search call and zero sync calls
  • preserves the dirty zero-hit retry for non-qmd backends asserts the wrapper syncs once, searches twice, and returns the second-search result
  • disabled vector/FTS modes with available: false do not make the index unhealthy

Live installed-runtime proof before source promotion:

  • openclaw memory status --json reported indexed and ready memory with files: 159, chunks: 2519, dirty: false, enabled/available FTS and vector, and custom.indexIdentity.status: valid
  • a synthetic valid no-hit returned 0 results in about 491.6ms with forcedSync.attempted=false and forcedSync.reason=healthy_zero_hit after gateway reload

Safety Boundaries

  • no model switch
  • no production reindex
  • no vector deletion
  • no memory config change
  • no secret/token output
  • no unrelated Pi-hole/plugin work

@openclaw-barnacle openclaw-barnacle Bot added extensions: memory-core Extension: memory-core size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 21, 2026
@clawsweeper

clawsweeper Bot commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 22, 2026, 12:45 AM ET / 04:45 UTC.

Summary
The PR adds a memory-core zero-hit status decision helper, skips forced sync/retry for healthy QMD zero-hit searches, preserves dirty non-QMD retry behavior, and adds focused regression tests.

PR surface: Source +51, Tests +96. Total +147 across 3 files.

Reproducibility: yes. Current main and v2026.6.9 still await forced sync after zero memory_search results, QMD sync awaits update maintenance, and the canonical issue includes live reports of 70s-class stalls; I did not rerun a live large-corpus QMD timing repro in this read-only review.

Review metrics: 1 noteworthy metric.

  • Zero-hit fallback policy: 1 retry gate changed. The PR changes when an empty memory_search result triggers forced sync/retry, which is compatibility-sensitive freshness behavior.

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

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #90023
Summary: This PR is a candidate fix for the canonical QMD zero-hit forced-sync stall; multiple open sibling PRs overlap the same root cause or adjacent timeout behavior.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🐚 platinum hermit
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Narrow the helper so clean built-in/non-QMD zero-hit searches still sync and retry.
  • Update the focused tests to cover QMD no-sync plus clean and dirty non-QMD retry behavior.

Risk before merge

  • [P2] Merging changes the existing zero-hit freshness fallback: healthy QMD indexes can now return an immediate empty result instead of doing search-time forced sync/retry.
  • [P1] Several open PRs target the same or adjacent memory_search/QMD stall area, so maintainers should choose one canonical landing path before merging overlapping branches.
  • [P1] Live PR status is currently unstable with failed checks reported on the head SHA, so merge should wait for rerun or log triage even though this review found no concrete patch defect.

Maintainer options:

  1. Accept the status-gated QMD skip
    Maintainers can merge this shape if they accept prompt empty results for healthy QMD zero-hit indexes instead of immediate search-time freshness repair.
  2. Use bounded or background refresh instead
    If immediate freshness repair must remain, change the QMD path to bound, debounce, or background the refresh outside the interactive tool wait.
  3. Pick one canonical overlapping PR
    Maintainers can pause or close duplicate branches once they choose between this PR, the narrower QMD-only PR, or the broader timeout/fallback PR.

Next step before merge

  • [P2] The remaining action is maintainer review of the fallback-policy tradeoff, overlapping PRs, and failed checks rather than a narrow automated repair.

Security
Cleared: No concrete security or supply-chain concern found; the diff is limited to memory-core TypeScript branching and colocated tests.

Review findings

  • [P1] Preserve clean non-QMD zero-hit retry — extensions/memory-core/src/tools.ts:116
Review details

Best possible solution:

Land one canonical memory-core fix that keeps QMD zero-hit update work out of the interactive memory_search path while explicitly preserving or accepting the desired freshness semantics, then reconcile the overlapping PRs.

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

Yes. Current main and v2026.6.9 still await forced sync after zero memory_search results, QMD sync awaits update maintenance, and the canonical issue includes live reports of 70s-class stalls; I did not rerun a live large-corpus QMD timing repro in this read-only review.

Is this the best way to solve the issue?

Yes, this is an acceptable narrow mitigation after the latest narrowing because it stays in the memory-core tool path and preserves dirty non-QMD retry. The best merge state still requires maintainer acceptance of the latency-versus-freshness policy, or a bounded/background refresh variant if immediate freshness must stay.

Full review comments:

  • [P1] Preserve clean non-QMD zero-hit retry — extensions/memory-core/src/tools.ts:116
    This helper now returns healthy_zero_hit for any otherwise healthy status, including built-in/non-QMD indexes, so those zero-hit searches skip the current final sync-and-retry path. The canonical report is QMD-specific; keep non-QMD retry behavior unless maintainers explicitly accept removing that freshness fallback.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR targets a concrete memory-core availability bug with a narrow affected path and normal maintainer-review blast radius.
  • merge-risk: 🚨 compatibility: The diff changes existing zero-hit forced-sync fallback behavior for current users with healthy QMD memory indexes.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes focused test output plus after-fix installed-runtime status and zero-hit result observations for the nonvisual memory_search path.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes focused test output plus after-fix installed-runtime status and zero-hit result observations for the nonvisual memory_search path.
Evidence reviewed

PR surface:

Source +51, Tests +96. Total +147 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 58 7 +51
Tests 1 100 4 +96
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 158 11 +147

What I checked:

Likely related people:

  • TurboTheTurtle: Authored merged PR 92639, which routed memory_search through transient QMD manager mode and touched the same tool/QMD lifecycle boundary. (role: recent memory_search and QMD lifecycle contributor; confidence: high; commits: ddacb7ba39d4; files: extensions/memory-core/src/tools.ts, extensions/memory-core/src/memory/qmd-manager.ts)
  • dreamhunter2333: Authored merged PR 91742, which changed the memory_search deadline and AbortSignal path adjacent to this zero-hit stall behavior. (role: recent memory_search timeout contributor; confidence: medium; commits: 8d72cb9401e5; files: extensions/memory-core/src/tools.ts, extensions/memory-core/src/tools.test.ts)
  • osolmaz: Authored merged PR 87907 for memory index identity validation, which is adjacent to the status/readiness checks this PR now uses. (role: adjacent memory index contributor; confidence: medium; commits: a4b4fed41287; files: extensions/memory-core/src/tools.ts, extensions/memory-core/src/memory/manager.ts, extensions/memory-core/src/memory/manager-sync-ops.ts)
  • Vincent Koc: Local blame and recent history carry the current memory_search/QMD snapshot through this area, including the existing zero-hit forced-sync block. (role: recent area contributor; confidence: medium; commits: 095a44c8de4d, c645ec4555c0; files: extensions/memory-core/src/tools.ts, extensions/memory-core/src/memory/qmd-manager.ts)
  • Peter Steinberger: Recent history shows QMD startup and memory provider refactors in the same backend/config surface that affects memory_search behavior. (role: adjacent QMD and memory configuration contributor; confidence: low; commits: 645c7b189757, 77e6e4cf87f7; files: extensions/memory-core/src/memory/qmd-manager.ts, packages/memory-host-sdk/src/host/backend-config.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.

@Lestat569769
Lestat569769 force-pushed the fix/memory-core-healthy-zero-hit branch from c8d0051 to bf0eb56 Compare June 21, 2026 17:03
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 21, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Merged
@Lestat569769
Lestat569769 force-pushed the fix/memory-core-healthy-zero-hit branch from bf0eb56 to 143de2d Compare June 21, 2026 23:56
@Lestat569769

Lestat569769 commented Jun 21, 2026

Copy link
Copy Markdown
Author

Updated the PR for the disabled-vector/FTS readiness finding and added inspectable focused test output plus runtime/status proof in the PR body. @clawsweeper re-review

@Lestat569769
Lestat569769 force-pushed the fix/memory-core-healthy-zero-hit branch from 143de2d to 925495a Compare June 22, 2026 00:27
@Lestat569769

Lestat569769 commented Jun 22, 2026

Copy link
Copy Markdown
Author

Patch update for ClawSweeper review:

  • Force-pushed branch fix/memory-core-healthy-zero-hit to 925495a4.
  • Narrowed the zero-hit no-sync path so dirty healthy zero-hit skips forced sync only for backend === "qmd", preserving dirty zero-hit retry for non-QMD/unknown backends.
  • Disabled vector/FTS modes still do not count as unhealthy.
  • Added regression coverage proving non-QMD dirty zero-hit still syncs and retries.

Focused verification:

git diff --check
node scripts/run-vitest.mjs extensions/memory-core/src/tools.test.ts

Result: 1 file passed, 19 tests passed.

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 22, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 22, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 22, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 22, 2026
@vincentkoc

vincentkoc commented Jul 1, 2026

Copy link
Copy Markdown
Member

Thanks for working on this. The zero-hit memory_search sync issue is now covered by the narrower canonical fix in #90030, which landed as 90e31be and closed #90023. Closing this PR as superseded so we keep the queue on the canonical implementation.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: memory-core Extension: memory-core merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Lestat569769 @vincentkoc