Repository navigation
fix: export synthetic OTel roots and retain READY wait progress - #767
zhongkechen wants to merge 45 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.
This comment has been minimized.
This comment has been minimized.
| public CompletableFuture<Void> runCheckpointContinuation(BaseDurableOperation owner, Runnable continuation) { | ||
| return runCheckpointContinuation(owner, continuation, InternalExecutor.INSTANCE); |
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 2adc34c. Step retry resumption now enters the existing operation-owned checkpoint continuation, so a configured synchronous or submit-and-wait executor cannot occupy the serialized checkpoint callback. For a retry initiated by a live attempt, the continuation waits for that attempt's worker publication and completion before registering its replacement. The existing continuation activity lease spans that transition, and a stopped owner is checked before dispatch/body entry. Step failure classification, retry policy and checkpoint contents are unchanged.
The public regression starts with a real failed step, retains its PENDING history, advances the local backend to READY, and resumes through the configured executor. Before the fix, rejection returned PENDING with backend READY; submit-and-wait blocked the batcher until the probe's bounded release. Afterward, rejection reaches the caller as retryable control with the original cause, and async/direct/submit-and-wait modes complete. Three additional live-retry gates cover old-worker exit, executor-return/publication, and both together; the continuation holds no polling monitor, preserves activity, and completed replay does not repeat the body.
Validation: 2,423 full Java17 tests (31 expected skips), 26 focused Java25 tests, and all 16 installed-artifact compatibility cases passed. The same source defect was independently reproduced and the narrow fix validated on the other affected non-held branches; no lifecycle, sampler, workflow, or conformance-scenario port is included.
This comment has been minimized.
This comment has been minimized.
| * Recovery re-exports retain identity and timestamps; Workflow carries duration and outcome. Remote parents are | ||
| * never owned. | ||
| */ | ||
| static Span startExecutionRoot( |
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 4091401. Successful handler output preparation now has a narrow failure boundary: if the configured output SerDes or oversized-result checkpoint throws, the existing completion thread dispatches one RETRYING End, then propagates the original normalized preparation failure. This releases the retained root before invocation return. The existing admission cut and subsequent manager drain are unchanged; this does not port the different handler-thread lifecycle from related PR #771 or cover later response-stream/runtime acknowledgment failures.
The real public tests reproduce both output-SerDes and greater-than-6MB checkpoint failures in both OTel views: before the change End/root-end/flush counts were zero and the root remained recording. They now each end and flush once, retain the caller error identity, and support another invocation. Combined preparation/End controls preserve the original failure and suppressed diagnostics, with the established direct JVM-fatal precedence. Full Java17: 2,462 tests (31 expected skips); 51 focused Java25 tests and all 16 compatibility cases passed.
| if (root != null) root.end(rootTimestamp); | ||
| if (shouldFlush && provider != null) { |
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 4091401 for both views. A root-end Exception or LinkageError now still runs the flush through the existing cleanup combiner before the plugin boundary isolates that error. Unisolated Errors retain the previous escape/skip-flush behavior; this is not a blanket finally around JVM-fatal cleanup. Earlier-primary/later-fatal and same-object suppression rules remain intact.
The public control uses an actual throwing root SpanProcessor followed by BatchSpanProcessor with automatic export deferred. Before the fix, the invocation succeeded but no completed Invocation/Workflow spans had been exported; afterward they are flushed before return. A processor that throws need not export the root itself. Seven additional root/flush priority controls plus existing partial-start/reentry/reuse controls pass. Validation: 2,462 Java17 tests (31 expected skips), 51 focused Java25 tests, and 16 installed-artifact compatibility cases.
This comment has been minimized.
This comment has been minimized.
| Throwable error, | ||
| Object executionInput, | ||
| Object executionResult) { | ||
| executionManager.beginInvocationEnd(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Update after rechecking the pre-PR source and real public controls: the selected-root outcome remains frozen, but that does not justify leaving new continuation admission open throughout output preparation. The narrow admission gap is fixed in 2e3edf1. New checkpoint-continuation admission closes before output SerDes and oversized-result checkpointing, without running owner callbacks or joining cleanup at that point. Existing owner signaling and accepted/running cleanup remain at End/manager close. Four old admission counterexamples now pass; awaited original-cause RETRYING, successful large checkpointing and publisher/cleanup controls are preserved. Full Java17 2,483 (31 existing skips), focused Java25 64, artifact matrix16/16.
This supersedes the earlier no-earlier-cut conclusion. It does not promise global outcome replacement, a full pre-End drain, or final terminal export for already-running unawaited work. The detailed current boundary and public evidence are in the direct response to the latest finding.
| // Register before re-reading: another checkpoint may already have delivered READY before this poll existed. | ||
| known = getOperation(); | ||
| if (isReadyOrTerminal(known)) { | ||
| update.complete(known); |
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 098c961. Before deciding whether an empty checkpoint batch needs a backend call, CheckpointManager now prunes polling futures that are already completed, failed or cancelled, under the existing pollingFutures monitor. Live consumers, including another future for the same operation ID, and real queued checkpoint updates are retained. This does not cancel an RPC that has already been admitted or change operation state/result semantics.
The deterministic component tests complete the exposed poll future exactly as the READY recheck does, then force the real delayed dispatcher to flush without closing CheckpointManager. Before the fix, all four already-done cases made an extra empty API call. They now make zero calls and acquire no checkpoint lease; four live-poll/real-update controls still make one balanced request and retain their results. Existing response-gate, READY, worker-handoff and shutdown controls also pass.
Validation on this branch: 2,474 full Java17 tests (31 expected skips), 56 focused Java25 tests and all 16 installed-artifact compatibility cases. The same source defect was independently reproduced and validated on the other affected non-held Java branches; workflow references, lifecycle ordering and the selected-outcome boundary are unchanged.
This comment has been minimized.
This comment has been minimized.
| DurableExecutionOutput output; | ||
| try { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Update after rechecking the pre-PR source and real public controls: the selected-root outcome remains frozen, but that does not justify leaving new continuation admission open throughout output preparation. The narrow admission gap is fixed in 2e3edf1. New checkpoint-continuation admission closes before output SerDes and oversized-result checkpointing, without running owner callbacks or joining cleanup at that point. Existing owner signaling and accepted/running cleanup remain at End/manager close. Four old admission counterexamples now pass; awaited original-cause RETRYING, successful large checkpointing and publisher/cleanup controls are preserved. Full Java17 2,483 (31 existing skips), focused Java25 64, artifact matrix16/16.
This supersedes the earlier no-earlier-cut conclusion. It does not promise global outcome replacement, a full pre-End drain, or final terminal export for already-running unawaited work. The detailed current boundary and public evidence are in the direct response to the latest finding.
| // Register before re-reading: another checkpoint may already have delivered READY before this poll existed. | ||
| known = getOperation(); | ||
| if (isReadyOrTerminal(known)) { | ||
| update.complete(known); |
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 098c961. Before deciding whether an empty checkpoint batch needs a backend call, CheckpointManager now prunes polling futures that are already completed, failed or cancelled, under the existing pollingFutures monitor. Live consumers, including another future for the same operation ID, and real queued checkpoint updates are retained. This does not cancel an RPC that has already been admitted or change operation state/result semantics.
The deterministic component tests complete the exposed poll future exactly as the READY recheck does, then force the real delayed dispatcher to flush without closing CheckpointManager. Before the fix, all four already-done cases made an extra empty API call. They now make zero calls and acquire no checkpoint lease; four live-poll/real-update controls still make one balanced request and retain their results. Existing response-gate, READY, worker-handoff and shutdown controls also pass.
Validation on this branch: 2,474 full Java17 tests (31 expected skips), 56 focused Java25 tests and all 16 installed-artifact compatibility cases. The same source defect was independently reproduced and validated on the other affected non-held Java branches; workflow references, lifecycle ordering and the selected-outcome boundary are unchanged.
This comment has been minimized.
This comment has been minimized.
| DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload)); | ||
| DurableExecutionOutput output; | ||
| try { | ||
| var outputPayload = config.getSerDes().serialize(result); |
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 2e3edf1. The earlier explanation conflated preserving the selected root outcome with deciding when to stop accepting new work. Preserving first-winner outcome selection does not require leaving checkpoint-continuation admission open during output preparation. The successful-result path now closes that admission before calling the output SerDes or checkpointing an oversized result. The early guard only sets the admission state under the existing coordination monitor; it does not iterate owners, run their callbacks, or join cleanup there. Subsequent rejected registrations retain their existing stop-owner behavior, and the End/manager-close signaling and drain remain in their existing places.
The real public tests hold output serialization after root success, then advance an actual PENDING condition to READY. Before the change, both an unawaited child and supported minSuccessful(1) parallel completion admitted a new worker; an executor rejection reached the child with its original cause while the selected root remained successful. All four admission cases now avoid that new dispatch. Two awaited controls preserve completion and original-cause RETRYING behavior, and a successful oversized-result checkpoint still works. Full Java17: 2,483 tests, zero failures/errors, 31 existing conditional skips; 64 focused Java25 tests and 16/16 artifact compatibility cases passed.
This does not add a global outcome-arbitration or all-work-drained barrier. Already-running work can still finish after End. On both the pre-PR base and candidate, the real Invocation-view probe exports its STARTED segment at End and ignores the later OperationEnd; another BatchSpanProcessor flush leaves that export unchanged. No new unflushed SDK span is inferred. The broader running-work/final-state-export limitation is not claimed fixed by this admission change.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| var outputPayload = config.getSerDes().serialize(result); | ||
| var output = | ||
| DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload)); | ||
| executionManager.closeCheckpointContinuationAdmission(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_53bft2vg4vjxd3j53a3loalx2z
P2: Closing admission is not atomic with continuation-failure selection. A failure can win immediately before this call while anyOf has already chosen the completed handler; fatal continuations can also publish after closing. The invocation then follows the success path with executionExceptionFuture completed exceptionally. For outputs over 6 MB, CheckpointManager silently skips the final EXECUTION/SUCCEED update while its batch future completes normally, so success is returned with an empty result that was never persisted. Arbitrate success and continuation failure under the same lock, reject all post-close invocation-control publications, and add a race test covering oversized output preparation.
Codex AI reviewOne correctness issue remains in invocation outcome arbitration, with residual race risk around oversized-result checkpointing. Reviewed commit |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Fixes #756. The first-invocation timing requirement is also tracked in aws/aws-durable-execution-sdk-js#938. Related implementation: aws/aws-durable-execution-sdk-js#935.
Description
Fallback Workflow and Invocation spans referenced an SDK-created parent that was never exported. Both OTel views now materialize
DurableExecutionRootbefore creating descendants and end/flush it before invocation return, including PENDING and RETRYING invocations. The root keeps the existing ARN-derived identity; both timestamps equal the checkpointed execution start. Its attributes identify the execution ARN and synthetic root. Complete remote parents remain externally owned. Recovery may re-export the same anchor; no persistent exactly-once exporter is introduced.Configured DurableSampler paths parent descendants onto the root's actual SpanContext, retaining resolved trace state and flags, including RECORD_ONLY. Per-span sampling intent is consumed before synchronous processors can reuse it. Same-loader samplers use the context carrier; foreign samplers use the existing one-shot bridge and preserve the complete delegate SamplingResult. Decision reuse is keyed by execution ARN and canonical trace ID within the existing 256-entry LRU. The documented builder/customizer must leave DurableSampler as the final installed sampler for these guarantees; a visible plain replacement retains ordinary per-span sampling. Live open parent spans also retain their provider's native clock lineage, with cached-context fallback for ended/replayed parents.
The readiness repairs retain executable READY work, normalize retry state, and resume condition attempts and step retries away from the serialized checkpoint callback. Activity remains registered through worker handoff; canceling an observation future cannot release queued/running work. Bounded-pool fairness, synchronous executors, rejection, close/admission races, stored terminal replay and completed-poll pruning have public or component regression controls. Owned continuation failures can select retryable invocation control before releasing their lease; an already-selected root outcome is not retroactively replaced.
After successful root outcome selection, new checkpoint-continuation admission closes before output serialization or oversized-result checkpointing. This early guard only closes admission: existing owner signaling remains at invocation End, and accepted/running handler cleanup plus checkpoint shutdown remain in manager close afterward. It adds no global outcome arbitration, handler-thread lifecycle migration, or all-work-drained barrier. Already-running unawaited work may still finish after End. The Invocation-view public probe on the pre-PR base and candidate exports its STARTED segment at End, then ignores the later OperationEnd; a second flush does not change that result. Await work whose result and completed telemetry must be included. The new tests distinguish this retained boundary from preventing new READY dispatch during output preparation.
Thread-bound OTel scopes now stay with the user-function worker that opened them. Invocation End closes only scopes owned by its current thread; a late UserFunctionEnd closes its own scope before the tracing-state guard. Both views restore the original application Context after the actual worker task retires, including reuse of the same worker and plugin. Recording-span End/flush, MDC behavior, selected outcomes and the existing cleanup boundary remain unchanged.
Output preparation failures dispatch one RETRYING End before propagating the original normalized failure, releasing the retained root. Root cleanup preserves original-failure/fatal precedence and suppressed diagnostics. An isolated root-end Exception or LinkageError still flushes completed spans; unisolated Errors retain their existing behavior. Root resources are captured and cleared before callbacks, supporting partial Start, reentrant cleanup and reuse. Outer response emission and runtime acknowledgment remain outside this SDK result-preparation boundary.
Demo/Screenshots
Not applicable (SDK/runtime and exported trace behavior).
Checklist
Testing
The final candidate passes full Corretto17
mvn -o -B clean verify: 2,493 tests, zero failures/errors, and 31 existing conditional skips. The relevant Java25 suites pass 134 tests. GitHub Build retains the repository's installed-artifact compatibility check. Remote validation is represented by the checks on the current PR head.Unit Tests
The percentage-failure fixture keeps an exact-history lane with serialized admission and a separate default-concurrency lane covering the threshold, stored results/errors, replay and completed side-effect counts. Operation replay/status handling; poll pruning with live consumers and real updates retained; continuation activity, close/failure selection, observation cancellation, and sampler/context ownership.
Integration Tests
Real public DurableExecutor/SerDes/backend controls cover output preparation, early parallel completion, READY admission, awaited rejection identity, successful oversized checkpointing, publisher/cleanup ordering and existing post-End drain. Actual BatchSpanProcessor probes on both the pre-PR base and candidate establish the retained late-completion export limit. Both OTel views cover root metadata/lifetime, sampling, provider clocks, isolated loaders, partial Start, cleanup errors and invocation reuse.
Examples
Existing local examples and conformance handlers remain in full verification. Cloud thresholds, assertions, scenario/catalog references and deployment locks are unchanged by the admission repair.