Skip to content

Commit 63ca57f

Browse files
fix(tracing): drain sync tracing processors on ACP shutdown
A sync ACP agent silently lost whatever business spans were still queued when the pod stopped. The lifespan drained `shutdown_default_span_queue`, which is the ASYNC path only; the sync tracing processors hold their own queue and nothing in the SDK ever shut them down. `get_sync_tracing_processors()` had exactly one caller — tracer.py, to CONSTRUCT a Trace — and no shutdown path at all. This is the same class of bug as the missing `sgp_obs.shutdown()` in the previous commit, and it compounds it from the other end. The business span is what an obs span's `agentex.business_trace_id` resolves to, so dropping it breaks the pivot from Tempo back to the SGP store — the backward edge points at a record that was never written. Found while working out what the obs-test-* agents in agentex-agents#2183 would still need after the SDK absorbs their bootstrap: each one carries a `sgp_flush_lifespan` that does exactly this, which is the tell that the SDK should have been doing it. Each processor is isolated — one that hangs or raises must not strand the spans held by the ones after it, and none of them may stop the pod shutting down. The last test pins the wiring rather than just the helper: a drain nothing calls is worthless, so it asserts the lifespan actually invokes both this and shutdown_sgp_obs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 777ca59 commit 63ca57f

3 files changed

Lines changed: 113 additions & 0 deletions

File tree

src/agentex/lib/sdk/fastacp/base/base_acp_server.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,40 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
119119
_detach_otel_context(otel_token)
120120

121121

122+
def _shutdown_sync_tracing_processors() -> None:
123+
"""Drain the sync tracing processors' queues at shutdown. Never raises.
124+
125+
``shutdown_default_span_queue`` covers the async path only. The sync processors
126+
keep their own queue and nothing in the SDK ever shut them down, so a sync ACP
127+
agent dropped whatever business spans were still queued when the pod stopped.
128+
That matters beyond the lost spans: the business span is what an obs span's
129+
``agentex.business_trace_id`` resolves to, so losing it breaks the pivot from
130+
Tempo back to the SGP store.
131+
132+
Each processor is isolated: one that hangs or raises must not stop the others,
133+
and none of them may stop the pod from shutting down.
134+
"""
135+
try:
136+
from agentex.lib.core.tracing.tracing_processor_manager import (
137+
get_sync_tracing_processors,
138+
)
139+
140+
processors = get_sync_tracing_processors()
141+
except Exception: # pragma: no cover - nothing to drain if this can't import
142+
logger.debug("sync tracing processors unavailable at shutdown", exc_info=True)
143+
return
144+
145+
for processor in processors:
146+
try:
147+
processor.shutdown()
148+
except Exception: # noqa: PERF203 - one bad processor must not block the rest
149+
logger.warning(
150+
"a sync tracing processor failed to flush on shutdown; "
151+
"some business spans may be lost",
152+
exc_info=True,
153+
)
154+
155+
122156
class BaseACPServer(FastAPI):
123157
"""
124158
AsyncAgentACP provides RPC-style hooks for agent events and commands asynchronously.
@@ -191,6 +225,11 @@ async def lifespan_context(app: FastAPI): # noqa: ARG001
191225
yield
192226
finally:
193227
await shutdown_default_span_queue()
228+
# The queue above is the ASYNC path only. Sync tracing processors
229+
# hold their own queue and nothing ever drained it, so a sync ACP
230+
# agent lost whatever business spans were still queued when the pod
231+
# stopped — including the ones the obs correlation points at.
232+
_shutdown_sync_tracing_processors()
194233
# Flush whatever sgp-obs still holds. A periodic exporter's buffer
195234
# is otherwise dropped when the pod stops, which for a short-lived
196235
# or scaled-to-zero agent can be most of what it recorded. No-op
@@ -199,6 +238,7 @@ async def lifespan_context(app: FastAPI): # noqa: ARG001
199238

200239
return lifespan_context
201240

241+
202242
async def _healthz(self):
203243
"""Health check endpoint"""
204244
result = {"status": "healthy"}

src/agentex/lib/sdk/fastacp/base/tests/__init__.py

Whitespace-only changes.
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
"""Tests for the ACP lifespan's shutdown drains.
2+
3+
``shutdown_default_span_queue`` covers the async span path. The SYNC tracing
4+
processors keep their own queue, and nothing in the SDK ever shut them down, so a
5+
sync ACP agent dropped whatever business spans were still queued when the pod
6+
stopped. That is worse than the spans themselves: the business span is what an obs
7+
span's ``agentex.business_trace_id`` resolves to, so losing it breaks the pivot from
8+
Tempo back to the SGP store.
9+
"""
10+
11+
from __future__ import annotations
12+
13+
from agentex.lib.sdk.fastacp.base import base_acp_server
14+
from agentex.lib.sdk.fastacp.base.base_acp_server import _shutdown_sync_tracing_processors
15+
16+
17+
class _Processor:
18+
def __init__(self, explode: bool = False) -> None:
19+
self.calls = 0
20+
self._explode = explode
21+
22+
def shutdown(self) -> None:
23+
self.calls += 1
24+
if self._explode:
25+
raise RuntimeError("flush timed out")
26+
27+
28+
def _patch_processors(monkeypatch, processors):
29+
import agentex.lib.core.tracing.tracing_processor_manager as mgr
30+
31+
monkeypatch.setattr(mgr, "get_sync_tracing_processors", lambda: processors)
32+
33+
34+
class TestSyncProcessorDrain:
35+
def test_every_processor_is_flushed(self, monkeypatch):
36+
a, b = _Processor(), _Processor()
37+
_patch_processors(monkeypatch, [a, b])
38+
_shutdown_sync_tracing_processors()
39+
assert (a.calls, b.calls) == (1, 1)
40+
41+
def test_one_failure_does_not_stop_the_others(self, monkeypatch):
42+
"""A processor that hangs or raises must not strand the spans held by the
43+
ones after it in the list."""
44+
bad, good = _Processor(explode=True), _Processor()
45+
_patch_processors(monkeypatch, [bad, good])
46+
_shutdown_sync_tracing_processors()
47+
assert good.calls == 1
48+
49+
def test_no_processors_is_a_no_op(self, monkeypatch):
50+
_patch_processors(monkeypatch, [])
51+
_shutdown_sync_tracing_processors() # must not raise
52+
53+
def test_an_unimportable_manager_does_not_fail_shutdown(self, monkeypatch):
54+
"""Nothing here may stop the pod from shutting down."""
55+
import builtins
56+
57+
real_import = builtins.__import__
58+
59+
def blocked(name, *args, **kwargs):
60+
if "tracing_processor_manager" in name:
61+
raise ImportError("boom")
62+
return real_import(name, *args, **kwargs)
63+
64+
monkeypatch.setattr(builtins, "__import__", blocked)
65+
_shutdown_sync_tracing_processors() # must not raise
66+
67+
def test_the_lifespan_calls_it(self):
68+
"""Pin the wiring, not just the helper: a drain nothing calls is worthless."""
69+
import inspect
70+
71+
source = inspect.getsource(base_acp_server.BaseACPServer.get_lifespan_function)
72+
assert "_shutdown_sync_tracing_processors()" in source
73+
assert "shutdown_sgp_obs()" in source

0 commit comments

Comments
 (0)