fix: clean up orphaned messages in delete_branch and propagate add_items metadata errors#3498
fix: clean up orphaned messages in delete_branch and propagate add_items metadata errors#3498kimnamu wants to merge 1 commit into
Conversation
… error propagation - delete_branch() now removes messages that are no longer referenced by any branch after structure metadata deletion (openai#3346) - add_items() now raises RuntimeError when structure metadata insertion fails instead of silently swallowing the error (openai#3348) Closes openai#3346 Closes openai#3348
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659f368812
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Closing in favor of two focused, single-issue PRs to make review easier:
Both reuse the session's existing helpers and each ships with its own regression test. The |
Summary
Two correctness fixes for
AdvancedSQLiteSession, each with a regression test.#3346 —
delete_branch()removesmessage_structurerows but never cleans up theactual messages, leaking storage indefinitely. The fix reuses the existing
_cleanup_orphaned_messages_sync()(whichadd_items()already calls on failure)so there's no new deletion logic — just a missing call site.
#3348 —
add_items()catches_insert_structure_metadata()failures, runs cleanup,but then returns normally. Callers cannot distinguish success from partial failure.
The fix adds a
raise RuntimeError(...) from eafter the existing cleanup block (3 lines).Approach
Both fixes reuse existing infrastructure rather than introducing new code:
self._cleanup_orphaned_messages_sync(conn)after structure deletionLEFT JOIN ... IS NULLpattern — no new query logic neededraise RuntimeError(...) from eafter the existingexceptcleanup blockBefore / After
delete_branch("feat")with branch-only messagesmessage_structuredeleted, orphan rows stay inmessages_tableforever_cleanup_orphaned_messages_sync()add_items([...])when metadata failsNone— caller assumes successRuntimeErrorwith cause chain; caller can handle or retryTest plan
test_delete_branch_cleans_up_orphaned_messages: creates branch-only messages, deletes branch, assertsmessages_tableis cleantest_add_items_raises_on_structure_metadata_failure: monkeypatches metadata insertion to fail, assertsRuntimeErrorand verifies orphan cleanup ranIssue number
Closes #3346
Closes #3348
Checks
make lintandmake format