fix: recover ADK LLM spans when ContextCacheConfig detaches the async context (#5524) by lupuletic · Pull Request #7305 · comet-ml/opik

lupuletic · 2026-07-01T11:06:58Z

Summary

When ADK's ContextCacheConfig is enabled and responses stream over SSE (Cloud Run / production), Opik's per-LLM spans are never finalized — they stay at _OPIK_SPAN_STATUS: started with no output or usage.

Root cause. OpikTracer creates the LLM span in before_model_callback and finalizes it in after_model_callback, handing it over via Opik's contextvar span stack (OpikContextStorage). ContextCacheConfig adds extra handle_context_caching / create_cache OpenTelemetry spans (google_llm.py) whose context.attach()/detach() cycles, across an SSE async-generator suspension, revert the contextvar to a snapshot predating before_model_callback's push. So top_span_data() returns None in after_model_callback and the span is abandoned. (There's no asyncio.create_task/copy_context fork — it's purely OTel contextvar attach/detach imbalance, which caching tips over.)

Fix

Register each LLM span in before_model_callback under a stable, per-model-call key that survives the fork: id(callback_context.actions). ADK builds one EventActions per model call and passes the same instance to both before_ and after_model_callback (invariant across streaming partials); object identity is immune to contextvar mutation, unlike the span stack or the OTel span id.

after_model_callback treats this registry as authoritative for which span is ours:

Recover the span by key and finalize it whether or not the (possibly detached) stack still holds it.
Pop the context stack only when it actually holds our span — so a parent span left on top after a detach is never mis-finalized.

The registry (PendingLlmSpanRegistry) is thread-safe and size-bounded (evict-oldest), because ADK does not guarantee an after_model_callback for every before_model_callback (a before-callback can short-circuit by returning a response, or the model call can error), so unclaimed entries must not accumulate on a long-lived shared tracer. It holds a threading.Lock, so it's excluded from __getstate__ and recreated in __setstate__.

Why `id(callback_context.actions)` (not the alternatives)

invocation_id — per invocation, so tool-loop LLM calls share it; not unique per call.
OTel call_llm span id — read from the OTel current-span contextvar, i.e. the exact state the caching bug corrupts.
A LIFO fallback list — picks the wrong invocation's span under concurrent SSE sessions.

EventActions is a per-call object, is truthy (pydantic v2, no __bool__), and is exposed identically to both callbacks — so the key is unique-per-call, partial-invariant, and contextvar-free.

Tests

tests/library_integration/adk/test_adk_context_cache_recovery.py:

Registry unit tests (keyed isolation — no LIFO collision; bounded eviction).
Recovery tests driving the real before_model/after_model callbacks with the span stack manually detached, asserting the span is finalized (output + end time) from the registry; a normal-path test (finalizes + clears both stack and registry); and a parent-span-on-top test (finalizes our span, leaves the parent untouched).

ruff check / ruff format pass. (I couldn't run the full ADK suite locally — no google-adk in my env — but validated the registry and the recovery logic against ADK 1.31.1 source.)

Notes

Scoped to the modern OpikTracer (ADK >= 1.3.0). LegacyOpikTracer (ADK < 1.3.0) predates ContextCacheConfig.
Applies to Opik's agent-callback registration path; the key relies on the shared EventActions ADK passes to agent (not plugin) callbacks.

… context With ADK ContextCacheConfig enabled and SSE streaming (Cloud Run), the extra OTel attach/detach cycles from handle_context_caching/create_cache revert Opik's contextvar span stack to a snapshot predating before_model_callback's push. after_model_callback then sees top_span_data() == None and never finalizes the LLM span, so it is stuck at _OPIK_SPAN_STATUS=started with no output (opik#5524). Register each LLM span in before_model_callback under a stable, per-model-call key -- id(callback_context.actions), the EventActions instance ADK creates once per model call and passes to both callbacks; it is invariant across streaming partials and immune to contextvar mutation. after_model_callback treats this registry as authoritative for which span is ours: it recovers and finalizes the span whether or not the (possibly detached) stack still holds it, and only pops the stack when it actually does -- so a parent span left on top is never mis-finalized. The registry is thread-safe and size-bounded (evict oldest) so before-callbacks without a matching after-callback can't grow it. Adds PendingLlmSpanRegistry with unit tests and recovery tests that finalize a span with the stack detached and with a parent span left on top. Closes comet-ml#5524

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20cf2532de

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- Registry eviction / missing callback_context.actions: after_model_callback no longer treats a registry miss as authoritative. It falls back to the context stack top when that top is our not-yet-finalized LLM span (is_externally_created_llm_span_that_just_started), so an evicted entry or a callback context without `actions` can't drop an active span. A parent span left on top by a detached context fails that check and is never mis-finalized. before_model_callback guards the `actions` access with getattr. - Stale entry on finalization error: drop the registry entry in the finally on any final-response exit (success or error), instead of after the fallible finalization, so a reused id() can't later map to a stale span. - Duplication: extract the shared, thread-safe, size-bounded OrderedDict into BoundedCache; LastModelOutputCache and PendingLlmSpanRegistry now compose it, so eviction/locking lives in one place. - Tests: unit-test BoundedCache directly; drive recovery through the real before/after callbacks and read the span from the public context storage (not tracer internals); add a no-`actions` stack-fallback test and keep the parent-span-on-top test.

lupuletic · 2026-07-01T12:02:22Z

Thanks — all five addressed in the latest commit.

Registry eviction (Codex) + missing callback_context.actions (Baz). Both are the same failure — a registry miss — and both are fixed together: after_model_callback no longer treats a registry miss as authoritative. It falls back to the context stack top when that top is our not-yet-finalized LLM span (is_externally_created_llm_span_that_just_started), so an evicted entry or a callback context without actions still finalizes the active span via the normal path. A parent span left on top by a detached context fails that type=="llm"/STARTED check, so it's never mis-finalized. before_model_callback also guards the actions access with getattr. (For what it's worth, Context.actions is present in current google-adk — CallbackContext = Context, context.py's actions property — but the guard makes it robust to older/custom callbacks regardless.)

Stale entry on finalization error (Baz). The registry entry is now dropped in the finally on any final-response exit (success or error), instead of after the fallible update()/pop_span_data(). So if finalization raises, no stale span is left for a reused id() to map to. Partial chunks keep the entry for the final response.

Duplicated bounded-cache logic (Baz). Extracted BoundedCache (thread-safe, size-bounded OrderedDict); LastModelOutputCache and PendingLlmSpanRegistry now compose it, so eviction/locking lives in one place.

Private-state test assertions (Baz). The registry behaviour is now unit-tested through the shared BoundedCache directly; the recovery tests drive the real before/after callbacks and read the span from the public context_storage (not tracer._pending_llm_spans), asserting on public span fields (output, end_time). Added a no-actions test that exercises the stack fallback (same path as eviction) with no internal poking, plus the parent-span-on-top test.

ruff check/format pass; BoundedCache keying/eviction/8-thread concurrency verified locally. I still can't run the live Vertex+streaming path locally, so CI remains the final gate on the ADK integration tests.

lupuletic · 2026-07-01T12:07:14Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eeca0796db

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- Empty-content leak (Codex P2 / Baz): has_empty_text_part_content returned before the span/key was resolved, so on a detached or final empty response neither the inline TTFT cleanup (top_span_data() was None) nor the finally ran, leaking _ttft_tracking entries and holding _pending_llm_spans entries until eviction. Resolve the span + key up front (registry-first, stack fallback), record span_id early, and let the finally drop both the TTFT entry and the pending-registry entry for final responses (partial chunks keep them). The empty-content branch now just returns. - Test naming: rename the BoundedCache tests to the repo's test_WHAT__CASE__EXPECTED_RESULT convention; add an empty-final-response leak-regression test.

lupuletic · 2026-07-01T14:29:10Z

Thanks — both addressed in the latest commit.

Empty-content cleanup leak (Codex P2 / Baz — same root cause). has_empty_text_part_content returned before the span/key was resolved, so on a detached or final-empty response neither the inline TTFT cleanup (top_span_data() was None there) nor the finally ran — leaking _ttft_tracking entries and holding _pending_llm_spans entries until eviction (which, as Codex notes, Gemini streaming hits routinely). Fixed by resolving the span + key up front (registry-first, stack fallback) and recording span_id early; the empty-content branch now just returns, and the finally drops both the TTFT entry and the pending-registry entry for final responses while keeping them for partial chunks. Added a test_after_model__empty_final_response__does_not_leak_registry_entry regression test.

Test naming (Baz). Renamed the BoundedCache tests to the test_WHAT__CASE__EXPECTED_RESULT convention from design/TESTING.md (the test_after_model__… recovery tests already had all three segments).

ruff check/format pass. CI remains the gate on the live ADK integration path.

Strengthen the empty-final-response leak test so it fails on a broken registration too: assert the entry exists right after before_model_callback runs, then that it's gone after after_model_callback (per review).

lupuletic · 2026-07-02T11:14:38Z

@baz-reviewer good catch — strengthened test_after_model__empty_final_response__does_not_leak_registry_entry in f3d6df7 to assert the pending-span entry exists right after before_model_callback and is gone after after_model_callback, so it now fails on a broken registration as well as on a cleanup leak.

lupuletic · 2026-07-02T11:14:40Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3d6df7fb9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

id(actions) alone is unsafe for entries that outlive their callback: a call whose after_model_callback never runs (a short-circuiting before-callback or a model error) leaves its entry behind, its actions object is GC'd, and CPython can reuse that id for a later call's EventActions -- so after_model_callback would recover/finalize the stale span while the real span stays started (comet-ml#5524 review). Store a strong reference to the actions object alongside the span and verify identity on lookup: the strong ref keeps the object alive so its id can't be recycled while the entry is live, and the identity check refuses a stale span if an id ever collides. register/get/pop now take the actions object. Adds registry tests: distinct-actions isolation and a recycled-id identity-refusal test.

lupuletic · 2026-07-02T11:28:22Z

@codex good catch on the id()-recycling hazard — that's real for entries that outlive their callback (short-circuit / model error, where after_model_callback never fires). Fixed in the latest commit: PendingLlmSpanRegistry now stores a strong reference to the actions object alongside the span and verifies identity on lookup. The strong ref keeps the object alive so its id can't be recycled while the entry is live; the identity check refuses a stale span if an id ever collides anyway. register/get/pop take the actions object, and there's a recycled-id regression test that seeds a colliding slot and asserts the stale span is not returned.

@codex review

chatgpt-codex-connector · 2026-07-02T11:35:17Z

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: a9f9251146

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…ding them The pending-span registry is size-bounded to cap entries whose after_model_callback never runs (a short-circuited before-callback or a failed/cancelled model call). Its BoundedCache dropped the oldest entry silently, so the evicted span was left stuck in the 'started' state with no end time, output, or usage. Route evictions through a callback: BoundedCache gains an optional on_evict (fired outside the lock, for capacity evictions only), and the tracer finalizes the dropped span (ready-for-finalization status, end time, TTFT cleanup, flush) idempotently and best-effort. The common eviction target is an unclaimed span, so this closes spans that were previously leaked; a rare live span evicted under >max_size concurrency is closed early rather than stranded.

lupuletic · 2026-07-03T10:19:06Z

Good catch — fixed in ba41f63.

You're right that BoundedCache(max_size) couldn't tell an unclaimed pending span from a still-in-flight one, so a capacity eviction dropped whichever span was oldest and left it stranded in started. Going the "avoid evicting live spans" route isn't reliable (nothing on the entry marks live-vs-abandoned), so I took your second suggestion — preserve a finalizable handle on eviction:

BoundedCache gained an optional on_evict(key, value) callback, fired only for capacity evictions (never for an overwrite or an explicit pop) and outside the lock, so the callback can do slower work / re-enter the cache without deadlocking.
PendingLlmSpanRegistry routes each evicted entry's span to the tracer's _finalize_evicted_llm_span, which sets ready_for_finalization + an end time, drops the matching TTFT entry, and flushes — idempotently (skips a span already finalized, so it's safe against the race where after_model_callback wins) and best-effort (never raises out of set).

Worth noting this also improves the common path: the oldest entry is overwhelmingly an unclaimed one (short-circuited before-callback, or a failed/cancelled model call whose after_model_callback never fires), so those spans were being silently leaked in started before — now they're properly closed. The rare live-span eviction (needs >max_size truly-concurrent in-flight calls on one tracer) is closed early rather than stranded. Force-closed spans carry opik_finalized_on_registry_eviction: true in metadata so an incomplete span is distinguishable from a normally finalized one.

Covered by test_pending_llm_span_registry__eviction__finalizes_dropped_span_only (registry wiring) and test_finalize_evicted_llm_span__started_span__closed_idempotently (finalizer + idempotency).

lupuletic · 2026-07-03T10:19:18Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba41f631a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…e-prefix eviction metadata Two review follow-ups: - Pre-1.29 google-adk (where ContextCacheConfig + SSE can already strand the span) has no public CallbackContext.actions property -- the shared per-call EventActions is only reachable as the private _event_actions. getattr(..., 'actions', None) returned None there, so before_model_callback never registered and after_model_callback fell back to the old detached-context path, leaving the span unfinalized for the exact scenario this change recovers. Resolve the key via a shared helper that prefers .actions and falls back to _event_actions (both resolve to the same object, so the callbacks agree on the key), used in both callbacks. - Rename the force-closed marker to _opik_finalized_on_registry_eviction to follow the leading-underscore internal-metadata convention (cf. _OPIK_SPAN_STATUS, _opik_graph_definition).

lupuletic · 2026-07-03T10:38:31Z

Both addressed in 4677e29.

@chatgpt-codex-connector — pre-1.29 _event_actions (P2): correct and important — for the ADK range where ContextCacheConfig exists but CallbackContext has no public .actions property, getattr(cc, "actions", None) returned None, so nothing was registered and recovery fell back to the old detached-context path for the exact scenario this PR targets. Fixed with a shared resolver used by both callbacks:

def _resolve_event_actions(callback_context):
    actions = getattr(callback_context, "actions", None)
    if actions is None:
        actions = getattr(callback_context, "_event_actions", None)
    return actions

Since the public property just returns self._event_actions, both paths resolve to the same object, so before_/after_model_callback agree on the key. Covered by test_after_model__legacy_event_actions__recovers_via_private_attr (a callback context exposing only _event_actions, driven through the detach path).

@baz-reviewer — internal metadata key naming: agreed — renamed opik_finalized_on_registry_eviction → _opik_finalized_on_registry_eviction to match the leading-underscore internal-metadata convention (_OPIK_SPAN_STATUS, _opik_graph_definition).

lupuletic · 2026-07-03T10:38:33Z

@codex review

chatgpt-codex-connector · 2026-07-03T10:38:46Z

To use Codex here, create an environment for this repo.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4677e2990f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

When ADK SSE-streams a response, the text can arrive in partial=True chunks and the terminal partial=False chunk can be empty. That empty terminal hits has_empty_text_part_content() and returned early before finalizing current_span, then the finally dropped the pending-span registry entry -- so under a detached ContextCacheConfig context (where the span is not on the stack either) the recovered span lost its only handle and stayed 'started' with no end time, output, or usage. Force-close the recovered span on the terminal empty response instead of just dropping it (popping it off the stack first if present). Partial empty chunks are unchanged: the span/TTFT/registry entry are kept so the later final response can finalize normally. Generalizes _finalize_evicted_llm_span into _force_close_llm_span(reason) shared by the eviction and empty-terminal paths; the force-closed marker becomes _opik_llm_span_force_closed_reason (registry_eviction / empty_terminal_response), still leading-underscore per the internal-key convention.

lupuletic · 2026-07-03T10:53:27Z

Fixed in b747380 — good catch, this was a real hole in the recovery.

You're right: when text streams as partial=True chunks and the terminal partial=False chunk is empty, has_empty_text_part_content() returned early before finalizing current_span, then the finally dropped the registry entry — so under a detached ContextCacheConfig context (span not on the stack either) the recovered span lost its only handle and stayed started.

I took the force-close option you offered (we don't buffer partial text, so there's nothing to accumulate from — the terminal chunk is genuinely empty). On the terminal empty response, when a span was recovered, it's now closed via the shared force-close path (ready-for-finalization status + end time + TTFT cleanup + flush), popping it off the stack first if it's there:

if adk_helpers.has_empty_text_part_content(llm_response):
    if not is_partial and current_span is not None:
        if stack_top is not None and stack_top.id == current_span.id:
            context_storage.pop_span_data(ensure_id=current_span.id)
        self._force_close_llm_span(current_span, reason="empty_terminal_response")
    return

Notes:

Partial empty chunks are unchanged — the span, its TTFT entry, and its registry entry are kept so the later final response finalizes normally (test_after_model__empty_partial_chunk__keeps_span_for_final_response).
Tool-call-only terminals don't take this path — has_empty_text_part_content is False when the single part is a function call (part.text is None), so those still finalize with their output.
Refactored the eviction finalizer into a shared _force_close_llm_span(reason); the marker is now _opik_llm_span_force_closed_reason (registry_eviction / empty_terminal_response), still leading-underscore per the internal-key convention.

New tests: ..._empty_final_response_detached__force_closes_recovered_span (the #5524 case), ..._empty_final_response_on_stack__force_closes_and_drops_entry, and the partial-chunk keep-alive above.

lupuletic · 2026-07-03T10:53:29Z

@codex review

chatgpt-codex-connector · 2026-07-03T11:13:25Z

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: b74738033d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

A force-closed span is set to READY_FOR_FINALIZATION, but the after_model_callback stack fallback only reclaims STARTED spans, so a force-closed span left on the context stack could not be reclaimed there and would mis-parent later directly-created spans. - _force_close_llm_span now pops the span off the current context stack when it is on top (covers the empty-terminal path and a same-context eviction; the redundant inline pop in the empty-terminal branch is removed). - before_model_callback creates its LLM span directly, bypassing the OTel patcher's READY_FOR_FINALIZATION reclaim, so it now first discards a stale force-closed top span via _discard_stale_finalized_top_span (a STARTED in-flight parent is never touched). This covers a span force-closed in a different async context (a concurrent registry eviction) that could not be popped there. Also add the fake_backend fixture to the tracer tests so span submissions hit the backend emulator instead of a real backend, matching the other ADK integration tests.

lupuletic · 2026-07-03T13:41:09Z

Both addressed in 0f832d8.

@baz-reviewer — "Evicted span stays on stack" (HIGH): correct — a force-closed span is READY_FOR_FINALIZATION, but the after_model_callback stack fallback only reclaims STARTED, so a force-closed span left on the stack couldn't be reclaimed and would mis-parent later spans. Fixed on both sides:

_force_close_llm_span now pops the span off the current context stack when it's on top (covers the empty-terminal path and a same-context eviction). The redundant inline pop in the empty-terminal branch is removed.
before_model_callback creates its LLM span directly (bypassing the OTel patcher's is_externally_created_llm_span_ready_for_immediate_finalization reclaim at opik_adk_otel_tracer.py), so it now first calls _discard_stale_finalized_top_span() to drop a stale force-closed top span before creating the new one. This is the one path that can see a span force-closed in a different async context (a concurrent registry eviction that couldn't pop that context's stack). A STARTED in-flight parent is never touched.

Tests: test_before_model__stale_force_closed_top_span__discarded_not_used_as_parent (the new span isn't parented under the stale one and the stale span is gone) and test_before_model__started_top_span__kept_as_parent (a genuine STARTED parent survives).

@baz-reviewer — "Real backend span submission": good catch — added the fake_backend fixture to all nine tracer tests so __internal_api__span__ hits the emulator, matching the other ADK integration tests.

FYI a few older inline comments got re-anchored to 0f83/b747 by the diff shift but were already resolved in earlier commits: the metadata underscore rename (now _opik_llm_span_force_closed_reason), the "stale span in finally" (the registry pop has been in finally since ba41f63), and Codex's empty-SSE-terminal P1 (fixed in b747380).

lupuletic · 2026-07-03T13:41:12Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0f832d83d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Force-finalizing on registry eviction corrupted a still-live call: under more than max_size concurrently-in-flight model calls, the evicted oldest entry can belong to a call whose after_model_callback has not run yet. Marking that span READY_FOR_FINALIZATION and sending it empty replaced a normally-traceable call with an empty span, and -- because the stack fallback only reclaims STARTED spans -- blocked its real after_model_callback from finalizing it. Evict the entry without mutating the span instead: the span stays on the context stack where the normal STARTED stack fallback still finalizes it (non-detached), and beyond the bound the detached-context recovery simply reverts to the pre-registry behavior -- strictly no worse. This also removes the force-closed span that could linger on the stack, so the before_model stale-top guard and its duplicate of the OTel patcher logic are gone too. Reverts the eviction on_evict/_finalize_evicted_llm_span/ _discard_stale_finalized_top_span machinery. Keeps _force_close_llm_span for the terminal empty-SSE-response path, which is the genuine final callback and so has no later after_model_callback to corrupt.

lupuletic · 2026-07-03T14:09:31Z

Both addressed in 03fe5dc — and they pointed at the same root cause, so I took Codex's option (a) and it resolves all of it.

@chatgpt-codex-connector — "Avoid force-closing live evicted LLM spans" (P2): you're right, and this is the important one. Under >max_size in-flight calls the evicted oldest entry can be a still-live call; force-closing it marked it READY_FOR_FINALIZATION + sent it empty, and since the stack fallback only accepts STARTED, its real after_model_callback could no longer finalize it — a traceable call became an empty span. I took "evict the entry without mutating the span": eviction now just drops the registry entry. The span stays on the context stack, where the normal STARTED stack fallback still finalizes it (non-detached); beyond the bound the detached-context recovery simply reverts to the pre-registry behavior — strictly no worse than before this PR.

This was the right call in hindsight: force-finalizing on eviction (added last round) caused more harm than the stranding it was trying to prevent, and eviction only triggers under >1000 concurrent in-flight calls on one tracer, which is practically unreachable.

@baz-reviewer — "Duplicate stale-span cleanup logic": resolved by the same change. Dropping the eviction force-close means no force-closed span can linger on the stack, so before_model_callback's _discard_stale_finalized_top_span (the duplicate of opik_adk_otel_tracer.py:141-156) is removed entirely rather than deduplicated — the OTel patcher's existing reclaim is the single source of truth again.

Net: reverted the on_evict / _finalize_evicted_llm_span / _discard_stale_finalized_top_span machinery. Kept _force_close_llm_span only for the terminal empty-SSE-response path (Codex's earlier P1) — that is the genuine final callback for the call, so there's no later after_model_callback to corrupt. Registry eviction is now covered by test_pending_llm_span_registry__over_capacity__drops_oldest_without_mutation (asserts the dropped span is left untouched).

lupuletic · 2026-07-03T14:09:32Z

@codex review

chatgpt-codex-connector · 2026-07-03T14:09:44Z

To use Codex here, create an environment for this repo.

chatgpt-codex-connector · 2026-07-03T14:15:03Z

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 03fe5dcfb1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

lupuletic · 2026-07-07T08:17:01Z

@alexkuzmik when you have a moment — this is the third in the ADK-tracer series you merged (#7266 output isolation, #7282 trace-output recovery), and it fixes the remaining case those two didn't: the per-LLM span itself getting stuck at started under ContextCacheConfig + SSE (issue #5524).

Both automated reviewers are clean (Codex: "no major issues"; Baz: passing) and all review threads are resolved. It's mergeable and up to date with main. Happy to rebase or adjust anything if needed.

lupuletic requested a review from a team as a code owner July 1, 2026 11:06

github-actions Bot added python Pull requests that update Python code tests Including test files, or tests related like configuration. Python SDK labels Jul 1, 2026

lupuletic force-pushed the fix/adk-context-cache-span-recovery branch from 20cf253 to 16db905 Compare July 1, 2026 11:09

chatgpt-codex-connector Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

baz-reviewer Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

Comment thread sdks/python/tests/library_integration/adk/test_adk_context_cache_recovery.py Outdated

chatgpt-codex-connector Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

baz-reviewer Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread sdks/python/tests/library_integration/adk/test_adk_context_cache_recovery.py Outdated

lupuletic marked this pull request as draft July 1, 2026 20:56

test: assert pending-span registration before asserting cleanup

f3d6df7

Strengthen the empty-final-response leak test so it fails on a broken registration too: assert the entry exists right after before_model_callback runs, then that it's gone after after_model_callback (per review).

baz-reviewer Bot added the baz: pending label Jul 2, 2026

chatgpt-codex-connector Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

baz-reviewer Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/pending_llm_spans.py

chatgpt-codex-connector Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

baz-reviewer Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py

chatgpt-codex-connector Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py

baz-reviewer Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/tests/library_integration/adk/test_adk_context_cache_recovery.py Outdated

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

chatgpt-codex-connector Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

baz-reviewer Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread sdks/python/src/opik/integrations/adk/opik_tracer.py Outdated

Merge branch 'main' into fix/adk-context-cache-span-recovery

0ba4a26

lupuletic marked this pull request as ready for review July 7, 2026 08:02

Merge branch 'main' into fix/adk-context-cache-span-recovery

6802570

lupuletic mentioned this pull request Jul 7, 2026

ADK integration: LLM spans stuck at 'started' when context caching is enabled (SSE streaming) #5524

Open

Uh oh!

Conversation

lupuletic commented Jul 1, 2026

Summary

Fix

Why id(callback_context.actions) (not the alternatives)

Tests

Notes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lupuletic commented Jul 1, 2026

Uh oh!

lupuletic commented Jul 1, 2026

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

lupuletic commented Jul 1, 2026

Uh oh!

Uh oh!

lupuletic commented Jul 2, 2026

Uh oh!

lupuletic commented Jul 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

lupuletic commented Jul 2, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 2, 2026

Uh oh!

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

lupuletic commented Jul 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Why `id(callback_context.actions)` (not the alternatives)