Repository navigation
fix(sdk): export stable synthetic roots before suspension - #966
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.
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.
ParidelPooya
left a comment
There was a problem hiding this comment.
1. Add durable.execution.arn to DurableExecutionRoot (requested before merge)
createSyntheticRoot sets one attribute, durable.execution.synthetic_root (execution-plugin.ts:418, invocation-plugin.ts:394). The composed test pins that set with toEqual (synthetic-root-lifecycle.composed.test.ts:88).
- Every other span these plugins create carries
durable.execution.arn. That includesInvocation,Workflowand the operation spans. - So the root is the one span that a query filtered on the execution ARN does not return.
- This PR's contract is that every copy of the root has identical span content.
- Suppose a later release adds the attribute. An execution that runs across that deploy exports two copies with the same span ID. One has the ARN and one does not.
- So adding the attribute later breaks the identical-copy contract for those executions.
- Today the root exports only at terminal completion. So this PR is the last release where adding it costs nothing.
2. "No second sampler query for the anchor" holds only when the sampler wrapper is installed
The anchor is created through startSpan. startSpan routes through DurableSampler.withDecision only when tryInstallDurableSampler succeeded. That fails when the tracer does not expose _sampler, for example a wrapping tracer. In that case the provider's sampler is called for the anchor too.
I counted sampler calls per invocation for this sequence: first PENDING, resume PENDING, terminal SUCCEEDED.
| Setup | This PR | main |
|---|---|---|
| Sampler wrapper installed | [1, 1, 1] | [1, 1, 1] |
| Sampler hidden behind a wrapping tracer | [2, 1, 3] | [1, 1, 3] |
The extra call is also visible in the output:
- With no visible sampler,
rootSamplingDecisionreturnsRECORD_AND_SAMPLED(execution-trace-context.ts:251-253). - So the
executionSamplingDecisionguard increateSyntheticRootpasses. - Now take a sampler that rejects its first call and samples later calls.
- Then the first
PENDINGinvocation exports the root alone, and itsInvocationchild is dropped. Onmainthat invocation exports nothing.
3. The README overstates how the terminal backup recovers a lost first flush
The anchor is gated on info.isFirstInvocation (execution-plugin.ts:271, invocation-plugin.ts:238).
- Core sets
isFirstInvocationonly inExecutionMode(with-durable-execution.ts:116). - Core selects
ExecutionModeonly when the operations array has at most one entry (execution-context.ts:100-103). - So a retry after the first invocation checkpointed a step runs in
ReplayMode, and it creates no anchor. - I measured this sequence. The first invocation checkpoints a step, then is killed before its flush. The retry returns
PENDING. The execution is then stopped while suspended. The result is 0 roots, on this branch and onmain, in both views. - In that sequence, the retry's
Invocationspan points at the root span ID, which is never exported. - A plugin constructed before the global provider registers behaves the same way. Its first invocation has tracing disabled, so no anchor is created.
So two README sentences hold only conditionally:
- "This backup covers a first invocation that never reached its flush" (
README.md:337) holds only if a terminal invocation runs later. - "first-invocation retries can emit further copies" (
README.md:335) holds only for retries that are still inExecutionMode.
4. Copy policy: consider emitting the anchor on every sampled fallback invocation
The current policy emits the anchor on the first and terminal invocations only. For first PENDING, two resumes PENDING, then terminal, roots per invocation are [1,0,0,1]. On main they are [0,0,0,1].
The policy leaves three gaps:
- The point 3 case: a lost first flush, then suspend, then stop or timeout. Result: 0 roots.
- The late-registered global provider from point 3. Result: no root until terminal.
- A sampler that samples only an intermediate invocation. That invocation's
Invocationspan has no exported parent. I measured 1 orphan span in both views, on this branch and onmain.
Suppose the anchor were emitted on every sampled fallback invocation that has executionStartTimestamp. Then all three gaps close. Each extra copy is one zero-duration span with the same content as the others. Deduplication by span ID already has to handle the first-invocation retry copies this PR adds.
5. Document that descendants end after the root
The anchor starts and ends at executionStartTimestamp, so its duration is 0. Workflow and Invocation spans are its children, and they end later. In a three-invocation execution, every child ended after the root by up to the full execution duration. On main the root covered its children within 0.3 ms.
This is a consequence of exporting the root before the execution ends, and I agree with the trade-off. Some trace viewers and validators flag a child that ends after its parent. Suggestion: add one sentence to the README section. It should say that DurableExecutionRoot is a zero-duration identity span, and that its descendants end after it.
|
Thanks for the detailed review. I reproduced the concerns and addressed them in db3d8958:
Validation: 456 full OTel tests, build/declaration and source/test type checks, and lint pass. Two extended existing unit tests and four public SDK step/wait/wait/resume tests fail on the prior head and pass with the fix. The public tests observe the next PENDING boundary after a failed first Batch exporter call, with the completed step still running only once. Composed tests separately model an invocation that never reached its flush; I am not claiming a literal process-kill or guaranteed-delivery test. Current-head CI is tracked on this PR. |
This comment has been minimized.
This comment has been minimized.
ParidelPooya
left a comment
There was a problem hiding this comment.
Thanks. I re-ran my probes against db3d8958, and all five points are resolved.
- Both views put
durable.execution.arnonDurableExecutionRoot. Across cold and warm retries, every copy is still identical in its SDK-owned fields. - When the sampler is hidden behind a forwarding tracer, sampler calls are
[1, 1, 3]again. That matchesmain. The lone early root no longer appears. - The README wording on recovery matches the code.
- A first invocation that is killed before its flush, followed by a replay-mode retry and then a stop, now leaves one exported root. Before this commit it left none. A late-registered provider and an intermediate-only sampled invocation both produce a root and leave no orphaned
Invocationspan. - The README documents the zero-duration root and its descendants ending after it.
The full OTel suite passes 456 of 456, and Biome and tsc --noEmit are clean.
Codex AI reviewNo actionable findings. Residual risk: repository tests were not executed, per review constraints. 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 #938. Follows the merged #935 fix for #934.
Description
A fallback execution stopped or timed out while suspended can leave exported descendants without their SDK-owned root, because the root previously appeared only at terminal completion. Both OTel views now create and end
DurableExecutionRootat each sampled fallback invocation's start when a stable backend start timestamp is available and the configured SDK tracer supports the durable sampler wrapper. The anchor carriesdurable.execution.arn; its start and end equal the backend timestamp, and its span ID remains execution-ARN-scoped.Each invocation emits at most one copy, including an invocation that also terminates. Identical copies on retries and intermediate resumes can recover a lost earlier export, including replay-mode retries after checkpointed work and late provider registration. Recovery still requires a later eligible invocation to deliver its copy: a service-side stop/timeout before any successful delivery can leave the root absent. Copies exclude request-specific state and terminal outcome.
DurableExecutionRootis an identity-only, zero-duration span whose descendants end after it; viewers or containment validators may flag that.Workflowcontinues to carry duration and outcome.With an installed
DurableSampler, the anchor reuses the invocation's resolved sampling decision without querying the sampler again. A forwarding tracer that hides or prevents wrapping its sampler retains the existing terminal-only root path and its provider's per-span sampling behavior; it receives no extra early-anchor sampler call. The once-per-invocation sampling guarantee does not apply to opaque tracers. Complete propagated remote parents remain unmaterialized. Managed upstream NOT_SAMPLED and RECORD_ONLY controls remain unchanged.Without
executionStartTimestamp, the existing terminal-only root/timestamp fallback remains; no wall-clock-derived early anchor is introduced. The README documents stable trace/configuration requirements, provider-resource differences, replay-mode retries, and export limitations. ExposedforceFlush()calls remain awaited, but delivery after process loss is not guaranteed. Public configuration, provider ownership, clock logic, checkpoint state, runtime dependencies, and non-root span construction code are unchanged.Demo/Screenshots
After a real step and two durable waits, each sampled invocation exports the same ended anchor before returning PENDING; Workflow remains deferred until terminal completion. If the first batch export fails, the next PENDING invocation delivers a replacement anchor without repeating the completed step. A later terminal invocation uses the same identity and does not emit a second root within that invocation.
Checklist
Testing
456 OTel tests pass across 22 suites, including installed-consumer compatibility and sampling/ID controls. Core/testing and OTel ESM/CJS/declaration builds, full OTel source/test type checking, package lint, and diff checks pass. The final focused public checks also retain the original normal-first-return Invocation-export assertion.
Unit Tests
Two existing lifecycle unit tests now assert that a resumed invocation exports an ARN-correlated zero-duration anchor before terminal completion and that the terminal hook does not duplicate it. Both extended unit tests fail on the prior PR head. The original invocation ARN and shared-trace topology assertions remain in place.
Composed Tests
48 real-provider cases cover both views, including opaque forwarding tracers, reject-first and intermediate-only sampling, late global registration, replay-mode recovery from an unflushed first invocation, immutable copies across resources, terminal deduplication, timestamp omission/mutation, propagated parents, explicit/ratio/changing sampling, and RECORD_ONLY. Twelve newly added composed regressions fail on the prior implementation. The unflushed-first-invocation case models missing end/flush hooks; it is not a literal process-kill test.
Integration Tests
Four public
withDurableExecution+LocalDurableTestRunnercases execute real step/wait/wait/resume paths with a BatchSpanProcessor, for both views with normal delivery and a failed first exporter call. The observer uses the public asynchronous hook contract. Snapshots after wrapper returns prove an anchor is present at the second PENDING boundary before a terminal hook, completed work executes once, and copies retain identical span content. All four fail at that intermediate-boundary assertion on the prior PR head and pass with this change.Examples
No API/example change is needed for this telemetry lifecycle fix. Cross-SDK conformance coordination remains tracked in aws/aws-durable-execution-conformance-tests#99; no conformance requirement files are added to this SDK repository.
CI fixture corrections
Concurrent waits and retry timers do not guarantee coalesced invocation boundaries. Include the focused
promise-all-waitandpromise-anytest corrections from #959: bound invocation completions by the initial invocation plus their independent wakeups (1–3 and 1–7 respectively), while preserving exact operation/history counts, terminal statuses, results and errors. The promise-any case additionally verifies all three starts and failures for each of its three steps. Existing timeouts remain in place. Both focused examples and root lint pass; these corrections change test expectations for valid schedules, not SDK runtime behavior.