Fix two branch dispatch key defects in the observers - #276
Merged
Conversation
Per-branch dispatch spans are stored under a lineage-aware key carrying the enclosing fan-out-index and branch chains. The key builder truncated a chain longer than the namespace prefix but never padded one that was shorter, so a caller whose lineage is shallower than the prefix built a key denoting the same lineage as the registered one while being unequal to it. An orphan provider call issued from branch middleware is exactly that caller: it carries empty chains, looked up (prefix, (), (), branch) against a span registered as (prefix, (None,), (), branch), missed, and fell through the ancestor walk to the invocation root instead of parenting under the branch dispatch span. Both backends carried the defect. Each helper's docstring says it mirrors the other, but nothing enforced that, so the fix is applied and tested in both.
The padding fix has two ways to be wrong and no fixture covers either on the Langfuse side, where no activated fixture reaches that helper. Under-padding is the original defect: a shallow lineage builds a key unequal to the registered one. Over-padding is the tempting sloppy fix: replacing the chain with None entries collapses genuinely distinct enclosing lineages, so a parallel-branches node inside outer fan-out instance 0 would share a key with the same node inside instance 1. Both helpers are driven by the same tests, including one asserting they agree across a matrix of lineages. They are duplicated by design, one per backend, and each docstring claims to mirror the other while nothing checked it -- which is how the same defect came to sit in both.
Both assert where a provider span lands when it is issued from a wrapper rather than the node body, so the calling node's span is not open and the span falls back to the nearest enclosing wrapper. 152's is the per-branch dispatch span; 153 nests a fan-out between them so its is the innermost instance span. 152 was failing on the key defect fixed separately, and is the only fixture that discriminates it. The driver rides on the generic graph builder rather than copying fixture 133's, which is hardcoded to that fixture's subgraph and node names. It attaches each orphan wrapper to whatever encloses its subgraph, which is the fixtures' own claim about the fallback: instance middleware for a fan-out target, branch middleware for a parallel branch. Node middleware would not do, running entirely inside the node span in both phases. Nine invariants are implemented rather than recorded as documentary, because the tree matcher pins "X appears under Y" but tolerates extra children, so it can express neither the absence claims nor the count nor the branch close ordering. Subgraph declarations are read from the case as well as the fixture top level. Fifteen fixtures across six capabilities use the case-level form that conformance-adapter section 11 does not document; reading only the documented level is what made 152 fail with a bare KeyError.
There was a problem hiding this comment.
Pull request overview
This PR activates observability conformance fixtures 152 and 153 (orphan LLM-span fallback under wrapper spans) and fixes a lineage-key normalization defect that prevented orphan spans emitted from branch middleware from parenting under the correct per-branch dispatch span.
Changes:
- Fix
_branch_dispatch_keyto pad lineage chains that are shallower than the namespace prefix in both the OTel and Langfuse observers. - Wire fixtures 152 and 153 into the observability conformance runner via a new
_run_orphan_fallback_fixturedriver that asserts additional invariants beyondspan_tree. - Add unit tests to prevent drift between the two key-builder implementations and to guard the padding behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/unit/test_observability_otel.py |
Adds unit coverage for branch-dispatch key normalization and cross-backend agreement. |
tests/conformance/test_observability.py |
Wires fixtures 152/153, adds an orphan-fallback driver plus invariant assertions. |
src/openarmature/observability/otel/observer.py |
Fixes branch-dispatch key normalization by truncating and padding chains to prefix depth. |
src/openarmature/observability/langfuse/observer.py |
Mirrors the same key normalization fix for the Langfuse backend. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A driver for both exists earlier in this branch and makes them pass. The passing was not worth having. The orphan provider call is enqueued before the branch's first inner node, so whether the per-branch dispatch span is registered when the observer resolves the parent depends on nothing yielding to the event loop in between. One `await asyncio.sleep(0)` in the wrapper, ordinary for real middleware, moves 152's orphan to the invocation root and 153's to the `work` branch dispatch span, which 153's own invariant forbids. Activating them would have certified a lucky interleaving as conformance. The lineage-key defect they exposed is real and stays fixed. It is necessary and not sufficient: these need the orphan parent resolved deterministically, by deferring the decision until the enclosing wrapper span is known, which is an observer change rather than a harness one. The deferral strings now carry the measurement so the next author starts from it instead of rediscovering a driver gap that is not the obstacle.
The guard compared a legacy `namespace + (branch_name,)` tuple against a dict keyed by the 4-tuple the opener actually stores, so it never matched. `_open_started_span` runs twice for a callable-branch started event, once from the engine task's `prepare_sync` and once from the async `__call__`, and the callable-branch arm returns before `open_spans` is written, so the usual idempotency check cannot short-circuit either. The second open overwrote the first in the registry; the overwritten span was never ended and never exported. Measured on a two-callable-branch graph: four opens for two branches. The exported span TREE is identical either way, which is why no conformance fixture catches it. What differs is the span published into the branch body as active, so a log record emitted from a callable branch carried a span id absent from the trace. The Langfuse observer's equivalent guard already used the shared key builder; this brings the two backends back into agreement.
`pytest.importorskip` was called while building a parametrize argument, so it ran at module import and its skip is module-scoped. Without the langfuse extra the whole file collapsed to a single skip, taking ~104 unrelated OTel tests with it, including the tripwire that exists to catch drift between the two hand-duplicated key builders. Measured with the import blocked: 1 skipped before, 104 passed and 3 skipped after. CI installs all extras, which is what kept it silent. Also pins the callable-branch dispatch span against being opened twice. That one asserts on the dispatch-span registry rather than the exported tree, because the tree is identical under the defect.
chris-colinsky
changed the title
The existing padding test uses a depth-1 prefix, where the branch slice is `chain[:0]` and is empty whether padded or not, so it only ever exercised the fan-out half. Deleting the `branches` padding from both observers left the whole suite green; deleting it from one was caught only incidentally, by the agreement test noticing the copies diverged. A depth-2 prefix slices `chain[:1]`, so a caller whose branch chain is shorter than `n - 1` builds an unpadded key against a padded registration. Both backends are driven, and the discrimination test still rules out a padding that collapses distinct branch lineages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This started as "wire fixtures 152/153" and ended as a pair of source fixes. The fixtures are not activated here, on purpose. Details below, because the reason is the interesting part.
Two source defects, both in per-branch dispatch span keying
1. Chains shallower than the prefix were never normalized.
_branch_dispatch_keytruncated a chain longer than the namespace prefix but never padded one that was shorter, so a caller with a shallow lineage built a key denoting the same lineage as the registered one while being unequal to it:An orphan provider call issued from branch middleware is exactly that caller. It missed the lookup and fell through the ancestor walk to the invocation root. Both backends carried it; each helper's docstring claims to mirror the other, but nothing enforced that.
2. The callable-branch dedup guard compared the wrong key shape. It tested a legacy
namespace + (branch_name,)tuple against a dict keyed by the 4-tuple the opener stores, so it never matched._open_started_spanruns twice for a callable-branch started event, once from the engine task'sprepare_syncand once from the async__call__, so the second open overwrote the first in the registry. The overwritten span was never ended and never exported.Measured on a two-callable-branch graph: four opens for two branches. The exported span tree is identical either way, which is why no fixture catches it. What differs is the span published into the branch body as active, so a log record emitted from a callable branch carried a span id absent from the trace. The Langfuse observer's equivalent guard already used the shared key builder; this brings the backends back into agreement.
Why 152 and 153 are still deferred
A driver for both exists earlier in this branch and makes them pass. The passing was not worth having.
The orphan provider call is enqueued before the branch's first inner node, so whether the dispatch span is registered when the observer resolves the parent depends on nothing yielding to the event loop in between. Inserting a single
await asyncio.sleep(0)in the wrapper, which is ordinary for real middleware, moves 152's orphan to the invocation root and 153's to theworkbranch dispatch span, the parent 153's own invariant forbids.Activating them would have pinned a lucky interleaving as conformance. Fix 1 is necessary and not sufficient: they need the orphan parent resolved deterministically, by deferring the decision until the enclosing wrapper span is known. That is an observer change, and it belongs in its own PR rather than riding along with a keying fix.
The deferral strings now carry that measurement, so the next author starts from it rather than rediscovering the driver gap that turned out not to be the obstacle.
Test coverage
Neither defect is reachable from any activated fixture, so both are pinned by unit tests:
Also fixes a test-gating defect found in the same review:
pytest.importorskipwas being called while building a parametrize argument, so it ran at module import and its skip is module-scoped. Without the langfuse extra the whole OTel unit file collapsed to a single skip. Measured with the import blocked: 1 skipped before, 104 passed and 3 skipped after. CI installs all extras, which is what kept it silent.Full suite: 2151 passed, 499 skipped.