Repository navigation
fix: finalize invocation hooks on the handler thread - #771
zhongkechen wants to merge 56 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.
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 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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
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 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.
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.
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.
This comment has been minimized.
This comment has been minimized.
| for (var index = plugins.size() - 1; index >= 0; index--) { | ||
| try { | ||
| runHook(plugins.get(index), p -> p.onInvocationEnd(info)); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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) .
| try { | ||
| restore.close(); | ||
| } catch (Throwable restorationFailure) { | ||
| if (restorationFailure != fatal) fatal.addSuppressed(restorationFailure); |
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 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.
This comment has been minimized.
This comment has been minimized.
| for (var index = plugins.size() - 1; index >= 0; index--) { | ||
| try { | ||
| runHook(plugins.get(index), p -> p.onInvocationEnd(info)); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
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 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) .
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--) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
| this(errorObject, retryable, null); | |
| super(errorObject.errorMessage()); | |
| this.errorObject = errorObject; | |
| this.retryable = retryable; |
Codex AI reviewTwo public API compatibility regressions remain: invocation-end ordering and exception cause initialization. Reviewed commit |
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
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.