Repository navigation
Delete semantic data whenever semantic storage is configured - #1831
xiongzubiao wants to merge 7 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Cached remote backends can bypass reachability checks, allowing partial deletion and orphaned semantic vectors.
2 open findings
What changed in this PR
Addresses #1830 by separating semantic cleanup from serving enablement, so configured semantic data is deleted even when semantic memory is disabled.
Changes:
- Adds deletion-only storage resolution without semantic startup or model dependencies.
- Updates deletion paths and centralizes existing project set selection.
- Adds regression tests and documents disabled-mode cleanup.
| File | Description |
|---|---|
| packages/server/src/memmachine_server/semantic_memory/storage/storage_base.py | Defines deletion-only storage protocol. |
| packages/server/src/memmachine_server/semantic_memory/semantic_session_manager.py | Centralizes project deletion set IDs. |
| packages/server/src/memmachine_server/semantic_memory/semantic_memory.py | Exposes set-filter helper. |
| packages/server/src/memmachine_server/main/memmachine.py | Uses configured storage for semantic cleanup. |
| packages/server/src/memmachine_server/common/resource_manager/semantic_manager.py | Builds storage without semantic startup. |
| packages/server/src/memmachine_server/common/resource_manager/resource_manager.py | Exposes deletion storage getter. |
| packages/server/src/memmachine_server/common/configuration/__init__.py | Distinguishes storage configuration from enablement. |
| packages/server/server_tests/memmachine_server/semantic_memory/test_semantic_session_manager.py | Checks set-selection equivalence. |
| packages/server/server_tests/memmachine_server/main/test_memmachine_mock.py | Covers cleanup gating and resolution failures. |
| packages/server/server_tests/memmachine_server/main/test_memmachine_delete_session.py | Extends disabled-mode deletion coverage. |
| packages/server/server_tests/memmachine_server/common/resource_manager/test_semantic_deletion_storage.py | Tests deletion-only backend construction. |
| packages/server/server_tests/memmachine_server/common/configuration/test_semantic_conf.py | Tests backend storage detection. |
| packages/server/server_tests/memmachine_server/common/configuration/test_configuration.py | Checks storage survives auto-disablement. |
| docs/open_source/configuration.mdx | Documents cleanup while disabled. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
VectorStoreSemanticStorage.delete_feature_set committed the row delete before deleting the vectors, so a failed vector delete left vectors that no retry could find. It now reads the vector uuids, deletes the vectors, then deletes the rows and the vectors of any row that appeared in between. A failed vector delete leaves the rows, and a retry deletes both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
build_semantic_deletion_storage (exposed as ResourceManagerImpl.get_semantic_deletion_storage) builds the configured semantic backend for deletes only: no startup() (no migrations or backfill), the vector collection opened rather than created, and no language model or embedder. It returns None when the store's feature table does not exist, i.e. the store was never initialized. It shares backend construction with get_semantic_storage and is a module function rather than a SemanticResourceManager method, so deletion does not depend on the manager. It returns a SemanticDeletionStorage protocol carrying only the deletes. SemanticSessionManager.project_deletion_set_ids computes the sets a project deletion removes from the org and project ids alone. With no metadata every set type the config-store resolution keeps is untagged and yields a default set id, so this matches that resolution without reading or registering set types. with_has_set_ids becomes public. The sqlite-vec tests skip where sqlite3 cannot load extensions, as the sqlite-vec store's own tests do. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
Project and episode deletion skipped semantic features, history and citations whenever semantic memory was disabled or auto-disabled, though the store still held them, and nothing recorded the skip. A project re-created under the same ids was later served the old features. Deletion now gates on SemanticMemoryConf.storage_configured(), which says whether the selected backend has its storage set, independent of whether semantic memory is served. It reaches the store through the deletion-only storage, removes the project's default sets, and refuses an empty set-id list, since an empty filter matches every feature. A store that was never initialized has nothing to delete and does not block the deletion. _cleanup_semantic_history, which logged and skipped when the service was not ready, is removed. Each deletion deletes what a retry needs last. Session deletion removes each episode batch's semantic history before the episodes and leaves the session Deleted on failure for the worker to retry. delete_episodes deletes the semantic history before the episodes. Resolving the storage up front catches an unreachable pgvector database, but not a cached Neo4j driver or vector store client, so the ordering carries the guarantee. The parametrized test_delete_session_clears_semantic_history_and_citations was not run locally: its fixture embeds through the OpenAI API and needs a real OPENAI_API_KEY. Fixes #1830 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
0609a37 to
76344ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76344accf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
delete_feature_set evaluated its filter twice: once to select the vector uuids and again to delete the rows. A row that matched only at the delete was committed as deleted before its vector, so a failed vector delete left a vector no retry could find, and a row that stopped matching in between lost its vector but kept its row. It now selects up to 1000 vector uuids by the filter, deletes those vectors, deletes exactly those rows by vector_uuid, and repeats until nothing matches. A row is never deleted before its own vector, and each statement binds at most 1000 parameters, matching the episode deletion batch and far below SQLite's limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
Deletion built its storage from the configuration on every call. The config API changes storage settings in place without rebuilding the semantic service, so after such a change deletion targeted a different backend from the one the process reads and writes, where before it went through the service. get_semantic_deletion_storage now returns the built service's storage when there is one, and builds from the configuration only when no service exists, which is the disabled case. Rebuilding semantic resources on a storage change is the config API's broader problem (#1758, #1752). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db008fd036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The deletion path skips startup() and so its Alembic migrations. On a store upgraded while semantic memory was disabled, it either found only the legacy prof table and treated the store as never initialized, or matched nothing under legacy set ids, and the deletion completed with the old semantic data still there. It now reads the database's Alembic revision without migrating: no semantic tables means never initialized, and a revision not at the script head raises ResourceNotReadyError, so the deletion is retried and logged instead of completing. The migrations still run only when semantic memory starts. The script head is read once per process. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Reviewed at 4575d15. The deletion path reads well: the storage is resolved once before anything is deleted, history goes before episodes, and vectors go before rows. One question.
semantic_schema_is_currentcompares the stored revision to head with==, so it also refuses a store that is past both hazards the tests pin (theproftable at001, legacy set ids beforeb65f7f4a9d2c) but behind a later migration, or ahead of head after a rollback. While semantic memory is disabled nothing migrates (apply_alembic_migrationsruns only fromSqlAlchemyPgVectorSemanticStorage.startup). So on apgvectororauto-on-Postgres deployment in the disabled period #1830 describes, the first upgrade that adds a semantic migration (the latest,e8d4a37b69f2, came with #1707) makes every project deletion fail and retry, and everydelete_episodescall fail, until semantic memory starts again. Is refusing a store that is already past both of those intended?
Verified: project deletion selects the same set ids as before. With no metadata, only tagless set types qualify, and they map to the same org and project defaults that project_deletion_set_ids returns. Neo4j deletes work without startup(): delete_feature_set and delete_history match on the set_id / history_id properties, not on the set labels or vector indexes that startup builds. The batch loop in VectorStoreSemanticStorage.delete_feature_set terminates, since vector_uuid is non-null and unique. alembic_pg is the repo's only Alembic environment, so the revision read can't pick up another component's alembic_version.
Deletion refused any pgvector store not at the migration head. While semantic memory is disabled nothing migrates, so the first release that added any semantic migration made every project and episode deletion fail until semantic memory started again, even when the new migrations only added an index or a nullable column the deletes never touch. Accept a store at or past b65f7f4a9d2c, the last migration that moves data. A store before it is still refused, since its data sits under the legacy table or set ids; so is a revision this code does not know, which a newer version wrote. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011szjDwQaudc8dvY4Xx2CgU
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@marvinyu-memverge Not intended; you're right. The deletes only touch |
|
I think long-term it may make more sense to remove enabling/disabling semantic memory as a configuration option at server run time. Instead, requests could be scoped to which memories they require. Any approach that allows changing the topology via run time configuration after deployment is harder to reason about and more fragile than simply routing requests correctly given a fixed topology. |
|
@edwinyyyu Agreed for the long term: a fixed topology with requests scoped to the memories they need is easier to reason about. The near term is different, though. The first stage of horizontal scaling runs with semantic memory disabled, so deployments that ran with it enabled before still hold semantic data, and project and episode deletion has to reach it. That's what this PR fixes. It's also compatible with the fixed-topology direction: deletion is keyed on whether semantic storage is configured, not on the enabled flag. Under that model, the build-from-config path used when no semantic service exists becomes unnecessary, since the service would always be built when its storage is configured; the deletion order and the migration check would still apply. |
Fixes #1830.
Problem
Project and episode deletion skipped semantic cleanup whenever
semantic_memory.enabledwas false, whether an operator disabled it or the config auto-disabled it. The store still held features, citations and history, nothing recorded the skip, and a project re-created under the same ids was later served the old features.Change
SemanticMemoryConf.storage_configured()(is the selected backend's storage set?) instead ofenabled.ResourceManagerImpl.get_semantic_deletion_storage, reaches the configured backend withoutstartup(), collection creation, or any model. When a semantic service is already built it uses that service's storage. It returnsNonefor a store that was never initialized, and raises for a pgvector store whose data migrations have not run, or that a newer version migrated, so legacy data is not silently missed; a store only missing later schema-only migrations is deleted from normally.SemanticSessionManager.project_deletion_set_idscomputes the project's sets from its ids alone. It is the same selection as before, now in one place.VectorStoreSemanticStorage.delete_feature_set, vectors before the rows that name them (in batches of 1000). A failure leaves the sessionDeletedfor the worker to retry, or returns the error fromdelete_episodes.Out of scope: which sets a project deletion should remove (#1732), rebuilding semantic resources when the config API changes storage (#1758, #1752), and a concurrent
update_featureracing a feature-set delete (exists with either delete order).Behaviour change for operators
With semantic memory disabled but its storage still set, deletion now contacts that store, and fails and retries if it cannot be reached. Unsetting
database(orfeature_storeandvector_collection) turns semantic cleanup off. The configuration docs say so.Testing
MemMachinedeletion paths. The ordering and race tests fail on the previous code.sqlite3cannot load extensions (macOS 3.12 and 3.13 CI runners), like the sqlite-vec store's own tests.test_delete_session_clears_semantic_history_and_citationsis parametrized overenabled. It was not run: its fixture needs a realOPENAI_API_KEY.uv run pytest packages: 2516 passed, 8 skipped. Ruff clean;tyat the base's 2 diagnostics. No server was run and no request went through the HTTP API.