Conversation
…ed procedure - Added new schema version V104 to SchemaVersion.cs and updated SchemaVersionConstants.cs to reflect the new maximum version. - Enhanced the MergeSearchParams stored procedure to include additional parameters for resource change capture and transaction handling. - Updated SqlServerSearchParameterStatusDataStore to handle new parameters based on schema version. - Modified SqlServerFhirDataStore to conditionally use MergeSearchParams based on pending search parameter updates. - Updated project file to set LatestSchemaVersion to 104. - Refactored integration tests to accommodate changes in search parameter handling and ensure proper synchronization.
…s and improved URL resolution
…and remove unused methods
…ns and data store
…ency control and ensure proper transaction management
…n SqlServerFhirDataStore to differentiate between transaction and non-transaction operations
…enhancing transaction handling and cache management
- Added new schema version V108 to SchemaVersion.cs. - Updated SchemaVersionConstants.cs to set Max schema version to 108. - Modified SqlServerSearchParameterStatusDataStore.cs to check for schema version 108 for enabling resource change capture. - Updated project file to reflect the latest schema version as 108.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
....Fhir.Shared.Core.UnitTests/Features/Search/SearchParameters/SearchParameterBehaviorTests.cs
Dismissed
Show dismissed
Hide dismissed
....Fhir.Shared.Core.UnitTests/Features/Search/SearchParameters/SearchParameterBehaviorTests.cs
Dismissed
Show dismissed
Hide dismissed
....Fhir.Shared.Core.UnitTests/Features/Search/SearchParameters/SearchParameterBehaviorTests.cs
Dismissed
Show dismissed
Hide dismissed
....Fhir.Shared.Core.UnitTests/Features/Search/SearchParameters/SearchParameterBehaviorTests.cs
Dismissed
Show dismissed
Hide dismissed
…d search parameter operations
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5433 +/- ##
=======================================
Coverage ? 77.24%
=======================================
Files ? 977
Lines ? 35904
Branches ? 5427
=======================================
Hits ? 27733
Misses ? 6813
Partials ? 1358 🚀 New features to boost your workflow:
|
…trada/atomicsearchparameteroperations
jestradaMS
changed the title
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Failed to log WaitForAllInstancesCacheConsistencyAsync event."); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
In general, to fix overly broad generic catch clauses, restrict the caught types to those you actually expect and can handle meaningfully. For asynchronous operations, a common refinement is to avoid catching OperationCanceledException (and sometimes TaskCanceledException) so that cooperative cancellation still works as intended. For background or logging helpers, it is still acceptable to catch “regular” exceptions while letting fatal exceptions propagate.
For this specific case in TryLogConsistencyWaitEventAsync (lines 203–214 in src/Microsoft.Health.Fhir.Core/Features/Search/Parameters/SearchParameterOperations.cs), the best fix is:
- Add a dedicated catch block for
OperationCanceledExceptionthat immediately rethrows, ensuring that cancellation flows correctly. - Optionally, add a catch for
TaskCanceledExceptionifTryLogEventcan throw it separately. - Keep a generic
catch (Exception ex)after those specific catches, to preserve the existing behavior of swallowing/logging non-cancellation errors.
This preserves all current functionality (logging failures still only produce a warning and do not affect callers) while no longer intercepting cancellation exceptions. All required types (OperationCanceledException, TaskCanceledException, Exception) are in System, which is already imported, so no new imports are needed. The only code to change is within the body of TryLogConsistencyWaitEventAsync.
| @@ -207,6 +207,11 @@ | ||
| using IScoped<ISearchService> searchService = _searchServiceFactory(); | ||
| await searchService.Value.TryLogEvent(nameof(WaitForAllInstancesCacheConsistencyAsync), status, text, startDate, cancellationToken); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Preserve cooperative cancellation semantics. | ||
| throw; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Failed to log WaitForAllInstancesCacheConsistencyAsync event."); |
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs
Fixed
Show fixed
Hide fixed
| if (syncEventDate.HasValue && !string.IsNullOrEmpty(eventText)) | ||
| { | ||
| if (!latestSyncByHost.TryGetValue(hostName, out var existingSync) | ||
| || syncEventDate.Value > existingSync.SyncEventDate) | ||
| { | ||
| latestSyncByHost[hostName] = (syncEventDate.Value, eventText); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Nested 'if' statements can be combined Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
In general, to fix this kind of issue you replace the pattern:
if (cond1)
{
if (cond2)
{
// body
}
}with:
if (cond1 && cond2)
{
// body
}provided there are no else branches and no side effects in the conditions.
For this specific case in SqlServerSearchParameterStatusDataStore.cs, the outer if at line 292 checks syncEventDate.HasValue && !string.IsNullOrEmpty(eventText) and contains only an inner if at line 294 that checks whether the dictionary already has a value or the new date is more recent. We can merge these into a single if by logically AND-ing all three conditions:
if (syncEventDate.HasValue
&& !string.IsNullOrEmpty(eventText)
&& (!latestSyncByHost.TryGetValue(hostName, out var existingSync)
|| syncEventDate.Value > existingSync.SyncEventDate))
{
latestSyncByHost[hostName] = (syncEventDate.Value, eventText);
}This preserves short-circuit behavior and ensures that syncEventDate.Value is only accessed when syncEventDate.HasValue is true, just as before. No new imports or methods are required; the change is localized to the foreach loop body around lines 288–299.
| @@ -289,13 +289,12 @@ | ||
| { | ||
| activeHosts.Add(hostName); | ||
|
|
||
| if (syncEventDate.HasValue && !string.IsNullOrEmpty(eventText)) | ||
| if (syncEventDate.HasValue | ||
| && !string.IsNullOrEmpty(eventText) | ||
| && (!latestSyncByHost.TryGetValue(hostName, out var existingSync) | ||
| || syncEventDate.Value > existingSync.SyncEventDate)) | ||
| { | ||
| if (!latestSyncByHost.TryGetValue(hostName, out var existingSync) | ||
| || syncEventDate.Value > existingSync.SyncEventDate) | ||
| { | ||
| latestSyncByHost[hostName] = (syncEventDate.Value, eventText); | ||
| } | ||
| latestSyncByHost[hostName] = (syncEventDate.Value, eventText); | ||
| } | ||
| } | ||
|
|
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs
Fixed
Show fixed
Hide fixed
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // stale search parameter caches and would write resources with wrong hashes. | ||
| // Use the same lookback as active host detection so we do not miss qualifying | ||
| // refresh events that occurred shortly before this instance entered the wait. | ||
| var activeHostsSince = DateTime.UtcNow.AddSeconds(-20 * _searchParameterCacheRefreshIntervalSeconds); |
Check failure
Code scanning / CodeQL
Possible loss of precision Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, to avoid integer overflow when using arithmetic results as floating-point values, ensure at least one operand in the arithmetic expression is of a floating-point type (e.g., double). That forces the operation to be done in floating-point, preventing 32-bit integer overflow and matching the semantics of APIs like DateTime.AddSeconds(double).
For this specific issue, we should change the computation of activeHostsSince so that the multiplication is done in double instead of int. The only place we need to modify is the expression passed to AddSeconds on line 213: -20 * _searchParameterCacheRefreshIntervalSeconds. We can do this by making 20 a double literal (20.0) or casting one operand to double (e.g., (double)_searchParameterCacheRefreshIntervalSeconds). This preserves intent (20 * intervalSeconds seconds in the past), avoids integer overflow, and does not change behavior for sane configuration values.
Concretely, in src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs, in the method containing lines 213–215, we should replace:
var activeHostsSince = DateTime.UtcNow.AddSeconds(-20 * _searchParameterCacheRefreshIntervalSeconds);with:
var activeHostsSince = DateTime.UtcNow.AddSeconds(-20.0 * _searchParameterCacheRefreshIntervalSeconds);No new methods, imports, or definitions are needed; this is a pure expression-level change.
| @@ -210,7 +210,7 @@ | ||
| // stale search parameter caches and would write resources with wrong hashes. | ||
| // Use the same lookback as active host detection so we do not miss qualifying | ||
| // refresh events that occurred shortly before this instance entered the wait. | ||
| var activeHostsSince = DateTime.UtcNow.AddSeconds(-20 * _searchParameterCacheRefreshIntervalSeconds); | ||
| var activeHostsSince = DateTime.UtcNow.AddSeconds(-20.0 * _searchParameterCacheRefreshIntervalSeconds); | ||
| var syncStartDate = activeHostsSince; | ||
| await _searchParameterOperations.WaitForAllInstancesCacheConsistencyAsync(syncStartDate, activeHostsSince, _cancellationToken); | ||
| } |
…ition for SearchParameter
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
1 similar comment
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
…ck and commit logic based on feedback from Sergey and recent chagnes to bundles
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| cmd.Parameters.AddWithValue("@IsResourceChangeCaptureEnabled", false); | ||
| cmd.Parameters.Add(new SqlParameter("@TransactionId", SqlDbType.BigInt) { Value = DBNull.Value }); | ||
|
|
||
| new ResourceListTableValuedParameterDefinition("@Resources").AddParameter(cmd.Parameters, Array.Empty<ResourceListRow>()); |
There was a problem hiding this comment.
All entries with empty lists are not needed. Removed in #5485
| } | ||
| } | ||
|
|
||
| public async Task<CacheConsistencyResult> CheckCacheConsistencyAsync(string targetSearchParamLastUpdated, DateTime syncStartDate, DateTime activeHostsSince, CancellationToken cancellationToken) |
4 participants
Description
This PR makes SearchParameter CRUD operations fully atomic and more reliable, especially for SQL Server-backed FHIR servers. It removes partial commit risks, reduces lock contention in transaction bundles, and makes the background refresh service the only owner of cache updates to prevent cache inconsistency.
It also improves diagnostics by adding clearer lifecycle logging, adds a dedicated retry policy for reindex operations to better handle transient SQL and Cosmos DB failures, and expands test coverage to verify that status updates no longer mutate the in-memory cache or publish mediator notifications.
Related issues
Addresses AB#183136.
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)