fix(memory-engine): compute update_memory_unit embeddings off the pooled connection - #3083
Merged
Merged
Conversation
…led connection update_memory_unit re-embedded inside the write transaction, so an interactive curation edit (or revert) held a pooled connection across the embedder call. This splits it into two phases against the pluggable memories store: - Phase 1 (no write transaction held): read the live/archived record, resolve the corrected entity set (idempotent find-or-create, autocommitted on a short connection), and read the canonical entity names — then compute the embedding with NO connection held. Carried into typed _MemoryEditPlan / _MemoryRevertPlan. - Phase 2 (short write transaction): re-read the row (skip cleanly if it was concurrently moved/deleted), reassert + link the resolved entities, apply the field edit / archive move through the store, and write the precomputed embedding. When entities are NOT part of the edit, the current linked names are re-read under the write lock and, only on a mismatch (a concurrent entity-only edit landed between the phases), the vector is re-embedded in-txn so it stays consistent with the committed unit_entities. The revert path re-embeds in-txn the same way if the surviving entity set differs from the Phase-1 snapshot. - Orphan reclaim: entities resolved (autocommitted) in Phase 1 whose edit did not durably apply — row concurrently invalidated, or Phase 2 raised — are reclaimed by forcing a graph-maintenance sweep in the finally block. The store's begin_txn/decide_txn cross-store write-group is preserved: it is opened inside the Phase-2 transaction and decided once Postgres commits. Follow-up to #3082 (which handled the consolidation + update_mental_model paths). Refs #2434 NOT YET VALIDATED against a live database — see PR description.
nicoloboschi
marked this pull request as ready for review
July 31, 2026 16:08
This was referenced Aug 5, 2026
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.
Problem
update_memory_unitre-embedded inside the write transaction (edit path and revert path), so an interactive curation edit held a pooled PostgreSQL connection checked out across the embedder call. Same saturation class as #3082 — the connection sits idle mid-embed while other work blocks onacquire.This is the follow-up promised in #3082, which fixed the two paths that didn't need to widen the read→write window (
update_mental_model+ consolidation).update_memory_unitis harder because #2917 rewrote it around the pluggable memories store (apply_edit/restore_memory+ thebegin_txn/decide_txncross-store write-group), so the embed-off-connection split has to be reconciled with that coordinator.Approach — two phases
Phase 1 (no write transaction held): read the live/archived record; for an entity-changing edit, resolve the corrected set (
resolve_entities_only, idempotent find-or-create, autocommitted on a short connection) and read its canonical names; for revert, read the archive'sentity_idssnapshot names. Then compute the embedding with no connection held, into a typed_MemoryEditPlan/_MemoryRevertPlan.Phase 2 (short write transaction):
return None) if it was concurrently moved/deleted between the phases.unit_entities. The revert path does the same if the surviving entity set differs from the Phase-1 snapshot.begin_txn/decide_txnwrite-group is opened inside the Phase-2 transaction and decided once Postgres commits.Orphan reclaim: entities resolved (and autocommitted) in Phase 1 whose edit didn't durably apply — row concurrently invalidated, or Phase 2 raised — are reclaimed by forcing a graph-maintenance sweep in the
finallyblock.Why no hard abort/retry
Unlike the original #2434 raw-SQL design (
SELECT … FOR UPDATE+ abort/retry), this relies on last-writer-wins for concurrent text/date edits — which is already the current single-transaction semantics — and only needs the bounded in-txn re-embed for the one case that would otherwise corrupt (embedding naming a stale entity set). This avoids threading a re-lock/abort/retry loop through the store's transaction coordinator. Open to switching to strict abort/retry if reviewers prefer a "never embed in-transaction" guarantee.Validation status
ruff+tyclean; module imports; the 23 existingtest_memory_curation.pycases (edit / revert / invalidate / entity-swap round-trips) collect and will exercise this end-to-end in CI on live PostgreSQL — they are the regression gate for this refactor.Refs #2434