Uh oh!
There was an error while loading. Please reload this page.
fix: settle conversation deletion before propagating cancellation - #4790
fix: settle conversation deletion before propagating cancellation#4790Excelius-Wang wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:863061ec0d
ℹ️ 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".
| await self._openai_client.conversations.delete( | ||
| conversation_id=session_id, | ||
| ) | ||
| self._session_id = None |
There was a problem hiding this comment.
If another task assigns a replacement ID through the public session_id setter after clear_session() is cancelled but before the remote deletion finishes, the surviving delete task unconditionally resets that replacement to None. The next session operation then creates another conversation instead of using the caller's replacement; compare _session_id with the captured session_id before clearing it, or serialize setter updates with the same lock.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for catching this. The settled delete now clears the local ID only when it still matches the conversation that was deleted, so a replacement installed through the public session_id setter survives. I also added a controlled cancellation regression test that rebinds the session before deletion completes and verifies that the next public write uses the replacement without creating another conversation.
Validation for the updated diff: the focused session/import suite passes 95 tests, targeted Ruff and mypy pass, and make tests-review passes 9,345 tests with 33 skipped. The required wrapper's format, lint, and typecheck stages pass; its default test run reached 9,288 passed and 29 skipped with four unrelated review_optional AdvancedSQLiteSession multiprocessing timing failures, whose complete five-case parameter set passes serially.
Excelius-Wang
commented
Aug 31, 2026
@codex review |
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
This pull request fixes a cancellation race in
OpenAIConversationsSession.clear_session(). If caller cancellation arrives after the Conversations API commits deletion but before the response is received, the method now settles the in-flight delete before re-raising cancellation. This prevents later session operations from reusing a deleted conversation ID and receiving a 404.If an application installs a replacement through the released public
session_idsetter while deletion is settling, cleanup now clears the local ID only when it still matches the conversation that was deleted. The replacement therefore survives and is used by the next public session operation.The fix moves the existing cancellation-safe
_await_mutationhelper to the neutral session module, preserves the existing SQLite import path through an explicit re-export, and adds controlled regression coverage for both lazy reinitialization and public setter replacement.Test plan
make tests-review: 9,345 passed, 33 skippedmake format,make lint, andmake typecheckpassedmake tests: 9,288 passed and 29 skipped; four unrelatedreview_optionalAdvancedSQLiteSession multiprocessing timing assertions failed under host scheduling, and the complete affected five-case parameter set passed seriallyIssue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR