Lineage Phase B3h (OSS) — retention + playbook-delete atomicity (final delete-path sweep)#198
Merged
Conversation
…on-atomic via commit=False; non-committing vec deletes; lock-wrapped)
…t single-record and bulk-by-status deletions - delete_user_playbook: move _fts_delete/_vec_delete after row delete (call _delete_playbook_search_rows with commit=False inside the lock, before final conn.commit) — prevents crash-window where FTS row is durably gone before the row delete commits - delete_agent_playbook: same fix, mirroring the user pattern - delete_all_user_playbooks_by_status: bring snapshot (_fetchall), row delete, and search cleanup into one lock+single commit; snapshot was previously outside the lock (race) and FTS cleanup committed before the row delete - add contract test asserting row+FTS (+vec when available) are gone and a non-matching playbook survives after delete_all_user_playbooks_by_status
…k reentrancy for nested-lock callers
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a ChangesPlaybook search-index cleanup and transaction control
Sequence Diagram(s)sequenceDiagram
participant Caller as delete_*_playbook<br/>_retention_delete_dependencies
participant Lock as self._lock
participant DB as SQLite<br/>(playbooks/FTS/vec)
participant Helper as _delete_playbook<br/>_search_rows<br/>(commit=False)
Caller->>Lock: acquire
Caller->>DB: DELETE FROM playbooks
Caller->>Helper: call(ids, commit=False)
Helper->>Lock: re-acquire (re-entrant)
Helper->>DB: chunked DELETE FROM *_fts
Helper->>DB: chunked DELETE FROM *_vec (if available)
Helper-->>Caller: return (no commit)
Caller->>DB: conn.commit()
Caller->>Lock: release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/storage/sqlite_storage/_playbook.py (1)
358-373:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd rollback guards around the new deferred-commit delete blocks.
These paths now rely on one final commit, but an exception before that commit can leave partial row/lineage/search-index changes pending on the shared connection. Roll back inside the same lock before re-raising.
Suggested pattern
with self._lock: - cur = self.conn.execute(...) - ... - self._delete_playbook_search_rows("user", [user_playbook_id], commit=False) - self.conn.commit() + try: + cur = self.conn.execute(...) + ... + self._delete_playbook_search_rows("user", [user_playbook_id], commit=False) + self.conn.commit() + except Exception: + self.conn.rollback() + raiseAlso applies to: 524-535, 890-907
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/_playbook.py` around lines 358 - 373, The delete_user_playbook method (and the other similar methods at lines 524-535 and 890-907) perform multiple operations that defer the final commit, but lack exception handling for failures that occur between the initial delete and the final commit call. Wrap the operations that occur after acquiring the lock in a try-except block, ensuring that if any exception is raised during the delete operations or search index cleanup, self.conn.rollback() is called within the same lock before re-raising the exception to prevent partial changes from being committed to the database.
🧹 Nitpick comments (1)
tests/server/services/storage/test_storage_contract_retention.py (1)
225-322: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover the retention vec cleanup branch too.
These retention tests exercise the new helper’s FTS path, but not the
_has_sqlite_vecpath foruser_playbooks_vec/agent_playbooks_vec. Add conditional vec assertions like the bulk-delete test so regressions in the vec table-name or chunk-delete path are caught when sqlite-vec is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/test_storage_contract_retention.py` around lines 225 - 322, The retention tests test_retention_user_playbook_delete_cleans_fts and test_retention_agent_playbook_delete_cleans_fts currently only verify FTS table cleanup but do not verify vec table cleanup for user_playbooks_vec and agent_playbooks_vec tables. Add conditional assertions (similar to those in the bulk-delete tests) after the delete_oldest_retention_target_rows call in both test functions to verify that vec rows are properly cleaned up when sqlite-vec is available. Check that the two oldest vec entries are removed and the surviving entry's vec row remains by using the same pattern as the FTS assertions but querying the appropriate vec tables instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 757-764: The retention transaction block containing the
self._delete_playbook_search_rows and
self._delete_source_windows_for_agent_playbook_ids method calls uses deferred
commits (commit=False), but lacks proper error handling. If any of these delete
operations or subsequent operations fail, the pending deletes will remain
uncommitted and could be inadvertently committed by a later operation. Wrap the
entire retention transaction block in a try/except statement that catches
exceptions, calls self.conn.rollback() to roll back any pending deferred
commits, and then re-raises the exception to preserve the original error
behavior.
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 524-535: The code captures user_playbook_ids via SELECT before
executing the DELETE statement, but another SQLiteStorage instance can modify
the database between these operations, causing a mismatch between the selected
IDs and the rows actually deleted. Instead of passing the pre-snapshot ids list
to the _delete_playbook_search_rows call on line 534, use the rowcount property
from the DELETE cursor (cur.rowcount) to determine which rows were actually
deleted, or restructure the code to use an atomic pattern like DELETE ...
RETURNING (if supported by SQLite) to ensure the IDs passed to
_delete_playbook_search_rows match the exact rows that were removed.
---
Outside diff comments:
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 358-373: The delete_user_playbook method (and the other similar
methods at lines 524-535 and 890-907) perform multiple operations that defer the
final commit, but lack exception handling for failures that occur between the
initial delete and the final commit call. Wrap the operations that occur after
acquiring the lock in a try-except block, ensuring that if any exception is
raised during the delete operations or search index cleanup,
self.conn.rollback() is called within the same lock before re-raising the
exception to prevent partial changes from being committed to the database.
---
Nitpick comments:
In `@tests/server/services/storage/test_storage_contract_retention.py`:
- Around line 225-322: The retention tests
test_retention_user_playbook_delete_cleans_fts and
test_retention_agent_playbook_delete_cleans_fts currently only verify FTS table
cleanup but do not verify vec table cleanup for user_playbooks_vec and
agent_playbooks_vec tables. Add conditional assertions (similar to those in the
bulk-delete tests) after the delete_oldest_retention_target_rows call in both
test functions to verify that vec rows are properly cleaned up when sqlite-vec
is available. Check that the two oldest vec entries are removed and the
surviving entry's vec row remains by using the same pattern as the FTS
assertions but querying the appropriate vec tables instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83d85ca3-cfaf-41d0-b329-8fa54026ef56
📒 Files selected for processing (3)
reflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_playbook.pytests/server/services/storage/test_storage_contract_retention.py
…y-ids consistency in delete_all_user_playbooks_by_status Fix 1 (_base.py _retention_perform_delete): wrap the dependency+target delete block in try/except and call self.conn.rollback() on any exception so uncommitted writes on the shared connection cannot be flushed by a subsequent unrelated commit. Fix 2 (_playbook.py delete_all_user_playbooks_by_status): change the DELETE to target the snapshot id set captured by the preceding SELECT (DELETE ... WHERE user_playbook_id IN (...)) instead of re-evaluating the WHERE predicate. This makes the DELETE and the subsequent _delete_playbook_search_rows cleanup consistent when two connections modify the same table between the SELECT and DELETE. Add tests/server/services/storage/test_retention_rollback_integration.py: two integration tests that inject failures into _retention_delete_target_rows and _retention_delete_dependencies and assert the connection is fully rolled back (rows and FTS index intact).
This was referenced Jul 1, 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
1 participant
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.
Lineage Phase B3h (OSS) — retention-cascade + playbook-delete atomicity (final delete-path sweep)
The last of the delete-path search-index-cleanup sweep started in B3d/B3e/B3f/B3g. OSS/SQLite-only (enterprise/Postgres deletes are transactional). Closes the remaining self-committing-helper-in-atomic-block and search-before-row-delete cases.
Core fix —
_delete_playbook_search_rowsis the last self-committing dependency helperIt used a per-id self-committing
_vec_deleteloop, which broke the retention cascade's "one critical section" atomicity (_retention_perform_delete). Rewrote it as(_kind, ids, *, commit=True):_delete_in_chunksfor both fts and vec (vec guarded on_has_sqlite_vec),with self._lock:,if commit: conn.commit().The 2 retention call sites pass
commit=False(atomic with the block's single commit); the 8 playbook callers keep the defaultcommit=True(also fixes a latent gap where, without sqlite-vec, the fts deletes were previously left uncommitted by this helper).Caller ordering bugs fixed (caught in review)
delete_all_user_playbooks_by_status— deleted+committed the search rows BEFORE deleting the playbook rows (separate statement), and snapshotted ids via_fetchalloutside the lock. Now: snapshot + row DELETE +_delete_playbook_search_rows(commit=False)in ONEwith self._lock:+ single commit; returns the real rowcount.delete_user_playbook/delete_agent_playbook(single-record) — same search-before-row ordering via self-committing_fts_delete/_vec_delete. Now atomic: row DELETE → rowcount-guarded hard_delete lineage emit →_delete_playbook_search_rows(commit=False)→ one commit. Audit-only-on-commit preserved.The other retention dependency helpers (
_delete_source_windows_*,_delete_optimizer_*) were verified already non-committing — untouched.Tests
New retention tests (user/agent playbook fts+vec cleaned within the atomic block) + a guard test for
delete_all_user_playbooks_by_status(matched rows+fts+vec gone; non-matching survivor intact). 45 storage/retention tests pass. Production file pyright-clean.Completes the delete-path atomicity class across profiles (B3c/B3f), interactions (B3d), retention interaction/profile helpers (B3e), and now playbooks + the retention playbook helper (B3h). Independent OSS PR; enterprise OSS-pin lags this SQLite-only fix harmlessly.
Summary by CodeRabbit
Bug Fixes
Tests