Skip to content

fix: export synthetic OTel roots and retain READY wait progress - #767

Open
zhongkechen wants to merge 45 commits into
mainfrom
fix/otel-synthetic-root-756
Open

zhongkechen wants to merge 45 commits into
mainfrom
fix/otel-synthetic-root-756

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.

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 DurableExecutionRoot before 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

  • PR template sections completed
  • Focused public regression and before/after controls completed
  • Formatting and full component verification completed
  • Existing artifact-compatibility CI gate retained
  • Behavior and limitations documented

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.

@zhongkechen
zhongkechen requested a review from a team October 2, 2026 20:45
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 20:45 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 20:45 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 20:56 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 21:13 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 21:30 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 21:42 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 2, 2026 22:30 — with GitHub Actions Active
@github-actions

This comment has been minimized.

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

This comment has been minimized.

Comment on lines +570 to +571
public CompletableFuture<Void> runCheckpointContinuation(BaseDurableOperation owner, Runnable continuation) {
return runCheckpointContinuation(owner, continuation, InternalExecutor.INSTANCE);

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

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 00:09 — with GitHub Actions Active
* 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.

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

Comment on lines +295 to +296
if (root != null) root.end(rootTimestamp);
if (shouldFlush && provider != null) {

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

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 00:37 — with GitHub Actions Active
Throwable error,
Object executionInput,
Object executionResult) {
executionManager.beginInvocationEnd();

This comment was marked as outdated.

@zhongkechen zhongkechen Oct 9, 2026 •

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.

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.

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

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 01:33 — with GitHub Actions Active
Comment on lines +179 to +180
DurableExecutionOutput output;
try {

This comment was marked as outdated.

@zhongkechen zhongkechen Oct 9, 2026 •

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.

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.

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

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 02:12 — with GitHub Actions Active
DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload));
DurableExecutionOutput output;
try {
var outputPayload = config.getSerDes().serialize(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.

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.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 18:03 — with GitHub Actions Active
failure);
UnrecoverableDurableExecutionException selected;
synchronized (activeThreads) {
if (closing && !(failure instanceof VirtualMachineError || failure instanceof ThreadDeath)) return;

This comment was marked as outdated.

@github-actions

This comment has been minimized.

Frank Chen added 2 commits October 9, 2026 18:33
Reuse the reviewed test-only pattern from 3b54d29 and 87a83ea. Preserve exact serialized-history assertions and independent default-concurrency, threshold, stored-result and side-effect replay coverage.
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 9, 2026 18:34 — with GitHub Actions Active
var outputPayload = config.getSerDes().serialize(result);
var output =
DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload));
executionManager.closeCheckpointContinuationAdmission();

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

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

One correctness issue remains in invocation outcome arbitration, with residual race risk around oversized-result checkpointing.

Reviewed commit 3a6ad164f83dc14c2c223cbc13c3d0d0b1f8eb91. Workflow run

This branch was successfully deployed

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

[Bug]: export stable fallback synthetic roots, including before suspension

1 participant