Skip to content

fix(opencode): handle duplicate message ID in append_message_to_session - #244

Merged
Leoyzen merged 2 commits into
mainfrom
fix/duplicate-message-id
Jul 20, 2026
Merged

fix(opencode): handle duplicate message ID in append_message_to_session#244
Leoyzen merged 2 commits into
mainfrom
fix/duplicate-message-id

Conversation

@Leoyzen

@Leoyzen Leoyzen commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix the ValueError: Duplicate message ID regression introduced by the D14 _pending_message_ids mechanism (PR #237 on main, PR #171 on feat/dynamic-team-mode).

Bug

In the sync path (POST /message), the REST handler pre-stores the assistant message at message_routes.py:392 before calling route_message. The event bridge then tries to store the same message again with the same canonical ID (via _pending_message_ids), causing ValueError: Duplicate message ID from the storage provider — repeated for every PartDeltaEvent (850+ errors per request in production logs).

Root Cause

REST handler (message_routes.py:392)     Event bridge (opencode_event_bridge.py:449)
├─ append_message_to_session(ID=A) ✅    ├─ _pending_message_ids.pop() → ID=A
└─ route_message(message_id=A)           └─ append_message_to_session(ID=A) 💥 Duplicate

The D14 fix made the event bridge reuse the REST handler's assistant_msg_id but didn't account for the REST handler already having stored the message.

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

The bug went undetected because the conftest mocks didn't match real behavior:

Mock Before After
_mock_append_message Unconditional append Check duplicate IDs like real MemoryProvider
_mock_route_message Dropped message_id in **kwargs Pass message_id to _pending_message_ids

New 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 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

Test Results

tests/servers/opencode_server/test_duplicate_message_id.py: 5 passed
tests/servers/opencode_server/ (full suite): 674 passed, 0 failed

Closes #229

…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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces graceful handling of duplicate message IDs in append_message_to_session by catching ValueError and treating duplicate writes as idempotent, preventing errors in the sync path where the REST handler pre-stores assistant messages. It also updates the test mocks and adds unit tests to verify this behavior. The reviewer suggested removing defensive None checks (via getattr) when checking for duplicate messages in memory, as the message ID fields are non-optional and guaranteed to be initialized.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +151 to +154
msg_id = getattr(msg.info, "id", None)
already_in_memory = msg_id is not None and any(
getattr(m.info, "id", None) == msg_id for m in messages[session_id]
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid adding defensive None checks (guards) for fields or attributes that are typed as non-optional (non-nullable) and guaranteed to be initialized, to maintain consistency with codebase conventions and avoid redundant code.

Since msg.info and msg.info.id are non-optional and guaranteed to be initialized on MessageWithParts, we can access them directly instead of using getattr with default values.

Suggested change
msg_id = getattr(msg.info, "id", None)
already_in_memory = msg_id is not None and any(
getattr(m.info, "id", None) == msg_id for m in messages[session_id]
)
msg_id = msg.info.id
already_in_memory = any(
m.info.id == msg_id for m in messages[session_id]
)
References
  1. Avoid adding defensive None checks (guards) for fields or attributes that are typed as non-optional (non-nullable) and guaranteed to be initialized, to maintain consistency with codebase conventions and avoid redundant code.

- 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'
@Leoyzen
Leoyzen merged commit 7b02c89 into main Jul 20, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Sub-issue E] Message Loss & RunLoop Lifecycle

1 participant