Refactor MergeService from Singleton to Transient with IMemoryCache #4095
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughMergeService switched from singleton with in-process ConcurrentDictionary to transient using IMemoryCache. Constructor adds IMemoryCache as first parameter; potential-duplicates and merge counter moved to cache (1h expiry for duplicates, non-expiring counter). DI lifetime changed to Transient; tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MergeController
participant MergeService
participant IMemoryCache
participant Repo as Repositories
Client->>MergeController: POST /merge (store duplicates)
MergeController->>MergeService: StoreDups(userId, potentialDups)
MergeService->>IMemoryCache: Get/Increment merge counter
MergeService->>IMemoryCache: Set per-user dups (key:userId) with 1h expiry
MergeService->>Repo: (other merge-related repo calls)
MergeService-->>MergeController: return requestId
MergeController-->>Client: 200 OK
sequenceDiagram
actor Client
participant MergeController
participant MergeService
participant IMemoryCache
Client->>MergeController: GET /merge/{requestId} (retrieve dups)
MergeController->>MergeService: RetrieveDups(userId, requestId)
MergeService->>IMemoryCache: Get per-user dups (key:userId)
alt entry found
MergeService->>IMemoryCache: Remove per-user dups (delete key)
MergeService-->>MergeController: return stored dups
else not found
MergeService-->>MergeController: return null / not found
end
MergeController-->>Client: 200/404
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
- Changed MergeService lifetime from Singleton to Transient in Startup.cs - Added IMemoryCache dependency to MergeService constructor - Replaced _mergeCounter with cache-based counter (no expiration, NeverRemove priority) - Replaced _potentialDups ConcurrentDictionary with cache-based storage (1 hour expiration) - Added cache key constants: MergeCounterCacheKey and PotentialDupsCacheKeyPrefix - Updated all test files to inject IMemoryCache mock - Added 4 new tests for cache behavior validation - Removed unused System.Collections.Concurrent and System.Threading imports Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
- Wrapped counter get and increment in lock(_cache) to ensure thread-safe operation - Prevents race conditions when multiple threads try to increment counter simultaneously Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
- Added dedicated _counterLock object to avoid locking on shared IMemoryCache instance - Made StoreDups method thread-safe with lock to prevent race conditions - Both counter increment and StoreDups now use same lock for consistency Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
|
commit 69c593cbb5: commit 1235315614: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors MergeService from a singleton to a transient service to fix a service lifetime violation where the singleton was injecting transient repository dependencies. The refactoring replaces instance-level state management with IMemoryCache for storing merge counters and potential duplicate word lists.
- Changed service registration from
AddSingletontoAddTransient - Introduced
IMemoryCachedependency for state management - Replaced
ConcurrentDictionarywith cache-based storage using user-specific keys
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Backend/Startup.cs | Changed MergeService registration from Singleton to Transient |
| Backend/Services/MergeService.cs | Added IMemoryCache dependency, replaced instance fields with cache operations, introduced static counter and lock for thread safety |
| Backend.Tests/Services/MergeServiceTests.cs | Updated test setup to inject IMemoryCache, added new tests for cache behavior |
| Backend.Tests/Controllers/MergeControllerTests.cs | Updated test setup to inject IMemoryCache for MergeService instantiation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4095 +/- ##
===========================================
+ Coverage 74.57% 86.08% +11.50%
===========================================
Files 295 54 -241
Lines 10938 4785 -6153
Branches 1372 589 -783
===========================================
- Hits 8157 4119 -4038
+ Misses 2385 522 -1863
+ Partials 396 144 -252
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Backend/Services/MergeService.cs:
- Around line 63-72: RetrieveDups currently calls _cache.TryGetValue(cacheKey)
then _cache.Remove(cacheKey) without synchronization, which can remove a newer
entry written by StoreDups; fix by performing the TryGetValue + Remove under the
same lock used by StoreDups so the read-and-delete is atomic (wrap the body of
RetrieveDups in that lock), or alternatively make the delete conditional by
comparing the stored version tuple.Item1 to an expected version (only remove if
versions match) to avoid deleting newer entries; reference RetrieveDups,
StoreDups, _cache, cacheKey and value.Item1 when applying the change.
🧹 Nitpick comments (4)
Backend.Tests/Services/MergeServiceTests.cs (2)
15-39: Tighten test cache setup (avoid undisposed ServiceProvider; create MemoryCache directly).Current setup allocates a
ServiceProviderper test and doesn’t dispose it. For these unit tests, constructingMemoryCachedirectly is simpler and avoids lifecycle leakage.Proposed change
- private IMemoryCache _cache = null!; + private MemoryCache _cache = null!; [SetUp] public void Setup() { - // Set up MemoryCache - var services = new ServiceCollection(); - services.AddMemoryCache(); - var serviceProvider = services.BuildServiceProvider(); - _cache = serviceProvider.GetService<IMemoryCache>()!; + _cache = new MemoryCache(new MemoryCacheOptions()); _mergeBlacklistRepo = new MergeBlacklistRepositoryMock(); _mergeGraylistRepo = new MergeGraylistRepositoryMock(); _wordRepo = new WordRepositoryMock(); _wordService = new WordService(_wordRepo); _mergeService = new MergeService(_cache, _mergeBlacklistRepo, _mergeGraylistRepo, _wordRepo, _wordService); } + + [TearDown] + public void TearDown() + { + _cache.Dispose(); + }
501-561: New cache tests: good start, but “newer request wins” isn’t actually being asserted.
TestStoreDupsOnlyStoresNewerRequestsdoesn’t create competing requests or assert which one wins; it currently only verifies “dups can be retrieved”. Consider either renaming it to match behavior, or make it deterministic (e.g., synchronize two tasks to overlap and assert that only the latest request’s result remains retrievable).Backend.Tests/Controllers/MergeControllerTests.cs (1)
16-50: Dispose the test MemoryCache (and avoid building a ServiceProvider if you don’t need DI).The test creates an
IMemoryCachevia a builtServiceProviderbut doesn’t dispose either. Since the class already implementsIDisposable, simplest is to instantiateMemoryCachedirectly and dispose it inDispose().Proposed change
- private IMemoryCache _cache = null!; + private MemoryCache _cache = null!; public void Dispose() { _mergeController?.Dispose(); + _cache?.Dispose(); GC.SuppressFinalize(this); } [SetUp] public void Setup() { - // Set up MemoryCache - var services = new ServiceCollection(); - services.AddMemoryCache(); - var serviceProvider = services.BuildServiceProvider(); - _cache = serviceProvider.GetService<IMemoryCache>()!; + _cache = new MemoryCache(new MemoryCacheOptions()); _mergeBlacklistRepo = new MergeBlacklistRepositoryMock(); _mergeGraylistRepo = new MergeGraylistRepositoryMock(); _wordRepo = new WordRepositoryMock(); _wordService = new WordService(_wordRepo); _mergeService = new MergeService(_cache, _mergeBlacklistRepo, _mergeGraylistRepo, _wordRepo, _wordService); _mergeController = new MergeController( _mergeService, new HubContextMock<MergeHub>(), new PermissionServiceMock()); }Backend/Services/MergeService.cs (1)
41-58: StoreDups: expiration policy doesn’t match stated “sliding expiration” requirement.You’re using
AbsoluteExpirationRelativeToNow = 1h. If the intended behavior is “expire 1 hour after last access”, this should beSlidingExpiration.Proposed change (if sliding expiration is required)
- var cacheOptions = new MemoryCacheEntryOptions { AbsoluteExpirationRelativeToNow = TimeSpan.FromHours(1) }; + var cacheOptions = new MemoryCacheEntryOptions { SlidingExpiration = TimeSpan.FromHours(1) };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Backend.Tests/Controllers/MergeControllerTests.csBackend.Tests/Services/MergeServiceTests.csBackend/Services/MergeService.csBackend/Startup.cs
🧰 Additional context used
🧬 Code graph analysis (4)
Backend/Startup.cs (1)
Backend/Services/MergeService.cs (1)
MergeService(15-466)
Backend.Tests/Controllers/MergeControllerTests.cs (1)
Backend/Services/MergeService.cs (1)
MergeService(15-466)
Backend.Tests/Services/MergeServiceTests.cs (2)
Backend/Services/MergeService.cs (2)
MergeService(15-466)List(63-72)Backend/Interfaces/IMergeService.cs (1)
List(20-20)
Backend/Services/MergeService.cs (1)
Backend/Interfaces/IMergeService.cs (1)
List(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: docker_build
🔇 Additional comments (3)
Backend/Startup.cs (1)
259-263: DI lifetime change looks correct; confirm callers don’t rely on per-instance state.Switching
IMergeService/MergeServiceto transient resolves the “singleton capturing transients” issue. Just ensure any previously “sticky” in-memory state is now handled viaIMemoryCache/static as intended, since the service will be re-created frequently.Backend/Services/MergeService.cs (2)
15-33: Constructor/fields look fine for transient + shared cache; double-check intended “shared state” model.Given
MergeServiceis now transient, using shared state viaIMemoryCache(and static members) is consistent with the goal of not capturing transient repos while keeping cross-instance request coordination.
24-29: VerifyInterlocked.Increment(ref ulong)support for your target frameworks.If the repo targets frameworks where
Interlocked.Incrementdoesn’t supportulong, this won’t compile. A common fallback is alongcounter with cast toulong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Backend.Tests/Mocks/WordRepositoryMock.cs (1)
97-109: Potential race condition on_getFrontierCallCountincrement.The
++_getFrontierCallCount == 1check is not atomic. If two threads callGetFrontierconcurrently, both could read and increment simultaneously, potentially causing both to wait (or neither to wait depending on timing).If the intent is strictly "only first caller waits," consider using
Interlocked.Increment:if (Interlocked.Increment(ref _getFrontierCallCount) == 1)However, if the current behavior is acceptable for your test scenarios (since this is mock code), this may be fine as-is.
Backend.Tests/Controllers/MergeControllerTests.cs (1)
39-40: LGTM! Consider storing and disposing the cache.The constructor call correctly aligns with the new
MergeServicesignature. For completeness, you could store theMemoryCachein a field and dispose it inDispose(), though for test code this is a minor concern.♻️ Optional: Track cache for disposal
+ private IMemoryCache _cache = null!; private IMergeBlacklistRepository _mergeBlacklistRepo = null!; // ... public void Setup() { + _cache = new MemoryCache(new MemoryCacheOptions()); _mergeBlacklistRepo = new MergeBlacklistRepositoryMock(); // ... - _mergeService = new MergeService(new MemoryCache(new MemoryCacheOptions()), _mergeBlacklistRepo, + _mergeService = new MergeService(_cache, _mergeBlacklistRepo, _mergeGraylistRepo, _wordRepo, _wordService); } public void Dispose() { _mergeController?.Dispose(); + (_cache as IDisposable)?.Dispose(); GC.SuppressFinalize(this); }Backend/Services/MergeService.cs (1)
24-28: Static lock for transient service is correct but may cause contention under load.The static
_cacheLockensures thread safety across all transient instances, which is necessary. However, since all users serialize on this single lock, it could become a bottleneck under high concurrent load.If performance becomes an issue, consider per-user locking (e.g.,
ConcurrentDictionary<string, object>keyed by userId) to reduce contention.♻️ Optional: Per-user locking for reduced contention
private static readonly ConcurrentDictionary<string, object> _userLocks = new(); // In StoreDups/RetrieveDups: var userLock = _userLocks.GetOrAdd(userId, _ => new object()); lock (userLock) { // cache operations }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Backend.Tests/Controllers/MergeControllerTests.csBackend.Tests/Mocks/WordRepositoryMock.csBackend.Tests/Services/MergeServiceTests.csBackend/Services/MergeService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Backend.Tests/Services/MergeServiceTests.cs
🧰 Additional context used
🧬 Code graph analysis (3)
Backend.Tests/Mocks/WordRepositoryMock.cs (2)
Backend/Models/Word.cs (3)
Word(10-177)Word(87-107)Word(110-132)Backend/Interfaces/IWordRepository.cs (16)
Task(9-9)Task(10-10)Task(11-11)Task(12-12)Task(13-13)Task(14-14)Task(15-15)Task(16-16)Task(17-17)Task(18-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)
Backend/Services/MergeService.cs (1)
Backend/Interfaces/IMergeService.cs (1)
List(20-20)
Backend.Tests/Controllers/MergeControllerTests.cs (1)
Backend/Services/MergeService.cs (1)
MergeService(15-470)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: test_build (8.0.x)
- GitHub Check: docker_build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
Backend.Tests/Mocks/WordRepositoryMock.cs (2)
12-13: LGTM!Clean use of C# 12 collection expressions for list initialization.
15-26: LGTM!Good addition for simulating asynchronous delays in tests. The XML documentation clearly explains the intended behavior.
Backend.Tests/Controllers/MergeControllerTests.cs (1)
10-10: LGTM!Required import for the new
MemoryCacheusage.Backend/Services/MergeService.cs (5)
15-22: LGTM! Good use of primary constructor with explicit readonly fields.The C# 12 primary constructor pattern combined with explicit readonly field declarations is a clean approach - it provides clarity and prevents accidental mutation of injected dependencies.
30-33: LGTM!Cache key prefix and otel tag constants are well-defined and follow clear naming conventions.
41-57: LGTM! Cache-based storage with proper thread safety.The logic correctly handles:
- Initial null storage (request started marker)
- Subsequent update with actual duplicates (same counter)
- Newer request preemption (higher counter)
- 1-hour expiration as per requirements
63-76: LGTM! One-time retrieval pattern correctly implemented.The remove-after-read pattern ensures each duplicate set is consumed only once, which matches the expected workflow.
Minor nit: The cache key construction could be moved before the lock since it's a pure string operation, reducing time spent holding the lock.
423-436: LGTM! Thread-safe counter increment with proper request preemption.The combination of
Interlocked.Incrementfor atomic counter updates and theStoreDupslogic correctly implements the "newest request wins" pattern. If a newer request starts while work is in progress, the current request gracefully yields.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 3 files and all commit messages, and made 5 comments.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec).
Backend.Tests/Mocks/WordRepositoryMock.cs line 16 at r4 (raw file):
private readonly List<Word> _frontier = []; private TaskCompletionSource<bool>? _getFrontierDelay;
This implementation is amusing. It isn't really quite using TaskCompletionSource as designed.
I suspect that you could skip the TaskCompletionSource and directly provide the Task you put in the TaskCompletionSource to the same effect.
Backend.Tests/Services/MergeServiceTests.cs line 506 at r4 (raw file):
Assert.That(await _mergeService.GetAndStorePotentialDuplicates(ProjId, 5, 5, userId1), Is.True); Assert.That(await _mergeService.GetAndStorePotentialDuplicates(ProjId, 5, 5, userId2), Is.True); Assert.That(await _mergeService.GetAndStorePotentialDuplicates(ProjId, 5, 5, userId1), Is.True);
Not sure this assertion belongs in this unit
Backend.Tests/Services/MergeServiceTests.cs line 513 at r4 (raw file):
[Test] public async Task TestGetAndStorePotentialDuplicatesSecondCallWins()
A comment here explaining how the results confirm the desired result would be good for future maintenance.
Backend.Tests/Services/MergeServiceTests.cs line 530 at r4 (raw file):
// Release and finish the first call delaySignal.SetResult(true); Assert.That(await firstCallTask, Is.False);
To use the TaskCompletionSource as designed I think this would not await the task, but instead look for the delaySignal.Result or something similar.
Backend.Tests/Controllers/MergeControllerTests.cs line 39 at r4 (raw file):
_wordRepo = new WordRepositoryMock(); _wordService = new WordService(_wordRepo); _mergeService = new MergeService(new MemoryCache(new MemoryCacheOptions()), _mergeBlacklistRepo,
This seems suspicious to me. I would expect the MemoryCache to be garbage collected after the MergeService completed based on this code alone.
What I would have expected is that you would have an AddSingleton which provided the cache for use by the service.
Code quote:
new MemoryCache(new MemoryCacheOptions())
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 5 comments.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @jasonleenaylor).
Backend.Tests/Controllers/MergeControllerTests.cs line 39 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
This seems suspicious to me. I would expect the MemoryCache to be garbage collected after the MergeService completed based on this code alone.
What I would have expected is that you would have an AddSingleton which provided the cache for use by the service.
Your expectation matches Copilot's original code. CodeRabbit suggested this simplification, which Claude assures is valid, but also says something like
_cache = new ServiceCollection().AddMemoryCache().BuildServiceProvider().GetRequiredService<IMemoryCache>();
"is more aligned with best practices".
Backend.Tests/Mocks/WordRepositoryMock.cs line 16 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
This implementation is amusing. It isn't really quite using TaskCompletionSource as designed.
I suspect that you could skip the TaskCompletionSource and directly provide the Task you put in the TaskCompletionSource to the same effect.
Done
Backend.Tests/Services/MergeServiceTests.cs line 506 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
Not sure this assertion belongs in this unit
Updates test name and added a few more. Or is that just very unnecessary?
Backend.Tests/Services/MergeServiceTests.cs line 513 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
A comment here explaining how the results confirm the desired result would be good for future maintenance.
Done
Backend.Tests/Services/MergeServiceTests.cs line 530 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
To use the TaskCompletionSource as designed I think this would not await the task, but instead look for the delaySignal.Result or something similar.
I don't think I get what yours suggesting.
|
Previously, imnasnainaec (D. Ror.) wrote…
It doesn't seem especially necessary to me, but it isn't harmful. My gut says it has little to no effect on the code coverage (what lines does it catch that the other tests don't?) |
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @copilot[bot]).
Backend.Tests/Services/MergeServiceTests.cs line 530 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I don't think I get what yours suggesting.
If you look at the example code here it might clarify, but it doesn't matter with the current implementation.
https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskcompletionsource
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 2 comments and resolved 1 discussion.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @jasonleenaylor).
Backend.Tests/Controllers/MergeControllerTests.cs line 39 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Your expectation matches Copilot's original code. CodeRabbit suggested this simplification, which Claude assures is valid, but also says something like
_cache = new ServiceCollection().AddMemoryCache().BuildServiceProvider().GetRequiredService<IMemoryCache>();"is more aligned with best practices".
Done
Backend.Tests/Services/MergeServiceTests.cs line 506 at r4 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
It doesn't seem especially necessary to me, but it isn't harmful. My gut says it has little to no effect on the code coverage (what lines does it catch that the other tests don't?)
No extra line coverage that I can see. The intent was scenario coverage.
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @copilot[bot]).
MergeServicewas registered as a Singleton but injected transient repositories (IMergeBlacklistRepository,IMergeGraylistRepository,IWordRepository), violating service lifetime best practices and capturing transient dependencies.Changes
Service Registration
MergeServicefromAddSingletontoAddTransientinStartup.csState Management
_mergeCounterstatic_potentialDups(ConcurrentDictionary) withIMemoryCache"MergeService_PotentialDups_{userId}"with 1-hour expirationThread Safety
_counterLockobject forStoreDupsoperationsConstructor
Tests
IMemoryCacheviaServiceCollectionWordRepositoryMockto enable delayingGetFrontierin testsOriginal prompt
This section details on the original issue you should resolve
<issue_title>Fix singleton MergeService's dependency on transient repositories</issue_title>
<issue_description>
MergeServiceis intentionally a Singleton inBackend/Startup.cs:However, it depends on two Repositories that are (and should be) transient. In
Backend/Services/MergeService.cs:Plan:
MergeServicelifetime from Singleton to TransientMergeServicedependencies:public MergeService(IMemoryCache cache, ..._mergeCounterand_potentialDupswith calls to the cache</issue_description>
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
This change is
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.