fix: reject stale conversation turns - #162
Merged
Merged
Conversation
franciscojavierarceo
requested review from
bbrowning,
jiahuei,
leseb,
maralbahari,
noobHappylife,
qandrew and
tjtanaa
as code owners
August 2, 2026 14:36
Collaborator
|
@franciscojavierarceo can retarget to main? |
franciscojavierarceo
changed the base branch from
codex/issue-102-postgres-pool-config
to
main
August 3, 2026 14:13
franciscojavierarceo
force-pushed
the
codex/issue-102-conversation-version-check
branch
from
August 3, 2026 14:13
88da78c to
1aaff97
Compare
maralbahari
reviewed
Aug 3, 2026
| .await | ||
| .map_err(ExecutorError::Storage) | ||
| .map_err(|error| match error { | ||
| StorageError::ConversationConflict { .. } => ExecutorError::ConversationLocked, |
Collaborator
There was a problem hiding this comment.
converts StorageError::ConversationConflict { conversation_id } into the unit-like ExecutorError::ConversationLocked. Consequently, persist.rs (line 30) logs only a generic message, and Error::source() cannot reach the original typed error. This makes concurrent-write incidents impossible to correlate with a conversation. Preserve StorageError as a #[source] while keeping the client-safe response.
Capture the persisted conversation version during rehydration and verify it under the existing storage lock before appending. Return conversation_locked consistently across HTTP, SSE, and WebSocket when the history changes, with SQLite and PostgreSQL race coverage. Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
franciscojavierarceo
force-pushed
the
codex/issue-102-conversation-version-check
branch
from
August 3, 2026 16:07
1aaff97 to
228de23
Compare
maralbahari
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
conversation_lockederror over HTTP, SSE, and WebSocket, includinggenerate: falseprevious_response_idfork behavior and the existing unchecked storage APIThis builds on the PostgreSQL persistence foundation merged in #142.
This implements optimistic conflict detection at persistence time. A pre-inference distributed lease or queue that also suppresses external tool side effects remains separate policy work.
Test Plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo testpre-commit run --all-filespostgres_concurrent_conversation_writes_have_contiguous_sequencesagainst PostgreSQL 17postgres_optimistic_conversation_conflictagainst PostgreSQL 17postgres_lock_wait_is_bounded_without_blocking_other_conversationsagainst PostgreSQL 17