LNK-4879: Fix retry ready to acquire

[edward-miller-lcg]

🛠️ Description of Changes
Fixed an issue where the RetryAttempts counter was not being incremented in several failure scenarios, preventing logs from ever reaching the MaxRetriesReached state.

Modified:

DataAcquisition.Domain.Application.Services.PatientDataService:
Incremented RetryAttempts in the catch blocks for ProcessingDelayException, TooManyRequestsException (429), and general Exception.
DataAcquisition.AcquisitionWorker.Services.AcquisitionProcessorBackgroundService:
Incremented RetryAttempts when processing a work item fails.
These changes ensure that the AcquisitionProcessingJob can accurately identify logs that have exceeded the maximum number of retries (5) and move them to the MaxRetriesReached status.

🧪 Testing Performed
Updated integration tests in PatientDataServiceTests.cs to verify that RetryAttempts is incremented to 1 (from 0) when a 429 error occurs.
Executed dotnet test DotNet\ServiceTests --filter "FullyQualifiedName~PatientDataServiceTests" and verified that all 15 tests passed.
Local 251 patient test that had OperationOutcomes as well as I changed the config to a bad EHR url that simulated a retry scenario:

🧑‍🔬 Unit Testing
Coverage: 0.0%

I have updated unit tests to cover my changes.

📓 Documentation Updated
N/A

Summary by CodeRabbit

Bug Fixes

• Enhanced error handling and retry tracking for patient data acquisition requests. The system now properly increments retry attempt counters and updates status transitions when errors occur, including handling of throttling delays and processing exceptions. This provides more accurate tracking of failed request attempts.

[coderabbitai]

📝 Walkthrough

Walkthrough

Updates exception handling in PatientDataService to increment RetryAttempts before retrying or updating status across multiple error paths. Status transitions adjusted from Pending to Failed in certain scenarios. Corresponding test expectations updated to reflect the incremented retry count in throttling scenarios.

Changes

Cohort / File(s)	Summary
Exception Handling & Retry Logic DotNet/DataAcquisition.Domain/Application/Services/PatientDataService.cs	Modified exception handling paths in ExecuteLogRequest to increment RetryAttempts before retrying or updating status; adjusted status transitions from Pending to Failed in ProcessingDelayException and generic Exception handlers; OpOutcomeException now increments RetryAttempts and logs retry note.
Test Expectations DotNet/ServiceTests/IntegrationTests/DataAcquisition/PatientDataServiceTests.cs	Updated unit test assertions across three 429 throttling scenarios to expect RetryAttempts of 1 instead of 0 after rescheduling logic; cases include seconds delay, date-based delay, and invalid negative header with default delay.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• LNK-4879: fix retry incrementing #1481: Directly implements the same code-level change to increment RetryAttempts in PatientDataService exception paths with corresponding test updates.
• LNK-4430: Data Acquisition - Handle 429 Status #1378: Modifies 429/throttling retry behavior in PatientDataService by incrementing RetryAttempts and adjusting reschedule logic.
• LNK-4630: Reduce AcquisitionProcessingJob.MaxRetry Attempts - Release Branch Only #1329: Modifies retry handling in the Data Acquisition flow, changing PatientDataService's RetryAttempts behavior and introducing DataAcquisitionLog.MaxRetryAttempts configuration.

Suggested reviewers

• nvmLantana
• smailliwcs
• amphillipsLGC

Poem

🐰 Retries now count, with each stumble and fall,
Status adjusts when exceptions call,
From Pending to Failed, the logic takes flight,
Tests hop along to match efforts just right! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'LNK-4879: Fix retry ready to acquire' is directly related to the main objective of the PR, which is to fix retry attempt incrementing in failure scenarios. However, the phrase 'ready to acquire' is somewhat unclear and doesn't fully convey the specific change (incrementing RetryAttempts counter).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-retry-ready-to-acquire

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

DotNet/ServiceTests/IntegrationTests/DataAcquisition/PatientDataServiceTests.cs (1)

449-520: Consider adding explicit RetryAttempts assertion for OpOutcomeException tests.

The ExecuteLogRequest_HandlesOpOutcomeException_500_SetsPendingStatus test verifies the status transition to Failed but doesn't assert that RetryAttempts == 1. Given the PR's focus on retry tracking, adding this assertion would strengthen test coverage and prevent regression.

💡 Suggested addition

         // Assert
         Assert.NotNull(updatedModel);
         Assert.Equal(RequestStatus.Failed, updatedModel.Status);
+        Assert.Equal(1, updatedModel.RetryAttempts);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/PatientDataServiceTests.cs`
around lines 449 - 520, The test
ExecuteLogRequest_HandlesOpOutcomeException_500_SetsPendingStatus currently
asserts the updatedModel.Status but omits verifying retry count; update the test
to also assert that updatedModel.RetryAttempts == 1 (or the expected value)
after the service call. Locate the UpdateDataAcquisitionLogModel captured in the
updatedModel callback (set in the mock for manager.UpdateAsync) and add a simple
Assert.Equal(expectedRetryCount, updatedModel.RetryAttempts) immediately after
the existing Assert.Equal(RequestStatus.Failed, updatedModel.Status) to ensure
retry tracking is validated for the OpOutcomeException path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@DotNet/DataAcquisition.Domain/Application/Services/PatientDataService.cs`:
- Around line 697-701: The log is currently writing raw exception text into
DataAcquisitionLog.Notes (see the log.Notes.Add call in
PatientDataService.ExecuteLogRequest), which may expose sensitive data; create
and use a sanitizer/redactor (e.g., SanitizeExceptionMessage or
RedactSensitiveInfo) to clean ex.Message and ex.InnerException?.Message (and
continue to sanitize FacilityId) before concatenation, ensure the sanitizer
strips URLs/tokens/PII and truncates length, and replace the existing direct use
of ex.Message and ex.InnerException?.Message in the log.Notes.Add call with the
sanitized outputs.

---

Nitpick comments:
In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/PatientDataServiceTests.cs`:
- Around line 449-520: The test
ExecuteLogRequest_HandlesOpOutcomeException_500_SetsPendingStatus currently
asserts the updatedModel.Status but omits verifying retry count; update the test
to also assert that updatedModel.RetryAttempts == 1 (or the expected value)
after the service call. Locate the UpdateDataAcquisitionLogModel captured in the
updatedModel callback (set in the mock for manager.UpdateAsync) and add a simple
Assert.Equal(expectedRetryCount, updatedModel.RetryAttempts) immediately after
the existing Assert.Equal(RequestStatus.Failed, updatedModel.Status) to ensure
retry tracking is validated for the OpOutcomeException path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 52144f82-2200-4152-855f-bf3ced9a3b7d

📥 Commits

Reviewing files that changed from the base of the PR and between 3042640 and 14dc49c.

📒 Files selected for processing (3)

• DotNet/DataAcquisition.Domain/Application/Services/PatientDataService.cs
• DotNet/ServiceTests/IntegrationTests/DataAcquisition/PatientDataServiceTests.cs
• tests_output.txt

[smailliwcs]

Recommend removing the comment since we are now counting deferral as a failure.