Implement message acknowledgment with retry logic for SignalR #4122
Conversation
Co-authored-by: imnasnainaec <[email protected]>
Co-authored-by: imnasnainaec <[email protected]>
|
@copilot Start over with a new pr. Be careful to follow the following 2 instructions:
|
Co-authored-by: imnasnainaec <[email protected]>
I've reverted all implementation changes in commit d0119a5 and created a focused plan for solution #1 only (message acknowledgment). The plan now:
The plan is ready for your review. I will not proceed with any implementation until you approve. |
|
@copilot Make the same updates to the ExportHub as the MergeHub. Approved to proceed. |
…ortHub Co-authored-by: imnasnainaec <[email protected]>
Implemented message acknowledgment for both MergeHub and ExportHub in commit 2b3f91a. Changes:
All backend tests pass (10/10 MergeController tests). |
|
@copilot Plan the following update but don't execute until the plan is approved:
|
I've created a detailed plan for implementing the retry logic with exponential backoff (5s, 10s, 15s intervals). Key components:
The plan is ready for your review. I will not proceed with implementation until approved. |
|
@copilot Plan approved. |
Co-authored-by: imnasnainaec <[email protected]>
Implemented retry logic with acknowledgment tracking in commit ad8d37f. Key changes:
All backend tests pass (10/10 MergeController tests). |
|
@copilot There's lots of duplicate code in |
…inate code duplication Co-authored-by: imnasnainaec <[email protected]>
Refactored duplicate code in commit d0942f0. Changes:
All tests pass (10/10). |
imnasnainaec
changed the title
|
commit 2b3f91a025: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4122 +/- ##
==========================================
- Coverage 74.92% 74.84% -0.08%
==========================================
Files 295 297 +2
Lines 10911 10989 +78
Branches 1366 1373 +7
==========================================
+ Hits 8175 8225 +50
- Misses 2341 2367 +26
- Partials 395 397 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Backend/Controllers/LiftController.cs`:
- Around line 423-426: The background fire-and-forget call to
_ackTracker.SendWithRetryAsync(RequestId, ...) can produce unobserved
exceptions; wrap the Task.Run payload in a try/catch that catches exceptions
from _ackTracker.SendWithRetryAsync and the inner
_notifyService.Clients.All.SendAsync call, and log them using the controller's
logger (e.g., _logger.LogError) so failures are observed; additionally, ensure
the returned Task is observed (assign to a variable and optionally attach a
continuation to log faults) and consider adding try/finally inside
SendWithRetryAsync to perform any necessary cleanup.
In `@Backend/Services/AcknowledgmentTracker.cs`:
- Around line 37-44: MarkAcknowledged currently does a non-atomic TryGetValue
then indexer set on _pendingAcks which can race with RemoveRequest and re-add a
key; change it to perform an atomic conditional update using
ConcurrentDictionary.TryUpdate so you only set the value to true if the existing
value matches the observed value (e.g., read old value with TryGetValue then
call _pendingAcks.TryUpdate(requestId, true, old)); this ensures the update is
atomic and avoids re-adding keys removed concurrently.
- Around line 52-86: SendWithRetryAsync currently calls TrackRequest(requestId)
but may exit early if sendMessageAsync() throws, leaving the entry in
_pendingAcks; wrap the send/retry logic in a try/finally so
RemoveRequest(requestId) is always executed regardless of exceptions, and
optionally catch/log exceptions from sendMessageAsync() inside the try before
rethrowing or letting them propagate so failures are visible while guaranteeing
cleanup of the tracked request.
🧹 Nitpick comments (5)
Backend/Interfaces/IAcknowledgmentTracker.cs (1)
11-17: Consider addingCancellationTokensupport for graceful shutdown.The
SendWithRetryAsyncmethod runs a retry loop with delays totaling up to 30 seconds. Without aCancellationToken, in-flight retries cannot be cancelled during application shutdown, potentially delaying graceful termination.💡 Optional enhancement
- Task SendWithRetryAsync(string requestId, string userId, Func<Task> sendMessageAsync); + Task SendWithRetryAsync(string requestId, string userId, Func<Task> sendMessageAsync, CancellationToken cancellationToken = default);Backend/Services/AcknowledgmentTracker.cs (1)
61-82: Optional: Early exit upon acknowledgment would reduce unnecessary delays.Currently, after sending, the method always waits the full delay period before checking acknowledgment. If the client acknowledges quickly, the method still waits. Consider using
TaskCompletionSourceor polling with smaller intervals for more responsive cleanup.Backend/Services/LiftService.cs (1)
225-230: Consider centralizing request ID generation.This
GenerateRequestId()implementation is identical toMergeService.GenerateRequestId(). Since both services generate IDs solely for theAcknowledgmentTracker, consider moving this responsibility to the tracker itself or a shared utility to reduce duplication.♻️ Optional: Move ID generation to AcknowledgmentTracker
The
IAcknowledgmentTrackerinterface could expose aGenerateRequestId()method, eliminating the need for both service interfaces to declare it:// In IAcknowledgmentTracker string GenerateRequestId(); // In AcknowledgmentTracker public string GenerateRequestId() => Guid.NewGuid().ToString();Then controllers would call
_ackTracker.GenerateRequestId()instead of_mergeService.GenerateRequestId().Backend/Helper/CombineHub.cs (1)
14-22: Consider validating requestId ownership.Any connected client can acknowledge any
requestId, since there's no validation that the acknowledging client is the intended recipient. While the risk is low (GUIDs are unguessable and short-lived), for defense-in-depth you could track which userId owns each requestId and validate it here.🛡️ Optional: Add ownership validation
-public void AcknowledgeMessage(string requestId) +public void AcknowledgeMessage(string requestId, string userId) { - _ackTracker.MarkAcknowledged(requestId); + _ackTracker.MarkAcknowledged(requestId, userId); }This would require the tracker to store and validate the userId associated with each requestId.
Backend/Controllers/MergeController.cs (1)
156-159: Unnecessary nestedTask.Run- consider simplifying.
GetDuplicatesThenSignalis already invoked viaTask.Runon line 144, so it's already running on a thread pool thread. The innerTask.Runon line 158 adds overhead without benefit. You can awaitSendWithRetryAsyncdirectly (fire-and-forget with discard if truly non-blocking is needed).♻️ Simplify by removing nested Task.Run
if (success) { var requestId = _mergeService.GenerateRequestId(); - // Run retry logic in background without blocking - _ = Task.Run(() => _ackTracker.SendWithRetryAsync(requestId, userId, - () => _notifyService.Clients.All.SendAsync(CombineHub.MethodSuccess, userId, requestId))); + // Fire-and-forget: don't await, just discard the task + _ = _ackTracker.SendWithRetryAsync(requestId, userId, + () => _notifyService.Clients.All.SendAsync(CombineHub.MethodSuccess, userId, requestId)); }Since this method is already running on a background thread, the async continuation will naturally run without blocking. The
_ =discard pattern is sufficient for fire-and-forget.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Backend/Helper/CombineHub.cs`:
- Around line 6-22: Update AcknowledgeMessage in CombineHub to validate that the
caller is the intended recipient before marking acknowledgment: change
AcknowledgeMessage to either accept a userId parameter or infer it and ensure
IAcknowledgmentTracker (referenced as _ackTracker) exposes methods to record the
mapping and to validate ownership (e.g., store requestId→userId when sending and
add a tracker method like TryMarkAcknowledged(requestId, userId) or
GetOwner(requestId)); inside AcknowledgeMessage, compare Context.UserIdentifier
to the owner userId from the tracker and only call
_ackTracker.MarkAcknowledged(requestId) if they match, otherwise ignore or
log/deny the attempt. Ensure CombineHub uses Context.UserIdentifier for the
check and that the tracker stores the mapping when you create/send messages so
lookup succeeds.
🧹 Nitpick comments (2)
src/components/App/SignalRHub.tsx (1)
95-109: Avoid blocking success dispatch on the acknowledgment RPC.
await connection.invoke(...)can delay UI updates if the call stalls or times out. Consider firing the ack call without awaiting it and keep the dispatch immediate.🛠️ Suggested adjustment
- if (connection && requestId) { - try { - await connection.invoke(acknowledgeMethodName, requestId); - } catch (error) { - console.warn("Failed to send acknowledgment:", error); - } - } - - dispatch(successAction); + if (connection && requestId) { + void connection + .invoke(acknowledgeMethodName, requestId) + .catch((error) => console.warn("Failed to send acknowledgment:", error)); + } + + dispatch(successAction);Backend/Startup.cs (1)
259-261: Singleton tracker needs instance affinity in multi-node deployments.If you run multiple backend instances, ensure sticky sessions (or a backplane/distributed cache) so a client’s ack hits the same instance that created the requestId; otherwise retries may not clear correctly.
| public class CombineHub(IAcknowledgmentTracker ackTracker) : Hub | ||
| { | ||
| // Names for the `method` parameter of _notifyService.Clients.All.SendAsync() | ||
| public const string MethodFailure = "Failure"; | ||
| public const string MethodSuccess = "Success"; | ||
|
|
||
| private readonly IAcknowledgmentTracker _ackTracker = ackTracker; | ||
|
|
||
| /// <summary> | ||
| /// Client method to acknowledge receipt of a SignalR message. | ||
| /// This provides confirmation that messages were successfully delivered. | ||
| /// </summary> | ||
| /// <param name="requestId">Unique identifier for the request being acknowledged</param> | ||
| public void AcknowledgeMessage(string requestId) | ||
| { | ||
| _ackTracker.MarkAcknowledged(requestId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CombineHub.cs" | head -20Repository: sillsdev/TheCombine
Length of output: 93
🏁 Script executed:
find . -type f -name "MergeController.cs" | head -20Repository: sillsdev/TheCombine
Length of output: 103
🏁 Script executed:
wc -l ./Backend/Helper/CombineHub.cs ./Backend/Controllers/MergeController.csRepository: sillsdev/TheCombine
Length of output: 158
🏁 Script executed:
cat -n ./Backend/Helper/CombineHub.csRepository: sillsdev/TheCombine
Length of output: 1482
🏁 Script executed:
sed -n '140,170p' ./Backend/Controllers/MergeController.csRepository: sillsdev/TheCombine
Length of output: 1649
🏁 Script executed:
find . -type f -name "*.cs" | xargs rg "interface IAcknowledgmentTracker" -A 10Repository: sillsdev/TheCombine
Length of output: 1114
🏁 Script executed:
cat -n ./Backend/Interfaces/IAcknowledgmentTracker.csRepository: sillsdev/TheCombine
Length of output: 816
🏁 Script executed:
find . -type f -name "*.cs" | xargs rg "class.*IAcknowledgmentTracker" -A 30 | head -100Repository: sillsdev/TheCombine
Length of output: 4405
🏁 Script executed:
cat -n ./Backend/Services/AcknowledgmentTracker.csRepository: sillsdev/TheCombine
Length of output: 5150
🏁 Script executed:
rg "Context\.UserIdentifier" ./Backend --type cs | head -5Repository: sillsdev/TheCombine
Length of output: 45
🏁 Script executed:
rg "Context\." ./Backend/Helper --type cs -A 2Repository: sillsdev/TheCombine
Length of output: 45
🏁 Script executed:
rg "Hub" ./Backend --type cs | grep "class.*Hub" | head -10Repository: sillsdev/TheCombine
Length of output: 536
AcknowledgeMessage must validate the caller's identity against the intended recipient.
Any client can call AcknowledgeMessage(requestId) and mark a message as acknowledged, even if that message was intended for another user. Since requestId is broadcast via Clients.All, all connected clients know every requestId in the system. This allows a malicious or compromised client to acknowledge another user's messages and suppress retry attempts.
Modify AcknowledgeMessage to accept or infer the intended userId, store requestId → userId mappings in the tracker, and validate against Context.UserIdentifier before marking acknowledgment.
🤖 Prompt for AI Agents
In `@Backend/Helper/CombineHub.cs` around lines 6 - 22, Update AcknowledgeMessage
in CombineHub to validate that the caller is the intended recipient before
marking acknowledgment: change AcknowledgeMessage to either accept a userId
parameter or infer it and ensure IAcknowledgmentTracker (referenced as
_ackTracker) exposes methods to record the mapping and to validate ownership
(e.g., store requestId→userId when sending and add a tracker method like
TryMarkAcknowledged(requestId, userId) or GetOwner(requestId)); inside
AcknowledgeMessage, compare Context.UserIdentifier to the owner userId from the
tracker and only call _ackTracker.MarkAcknowledged(requestId) if they match,
otherwise ignore or log/deny the attempt. Ensure CombineHub uses
Context.UserIdentifier for the check and that the tracker stores the mapping
when you create/send messages so lookup succeeds.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 6 files and all commit messages.
Reviewable status: 6 of 14 files reviewed, 1 unresolved discussion.
Refactored: Eliminated Code Duplication in Retry Logic
Changes Made
Moved the duplicate
SendWithRetrymethod from bothMergeControllerandLiftControllerinto theAcknowledgmentTrackerservice asSendWithRetryAsync.Files Modified
Backend/Services/AcknowledgmentTracker.csSendWithRetryAsyncmethod that encapsulates the entire retry logicFunc<Task>) to send messages, making it reusableILoggerparameter for loggingBackend/Interfaces/IAcknowledgmentTracker.csSendWithRetryAsyncmethod signature to interfaceBackend/Controllers/MergeController.csSendWithRetrymethod (~45 lines)_ackTracker.SendWithRetryAsync()with lambda for sending messageBackend/Controllers/LiftController.csSendWithRetrymethod (~45 lines)_ackTracker.SendWithRetryAsync()with lambda for sending messageBenefits
✅ Eliminated Duplication: Removed ~90 lines of duplicate code across both controllers
✅ Single Responsibility: Retry logic now lives in one place (AcknowledgmentTracker)
✅ Easier Maintenance: Future changes to retry behavior only need to be made in one location
✅ More Testable: Retry logic can be unit tested independently
✅ Flexible Design: Callback pattern allows any SignalR message to use retry logic
✅ All Tests Pass: 10/10 MergeController tests pass
Implementation Details
The new
SendWithRetryAsyncmethod:Func<Task>callback for sending the message() => _notifyService.Clients.All.SendAsync(...)Testing
This refactoring improves code quality without changing functionality.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
This change is
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.