Repository navigation
feat(sdk): run plugin hooks off checkpoint thread - #823
Conversation
This comment has been minimized.
This comment has been minimized.
|
The updated assertion in |
332b07c to
f0fceb9
Compare
f0fceb9 to
fca073c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
fca073c to
aeebd12
Compare
This comment was marked as resolved.
This comment was marked as resolved.
The end and change hooks for a checkpoint response ran on the checkpoint thread, the thread that applies every response for every branch, so a slow hook delayed every checkpoint. They now run elsewhere, in an order plugins can rely on. The checkpoint thread records each terminal operation's end hook before it publishes the response. The thread that then observes the result runs the hook before the operation returns: the operation's own thread after a synchronous checkpoint, or the thread that reads a result another response completed, such as a branch resumed after a refresh. Another thread that observes the result meanwhile waits for the hook; a read made from inside any plugin hook neither runs nor waits for an end hook, so hooks cannot wait on each other. So on a logical path an end hook finishes before the next operation starts, which the OpenTelemetry plugin needs for its span order. End hooks no thread claims run on the handler thread before on_invocation_end, descendants before ancestors, and an operation ends at most once per invocation. on_operation_change runs on a dispatcher thread. Notifications that arrive while a plugin is still in the hook merge into the pending notification: updated_operations carries the newest state of every operation changed since the previous delivery, operations the newest map. So the backlog is at most one pending notification. The plugin contract docstring states where each hook runs and what order it may assume.
aeebd12 to
cfaae2b
Compare
|
@zhongkechen Thanks, and agreed. The current revision keeps the original assertion: |
Codex AI reviewNo confirmed actionable findings. Review was limited to read-only inspection; tests were not executed, leaving residual runtime concurrency risk. Reviewed commit |
Problem
Plugin hooks for a checkpoint response (
on_operation_end,on_operation_change) ran on the checkpoint thread. That is the one thread that sends every checkpoint of an invocation and applies every response, for every branch. So a slow hook, such as a network export, delayed every checkpoint in the invocation.Simply detaching the hooks is not an option. The OpenTelemetry plugins derive span order from hook call order, and the OpenTelemetry conformance suite asserts that order: a wait's span must end before the span of the step that follows it starts. So an operation's end hook must run before that operation returns to user code.
What changed
The checkpoint thread runs no plugin code. For each response it:
End hooks run on the thread that observes the operation's terminal result, before that operation returns to user code:
A thread that observes a result while another thread is running its end hook waits for the hook. A read made from inside any plugin hook, on the hook's own thread, is the exception: it neither runs nor waits for an end hook. So a result read cannot make the hook that makes it wait for another end hook, and a lock a plugin holds across a hook (the contract tells plugins to guard shared state with one) cannot be needed by an end hook that read starts. Before this PR, result reads dispatched nothing anywhere; this keeps that true inside hooks. A terminal operation no thread reads during the invocation gets its end hook on the handler thread before
on_invocation_end; a completed descendant nobody read (a callback created and never awaited) runs with, and before, its ancestor's end hook. An operation's end hook runs at most once per invocation, even when a response lists an operation the invocation input already carried.Change notifications run on a dispatcher thread, in response order. When a plugin is still in the hook as responses arrive, they merge into one pending notification:
updated_operationscarries the newest state of every operation changed since the previous delivery,operationsthe newest full map. Intermediate states and the number of calls are not guaranteed. The pending notification is delivered beforeon_invocation_end. The single pending slot bounds the backlog whatever a plugin's speed. If the process cannot start the thread, the merged notification is delivered once on the handler thread beforeon_invocation_end.Shutdown order is unchanged: branch pools are joined, the checkpoint thread is stopped and joined, unclaimed end hooks run, the dispatcher is joined, then
on_invocation_endruns.Failures: ordinary hook exceptions are logged and ignored, as before. Thread-control exceptions (
KeyboardInterrupt,SystemExit,GeneratorExit) propagate on operation threads and the handler thread; on the dispatcher thread they are logged, the other plugins still receive the notification, and delivery continues.The
DurableInstrumentationPluginclass docstring states this contract: where each hook runs, what order a plugin may rely on, and the merge rule.Behavior changes for plugin authors
on_operation_changemay be coalesced; a plugin must not assume one call per checkpoint response.on_operation_endfor an operation completed outside the invocation runs on the thread that reads the result, not on the checkpoint thread.main, a response that listed an already-reported operation produced a second end hook.Tests
tests/plugin_hook_registry_test.py(48 tests): record-before-publish and record-before-signal observed at the loop's publish and signal points; claim on the synchronous wait (success and error paths, including a failed later pagination page); claim on read; a second observer waits for a running hook; descendants before ancestors; once per invocation across the registry and the replay path; every claim decision under one lock, in one acquisition; the replay-boundary reads take no hook; two end hooks reading each other's operations concurrently both return; a change hook that holds a lock and reads a callback result does not deadlock with the callback's end hook; a propagation-metadata provider that reads a result runs no hook; a read from inside a hook runs no hook and the consuming read later does; the child-context end hook reads without waiting; a control exception in one drained hook does not stop the others; the dispatcher's order, merge rule, join before the end hook, fallback without a thread, and control-exception handling; drain and shutdown order.tests/e2e/plugin_hook_contract_int_test.py(8 tests) through the local test runner: end hook before the operation returns; slow hook delays only its own branch; child before parent; map items before the map; early completion exception (the quick branch returns only once the slow branch is running, since a branch the pool has not started is cancelled on early completion and has no end hook; the previous revision assumed the start and failed on the slower CI runners); a wait in a branch resumed in-process by a refresh; a callback result read.tests/e2e/handler_context_propagation_int_test.py: contextvar visibility on the dispatcher thread and the operation thread.Notes
UpdatedOperationIdsoperations already delivered in a response. The service does neither. The once-per-invocation rule makes the SDK's behavior the same under both. A separate change to the runner may follow.