From a8a31ea543cfd505a67dd9aaa550640fa0532214 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 May 2026 16:51:31 +0000 Subject: [PATCH 1/3] fix(sdk): prevent duplicate usage_id error when registering LLMs When an agent has multiple LLM objects with the same usage_id (e.g., both the agent and its condenser using separate LLM instances with the default usage_id='default'), _ensure_agent_ready() would fail with: ValueError: Usage ID 'default' already exists in registry The fix tracks newly-added usage_ids during the LLM registration loop, so subsequent LLMs with duplicate IDs are skipped rather than causing an error. Co-authored-by: openhands --- .../conversation/impl/local_conversation.py | 1 + tests/sdk/llm/test_llm_registry.py | 60 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index a6bb1ec60e..e8e2b13804 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -635,6 +635,7 @@ def _ensure_agent_ready(self) -> None: for llm in list(self.agent.get_all_llms()): if llm.usage_id not in registered: self.llm_registry.add(llm) + registered.add(llm.usage_id) self._agent_ready = True diff --git a/tests/sdk/llm/test_llm_registry.py b/tests/sdk/llm/test_llm_registry.py index 9933a835f9..b7c7775e3d 100644 --- a/tests/sdk/llm/test_llm_registry.py +++ b/tests/sdk/llm/test_llm_registry.py @@ -306,3 +306,63 @@ def test_llm_registry_does_not_reset_metrics_for_independent_llms(): # llm2 should have its own independent metrics assert llm2.metrics is not llm1.metrics assert llm2.metrics.accumulated_cost == 0.0 + + +def test_conversation_handles_duplicate_usage_id_llms(): + """Test conversation gracefully handles multiple LLM objects with same usage_id. + + This regression test verifies the fix for: + ValueError: Usage ID 'default' already exists in registry + + The bug occurred when an agent had both its own LLM and a condenser with a + separate LLM object (different Python objects but same usage_id='default'). + The _ensure_agent_ready method would try to add both to the registry, and + the second add would fail. + + The fix ensures we track newly-added usage_ids during the loop to skip + duplicates. + """ + import tempfile + + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + from openhands.sdk.conversation import Conversation + + # Create two separate LLM objects with the same default usage_id + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + # usage_id defaults to "default" + ) + condenser_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + # usage_id defaults to "default" - same as agent_llm + ) + + # Verify they are different objects with the same usage_id + assert id(agent_llm) != id(condenser_llm) + assert agent_llm.usage_id == condenser_llm.usage_id == "default" + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + agent = Agent(llm=agent_llm, condenser=condenser, tools=[]) + + # Verify agent has two LLM objects with the same usage_id + llms = list(agent.get_all_llms()) + assert len(llms) == 2 + assert llms[0].usage_id == llms[1].usage_id == "default" + + with tempfile.TemporaryDirectory() as tmpdir: + # Create conversation - this should not raise ValueError + convo = Conversation(agent=agent, workspace=tmpdir, persistence_dir=tmpdir) + + # Trigger _ensure_agent_ready by sending a message + # This previously raised: ValueError: Usage ID 'default' already exists + convo.send_message("Hello") + + # Verify only one LLM was registered (first one wins) + assert "default" in convo.llm_registry.list_usage_ids() + assert len(convo.llm_registry.list_usage_ids()) == 1 From e727887e650a0b9147f425d736a7c880912464e8 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 May 2026 17:05:13 +0000 Subject: [PATCH 2/3] fix(sdk): validate unique LLM usage_ids at agent construction time Instead of silently skipping duplicate LLMs during registry registration (which could cause confusion when LLMs have different settings), we now validate at Agent construction time that all reachable LLMs have unique usage_id values. This provides a clear error message guiding users to set distinct usage_id values, e.g.: LLM(model='gpt-4o', usage_id='agent') LLM(model='gpt-4o-mini', usage_id='condenser') The duplicate usage_id issue commonly occurs when an agent is deserialized from JSON, as Pydantic creates separate LLM objects for the agent and condenser even if they have the same configuration. Changes: - Add _validate_unique_llm_usage_ids validator to AgentBase - Add warning log in _ensure_agent_ready as defense-in-depth - Update tests to verify error at construction time Co-authored-by: openhands --- openhands-sdk/openhands/sdk/agent/base.py | 29 +++++++ .../conversation/impl/local_conversation.py | 15 +++- tests/sdk/llm/test_llm_registry.py | 81 +++++++++++++------ 3 files changed, 99 insertions(+), 26 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/base.py b/openhands-sdk/openhands/sdk/agent/base.py index dbd580e2d1..e5b60dbddc 100644 --- a/openhands-sdk/openhands/sdk/agent/base.py +++ b/openhands-sdk/openhands/sdk/agent/base.py @@ -304,6 +304,35 @@ def _decrypt_mcp_config(cls, data: Any, info: ValidationInfo) -> Any: return data + @model_validator(mode="after") + def _validate_unique_llm_usage_ids(self) -> AgentBase: + """Validate that all LLMs reachable from this agent have unique usage_ids. + + This prevents confusing behavior where multiple LLMs with the same usage_id + (e.g., agent LLM and condenser LLM both using 'default') would result in + only one being registered for metrics tracking. + """ + usage_id_to_llms: dict[str, list[LLM]] = {} + for llm in self.get_all_llms(): + usage_id_to_llms.setdefault(llm.usage_id, []).append(llm) + + duplicates = { + uid: llms for uid, llms in usage_id_to_llms.items() if len(llms) > 1 + } + if duplicates: + details = [] + for usage_id, llms in duplicates.items(): + models = ", ".join(llm.model for llm in llms) + details.append(f"usage_id='{usage_id}' used by: {models}") + raise ValueError( + f"Multiple LLMs share the same usage_id. Each LLM must have a " + f"unique usage_id for proper metrics tracking. Duplicates found: " + f"{'; '.join(details)}. Set distinct usage_id values, e.g.: " + f"LLM(model='gpt-4o', usage_id='agent'), " + f"LLM(model='gpt-4o-mini', usage_id='condenser')" + ) + return self + @model_serializer(mode="wrap") def _serialize_with_mcp_handling(self, handler, info: SerializationInfo): """Serialize the agent, handling mcp_config encryption/redaction. diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index e8e2b13804..948160847b 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -633,9 +633,18 @@ def _ensure_agent_ready(self) -> None: self.llm_registry.subscribe(self._state.stats.register_llm) registered = set(self.llm_registry.list_usage_ids()) for llm in list(self.agent.get_all_llms()): - if llm.usage_id not in registered: - self.llm_registry.add(llm) - registered.add(llm.usage_id) + if llm.usage_id in registered: + # Skip duplicates but warn - this shouldn't happen if agent + # was validated properly, but can occur with deserialized + # agents that have the same usage_id on multiple LLMs + logger.warning( + f"Skipping duplicate LLM registration for usage_id " + f"'{llm.usage_id}' (model={llm.model}). Consider using " + f"distinct usage_id values for each LLM." + ) + continue + self.llm_registry.add(llm) + registered.add(llm.usage_id) self._agent_ready = True diff --git a/tests/sdk/llm/test_llm_registry.py b/tests/sdk/llm/test_llm_registry.py index b7c7775e3d..3d8593009e 100644 --- a/tests/sdk/llm/test_llm_registry.py +++ b/tests/sdk/llm/test_llm_registry.py @@ -308,27 +308,25 @@ def test_llm_registry_does_not_reset_metrics_for_independent_llms(): assert llm2.metrics.accumulated_cost == 0.0 -def test_conversation_handles_duplicate_usage_id_llms(): - """Test conversation gracefully handles multiple LLM objects with same usage_id. +def test_agent_rejects_duplicate_usage_id_llms(): + """Test agent validation rejects duplicate usage_ids at construction time. - This regression test verifies the fix for: - ValueError: Usage ID 'default' already exists in registry + When an agent has multiple LLMs with the same usage_id (e.g., both the agent + LLM and condenser LLM using 'default'), validation should fail with a clear + error message guiding the user to set distinct usage_id values. - The bug occurred when an agent had both its own LLM and a condenser with a - separate LLM object (different Python objects but same usage_id='default'). - The _ensure_agent_ready method would try to add both to the registry, and - the second add would fail. + This is the proper fix for: + ValueError: Usage ID 'default' already exists in registry - The fix ensures we track newly-added usage_ids during the loop to skip - duplicates. + Rather than silently skipping duplicates during registration (which could + cause confusion if the LLMs have different settings), we now catch this + at agent construction time. """ - import tempfile - + import pytest from pydantic import SecretStr from openhands.sdk.agent import Agent from openhands.sdk.context.condenser import LLMSummarizingCondenser - from openhands.sdk.conversation import Conversation # Create two separate LLM objects with the same default usage_id agent_llm = LLM( @@ -337,7 +335,7 @@ def test_conversation_handles_duplicate_usage_id_llms(): # usage_id defaults to "default" ) condenser_llm = LLM( - model="gpt-4o", + model="gpt-4o-mini", # Different model! api_key=SecretStr("test-key"), # usage_id defaults to "default" - same as agent_llm ) @@ -348,21 +346,58 @@ def test_conversation_handles_duplicate_usage_id_llms(): # Create condenser with its own LLM condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should fail with a clear error message + with pytest.raises(ValueError) as exc_info: + Agent(llm=agent_llm, condenser=condenser, tools=[]) + + error_msg = str(exc_info.value) + assert "Multiple LLMs share the same usage_id" in error_msg + assert "usage_id='default'" in error_msg + assert "gpt-4o" in error_msg + assert "gpt-4o-mini" in error_msg + + +def test_agent_accepts_distinct_usage_id_llms(): + """Test agent accepts LLMs with distinct usage_ids.""" + import tempfile + + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + from openhands.sdk.conversation import Conversation + + # Create LLMs with distinct usage_ids + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + usage_id="agent", + ) + condenser_llm = LLM( + model="gpt-4o-mini", + api_key=SecretStr("test-key"), + usage_id="condenser", + ) + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should succeed agent = Agent(llm=agent_llm, condenser=condenser, tools=[]) - # Verify agent has two LLM objects with the same usage_id + # Verify both LLMs are discoverable llms = list(agent.get_all_llms()) assert len(llms) == 2 - assert llms[0].usage_id == llms[1].usage_id == "default" + usage_ids = {llm.usage_id for llm in llms} + assert usage_ids == {"agent", "condenser"} with tempfile.TemporaryDirectory() as tmpdir: - # Create conversation - this should not raise ValueError + # Create conversation - this should work fine convo = Conversation(agent=agent, workspace=tmpdir, persistence_dir=tmpdir) - - # Trigger _ensure_agent_ready by sending a message - # This previously raised: ValueError: Usage ID 'default' already exists convo.send_message("Hello") - # Verify only one LLM was registered (first one wins) - assert "default" in convo.llm_registry.list_usage_ids() - assert len(convo.llm_registry.list_usage_ids()) == 1 + # Verify both LLMs were registered + assert "agent" in convo.llm_registry.list_usage_ids() + assert "condenser" in convo.llm_registry.list_usage_ids() + assert len(convo.llm_registry.list_usage_ids()) == 2 From 497d439b3fc65ba443253f8224e6c841f0903129 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 May 2026 23:46:48 +0000 Subject: [PATCH 3/3] fix(sdk): only reject LLMs with same usage_id when configs differ The original validation was too strict - it rejected any agent with multiple LLMs sharing a usage_id, even when they had identical configurations. This broke the common pattern of sharing an LLM object between the agent and its condenser, because when the agent is serialized and restored from JSON, Pydantic creates separate LLM objects (even though they're identical). The new validation only raises an error when LLMs have the same usage_id but *different* configurations, which is the actual problematic case that would lead to confusing behavior. Cases that now work: 1. Sharing the same LLM object (deduped by object identity) 2. Deserialized agents with identical LLM configs (configs match) Cases that still error (with helpful message): 3. Different LLMs with same usage_id but different models/settings Co-authored-by: openhands --- openhands-sdk/openhands/sdk/agent/base.py | 52 ++++++++----- tests/sdk/llm/test_llm_registry.py | 90 +++++++++++++++++++---- 2 files changed, 108 insertions(+), 34 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/base.py b/openhands-sdk/openhands/sdk/agent/base.py index e5b60dbddc..e97da7fb4a 100644 --- a/openhands-sdk/openhands/sdk/agent/base.py +++ b/openhands-sdk/openhands/sdk/agent/base.py @@ -306,31 +306,43 @@ def _decrypt_mcp_config(cls, data: Any, info: ValidationInfo) -> Any: @model_validator(mode="after") def _validate_unique_llm_usage_ids(self) -> AgentBase: - """Validate that all LLMs reachable from this agent have unique usage_ids. - - This prevents confusing behavior where multiple LLMs with the same usage_id - (e.g., agent LLM and condenser LLM both using 'default') would result in - only one being registered for metrics tracking. + """Validate that LLMs with the same usage_id have the same configuration. + + When multiple LLM references share a usage_id, they must have identical + settings. This commonly happens when: + 1. An LLM object is intentionally shared (e.g., agent and condenser use + the same LLM) - this is fine, they're the same object + 2. An agent is deserialized from JSON, creating separate LLM objects from + what was originally a shared reference - this is fine if configs match + 3. Different LLMs are created with the same usage_id but different settings + (e.g., different models) - this is an error + + Only case 3 raises an error, as silently using one LLM config while + ignoring another would be confusing. """ usage_id_to_llms: dict[str, list[LLM]] = {} for llm in self.get_all_llms(): usage_id_to_llms.setdefault(llm.usage_id, []).append(llm) - duplicates = { - uid: llms for uid, llms in usage_id_to_llms.items() if len(llms) > 1 - } - if duplicates: - details = [] - for usage_id, llms in duplicates.items(): - models = ", ".join(llm.model for llm in llms) - details.append(f"usage_id='{usage_id}' used by: {models}") - raise ValueError( - f"Multiple LLMs share the same usage_id. Each LLM must have a " - f"unique usage_id for proper metrics tracking. Duplicates found: " - f"{'; '.join(details)}. Set distinct usage_id values, e.g.: " - f"LLM(model='gpt-4o', usage_id='agent'), " - f"LLM(model='gpt-4o-mini', usage_id='condenser')" - ) + # Check for conflicting LLM configurations with the same usage_id + for usage_id, llms in usage_id_to_llms.items(): + if len(llms) <= 1: + continue + + # Compare LLM configs (excluding usage_id itself) + # Use model_dump with redaction to compare functional equivalence + first_config = llms[0].model_dump(exclude={"usage_id"}) + for other_llm in llms[1:]: + other_config = other_llm.model_dump(exclude={"usage_id"}) + if first_config != other_config: + raise ValueError( + f"Multiple LLMs share usage_id='{usage_id}' but have " + f"different configurations. This would cause one LLM's " + f"settings to be silently ignored. Set distinct usage_id " + f"values for LLMs with different configs, e.g.: " + f"LLM(model='gpt-4o', usage_id='agent'), " + f"LLM(model='gpt-4o-mini', usage_id='condenser')" + ) return self @model_serializer(mode="wrap") diff --git a/tests/sdk/llm/test_llm_registry.py b/tests/sdk/llm/test_llm_registry.py index 3d8593009e..1f04e1c110 100644 --- a/tests/sdk/llm/test_llm_registry.py +++ b/tests/sdk/llm/test_llm_registry.py @@ -308,19 +308,15 @@ def test_llm_registry_does_not_reset_metrics_for_independent_llms(): assert llm2.metrics.accumulated_cost == 0.0 -def test_agent_rejects_duplicate_usage_id_llms(): - """Test agent validation rejects duplicate usage_ids at construction time. +def test_agent_rejects_conflicting_usage_id_llms(): + """Test agent validation rejects LLMs with same usage_id but different configs. - When an agent has multiple LLMs with the same usage_id (e.g., both the agent - LLM and condenser LLM using 'default'), validation should fail with a clear + When an agent has multiple LLMs with the same usage_id but different + configurations (e.g., different models), validation should fail with a clear error message guiding the user to set distinct usage_id values. - This is the proper fix for: - ValueError: Usage ID 'default' already exists in registry - - Rather than silently skipping duplicates during registration (which could - cause confusion if the LLMs have different settings), we now catch this - at agent construction time. + This catches the dangerous case where silently using one LLM config while + ignoring another would lead to unexpected behavior. """ import pytest from pydantic import SecretStr @@ -328,7 +324,7 @@ def test_agent_rejects_duplicate_usage_id_llms(): from openhands.sdk.agent import Agent from openhands.sdk.context.condenser import LLMSummarizingCondenser - # Create two separate LLM objects with the same default usage_id + # Create two separate LLM objects with the same usage_id but different models agent_llm = LLM( model="gpt-4o", api_key=SecretStr("test-key"), @@ -352,10 +348,76 @@ def test_agent_rejects_duplicate_usage_id_llms(): Agent(llm=agent_llm, condenser=condenser, tools=[]) error_msg = str(exc_info.value) - assert "Multiple LLMs share the same usage_id" in error_msg assert "usage_id='default'" in error_msg - assert "gpt-4o" in error_msg - assert "gpt-4o-mini" in error_msg + assert "different configurations" in error_msg + + +def test_agent_accepts_shared_llm_with_same_usage_id(): + """Test agent accepts when the same LLM is shared (e.g., with condenser). + + When an agent shares the same LLM object between itself and its condenser, + this should work fine since they're the same object. + """ + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + + # Create a single LLM object shared between agent and condenser + shared_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + ) + + # Create condenser sharing the agent's LLM + condenser = LLMSummarizingCondenser(llm=shared_llm, max_size=100) + + # Agent construction should succeed + agent = Agent(llm=shared_llm, condenser=condenser, tools=[]) + + # Verify only one LLM is discoverable (deduped by object identity) + llms = list(agent.get_all_llms()) + assert len(llms) == 1 + assert llms[0] is shared_llm + + +def test_agent_accepts_duplicate_llms_with_identical_config(): + """Test agent accepts multiple LLMs with same usage_id if configs match. + + When an agent is deserialized from JSON, Pydantic creates separate LLM + objects even if they were originally the same object. As long as their + configurations match, this should not raise an error. + """ + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + + # Simulate what happens during deserialization: two separate objects + # with identical configs + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + usage_id="shared", + ) + condenser_llm = LLM( + model="gpt-4o", # Same model + api_key=SecretStr("test-key"), # Same key + usage_id="shared", # Same usage_id + ) + + # Verify they are different objects + assert id(agent_llm) != id(condenser_llm) + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should succeed since configs match + agent = Agent(llm=agent_llm, condenser=condenser, tools=[]) + + # Verify both LLMs are discoverable (not deduped since different objects) + llms = list(agent.get_all_llms()) + assert len(llms) == 2 def test_agent_accepts_distinct_usage_id_llms():