Conversation
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the deletion ordering or retry behavior is wrong, vector chunks and metadata can be removed in separate stores, leaving a partially deleted document after a failure. Reverting the change cannot restore records already deleted, although a later retry can clean up the remaining metadata.
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Reviewed at head c310295. The deletion order now removes vector/chunk storage before the SQLite transaction, so failures at index save, chunk deletion, or metadata deletion leave the document discoverable for an idempotent retry. The new regression suite exercises all three failure boundaries and restart persistence; local Windows/Python 3.12 run: 56 passed (7 pre-existing warnings). All repository CI checks are green, including Windows/macOS/Linux pytest, Python 3.10-3.14 smoke tests, CodeQL, format-check, and build.
Deleting a knowledge-base document currently commits removal of its
KBDocumentandKBMediarows before deleting its chunks and FAISS vectors. If index persistence or chunk deletion then fails, the request raises an error but the document has already disappeared from the document list. The remaining stored chunks have lost their metadata and the user cannot select the document in the dashboard to retry deletion.Modifications
Move
vec_db.delete_documents()before the metadata transaction. A storage failure keeps the document and media records discoverable; retrying completes the existing idempotent deletion. A later metadata-transaction failure also retains these records for retry, even when the chunks have already been removed.This is an ordering fix in the knowledge-base application workflow. It does not make the separate stores atomic, restore vectors after a partial failure, add automatic retries, or change FAISS internals. API schemas and normal successful deletion are unchanged.
Add parameterized tests using real temporary SQLite metadata/chunk databases and a real FAISS index. The embedding provider returns a fixed vector; faults are injected at index save, chunk deletion, and the document-row deletion inside the metadata transaction. The tests reopen the metadata database, retry deletion, check the other document's data remains, and reopen the vector store to verify persistence.
This was found by inspecting the current deletion path. Related #9120 handles media cleanup on successful single-document deletion; #9303 handles whole-knowledge-base cascades. Neither changes this single-document failure ordering.
Verification steps and test results
Windows, Python 3.12; base
bd046ed29914ee559e9bf47676ccb71a84f747ba.Before the production change, the index-save and chunk-delete regression cases both failed because the target document was absent after reopening the metadata database:
After the fix, including the metadata rollback/retry case:
The warnings include the existing
audioopdeprecation and aiosqlite worker-threadEvent loop is closedwarnings. They are not failed assertions. The full repository suite, Linux/macOS runs, live model calls, and browser-level interactions were not run locally.ruff format --check .,ruff check ., andgit diff --cached --checkpass. The new test file was also explicitly formatted and linted.Checklist
AI assistance
Codex assisted with code inspection, reproduction, implementation, validation, and this PR description. The tests and results above were executed locally; the change does not claim end-to-end atomicity across independent stores.
CI follow-up: synchronize watchdog tests
The first remote run passed on Ubuntu and Windows. On macOS, all three new knowledge-base tests passed, but
test_event_loop_watchdog_writes_rotating_logfailed because the sampled stack did not include the test coroutine (2886 passed, 1 failed). Its fixed 50ms blocking window can end before log opening and stack capture complete.A separate test-only commit replaces the fixed sleeps in the two watchdog dump tests with bounded
threading.Eventwaits andfinallycleanup. The rotation test still writes real stack traces and checks the original rotated file contents; it now specifically checks the event-loop thread's stack contains the test coroutine. Production watchdog behavior is unchanged.Local fault injection adding 80ms to the real log-open operation reproduces the original missing-test-frame assertion; the same probe passes with synchronization. The watchdog module plus the new knowledge-base tests pass together (
8 passed), and full Ruff formatting/lint checks pass. Remote CI on commitc3102950ffd4b919bf1a206e522a5dfbcfdb6dccis now complete: all 23 checks passed. The actual test steps on Ubuntu, Windows, and macOS all succeeded. Formatting, CodeQL, dashboard CI, and smoke tests also passed. Unit-test run.Summary by Sourcery
Make single-document knowledge-base deletion retryable by removing stored chunks before committing document metadata removal.
Bug Fixes:
CI:
Tests: