fix(sessions): reply session init fails under rapid consecutive turns#99048
fix(sessions): reply session init fails under rapid consecutive turns#99048rushitgit wants to merge 3 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed July 2, 2026, 9:47 AM ET / 13:47 UTC. Summary PR surface: Source +2, Tests +103. Total +105 across 2 files. Reproducibility: yes. for the source path: current main retries one stale snapshot and then throws, while the PR and related reports show the same conflict family under rapid same-session turns. I did not run a fresh current-main live repro in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the bounded retry mitigation, but land it only after the regression coverage is tied to the production retry path and maintainers accept it as a companion to the broader session-conflict fixes. Do we have a high-confidence way to reproduce the issue? Yes for the source path: current main retries one stale snapshot and then throws, while the PR and related reports show the same conflict family under rapid same-session turns. I did not run a fresh current-main live repro in this read-only review. Is this the best way to solve the issue? No, not quite as submitted. The bounded retry is an acceptable mitigation with now-sufficient real-output proof, but the regression test should exercise or bind to the production retry loop before this is the best merge shape. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0ccdef5dcf76. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +2, Tests +103. Total +105 across 2 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Reply session initialization commits a revision-guarded write and retried exactly once on conflict before throwing "reply session initialization conflicted for <sessionKey>". Under bursts of rapid consecutive turns on the same session (e.g. several quick messages sent back to back before the prior turn's session-state write settles), a single retry was not always enough, surfacing the error directly to end users even though a subsequent attempt — reading a fresh, correctly-serialized snapshot — would very likely succeed. Raise the retry ceiling from 1 retry to a small bounded budget (4 total attempts) so this transient contention is absorbed transparently, while still throwing if the store is genuinely stuck after exhausting the budget. No change to locking, snapshot, or commit semantics — only the retry count. Co-authored-by: Cursor <[email protected]>
852355f to
00b8d39
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Exercises the real session-accessor commit/snapshot functions under an induced 3-writer revision-conflict burst, proving the old 2-attempt ceiling throws and the new 4-attempt ceiling absorbs the same race. Co-authored-by: Cursor <[email protected]>
openclaw-barnacle
Bot
added
size: S
triage: needs-pr-context
Remove non-essential explanatory comments from the retry-budget change and regression test. Co-authored-by: Cursor <[email protected]>
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
removed
the
triage: needs-pr-context
|
@clawsweeper re-review Additional real behavior proof, redacted. Pre-fix user-visible TUI proof:
Post-fix runtime proof on this PR head:
Observed post-fix result: Target error check: Focused code proof is also included in |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
|
Closing this as superseded by #98835, which has now landed as 826c84e. #98835 fixes the shared false-conflict predicate at the session-accessor commit boundary rather than increasing the retry budget: same-session metadata drift is accepted, while real session identity changes remain guarded. That head passed the real behavior proof lane plus the session-accessor boundary and full relevant hosted CI before merge. This PR was useful as a mitigation candidate, but it still had an open coverage finding and is no longer the best fix now that the root session initialization fix is on main. If a distinct rapid-turn conflict still reproduces on current main after 826c84e, please open or refresh an issue with current-main logs and behavior proof. Published package parity remains tracked separately in #98416. |
2 participants
What Problem This Solves
Rapid consecutive turns on the same reply session can hit
reply session initialization conflictedwhen the session-store revision advances between snapshot and commit more than once. The previous code retried one stale snapshot, then surfaced the error to the user.Change
Replace the boolean stale-snapshot retry flag with a bounded attempt counter. The retry budget is now 4 total attempts.
The snapshot, commit, locking, and final failure behavior are unchanged. This only gives transient same-session revision conflicts a small bounded retry window.
Evidence
Pre-fix TUI observation, redacted:
Post-fix runtime proof on this PR head:
Target error check for the post-fix run:
Added
src/auto-reply/reply/session-init-retry-budget.test.ts. It uses the real file-backedloadReplySessionInitializationSnapshotandcommitReplySessionInitializationfunctions and induces the same 3-writer revision-conflict burst againstagent:main:main.Checked locally:
npx tsc --noEmit -p tsconfig.core.jsonhas no new errors. The existingTS4058insrc/secrets/config-io.tsreproduces onmain.Notes
This is a small mitigation for transient same-session write races. It does not replace the related root-cause work in #98416 or #98835.