fix(otel): release unreleased scope on operation re-entry

[wangyb-A]

Problem

Follow-up to #650, addressing the P1 finding from its automated review.

wrap_user_function re-raises SuspendExecution before on_user_function_end, so a user function that suspends never reports an end and the OTel context scope it attached is never released. The map/parallel coordinator resumes a timed suspend in-process (concurrency/executor.py, the while timed_resumes and timed_resumes[0][0] <= now: ... submit(branch) loop), re-entering the same operation ID on the same worker thread. The second _attach_context then overwrote the first token in the registry:

1. Run 1 attaches token A for child context ctx-N; the branch suspends, so A is never detached.
2. The timed resume re-enters ctx-N; the registry entry is replaced, making A unreachable forever.
3. Run 2 finishes and detaches its own token, which resets the context variable to its value at that token's creation — the abandoned span.

The result is an ended-but-current span for any later work on that pool thread: stale parenting for auto-instrumented spans and stale trace/span ids in logs. Scope is confined to one invocation, because the branch pools are shut down before the invocation returns.

Change

_attach_context now releases an existing scope for the same key before attaching the new one, in both plugins. Nothing else changes: the normal start/end pairing, the invocation-level scope, and the cross-thread guard all behave as before.

The message is logged at debug level rather than warning. An in-process timed resume makes re-entry expected today, and a map with many waiting branches would otherwise emit one warning per resume. Once the SDK reports user functions that do not complete, re-entry becomes genuinely unexpected and the level can be raised.

Why this is a mitigation, not the root fix

The root cause is that the SDK never notifies plugins when a user function fails to complete. Three exception types bypass the except Exception branch in wrap_user_function because they subclass BaseException: SuspendExecution/TimedSuspendExecution (exceptions.py:468,479), OrphanedChildException (exceptions.py:549) and BackgroundThreadError (exceptions.py:447).

This change closes the re-entry path, which is the only one that silently loses a token. Three cases remain, each now pinned by a test rather than left implicit:

• Nested unwinding. When an outer child context and an inner one both suspend, re-entry releases scopes in replay order, not in reverse attach order, so ending the inner operation restores the scope captured for the abandoned outer span. Deterministic CONTEXT span ids make that indistinguishable downstream — same trace id and span id, so parenting and log correlation are unaffected — but the current span object is never exported, so anything an instrumentation library records on it is lost.
• Resume on a different worker. A token attached on another thread cannot be reset here, so it is dropped. The worker that suspended keeps the abandoned span current for the windows between attached scopes on that thread.
• Orphaned branches. OrphanedChildException abandons a scope with no re-entry to trigger this guard.

All three need a hook fired on the originating worker thread whenever a user function does not complete — a core SDK API addition, filed separately.

Tests

Five tests per plugin. The three re-entry tests fail without the change; the two limitation tests document behaviour the guard does not alter:

• re-entry of a suspended child context leaves the pre-operation context current, not the abandoned span
• re-entry of the same step attempt key releases the previous scope
• nested re-entry: resumed inner code runs under the resumed inner span, ending the inner operation restores the abandoned outer span object with identical ids, and leaving the outer context still unwinds to the pre-invocation context
• re-entry on another thread: the foreign token is dropped, the re-entering thread unwinds cleanly, and the still-running worker is probed to show it keeps the abandoned span current
• the autouse fixture from fix(otel): balance otel context attach and detach #650, asserting each test leaves the OTel context as it found it, covers all of the above

The two limitation assertions flip once the core hook exists, so they act as tripwires rather than as blessed behaviour.

Verification

• hatch run dev-otel:test — 146 passed
• hatch run test:all — 3244 passed, 2 skipped
• hatch run types:check — clean
• hatch fmt --check (otel package) — clean
• Reverting only the source change fails 6 of the 8 re-entry tests, including both nested tests
• Throwaway runtime probe using DurableFunctionTestRunner with a real parallel branch that waits while its sibling works, i.e. the inline-wait resume shape covered by wait_inline_completion_test.py. The execution succeeded and the guard fired exactly once, for the resumed branch's child-context key. That confirms the re-entry actually happens in a real execution rather than only in the unit-test hook sequence.

[wangyb-A]

Fair catch, and taken in 9ab5dc1. The with ThreadPoolExecutor(...) block did shut the pool down before the assertions, so the contaminated worker was already gone. The test now keeps the worker alive and probes it after re-entry, in both plugins: the foreign token is dropped rather than reset, the re-entering thread unwinds to its pre-operation context, and the worker that suspended is asserted to still carry the abandoned span.

Two clarifications on impact. The plugin's own operation spans are not misparented, because _start_span takes its parent from the span registry rather than from the ambient context, and a later branch on that worker attaches its own scope built from _extracted_context. What is affected is auto-instrumented spans and get_current_span_context() log correlation in the windows between attached scopes on that thread.

Agreed that the real cleanup has to run on the originating worker. That needs a hook fired from wrap_user_function when a user function does not complete, which is a core SDK API addition and outside this PR; note it must cover OrphanedChildException and BackgroundThreadError too, since both also subclass BaseException and skip the end hook. I am filing that separately, and this test flips to the correct assertion once it lands.

[github-actions]

Codex AI review

[P2] Preserve LIFO order when replacing a scope. If this key belongs to an outer context while an inner scope remains current, detaching only the outer token resets the context stack out of order. Re-entering the inner context then captures the abandoned outer span, so resumed outer code records data on a span that is never exported. On same-thread re-entry, unwind all newer tokens for that thread in reverse attachment order through this key before attaching the replacement. Apply the same fix in invocation_plugin.py and update the nested tests to require the resumed outer span to be current.

[github-actions]

Codex AI review

Found one correctness issue in nested in-process re-entry. Static review only; repository tests were not executed.

Reviewed commit 9ab5dc12a9965058178a2f289f811a63c9fdcf2f. Workflow run

[github-actions]

Claude AI review

No actionable findings. The change is a correct, tightly-scoped fix: _attach_context releases an already-registered scope before attaching a new one, closing the token-leak path where a suspended-then-in-process-resumed operation re-enters the same key and the later detach would restore the abandoned span as current.

Verification performed:

• Same-thread re-entry (packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/execution_plugin.py:176-182 and invocation_plugin.py:178-184): detaching token A restores the pre-op context before token B is attached, so the end hook unwinds to the pre-operation context rather than the abandoned span. Correct.
• Cross-thread re-entry: _detach_context pops the foreign entry but skips the actual otel_context.detach (thread guard), so no cross-thread reset error, and the registry ends in the same state as before the change — no regression. The residual "originating worker stays dirty" behavior is a documented, test-pinned limitation, not introduced here.
• Recursive locking: _attach_context calls _detach_context while holding the lock; both _lock and _operation_spans_lock are pre-existing RLocks, so the re-entrant acquisition is safe.
• Normal flow: the detach branch only fires on genuine suspend+resume re-entry (step retries increment the attempt key; context/step keys don't collide; the invocation-scope key is cleared by _reset_state before any re-attach), so the standard start/end pairing is unchanged.

Pre-existing behaviors the PR correctly leaves out of scope (not new defects): the abandoned span object itself is never ended/exported, and the nested-unwind and cross-thread cases remain imperfect pending a core-SDK "user function did not complete" hook. Each is pinned by a tripwire test.

Test adequacy: strong. Four new tests per plugin (same-thread single re-entry, same step-attempt re-entry, cross-thread limitation, nested-unwind limitation) plus the autouse context-balance fixture cover the changed behavior. Residual test risk is limited to the deliberately-documented limitation assertions, which are framed as tripwires that flip when the root-cause hook lands — appropriate given this PR is explicitly a mitigation.

Reviewed commit 9ab5dc12a9965058178a2f289f811a63c9fdcf2f. Workflow run

[wangyb-A]

Filed the follow-up for the root cause: #658 — notify plugins when a user function does not complete, fired from a finally in wrap_user_function so it also covers OrphanedChildException, BackgroundThreadError and SystemExit/KeyboardInterrupt, not just suspension.

That issue is what supersedes the per-thread LIFO unwind suggested in review here: with the hook firing on the originating thread as the exception unwinds, inner scopes are released before outer ones by construction, so no stale token exists at re-entry and no per-thread scope stack is needed in the plugins. It also covers the two cases this PR could not: a resume landing on a different worker, and orphaned branches, which have no re-entry to trigger the guard.

The two limitation tests added here assert current behaviour deliberately and will fail when #658 lands, which is the signal to update them.