Clear persisted instance output during Azure Storage rewind - #1397
wangbill (YunchuWang) wants to merge 8 commits into
Conversation
Replace the complete Azure Table instance row with its current ETag so rewind removes the terminal Output property without clobbering concurrent fields. Add unit and Azurite coverage for persisted state, retries, terminal rewrites, instance reuse, and shared large-output blobs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The core behavior change is narrowly scoped, uses ETag-guarded replaces to avoid concurrency clobbering, and is backed by targeted unit and Azurite scenario coverage.
Pull request overview
This PR fixes Azure Storage rewind persistence by ensuring the Instances-table Output property is physically removed when rewinding a terminal orchestration back to Pending, using an ETag-guarded full-entity Replace to avoid clobbering concurrent updates and to preserve unrelated properties.
Changes:
- Update
AzureTableTrackingStore.UpdateStatusForRewindAsyncto read the full Instances row, removeOutput, andReplaceusing the current ETag (instead ofMerge). - Add raw-storage unit coverage for output deletion, idempotency when output is missing, and ETag conflict propagation.
- Add Azurite scenario coverage for large-output rewinds to ensure the instance
Outputis removed while the history-referenced blob remains available.
File summaries
| File | Description |
|---|---|
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs |
Switch rewind status update from Merge to ETag-guarded Replace and remove persisted Output property. |
test/DurableTask.AzureStorage.Tests/RewindOutputTrackingStoreTests.cs |
Add raw Azurite tests validating Output removal, idempotency, terminal rewrites, and instance ID reuse behavior. |
test/DurableTask.AzureStorage.Tests/AzureTableTrackingStoreTest.cs |
Add mock-based unit tests asserting full-entity Replace with current ETag and 412 propagation behavior. |
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs |
Add end-to-end coverage for large-output rewind: instance Output cleared while shared history blob remains. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Remove terminal history payload references before clearing the instance output and deleting the uniquely owned large-message blobs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf71194c-b467-4684-b5f6-f31ff9a2e60e
wangbill (YunchuWang)
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Blob deletion exceptions can abort the rewind after storage pointers are removed, leaving the instance rewound but not revived; making deletion best-effort (or otherwise non-fatal post-pointer-removal) would improve reliability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Normalize ordinary blob retry and timeout failures, log them after table dereference, and continue producing rewind targets while preserving caller cancellation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
|
Copilot review |
There was a problem hiding this comment.
🔵 Needs a closer look
The changes affect core rewind/persistence semantics and storage cleanup ordering in production and should receive final human validation despite strong test coverage.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review completed on |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
wangbill (YunchuWang)
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The functional change (removing persisted Output via ETag-bound replace while preserving other fields) is implemented coherently and is backed by comprehensive unit and scenario test coverage for concurrency and regression cases.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
|
Addressed the test-code review comments in 5acbb3f. Injected handlers and cached HTTP transports now have explicit test-scoped ownership; DelegatingHandler disposes its inner handler through the normal base disposal chain, without duplicate manual disposal. Added explicit non-null assertions, preserved absent-versus-null history checks with TryGetValue, and removed the shadowed local name. This follow-up changes tests only; the narrowed production implementation is unchanged. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to correct a known Azure Table merge limitation, adds execution/ETag fencing for safety, and includes substantial unit and scenario regression coverage for the new behavior.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies core rewind/concurrency behavior in the Azure Storage provider with intricate recovery logic, so it warrants final human review despite strong test coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
There was a problem hiding this comment.
🔵 Needs a closer look
It makes substantial, concurrency-sensitive changes to the Azure Table rewind algorithm and instance mutation semantics that warrant final human review despite strong test coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4b381ba-7e3f-4548-ab7d-73eac7a4466a
There was a problem hiding this comment.
🟡 Changes recommended
Three unresolved critical issues remain in AzureTableTrackingStore.cs.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
| TableEntity liveChildRewindStartEntity = | ||
| await this.GetInstanceEntityForRewindAsync(childInstanceId, cancellationToken); | ||
| var liveEdge = new RewindRecoveryEdge( | ||
| instanceId, | ||
| executionId, | ||
| taskScheduledId.GetValueOrDefault(), | ||
| childInstanceId, | ||
| liveChildRewindStartEntity.GetString(nameof(OrchestrationInstance.ExecutionId))); |
| // "clear" failure event by making RewindEvent: replay ignores row while dummy event preserves rowKey | ||
| entity[nameof(TaskFailedEvent.Reason)] = "Rewound: " + entity.GetString(nameof(HistoryEvent.EventType)); | ||
| entity[nameof(TaskFailedEvent.Reason)] = RewoundReasonPrefix + eventType; | ||
| entity[nameof(TaskFailedEvent.EventType)] = nameof(EventType.GenericEvent); | ||
|
|
||
| await this.HistoryTable.ReplaceEntityAsync(entity, entity.ETag, cancellationToken); |
| $"{AzureTableQueryFilter.ColumnEquals(nameof(OrchestrationInstance.ExecutionId), executionId)} and " + | ||
| $"{AzureTableQueryFilter.ColumnEquals(nameof(HistoryEvent.EventType), nameof(EventType.ExecutionCompleted))}"; |
Summary
Outputproperty from the current Instances row when Azure Storage rewinds the selected execution.Why this revision is narrower
The original rewind used Azure Table Merge, which updated the status but could not remove the previous
Outputproperty. This change fixes the persisted current-instance record, rather than hiding output in an API serializer.The previous revision also deleted output blobs. Deterministic review probes demonstrated that physical deletion could break an already-started public status query. Immediate physical deletion is no longer part of this PR.
This is not complete failure-data erasure. Rewind continues to rewrite failed history events as before, but retains their payload properties and blob references. Associated blobs remain on the existing lifecycle until an explicit instance purge. No automatic GC, timer, service, cleanup ledger, or new recovery protocol is introduced or assumed.
Execution identity and concurrency
Each recursive rewind captures its execution ID and Instances ETag before changing that node's history. Before replacing the Instances row, the implementation rereads the complete entity and validates that it still belongs to the selected execution.
UseInstanceTableEtagis enabled.This lets the existing rewind flow retain the child targets it has already discovered and enqueue them after an equivalent competing reset.
Recovering after a failed reset
A same-execution terminal-state repair can change the parent ETag and leave it Failed. That conflict still surfaces as an error, and the service still sends no revival messages until the entire target enumeration succeeds.
To keep a later parent retry useful, this revision preserves and reselects the existing
GenericEventmarker with reasonRewound: SubOrchestrationInstanceFailed. It also preserves the original meaning of rewound completion markers instead of overwriting it withRewound: GenericEvent. No new stored field or recovery record is added.Historical recovery requires evidence for the child's current execution, including its rewound completion marker and the persisted
ExecutionStarted.ParentInstancerelationship. Parent instance ID, parent execution ID, and the scheduled child edge must match. A live current-execution completion, unrelated reused execution, or unprovable ownership rejects historical recovery.This is explicit retry recovery, not an automatic background recovery service or an internal retry loop. Once contention stops, retrying the parent can rediscover the previously reset descendants.
Regression coverage
The permanent tests cover:
Outputremoval and preservation of unrelated fields.UseInstanceTableEtag=true, including public suspend/resume: stale write rejected with 412, raw Pending/no-Output state, and successful revival.Focused tests passed on both target frameworks: 78 passed, 0 failed, 0 skipped on net8.0; 78 passed, 0 failed, 0 skipped on net48.
To rerun the focused coverage:
Scope and remaining limitations
UseInstanceTableEtag=false, existing unconditional delayed writers still lack conditional-write protection. This PR does not redesign that provider mode.azure-functions-durable-extensionneeds to update its dependency to consume it.Related: Azure/azure-functions-durable-extension#968