fix(memory-core): bound zero-hit forced sync so memory_search can't hang the whole deadline#94564
fix(memory-core): bound zero-hit forced sync so memory_search can't hang the whole deadline#94564esqandil wants to merge 3 commits into
Conversation
…ang the whole deadline
memory_search runs a one-shot forced index refresh when a query returns
zero hits against a populated index:
if (rawResults.length === 0 && activeMemory.manager.sync) {
await activeMemory.manager.sync({ reason: "search", force: true });
...
}
`manager.sync({ force: true })` does not accept an AbortSignal, so this
await is unbounded. Against a large or degraded store the forced refresh
can run for many seconds; the call then blocks all the way to the hard
15s `memory_search` deadline and latches the 60s provider-error cooldown
— even when the store is healthy and merely slow. The wrapper's
deadlineSignal was only threaded into `manager.search()`, never into this
sync, so the abort could not cancel the hang. Observed live as
`memory_search timed out after 15s` for corpus=memory/sessions/all while
`openclaw memory status --deep` reported the store fully healthy.
The forced sync is also redundant for correctness: `manager.search()`
already (a) performs a synchronous cold-start bootstrap sync on an empty
index and (b) schedules an async background freshness sync per call. The
tool-level forced sync is only an opportunistic staleness retry, so it
should never be allowed to consume the entire interactive budget.
Fix:
- Bound the forced sync by `deadline - safety-margin` via
waitForForcedSyncWithinBudget(). If it settles in time we run the retry
search as before (healthy path unchanged). If it outruns the budget we
abandon the wait (the sync keeps running in the background, deduped by
the manager) and return current results as a normal, available response
instead of timing out and tripping the cooldown.
- Thread timeoutMs/startedAt through runMemorySearchToolWithDeadline so
the budget is computed from the real remaining deadline.
- Surface an honest timeout message: a deadline timeout no longer reports
as "embedding/provider error" (which misdirects diagnosis); it now says
the search timed out before the backend responded and points at latency
/ `openclaw memory status --deep`.
Tests:
- New: zero-hit forced sync that outruns the deadline returns an
available empty result, runs the initial search only, and does NOT
latch the cooldown (next call still queries memory).
- New: timeout surfaces the honest message/action.
- Updated existing timeout assertions to the new message.
- Added setMemorySyncImpl to the tool manager mock.
Full extension-memory suite green (1259 tests); tsgo:extensions and
oxlint clean.
Refs: #4
openclaw-barnacle
Bot
added
extensions: memory-core
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 4:25 PM ET / 20:25 UTC. Summary PR surface: Source +132, Tests +55, Other +1. Total +188 across 60 files. Reproducibility: yes. at source level: current main still synchronously awaits a zero-hit forced sync, and #90023 includes live QMD reports of the same stall. I did not run a live large-corpus QMD repro in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a narrow memory-core-only fix that bounds or avoids zero-hit forced sync, preserves one-shot cleanup behavior, and includes redacted after-fix real behavior proof from a patched setup. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main still synchronously awaits a zero-hit forced sync, and #90023 includes live QMD reports of the same stall. I did not run a live large-corpus QMD repro in this read-only review. Is this the best way to solve the issue? No, not yet: bounding the forced sync is plausible, but this branch should be narrowed, refreshed, and proven in a patched real setup before it is the best merge path. Maintainers also need to choose this generic fail-open behavior versus a narrower QMD-specific fix. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2609b9722280. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +132, Tests +55, Other +1. Total +188 across 60 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
…nded-forced-sync # Conflicts: # extensions/memory-core/src/tools.test.ts
The bounded zero-hit forced sync prevents a slow/stuck sync from hanging the interactive memory_search deadline for long-lived (gateway) managers. But one-shot CLI runs (oneShotCliRun -> purpose=cli) tear the manager down in the tool's finally block, and close() awaits any in-flight sync UNBOUNDED (manager.ts awaitCurrentSync / qmd-manager pendingUpdate). That relocated the exact hang the bound prevents into cleanup. Skip the optional forced refresh entirely for purpose=cli: a one-shot run has no 'next call' to benefit from a warmed index, and search() already force-syncs a cold index inline + schedules a background sync, so nothing is lost. Addresses the ClawSweeper P1 on PR openclaw#94564. Adds a regression test: a one-shot run with a never-settling sync returns an available empty result, never starts the sync, and closes once.
openclaw-barnacle
Bot
added
channel: line
openclaw-barnacle
Bot
added
extensions: deepinfra
extensions: chutes
extensions: diffs
extensions: openrouter
extensions: copilot
size: L
triage: needs-pr-context
clawsweeper
Bot
added
the
merge-risk: 🚨 other
|
Merge-ready status (maintainer summary) Scope of this PR is exactly 5 files, all under
This branch is already merged up to |
clawsweeper
Bot
added
status: 🛠️ actively grinding
|
@esqandil thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward. Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive. Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it. |
|
Thanks for the work here. The zero-hit memory_search stall is now fixed by the narrower canonical PR #90030, which landed as 90e31be and closed #90023. Closing this PR as superseded; this branch also carries a broad 60-file surface, so any remaining independent work should come back as a much narrower PR. |
2 participants
Summary
memory_searchcan hang for its entire 15s deadline — and then latch a 60s cooldown — on a query that returns zero hits against a populated index, even when the store is perfectly healthy and merely slow.The zero-hit path runs a one-shot forced index refresh:
manager.sync({ force: true })takes noAbortSignal, so thisawaitis unbounded. The deadline wrapper threads itsdeadlineSignalintomanager.search()but not into thissync(), so when the deadline fires the forced sync keeps running and the tool blocks to the hard timeout. The failure then surfaces as a misleading "Memory search is unavailable due to an embedding/provider error" and tripsMEMORY_SEARCH_TOOL_COOLDOWN_MS(60s), so subsequent calls fast-fail too.Observed live:
memory_searchreturnedtimed out after 15sforcorpus=memory/sessions/allwhileopenclaw memory status --deepreported the same store as fully healthy (indexIdentity: valid, embeddings + vectors ready). The tool-vs-CLI disagreement is the tell — the hang is in the in-process tool path, not the backend.The forced sync is also redundant for correctness:
manager.search()already (a) does a synchronous cold-start bootstrap sync on an empty index, and (b) schedules an async background freshness sync per call. The tool-level forced sync is only an opportunistic staleness retry — it should never be able to consume the whole interactive budget.Fix
deadline − safety-margin(waitForForcedSyncWithinBudget). If it settles in time, the retry search runs exactly as before (healthy path unchanged). If it outruns the budget, abandon the wait — the sync keeps running in the background (deduped by the manager viathis.syncing) and the next call's background sync catches the index up — and return the current results as a normal, available response instead of timing out and latching the cooldown.timeoutMs/startedAtthroughrunMemorySearchToolWithDeadlineso the budget is computed from the real remaining deadline.openclaw memory status --deep.Behavior matrix
Tests
tools.test.ts): a zero-hit forced sync that outruns the deadline returns an available empty result, runs the initial search only (no retry), and does not latch the cooldown (next call still queries memory).tools.test.ts/tools.citations.test.tsto the new message (genuine provider-error assertions left unchanged).setMemorySyncImplto the memory tool-manager mock.Full
extension-memorysuite green (1259 tests);tsgo:extensionsandoxlintclean.Related
Notes
Scoped intentionally to the interactive search hot path. Making the 15s timeout itself configurable (the other half of #4) is left out to keep this change reviewable; happy to follow up.
What Problem This Solves
memory_searchblocks for its full 15s deadline and then latches a 60s cooldown on a zero-hit query against a healthy-but-slow index, because the zero-hit forced refreshawait activeMemory.manager.sync({ reason: "search", force: true })takes noAbortSignaland is awaited unbounded. The failure is then mislabeled an "embedding/provider error" (it is not). This PR bounds that wait so a slow sync can never consume the interactive budget, and tells the truth in the timeout message. The follow-up commit also closes the one-shot-CLI variant whereclose()would re-block on the same sync during cleanup.Evidence
Setup. Long-lived gateway daemon (
node …/dist/index.js gateway --port), builtin memory backend (OpenAItext-embedding-3-small, 1536-dim, per-agent sqlite). This is the deployment that surfaced the bug; the gateway path never setsoneShotCliRun, somemoryManagersToCloseis empty and the bounded-sync fix applies directly.Before (observed live on the unpatched build). A zero-hit
memory_searchagainst a healthy-but-large store latched the failure. The tell is that the CLI path reports the same store healthy at the same moment (different code path), so the hang is in-process, not in the backend:Measured backend latencies in that window (local embed/rerank sidecars + OpenAI direct) were all sub-second — the 15s was spent blocked in the unbounded
await manager.sync(...), not in the provider. (This is also why the old "embedding/provider error" string sent at least one investigation down the wrong path — see #4.)After (deterministic reproduction of the race, on this branch). The live hang is a timing race, so the durable proof is a hermetic test that forces the worst case — a forced sync that never settles — and asserts the patched tool returns promptly as an available empty result, runs the initial search only, and does not latch the cooldown. Run on the built branch:
Full
extension-memorysuite is green on the branch (tools.test.ts20,tools.citations.test.ts20). I have not run a patched gateway binary in production — the after-fix evidence here is the hermetic test that reproduces the exact before/after the live incident showed, not a re-observed live run.Update — rebased onto
main+ one-shot cleanup fixmaininto the branch (no history rewrite / no force-push). The two previously-red checks —check-lint(src/gateway/server-methods/update.ts:299) andchecks-node-agentic-gateway-methods(update.test.ts:813) — were stale-base failures in files this PR does not touch, both already fixed onmain; the merge clears them.oneShotCliRun(purpose=cli) managers the optional zero-hit forced sync is now skipped entirely: those managers are torn down in the tool'sfinally, andclose()awaits any in-flight sync unbounded (manager.tsawaitCurrentSync;qmd-managerpendingUpdate/queuedForcedUpdate), which would relocate the exact hang into cleanup. A one-shot run has no "next call" to benefit from a warmed index, andsearch()already bootstrap-syncs a cold index inline + schedules a background sync, so correctness is unchanged. New regression test added.