Skip to content

fix: finalize invocation hooks on the handler thread - #771

Merged
zhongkechen merged 57 commits into
mainfrom
fix/otel-handler-context
Oct 13, 2026
Merged

zhongkechen merged 57 commits into
mainfrom
fix/otel-handler-context

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

Keep root-handler OpenTelemetry context active through handler cleanup and invocation-end hooks. Start, the handler and its finally blocks, and End run on the same handler thread. The caller awaits cleanup and End, including after suspension/retry and without plugins. A blocked cleanup or End hook can hold the invocation until its runtime timeout.

Suspension and termination select the manager outcome before waking operation waiters. A fast returning or throwing handler finally cannot replace that selected outcome; a genuinely earlier body outcome still wins. The manager observer only captures the outcome, so early publication does not synchronously wait for handler cleanup or dispatch End.

Both OTel views preserve valid same-trace ambient context or activate canonical context, then restore their owning Scope in End finally. End hooks unwind in reverse registration order, including after partially failed Start. Remaining hooks run after an unisolated Error; primary errors retain suppressed cleanup diagnostics, with the first JVM-fatal failure taking precedence. Success/failure serialization and durable large-result checkpointing finish before terminal End. Preparation failures report RETRYING and retain their original failure if End also fails, unless cleanup introduces the first JVM-fatal error. Retryable controls now use that same End-error combiner: an ordinary End Error is suppressed under the original retry control; a direct Root End JVM fatal keeps its existing precedence with the control/cause retained as diagnostics. No arbitrary control-cause unwrapping or global body classification is added.

The plugin requires DurableExecutor.supportsSameThreadInvocationHooks(), introduced with core 2.2.2 (currently 2.2.2-SNAPSHOT). Constructors reject released core 2.2.1 before provider/context setup. Older plugin binaries remain supported on the new core with its documented hook ordering.

Pre-start MDC-capture JVM fatals, including standard transport wrappers, complete the observation future with the original fatal before escaping the worker, with zero Start/body/End. Private cause inspection is single-read and identity-cycle-safe for finite chains. Ordinary initialization/body classification is retained. End freezes the SDK preparation outcome; later manager cleanup, envelope/OutputStream emission, runtime ACK and outer worker MDC restoration occur afterwards. Those boundaries do not dispatch a second End or rewrite its snapshot. Post-End MDC restoration JVM fatals (direct or in recognized transport wrappers) now settle the caller observation with the original fatal before that fatal escapes the actual worker, for asynchronous and inline executors. An earlier fatal remains primary with restoration diagnostics suppressed; a restoration fatal takes precedence over an earlier nonfatal delivery/End failure. Ordinary restoration failures retain the selected caller outcome. End notification is not runtime acknowledgment.

An owned SDK checkpoint continuation now publishes resumption/deserialization and dispatch failures as retryable manager control before releasing its activity reservation. The caller receives UnrecoverableDurableExecutionException with the original cause and End reports RETRYING. Rejected worker admission rolls back its registration; rejection is assumed to occur before task acceptance. Direct JVM fatals also settle observation after lease release before worker escape. Ordinary unowned helper failures, normal closing and handler/predicate classification retain their existing behavior.

READY polling retains executable work and publishes each operation worker before resumption. Retry attempts yield through the configured executor while an independent coordinator owns activity through next-worker registration. Cancellation affects observation rather than the actual queued task. Close stops only operations owned by unfinished continuations and drains their real registrations before checkpoint shutdown. Terminal live/poll/replay outcomes preserve stored results and failures, including absent StepDetails. Public controls establish the repaired protocol defects; the exact cause of an earlier cloud invariant failure remains qualified because its full history was unavailable.

Sampling intents are consumed once before processors run. Only a same-copy DurableSampler receives the full context carrier; foreign samplers resolve their own full SamplingResult. Deferred results are keyed by execution ARN and canonical trace ID in the existing 256-entry LRU cache, with another delegate evaluation possible after eviction. Visible replacement samplers receive no unconsumable thread bridge; opaque agent providers retain the existing bridge because their sampler is not publicly inspectable. Application sampling policy and ambient restoration remain covered by real two-loader/provider controls.

Related PR: #769 integrates this implementation and must follow it. Factory migration #782 is separate. This branch retains its 20-case catalog at 02d6dca971a38c13d94d6233d12f687e55b2a572, with the compatible workflow's bounded read-only layer retries and existing gates/locks.

Validation

  • Full Java17 clean verify: 2,714 tests, zero failures/errors, 31 existing conditional skips.
  • Post-End MDC and continuation repair: 177 focused Java25 controls, 73 MDC-boundary cases in full Java17, and 16/16 artifact cases. The original post-End code failed 44/57 controls; corrected cases verify sync/async SUCCESS/PENDING notifications, VMError/ThreadDeath and standard wrappers, original caller/worker identity, no hang, earlier fatal precedence and suppressed diagnostics. Ordinary restoration and diagnostic read-once controls remain.
  • Earlier outcome repair: 127 focused controls on Java17 and Java25. Of 18 natural step/WFC/termination controls, 12 fail before the fix and all pass afterward; four manager-channel controls retain prior success or original ordinary/VMError/ThreadDeath identity. Executor coverage includes asynchronous, inline-root with child workers, and submit-and-wait dispatch, without claiming fully inline child-operation support.
  • Sampling controls cover real isolated loaders/providers, complete custom attributes and TraceState, shared-trace execution isolation, one-shot consumption, ambient cleanup and delegate counts while cache entries remain resident.
  • Required installed-artifact matrix: 16/16 passed across released/current core/plugin and OTel API variants, including explicit old-core/new-plugin rejection and retained old-plugin/new-core support.

Latest End-boundary validation: 2,714 full Java17 tests (31 existing conditional skips, no failures/errors), 194 focused Java25 controls and 16/16 artifact cases. Four continuation-plus-End negative cases and eight same-instance MDC-restoration negatives now pass; explicit identity-safe MDC close preserves the original fatal on the worker without TWR self-suppression. Existing MDC, ordinary restoration, retry and lifecycle controls remain enabled. The prior 007031a Java25 cloud performance failure passed one unchanged-threshold retry (500-step replay 3/29/2 ms, all 36 tests pass); new-head CI is separate.

Ordinary MDC restoration fallback

After an ordinary outer MDC restoration failure, one guarded clear attempt prevents inheritable invocation state from reaching a replacement pool worker when that clear succeeds. The selected caller outcome and original worker failure remain; a secondary ordinary clear failure is suppressed, while its JVM fatal follows the existing settle-before-worker-throw classifier. Initialization and handler/body policy remain unchanged. Clearing can discard ambient MDC that could not be restored. A broken clear cannot guarantee clean replacement state or quarantine of an arbitrary caller-owned executor; those limits are documented.

Full Java17: 2,714 tests, no failures/errors, 31 conditional skips; 159 focused Java25 controls and all 16 artifact cases pass. A real BasicMDCAdapter/single-thread-pool negative proved that replacing the failed worker still inherited an End marker. Working-clear and ordinary/assertion/same-error/direct-or-wrapped VM/ThreadDeath fallback controls cover SUCCESS/PENDING and retain single End status.

@zhongkechen
zhongkechen requested a review from a team October 3, 2026 00:37
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 00:37 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 00:38 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 01:38 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 02:01 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 03:04 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
@github-actions

This comment has been minimized.

@zhongkechen zhongkechen changed the title fix(otel): keep root handler instrumentation in the execution trace fix: activate the canonical OTel context for root handlers Oct 3, 2026
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 04:11 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 04:36 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/plugin/HandlerScoped.java Outdated
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 05:01 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 00:09 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 00:58 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 01:33 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 02:12 — with GitHub Actions Active
public void onInvocationEnd(InvocationEndInfo info) {
run(p -> p.onInvocationEnd(info));
Error firstError = null;
for (var index = plugins.size() - 1; index >= 0; index--) {

This comment was marked as outdated.


public UnrecoverableDurableExecutionException(ErrorObject errorObject, boolean retryable) {
super(errorObject.errorMessage());
this(errorObject, retryable, null);

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@zhongkechen

Copy link
Copy Markdown
Contributor Author

Regarding this review: we consider the compatibility impact of invocation-end hook ordering and late initCause(...) initialization limited and acceptable. These findings are non-blocking for this PR.

We will keep the current implementation and will not fix these two compatibility concerns in this PR. We will not add a legacy-order compatibility mode or versioned LIFO opt-in, or restore the previous cause-initialization behavior.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 13, 2026 18:11 — with GitHub Actions Active
@zhongkechen
zhongkechen merged commit 1064d0e into main Oct 13, 2026
17 checks passed
@zhongkechen
zhongkechen deleted the fix/otel-handler-context branch October 13, 2026 18:13
public void onInvocationEnd(InvocationEndInfo info) {
run(p -> p.onInvocationEnd(info));
Error firstError = null;
for (var index = plugins.size() - 1; index >= 0; index--) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex AI review · Finding arf_v1_njvdmrbda6myb4mc5dktqd76ih

[P2] Preserve the released invocation-end ordering contract. Existing API documentation promises registration order, so reversing legacy end hooks in a patch release can break plugins whose cleanup depends on that ordering. Keep forward order for existing plugins and add an explicit/versioned LIFO opt-in for scope-owning plugins, with mixed legacy/OTel compatibility tests.

Comment on lines +298 to +300
Outcome.capture(() -> initializationFailure.apply(initializationCause))
.complete(result);
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex AI review · Finding arf_v1_ojgonbwgz6iy3pas3daznjneyu

[P2] Re-throw fatal failures while preparing an initialization error. If MDC capture fails ordinarily and error-response serialization subsequently throws VirtualMachineError or ThreadDeath, this capture settles the caller but returns normally from the actual executor worker. That relocates the fatal and leaves the affected worker alive, unlike the fatal-preservation logic used for capture and normal preparation failures. Capture the outcome, complete result, then call rethrowLifecycleFatal on its failure before returning; add asynchronous and inline regression tests.

@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

Two confirmed issues affect plugin compatibility and lifecycle-fatal propagation.

Reviewed commit 176b60f75bf45011daa7ad761a270612fe30a50c. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — 176b60f7 Deployed Oct 13, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants