Repository navigation
test: cover OTel replay and user callback context in Java - #769
zhongkechen wants to merge 64 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| 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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The Javadoc was stale and is corrected in b882ea7. The explicit 2.2.2 lifecycle boundary runs invocation End in reverse registration order on the handler owner so nested context resources unwind correctly; other hooks retain registration order. The README documents this behavior and its minimum-compatible-core capability, with PluginRunner/InvocationEndCompatibilityTest coverage. Old plugin binaries remain loadable with the documented new End ordering. The wording now states that exception instead of promising registration order for every callback; the selected LIFO contract is retained.
This comment has been minimized.
This comment has been minimized.
| // termination can win while the handler is still unwinding; its finally block must not replace it. | ||
| var outcome = manager.runUntilCompleteOrSuspend(body).handle(Outcome<O>::new); | ||
| Supplier<DurableExecutionOutput> task = () -> { | ||
| Outcome.capture(this::invokeHandler).complete(body); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The same-owner 2.2.2 lifecycle contract deliberately waits for handler finally blocks and all End hooks, including after suspension/retry and without plugins. This is documented in the Root handler context section and exercised by InvocationFinalizationIntegrationTest/InvocationEndCompatibilityTest with cleanup held beyond700ms. A cleanup that never returns can consume the Lambda deadline; that is a real limitation, not a bounded-delivery guarantee. Introducing a timer that returns while cleanup still owns the thread would change the selected contract. The current coordinator-fatal repair addresses lost runtime failures without reinstating caller-thread finalization or a cleanup timeout.
| } finally { | ||
| // End and worker restoration have run before publishing. A restoration failure still escapes its | ||
| // owner, without changing the invocation outcome already delivered to the end hooks. | ||
| outcome.complete(result); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This repeats the post-End boundary explained in #769 (comment) and #771 (comment). The outer MDC restore follows the single End dispatch; its failure escapes its owner while the selected SDK outcome remains frozen. Inline ordinary restoration failures preserve that selected outcome; broken adapters may leave MDC state uncertain. Pre-start direct/standard-wrapper JVM fatals have the separate settle-before-worker-throw guarantee. The current coordinator repair does not reclassify post-End restoration or ordinary handler failures.
| finishCheckpointContinuation(registration); | ||
| } | ||
| completion.complete(null); | ||
| } catch (Throwable failure) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in b882ea7. A direct VirtualMachineError/ThreadDeath in an SDK checkpoint continuation now publishes retryable manager control flow before releasing its final activity reservation. This prevents READY work from silently returning PENDING. The caller receives UnrecoverableDurableExecutionException with the original fatal as its cause; started End hooks receive RETRYING. After the task registration is released, the observation future settles with the original fatal before that fatal escapes the coordinator worker.
The public WFC regression uses a real pending checkpoint, a READY poll response and a resumed-state SerDes fault on the actual coordinator. Before the change, the caller returned PENDING with backend READY and no worker escape. Four direct VM/ThreadDeath cases, with and without plugins, now verify caller/cause identity, End status, bounded completion and worker escape. Controls also cover observer callbacks closing the manager after lease release, inline execution without double release, cancellation/admission/close, body-first outcomes and ordinary failures.
Full Java17 verification passed 2,671 tests (31 existing skips, zero failures/errors), 156 focused Java17/25 controls and 16/16 artifact compatibility cases. The cause constructor is additive. This direct-fatal SDK-continuation boundary does not globally reinterpret wrapped, handler/body, initialization or post-End failures. All nine 26-case scenario/workflow/metadata assets remain byte-identical. Related PR: #771 remains the merge prerequisite. Older observer implementations require separate validation before a port.
| Outcome.capture(() -> initializationFailure.apply(initializationCause)) | ||
| .complete(result); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Ordinary pre-start MDC capture failure retains the existing strict initialization policy: when its error can be serialized, it produces FAILED with zero Start/body/End. The deliberately different JVM-fatal initialization boundary completes the observation future with the original direct/standard-wrapper fatal before worker escape and never maps that fatal to durable FAILED. Both paths are covered by the capture-failure controls and documented in the Root handler context section. Changing ordinary capture to best-effort or RETRYING would establish a different initialization policy; it is not part of the current scoped repair.
| 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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The Javadoc was stale and is corrected in b882ea7. The explicit 2.2.2 lifecycle boundary runs invocation End in reverse registration order on the handler owner so nested context resources unwind correctly; other hooks retain registration order. The README documents this behavior and its minimum-compatible-core capability, with PluginRunner/InvocationEndCompatibilityTest coverage. Old plugin binaries remain loadable with the documented new End ordering. The wording now states that exception instead of promising registration order for every callback; the selected LIFO contract is retained.
| for (var index = plugins.size() - 1; index >= 0; index--) { | ||
| try { | ||
| runHook(plugins.get(index), p -> p.onInvocationEnd(info)); | ||
| } catch (Error failure) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The End-loop liveness tradeoff is acknowledged in #769 (comment). End hooks unwind on the handler owner and all remaining cleanup is awaited. The first JVM-fatal failure remains primary, with other errors suppressed, but a later cleanup that never returns can prevent its delivery until the runtime deadline. Immediate propagation or bounded/best-effort cleanup would change that explicit same-owner, all-cleanup contract. The implementation and public lifecycle controls retain the documented behavior.
This comment has been minimized.
This comment has been minimized.
| } catch (Throwable failure) { | ||
| completion.completeExceptionally(failure); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in 18d4e3c. An owned SDK continuation now publishes retryable manager control with the original failure as its cause before its activity lease can release and select PENDING. Rejected user-worker admission also rolls back that operation's registration directly through the manager, without assuming the coordinator has a user ThreadContext. This relies on rejection before task acceptance. Observation cancellation cannot discard the operation failure. Unowned ordinary helper failures retain their existing observation behavior, and normal closing does not replace the selected outcome or stop unrelated operations. Direct JVM fatal observation/worker escape remains intact.
Public WFC negatives reproduced both problems with and without plugins: resumed SerDes failures returned PENDING while the backend was READY; next-worker rejection left the caller pending beyond the bounded observation. The fixed controls verify the original cause, RETRYING End, unchanged persisted READY state, no phantom rejected-child registration, and successful subsequent resumption with the expected predicate count. Existing admission-versus-close, cancelled-observation, direct/submit-and-wait READY and worker-escape controls pass. Handler/predicate failure checkpoint classification is unchanged.
Combined full Java17 verification passed 2,742 tests (31 existing conditional skips, zero failures/errors), 202 focused Java25 controls and 16 artifact compatibility cases. The final operation-registration assertions also pass separately. The 26-case S1c references and eight scenario/workflow/wire assets are byte-identical; BaseDurableOperation changes only worker admission, with its error-metadata region unchanged.
| Outcome.capture(this::invokeHandler).complete(body); | ||
| var selected = outcome.join(); | ||
| return finishInvocation(selected.value(), selected.failure()); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This retains the explicitly selected 2.2.2 lifecycle contract: Start, handler/finally and End run on the handler owner, and the invocation waits for cleanup/End, including beyond 500 ms. The public delayed-cleanup controls cover zero plugins and a no-op plugin. The README states that blocked user cleanup can block the response. Adding a timeout/alternate finalizer would change that contract and thread ownership; no bounded-handoff policy is added here. Prior explanation: #769 (comment) .
| } finally { | ||
| // End and worker restoration have run before publishing. A restoration failure still escapes its | ||
| // owner, without changing the invocation outcome already delivered to the end hooks. | ||
| outcome.complete(result); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The JVM-fatal concern is fixed in 18d4e3c, integrating the validated fix from related PR #771 (80110fc). The earlier interpretation that an already-sent End permits asynchronous caller success after a restoration fatal was incorrect. Direct/recognized transport-wrapped VMError or ThreadDeath now settles the caller observation with the original fatal before worker escape, for asynchronous and inline executors. End stays exactly once with its previously selected status; earlier fatal precedence and suppressed cleanup diagnostics remain.
The 73 public MDC controls cover SUCCESS/PENDING, both fatal types, standard wrappers and fatal diagnostic getters, caller/worker identity, bounded observation, and ordinary restoration behavior. Combined full Java17 verification passed 2,742 tests, 202 focused Java25 controls and 16 artifact cases. The README and PR description now state this distinction.
Ordinary restoration behavior remains unchanged. A broken MDC adapter may prevent actual restoration; this patch does not promise that a fallback clear can repair it or erase saved ambient state. It does not broaden handler/body or ordinary initialization classification.
| Outcome.capture(() -> initializationFailure.apply(initializationCause)) | ||
| .complete(result); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The ordinary initialization policy is unchanged: an ordinary MDC capture failure before Start is classified as a handler initialization failure and produces FAILED when its diagnostic response is serializable; no unpaired Start/End is emitted. Direct/recognized wrapped JVM fatals instead settle the observation with the original fatal before worker escape, without a durable FAILED response. Public direct/async controls cover both. The README explicitly documents this distinction; a best-effort or retryable policy for every ordinary initialization error would be a separate behavior change. Prior explanation: #769 (comment) .
| } catch (Error failure) { | ||
| // Finish unwinding earlier plugins' thread-local scopes before propagating an end-hook error. | ||
| if (firstError == null) { | ||
| firstError = failure; | ||
| } else if (firstError != failure) { | ||
| if (isJvmFatal(failure) && !isJvmFatal(firstError)) { | ||
| failure.addSuppressed(firstError); | ||
| firstError = failure; | ||
| } else { | ||
| firstError.addSuppressed(failure); | ||
| } | ||
| } | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The selected lifecycle contract finishes all End cleanup on the handler owner, in LIFO order, before returning or propagating its selected Error. A first JVM fatal remains primary with later cleanup diagnostics suppressed. Public controls and the README explicitly cover blocked later cleanup and do not promise a bounded response in that case. Skipping remaining cleanup or moving finalization to another owner would change that contract. Prior explanation: #769 (comment) .
| 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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The stale Javadoc was corrected in b882ea7. Invocation End is the documented reverse-registration-order exception at the explicit 2.2.2 lifecycle boundary; other hooks keep registration order. The README states the minimum-compatible-core capability and old-plugin/new-core dispatch behavior, with artifact and compatibility tests. This intentionally selected LIFO contract supports same-owner nested context cleanup; it is not described as behaviorally identical ordering for old plugin lists. Prior reply: #769 (comment) .
This comment has been minimized.
This comment has been minimized.
| } catch (Throwable failure) { | ||
| completion.completeExceptionally(failure); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_apr5vnso2hgqtj3wurah43s3by
P1 — Propagate ordinary continuation failures into the execution. WaitForConditionOperation discards this returned future, so deserialization or next-worker dispatch failures only fail an unobserved future. An executor rejection can also leave the operation registered as active, hanging the invocation indefinitely. Route failures through the manager/owner failure path before releasing the activity lease, or make every polling chain observe and propagate this future.
There was a problem hiding this comment.
Fixed in 18d4e3c. An owned SDK continuation now publishes retryable manager control with the original failure as its cause before its activity lease can release and select PENDING. Rejected user-worker admission also rolls back that operation's registration directly through the manager, without assuming the coordinator has a user ThreadContext. This relies on rejection before task acceptance. Observation cancellation cannot discard the operation failure. Unowned ordinary helper failures retain their existing observation behavior, and normal closing does not replace the selected outcome or stop unrelated operations. Direct JVM fatal observation/worker escape remains intact.
Public WFC negatives reproduced both problems with and without plugins: resumed SerDes failures returned PENDING while the backend was READY; next-worker rejection left the caller pending beyond the bounded observation. The fixed controls verify the original cause, RETRYING End, unchanged persisted READY state, no phantom rejected-child registration, and successful subsequent resumption with the expected predicate count. Existing admission-versus-close, cancelled-observation, direct/submit-and-wait READY and worker-escape controls pass. Handler/predicate failure checkpoint classification is unchanged.
Combined full Java17 verification passed 2,742 tests (31 existing conditional skips, zero failures/errors), 202 focused Java25 controls and 16 artifact compatibility cases. The final operation-registration assertions also pass separately. The 26-case S1c references and eight scenario/workflow/wire assets are byte-identical; BaseDurableOperation changes only worker admission, with its error-metadata region unchanged.
| } finally { | ||
| // End and worker restoration have run before publishing. A restoration failure still escapes its | ||
| // owner, without changing the invocation outcome already delivered to the end hooks. | ||
| outcome.complete(result); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_2qzxcu2ohg5i5463rcc6m2ysfs
P1 — Do not publish the selected result when MDC restoration dies. This finally completes the caller future even if restoration throws VirtualMachineError or ThreadDeath; with an asynchronous executor the caller can return SUCCEEDED/PENDING while only the worker observes the fatal. Complete the caller exceptionally for JVM-fatal restoration failures before rethrowing them, and attempt an MDC.clear() fallback for ordinary failures so reusable threads cannot retain invocation MDC.
There was a problem hiding this comment.
The JVM-fatal concern is fixed in 18d4e3c, integrating the validated fix from related PR #771 (80110fc). The earlier interpretation that an already-sent End permits asynchronous caller success after a restoration fatal was incorrect. Direct/recognized transport-wrapped VMError or ThreadDeath now settles the caller observation with the original fatal before worker escape, for asynchronous and inline executors. End stays exactly once with its previously selected status; earlier fatal precedence and suppressed cleanup diagnostics remain.
The 73 public MDC controls cover SUCCESS/PENDING, both fatal types, standard wrappers and fatal diagnostic getters, caller/worker identity, bounded observation, and ordinary restoration behavior. Combined full Java17 verification passed 2,742 tests, 202 focused Java25 controls and 16 artifact cases. The README and PR description now state this distinction.
Ordinary restoration behavior remains unchanged. A broken MDC adapter may prevent actual restoration; this patch does not promise that a fallback clear can repair it or erase saved ambient state. It does not broaden handler/body or ordinary initialization classification.
| Runnable work = (Runnable & CompletableFuture.AsynchronousCompletionTask) () -> { | ||
| SafeCloseable restore; | ||
| try { | ||
| restore = restoreMdcOnClose(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_aez22plhcmwmocotes2l74q24h
P2 — Do not turn MDC snapshot failures into terminal workflow failures. Every nonempty plugin configuration now performs this optional logging operation before hooks or user code, and an ordinary adapter failure is routed through finishFailure, emitting durable FAILED. Treat capture as best-effort with a safe baseline, or propagate an invocation-level retry without writing a terminal failure.
There was a problem hiding this comment.
The ordinary initialization policy is unchanged: an ordinary MDC capture failure before Start is classified as a handler initialization failure and produces FAILED when its diagnostic response is serializable; no unpaired Start/End is emitted. Direct/recognized wrapped JVM fatals instead settle the observation with the original fatal before worker escape, without a durable FAILED response. Public direct/async controls cover both. The README explicitly documents this distinction; a best-effort or retryable policy for every ordinary initialization error would be a separate behavior change. Prior explanation: #769 (comment) .
| public void onInvocationEnd(InvocationEndInfo info) { | ||
| run(p -> p.onInvocationEnd(info)); | ||
| Error firstError = null; | ||
| for (var index = plugins.size() - 1; index >= 0; index--) { |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_ekfqfzcr2ba7cnilv2ygyeeh5b
P2 — Preserve the existing registration-order contract. Reversing onInvocationEnd changes behavior for existing plugin lists previously promised registration-order dispatch; updating the Javadocs does not protect plugins whose flush or cleanup dependencies rely on that ordering. Retain registration order, or introduce an explicit opt-in LIFO scope mechanism.
There was a problem hiding this comment.
The stale Javadoc was corrected in b882ea7. Invocation End is the documented reverse-registration-order exception at the explicit 2.2.2 lifecycle boundary; other hooks keep registration order. The README states the minimum-compatible-core capability and old-plugin/new-core dispatch behavior, with artifact and compatibility tests. This intentionally selected LIFO contract supports same-owner nested context cleanup; it is not described as behaviorally identical ordering for old plugin lists. Prior reply: #769 (comment) .
| } catch (Error failure) { | ||
| // Finish unwinding earlier plugins' thread-local scopes before propagating an end-hook error. |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_zevxyf2hukihhhwchdou5q7lzi
P2 — Propagate JVM-fatal hook failures without waiting behind arbitrary cleanup. After catching VirtualMachineError or ThreadDeath, the loop continues invoking every remaining end hook. If a later hook blocks, the fatal never reaches the worker or invocation caller. Rethrow JVM-fatal failures immediately, or make subsequent best-effort cleanup bounded.
There was a problem hiding this comment.
The selected lifecycle contract finishes all End cleanup on the handler owner, in LIFO order, before returning or propagating its selected Error. A first JVM fatal remains primary with later cleanup diagnostics suppressed. Public controls and the README explicitly cover blocked later cleanup and do not promise a bounded response in that case. Skipping remaining cleanup or moving finalization to another owner would change that contract. Prior explanation: #769 (comment) .
Codex AI reviewFound two P1 lifecycle failures and three P2 compatibility/error-handling regressions. Review was static only, as requested. Reviewed commit |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Description
Add Java public-API handlers and both-view Lambda resources for OTel requirements 21–26: completed-step replay, active user-function contexts, callback/retry contexts, invocation retry status, terminal callback failure without error details, and external callback completion across later replays. Existing cases and coverage gates remain intact.
Related PR #771 must merge first. This branch includes its same-owner Start/body/finally/End lifecycle: the caller waits for handler cleanup and End, including after suspension/retry and without plugins. Manager control is selected before waking operation waiters, retaining a genuinely earlier body outcome. OTel scopes close in End finally and hooks unwind in reverse registration order. The plugin requires the core lifecycle capability introduced in 2.2.2-SNAPSHOT; old-core/new-plugin incompatibility is explicit, while old-plugin/new-core support remains tested. Factory migration #782 is separate.
SDK response serialization and large-result checkpointing finish before terminal End; preparation failures report RETRYING. Primary/suppressed cleanup diagnostics preserve JVM-fatal precedence. End is a notification of the selected SDK outcome, not runtime acknowledgment. Post-End MDC restoration JVM fatals, direct or in recognized transport wrappers, settle the caller observation with the original fatal before escaping the actual worker, for asynchronous and inline executors. End remains once with its already selected status. Ordinary restoration and handler/body classification are unchanged. Pre-start MDC fatals separately settle before worker escape with zero Start/body/End; private diagnostic inspection is single-read and identity-cycle-safe for finite chains.
READY work resumes after the previous attempt retires, with an independent coordinator retaining activity through the next worker's registration. Each READY attempt yields through the configured executor; cancellation affects observation only. Close atomically stops admission and drains actual owned continuations without stopping unrelated operations. Unexpected resumption/deserialization or dispatch failures in an owned continuation now reach retryable manager control before lease release, with the original cause and unchanged persisted state. Direct JVM fatals also escape the coordinator after observation settlement. Rejected user-worker admission rolls back its registration; this assumes rejection before task acceptance. Ordinary unowned helper failures and normal-close behavior retain their existing contracts. Predicate failures still follow the operation's checkpoint policy.
Both reusable workflow and conformance_test_ref remain pinned to shared PR aws/aws-durable-execution-conformance-tests#131 at bdb4f1cd0f9252c1aaa978bb8b341b71f2b9d9dc, with 26 requirements per view. Case 24 retains RETRYING/UNSET to SUCCEEDED/OK. Case 25 sends callback failure without Error after the creating invocation completes, requiring empty error payload and UNSET. Case 26 creates its target callback on the root context, checkpoints its result and crosses two later callback barriers. Its terminal span belongs to the second invocation; raw S3 counts and quiescence verify that later replays add no duplicate export.
The first cloud run exposed Java case-25 metadata incorrectly reporting ERROR in all four view/backend combinations. Only a fully empty ErrorObject becomes absent plugin error metadata. Persisted ErrorObject and caller failure remain intact; present empty strings or an explicit empty stack list remain error details. Cloud history shows CallbackFailedDetails.Error.Payload={}, not the SDK-consumed Operation.Error wire. Exact-wire and public callback controls separately cover omitted/null/empty inputs and replay.
SDK sampling intents are consumed once before processors run. A same-copy DurableSampler receives the context carrier; foreign and opaque providers retain the one-shot bridge and their full SamplingResult. Visible plain replacements retain normal per-span sampling. Deferred results use execution ARN and trace ID in the existing 256-entry LRU, so eviction may cause reevaluation. Parent context, custom attributes, trace state, application delegate policy and ambient restoration remain covered. This branch does not add synthetic roots or change factory APIs.
Validation