Skip to content

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

Open
zhongkechen wants to merge 56 commits into
mainfrom
fix/otel-handler-context
Open

zhongkechen wants to merge 56 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
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The ordering change is deliberate at the explicitly documented 2.2.2 lifecycle boundary, with the minimum-compatible-core capability checked before plugin setup. Invocation End runs in LIFO order on the handler owner to unwind nested scopes; the other hooks keep registration order. Old plugin binaries are still loadable on the new core, with this documented new End ordering. This is not claimed to preserve the old behavioral ordering. The retained public ordering/cleanup tests and installed-artifact matrix verify that selected contract.

var outcome = Outcome.capture(task);
var restoringAfterNonfatalOutcome = false;
try {
try (var ignored = restore) {

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6794136. When an earlier End/preparation JVM fatal is selected, MDC restoration now closes explicitly once and suppresses a restoration failure only when it is a different object. Throwing the same fatal no longer becomes try-with-resources IllegalArgumentException. The original fatal still settles the caller observation before escaping the actual worker; End remains once with its selected status. Ordinary restoration and initialization policies are unchanged.

All eight public same-instance controls failed on the old code's worker identity and now pass: asynchronous/inline, SUCCESS/PENDING and VMError/ThreadDeath. Existing different-error suppression, wrapper/getter, ordinary restoration and lifecycle controls remain enabled. Full Java17: 2,698 tests, no failures/errors, 31 conditional skips; 194 focused Java25 controls and all 16 artifact cases pass.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 8, 2026 22:50 — 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The explicit 2.2.2 lifecycle/version boundary and reverse invocation-End ordering are documented and tested; the detailed compatibility explanation is #771 (comment) . Old plugin binaries remaining loadable is not a claim of unchanged End ordering. Same-owner nested scopes require the selected LIFO cleanup contract; other hooks retain registration order.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 8, 2026 23:33 — with GitHub Actions Active
@zhongkechen zhongkechen mentioned this pull request Oct 8, 2026
3 of 7 tasks
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This repeats the versioned lifecycle ordering concern addressed at #771 (comment) . The README/Javadoc explicitly define the 2.2.2 same-owner, LIFO invocation-End boundary and minimum-compatible-core capability. Existing plugin binaries remain loadable with that documented new ordering; unchanged old behavioral ordering is not promised. The retained ordering/cleanup and installed-artifact controls pass.

@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 on lines +110 to +112
for (var index = plugins.size() - 1; index >= 0; index--) {
try {
runHook(plugins.get(index), p -> p.onInvocationEnd(info));

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The explicit 2.2.2 lifecycle boundary documents reverse invocation-End order for same-owner nested cleanup, with a minimum-compatible-core capability. Old plugin binaries remaining loadable does not promise unchanged behavioral ordering. Other hooks retain registration order. The detailed compatibility disposition and retained controls are described at #771 (comment) .

Comment on lines +366 to +369
try {
restore.close();
} catch (Throwable restorationFailure) {
if (restorationFailure != fatal) fatal.addSuppressed(restorationFailure);

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0e55c1a. The existing fatal-restoration helper now attempts MDC.clear after an ordinary restore failure even when End/preparation already selected a JVM fatal. That first fatal still owns caller and actual-worker propagation; restoration/classifier/clear failures remain suppressed with identity guards. Initialization, handler/body classification and the already dispatched End outcome are unchanged.

The public negative uses BasicMDCAdapter and a standard single-thread ThreadPoolExecutor. In all four success/PENDING × InternalError/ThreadDeath controls, the old replacement worker inherited the End marker. Successful fallback now clears it. Thirty-two combination controls cover secondary ordinary/Error/fatal/wrapped-fatal failures, alongside existing outcome tests. A broken clear cannot guarantee cleanliness, and ambient MDC may be lost; the README retains those limits. Combined validation: 2,755 Java17 tests (31 expected skips), 112 focused Java25 tests and all 16 installed-artifact compatibility cases passed.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 00:58 — with GitHub Actions Active
Comment on lines +110 to +112
for (var index = plugins.size() - 1; index >= 0; index--) {
try {
runHook(plugins.get(index), p -> p.onInvocationEnd(info));

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the same documented 2.2.2 lifecycle boundary discussed at #771 (comment) . Invocation End uses LIFO on the handler owner thread, with an explicit minimum-compatible-core capability; other hooks retain registration order. Old plugin binaries remaining loadable does not promise unchanged End ordering. The current README/Javadoc and compatibility/order controls retain that explicit migration boundary.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 01:33 — 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The explicit 2.2.2 same-owner/LIFO invocation-End migration boundary remains as documented, with the minimum-compatible-core capability and installed-artifact/order controls. Old plugin binaries remaining loadable is not a promise of unchanged End ordering. This design has no mixed forward/LIFO opt-in mode; adding one would be a different API choice. The existing disposition is #771 (comment) .

@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--) {

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 promised registration order, so reversing legacy end hooks can break plugins with ordering dependencies during a patch upgrade. Keep forward order for existing plugins and add an explicit/versioned LIFO opt-in for scope-owning plugins, including mixed-order compatibility tests.


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

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_x55wb7mvpikru4wweiruquh3pc

[P2] Preserve late cause initialization for the existing constructor. Delegating to the new overload invokes Throwable(String, null), which permanently initializes the cause. Existing callers that use initCause(...) after either old constructor will now receive IllegalStateException. Keep the old super(message) initialization and field assignments in this constructor, reserving the cause-aware superclass call for the new overload.

Suggested change
this(errorObject, retryable, null);
super(errorObject.errorMessage());
this.errorObject = errorObject;
this.retryable = retryable;

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

Two public API compatibility regressions remain: invocation-end ordering and exception cause initialization.

Reviewed commit be44fd7d1447323978b318872275a4db5afef6f1. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — be44fd7d Deployed Oct 9, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2043
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.

1 participant