refactor(kb): create pages and mental models atomically - #3369
Open
Sanderhoff-alt wants to merge 1 commit into
Open
refactor(kb): create pages and mental models atomically#3369Sanderhoff-alt wants to merge 1 commit into
Sanderhoff-alt wants to merge 1 commit into
Conversation
Knowledge pages and their backing mental models share one lifecycle, but they were committed in separate transactions. A page insert failure could therefore leave an orphaned mental model. Extract mental-model embedding generation and insertion into typed helpers. Create the bank, mental model, and page through one connection and transaction, so any page failure rolls back all related writes. Preserve the duplicate-page contract by returning None only for uq_kp_folder_pagename. Add PostgreSQL and Oracle regression coverage for rollback after a real mental model insert, plus parent-validation and duplicate-name coverage.
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.
Summary
Create each knowledge page and its backing mental model in one database transaction so both records become visible together or both roll back.
Important
This PR and compensation PR #3370 are mutually exclusive alternatives for the same issue. Merge exactly one, not both. This atomic design is the stronger consistency option and has a larger refactoring surface.
Problem
A knowledge page stores its content in a backing mental model. The current flow commits the mental model first and inserts the page in a later transaction. If parent validation, the page insert, the database connection, or task execution fails after the first commit, the bank can retain an orphaned mental model with no knowledge page referencing it.
The intended invariant is: a page and its backing mental model either both exist or neither exists.
Design
Extract mental-model embedding generation and pinned-model insertion into typed internal helpers.
create_mental_modelcontinues to use those helpers, preserving its existing behavior, whilecreate_knowledge_pageuses the insertion helper through its own connection.Generate the embedding before opening the transaction, then create the bank if needed, validate the parent, insert the mental model, and insert the page through one connection and one transaction. This keeps model inference outside the transaction while making all database writes atomic.
Run the default-bank-template hook only after commit because the hook opens independent connections. This preserves the existing lazy bank creation behavior without allowing nested connections to escape the page transaction.
Failure semantics
Any exception or cancellation before commit rolls back the bank, mental model, and page writes made by this operation. No application-level cleanup is required.
Only a violation of
uq_kp_folder_pagenameis translated toNone, preserving the HTTP 409 duplicate-page contract. Other unique-constraint violations continue upward instead of being mislabeled as page-name conflicts.Compatibility
The shared insertion helper retains the existing search-vector behavior and the existing PostgreSQL and Oracle SQL adaptation path. The public API and returned page shape do not change.
Verification
memory_engine.py: passedORACLE_TEST_DSNTrade-offs
This design provides database-enforced atomicity across exceptions, cancellation, connection loss, and process termination. Its cost is a larger internal refactor because the existing mental-model insertion path must be reusable inside a caller-owned transaction.
PR #3370 keeps the existing transaction boundary and performs best-effort cleanup instead. It has a smaller production diff but cannot guarantee cleanup after process or database failure.