You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix ControlQueue buffer cleanup after batch failures - #1434
Roll back buffer IDs newly reserved by a ControlQueue batch if the batch fails before handoff.
Preserve reservations from earlier batches, including duplicates.
Regression coverage
One mocked test verifies new reservations are removed, an existing duplicate reservation survives, and receiving can continue after the failed batch.
Validation
The regression fails without the fix.
Passes on .NET 8 and .NET Framework 4.8.
No Azure Storage or experiment resources used.
This PR is limited to ControlQueue.cs and its focused test in MessageManagerTests.cs. Session management, work-item preparation, prefetch retry, shutdown, rewind, and completion behavior are unchanged.
Release failed dequeue and deduplication buffer slots, retry storage timeouts, and abandon sessions that never handed off a work item without disturbing deferred or replacement-session ownership.
Add focused regressions for these failure paths.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The failure cleanup can remove an already-buffered duplicate’s reservation while its original message remains live, undercounting buffered messages and allowing receives to exceed ControlQueueBufferThreshold.
This PR improves Azure Storage failure recovery by cleaning up message-buffer reservations and sessions when processing fails before work is handed off.
Changes:
Adds cleanup for failed dequeue batches, execution-start deduplication, and work-item preparation.
Retries storage-request cancellations when the listener itself has not been canceled.
Carry only newly reserved message IDs through execution-start status checks so failures do not remove buffer entries owned by earlier copies of duplicates.
Cover duplicates whose original messages remain in pending batches or active sessions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
A pending duplicate can be requeued after preparation failure without restoring its buffer reservation, causing it to escape ControlQueueBufferThreshold accounting.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Restore buffer reservation when requeuing pending duplicate
If a duplicate of a current message arrives while work-item preparation awaits storage, it enters session.PendingMessages without reserving another buffer slot. This abandonment removes the shared message ID from PendingOrchestratorMessages, but session release requeues the pending copy without restoring its reservation, so it no longer counts against ControlQueueBufferThreshold. Preserve or restore reservations for retained pending copies during the session handoff, and add a preparation-failure regression test for this case.
Re-establish buffer entries for pending and deferred messages at the locked session handoff. This preserves a reservation when abandonment of a current message removes the entry shared with a pending duplicate.
Add a deterministic preparation-failure regression that injects the duplicate while storage validation is awaiting completion.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Critical reservation-ownership bugs can leave reservations unreleased after forced shutdown or remove reservations for retained duplicates, blocking dequeuing or leaving buffered messages uncounted against the threshold.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Forced shutdown leaks buffer reservations for aborted sessions
Forced shutdown calls AbortAllSessions(), which clears the active registry without releasing buffer reservations, so this guard skips cleanup if an in-flight preparation request subsequently cancels or times out. Restarting the same service reuses those reservations and can block dequeuing once ControlQueueBufferThreshold is reached. Distinguish aborted sessions from already-cleaned sessions, or release their reservations during abort, while preserving replacement-session ownership.
Rollback can remove reservations retained by duplicate sessions
After abandonment removes a reservation, this dequeue can reserve the same ID while an earlier duplicate remains pending; if TryReleaseSession requeues that duplicate before this batch fails, its TryAdd does not establish a separate reservation. This rollback then removes the retained duplicate's reservation, leaving buffered messages uncounted against ControlQueueBufferThreshold; the same issue occurs in OrchestrationSessionManager.cs:167–169. Coordinate reservation ownership between acquisition, session retention, and both rollback paths rather than removing entries solely by saved ID.
Remove reservations for messages owned by aborted sessions while preserving IDs still held by independent pending batches. Keep late cleanup from touching replacement sessions.
Cover delayed preparation cancellation and failure, all session message collections, resumed receiving, and shared pending reservations.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
During forced shutdown, concurrent collection mutations can make session enumeration throw before active sessions are cleared, interrupting shutdown before the lease manager stops.
Restore the existing AbortAllSessions behavior rather than enumerate message collections without synchronization shared with processing code. Remove the dedicated cleanup tests and their now-unneeded test adaptation.
Keep the earlier failed-dequeue, deduplication, and work-item-preparation fixes unchanged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Reservation cleanup can leave duplicate ExecutionStarted messages buffered but uncounted toward ControlQueueBufferThreshold, requiring recovery fixes and regression coverage for both arrival timings.
Keep only rollback of buffer IDs newly inserted by a failed ControlQueue receive batch and a focused mocked regression test.
Remove session management, work-item preparation, prefetch retry and handoff changes along with their tests and shared helper additions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed the narrowed patch at 71e83fd. The earlier session-management, work-item-preparation and prefetch changes have been removed, so the associated findings no longer apply to this diff. The remaining ControlQueue change rolls back only IDs newly reserved by a failed batch, after all batch tasks complete, preserving earlier duplicate reservations. The focused regression passed on net8.0 and net48. No remaining blocking findings in this scope. Upstream CI is still running.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wangbill (YunchuWang)
left a comment
Summary
Regression coverage
One mocked test verifies new reservations are removed, an existing duplicate reservation survives, and receiving can continue after the failed batch.
Validation
This PR is limited to
ControlQueue.csand its focused test inMessageManagerTests.cs. Session management, work-item preparation, prefetch retry, shutdown, rewind, and completion behavior are unchanged.