fix(sessions): guard destructive deletes inside the store transaction - #124496
Closed
JoaoMarcos44 wants to merge 1 commit into
Closed
JoaoMarcos44 wants to merge 1 commit into
JoaoMarcos44 wants to merge 1 commit into
Conversation
Protect explicit session deletion at the SessionDB ownership boundary instead of preflighting leases in each UI surface. The opt-in guard runs under the same writer transaction as DELETE, covers turn leases, compression locks, and cascaded delegate children, and makes bulk deletion all-or-nothing. User-facing delete/prune paths opt in; internal cleanup keeps the existing default semantics. Refs NousResearch#123583 Related: NousResearch#123725
kshitijk4poor
added a commit
that referenced
this pull request
Sep 27, 2026
delete_session/delete_sessions cascade-delete delegate children, but the
write-guard check only looked at the root. A guarded delegate child could be
removed out from under its live turn, and in bulk delete an active id that
was also another selected root's delegate child was reported in
skipped_active while the cascade deleted it anyway.
Check {root, *delegate children} via a small _guarded_ids helper: single
delete refuses, bulk delete skips the root, so the cascade never touches a
guarded row. Ports the delegate-protection idea from #124496.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
|
Thanks @JoaoMarcos44. The session-delete guard landed on main via #125157 (230f89b), built on #123725's in-transaction approach. Closing this one as superseded. |
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.
What does this PR do?
Prevents user-facing session deletion from removing a row while a live turn or compression still owns it.
The recovery half of #123583 is already on
mainvia #124117: if a row disappears under a live agent, the agent recreates it and replays the transcript. This PR addresses the remaining entry-side race at the SessionDB delete boundary.Instead of preflighting a lease in each UI and then deleting in a second transaction,
delete_session()anddelete_sessions()gain an opt-inreject_active_write_guardsflag. When enabled, the existing transcript write guards are checked inside the same writer transaction as the DELETE.That closes the check/delete TOCTOU window and covers both:
The guard also covers recursive delegate children that the delete would cascade.
Related Issue
Refs #123583.
Agent-side recovery is already merged in #124117.
Type of Change
Root cause / ownership
The store already owns the authoritative in-transaction guard in
_check_transcript_write_guards().A surface-level sequence like:
cannot guarantee safety because a turn can acquire the lease between the read and the delete. The invariant has to be enforced under the same writer transaction that removes the row.
Changes Made
SessionDB.delete_session(..., reject_active_write_guards=True)SessionDB.delete_sessions(..., reject_active_write_guards=True)hermes sessions delete;sessions export --delete-after-verified;session.delete.Regression contract
tests/hermes_state/test_session_delete_write_guards.pycontains two store-level invariants using a real temporarySessionDB:The tests are deliberately at the store ownership boundary instead of duplicating the same lease logic in each UI test.
Relationship to existing work
@kshitijk4poor — #124117, building on @strzhao — #123641
Merged agent-side recovery for #123583. This PR does not duplicate it.
@shali10 — #123725
Addresses the same entry-side goal, but its current implementation checks
session_turn_lease_holder()outside the delete transaction.The review by @jonpol01 demonstrated concrete gaps in that approach:
This PR implements the ownership-boundary direction identified in that review: an opt-in guard inside
delete_session/delete_sessions, reusing the existing store invariant rather than creating another lease reader.No code from #123725 is copied.
Verification at
8f7c1b673ddd1fcb49a8d7e2020e5de847152c2bmain@c2af461706ae0dbab7271caf45bb9eefde4cdc53;mainduring implementation;Checklist
main.pytest tests/ -qrun not claimed.