Fix integration tests breaking on SearchParameter duplicate-URL check on retry - #5749
Open
apurvabhaleMS wants to merge 34 commits into
Open
Fix integration tests breaking on SearchParameter duplicate-URL check on retry#5749apurvabhaleMS wants to merge 34 commits into
apurvabhaleMS wants to merge 34 commits into
Conversation
… SQL error 2627 ICM-833659983 / AB#198804 Root cause: TryGetValue + Remove on the shared HTTP request-context Properties dictionary were not atomic. Under parallel bundle processing, two concurrent threads could both read the same PendingSearchParameterStatus before either removed it, causing pendingStatuses = [URI, URI] in MergeAsync. This produced a duplicate row in the @searchParams TVP, violating the UNIQUE (Uri) constraint on dbo.SearchParamList and returning SQL error 2627 / HTTP 500. Fix: wrap TryGetValue + Remove in lock(properties) so only one thread can claim the pending status; subsequent threads see the key already removed. Tests added: - Unit: SetAndClearPendingSearchParameterStatus_WhenCalledConcurrently_Only OneResourceReceivesStatus (SqlServerFhirDataStoreUnitTests) — deterministic, proves race with Parallel.For, fails before fix / passes after. - E2E regression guard: GivenParallelTransactionBundleWithSearchParamAndPatients _WhenPosted_ShouldNotThrowUniqueKeyConstraint (ReindexTests) — probabilistic, runs in CI/EUAP.
- catch (Exception) instead of bare catch (CodeQL SA1025) - Assert bundle response NotNull + correct entry count before checking failures - Fail fast on reflection field-not-found instead of silent ?.SetValue - Remove DistinctBy from E2E XML doc comment (lock-only fix was applied) - Soften 'DETERMINISTIC' claim in unit test summary comment
…rallel coverage Finding 1 (medium): add code comment to SetAndClearPendingSearchParameterStatus documenting the pre-existing single-slot limitation — bundles with multiple SearchParameter entries can lose earlier statuses due to key overwrite before consume. Pre-existing behavior, tracked separately. Finding 2 (medium): update E2E test XML doc to explain why Transaction was chosen over Batch and confirm that both parallel paths are protected by the same lock fix via the shared SetAndClearPendingSearchParameterStatus code path. Finding 3 (low): no code change needed — the lock prevents duplicates regardless of URL length; the 128-char boundary is a TVP schema constraint unrelated to the concurrency fix.
…different resource A PUT with a new unique resource ID but an existing URL violates the 1-URL-per-resource invariant. Two resources sharing a URL causes SQL 2627 errors in bundle operations: DELETEs operate by resource ID and derive SearchParameter URLs at runtime — if two IDs share a URL, MergeAsync receives @searchParams TVP rows [URL, URL] → UNIQUE constraint violation. Fix: in CreateOrUpdateSearchParameterBehavior, when prevSearchParamResource is null (brand-new resource being PUT), reject if the URL is already registered in the SearchParameter definition manager. Note: this fix is complementary to the ICM-833659983 lock fix (concurrent SetAndClearPendingSearchParameterStatus race) committed earlier in this PR. With Bug 187119 fixed, duplicate URLs cannot enter the system via PUT, eliminating the DELETE-derived duplicate scenario. The race-condition lock remains as defence-in-depth for the parallel-bundle scenario.
…dingHardDelete) TryGetSearchParameter without excludePendingDelete returns ALL states including PendingHardDelete. The previous check would block legitimate recreation of a SearchParameter after hard-delete (the URL is still in the definition manager as PendingHardDelete until a reindex cleans it up). Only reject when the existing SP is in an active state (Supported, Disabled). Allow the PUT when the existing holder is PendingDelete or PendingHardDelete. This preserves the GivenBulkDeleteRequest_WhenSearchParametersDeleted test flow: HardDelete -> URL enters PendingHardDelete state -> PUT recreates the same SP.
PUT was already guarded. POST (CreateAsync) had no URL-uniqueness check, so posting a SearchParameter with a URL already owned by an active resource silently created a second resource, accumulating duplicate-URL pairs over repeated EUAP test runs. Root cause of the aaaa URL errors on EUAP: GivenAnExistingSearchParameter_WhenUpdatingWithUrlLongerThan128 PUTs a SearchParameter with Url = prefix + 88 * 'a' (128 chars, always the same) on every run but does not clean it up. After N runs, N resources share URL=aaaa. Any parallel delete bundle (e.g. ReindexTests.InitializeAsync DeleteResourcesAsync) derives URL=aaaa for each, hitting the TVP UNIQUE constraint (SQL 2627). Same guard as PUT: reject only when existing SP is in an active state (Supported / Disabled), allow when PendingDelete / PendingHardDelete.
…ated unit test The lock was added to prevent SQL 2627 caused by duplicate URIs in the @searchParams TVP. The actual root cause is Bug 187119: PUT/POST allowed creating a new SearchParameter resource with a URL already owned by a different active resource. With that fix in place, duplicate-URL resources can no longer accumulate, so the parallel-delete scenario that triggered the TVP violation cannot arise. Revert the lock and remove its unit test. The E2E regression guard in ReindexTests remains.
…rlLongerThan128 test Without cleanup, each test run left an active SearchParameter with URL=aaaa (128 chars, always the same) in the database. Accumulation over repeated EUAP runs meant N resources shared the same URL — the parallel delete bundles in ReindexTests.InitializeAsync would then fail with SQL 2627. Add a finally block to hard-delete the created SP so each run is self-contained.
…actor test helpers
…l form with char 'c'
… cleanup in URL-too-long test
…vert unrelated file changes
…; drop repeatChar/urlOverride
apurvabhaleMS
added
Enhancement-Test
apurvabhaleMS
marked this pull request as ready for review
August 21, 2026 20:52
Fernando Henrique Inocêncio Borba Ferreira (fhibf)
approved these changes
Aug 21, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5749 +/- ##
==========================================
+ Coverage 78.64% 78.68% +0.03%
==========================================
Files 1019 1019
Lines 37657 37657
Branches 5729 5729
==========================================
+ Hits 29617 29632 +15
+ Misses 6642 6623 -19
- Partials 1398 1402 +4 🚀 New features to boost your workflow:
|
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
3 participants
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
The duplicate SearchParameter URL check in
CreateOrUpdateSearchParameterBehavior(added in #5710) rejects a POST/PUT when an active resource already owns the requested URL. Because SearchParameter deletes are lazy — the resource staysIsDeleted=0indbo.Resourceuntil reindex clears the search index — several integration tests broke:FhirStorageTestscreates SearchParameters through the shared helperCreatePatientSearchParam, which uses a fixed URL (.../Patient-{name}-Integration-FhirStorageTests) and POSTs it (server-assigned Id). Its cleanup only removes the entry from the definition manager and status table — it never deletes the resource. So the resource lingers active and the next test (or rerun) with the same URL is rejected with:Related issues
Addresses AB205101
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)