Skip to content

Commit a815de0

Browse files
committed
fix(weave): resolve ch_client off the loop and map cost columns like sync
acalls_query_stats passed self.ch_client as an argument to asyncio.to_thread, so it was evaluated on the event loop: minting blocks there on first use, and concurrent calls then handed one thread's client to every other thread. acalls_query ignored include_costs. A cost query's SELECT carries sort-only columns ahead of summary_dump, so zipping against select_fields alone shifts every later value onto the wrong key -- silently, since the zip was strict=False. Uses get_cost_result_columns like the sync path, and strict=True so a future mismatch fails loudly.
1 parent b06f9fc commit a815de0

2 files changed

Lines changed: 129 additions & 6 deletions

File tree

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
"""Guards for the two ways the async calls reads can silently diverge from sync.
2+
3+
Both are failure modes that produce wrong data or wrong threading rather than an
4+
exception, so neither would show up in a smoke test.
5+
"""
6+
7+
import asyncio
8+
import threading
9+
from unittest.mock import MagicMock, patch
10+
11+
import pytest
12+
13+
from weave.trace_server import trace_server_interface as tsi
14+
from weave.trace_server.async_clickhouse_trace_server import (
15+
AsyncClickHouseTraceServer,
16+
)
17+
from weave.trace_server.calls_query_builder.calls_query_builder import (
18+
CallsMergedField,
19+
OrderField,
20+
)
21+
from weave.trace_server.token_costs import get_cost_result_columns
22+
23+
PROJECT = "UHJvamVjdEludGVybmFsSWQ6MQ=="
24+
25+
26+
def test_acalls_query_stats_never_touches_ch_client_on_the_event_loop():
27+
"""`ch_client` is thread-local and minting one blocks.
28+
29+
Passed as an argument to `to_thread` it would be evaluated on the loop:
30+
blocking it on first use, and then handing one thread's client to every
31+
other thread running this concurrently.
32+
"""
33+
server = AsyncClickHouseTraceServer(host="test_host")
34+
loop_thread = threading.get_ident()
35+
touched_on_loop: list[bool] = []
36+
37+
class Guard:
38+
def __get__(self, obj, owner=None):
39+
touched_on_loop.append(threading.get_ident() == loop_thread)
40+
return MagicMock()
41+
42+
async def run():
43+
with (
44+
patch.object(type(server), "ch_client", Guard()),
45+
patch.object(type(server), "table_routing_resolver", MagicMock()),
46+
patch.object(server, "_aquery", return_value=MagicMock(result_rows=[[0]])),
47+
patch(
48+
"weave.trace_server.async_clickhouse_trace_server.build_calls_stats_query",
49+
return_value=("SELECT 1", ["count"], None),
50+
),
51+
patch(
52+
"weave.trace_server.async_clickhouse_trace_server.calls_stats_res",
53+
return_value=tsi.CallsQueryStatsRes(count=0),
54+
),
55+
):
56+
await server.acalls_query_stats(tsi.CallsQueryStatsReq(project_id=PROJECT))
57+
58+
asyncio.run(run())
59+
assert touched_on_loop, "ch_client was never resolved; the test proves nothing"
60+
assert not any(touched_on_loop), (
61+
"ch_client was resolved on the event loop thread; it must be resolved "
62+
"inside the executor callback"
63+
)
64+
65+
66+
class _Captured(Exception):
67+
"""Carries the zip keys out before any downstream validation runs."""
68+
69+
def __init__(self, columns):
70+
self.columns = columns
71+
72+
73+
@pytest.mark.parametrize("include_costs", [True, False])
74+
def test_acalls_query_maps_columns_the_way_sync_does(include_costs: bool):
75+
"""A cost query's SELECT carries sort-only columns ahead of `summary_dump`.
76+
77+
Zipping row values against `select_fields` alone shifts every value after
78+
that point onto the wrong key, which is silent rather than an error.
79+
"""
80+
server = AsyncClickHouseTraceServer(host="test_host")
81+
select = ["id", "project_id", "summary_dump"]
82+
order = [OrderField(field=CallsMergedField(field="started_at"), direction="ASC")]
83+
cq = MagicMock(
84+
select_fields=[MagicMock(field=f) for f in select], order_fields=order
85+
)
86+
cq.as_sql.return_value = "SELECT 1"
87+
88+
expected = get_cost_result_columns(select, order) if include_costs else select
89+
90+
def capture(d):
91+
raise _Captured(list(d))
92+
93+
async def run():
94+
with (
95+
patch.object(server, "_build_calls_query", return_value=(cq, None)),
96+
patch.object(
97+
server,
98+
"_aquery",
99+
return_value=MagicMock(result_rows=[list(range(len(expected)))]),
100+
),
101+
patch(
102+
"weave.trace_server.async_clickhouse_trace_server.ch_call_dict_to_call_schema_dict",
103+
side_effect=capture,
104+
),
105+
):
106+
await server.acalls_query(
107+
tsi.CallsQueryReq(project_id=PROJECT, include_costs=include_costs)
108+
)
109+
110+
with pytest.raises(_Captured) as exc:
111+
asyncio.run(run())
112+
assert exc.value.columns == expected

weave/trace_server/async_clickhouse_trace_server.py

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
make_spans_count_query,
5757
make_spans_list_query,
5858
)
59+
from weave.trace_server.token_costs import get_cost_result_columns
5960
from weave.trace_server.tracing import traced
6061

6162
_T = TypeVar("_T")
@@ -284,11 +285,17 @@ async def acalls_query(self, req: tsi.CallsQueryReq) -> tsi.CallsQueryRes:
284285
cq, settings = await asyncio.to_thread(self._build_calls_query, req)
285286
pb = ParamBuilder()
286287
raw_res = await self._aquery(cq.as_sql(pb), pb.get_params(), settings=settings)
287-
select_columns = [c.field for c in cq.select_fields]
288+
if req.include_costs:
289+
# Cost query SELECT adds ORDER BY fields; result columns must match.
290+
select_columns = get_cost_result_columns(
291+
[c.field for c in cq.select_fields], cq.order_fields
292+
)
293+
else:
294+
select_columns = [c.field for c in cq.select_fields]
288295
calls = [
289296
tsi.CallSchema.model_validate(
290297
ch_call_dict_to_call_schema_dict(
291-
dict(zip(select_columns, row, strict=False))
298+
dict(zip(select_columns, row, strict=True))
292299
)
293300
)
294301
for row in raw_res.result_rows
@@ -300,10 +307,14 @@ async def acalls_query_stats(
300307
self, req: tsi.CallsQueryStatsReq
301308
) -> tsi.CallsQueryStatsRes:
302309
"""Native-async twin of `calls_query_stats`."""
303-
read_table = await asyncio.to_thread(
304-
self.table_routing_resolver.resolve_read_table,
305-
req.project_id,
306-
self.ch_client,
310+
# `ch_client` is thread-local and minting one blocks. Resolve it inside
311+
# the pool thread: as an argument it would be evaluated on the event
312+
# loop, blocking it on first use and then handing one thread's client to
313+
# every other thread that runs this concurrently.
314+
read_table = await self._run_on_ch_executor(
315+
lambda: self.table_routing_resolver.resolve_read_table(
316+
req.project_id, self.ch_client
317+
)
307318
)
308319
pb = ParamBuilder()
309320
query, columns, settings = build_calls_stats_query(req, pb, read_table)

0 commit comments

Comments
 (0)