Composite Key (WorkflowInstanceId + ExportTaskId) for Export Deduplication#544
Open
Composite Key (WorkflowInstanceId + ExportTaskId) for Export Deduplication#544
Conversation
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
WalkthroughThe changes introduce a composite key mechanism using WorkflowInstanceId and ExportTaskId to replace the previous ExportTaskId-only deduplication logic across export services. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/InformaticsGateway/Services/Export/ExportServiceBase.cs (1)
593-610:⚠️ Potential issue | 🔴 Critical
BaseProcessMessagestill has unacked-duplicate and stale-key failure modes.At Line 594–598, duplicate requests are dropped without settling the broker message.
At Line 605–610, ifexportFlow.Post(...)fails afterAdd, the key is never removed.Suggested fix
string exportKey = BuildExportKey(exportRequest.WorkflowInstanceId, exportRequest.ExportTaskId); if (ExportRequests.ContainsKey(exportKey)) { _logger.ExportRequestAlreadyQueued(exportRequest.CorrelationId, exportRequest.ExportTaskId); + MessageSubscriber.Acknowledge(eventArgs.Message); return; } ... ExportRequests.Add(exportKey, exportRequestWithDetails); if (!exportFlow.Post(exportRequestWithDetails)) { + ExportRequests.Remove(exportKey); _logger.ErrorPostingExportJobToQueue(exportRequest.CorrelationId, exportRequest.ExportTaskId); MessageSubscriber.Reject(eventArgs.Message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InformaticsGateway/Services/Export/ExportServiceBase.cs` around lines 593 - 610, The duplicate-request branch and the failed-post branch leak unacked messages and stale keys: when BuildExportKey finds an existing key (ExportRequests.ContainsKey) you must settle the broker message (e.g., ack or reject via MessageSubscriber) instead of just returning; and when you Add exportRequestWithDetails to ExportRequests then call exportFlow.Post, ensure that if Post returns false (or throws) you remove the key from ExportRequests (use Remove/TryRemove on ExportRequests for the same key) before calling MessageSubscriber.Reject and logging via _logger.ErrorPostingExportJobToQueue; also guard the Post call with try/finally to remove the key on exceptions to avoid stale entries.src/InformaticsGateway/Services/Export/ExtAppScuExportService.cs (1)
70-87:⚠️ Potential issue | 🔴 CriticalSettle duplicate messages and clean up key on
Postfailure.At Line 71–75, returning on duplicate without
Acknowledge/Rejectleaves the message unacked.
At Line 82–87, whenPostfails, the key remains inExportRequests, causing stale dedupe state.Suggested fix
string exportKey = BuildExportKey(externalAppRequest.WorkflowInstanceId, externalAppRequest.ExportTaskId); if (ExportRequests.ContainsKey(exportKey)) { _logger.ExportRequestAlreadyQueued(externalAppRequest.CorrelationId, externalAppRequest.ExportTaskId); + MessageSubscriber.Acknowledge(eventArgs.Message); return; } ... ExportRequests.Add(exportKey, exportRequestWithDetails); if (!exportFlow.Post(exportRequestWithDetails)) { + ExportRequests.Remove(exportKey); _logger.ErrorPostingExportJobToQueue(externalAppRequest.CorrelationId, externalAppRequest.ExportTaskId); MessageSubscriber.Reject(eventArgs.Message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InformaticsGateway/Services/Export/ExtAppScuExportService.cs` around lines 70 - 87, Duplicate incoming messages are currently returned without settling the message and when exportFlow.Post fails the dedupe key in ExportRequests is left behind; fix by calling MessageSubscriber.Acknowledge(eventArgs.Message) (or Reject as appropriate) in the duplicate branch where _logger.ExportRequestAlreadyQueued is invoked so the message is settled, and when exportFlow.Post returns false remove the previously added ExportRequests entry (the one created with BuildExportKey and ExportRequestEventDetails) before calling _logger.ErrorPostingExportJobToQueue and MessageSubscriber.Reject(eventArgs.Message) so the dedupe state is cleaned up; locate symbols BuildExportKey, ExportRequests, _logger.ExportRequestAlreadyQueued, externalAppRequest, ExportRequestEventDetails, exportFlow.Post, _logger.ErrorPostingExportJobToQueue, and MessageSubscriber.Reject/Acknowledge to apply the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/InformaticsGateway/Services/Export/ExportServiceBase.cs`:
- Around line 593-610: The duplicate-request branch and the failed-post branch
leak unacked messages and stale keys: when BuildExportKey finds an existing key
(ExportRequests.ContainsKey) you must settle the broker message (e.g., ack or
reject via MessageSubscriber) instead of just returning; and when you Add
exportRequestWithDetails to ExportRequests then call exportFlow.Post, ensure
that if Post returns false (or throws) you remove the key from ExportRequests
(use Remove/TryRemove on ExportRequests for the same key) before calling
MessageSubscriber.Reject and logging via _logger.ErrorPostingExportJobToQueue;
also guard the Post call with try/finally to remove the key on exceptions to
avoid stale entries.
In `@src/InformaticsGateway/Services/Export/ExtAppScuExportService.cs`:
- Around line 70-87: Duplicate incoming messages are currently returned without
settling the message and when exportFlow.Post fails the dedupe key in
ExportRequests is left behind; fix by calling
MessageSubscriber.Acknowledge(eventArgs.Message) (or Reject as appropriate) in
the duplicate branch where _logger.ExportRequestAlreadyQueued is invoked so the
message is settled, and when exportFlow.Post returns false remove the previously
added ExportRequests entry (the one created with BuildExportKey and
ExportRequestEventDetails) before calling _logger.ErrorPostingExportJobToQueue
and MessageSubscriber.Reject(eventArgs.Message) so the dedupe state is cleaned
up; locate symbols BuildExportKey, ExportRequests,
_logger.ExportRequestAlreadyQueued, externalAppRequest,
ExportRequestEventDetails, exportFlow.Post,
_logger.ErrorPostingExportJobToQueue, and MessageSubscriber.Reject/Acknowledge
to apply the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26c9a171-0d89-407b-809f-1c093cde860d
📒 Files selected for processing (3)
src/InformaticsGateway/Services/Export/ExportServiceBase.cssrc/InformaticsGateway/Services/Export/ExtAppScuExportService.cssrc/InformaticsGateway/Test/Services/Export/ExportServiceBaseTest.cs
bluna301
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Previously,
ExportServiceBaseused onlyExportTaskIdas the key for tracking in-progress export requests. This caused requests with the sameExportTaskIdbut differentWorkflowInstanceIdsto be incorrectly deduplicated, silently dropping valid export tasks. This eventually leads to RabbitMQ timeout, Informatics Gateway -- RabbitMQ disconnection, and blocking of the Informatics Gateway due to failure to resolve the dropped valid export tasks.This issue commonly happens when two workflow instances are executing at the same time, and the Task Ids associated with each workflow are identically named (for example,
"id": "export-svrtk-orthanc"for a task that exports SVRTK series to ORTHANC).Manifestation in MONAI Deploy Express environment:
mdl-iglogs:Rabbit MQ UI (Timeout Period):
RabbitMQ UI (IG Blocked):
Changes:
BuildExportKey()helper combiningWorkflowInstanceIdandExportTaskIdas a composite keyReportingActionBlocklookups and inExtAppScuExportService.ExportRequestsdictionaryDataflowTest_SameExportTaskId_DifferentWorkflowInstanceId_BothProcessedverifying both tasks are processed"Status
Ready
Types of changes
Summary by CodeRabbit
Bug Fixes
Tests