fix(opencode): handle duplicate message ID in append_message_to_session - #273
Closed
Million-mo wants to merge 2 commits into
Closed
fix(opencode): handle duplicate message ID in append_message_to_session#273Million-mo wants to merge 2 commits into
Million-mo wants to merge 2 commits into
Conversation
…on (#229) In the sync path (POST /message), the REST handler pre-stores the assistant message before the event bridge tries to store it with the same canonical ID (via _pending_message_ids). This caused ValueError: Duplicate message ID from the storage provider, repeated for every PartDeltaEvent (850+ errors per request in production logs). Fix: append_message_to_session now catches ValueError for duplicate message IDs and treats the write as idempotent (skip + debug log). The in-memory dict also skips duplicate appends. Test infrastructure fixes (conftest.py): - _mock_append_message: check for duplicate message IDs like real MemoryProvider (was: unconditional append) - _mock_route_message: pass message_id and set _pending_message_ids on session_pool_integration (was: drop message_id in **kwargs) - Pre-initialize _pending_message_ids/_pending_message_metadata as real dicts on the AsyncMock to avoid auto-created Mock attributes New tests (test_duplicate_message_id.py): - test_mock_append_message_raises_on_duplicate: verifies mock catches duplicates (meta-test for test infrastructure) - test_duplicate_assistant_message_does_not_raise: verifies the fix handles double-write gracefully - test_different_messages_both_stored: non-duplicate messages still work - test_user_message_then_assistant_message_no_error: mixed messages OK - test_route_message_sets_pending_message_ids: verifies mock passes message_id through to _pending_message_ids Closes #229
- Remove unused import (Any) and unused variable (event_bridge) - Sort imports (ruff I001) - Accept review suggestion: replace getattr with direct attribute access (msg.info.id instead of getattr(msg.info, 'id', None)) per codebase convention 'DO NOT USE getattr and hasattr'
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
2 participants
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
Fix the
ValueError: Duplicate message IDregression introduced by the D14_pending_message_idsmechanism (PR #237 onmain, PR #171 onfeat/dynamic-team-mode).Bug
In the sync path (
POST /message), the REST handler pre-stores the assistant message atmessage_routes.py:392before callingroute_message. The event bridge then tries to store the same message again with the same canonical ID (via_pending_message_ids), causingValueError: Duplicate message IDfrom the storage provider — repeated for everyPartDeltaEvent(850+ errors per request in production logs).Root Cause
The D14 fix made the event bridge reuse the REST handler's
assistant_msg_idbut didn't account for the REST handler already having stored the message.Fix
append_message_to_sessionnow catchesValueErrorfor duplicate message IDs and treats the write as idempotent (skip + debug log). The in-memory dict also skips duplicate appends.Test Infrastructure Fixes
The bug went undetected because the conftest mocks didn't match real behavior:
_mock_append_messageMemoryProvider_mock_route_messagemessage_idin**kwargsmessage_idto_pending_message_idsNew Tests (
test_duplicate_message_id.py)test_mock_append_message_raises_on_duplicate— verifies mock catches duplicates (meta-test)test_duplicate_assistant_message_does_not_raise— verifies fix handles double-write gracefullytest_different_messages_both_stored— non-duplicate messages still worktest_user_message_then_assistant_message_no_error— mixed messages OKtest_route_message_sets_pending_message_ids— verifies mock passes message_id throughTest Results
Closes #229