fix: durable memory writes no longer exit 1 when post-write embed/anchor fails#399
fix: durable memory writes no longer exit 1 when post-write embed/anchor fails#399gabaum10 wants to merge 5 commits into
Conversation
coryzibell
left a comment
coryzibell
left a comment
There was a problem hiding this comment.
VERDICT: REQUEST CHANGES
The core change is right — durable-then-best-effort is the correct shape, and genuine write failures still exit non-zero (verified: upsert/read-back still use ?). Durability is sound: add_one read-back-verifies and bail!s before any side effect runs (src/handlers/memory.rs:446,62). But two things go back down the chute: partial-success is invisible to the consumers that matter, and half the change (anchor) is entirely untested.
Findings (ranked)
Critical
1. Partial-success is invisible to --json and exit-code consumers. src/handlers/memory.rs:479, 1126, 1815, 1898, 1992, 2082, 2212 (embed) and the anchor twins. On failure the entry lands durable-but-unsearchable (no embedding → absent from --semantic until a manual embed --all), and the only signal is a stderr eprintln!. Exit code is 0 — byte-identical to full success. Under --json, the payload gets no embed_deferred/embedding_failed field, so a structured caller (hearth/companions consume --json) reads clean success JSON and cannot detect the degraded state. The question "surfaced or fully silent" answers: surfaced to a human on stderr, silent to every machine consumer. Fix: add a structured field to the --json payload (e.g. "embed_deferred": true) so the degradation is programmatically detectable. This is the main lever.
2. The anchor-failure path is entirely untested. tests/write_side_effect_nonfatal.rs. All three failure tests use mx_with_broken_model_cache, which breaks embed. But auto_embed failing means the entry has no embedding, and auto_anchor early-returns Ok(()) when entry.embedding.is_none() (src/helpers.rs:405-407). So the anchor error branch — 6 of the 12 changed call sites — never executes in any test. The PR claims symmetric Add/Update/Edit/Append/Prepend/Restore anchor coverage; zero anchor failures are actually forced. Add a test that forces auto_anchor to fail on an already-embedded entry.
Warnings
3. 4 of 6 single-entry sites have no non-fatal embed test. Only Add (:479) and Append (:1992) are exercised. Update (:1798), Edit (:1898), Prepend (:2082), Restore (:2212) rely on "same code shape." Same shape, yes — but the commit message asserts all six, and untested is untested.
4. Merge/interaction conflict with PR #402 is near-certain. Both PRs edit add_one, the Add --json payload construction, and the add-batch embed block (#399 rewrites the hoisted embed pass at :2700; #402 adds a pre-write dedup gate to the same funnels). Semantically complementary — #402 dedups before the write, #399 softens side effects after — but the textual conflict in add_one and the batch block needs hand-reconciliation. More important: the merged --json schema needs joint review. #402 adds skipped/duplicate_of/status/dedup_lookup_warning/dedup: bypassed_no_session; #399 adds none for its degraded path (finding 1). See the Merge order section below.
5. Reconcile is real but manual and unsignalled in the DB. Confirmed mx memory embed --all re-embeds via list_all (:2690-2734), so a missed entry will be picked up — the PR comment's claim is accurate. But nothing triggers it automatically, it re-embeds the entire graph (not just the gaps), and there's no persistent "needs-embed" marker. A backgrounded caller that discards stderr leaves the entry silently unsearchable until a human remembers. Finding 1's structured field would also give a future targeted reconcile something to key on.
Suggestions
6. The let _ = expr.map_err(|e| eprintln!(...)) idiom reads oddly (all 12 sites). Using map_err for a side effect and then discarding a Result<(),()> obscures intent. if let Err(e) = expr { eprintln!(...) } says exactly what it does. Readability — the code is read a hundred times.
7. The warning omits the entry id. Every site prints "post-write embed failed (entry durable): {e}" with no kn- id. For the "reconcile later" story to be actionable, a human needs to know which entry. Add &id.
8. Does not touch #398, correctly out of scope — but note the direction. The diff wraps the existing auto_anchor(&id, db.as_ref(), removed)? (:1817) without altering anchor-merge plumbing, so #398 (--add-anchor clobbered by the auto-anchor pass) is neither fixed nor regressed. Caveat: making the anchor pass non-fatal makes an anchor-pass failure quieter, mildly at odds with #398's "at minimum fail loudly" ask on that path. Worth a line in the PR body.
9. Tests are network- and environment-dependent (self-documented): they require huggingface.co reachability, and trigger_check.rs couldn't run locally (SurrealDB IAM). CI must actually exercise both.
Merge order
Agreed cross-PR plan for the three concurrent memory PRs, to prevent conflicts:
- Recommended landing order: #401 → #402 → #399 (this PR lands last).
- Rationale: #402 changes
add_one's signature and return type (Result<KnowledgeEntry>→Result<WriteOutcome>) and addsskipped/duplicate_of/statusto the--jsonpayload. This PR should rebase on top of #402 and add its structured degraded-state field (finding 1, e.g.embed_deferred) into that already-richer JSON schema, so the JSON contract is designed once, coherently. - Semantics compose correctly: #402's dedup-skip early-return means no post-write side effects run on a skip — which is the right behavior. #402 dedups before the write; #399 softens side effects after. No semantic conflict, only textual — resolved by landing in this order.
The nut goes back to the examination table. Fix 1 and 2, fold the JSON field into #402's schema after rebase, and it earns the chute to the left.
…#402 review F1) BLOCKER: dedup_hash keyed on title+body only, and get_entries_for_session scoped candidates by session+owner but not category. Identical title+body re-filed under a different category in the same session silently collided -- the second write was skipped as "already saved" pointing at a row in the wrong category (undocumented false-positive data loss, the exact class the ticket named as worst-case). Fix scopes the candidate query by category instead of folding a third field into dedup_hash (avoids the LANDMINE ambiguity the hash's doc-comment already warns about): get_entries_for_session/DedupIndex/dedup_gate now thread category through, and DedupIndex's group/seen keys include category so a same-hash entry from a different category's fetch can never shadow another category's check. All three write funnels (add_one, single-add --type, batch inline --type) now pass category through the shared gate. Tags are deliberately excluded from the identity (edge-modeled, not a scalar row field; folding them in costs a query per candidate) -- documented in dedup_hash's new IDENTITY CONTRACT note and pinned by identical_content_different_tags_still_dedups. New/updated tests: identical_content_different_category_does_not_dedup, identical_content_different_tags_still_dedups, test_get_entries_for_session_category_scope_excludes_other_categories, plus the existing get_entries_for_session/DedupIndex test suites updated for the new category parameter. docs/src/memory.typ coverage-boundary note updated to describe the category scope and the tags exclusion, and the deferred UNIQUE index note updated to key on (session, owner, category, dedup_hash). Deferred (not in this PR, per review triage lanes 2-5): TOCTOU-open in-process gate (garbaggio UNIQUE index option), unbounded get_entries_for_session candidate fetch, and the #399/#401 merge-order plan -- all acknowledged/comment-lane findings, not blockers.
3ad0e98 to
df0cb71
Compare
Re-review (Verdictia Stern) — force-pushed branch @ df0cb71VERDICT: REQUEST CHANGES — both prior blockers remain open. Confirmed by the author's own note: "Review-feedback commits (embed_deferred field + anchor-failure test) coming in a follow-up round." The rebase landed; the fixes did not. Blocker 1 — Partial success invisible to
|
mx stores content_hash but never enforces it, so regenerated/recased
duplicates (same meaning, different case/punctuation/whitespace) land as
separate rows. Add a normalized dedup_hash (title+body) checked at the write
boundary, against the writer's own same-session entries, via a shared
pre-upsert helper covering BOTH write funnels:
- add_one (single `memory add` + the AddBatch standard-path delegate)
- the AddBatch fact-type inline path ({type, content} lines), which builds
its entry and calls db.upsert_knowledge directly -- never touches add_one,
so it needed its own gate wired to the same DedupIndex.
On a same-owner, same-session hit the write is skipped and reported as
already persisted (exit 0, not a failure). `--allow-duplicate` (single add)
/ `allow_duplicate` (per JSONL line) forces the write through.
Candidate fetch is a new KnowledgeStore::get_entries_for_session, field-scoped
on the indexed `session` record-link (NOT the EXTRACTED_FROM edge, which is
frequently missing), hard owner-scoped in application SQL (`owner = $owner`
or `owner IS NONE` for unowned public entries -- never delegated to the
public-permissive visibility filter), and projecting only id/title/body
(never the 768-dim embedding). AgentContext is derived from the entry's own
owner, matching the existing write_verification_ctx pattern.
The in-process DedupIndex fetches lazily, one query per distinct
(session_id, owner) group actually seen in a run -- exactly one query for
the common single-session single-owner batch, and correct (not silently
mis-scoped) for batches spanning multiple sessions/owners, without querying
once per entry.
session_id=None bypasses dedup entirely (by design) and surfaces an
observable bypass signal (`"dedup": "bypassed_no_session"` in --json, a
stderr note in plain mode) rather than failing silently. TOCTOU: this is
in-process, best-effort duplicate prevention, not a DB-level uniqueness
guarantee -- documented as such, not over-claimed.
Output contract treats a skip as success, never failure: affirmative
"Already saved" language (never bare "skipped"/"duplicate"), a positional
per-entry batch line carrying the duplicate_of kn-id (so positional
mark-back stays aligned), and its own "K already saved" tally in the batch
summary, separate from added/failed.
Docs: memory.typ gains a "Write-boundary deduplication" section (behavior
contract, coverage boundary, --allow-duplicate), architecture.typ
distinguishes the legacy content_hash from the new dedup_hash, CLAUDE.md's
maintenance rule now points at PR bodies (no CHANGELOG file exists in this
repo).
Tests: real-store (SurrealDB in-memory) unit tests for get_entries_for_session
scoping (owner exclusion, None-owner, session-field scoping, no embedding
leak), a real-store counting-spy proving one query per distinct group not
per entry, and CLI-level integration tests (tests/dedup_write_boundary.rs)
covering both write funnels' idempotency, the evolved-re-add case,
--allow-duplicate, the session_id=None bypass, and the json skip contract.
No schema change (knowledge_session and knowledge_owner indexes already
exist). Legacy content_hash (title-only, import-time) is untouched.
Note for review: sibling PR #401 (feat/exclude-tags-search-list) is open
against this repo and also touches src/handlers/memory.rs and
src/surreal_db/knowledge.rs -- landing order is Cory's call. This PR does
not touch the areas #401 changes (search/list filtering) as far as this
diff shows, but a rebase may still be needed depending on landing order.
Also note: PR #399 (fix/non-fatal-post-write-side-effects) touches the same
add_one/AddBatch functions; its WriteOutcome-adjacent changes (embed/anchor
skip notices) are pre-existing and out of scope here, but whoever merges
second should expect to reconcile call sites.
b9d6d0c to
024b1ea
Compare
…#402 review F1) BLOCKER: dedup_hash keyed on title+body only, and get_entries_for_session scoped candidates by session+owner but not category. Identical title+body re-filed under a different category in the same session silently collided -- the second write was skipped as "already saved" pointing at a row in the wrong category (undocumented false-positive data loss, the exact class the ticket named as worst-case). Fix scopes the candidate query by category instead of folding a third field into dedup_hash (avoids the LANDMINE ambiguity the hash's doc-comment already warns about): get_entries_for_session/DedupIndex/dedup_gate now thread category through, and DedupIndex's group/seen keys include category so a same-hash entry from a different category's fetch can never shadow another category's check. All three write funnels (add_one, single-add --type, batch inline --type) now pass category through the shared gate. Tags are deliberately excluded from the identity (edge-modeled, not a scalar row field; folding them in costs a query per candidate) -- documented in dedup_hash's new IDENTITY CONTRACT note and pinned by identical_content_different_tags_still_dedups. New/updated tests: identical_content_different_category_does_not_dedup, identical_content_different_tags_still_dedups, test_get_entries_for_session_category_scope_excludes_other_categories, plus the existing get_entries_for_session/DedupIndex test suites updated for the new category parameter. docs/src/memory.typ coverage-boundary note updated to describe the category scope and the tags exclusion, and the deferred UNIQUE index note updated to key on (session, owner, category, dedup_hash). Deferred (not in this PR, per review triage lanes 2-5): TOCTOU-open in-process gate (garbaggio UNIQUE index option), unbounded get_entries_for_session candidate fetch, and the #399/#401 merge-order plan -- all acknowledged/comment-lane findings, not blockers.
auto_embed/auto_anchor ran with `?` after the durable write had already committed (upsert_knowledge + read-back verified). A transient embed or anchor failure -- model download hiccup, etc. -- propagated straight to main() and exited non-zero even though the entry (or edit) had already landed. Callers treating exit-1 as "it didn't happen" would retry and duplicate the write. Made the post-write embed/anchor steps non-fatal (log + continue) across all six write-then-side-effect sites: Add (both the shared add_one path and the --type quick-fact path), Update, Edit, Append, Prepend, and Restore. Mirrors the existing non-fatal backup_content pattern. The write itself still bails non-zero on genuine failure (permission denial, missing entry, invalid category) -- only the best-effort side effects after a confirmed-durable write are now soft. Adds tests/write_side_effect_nonfatal.rs: forces a deterministic embed failure (MX_ISOLATE_MODELS pointed at a blocked path) to assert exit 0 + warning + persisted write on Add and Append, plus two genuine-failure cases (missing entry, invalid category) asserting exit stays non-zero.
…ns pollute --json stdout (pre-existing contract bug, flagged separately)
…(Cory B1) add_one's post-write embed/anchor steps are non-fatal by design (the entry is already durable and read-back verified before either runs), but a failure there was only ever eprintln'd -- a --json caller had zero signal that the write landed in a degraded state. Capture the failure in a new WriteSideEffects struct threaded through WriteOutcome::Written and fold embed_deferred/anchor_deferred into the Add JSON payload, following the same conditional-field pattern #402 established for dedup/dedup_lookup_warning (present only when the step actually failed; absent on success or on a deliberate --no-embed/--no-auto-anchor skip, which already has its own "(skipped)" notice). AddBatch always calls add_one with embed=false/no_auto_anchor=true (embed is hoisted, anchoring deferred to the nightly run), so its side_effects are always empty -- destructured and intentionally ignored there.
auto_anchor early-returns Ok(()) when embedding.is_none() (covered by no_embedding_skips_anchoring), but the path past that guard had zero coverage: a candidate-fetch failure on an entry that IS embedded. Adds FailingScoredStore, a KnowledgeStore test double that wraps a real in-memory SurrealDB and forces semantic_search_entries_scored to error (same shape as handlers::memory::FailingStore, different failing method), plus a test proving the failure surfaces as Err rather than being swallowed inside auto_anchor itself -- which is what lets add_one (B1) catch it non-fatally and record it as anchor_deferred.
df0cb71 to
ae06332
Compare
|
Both blockers addressed: B1 — embed/anchor side-effect failures now surface as conditional embed_deferred/anchor_deferred fields in the --json payload (same conditional-field pattern as #402's dedup fields, folded per your note); B2 — anchor-failure path test added (already-embedded entry, candidate-fetch failure surfaces past the embedding.is_none() guard). Rebased onto #402's head per landing order. Re-requesting review. |
3 participants
Symptom
mx memory append(and its siblings) reported exit 1 on writes that had already landed. Callers treating exit 1 as "the write failed" re-fired — and the re-fire created duplicates. Hit in real use: a backgroundedappendreported failure on a write that was already durable, the caller retried, and the entry got written twice.Root cause
The durable write commits first, then post-write side effects run:
auto_embed(...)?andauto_anchor(...)?. Those?operators propagate transient side-effect failures — model cold-load, a websocket blip — straight to a non-zero exit, after the entry is already durable. So a fully-successful, committed write surfaced as a failure because an optional follow-up step hiccupped.Diagnosis was trace-level: walking iteration counts and the exact
?operators in the post-write blocks ofsrc/handlers/memory.rs.Fix
Post-write embed/anchor is now non-fatal: on failure it logs a stderr warning noting that the entry itself is durable, and exits 0. This mirrors the existing non-fatal backup pattern already in the codebase. Applied symmetrically across Append / Add / Update / Edit / Prepend / Restore. Genuine write failures — where the durable write itself fails — still exit non-zero.
Validation
Adversarial repro against the patched build: corrupted the cached ONNX blob to force
TractProvider::newto fail at the exact diagnosed line.New test file
tests/write_side_effect_nonfatal.rs, 5/5 passing.cargo fmtandcargo clippyclean.Known follow-up (not in this PR)
--no-embed/--no-auto-anchoremit their "(... skipped)" skip-notices viaprintln!to stdout, ahead of the JSON payload under--json— which corrupts the "stdout is JSON" contract. Surfaced by the new tests (theadd_plain_no_embedhelper drops--jsonto work around it). Pre-existing, independent of this change. Left for a separate call on the broader "human-facing notices never touch stdout under--json" rule.Note
The local test environment couldn't run
tests/trigger_check.rs(SurrealDB IAM permissions — environmental, not a code issue). CI should exercise it properly.