Repository navigation
Fix Pending registration for entity-started orchestrations - #1430
wangbill (YunchuWang) wants to merge 2 commits into
Conversation
Register Azure Table Pending instances after enqueueing entity-generated starts and before checkpointing the sender. Reconcile generated starts, discard terminated delayed executions, and preserve conditional-write and large-input metadata conventions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical issue allows a new Pending child with a reused ID to run despite termination because older completed history causes the termination to be discarded, requiring a fix and regression coverage.
1 open issue
What changed in this PR
This PR addresses #1258 by registering entity-started orchestrations as Pending in the Azure Table tracking store, making scheduled children discoverable and terminable before execution.
Changes:
- Registers Pending state after enqueueing and before checkpointing the sending entity.
- Adds start reconciliation, delayed-start termination checks, and large-input reference handling.
- Adds regression tests and documents persistence ordering and worker-upgrade requirements.
| File | Description |
|---|---|
| test/DurableTask.AzureStorage.Tests/EntityStartedOrchestrationTests.cs | Adds scheduling, termination, retry, and race coverage. |
| src/DurableTask.AzureStorage/Tracking/ITrackingStore.cs | Clarifies Pending registration documentation. |
| src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs | Preserves large-input URLs in Pending rows. |
| src/DurableTask.AzureStorage/OrchestrationSessionManager.cs | Includes registered entity starts in reconciliation. |
| src/DurableTask.AzureStorage/Messaging/TaskHubQueue.cs | Returns queued-message metadata to callers. |
| src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs | Registers entity starts and checks for terminated Pending executions. |
| docs/providers/azure-storage.md | Documents entity-start behavior and compatibility requirements. |
Review effort: Balanced
Reconcile execution-independent termination against the newer Pending execution before discarding stale terminal history, with an ETag-checked merge. Keep ordinary client and awaited-child admission free of extra status reads while retaining the registered-entity race check. Add cross-framework lifecycle, identity, race, failure and cancellation regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Reviewed original head 250f78f3 and follow-up a5435df69eee1e8351227b911c13b18409c59d8f.
Found and fixed: P1 — termination could be discarded for a newer Pending execution when an instance ID still had older terminal history. The real client/entity/worker regression reproduced Expected Terminated / Actual Completed in both instance-table ETag modes: the delayed replacement actually ran. The follow-up now reconciles execution-independent termination against the current Pending row before rejecting the old terminal-history batch. Its conditional merge uses the observed ETag, preserves execution/generation/reason/history, and propagates conflicting writes, cancellation and storage failures instead of overwriting or recreating a row. A termination targeting the old execution does not cancel the replacement.
The review also removed the newly added status read for ordinary client, awaited-child and legacy starts, and replaced repeated message scans with one local scan. A fresh check remains only for registered entity admission: a separate failing regression showed that a reconciliation snapshot can become stale while the start waits behind termination. Query-count cases prove that scope and single-read reuse on the terminal-history path. The two nullable/analyzer comments were addressed with explicit checks/assertions, not casts or warning suppression.
I checked queue-before-Pending-before-sender-checkpoint ordering, default/custom-store boundaries, large-input references, conditional updates and non-resurrection, targeted versus execution-independent termination, and the final regression evidence. No additional high-confidence correctness issue identified in the revised diff.
Parent verification of the retained before/after TRX records: final entity matrix 59 + 59, existing Azure Storage lifecycle cases 128 + 128, and Core entity cases 24 + 24 across .NET 8 / .NET Framework 4.8: 422 passed, zero failed/skipped. Final multi-target build succeeded with zero warnings/errors.
Production boundary: agent-run validation used only owned, isolated local Azurite. No real Azure Storage account or production environment was tested, provisioned or deployed to. Local/hosted test results are not production qualification; all task-hub workers still need the documented compatible upgrade. Full-solution and live-production validation are not claimed. This is a comment review, not approval or merge.
There was a problem hiding this comment.
🟡 Changes recommended
Ignoring a failed execution registration after a concurrent termination can let the entity checkpoint with the old generation, causing the new start to be deferred and eventually discarded.
1 open issue
1 resolved since last review
Review effort: Balanced
| : this.messageManager.GetBlobUrl(queuedMessage.CompressedBlobName); | ||
| try | ||
| { | ||
| await this.trackingStore.SetNewExecutionAsync(startEvent, existingInstance?.ETag, inputPayloadOverride); |
There was a problem hiding this comment.
Confirmed against unchanged head a5435df6 using a real entity/worker plus owned local Azurite, in both instance-table ETag modes. A repeated termination changed only the same old historyless Terminated row's ETag. The source entity checkpointed/acked once, but its new start carried generation 8 while the stored row stayed at the old execution/generation 7. Driving the actual queued message through the unchanged reconciler produced real deliveries 2–10; it was deleted at 10 and child dispatches stayed zero. Only that owned message's visibility delays were shortened to bound the experiment; counts and production reconciliation were not fabricated.
This is a blocking start-loss regression. A false registration result cannot be treated as success merely because a different checkpoint or execution might have won: this case leaves the same prior terminal execution current. The minimum correction should refetch/conditionally reconcile that outcome or fail before the source checkpoint so the existing entity replay can recover. No new retry framework is needed. This review made no production-code changes, and this thread intentionally remains unresolved.
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Reviewed exact head a5435df69eee1e8351227b911c13b18409c59d8f again for necessity, scope creep and regressions. Changes still required; do not merge this head. My earlier no-additional-issues verdict was incomplete.
P1 — lost entity-started orchestration after a benign ETag conflict. SendEntityOrchestrationStartAsync ignores SetNewExecutionAsync returning false. A repeated termination can update only the same old historyless Terminated execution's ETag, leaving the new generation unregistered while the entity nevertheless checkpoints successfully. This was reproduced on unchanged production code in both ETag modes: source invocation/ack once, new start generation 8, stored old execution/generation 7, actual reconciliation deliveries 2–10, then queue deletion with zero child dispatches. The existing inline thread has the detailed evidence and remains unresolved. The minimum fix is explicit registration-outcome handling that preserves legitimate newer/progressed execution outcomes but does not commit this failed replacement; use the existing replay/retry lifecycle instead of another abstraction.
Necessity: 901 of 1,062 added lines are tests; production adds 151 and removes 19 lines across five files. Pending registration, queue/sender ordering, generation reconciliation, the demonstrated admission recheck, conditional termination and large-input/sender metadata handling each support a concrete correctness requirement. There is no new dependency, public API or schema framework to justify removing wholesale. The three start/termination scans could be organized differently, but cosmetic restructuring is not a substitute for handling the dropped registration result. Avoid adding generic retries/caches/capability layers. A same-prepared-envelope 503 retry failed in a direct helper probe, but actual stock entity replay rebuilt a fresh envelope and completed; that is not reported as a second independently confirmed production bug.
Hosted CI: all 13 Azure Storage job failure statuses refer to the shared run 1819540 in build 314139: 321 passed, one failed EnsureOwnedQueueExclusive, one existing skip. The identical queue-ownership assertion also fails on pre-PR main build 313418/run 1816202 and passes on the exact-base main run 1818967. That is a verified pre-existing intermittent failure signature, not proof this PR caused it; CI is nevertheless currently red. The previously passing 422 focused cases did not exercise the newly demonstrated registration-conflict loss.
This was review/reproduction only: no production changes, commit or push, and temporary test edits/emulator were cleaned up. No real Azure Storage or production environment was tested. This is a comment review, not approval or merge.

Summary
Fixes #1258.
Entity-generated orchestration starts bypass the client creation path, so the default Azure Table tracking store previously had no instance record until execution began. Register these starts as
Pendingso scheduled children are discoverable and terminable before execution.Review correction
Review found a correctness gap in the original head
250f78f3: when an entity reused an ID with Completed history, an execution-independent termination was discarded against that old history and the newer delayed child still ran. A real client/entity/worker regression reproduced Expected Terminated, Actual Completed with both instance-table ETag settings.The follow-up reconciles that termination against the current Pending row before rejecting the old terminal-history batch. Its merge uses the observed ETag, preserving execution ID, generation, reason and prior history; replacement/checkpoint/purge conflicts and storage failures propagate to the normal worker lifecycle. A termination explicitly targeting the old execution cannot cancel the newer Pending execution.
The follow-up also makes the two nullness assumptions explicit and removes the extra admission lookup for ordinary client, awaited-child and legacy starts. Registered entity starts retain a scoped fresh check: the earlier reconciliation snapshot can become stale while a start waits behind termination. A failing admission-race regression demonstrates why that check is necessary; query-count tests verify the scope and single-snapshot reuse for the terminal-history path.
Before and after
EntityStartedScheduledOrchestrationIsPendingAndCanBeTerminatedfailed with missing child state after the real entity dispatch checkpoint. The corrected flow exposes Pending state through point and list queries, preserves scheduled time/input/tags/parent metadata, and consumes the terminated delayed start without executing orchestration code.ReusedEntityStartedScheduledOrchestrationCanBeTerminatedfailed on250f78f3in both ETag modes because the replacement child ran. Both variants now remain Terminated, retain the new execution identity/generation/reason, and leave the old terminal history intact.Persistence ordering and compatibility
The queue write remains before Pending registration and the sending entity's history checkpoint. Workers defer starts while registration is in flight. Conditional inserts and ETag-checked replacements prevent a late Pending write from rolling back an execution or overwriting a newer one. The new reused-history termination path also uses an ETag-checked merge rather than an upsert; conflicts, cancellation and unexpected storage failures are not reported as successful termination.
This applies to the default Azure Table tracking store; custom instance stores retain their existing send path. No public API or storage schema is added. Upgrade all workers in a task hub for the new reconciliation/termination guarantees; older entity messages without a generation retain legacy behavior. At-least-once delivery remains unchanged.
Missing-row termination handling for already-purged instances (#1398 / #1415) remains out of scope.
Validation and production boundary
No real Azure Storage or production testing was performed. No customer/production resources were accessed, no Azure Storage account was provisioned, and no deployment was performed. This is local validation, not production qualification.
Storage runs used two owned, private in-memory Azurite processes on loopback ports 12650-12652 and 12660-12662, isolating the legacy task hub between frameworks. Both helpers were stopped after validation.
Total: 422 passed, 0 failed. The final sequential multi-target build succeeded with 0 warnings and 0 errors; Git whitespace checks passed. Before/after commands and TRX evidence are retained in the session's
review1430-validation.txtandreview1430-testsartifacts.Focused command:
The review cases cover old Completed/Failed/Terminated/Canceled histories, explicitly old-targeted versus execution-independent termination, large reasons and metadata preservation, concurrent replacement/checkpoint/purge, cancellation/error propagation, post-reconciliation admission and scoped status-read counts.