Skip to content

fix: release GenAINestedClientSpanProcessor state with the parent span - #970

Closed
syed-ahsan-ishtiaque wants to merge 3 commits into
mainfrom
fix/gen-ai-nested-parent-state
Closed

syed-ahsan-ishtiaque wants to merge 3 commits into
mainfrom
fix/gen-ai-nested-parent-state

Conversation

@syed-ahsan-ishtiaque

Copy link
Copy Markdown
Contributor

Why

GenAINestedClientSpanProcessor demotes the outer span of nested GenAI inference CLIENT spans (a framework span wrapping the model SDK span with the same gen_ai.operation.name) to INTERNAL. To do that it records "this parent has a same-operation inference child" when the child ends, and removes the record when the parent ends.

The record was never removed in three cases, so each such model call left one entry until shutdown:

  • Remote parent: the model call's parent is in another process (for example, a worker that makes the call directly under an extracted context). The parent never ends locally.
  • Parent ends before the child: fire-and-forget async work. The child recreated the record after the parent was gone.
  • Parent never ended: the span was garbage-collected, but its record stayed.

Reproduced on main: 1,000 model calls in each case left 3,000 entries after the calls ended. GC and force_flush did not remove them. This affects every application with agent observability enabled today, because that is when the processor is installed.

What changes

  • State is kept only for local parents that are still recording when a child starts. It is released when the parent ends or is garbage-collected.
  • A remote or already-ended parent gets no state. A child that ends after its parent does not recreate it.
  • Demotion behavior is unchanged for normal nesting (two or three levels, different operations, non-LLM parents).

Trade-off: a child whose context was rebuilt with only the parent's span context (trace.set_span_in_context(NonRecordingSpan(...)) / trace.setSpanContext()) from a local parent can no longer demote that parent, because the parent span object is not reachable from the context. Main handled this case by ID, but it is the same mechanism that leaked. Instrumentations pass the real parent span in the context, so this case is unusual.

How it works (Python)

  • on_start: for a CLIENT child whose parent in the context is a local SDK span that is still recording, add an entry for the parent, keyed by trace and span ID, with a weakref.finalize on the parent. Only CLIENT children create entries, since span kind is fixed at start.
  • on_end: pop the span's own entry (demote it if a child recorded the same operation), and record this span's operation on its parent's entry if the parent is still tracked. No entry is created for a parent that is not tracked.
  • Garbage collection. The finalizer only appends the key to a lock-free deque. Every lifecycle call drains the deque, so a collected parent's entry is removed on the next span start or end.
  • Re-entrancy. Garbage collection can run while the lock is held and end a span on the same thread, for example by closing an abandoned streaming generator that holds a span open. The finalizer, set and keys are built before the lock is taken, and the lock is an RLock, so a re-entrant on_end cannot deadlock. An earlier version of this fix built them under a plain Lock and deadlocked in a stress test; the regression test below covers it.
  • If the parent ends on another thread between the check and the insert, the entry is removed right after the insert.
  • Keys now include the trace ID as well as the span ID.

Testing

  • New unit tests: three levels of nesting; a remote parent keeps no state; a child that ends after its parent, or starts after it, keeps no state; non-CLIENT children keep no state; parents collected without ending are released by the next span and by force_flush; a finalizer during GC with the lock held does not deadlock; on_end re-entered on the same thread with the lock held does not deadlock; shutdown clears state and detaches finalizers.
  • A subprocess regression test with gc.set_threshold(50) and abandoned streaming generators, which deadlocked the earlier version of this fix and passes now.
  • Existing demotion tests pass unchanged.
  • Locally: black, isort, flake8 and pylint pass. The full distro run matches main (the same 26 local failures, mostly MCP instrumentation tests that fail without this change too; the CrewAI, LangChain, LlamaIndex and OpenAI Agents suites need packages not installed locally).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The processor recorded "parent has a same-operation inference child" when
an inference CLIENT child ended, and removed the record only when the
parent ended locally. Records were never removed when the parent was
remote, ended before its child, or was garbage-collected without ending,
so each such model call left an entry until shutdown.

Keep state only for local parents that are still recording when a child
starts, and release it with the parent: when the parent ends, or when it
is garbage-collected. Remote and already-ended parents get no state, and a
child that ends after its parent does not recreate it.

Python keys state by trace and span ID with a weakref.finalize on the
parent; the finalizer only queues the key, and every lifecycle call drains
the queue, so garbage collection while the lock is held cannot deadlock.
Only CLIENT children create state, since span kind is fixed at start.
JS keeps state in WeakMaps keyed by the span objects, and does not filter
by kind at start because VercelAISpanProcessor promotes spans in onEnd.
…rant

Garbage collection can run while the processor holds its lock and end a
span on the same thread, for example by closing an abandoned streaming
generator that holds a span open. That span's on_end then waited forever
on the non-reentrant lock. Build the finalizer, set and keys before taking
the lock, and use an RLock so a re-entrant on_end cannot deadlock.

Also re-check the parent after inserting its state, so a parent that ended
on another thread in between does not keep an entry.
@syed-ahsan-ishtiaque
syed-ahsan-ishtiaque marked this pull request as ready for review November 9, 2026 23:00
@syed-ahsan-ishtiaque
syed-ahsan-ishtiaque requested a review from a team as a code owner November 9, 2026 23:00
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @syed-ahsan-ishtiaque's task in 2m 41s —— View job


Claude PR Review

  • Fetch prior reviews/comments to dedupe (none found)
  • Read PR diff and changed files
  • Analyze processor implementation for correctness
  • Analyze tests for coverage
  • Post review findings

Summary

Nice fix. The redesign cleanly addresses the three leak paths called out in the description (remote parent, parent-ends-first, parent-never-ends), and the re-entrancy reasoning is tight.

What stands out as well-handled:

  • Lock-free deque + weakref.finalize for GC'd parents, drained by every lifecycle call — bounded memory even for abandoned parents.
  • RLock + building the finalizer/entry/key outside the lock, so a GC that ends a span on the same thread (closing an abandoned generator) can re-enter on_end without deadlock. The subprocess stress test at test_spans_ended_by_garbage_collection_do_not_deadlock is a good guard against future regressions.
  • The elif not parent.is_recording() cleanup at gen_ai_nested_client_span_processor.py:76-81 correctly handles the race where the parent ends between the is_recording() check and the setdefault; finalizer.detach() is idempotent so the double-detach after parent-on-end popped the entry is safe.
  • finalizer.atexit = False on :69 — correct, prevents finalizer running after interpreter shutdown when the processor state is already torn down.
  • Keying by (trace_id, span_id) instead of just span_id avoids cross-trace collisions.
  • Tests cover all the stated leak paths and both deadlock scenarios.

Minor observations (non-blocking, no change requested):

  • Under a parent-ends-concurrently-with-child.on_start race, the parent's on_end can pop the just-inserted (empty) entry before child.on_end records its operation. Demotion is skipped in that window. The PR description acknowledges races like this as acceptable, and no state leaks — just worth being aware of.
  • shutdown() doesn't set a terminal flag, so a late on_start after shutdown could repopulate _parents. In practice the TracerProvider is shut down with the processor and no new spans start, so this is theoretical.
  • The subprocess regression test uses gc.set_threshold(50) and a 20 s join / 40 s subprocess timeout. If CI runners are slow this could get flaky; keep an eye on it, but the tuning looks reasonable.

No changes required. LGTM.
· Branch: fix/gen-ai-nested-parent-state

if span.kind != SpanKind.CLIENT:
def on_start(self, span: Span, parent_context: Optional[Context] = None) -> None:
# Only a CLIENT child can demote its parent. Span kind is fixed at start in this SDK.
if span.kind != SpanKind.CLIENT or span.parent is None or span.parent.is_remote:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind is assumed fixed at start

on_start skips non-CLIENT children (span.kind != SpanKind.CLIENT, "Span kind is fixed at start in this SDK"). Main checked kind at end, and the JS version (#589) still does, because VercelAISpanProcessor promotes INTERNAL to CLIENT in onEnd.

This is fine today, since no distro code promotes a span to CLIENT. But if a future processor does what the Vercel one does, the outer span will silently stop being demoted, with no error and no failing test.

Suggestion: add a comment at that check explaining the assumption, plus a small guard test that scans the distro source for _kind assignments and compares them with an allowlist. Today there are three:

  • aws_lambda_span_processor.py → SERVER
  • this processor → INTERNAL
  • the opentelemetry_lite_sdk.py constructor

Any new _kind assignment would then fail CI and point back here.

@syed-ahsan-ishtiaque syed-ahsan-ishtiaque left a comment •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head c7e0ee4 against main and compared it with #971.

The original remote-parent, late-child, and abandoned-parent cases are fixed, and normal two-/three-level demotion works for all four inference operations. One cleanup issue is recorded inline: a parent from another tracer provider can finish without notifying this processor, leaving its state while the ended parent object remains reachable. This also introduces retention outside GenAI traffic: 1,000 completed requests with foreign SERVER parents and ordinary local HTTP CLIENT children retain 1,000 entries here, versus zero on main and #971.

I also verified the reconstructed-local-context trade-off documented in the description: wrapping an active local parent's span context in NonRecordingSpan leaves both spans CLIENT here; main and #971 demote the outer span. This is a comparison trade-off already disclosed by this PR.

Validation: 2,196 core distro tests and 351 subtests passed (25 skipped); Black, isort, and flake8 passed. Added independent baseline/fix lifecycle comparisons, all four inference operations, six-thread GC/end stress (1,600 span pairs and 2,502 kind assertions), and the streaming-generator GC regression. Optional framework suites were not run locally.

tracked = key in self._parents
if tracked:
return
finalizer = weakref.finalize(parent, self._collected.append, key)

@syed-ahsan-ishtiaque syed-ahsan-ishtiaque Nov 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Avoid tracking parents whose end callbacks this processor cannot observe

A recording SDK parent from another TracerProvider passes the checks above, so this registers it in _parents. When that parent ends, it calls its own provider's processors; this processor never receives the parent's on_end. The entry therefore remains while the ended parent object is reachable, since its finalizer observes collection rather than span end.

This also introduces retention for ordinary non-GenAI traffic. I reproduced 1,000 completed requests using SERVER parents from a second provider without this processor and local HTTP CLIENT children; neither span had any GenAI attributes. Retaining the ended parent objects, then running gc.collect(), force_flush(), and 1,000 unrelated completed spans leaves 1,000 entries here, versus zero on main and #971. Releasing the parent objects finally clears the entries.

The same ownership gap also leaves the original GenAI case partially unfixed: completed foreign-provider parents with CLIENT/chat children retain entries, including when the parent ends first. Please restrict tracking to parents whose lifecycle this processor observes, or provide cleanup for foreign-provider parents, and add cross-provider tests for both GenAI and ordinary HTTP children.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants