-
Notifications
You must be signed in to change notification settings - Fork 23
feat(plugin): report user functions that do not complete #659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| OperationStartInfo, | ||
| OperationType, | ||
| UserFunctionEndInfo, | ||
| UserFunctionIncompleteInfo, | ||
| UserFunctionOutcome, | ||
| UserFunctionStartInfo, | ||
| ) | ||
|
|
@@ -595,6 +596,42 @@ def _step_end_info( | |
| ) | ||
|
|
||
|
|
||
| def _step_incomplete_info( | ||
| operation_id: str, | ||
| parent_id: str | None = None, | ||
| attempt: int = 1, | ||
| ) -> UserFunctionIncompleteInfo: | ||
| return UserFunctionIncompleteInfo( | ||
| operation_id=operation_id, | ||
| operation_type=OperationType.STEP, | ||
| sub_type=OperationSubType.STEP, | ||
| name=operation_id, | ||
| parent_id=parent_id, | ||
| start_time=START_TIME, | ||
| is_replayed=False, | ||
| status=OperationStatus.STARTED, | ||
| is_replay_children=False, | ||
| attempt=attempt, | ||
| ) | ||
|
|
||
|
|
||
| def _context_incomplete_info( | ||
| operation_id: str, parent_id: str | None = None | ||
| ) -> UserFunctionIncompleteInfo: | ||
| return UserFunctionIncompleteInfo( | ||
| operation_id=operation_id, | ||
| operation_type=OperationType.CONTEXT, | ||
| sub_type=OperationSubType.RUN_IN_CHILD_CONTEXT, | ||
| name=operation_id, | ||
| parent_id=parent_id, | ||
| start_time=START_TIME, | ||
| is_replayed=False, | ||
| status=OperationStatus.STARTED, | ||
| is_replay_children=False, | ||
| attempt=1, | ||
| ) | ||
|
|
||
|
|
||
| def _context_start_info( | ||
| operation_id: str, parent_id: str | None = None | ||
| ) -> UserFunctionStartInfo: | ||
|
|
@@ -864,75 +901,85 @@ def test_reentered_step_attempt_releases_the_previous_scope(): | |
| assert plugin._context_tokens == {} | ||
|
|
||
|
|
||
| def test_reentry_on_another_thread_leaves_the_originating_worker_dirty(): | ||
| """Pin what re-entry can and cannot clean up across threads. | ||
| def test_suspension_releases_the_scope_on_the_originating_worker(): | ||
| """Verify the suspending worker releases its own scope. | ||
|
|
||
| A resumed branch can land on a different pool thread than the one that | ||
| suspended. Re-entry drops the foreign token instead of resetting it, because | ||
| a context token can only be reset on its own thread, and it unwinds cleanly | ||
| on the thread that re-entered. The worker that suspended keeps the abandoned | ||
| span current: releasing it needs a hook invoked on that thread when the user | ||
| function fails to complete, which the SDK does not provide. The worker is | ||
| kept alive here so this limitation is asserted rather than hidden by pool | ||
| shutdown; the assertion flips once such a hook exists. | ||
| A suspended user function reports no outcome, so the SDK fires | ||
| on_user_function_incomplete on the thread that ran it -- the only thread that | ||
| can reset its context token. The worker is kept alive and probed to prove it | ||
| is left clean even though the resume lands on a different thread. | ||
| """ | ||
| plugin, _ = _create_plugin() | ||
| plugin, exporter = _create_plugin() | ||
| plugin.on_invocation_start(_invocation_start_info()) | ||
| before_context = otel_context.get_current() | ||
| span_key = "step-1:attempt:1" | ||
|
|
||
| with ThreadPoolExecutor(max_workers=1) as worker: | ||
| # The suspending run happens on the worker and never reports an end. | ||
| worker.submit( | ||
| plugin.on_user_function_start, _step_start_info("step-1") | ||
| ).result() | ||
| abandoned_span = plugin._get_span(span_key) | ||
| assert abandoned_span is not None | ||
| foreign_thread_ident, _foreign_token = plugin._context_tokens[span_key] | ||
| assert foreign_thread_ident != threading.get_ident() | ||
|
|
||
| # The timed resume lands on this thread instead. | ||
| def suspend_on_worker() -> tuple[int, bool]: | ||
| plugin.on_user_function_start(_step_start_info("step-1")) | ||
| attached_span_id = trace.get_current_span().get_span_context().span_id | ||
| plugin.on_user_function_incomplete(_step_incomplete_info("step-1")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex AI review [P2] Add end-to-end coverage for automatic core-to-OTel dispatch. This test manually invokes the new callback, while the core tests use only a recording plugin, so both suites would pass if a real suspended branch never delivered this hook correctly. Add an integration test using the local runner and a real map/parallel suspension that verifies the originating worker is clean and resumed span parenting is correct, as required for this new public cross-component behavior. |
||
| return ( | ||
| attached_span_id, | ||
| trace.get_current_span().get_span_context().is_valid, | ||
| ) | ||
|
|
||
| attached_span_id, span_still_current = worker.submit(suspend_on_worker).result() | ||
| suspended_span = plugin._get_span(span_key) | ||
|
|
||
| # The scope was released on the worker, and its span is left open. | ||
| assert attached_span_id != 0 | ||
| assert span_still_current is False | ||
| assert span_key not in plugin._context_tokens | ||
| assert suspended_span is not None | ||
| assert not exporter.get_finished_spans() | ||
|
|
||
| # The timed resume lands on this thread, with nothing stale to unwind. | ||
| plugin.on_user_function_start(_step_start_info("step-1")) | ||
| assert plugin._context_tokens[span_key][0] == threading.get_ident() | ||
| plugin.on_user_function_end(_step_end_info("step-1")) | ||
|
|
||
| # This thread unwound to where it started. | ||
| assert otel_context.get_current() == before_context | ||
|
|
||
| # The originating worker is still carrying the abandoned span. | ||
| worker_span_id = worker.submit( | ||
| lambda: trace.get_current_span().get_span_context().span_id | ||
| # The originating worker is still clean. | ||
| worker_span_valid = worker.submit( | ||
| lambda: trace.get_current_span().get_span_context().is_valid | ||
| ).result() | ||
| assert worker_span_id == abandoned_span.get_span_context().span_id | ||
| assert worker_span_valid is False | ||
|
|
||
| plugin.on_invocation_end(_invocation_end_info()) | ||
|
|
||
|
|
||
| def test_nested_reentry_restores_the_abandoned_outer_scope(): | ||
| """Pin nested re-entry: correct ids, but the abandoned outer span object. | ||
| def test_nested_suspension_unwinds_scopes_in_reverse_order(): | ||
| """Verify nested suspends release inner-first and resume without stale scopes. | ||
|
|
||
| When an outer child context and an inner one both suspend, re-entry releases | ||
| each scope in the order the operations are replayed, which is not the reverse | ||
| of the order they were attached. Ending the inner operation therefore | ||
| restores the scope captured for the abandoned outer span rather than the | ||
| resumed one. Deterministic CONTEXT span ids make the two indistinguishable | ||
| downstream -- same trace id and span id, so parenting and log correlation are | ||
| unaffected -- but the current span object is one that is never exported, so | ||
| anything an instrumentation library records on it is lost. Reverse-order | ||
| unwinding needs the SDK to report the suspension; this test documents the | ||
| current behaviour and flips when that lands. | ||
| The incomplete hook fires as the exception propagates outward, so the inner | ||
| context's scope is released before its enclosing one. On resume, ending the | ||
| inner operation restores the resumed outer scope rather than the one captured | ||
| for the suspended run. | ||
| """ | ||
| plugin, _ = _create_plugin() | ||
| plugin, exporter = _create_plugin() | ||
| plugin.on_invocation_start(_invocation_start_info()) | ||
| before_context = otel_context.get_current() | ||
|
|
||
| # Both contexts suspend, so neither reports an end. | ||
| plugin.on_user_function_start(_context_start_info("ctx-outer")) | ||
| abandoned_outer = plugin._get_span("ctx-outer") | ||
| suspended_outer = plugin._get_span("ctx-outer") | ||
| plugin.on_user_function_start( | ||
| _context_start_info("ctx-inner", parent_id="ctx-outer") | ||
| ) | ||
| assert abandoned_outer is not None | ||
| assert suspended_outer is not None | ||
|
|
||
| # Both contexts suspend: the inner one unwinds first. | ||
| plugin.on_user_function_incomplete( | ||
| _context_incomplete_info("ctx-inner", parent_id="ctx-outer") | ||
| ) | ||
| assert trace.get_current_span() is suspended_outer | ||
|
|
||
| plugin.on_user_function_incomplete(_context_incomplete_info("ctx-outer")) | ||
| assert otel_context.get_current() == before_context | ||
| assert set(plugin._context_tokens) == {"__invocation_context__"} | ||
| # Neither span is ended: both operations are still in flight. | ||
| assert not exporter.get_finished_spans() | ||
|
|
||
| # The timed in-process resume replays both contexts, outer first. | ||
| plugin.on_user_function_start(_context_start_info("ctx-outer")) | ||
|
|
@@ -942,27 +989,14 @@ def test_nested_reentry_restores_the_abandoned_outer_scope(): | |
| ) | ||
| resumed_inner = plugin._get_span("ctx-inner") | ||
| assert resumed_outer is not None | ||
| assert resumed_inner is not None | ||
| assert resumed_outer is not abandoned_outer | ||
|
|
||
| # Resumed inner code runs under the resumed inner span. | ||
| assert resumed_outer is not suspended_outer | ||
| assert trace.get_current_span() is resumed_inner | ||
|
|
||
| plugin.on_user_function_end(_context_end_info("ctx-inner", parent_id="ctx-outer")) | ||
|
|
||
| # The restored scope carries the abandoned outer span, whose ids match the | ||
| # resumed one because CONTEXT span ids are derived from the operation id. | ||
| assert trace.get_current_span() is abandoned_outer | ||
| assert ( | ||
| abandoned_outer.get_span_context().span_id | ||
| == resumed_outer.get_span_context().span_id | ||
| ) | ||
| assert ( | ||
| abandoned_outer.get_span_context().trace_id | ||
| == resumed_outer.get_span_context().trace_id | ||
| ) | ||
| # The resumed outer scope is restored, not the one from the suspended run. | ||
| assert trace.get_current_span() is resumed_outer | ||
|
|
||
| # Leaving the outer context still unwinds to where the invocation started. | ||
| plugin.on_user_function_end(_context_end_info("ctx-outer")) | ||
| assert otel_context.get_current() == before_context | ||
| assert set(plugin._context_tokens) == {"__invocation_context__"} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex AI review
[P1] Raise the OTel package's minimum core SDK version.
UserFunctionIncompleteInfois absent from the currently supported core1.8.0, while OTel still declaresaws-durable-execution-sdk-python>=1.8.0. Environments pinned to 1.8.0 will resolve the new OTel package but fail during import and plugin entry-point loading. Update both the package dependency andtest-pypi-otelenvironment to the first core release containing this hook, or provide a compatibility fallback.