mirror of
https://github.com/simonw/datasette.git
synced 2026-09-09 01:54:15 +02:00
Propagate otel context across the thread boundaries
Spans created on a worker thread resolve their parent from that thread's ambient context, so without this every span produced below Database came back as an unparented root, disconnected from the request that caused it. Carrying the caller's context across each boundary is also what makes the thread-pool wait visible: db.query covers the full round trip, the new db.query.execute covers only the work inside the worker, and the gap between them is the queueing the old tracer folds invisibly into one number. - execute_fn()'s executor.submit() and execute_isolated_fn()'s run_in_executor() (immutable databases) now run the callable inside a contextvars.copy_context(). A *fresh* copy per submit is required: concurrently entering one shared Context raises "RuntimeError: cannot enter context ... already entered". - WriteTask carries the otel Context captured on the event loop at enqueue time plus an enqueued_at_ns timestamp (both need __slots__ entries, or they fail with AttributeError at runtime). _execute_writes attaches that context right after the _SHUTDOWN check and detaches it in a finally spanning all three execution branches - the write thread is persistent and shared, so a leaked token would grow its context stack for every write processed afterwards, and a wrong-token detach only logs rather than raising. - New spans: db.query.execute (read worker thread), db.write.queue_wait (explicit start/end timestamps, so its duration is the real enqueue -> dequeue wait rather than the microseconds spent building the span) and db.write.execute (skipped in the conn_exception branch, where fn never runs). db.query.execute honours log_sql_errors for the same reason db.query does: facet suggestion probes with log_sql_errors=False and would otherwise paint two red spans per text column on every table page. - The write-thread warm-up prepare_connection is left as a documented orphan root - no caller context exists that early. Tests assert actual parent/child span-id relationships in a shared trace, not just that spans exist, since an unparented root looks identical to a correct span if you only check presence. Note that copy_context() copies every ContextVar, not just OTel's, so Datasette's own context vars (_skip_permission_checks, _permission_check_cache, _in_datasette_client) now flow into worker threads where they previously did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
59bfa495cc
commit
582d79a148
3 changed files with 464 additions and 67 deletions
|
|
@ -3,11 +3,13 @@ Tests for the datasette.database.Database class
|
|||
"""
|
||||
|
||||
import asyncio
|
||||
import threading
|
||||
import uuid
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
import sqlite_utils
|
||||
from opentelemetry import context as otel_context_api
|
||||
|
||||
from datasette.app import Datasette
|
||||
from datasette.database import (
|
||||
|
|
@ -1223,3 +1225,110 @@ async def test_database_close_is_idempotent(tmpdir):
|
|||
# Second call should be a no-op, not raise
|
||||
db.close()
|
||||
ds._internal_database.close()
|
||||
|
||||
|
||||
_CONTEXT_LEAK_MARKER_KEY = "otel-context-leak-marker"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("num_sql_threads", (0, 1))
|
||||
async def test_write_thread_context_is_detached_between_tasks(
|
||||
tmp_path, monkeypatch, num_sql_threads
|
||||
):
|
||||
"""
|
||||
The write thread attaches each task's otel Context and must detach it
|
||||
again before picking up the next task. The thread is persistent and
|
||||
shared, so a leaked token would grow that thread's context stack for the
|
||||
rest of the process - and a *wrong*-token detach only logs a warning
|
||||
rather than raising, so "does it throw" cannot catch either mistake.
|
||||
|
||||
Two things are asserted, because neither alone is sufficient:
|
||||
|
||||
1. Each task observes the context value that was current on the event
|
||||
loop when it was queued. This is what fails if the Context is not
|
||||
carried on WriteTask, or is never attached. It does *not* catch a
|
||||
missing detach: attach() replaces the current Context wholesale, so a
|
||||
leftover one from a previous task is simply overwritten.
|
||||
2. The write thread's attach depth is identical at the same point in
|
||||
every task. This is what fails if detach is missing - the stack grows
|
||||
by one per task - and it holds across a task that raises, because the
|
||||
detach lives in a `finally`.
|
||||
|
||||
An otel context value is used rather than a plain contextvars.ContextVar:
|
||||
a plain var set on the event loop never crosses into the write thread, so
|
||||
the probe would read None every time and the test could not fail.
|
||||
"""
|
||||
name = f"context_leak_test_{num_sql_threads}"
|
||||
db_path = tmp_path / f"{name}.db"
|
||||
sqlite3.connect(db_path).close()
|
||||
ds = Datasette([str(db_path)], settings={"num_sql_threads": num_sql_threads})
|
||||
db = ds.get_database(name)
|
||||
await db.execute_write("create table t (id integer primary key)")
|
||||
|
||||
write_thread_name = f"_execute_writes for database {name}"
|
||||
depth = {"value": 0}
|
||||
real_attach = otel_context_api.attach
|
||||
real_detach = otel_context_api.detach
|
||||
|
||||
def counting_attach(context):
|
||||
token = real_attach(context)
|
||||
if threading.current_thread().name == write_thread_name:
|
||||
depth["value"] += 1
|
||||
return token
|
||||
|
||||
def counting_detach(token):
|
||||
real_detach(token)
|
||||
if threading.current_thread().name == write_thread_name:
|
||||
depth["value"] -= 1
|
||||
|
||||
# Patched on the opentelemetry.context module itself, which is what both
|
||||
# database.py and opentelemetry.trace.use_span() look the functions up on.
|
||||
monkeypatch.setattr(otel_context_api, "attach", counting_attach)
|
||||
monkeypatch.setattr(otel_context_api, "detach", counting_detach)
|
||||
|
||||
seen_markers = []
|
||||
seen_depths = []
|
||||
|
||||
def probe(conn):
|
||||
seen_markers.append(otel_context_api.get_value(_CONTEXT_LEAK_MARKER_KEY))
|
||||
seen_depths.append(depth["value"])
|
||||
|
||||
def failing_probe(conn):
|
||||
probe(conn)
|
||||
# Exercises the write thread's exception path: the detach still has
|
||||
# to happen, which is why it lives in a `finally`.
|
||||
raise ValueError("deliberate failure inside a write task")
|
||||
|
||||
try:
|
||||
for i in range(5):
|
||||
ctx = otel_context_api.set_value(_CONTEXT_LEAK_MARKER_KEY, f"marker-{i}")
|
||||
token = real_attach(ctx)
|
||||
try:
|
||||
if i == 2:
|
||||
with pytest.raises(ValueError):
|
||||
await db.execute_write_fn(failing_probe)
|
||||
else:
|
||||
await db.execute_write_fn(probe)
|
||||
finally:
|
||||
real_detach(token)
|
||||
|
||||
# Sanity check: no marker is active in *this* (event loop) context
|
||||
# right now, so the final probe is a fair test of the write thread's
|
||||
# own state rather than something this test forgot to clean up.
|
||||
assert otel_context_api.get_value(_CONTEXT_LEAK_MARKER_KEY) is None
|
||||
await db.execute_write_fn(probe)
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
assert seen_markers == [
|
||||
"marker-0",
|
||||
"marker-1",
|
||||
"marker-2",
|
||||
"marker-3",
|
||||
"marker-4",
|
||||
None,
|
||||
]
|
||||
assert len(set(seen_depths)) == 1, (
|
||||
f"write thread context stack grew across tasks: {seen_depths} - "
|
||||
"a token was attached without being detached"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue