1 Commits

Author	SHA1	Message	Date
Nan Gao	5ae953fc2f	fix(memory): keep the retrieval index Pod-local and refresh scopes written by peers (#6494) ... * fix(memory): keep the retrieval index Pod-local and refresh scopes written by peers DeerMem's derived SQLite FTS5 index for every user was one WAL database pinned to {storage_path}/.retrieval. With several Gateway Pods sharing one home volume each Pod opened that file over a network filesystem (unsupported by SQLite), every Pod start emptied and refilled it under its peers, one Pod's corruption recovery deleted files the others held open, and a Pod never learned that a peer had written a user's facts, so memory_search served stale results. - New backend-private key memory.backend_config.retrieval_index_path: empty keeps {storage_path}/.retrieval, a relative path resolves against storage_path, an absolute path is used as-is. The full rebuild and the corruption recovery act only on that configured index. - rebuild_index records each agent scope's manifest signature (mtime_ns, size, revision) before reading its facts; search_facts rebuilds a scope whose live signature differs because another process wrote the user's memory. An own commit advances the recorded signatures of that user's scopes only when they were in sync at the previous revision, so own writes never rebuild while an interleaved peer write still does. - Gateway startup warns, in the style of _validate_agent_storage, when a declared multi-instance deployment keeps the DeerMem index inside storage_path. - config.example.yaml documents the key; config_version 55 -> 56, mirrored in the Helm chart and its README. - Helm mounts an emptyDir at /var/lib/deerflow/memory-index, points the key at it, drops the legacy memory.storage_path: memory.json line, and documents the memory layout next to the other multi-replica prerequisites. * docs(changelog): link the memory retrieval index entry to #6494 * fix(memory): never advance the index signature on a no-op commit and resync whole scopes Two review findings on the cross-replica retrieval re-sync: - save() and apply_changes() inferred the replaced manifest revision as "returned revision - 1". The commit helper returns the existing manifest unchanged for a no-op (unchanged-value or empty change set), so after a peer bumped the manifest an own no-op write advanced this process's stale index signature to the peer's revision without carrying the peer's delta: a fact the peer deleted stayed searchable until the manifest changed again. Both paths now read the real pre-commit revision under the user lock and pass committed=(pre_revision, signature) only when the commit produced a new revision; a no-op passes None so the next search still rebuilds the scope. - The scoped rebuild (signature mismatch and the pre-existing dirty-scope path) listed facts through list_facts() with its default limit of 100 and then replaced every row of the scope, so a scope with more than 100 facts lost its tail on the first re-sync. It now reads the scope's complete fact list. * fix(memory): fence index signature promotion against a concurrent scope rebuild Review finding on the cross-replica re-sync: a successful adapter notification does not prove its delta is still in the index when the signature is promoted. The SQLite lock covers single adapter calls only, and neither the scope lock nor DeerMem's warm-up lock spans "notify, then promote", so rebuild_index can replace a scope's rows in between. After a peer summary-only commit (r+1), A's search starts a scoped refresh that captures the r+1 signature and reads f1/f2; A's own deletion of f2 commits r+2 and removes the row; the refresh then installs its stale f1/f2 and marks the scope synced at r+1; the deletion resumes and promoted that signature to r+2, so later searches saw the live signature as equal and kept returning the resurrected f2 until another manifest change. Mechanism: a generation fence rather than a lock. rebuild_index bumps a per-scope generation (scoped rebuild) or a storage-wide generation (full rebuild) under _cache_lock whenever it publishes or forgets a scope's rows, in both mark_synced and forget_synced. _dispatch_retrieval_notifications snapshots the generations under _cache_lock before its first adapter call and, at promotion time, skips every scope whose generation moved (and everything when the storage-wide generation moved), leaving the rebuild's own captured signature in place; that signature predates the commit, so the next search's comparison rebuilds the scope. A lock would have had to serialize notification plus promotion with bulk install plus publication while search_facts -> rebuild_index -> load() already takes the scope, file and cache locks; the fence adds no lock-order obligations and only costs a redundant rebuild in the rare race. Every interleaving is covered: a rebuild that completes before the snapshot is reflected in the captured generation and promotion proceeds on an index that carries both the rebuild's rows and this write's delta; one that publishes between snapshot and promotion is skipped; one that publishes after the promotion records its stale captured signature, which the next search catches. Tests reproduce both orders deterministically with threading.Event ordering (adapter wrapper that holds the bulk install and the remove notification): test_promotion_is_fenced_against_a_refresh_that_installs_in_between (red before the fix) and test_stale_refresh_installing_after_a_promotion_is_caught_by_the_next_search (regression guard for the mirrored order). * fix(memory): publish a rebuild's signature under the lock that replaced its rows Review finding on the cross-replica re-sync: two peer-triggered refreshes of one scope could replace rows in one order and publish their captured signatures in the other. On a warmed Pod A, refresh R1 captures r+1 with f1/f2 and waits before installing; a peer deletes f2 at r+2; refresh R2 captures r+2, installs f1 only and pauses after bulk_rebuild returns, before mark_synced; R1 installs its stale f1/f2 and publishes r+1; R2 then publishes r+2 over R1's rows. Later searches saw the live signature as equal and kept returning the deleted fact. The generation fence only guards own-write promotion, and the adapter's SQLite lock ends when the install returns. Mechanism: a per-storage _retrieval_publish_lock held across the row replacement and the signature publication, for the bulk path (bulk_rebuild + mark_synced / forget_synced) and the clear+upsert fallback, scoped and full alike. Overlapping refreshes of a scope therefore serialize their install+publish pairs: whichever installs last also publishes last, and a stale captured signature published last differs from the live manifest, so the next search's compare rebuilds the scope. Fact reading (load() / the glob) stays outside the lock so a slow read never blocks refreshes of other scopes. The startup full rebuild holds the lock for its single bulk install; scoped refreshes wait for it, which is acceptable because before warm-up completes a search would be rebuilding its scope anyway (DeerMem.warm_retrieval takes its own _retrieval_lock only after rebuild() returns, so the two never nest). Lock order: the publish lock is acquired with no storage lock held -- search_facts releases _cache_lock before calling rebuild_index, reload() leaves its scope/file-lock block before rebuilding, and DeerMem._ensure_retrieval_scopes holds only its outer RLock, which nothing inside rebuild_index acquires. Inside the lock only the adapter's own lock and _cache_lock are taken, and both are leaf locks that never call back into storage, so no cycle is possible. Test: test_rebuild_publication_stays_ordered_with_row_replacement drives the reviewer's interleaving with threading.Event ordering (red before the fix). The pause harness now holds a refresh after its fact read, outside the publish lock, instead of inside the adapter's rebuild, so the earlier fence tests keep their semantics without holding the new lock while paused. * fix(memory): read each manifest once per full rebuild and serve the local index on a compare read error Two suggestion-level review items on the cross-replica retrieval re-sync. - The full rebuild cached manifest signatures keyed by (manifest path, agent name), but _scope_signature reads only the manifest file -- every agent bucket of a user shares one memory.json -- so the startup scan stat+read+ parsed the same file once per bucket that has facts. The cache is now keyed by the manifest path alone while synced signatures are still recorded per (user, agent) scope: users manifest reads instead of users x agents. - The per-search freshness compare put a shared-volume manifest read on every search_facts call, and _load_memory_file raises MemoryStorageCorruption on OSError and malformed JSON, so a transient network-filesystem read error or a corrupt memory.json failed a search the Pod-local index could still answer. The compare now catches OSError, ValueError and MemoryStorageCorruption: it logs a warning naming the scope (traceback at debug), serves the current local index for that scope and leaves the recorded synced signature untouched so the next search compares again. It deliberately does not mark the scope dirty: a scoped rebuild would hit the same failing read, forget the scope's signature and fail the search path too. * docs(changelog): resolve merge markers left in the Unreleased section * fix(memory): forget a scope's index signature when an own delta cannot be proved compatible Review finding on the cross-replica re-sync: skipping promotion left a newer rebuild's signature trusted even after this dispatcher had overwritten its rows with an older notification. On a warmed Pod A, an own PATCH of f2 commits r+1 and pauses after releasing the user lock, before its adapter upsert; Pod B deletes f2 at r+2; a concurrent search on A detects the peer change, rebuilds the scope without f2 and publishes r+2; the pending r+1 upsert then reinserts the deleted fact, and the revision check merely skipped because the synced revision was r+2 rather than the PATCH's previous r. The signature stayed equal to the live manifest, so searches returned the deleted fact indefinitely. The generation fence could not help: it only suppresses promotion, the stale adapter mutation still happens. Rule: after an own commit's notifications are applied, the mutated scope's recorded signature may stay only when the delta is provably compatible with the published snapshot -- the scope is synced at exactly previous_revision and no rebuild published rows for it (or storage-wide) since the fence snapshot -- in which case it is promoted as before. In every other case where this dispatcher mutated the adapter for the scope (synced at another revision, newer or older; generation moved; not synced at all; or notifications without a committed revision, i.e. migration and clear paths) the scope's signature is forgotten so the next search's compare rebuilds it. The user's other scopes were not mutated by this delta: they are promoted when provable and otherwise left untouched. The decision runs under _cache_lock together with the generation reads, so a rebuild publishing concurrently is either observed or observes the forget; the failure path (dirty + forget) is unchanged. Orders covered: the reviewer's (publish r+2, then the stale upsert) forgets and the next search rebuilds without f2; the earlier fence order (publish between adapter op and promotion) now forgets too, one redundant rebuild; no concurrent rebuild promotes exactly as before. Test: test_stale_own_delta_replayed_over_a_newer_snapshot_forgets_the_scope holds the PATCH's adapter upsert after the user lock is released (new pause_next_upsert hook on the test adapter), Event-ordered, red before the fix. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>	2026-10-08 21:20:45 +08:00