fix: AdvancedSQLiteSession clear_session and pop_item metadata leaks

[okaditya84]

Summary

AdvancedSQLiteSession maintains the auxiliary message_structure and turn_usage tables in its add_items override, but it inherited clear_session and pop_item unchanged from SQLiteSession, which only touch the messages and sessions tables.

Those aux tables declare ON DELETE CASCADE foreign keys, but SQLite does not enforce foreign keys unless PRAGMA foreign_keys=ON is set, and the SDK never enables it (only WAL is configured). So the cascade never fires and the rows leak. Concretely:

• clear_session() left every message_structure and turn_usage row behind. list_branches, get_session_usage, and get_turn_usage kept reporting stale data, and because _insert_structure_metadata seeds from MAX(sequence_number)/MAX(user_turn_number), sequence and turn numbering stayed permanently offset for any items added after clearing (e.g. the next item got sequence_number=3, user_turn_number=2 instead of 1, 1).
• pop_item() deleted the globally highest-id message for the session — ignoring the current branch — and never removed the matching message_structure row, orphaning it and corrupting subsequent numbering. On a branched session it could also delete a message belonging to a different branch.

This is the same class of bug already fixed for the sibling methods delete_branch (#3347) and add_items atomicity (#3349); clear_session and pop_item were the remaining unpatched siblings.

Fix: override both methods so the metadata tables stay consistent, mirroring the existing delete_branch cleanup. pop_item now pops the most recent item of the current branch and reuses _cleanup_orphaned_messages_sync, so the underlying (branch-shared) message row is removed only when no other branch still references it. clear_session clears both aux tables in the same transaction and resets the in-memory branch pointer to main.

Test plan

Added three regression tests to tests/extensions/memory/test_advanced_sqlite_session.py:

• test_clear_session_removes_structure_and_usage_metadata — after clear_session, both aux tables are empty and numbering resets to (1, 1).
• test_pop_item_removes_its_structure_row — no message_structure row references a deleted message after a pop.
• test_pop_item_respects_current_branch_and_keeps_shared_messages — popping on a branch removes only that branch's item and leaves messages still referenced by another branch intact.

All three fail on main and pass with this change. Verified locally:

• make format / make lint — clean
• make typecheck — 0 errors (pyright + mypy)
• make tests — full suite: 4903 passed, 6 skipped

Issue number

N/A (bug fix; no existing issue — happy to open one if preferred).

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all verification steps pass
• If using Codex, I've run /review before submitting this PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 223fab9be0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Remove stale usage when popping a whole turn

When callers have stored usage for the latest branch turn and then pop_item() removes the last message_structure row for that turn, this path only deletes the structure row and orphaned message rows. The matching turn_usage row remains, so get_turn_usage() and get_session_usage() continue to report usage for a turn that no longer exists in the current branch; clear_session() and delete_branch() already clean this table, so this pop path should delete the turn's usage once no structure rows reference it.

Useful? React with 👍 / 👎.

[okaditya84]

Good call — fixed in 2c97250. pop_item now deletes the turn's turn_usage row once no message_structure rows reference that (branch, user_turn_number) anymore, so a fully-popped turn no longer shows up in get_turn_usage/get_session_usage, while a partially-popped turn keeps its usage. This matches the cleanup delete_branch and clear_session already do. Added a regression test covering both the partial and full-pop cases.

[Copilot]

Pull request overview

This pull request fixes metadata leaks and branch-incorrect behavior in AdvancedSQLiteSession by overriding inherited SQLiteSession methods that previously only modified the base messages/sessions tables. The change keeps message_structure and turn_usage consistent with item deletion/clearing, preventing stale analytics and sequence/turn-number drift.

Changes:

• Override AdvancedSQLiteSession.clear_session() to delete message_structure and turn_usage rows explicitly (not relying on SQLite FK cascades).
• Override AdvancedSQLiteSession.pop_item() to pop the latest item from the current branch and delete the corresponding message_structure row, then clean up orphaned message rows safely.
• Add regression tests ensuring metadata tables are cleared on clear_session() and remain non-orphaned / branch-correct after pop_item().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/agents/extensions/memory/advanced_sqlite_session.py	Overrides pop_item/clear_session to keep message_structure/turn_usage consistent and branch-correct without relying on SQLite FK enforcement.
tests/extensions/memory/test_advanced_sqlite_session.py	Adds regression coverage for metadata cleanup, correct branch popping, and preventing orphaned message_structure rows.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[okaditya84]

@seratch would you be able to take a look when you have a moment? 🙏

This fixes the two remaining AdvancedSQLiteSession methods (clear_session and pop_item) that still relied on SQLite FK cascades to clean up message_structure/turn_usage — the same class of fix already merged for delete_branch (#3347) and add_items (#3349). The automated Codex suggestion (stale turn_usage on a fully-popped turn) is addressed in 2c97250. Locally: make format/lint clean, make typecheck 0 errors, and the full test suite passes (4903 passed / 6 skipped). Happy to adjust anything.

[seratch]

Thanks for the contribution. The underlying clear_session() and branch-aware pop_item() bugs are valid, and overriding these methods in AdvancedSQLiteSession is the right implementation layer.

Before we can merge this, please make three focused changes: remove the matching turn_usage row when popping the final structure row for a branch turn; snapshot the branch before dispatching pop_item() to the worker and reset the branch to main inside the locked clear operation after commit; and extend the tests to actually pop a shared copied message and cover the branch-state interleaving or cancellation case.

These changes are needed to preserve metadata consistency and prevent stale operations from affecting surviving branch state. Once covered by focused regression tests, this should be ready for another review.

[okaditya84]

Thanks for the contribution. The underlying clear_session() and branch-aware pop_item() bugs are valid, and overriding these methods in AdvancedSQLiteSession is the right implementation layer.

Before we can merge this, please make three focused changes: remove the matching turn_usage row when popping the final structure row for a branch turn; snapshot the branch before dispatching pop_item() to the worker and reset the branch to main inside the locked clear operation after commit; and extend the tests to actually pop a shared copied message and cover the branch-state interleaving or cancellation case.

These changes are needed to preserve metadata consistency and prevent stale operations from affecting surviving branch state. Once covered by focused regression tests, this should be ready for another review.

Thanks for the review. I will do the respective changes and push right away.

[okaditya84]

Thanks for the detailed review @seratch! All three changes are done and pushed as focused commits:

1. Remove stale turn_usage when a branch turn is emptied — pop_item now deletes the turn's turn_usage row once no message_structure row references that (branch_id, user_turn_number) anymore; a partially-popped turn keeps its usage. (2c972502, also addresses the Codex suggestion.)

2. Snapshot the branch before dispatch / reset inside the locked clear —

   • pop_item reads _current_branch_id once at call time and uses that snapshot inside the worker, so a concurrent switch_to_branch() can't redirect an in-flight pop. (50d2b2f9)
   • clear_session resets _current_branch_id to main inside the locked operation, after commit, so the reset is atomic with the deletions and no other locked op sees the session cleared while the pointer still references a deleted branch. (f44364f2)

3. Extended tests (a1f9a5a4):

   • test_pop_item_deletes_shared_copied_message_only_when_unreferenced — pops a message copied (shared) into a branch, asserts it survives while another branch references it and is removed once unreferenced.
   • test_pop_item_uses_branch_snapshot_when_branch_switches_concurrently — interleaves a switch_to_branch after pop_item is dispatched and asserts the pop still targets the call-time branch.
   • test_pop_item_removes_turn_usage_only_when_turn_emptied and test_clear_session_resets_current_branch_to_main.

Verified locally: make format/make lint clean, make typecheck 0 errors, full suite 4907 passed / 6 skipped. Ready for another look whenever you have a moment — happy to adjust anything further.