Honour forget, reset and delete on the retrieve path, and read a resource text back - #141
Merged
Merged
Conversation
…ext back
Two follow-ups to the memory-API work on this branch.
**forget and reset were not honoured by /v1/retrieve.** Both wrote a durable tombstone, and every
read through the Python serving pipeline honoured it, so `get_all` returned 0 immediately.
`/v1/retrieve` does not go through that pipeline: on a native backend the engine assembles the
context pack itself, and it has never heard of a memory tombstone. So a forgotten memory kept
coming back from retrieve, verbatim:
before forget: get_all=2, retrieve contains the subject's secret = True
forget: http 200, removed_count 54
after forget: get_all=0, retrieve contains the secret = STILL True
That is a deletion defect, not a ranking nuance, and it is invisible to any check that asks only
`/v1/memories`.
The engine could already do this and nothing called it. `matrixark_forget_scope` physically removes
a scope's records: it refuses an under-specified scope that would match everything, rewrites each
hash field down to its survivors, commits one durable batch, and clears the scan and hgetall caches
-- its own comment says that last part exists "so a subsequent retrieve/get_all never re-serves a
forgotten record". So this needed no engine change, just the wiring.
The tombstone is still written first: it is the durable, auditable record of the forget, it keeps
the serving view correct even if the engine call fails, and its order-aware semantics are what let
a subject be re-ingested afterwards. If the engine call fails the result says so rather than
reporting a clean forget over data that is still there.
Scope handling is what the tests mostly cover, because getting it wrong is what would be dangerous.
forget recomputes the SUBJECT's identity hashes -- a request scope carries hashes derived from the
CALLER, and purging with those removes the wrong subject -- and keeps user_id, since dropping it
would widen a forget into a tenant wipe. reset does the opposite: drops the user/session dimensions,
keeps the tenant hash, and declines to purge at all when no tenant hash resolves.
STILL OPEN: `delete` of a single memory has the same hole. The engine has no record-id-level
removal -- `matrixark_forget_scope` matches by scope, not by id -- so it needs an engine addition
rather than wiring.
**Reading a resource's or skill's text back.** `list_skills` and `list_resources` return a POINTER
and metadata; the text is stored, split across `resource_chunk` records, but nothing reassembled
it. `get_resource_content` does, and pages it -- an attachment can be far larger than belongs in
one response, so it returns at most `chunk_limit` chunks and `max_chars` characters and reports
`next_chunk_offset` when there is more.
Ordering would have failed silently: chunks carried no order of their own, so reassembly depended
on the log being read back in append order, which is a property of how it was read rather than of
the records. Each chunk is now stamped with `chunk_index` at ingest and read back in that order,
falling back to log order for chunks written before the index existed.
25 new tests across the two.
…he engine The last of the three deletion operations that stopped at the serving view. `delete` wrote its tombstone, `get_all` dropped from 2 to 1 immediately, and `/v1/retrieve` -- assembled inside the engine, which has never heard of a memory tombstone -- went on serving the deleted memory. forget and reset could be fixed by wiring `matrixark_forget_scope`, which already existed. delete could not: that op matches by SCOPE, and a delete addresses one memory. So this adds `matrixark_delete_records`. It is deliberately a dumb primitive. Deciding what a delete covers is the subtle part -- the addressed event, its single-source derivatives, and the embeddings and index postings pointing at any of them, while MULTI-source derivatives are demoted (rewritten with the source trimmed) rather than removed. That rule already exists, once, in `delete_memory`. Re-deriving it in Rust would put two copies of it in the tree in two languages, which is exactly the kind of drift these fixes have been chasing. So the engine takes a list of ids and removes what matches; the caller decides the list, and `delete_memory` now reports the set it decided instead of keeping it private. The engine side mirrors `forget_scope_records` -- same shard walk, same survivor rewrite, same single durable batch that also clears the scan and hgetall caches so a later retrieve cannot re-serve a removed record from cache. Only the predicate differs: a record matches on any id it is addressable by, its own identity or a pointer it carries (`ref_hash`, `ref_hashes`). Matching those pointers is what stops a delete leaving orphaned postings behind that still surface the text. An empty id list removes NOTHING and returns early. That guard is the whole safety story: without it the walk would match every record and a no-op delete would wipe the store. Verified live on the same store: delete before: get_all=2, retrieve serves the canary -> after: get_all=1, retrieve clean reset before: retrieve serves the canary -> after: get_all=0, retrieve clean forget before: retrieve serves the secret -> after: get_all=0, retrieve clean All three deletion operations now mean the same thing on every read path. 5 more tests (18 in that suite), including that the closure is passed through rather than re-derived, that an empty closure purges nothing, and that an engine failure is reported rather than reported as a clean delete.
An update is a supersede: the new text is ingested and the old id tombstoned. `get_all` honoured
that immediately. `/v1/retrieve` is assembled inside the engine, which has never heard of a
tombstone, so the OLD text kept being served -- and because it outranked the new one, a search
after a successful update returned the stale value:
after update: get_all = the new text
retrieve = the OLD text
The inherited implementation already computes exactly which records the old version covered, and
its own comment says the sweep exists "so the old text can't leak via retrieval after the update".
The engine simply never learned about it. `update_memory` now reports that set, the native adapter
purges it in the engine, and the stale value is gone from retrieve.
The purge helper is now shared with `delete_memory` rather than duplicated.
A purge also invalidates the background refresh skip. That skip rests on "dirty state only changes
when records are APPENDED, and the record count is a cheap proxy for that" -- an engine purge
breaks the assumption, because it removes and rewrites records in place without moving the count,
so a pass that had nothing to do could stay skipped while the node whose summary it just removed
never gets rebuilt. This is reasoned from the invariant rather than measured: it did not change the
symptom below, but the assumption it protects is genuinely violated by a purge.
STILL OPEN, and I could not pin it: after an update the NEW value is not retrievable either. It is
correct in `get_all`, and the old value is now correctly gone, but a search returns neither.
Isolating work so far, so the next person does not repeat it:
* it is not the refresh-skip token -- clearing it on purge changed nothing;
* it is not the scope -- the replacement carries a correct scope_key (t/u/s all present);
* it is not that the node is dead -- a later unrelated ingest into the SAME scope IS
retrievable, so the node's summary is being rebuilt and the replacement is being left out of
it;
* re-ingesting onto the old record's node_path did not fix it either, and that change was
reverted rather than shipped unproven.
So the replacement record exists, is correctly scoped, and is excluded from the content the node's
summary covers. That is where to look next.
mem0 suites, delete/forget, the engine-purge suite and the native-path guard all green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…able
After a successful update the NEW value was in `get_all` and in no search. The superseded value it
replaced had already been committed, so before the previous fix a search returned the stale value
and afterwards it returned nothing at all -- an update that reported success and left the memory
unfindable.
Reading the records straight out of the engine is what settled it. The replacement sat at
`extraction_phase: hot_path` / `status: observed` with no `node_path`, while an ordinary ingest in
the same scope reached `pending_async` and then `final` / `extraction_committed`:
replacement phase=hot_path status=observed node_path=None
plain phase=pending_async status=pending
plain phase=final status=extraction_committed
`finalize` is honoured by the DISPATCH layer, which runs session_commit as a second tool call --
`adapter.ingest()` ignores the flag entirely. `update_memory` re-ingests through the adapter
directly, passing `finalize: True` into a method that has never read it, so the replacement was
never committed and retrieval, which serves committed content, could not see it. It is now
committed explicitly; an update returns the new id, so it is synchronous by contract and should not
wait for the idle-commit debounce.
Only the native path does this. On the JSONL backend the re-ingest is already retrievable, and
committing there BREAKS it: doing this in the shared implementation turned
`test_update_supersede_retrieve_returns_new` into an empty context pack. The shared implementation
now just reports the scope it re-ingested into, and the native adapter finishes the write.
Also removes a duplicated `_purge_record_ids_in_engine` + `update_memory` block that a re-applied
patch had left behind. Python binds the last definition, so the behaviour was already correct and
the file simply carried a second copy that would have diverged on the next edit.
All four operations now mean the same thing on both read paths:
update get_all = new value, retrieve = new value, old value gone from both
delete get_all 2 -> 1, retrieve clean
reset get_all 0, retrieve clean
forget get_all 0, retrieve clean
Full mem0 surface asserted on values: 22/22. mem0 suites 56/56, delete/forget 21/21, engine-purge
and resource-content 30/30, native-path guard green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`drain_due_idle_session_commits` runs once per ingest and opened with `read_all()` -- on a native
backend, the whole record log shipped over the proxy -- while using the result for nothing but
`matrixark_async_pipeline_task` records. Both of its loops skip everything else.
It now goes through a seam the native adapter overrides with the typed scan that already existed
for this, `idle_commit_task_records`. The JSONL backend keeps `read_all()`, where it is an
in-memory walk with nothing to save.
This was blocked on an ordering question, which turned out to be answerable rather than a reason to
leave it alone. The drain decides last-write-wins from list position, and the scan ends in
`compact_latest_context_state_records`, so the worry was that compaction reorders or collapses the
tasks. It does neither: that function keys only `context_summary`, `context_model_registry` and
some `context_embedding` rows -- a pipeline task gets no key and passes through untouched -- and it
re-sorts by the original index, so append order survives either way. Both halves of that argument
are pinned by tests against the real compaction rather than asserted.
An empty scope falls back to the full read on purpose: `idle_commit_task_records({})` degenerates
into a cross-scope full-store scan, which is precisely the cost this exists to avoid, so
"optimising" an unscoped call would turn it into the worst case. A failing scan falls back too --
the drain must not stop because a scan did.
Native full-store reads per ingest: 5.7 -> 4.0 (34 -> 24 across 6 ingests, instrumented at the read
choke points). What remains is one read in `batch_extract` and one `_read_raw_records` from
`session_commit`, plus the three one-time index backfills.
6 new tests. mem0 suites, delete/forget, engine-purge and the native-path guard all green, and the
live checks still hold: no cross-user leak, and forget / reset / delete / update all honoured on
both read paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Look an idempotency key up once per dispatch, not twice
`_idempotent_replay_response` looks the key up at the start of a dispatch, and returns a replay if
it finds one -- so the call only continues when the key has been proven ABSENT.
`_finalize_write_response` then looked the same key up again a moment later. An ingest that
finalizes is two dispatches, so that was two redundant lookups per ingest on the request thread.
The answer is now noted on the call's own args and consumed by finalize.
Skipping a check is only safe if it was actually made, so that is what the tests mostly cover: the
note is per key and per call, a tool that finalizes without going through the replay path still
looks, and a key that differs from the one proven absent is still looked up. A replay hit
short-circuits before finalize as it always did.
mem0 suites 56/56, delete/forget 21/21, the full mem0 surface 22/22 on values, and the live checks
still hold -- no cross-user leak, and forget / reset / delete / update all honoured on both read
paths.
5 new tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two follow-ups to the memory-API work merged in #133.
forget and reset were not honoured by
/v1/retrieveBoth wrote a durable tombstone, and every read that goes through the Python serving pipeline
honoured it —
get_allreturned 0 immediately./v1/retrievedoes not go through that pipeline:on a native backend the engine assembles the context pack itself, and it has never heard of a
memory tombstone (
matrixark_memory_tombstoneappears nowhere in the engine). So a forgottenmemory kept coming back from retrieve, verbatim, as
type=eventitems:That is a deletion defect, not a ranking nuance, and it is invisible to any check that asks only
/v1/memories.The engine could already do this and nothing called it.
matrixark_forget_scopephysicallyremoves a scope's records: it refuses an under-specified scope that would match everything,
rewrites each hash field down to its survivors, commits one durable batch, and clears the scan and
hgetall caches — its own comment says that last part exists "so a subsequent retrieve/get_all
never re-serves a forgotten record". The only mention of it anywhere in Python was an unrelated
test name. So this needed no engine change and no rebuild, just the wiring.
The tombstone is still written first, deliberately: it is the durable, auditable record of the
forget, it keeps the serving view correct even if the engine call fails, and its order-aware
semantics are what let a subject be re-ingested afterwards. If the engine call fails, the result
says so (
engine_purge.ok = false) rather than reporting a clean forget over data that is stillthere.
Scope handling is what the tests mostly cover, because getting it wrong is the dangerous part:
identity_hashes. A request scopecarries hashes derived from the caller, and purging with those removes the wrong subject. It
keeps
user_id, since dropping it would widen a forget into a tenant wipe.declines to purge at all when no tenant hash resolves — there is then no safe scope to hand the
engine.
Verified live on the same store, before and after:
get_all=0and retrieve cleanget_all=0and retrieve cleandelete, and a record-removal op in the engine
deletehad the same hole, and could not be fixed by wiring:matrixark_forget_scopematches byscope, and a delete addresses one memory. So this adds
matrixark_delete_records.It is deliberately a dumb primitive. Deciding what a delete covers is the subtle part — the
addressed event, its single-source derivatives, and the embeddings and index postings pointing at
any of them, while multi-source derivatives are demoted (rewritten with the source trimmed)
rather than removed. That rule already exists, once, in
delete_memory. Re-deriving it in Rustwould put two copies of it in the tree in two languages, which is exactly the kind of drift these
fixes have been chasing. So the engine takes a list of ids and removes what matches; the caller
decides the list, and
delete_memorynow reports the set it decided instead of keeping it private.The engine side mirrors
forget_scope_records— same shard walk, same survivor rewrite, samesingle durable batch that also clears the scan and hgetall caches. Only the predicate differs: a
record matches on any id it is addressable by, its own identity or a pointer it carries
(
ref_hash,ref_hashes). Matching those pointers is what stops a delete leaving orphanedpostings behind that still surface the text.
An empty id list removes nothing and returns early. That guard is the whole safety story:
without it the walk would match every record and a no-op delete would wipe the store.
deleteget_all2→1, retrieve still serves itget_all2→1 and retrieve cleanresetget_all=0and retrieve cleanforgetget_all=0and retrieve cleanAll three deletion operations now mean the same thing on every read path.
Reading a resource's or skill's text back
list_skillsandlist_resourcesreturn a pointer (raw_uri,cloud_key) and metadata. The textitself is already stored, split across
resource_chunkrecords at ingest, but nothing reassembledit — so "give me this skill's content" had no answer.
get_resource_contentdoes, and pages it: an attachment can be far larger than belongs in one JSONresponse, so it returns at most
chunk_limitchunks andmax_charscharacters and reportsnext_chunk_offsetwhen there is more.skill_hashandresource_hashare aliases.Ordering would have failed silently. Chunks carried no order of their own, so reassembly depended
on the log being read back in append order — a property of how it was read, not of the records.
Each chunk is now stamped with
chunk_indexat ingest and read back in that order, falling back tolog order for chunks written before the index existed, which is the only ordering those have. A
test builds the chunks out of order in the log and asserts the content comes back in the right one.
Verification
25 new tests. On this tree: mem0 suites 56/56, delete/forget 21/21, the native-path guard, the
record-cache suite, and both new suites green.