Skip to content

fix: #3346 clean up branch-only messages when deleting branches#3347

Open
Aphroq wants to merge 1 commit into
openai:mainfrom
Aphroq:fix/advanced-sqlite-delete-branch-orphans
Open

fix: #3346 clean up branch-only messages when deleting branches#3347
Aphroq wants to merge 1 commit into
openai:mainfrom
Aphroq:fix/advanced-sqlite-delete-branch-orphans

Conversation

@Aphroq
Copy link
Copy Markdown
Contributor

@Aphroq Aphroq commented May 11, 2026

Summary

This pull request fixes AdvancedSQLiteSession.delete_branch() leaving branch-only message rows behind after deleting a branch. After removing the deleted branch's message_structure rows, the method now cleans up message rows for the current session that are no longer referenced by any structure row, using the same lock and transaction while preserving messages still shared by main or other branches.

Test plan

  • uv run pytest -q tests/extensions/memory/test_advanced_sqlite_session.py::test_delete_branch_removes_branch_only_messages tests/extensions/memory/test_advanced_sqlite_session.py::test_branching_functionality tests/extensions/memory/test_advanced_sqlite_session.py::test_branch_deletion_with_force
  • uv run pytest -q tests/extensions/memory/test_advanced_sqlite_session.py
  • bash .agents/skills/code-change-verification/scripts/run.sh

Issue number

Closes #3346

Checks

  • I've added new tests (if relevant)
  • I've added/updated the relevant documentation
  • I've run make lint and make format
  • I've made sure tests pass

@github-actions github-actions Bot added bug Something isn't working feature:extensions labels May 11, 2026
@Aphroq Aphroq changed the title fix(sessions): clean up branch-only messages when deleting branches fix: #3346 clean up branch-only messages when deleting branches May 11, 2026
@Aphroq Aphroq force-pushed the fix/advanced-sqlite-delete-branch-orphans branch from c36a54e to 5732480 Compare May 11, 2026 12:44
@Aphroq
Copy link
Copy Markdown
Contributor Author

Aphroq commented May 11, 2026

Pushed follow-up commit to the PR branch with two extra boundary hardenings:

  • _cleanup_orphaned_messages_sync() now uses a session-scoped set-based DELETE instead of first materializing every orphan id in Python and building a long IN (...) parameter list. The delete remains scoped to the current session_id, and it preserves any message still referenced by message_structure for that session.
  • Added descendant branch coverage: if branch B inherits messages from branch A, deleting branch A does not remove messages still referenced by branch B.
  • Added large orphan cleanup coverage: cleaning many orphan rows uses a fixed number of SQL parameters, avoiding SQLite bind parameter limit issues.

@Aphroq
Copy link
Copy Markdown
Contributor Author

Aphroq commented May 11, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature:extensions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AdvancedSQLiteSession.delete_branch() leaves branch-only messages in the base table

1 participant