[liustve]

Prevent leaked parent state in the GenAI nested CLIENT span processor.

• Track active local CLIENT spans and clean up on end or garbage collection.
• Prevent remote/untracked parents and late children from retaining or recreating state.
• Key state by trace ID and span ID to avoid cross-trace collisions.
• Add atomic, reentrant DictWithLock operations and immutable snapshots for tests.

Validation: 162 tests and 29 subtests passed, including GC and concurrency regressions.

[github-actions]

Claude PR Review

Reviewing...

• Check prior review comments to dedupe
• Read PR diff
• Examine changed files
• Post review feedback

View job run • fix/genai-nested-span-state-cleanup

[github-actions]

Claude finished @liustve's task in 5m 2s —— View job

---

Claude PR Review

• Check prior review comments to dedupe
• Read PR diff
• Examine changed files
• Post review feedback

Summary

Thorough and well-crafted fix. No blocking issues found; nits only.

What works well

• Lifetime safety via weakref finalizer: on_start registers remove_collected_span so an abandoned local CLIENT span (parent never calls on_end) can't leak state. The pop_if_matches guard (state.span_ref is span_ref) correctly handles the span-id collision case where a newer entry replaced the old one. (gen_ai_nested_client_span_processor.py:45-54)
• Race fix via update_if_present: Updating the parent entry only if it still exists prevents the child from recreating state for an already-ended parent — the key invariant that keeps the dict bounded. (gen_ai_nested_client_span_processor.py:83-87)
• Correct RLock choice: Needed because the weakref callback can fire re-entrantly from inside operations that already hold the lock (e.g., during GC triggered by get_dict's mapper). The subprocess-isolated GC test in test_dict_with_lock_get_dict_handles_weak_reference_cleanup is a nice touch to avoid hanging the suite on regression.
• Keying by (trace_id, span_id) eliminates the cross-trace span-id collision risk (verified by test_on_end_same_span_id_in_different_traces_does_not_share_state).
• is_remote filter on the parent side plus the "update only if present" check on the child side gives two independent lines of defense against unbounded growth under remote-parent traffic (verified by the 1000-iteration test).
• Immutable snapshot API (MappingProxyType + frozenset) keeps the test accessor read-only without exposing internal mutable state.

Minor observations (non-blocking)

• The attribute/accessor name _active_parent_span_id_and_operation_to_gen_ai_client_child was accurate when the key was (span_id, operation) and value was True. It is now keyed by (trace_id, span_id) with value _ParentSpanState(span_ref, {operations}) — the name no longer matches the structure. Consider something like _active_parents or _active_parent_states for readability. The public getter inherits the same length. (gen_ai_nested_client_span_processor.py:37)
• _ParentSpanState.child_operations: set[str] is a mutable field on a NamedTuple — intentional and safe here (always mutated under lock in update_if_present), but worth a one-line comment that the set is lock-guarded, since NamedTuple normally implies full immutability.
• on_start creates an entry for every CLIENT span (including non-GenAI HTTP/DB clients); that entry is always popped in on_end. Not a leak, but it means every CLIENT span pays a dict insert + weakref registration. Acceptable for the correctness it buys, just worth being aware of.
  • fix/genai-nested-span-state-cleanup

[github-actions]

Claude finished @liustve's task in 7m 16s —— View job

---

Claude PR Review

• Check prior review comments to dedupe
• Read PR diff
• Examine changed files
• Post review feedback

Summary

The latest commit (353f84e "Clarify GenAI parent state mapping and locking") addresses both nits raised in my previous review:

• The attribute _active_parent_span_id_and_operation_to_gen_ai_client_child is now renamed to _active_trace_id_and_span_id_to_parent_span_state, and the public getter to get_active_parent_operations() — these names now accurately reflect that the key is (trace_id, span_id) and the value is _ParentSpanState. (gen_ai_nested_client_span_processor.py:38)
• The _ParentSpanState.child_operations field now carries a line comment stating that the set is lock-guarded — exactly what I asked for to make the mutability-on-a-NamedTuple intent obvious. (gen_ai_nested_client_span_processor.py:25-26)

I spot-checked the remaining logic (weakref finalizer + pop_if_matches, update_if_present race fix, keying by (trace_id, span_id), is_remote filter, immutable snapshot) and have no new substantive findings. LGTM.
• fix/genai-nested-span-state-cleanup

[github-actions]

Claude finished @liustve's task in 58s —— View job

---

Claude PR Review

• Check prior review comments to dedupe
• Read PR diff
• Examine changed files
• Post review feedback

Summary

The only new commit since my prior LGTM (353f84e) is 8fcac5d "Document span kind assumption in GenAI nested processor", which adds a two-line explanatory comment in on_start noting that span kind is fixed at start in this SDK, so only spans that start as CLIENT are tracked. No functional change.

The comment is accurate and useful — it answers the natural question "why do we filter on span kind at on_start rather than at on_end when the demotion happens there?" and documents the assumption that if a downstream processor later rewrites a span to CLIENT, that span would not be tracked by this processor.

No new findings. Still LGTM.
• fix/genai-nested-span-state-cleanup

[github-actions]

Claude finished @liustve's task in 2m 22s —— View job

---

Claude PR Review

• Check prior review comments to dedupe
• Read PR diff
• Examine changed files
• Post review feedback

Summary

The only new commit since my prior LGTM (8fcac5d) is 0eb59a9 "Simplify GenAI span cleanup and restore test helper", which addresses two of @syed-ahsan-ishtiaque's three nits:

• Plain pop in the weakref callback: remove_collected_span now does pop(span_key) directly, and pop_if_matches (plus its two tests) is deleted from DictWithLock. The simplification is correct — the weakref holds the only strong reference to its own callback via the _ParentSpanState entry, so if the entry is popped or replaced, the weakref is dropped and its callback cannot fire. The RLock is still needed because the callback can fire re-entrantly from inside operations that already hold the lock (e.g., GC triggered by get_dict's mapper), and the subprocess-isolated regression test now exercises that path with the plain pop. (gen_ai_nested_client_span_processor.py:49-52)
• _make_llm_span helper restored in the test file, trimming the repeated two-line boilerplate from each test. The new test_<method>_<scenario> names are kept, which the author explained as a deliberate convention. (test_gen_ai_nested_client_span_processor.py:32-35)

The third nit (replace get_active_parent_operations() with direct private-field reads using # noqa: SLF001) was declined by the author in favor of keeping the read-only public snapshot. That's a reasonable judgment call — the snapshot is ~15 lines and insulates tests from the internal layout.

I re-verified the rest of the processor logic (weakref finalizer, update_if_present race fix, keying by (trace_id, span_id), is_remote filter, immutable snapshot) and found no new issues. Still LGTM.
• fix/genai-nested-span-state-cleanup