fix: make session mutations atomic - #4212
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31bd54eb7f
ℹ️ 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".
31bd54e to
9d49724
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d49724abc
ℹ️ 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".
| if transaction_succeeded: | ||
| committed = True | ||
| return |
There was a problem hiding this comment.
Preserve Redis cancellation after committed writes
When RedisSession.add_items() is cancelled after EXEC has succeeded, or while the pipeline reset is still finishing, this branch marks the mutation committed and returns normally instead of re-raising the recorded CancelledError. The write can still be treated as successful to avoid retrying the batch, but the cancelled caller should observe cancellation after cleanup; otherwise a cancelled Runner.run() using Redis-backed sessions can continue as if it was never cancelled.
Useful? React with 👍 / 👎.
This pull request fixes mutation atomicity across the built-in persistent Session backends instead of landing separate backend-specific fixes.
Issue #4202 and PR #4203 demonstrated that failed SQLite writes can leave a partial transaction and stranded write lock. Issue #4205 and PR #4206 demonstrated that concurrent
SQLAlchemySession.pop_item()calls can return the same item even though only one caller deleted it. Both are instances of the same Session contract violation: a mutation must either complete exactly once or leave the previously committed history unchanged.The change:
There are no public API changes. Existing MongoDB records remain readable, configured storage names and expiration behavior are preserved, and injected backend clients remain caller-owned.
This pull request resolves #4202 and #4205. It incorporates and supersedes #4203 and #4206.
Test plan
env UV_DEFAULT_INDEX=https://pypi.org/simple bash .agents/skills/code-change-verification/scripts/run.sh.Issue number
Closes #4202
Closes #4205
Supersedes #4203
Supersedes #4206
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR