fix(kb): compensate mental models when page creation fails - #3370
Open
Sanderhoff-alt wants to merge 1 commit into
Open
fix(kb): compensate mental models when page creation fails#3370Sanderhoff-alt wants to merge 1 commit into
Sanderhoff-alt wants to merge 1 commit into
Conversation
Knowledge-page creation committed its mental model before inserting the page. Failures outside the existing duplicate-name path could leave an orphan. Add best-effort compensation for every page-insert failure. Shield cleanup from task cancellation, wait through repeated cancellation, and preserve the original page error when cleanup fails or cancels itself. Validate deterministic parent errors before model creation, while repeating the check inside the page transaction to observe current state. Keep the duplicate None result so HTTP callers continue to receive 409. Cover missing and invalid parents, ordinary failures, duplicate names, cleanup failures, and single or repeated task cancellation.
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
Remove a newly committed backing mental model when the subsequent knowledge-page insert fails, while preserving the original page error and existing duplicate-page behavior.
Important
This PR and atomic transaction PR #3369 are mutually exclusive alternatives for the same issue. Merge exactly one, not both. This compensation design minimizes production refactoring but provides best-effort rather than database-enforced consistency.
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. Cleanup existed only for the duplicate-page path, so parent errors, database errors, task cancellation, and other failures after model creation could leave an orphaned mental model.
The desired operational outcome is to remove the model whenever this process observes that page creation failed.
Design
Keep the existing
create_mental_modelpath and transaction boundary. Before model creation, reject deterministic missing-parent and page-as-parent errors to avoid a known partial write. Repeat parent validation in the page transaction so the insert still observes current database state.After the mental model commits, route duplicate conflicts, ordinary exceptions, and task cancellation through one compensation helper that deletes the newly created model.
Run deletion in a separate task and await it through
asyncio.shield. If the caller is cancelled again while cleanup is running, defer that cancellation until cleanup completes. Distinguish caller cancellation from the cleanup task cancelling itself so cleanup failure cannot replace the original page outcome.Failure semantics
For an ordinary page failure, log any cleanup failure and re-raise the original page exception. For duplicate page names, retain the existing
Noneresult so the HTTP layer returns 409 even if cleanup fails. For caller cancellation, complete the cleanup attempt and then propagate cancellation.Compensation is deliberately best-effort. A process crash, permanent database failure, or termination between the mental-model commit and cleanup can still leave an orphan. PR #3369 removes that residual window by using one transaction.
Compatibility
The implementation continues to call the existing public mental-model create and delete paths, preserving their database-specific behavior, lazy bank creation, authorization suppression, logging, and default-template handling. The public API and returned page shape do not change.
Verification
memory_engine.py: passedTrade-offs
This option changes fewer production lines and avoids refactoring mental-model insertion internals. The cost is a substantially more complex cancellation protocol and a consistency guarantee that depends on the application remaining alive and able to issue the compensating delete.
PR #3369 is the preferred alternative when database-enforced atomicity is worth the larger internal refactor.