Repository navigation
feat: persist UUID episode identifiers - #1707
Conversation
20918da to
10e0ae2
Compare
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Reviewed at 10e0ae2. The parallel write and the grace period on missing
episodes both look right to me; three things I'd want settled before this
goes in, all reproduced locally:
-
Upgrading an existing deployment. The episode store only runs create_all,
so an existing episodestore keeps its INTEGER id column, and inserting a
UUID into it fails (SQLite: "IntegrityError: datatype mismatch"; Postgres
rejects it the same way). Because the store write now runs alongside the
episodic and semantic writes, each of those failed adds still lands in
episodic memory and the semantic queue. So on an un-migrated database
every add errors and leaves an orphan. Could we either ship a migration
(semantic storage already has one for its history_id -> string change) or
check the column type at startup and refuse to start? Either way it fails
loudly instead of half-writing. -
Orphans can't be deleted by id. Same shape outside the upgrade case: if
episode_storage.add_episodes fails (DB timeout, dropped connection), the
episode is still in episodic memory and shows up in search, but the new
existence check in delete_episodes raises ResourceNotFoundError for it,
so it can only be cleared by deleting the whole session. Either
delete_episodes should go ahead for ids that aren't in the store, or a
failed store write should undo the episodic write. -
Ordering within a batch. History now sorts by (created_at, history_id),
and every message in one add shares a created_at whenever the client sends
the same timestamp for all of them (e.g. importing a transcript stamped
per session). Ties then fall back to the random UUID, so ingestion can
process the messages out of order and the older fact wins. I ran
test_ingestion_keeps_latest_value_across_uuid_ordered_batches with every
entry at the same created_at: it ends on "violet" instead of "red". On
main the int id kept insertion order. Something that breaks ties by
position in the batch would restore it.
Verified and fine: created_at reaches semantic history on all four backends,
the 30 s grace period covers ingestion reading an episode before its store
row commits, and CI is green at head.
We do not support upgrade or backward compatibility. |
|
Also using UUID types everywhere prevents bugs:
|
edwinyyyu
left a comment
There was a problem hiding this comment.
Reviewed at 3755d06 against main 8101f14. Inline comments below. The three points already raised (migration, orphans on partial failure, batch ordering) remain open; I have not repeated them except where the inline comment adds a mechanism.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
3755d06 to
07f4317
Compare
edwinyyyu
left a comment
There was a problem hiding this comment.
Second pass at 07f4317. Three inline comments below, plus one item on a file outside the diff:
packages/common/src/memmachine_common/api/doc.py:634 still gives EPISODIC_ID = ["123", "345"] and EPISODIC_IDS = [["123", "345"], ["23"]] as the public examples, and docs/openapi.json is generated from them. This PR changes the id format every client sees, so those examples now describe ids that can never match. Please update them to UUID strings and regenerate docs/openapi.json in this PR.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
edwinyyyu
left a comment
There was a problem hiding this comment.
One more inline comment on the metadata normalization.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
edwinyyyu
left a comment
There was a problem hiding this comment.
Four more inline comments: the history timestamp doubling as the ingestion debounce clock, and three on the grace-period bookkeeping.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
edwinyyyu
left a comment
There was a problem hiding this comment.
One naming nit.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
edwinyyyu
left a comment
There was a problem hiding this comment.
One more on the missing-episode bookkeeping.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
leomem
left a comment
There was a problem hiding this comment.
Reviewed at a2cbde7. Batch delete from the Python client and the CLI fails after this change.
DeleteEpisodicMemorySpec.episodic_id is now UUID | None (packages/common/src/memmachine_common/api/spec.py:693), but the client still sends an empty string by default:
packages/client/src/memmachine_client/memory.py:598:delete_episodic(self, episodic_id: str = "", ...)passesepisodic_idinto the spec unconditionally.packages/client/src/memmachine_client/cli.py:456:--iddefaults to"".
So memory.delete_episodic(episodic_ids=[...]) and memmachine delete-episodic --ids <uuid> raise before any request is sent. Reproduced on this branch:
DeleteEpisodicMemorySpec(org_id="o", project_id="p", episodic_id="", episodic_ids=[str(uuid4())])
# ValidationError: episodic_id
# Input should be a valid UUID, invalid length: expected length 32 for simple format, found 0 [type=uuid_parsing, input_value='']An older client that sends "episodic_id": "" now gets a 422 from the server for the same reason. The client tests only call delete_episodic with a single id, so CI doesn't catch it.
Suggested fix: default episodic_id to None in delete_episodic and in the CLI's --id, and add a client test that deletes by episodic_ids alone. If old clients should keep working, the spec could also map "" to None before validation.
|
@malatewang one review thread is still open and may be easy to miss since it sits on an older review: #1707 (comment) Short version: keeping the store insert concurrent with the memory writes is a correctness tradeoff (partial writes with no scan-free recovery), so it needs a number. The saving is bounded by the store insert time itself, and that is already instrumented as 🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account. |
That number will not provide any information. Any overhead is really workload and hardware dependent. |
|
6f6fa2e to
c4216f2
Compare
edwinyyyu
left a comment
There was a problem hiding this comment.
Final-state pass at 6f6fa2e. Six inline comments: two correctness, two dead code, one stale docstring, one leftover test setup. Two items on files outside the diff:
-
PR body, Compatibility. It declares only the episode table incompatible with existing databases. The semantic history and citation tables are too: existing
history_idand citation values are integer strings, every reader now doesUUID(...)on them, and history rows from before migration62dff1150a46have a NULLcreated_atthat the new registration-time lookup cannot handle. Under the stated no-upgrade stance that is fine, but the body should say all three tables, not one. -
docs/open_source/configuration.mdx. Thesemantic_memoryparameter table and samples do not list the newmissing_episode_grace_period_seckey. The same section's existing field isingestion_trigger_age_seconds, an integer, so the new key should also follow that spelling and type within the section.
🤖 Written by Claude Fable 5.1 via Claude Code; posted from @edwinyyyu's account.
|
On the final-state review summary (#1707 (review)): the documentation gap for missing_episode_grace_period_sec is valid. One naming correction: docs/open_source/configuration.mdx currently documents ingestion_trigger_age as an HH:MM:SS duration, matching the YAML configuration model. ingestion_trigger_age_seconds is the API update field, not the key used in that documentation section. I propose adding missing_episode_grace_period_sec as a numeric seconds field to both YAML samples and the parameter table, while keeping the existing age key unchanged. |
ee207fe to
f7ef38f
Compare
…UUIDs and scores, and refuse invalid inputs (MemMachine#1733) * Look up the segments that derivatives belong to in the segment store Event memory resolves a search hit, a derivative's vector record, to the derivative's segment through a `_segment_uuid` property it copies onto every vector record. The segment store already holds that mapping, on the derivative's own row. `SegmentStorePartition.get_segment_uuids_by_derivative_uuids` reads it. The SQLAlchemy partition answers with one query on the derivative table, served by its primary key, and checks the partition's liveness in the same statement, as its other reads do. A derivative the partition does not hold is left out of the result. The in-memory partition the event memory tests use implements it too. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Resolve event memory's search hits through the segment store A derivative's vector record carried its segment's UUID as the `_segment_uuid` property, and a search read it back from each match. That copy is only as fresh as the vector store's reads, and the segment store holds the same mapping on the derivative's row. A search now asks the vector store for no properties and maps the matched derivatives to their segments with one `get_segment_uuids_by_derivative_uuids` call. A match whose derivative the segment store no longer holds, because its segment was deleted and its vector outlived it, is dropped. Vector records no longer carry `_segment_uuid`, and the schema event memory expects of its collection no longer declares it; a collection created with it keeps declaring it. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Resolve semantic search hits through a vector_uuid column on the feature row A feature's vector record was keyed by a UUID derived from the feature's id, and carried eleven properties copied from the feature's row, among them `feature_id`, which a search read back to resolve a hit to its row. The caller's feature metadata was merged into the same properties. - `update_feature` read the stored vector back through `VectorStoreCollection.get` to write the record again with fresh properties, since an upsert replaces a whole record. The value read is written back, so a stale read becomes a lasting wrong write: of two concurrent updates of one feature, one with a new embedding and one without, the second can write the old embedding over the new one; and a read that misses the record fails the update after its row was committed (MemMachine#1721). - Nothing filtered on the copied properties; the row is their authority. The feature row now carries `vector_uuid`, a fresh UUID that keys its vector record, and the record carries no properties. A search resolves its hits through that column, in the order the search returned them, dropping a hit whose row is gone. `update_feature` writes the vector store only when given a new embedding, and reads nothing back. The delete paths take the UUIDs of exactly the rows they delete with RETURNING. The collection declares no indexed properties. Breaking: a feature table created before this has no `vector_uuid` column, and the table is created with `create_all`, which adds none; and a collection created with the old declared properties no longer matches the configuration `open_or_create_collection` asks for. No migration is included. Fixes MemMachine#1721. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Answer a vector store query with record UUIDs and scores; remove get and close_collection A query returned each match's record: its properties, and its vector on request. They were copies of what the callers' own stores hold, only as fresh as the vector store's reads, and since the previous two commits no caller reads them. `get` has had no caller since semantic memory stopped reading its vectors back, and a `get` safe to write from would need every backend to reflect every write that returned before it, from any process. Every store's `close_collection` did nothing, and nothing called it. - `QueryMatch` carries a score and a `record_uuid`, and `query` loses `return_vector` and `return_properties`. Properties are still stored and filtered on. - `VectorStoreCollection.get` and `VectorStore.close_collection` go, with what only served them: the stores' record parsing, `VectorSearchEngine.get_vectors`, and sqlite-vec's vector decoding. - `Record` is input-only, so its vector is required and its properties default to `{}`. The model rejects a missing vector at construction, so the stores' checks for one and their coalescing of `None` properties go. Mechanical: event memory and semantic memory read `match.record_uuid` in place of `match.record.uuid` and stop passing the flags, and the in-memory test collection follows the interface. Tests of the return flags, of `get`, and of values read back through a query go. Tests that check what a store holds read the backend past the store: the SQLite stores' records tables, a Qdrant scroll of the collection's partition, a Milvus `get` on the client. The tests of a store refusing a `None` vector become tests that the model refuses a missing one, plus one that properties default to `{}` and one that a record without properties is stored. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Refuse vector store inputs no collection can hold or search Each store refuses, with a ValueError and before any backend call: - a record whose declared property holds a value of another type, since the collection indexes and filters the property as its declared type (`require_declared_types`: the value's type is the declared type, and a bool is not an int); - a record vector or a query vector whose width is not the collection's dimensions; - a query vector with a coordinate that is not finite, or a score threshold that is not finite. The model checks a record's own vector for finiteness: `Record.vector` is `list[FiniteFloat]`, so pydantic refuses a NaN or infinite coordinate when the record is built, in the pass that already validates each coordinate. A query vector is a plain sequence, so the stores check it. An embedding endpoint can return NaN whatever the caller does, so both kinds of vector are checked. Qdrant and Milvus check a filter's property keys with the other inputs, before the early return for no query vectors. `validate_identifier` uses `fullmatch`: `$` also matches before a trailing newline, so `"name\n"` passed as a namespace, a collection name or a property key. The ABC states these refusals in `upsert` and `query`. Tests: each store refuses each input, and a record refuses a coordinate that is not finite. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * State the vector store's consistency in its contract The contract said nothing about what a query sees of earlier writes. `VectorStoreCollection` now states what every store keeps: An `upsert` or `delete` is durable once it returns; queries may not reflect it right away. A store that guarantees more states it. Neither event memory nor semantic memory reads its own writes back through the vector store: a search's hits resolve through the segment store or the feature row, which hold the mapping. Milvus reads at the consistency level its collections are configured with, and a replicated Qdrant may answer a query from a replica that has not applied a write yet, so a stronger promise would bind every store to read settings with costs of their own. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Name the derivative-to-segment map segments_by_derivatives Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Drop a None check no property reaches, and a filter branch no caller takes - The Qdrant store's payload skipped a None property, but a Record's properties are PropertyValues, which exclude None, so the model refuses one before the store sees it. - Semantic storage's _apply_feature_filter took a Select or a Delete, but only selects reach it: delete_feature_set filters its DELETE ... RETURNING itself. It takes and answers a Select, and the cast at its caller goes. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Tidy the tests the review found out of place - The SQLite stores' four input-validation tests sat inside TestFilters, under a "Delete" banner meant for TestDelete; they move into TestInputValidation, and the banner moves above TestDelete. - Two event memory schemas still declared `_segment_uuid`, which event memory no longer reserves: the context test declares `_timestamp` alone, and the missing-base-field test declares nothing. - The 40,000-feature test's comment says which SQLite the bind limit is the feature store's. Tests only; the same tests run and pass. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Fold the declared-type check into the vector store utilities declared_properties.py held one function beside utils.py, which holds the other input checks every store runs (dimensions, query vector, score threshold, identifiers). It imports only common.data_types, which imports nothing back, so it moves into utils.py as is. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Resolve semantic search hits in one statement Since queries answer record UUIDs alone, a semantic search mapped its hits to feature ids in one select, then loaded the features by id with the filter in another: two statements in two sessions, and the first ran even when the search found nothing. One select on the vector_uuid column with the filter now loads the features, ordered as the hits were, and no select runs for a search with no hits. Results are unchanged: the same features, in the same order, under the same filter. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Refuse a query limit that is not positive The contract said nothing of a limit at or below zero, and the stores disagreed: the SQLite stores and Milvus answered empty results, and Qdrant passed the limit on for the server to reject. A limit that is not positive is now a ValueError in every store, stated in the query contract beside the other refused inputs, and checked whether or not there are query vectors. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Refuse a search for no memories before it does any work A search whose top_k is not positive asks for nothing, yet it reached the vector store, which refuses a limit that is not positive, only after event memory had embedded the query: a request for nothing still cost an embedding call. SearchMemoriesSpec.top_k is now positive (gt=0), so request validation refuses it with a 422 naming top_k, the MCP tool included, and LongTermMemory.search_scored refuses a num_episodes_limit that is not positive before the query is embedded, for callers of the Python API. The event backend's window test no longer runs a limit of 0 on the in-memory vector fake, which does not check limits; its lower bound is now exercised with a negative expand_context. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> * Show the positive top_k in the OpenAPI document docs/openapi.json, as docs/tools/generate_openapi.py writes it for the search specification's top_k: exclusiveMinimum 0 and the description. The generator's other differences under the locked FastAPI are left to the regeneration that carries them. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]> Rebased onto main. Main refuses a top_k that is not positive since MemMachine#1740, with ge=1 and its own tests, so this commit keeps that bound and those tests in place of the gt=0 bound and the test described above, and docs/openapi.json shows minimum 1. vector_store_semantic_storage.py keeps importing datetime, which the history methods MemMachine#1707 added use.
Summary
Episodes receive server-generated UUIDs before persistence. A database-backed
increasing sequence number gives every episode a stable tie breaker when event
times match. Both SQLite and PostgreSQL reserve numbers atomically; semantic
history stores the same sequence instead of a batch position. Episode and
semantic history reads order by episode time and sequence.
The episode store, episodic memory, and semantic registration still write
concurrently to avoid serializing request latency on the episode database
commit. Semantic history keeps registration time separate from episode time.
Registration and ingestion age checks use the semantic storage clock, so clock
skew between application workers and the database does not prematurely expire
missing episodes or trigger ingestion. Empty episode metadata is normalized to
Noneacross in-memory and stored read paths.Delete behavior and partial writes
Deleting a valid but unknown episode UUID is idempotent and returns success.
Deletion attempts cleanup in each configured backend even when the episode
store row is absent. The episode-store existence read has been removed. All
concurrent add failures are logged before the request raises its first error.
Concurrent writes can still leave a partial record if one backend fails. The
semantic ingestion retry window remains in place, is configurable through
semantic_memory.missing_episode_grace_period_sec, and is applied in debug mode.Compatibility
This PR does not upgrade old integer episode IDs. Existing integer-key episode
tables, semantic history rows, and citations that refer to those IDs require
an explicit data migration before use with UUID episodes. For databases
already using the UUID schema, the PostgreSQL semantic history migration and
the vector semantic storage startup migration rename the batch position column
to the episode sequence column.
Verification
Focused episode, semantic, and episodic tests pass. Ruff checks and formatting
pass on changed Python files. PostgreSQL and Neo4j integration tests could not
run because Docker is unavailable in this environment. The broad type check is
blocked by optional packages absent from the test environment.
The OpenAPI document was regenerated with UUID examples and schemas.