Guard the detached instance root on the key it is stored under

[chris-colinsky]

Step one of the #279 rework. Small on purpose: nothing else in the detached path is safe until both callers agree on the key.

The defect

_sync_subgraph_spans tested if prefix in inv_state.detached_roots — the bare prefix — while _open_detached_fan_out_instance_root stores under prefix + (str(fan_out_index),). The guard therefore never matched a detached fan-out instance, and the arm fired once per inner node event rather than once per instance.

Reachable on an ordinary graph, on main today, with nothing from the reverted work: a detached fan-out whose instance subgraph has two nodes.

_open_detached_fan_out_instance_root called 2 time(s) for 1 instance
   from ns=('fo', 'a')
   from ns=('fo', 'b')

traces: 3                     <- expected 2
  ['a']                       <- alone, parent never exported
  ['b', 'fo', 'openarmature.invocation']
  ['fo', 'openarmature.invocation']

The second open replaced the first in both detached_roots and detached_invocation_spans, so the first root and its detached invocation span were never ended and never exported. One instance's inner nodes ended up in different traces, with the first pointing at a parent span id nothing emitted.

It is invisible with a single-node instance subgraph, which is why no existing test saw it: one event, one open.

The fix

The instance arm now tests its own key. The bare-prefix test above it stays, because a detached subgraph root really is stored that way. Both shapes share one dict, which is what made the mismatch easy to miss, and the comment now says so.

Why this lands alone

The previous attempt at #279's gap 3 mirrored the detached arms into orphan synthesis first. That turned this latent mismatch into a live double-open, and it was one of four blockers an adversarial review found, which is why the whole mirroring was reverted.

Remaining steps, in order, recorded on #279:

2. Give _open_detached_subgraph_root an identity source independent of the triggering event, so routing it through the orphan path cannot blank openarmature.subgraph.name.
3. Establish the invocation-span precondition in the orphan path, or gate the detached arms on it existing.
4. Only then mirror the arms, re-checking the instance-index question against every reader.

Tests

test_detached_fan_out_instance_opens_one_root_per_instance fails on main and passes here; removing the guard turns it red again.

It asserts the trace shape a consumer depends on rather than the call count: no span left pointing at an unexported parent, the instance's inner nodes sharing one trace, and exactly two traces. openarmature.invocation is excluded from the dangling-parent check deliberately and with the reason stated — a detached invocation span is parented under a synthetic NonRecordingSpan that carries the new trace id and is never exported by design, with the fan-out node span holding a Link to it instead.

2196 passing; ruff, format and pyright clean.

[Copilot]

Pull request overview

This PR fixes a detached fan-out observability defect in the OTel observer where a detached fan-out instance root was guarded using the wrong key shape, causing a root to be re-opened per inner node event and leading to abandoned spans and incorrect trace grouping.

Changes:

• Fix _sync_subgraph_spans to guard detached fan-out instance roots using the same key shape that _open_detached_fan_out_instance_root stores under.
• Add a regression test asserting exported-span trace shape for a detached fan-out instance with a multi-node instance subgraph.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
tests/unit/test_observability_otel.py	Adds a regression test for detached fan-out instance root opening and resulting trace/parentage shape.
src/openarmature/observability/otel/observer.py	Corrects detached fan-out instance root guarding to prevent per-inner-node double-open and abandoned roots.

Suppressed comments (1)

tests/unit/test_observability_otel.py:6455

• Like the rest of this test module, prefer .context.trace_id on exported spans instead of get_span_context(), which may not exist on ReadableSpan and makes this test more brittle than necessary.

    trace_of = {s.name: cast("Any", s.get_span_context()).trace_id for s in spans}
    assert trace_of["a"] == trace_of["b"], (
        "the two inner nodes of one fan-out instance landed in different traces"
    )
    trace_count = len({cast("Any", s.get_span_context()).trace_id for s in spans})
    assert trace_count == 2, f"expected the parent trace plus one detached instance trace, got {trace_count}"

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.