Skip to content

Commit e5a7efe

Browse files
Fix Laminar span stack warning in LocalConversation (#1039)
Co-authored-by: openhands <openhands@all-hands.dev>
1 parent 703718d commit e5a7efe

File tree

5 files changed

+329
-19
lines changed

5 files changed

+329
-19
lines changed

openhands-sdk/openhands/sdk/conversation/base.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@
99
from openhands.sdk.conversation.types import ConversationCallbackType, ConversationID
1010
from openhands.sdk.llm.llm import LLM
1111
from openhands.sdk.llm.message import Message
12+
from openhands.sdk.observability.laminar import (
13+
end_active_span,
14+
should_enable_observability,
15+
start_active_span,
16+
)
1217
from openhands.sdk.security.confirmation_policy import (
1318
ConfirmationPolicyBase,
1419
NeverConfirm,
@@ -76,6 +81,25 @@ class BaseConversation(ABC):
7681
exchange, execution control, and state management.
7782
"""
7883

84+
def __init__(self) -> None:
85+
"""Initialize the base conversation with span tracking."""
86+
self._span_ended = False
87+
88+
def _start_observability_span(self, session_id: str) -> None:
89+
"""Start an observability span if observability is enabled.
90+
91+
Args:
92+
session_id: The session ID to associate with the span
93+
"""
94+
if should_enable_observability():
95+
start_active_span("conversation", session_id=session_id)
96+
97+
def _end_observability_span(self) -> None:
98+
"""End the observability span if it hasn't been ended already."""
99+
if not self._span_ended and should_enable_observability():
100+
end_active_span()
101+
self._span_ended = True
102+
79103
@property
80104
@abstractmethod
81105
def id(self) -> ConversationID: ...

openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,7 @@
2626
from openhands.sdk.llm import LLM, Message, TextContent
2727
from openhands.sdk.llm.llm_registry import LLMRegistry
2828
from openhands.sdk.logger import get_logger
29-
from openhands.sdk.observability.laminar import (
30-
end_active_span,
31-
observe,
32-
should_enable_observability,
33-
start_active_span,
34-
)
29+
from openhands.sdk.observability.laminar import observe
3530
from openhands.sdk.security.confirmation_policy import (
3631
ConfirmationPolicyBase,
3732
)
@@ -144,10 +139,10 @@ def _default_callback(e):
144139
secret_values: dict[str, SecretValue] = {k: v for k, v in secrets.items()}
145140
self.update_secrets(secret_values)
146141

142+
super().__init__() # Initialize base class with span tracking
147143
self._cleanup_initiated = False
148144
atexit.register(self.close)
149-
if should_enable_observability():
150-
start_active_span("conversation", session_id=str(desired_id))
145+
self._start_observability_span(str(desired_id))
151146

152147
@property
153148
def id(self) -> ConversationID:
@@ -306,7 +301,7 @@ def run(self) -> None:
306301
# Re-raise with conversation id for better UX; include original traceback
307302
raise ConversationRunError(self._state.id, e) from e
308303
finally:
309-
end_active_span()
304+
self._end_observability_span()
310305

311306
def set_confirmation_policy(self, policy: ConfirmationPolicyBase) -> None:
312307
"""Set the confirmation policy and store it in conversation state."""
@@ -389,7 +384,7 @@ def close(self) -> None:
389384
return
390385
self._cleanup_initiated = True
391386
logger.debug("Closing conversation and cleaning up tool executors")
392-
end_active_span()
387+
self._end_observability_span()
393388
for tool in self.agent.tools_map.values():
394389
try:
395390
executable_tool = tool.as_executable()

openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,7 @@
2828
)
2929
from openhands.sdk.llm import LLM, Message, TextContent
3030
from openhands.sdk.logger import get_logger
31-
from openhands.sdk.observability.laminar import (
32-
end_active_span,
33-
observe,
34-
should_enable_observability,
35-
start_active_span,
36-
)
31+
from openhands.sdk.observability.laminar import observe
3732
from openhands.sdk.security.confirmation_policy import (
3833
ConfirmationPolicyBase,
3934
)
@@ -438,6 +433,7 @@ def __init__(
438433
which agent/conversation is speaking.
439434
secrets: Optional secrets to initialize the conversation with
440435
"""
436+
super().__init__() # Initialize base class with span tracking
441437
self.agent = agent
442438
self._callbacks = callbacks or []
443439
self.max_iteration_per_run = max_iteration_per_run
@@ -513,8 +509,7 @@ def __init__(
513509
secret_values: dict[str, SecretValue] = {k: v for k, v in secrets.items()}
514510
self.update_secrets(secret_values)
515511

516-
if should_enable_observability():
517-
start_active_span("conversation", session_id=str(self._id))
512+
self._start_observability_span(str(self._id))
518513

519514
@property
520515
def id(self) -> ConversationID:
@@ -653,7 +648,7 @@ def close(self) -> None:
653648
except Exception:
654649
pass
655650

656-
end_active_span()
651+
self._end_observability_span()
657652

658653
try:
659654
self._client.close()
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
"""Test for the span double-ending issue in LocalConversation."""
2+
3+
import logging
4+
import tempfile
5+
from unittest.mock import patch
6+
7+
import pytest
8+
from pydantic import SecretStr
9+
10+
from openhands.sdk.agent import Agent
11+
from openhands.sdk.conversation.impl.local_conversation import LocalConversation
12+
from openhands.sdk.llm import LLM
13+
14+
15+
def create_test_agent() -> Agent:
16+
"""Create a test agent."""
17+
llm = LLM(model="gpt-4o-mini", api_key=SecretStr("test-key"), usage_id="test-llm")
18+
return Agent(llm=llm, tools=[])
19+
20+
21+
def test_no_double_span_ending_warning(caplog):
22+
"""Test that LocalConversation doesn't produce double span ending warnings."""
23+
24+
# Create test agent
25+
agent = create_test_agent()
26+
27+
# Create a temporary workspace
28+
with tempfile.TemporaryDirectory() as temp_dir:
29+
# Create conversation
30+
conversation = LocalConversation(
31+
agent=agent,
32+
workspace=temp_dir,
33+
visualize=False, # Disable visualization to simplify test
34+
)
35+
36+
# Capture logs at WARNING level
37+
with caplog.at_level(logging.WARNING):
38+
# Mock the agent.step to raise an exception to trigger the finally block
39+
with patch(
40+
"openhands.sdk.agent.agent.Agent.step",
41+
side_effect=Exception("Test exception"),
42+
):
43+
# Try to run the conversation (will fail due to mocked exception)
44+
with pytest.raises(Exception):
45+
conversation.run()
46+
47+
# Close the conversation (this would normally be called by __del__)
48+
conversation.close()
49+
50+
# Check that no warning about empty span stack was logged
51+
warning_messages = [
52+
record.message for record in caplog.records if record.levelname == "WARNING"
53+
]
54+
span_warnings = [
55+
msg
56+
for msg in warning_messages
57+
if "Attempted to end active span, but stack is empty" in msg
58+
]
59+
60+
# This test should fail initially (showing the bug exists)
61+
# After the fix, there should be no span warnings
62+
assert len(span_warnings) == 0, f"Found span warnings: {span_warnings}"
63+
64+
65+
def test_span_ending_with_successful_run(caplog):
66+
"""Test span ending behavior with a successful run (no exceptions)."""
67+
68+
# Create test agent
69+
agent = create_test_agent()
70+
71+
# Create a temporary workspace
72+
with tempfile.TemporaryDirectory() as temp_dir:
73+
# Create conversation
74+
conversation = LocalConversation(
75+
agent=agent, workspace=temp_dir, visualize=False
76+
)
77+
78+
# Mock the agent.step to finish immediately (no iterations)
79+
def finish_immediately(*args, **kwargs):
80+
conversation._state.execution_status = (
81+
conversation._state.execution_status.__class__.FINISHED
82+
)
83+
84+
# Capture logs at WARNING level
85+
with caplog.at_level(logging.WARNING):
86+
with patch(
87+
"openhands.sdk.agent.agent.Agent.step", side_effect=finish_immediately
88+
):
89+
# Run the conversation successfully
90+
conversation.run()
91+
92+
# Close the conversation
93+
conversation.close()
94+
95+
# Check that no warning about empty span stack was logged
96+
warning_messages = [
97+
record.message for record in caplog.records if record.levelname == "WARNING"
98+
]
99+
span_warnings = [
100+
msg
101+
for msg in warning_messages
102+
if "Attempted to end active span, but stack is empty" in msg
103+
]
104+
105+
assert len(span_warnings) == 0, f"Found span warnings: {span_warnings}"
106+
107+
108+
def test_no_span_operations_when_observability_disabled(caplog):
109+
"""Test that no span operations occur when observability is disabled."""
110+
111+
# Create test agent
112+
agent = create_test_agent()
113+
114+
# Create a temporary workspace
115+
with tempfile.TemporaryDirectory() as temp_dir:
116+
# Create conversation
117+
conversation = LocalConversation(
118+
agent=agent, workspace=temp_dir, visualize=False
119+
)
120+
121+
# Mock the agent.step to finish immediately
122+
def finish_immediately(*args, **kwargs):
123+
conversation._state.execution_status = (
124+
conversation._state.execution_status.__class__.FINISHED
125+
)
126+
127+
# Capture logs at WARNING level
128+
with caplog.at_level(logging.WARNING):
129+
# Run and close the conversation
130+
with patch(
131+
"openhands.sdk.agent.agent.Agent.step", side_effect=finish_immediately
132+
):
133+
conversation.run()
134+
conversation.close()
135+
136+
# Check that no warning about empty span stack was logged
137+
warning_messages = [
138+
record.message for record in caplog.records if record.levelname == "WARNING"
139+
]
140+
span_warnings = [
141+
msg
142+
for msg in warning_messages
143+
if "Attempted to end active span, but stack is empty" in msg
144+
]
145+
146+
assert len(span_warnings) == 0, f"Found span warnings: {span_warnings}"

0 commit comments

Comments
 (0)