Skip to content

Commit 1e9fa5c

Browse files
author
Alex Wang
committed
fix(otel): stop inferring nesting from parent_id
Reverts the ancestry check from the previous commit and fixes the order in which the attached context is built. Both were review findings; the first one was a regression I introduced. parent_id is checkpoint hierarchy, not the Python call stack. A virtual (FLAT) map/parallel branch deliberately reports its inner operations' parent as the grandparent -- None for a top-level branch -- while the branch's own context scope is still running (see DurableContext.is_virtual and create_child_context, where child_parent_id is the *parent's* parent when is_virtual). Treating such an inner step as root-level therefore detached the live branch scope at the first inner step, and work between two inner steps fell out of the durable trace. That is worse than the abandoned sibling scope the check was meant to catch, so the narrower same-key guard is restored. The gap that leaves -- a scope abandoned by a *different* operation on the same branch-pool worker -- cannot be closed from the hook payloads, because a live FLAT branch scope is indistinguishable from an abandoned sibling. It needs the SDK to report the end of a suspended user function, which is tracked separately; the docstring says so rather than implying the helper handles it. Second finding: the context to attach was built by the caller before enter_scope ran its cleanup, so it copied baggage and suppression values out of the very scope about to be detached, and detaching afterwards could not remove them from an already-built Context. enter_scope now takes a factory and calls it after cleanup. Adds a FLAT-branch test asserting the branch scope stays current across two inner steps that report no parent, and a test that the factory observes the post-cleanup context. Drops the three tests that asserted the reverted rule.
1 parent 6c0c70f commit 1e9fa5c

4 files changed

Lines changed: 176 additions & 173 deletions

File tree

packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/context_scope.py

Lines changed: 44 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
import logging
3434
import threading
3535
from dataclasses import dataclass
36-
from typing import TYPE_CHECKING, Any
36+
from typing import TYPE_CHECKING, Any, Callable
3737

3838

3939
if TYPE_CHECKING:
@@ -78,34 +78,36 @@ def _detach(entry: _Entry) -> None:
7878
def enter_scope(
7979
owner: Any,
8080
key: str,
81-
context: Context,
81+
context_factory: Callable[[], Context],
8282
epoch: int = 0,
83-
parent_key: str | None = None,
8483
) -> None:
85-
"""Attach ``context`` on this thread and remember how to restore it.
84+
"""Attach a context on this thread and remember how to restore it.
8685
87-
Scopes this owner holds that the new one does not nest inside are unwound
88-
first, as are any left over from an earlier ``epoch``. Both cover the paths
89-
where a paired pop never runs: the SDK re-raises ``SuspendExecution`` without
86+
Scopes left over from an earlier ``epoch``, or from an earlier entry under the
87+
same ``key``, are unwound first: the SDK re-raises ``SuspendExecution`` without
9088
calling ``on_user_function_end``, so a suspended operation leaves its scope
91-
attached, and the worker that ran it goes on to other work.
89+
attached.
90+
91+
``context_factory`` is called *after* that cleanup, not before. The context to
92+
attach is normally derived from what is current, so building it first would
93+
copy values from a scope that is about to be detached -- baggage, suppression
94+
flags -- and detaching afterwards cannot remove them from a context that has
95+
already been constructed.
9296
9397
Args:
9498
owner: The plugin instance pushing the scope.
9599
key: Registry key for the scope, unique per owner (operation or attempt).
96-
context: The context to attach.
100+
context_factory: Builds the context to attach, called after cleanup.
97101
epoch: The owner's invocation counter; scopes from older epochs are
98102
discarded before the new scope is pushed.
99-
parent_key: Scope key of the enclosing operation, or None for a
100-
root-level one. Used to tell legitimate nesting from a stale scope.
101103
"""
102104
from opentelemetry import context as otel_context
103105

104106
owner_id = id(owner)
105107
_discard_stale(owner_id, epoch)
106-
_unwind_non_ancestors(owner_id, parent_key)
108+
_discard_reentered(owner_id, key)
107109
try:
108-
token = otel_context.attach(context)
110+
token = otel_context.attach(context_factory())
109111
except Exception: # noqa: BLE001
110112
logger.debug("Failed to attach OTel context scope %s", key, exc_info=True)
111113
return
@@ -154,48 +156,38 @@ def depth(owner: Any | None = None) -> int:
154156
return sum(1 for entry in _state.entries if entry.owner_id == owner_id)
155157

156158

157-
def _unwind_non_ancestors(owner_id: int, parent_key: str | None) -> None:
158-
"""Drop scopes this owner holds on this thread that the new scope is not inside.
159-
160-
A scope may stay attached only while the operation it belongs to is still
161-
running on this thread, and the only place that can be checked is here: the
162-
suspending path has no end hook, so a suspended operation leaves its scope
163-
behind. Two cases produce one:
164-
165-
* A branch-pool worker has no branch affinity. If branch A suspends and its
166-
worker next runs branch B, A's scope has the same epoch and a different key,
167-
so neither the epoch nor a same-key check clears it. B would nest inside A
168-
and, on exit, detach back into it.
169-
* The same operation can be re-entered when its branch is resubmitted.
170-
171-
The new scope nests inside its parent operation, so anything above that parent
172-
is stale. When the parent is absent -- a root-level operation, or a parent that
173-
never ran on this thread -- nothing this owner holds here can enclose it.
174-
175-
Detaching necessarily discards entries stacked above the cut, including other
176-
owners', because the underlying ``ContextVar`` can only be reset in order. The
177-
normal nesting path is a no-op, so that does not disturb a second plugin
178-
tracking the same operations.
159+
def _discard_reentered(owner_id: int, key: str) -> None:
160+
"""Unwind a scope this owner already holds under ``key`` on this thread.
161+
162+
The epoch check only catches a *previous invocation's* leftovers. The same
163+
operation key can also be entered twice inside one invocation, when a
164+
suspended operation is re-entered after its branch is resubmitted, and its
165+
first scope is still attached because the suspending path had no end hook to
166+
pop it. Without this, the second enter would stack on the first and the
167+
eventual end hook -- which pops one scope -- would leave the original
168+
attached.
169+
170+
A scope abandoned by a *different* operation on this thread cannot be
171+
detected here. Physical nesting is not derivable from the hook payloads:
172+
``parent_id`` is checkpoint hierarchy, and a FLAT map/parallel branch
173+
deliberately reports its inner operations' parent as the grandparent (see
174+
``DurableContext.is_virtual``), so a live branch scope would be
175+
indistinguishable from an abandoned sibling. Closing that gap needs the SDK
176+
to report the end of a suspended user function, which it does not do today.
179177
"""
180-
positions = [
181-
position
182-
for position, entry in enumerate(_state.entries)
183-
if entry.owner_id == owner_id
184-
]
185-
if not positions:
186-
return
187-
if parent_key is not None and _state.entries[positions[-1]].key == parent_key:
188-
# Normal nesting: the innermost scope we hold is the new scope's parent.
178+
index = next(
179+
(
180+
position
181+
for position, entry in enumerate(_state.entries)
182+
if entry.owner_id == owner_id and entry.key == key
183+
),
184+
None,
185+
)
186+
if index is None:
189187
return
190-
cut = positions[0]
191-
if parent_key is not None:
192-
for position in reversed(positions):
193-
if _state.entries[position].key == parent_key:
194-
cut = position + 1
195-
break
196-
for entry in reversed(_state.entries[cut:]):
188+
for entry in reversed(_state.entries[index:]):
197189
_detach(entry)
198-
del _state.entries[cut:]
190+
del _state.entries[index:]
199191

200192

201193
def _discard_stale(owner_id: int, epoch: int) -> None:

packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/execution_plugin.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -524,9 +524,10 @@ def on_user_function_start(self, info: UserFunctionStartInfo) -> None:
524524
context_scope.enter_scope(
525525
self,
526526
self._scope_key(info),
527-
trace.set_span_in_context(span, self._scope_base_context()),
527+
# Built after enter_scope's cleanup so it cannot inherit values
528+
# from a scope that is about to be detached.
529+
lambda: trace.set_span_in_context(span, self._scope_base_context()),
528530
epoch=self._epoch,
529-
parent_key=info.parent_id,
530531
)
531532

532533
def on_user_function_end(self, info: UserFunctionEndInfo) -> None:

packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/invocation_plugin.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -620,9 +620,10 @@ def on_user_function_start(self, info: UserFunctionStartInfo) -> None:
620620
context_scope.enter_scope(
621621
self,
622622
self._scope_key(info),
623-
trace.set_span_in_context(span, self._scope_base_context()),
623+
# Built after enter_scope's cleanup so it cannot inherit values
624+
# from a scope that is about to be detached.
625+
lambda: trace.set_span_in_context(span, self._scope_base_context()),
624626
epoch=self._epoch,
625-
parent_key=info.parent_id,
626627
)
627628

628629
def on_user_function_end(self, info: UserFunctionEndInfo) -> None:

0 commit comments

Comments
 (0)