Skip to content

test: cover OTel replay and user callback context in Java - #769

Open
zhongkechen wants to merge 64 commits into
mainfrom
test/otel-conformance-21-24
Open

zhongkechen wants to merge 64 commits into
mainfrom
test/otel-conformance-21-24

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Oct 2, 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

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

  • Full Java17 clean verify: 2,742 tests, zero failures/errors, 31 existing conditional skips.
  • 202 focused Java25 controls plus Java17 coverage exercise post-End fatal identity/settlement, direct coordinator fatals, ordinary resumption errors, rejected dispatch, READY handoff, cancellation, close and replay. Updated operation-registration assertions pass separately; retained root bookkeeping is not mistaken for a rejected child registration.
  • Ordinary continuation negatives: both SerDes cases returned PENDING at backend READY and both rejected-worker cases timed out before the fix. Corrected public calls receive the original cause through retryable control; a later invocation completes with the expected predicate count. Post-End controls retain single End status and earlier-fatal/suppressed diagnostics.
  • All 16 installed core/plugin/OTel API compatibility cases pass, including explicit old-core/new-plugin rejection and old-plugin/new-core support.
  • Actual case-26 handlers run four invocations with JSON-encoded callback payloads, one marker side effect and raw target counts 0/1/1/1; all 52 mappings validate. Error-metadata controls preserve persisted failures and completed-work replay.
  • Matched S1c cloud proof on 14db8ae: 112/112 passed (four view/backend suites ×26 and two short suites ×4), zero uncovered/unimplemented cases, including 24/25/26: https://github.com/aws/aws-durable-execution-sdk-java/actions/runs/37715017463. New-head CI is tracked separately.
  • Formatting/diff checks pass. Workflow/catalog pins and scenario resources are unchanged. BaseDurableOperation changes only worker admission; its zero-detail metadata extraction is unchanged. Strict performance thresholds and default-concurrency/replay coverage remain intact.

@zhongkechen
zhongkechen marked this pull request as ready for review October 3, 2026 00:03
@zhongkechen
zhongkechen requested a review from a team October 3, 2026 00:03
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 00:03 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 00:03 — with GitHub Actions Active
@github-actions

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 3, 2026 02:18 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
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 03:08 — with GitHub Actions Active
Comment thread sdk/src/main/java/software/amazon/lambda/durable/execution/DurableExecutor.java Outdated
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 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.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 8, 2026 17:31 — with GitHub Actions Active
// 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.

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 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.

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 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.

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 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.

Comment on lines +290 to +291
Outcome.capture(() -> initializationFailure.apply(initializationCause))
.complete(result);

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.

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.

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 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.

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 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.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 8, 2026 18:22 — with GitHub Actions Active
Comment on lines +599 to +600
} catch (Throwable failure) {
completion.completeExceptionally(failure);

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 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.

Comment on lines +126 to +128
Outcome.capture(this::invokeHandler).complete(body);
var selected = outcome.join();
return finishInvocation(selected.value(), selected.failure());

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 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) .

Comment on lines +301 to +304
} 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.

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 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.

Comment on lines +290 to +291
Outcome.capture(() -> initializationFailure.apply(initializationCause))
.complete(result);

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 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) .

Comment on lines +113 to +126
} 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.

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 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.

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 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) .

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 8, 2026 20:37 — with GitHub Actions Active
Comment on lines +605 to +606
} catch (Throwable failure) {
completion.completeExceptionally(failure);

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_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.

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 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.

Comment on lines +301 to +304
} 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);

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_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.

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 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();

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_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.

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

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_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.

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 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) .

Comment on lines +113 to +114
} catch (Error failure) {
// Finish unwinding earlier plugins' thread-local scopes before propagating an end-hook error.

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_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.

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 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) .

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

Found two P1 lifecycle failures and three P2 compatibility/error-handling regressions. Review was static only, as requested.

Reviewed commit b882ea7c57a688d35a0eb694629d494a98769c57. Workflow run

@zhongkechen
zhongkechen requested a deployment to ai-pr-review-runtime October 8, 2026 21:19 — with GitHub Actions In progress

This branch is being deployed

1 in progress deployment
ai-pr-review-runtime — 18d4e3c9 Deployed Oct 8, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #1910
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