Fix lock contention

[bob2681312]

Description

Why is this change being made?

• To improve performance of the library, so that it can process retrieving many secrets concurrently

What is changing?

• Changed locking from a global lock to a per-secret lock
• Add locking when getting secrets from cache to resolve concurrency issues.
• Changed TrimCacheToSizeLimit function in SecretCacheItem.cs to reference correct constant
• In ISecretsManagerCache, convert GetCachedSecret method to an async method to be able to do proper locking.

Breaking: ISecretsManagerCache.GetCachedSecret signature changed from SecretCacheItem to Task<SecretCacheItem>. Implementers of the interface must update.

---

Testing

How was this tested?

dotnet test -f net8.0

Note: Some tests could not be run locally as they require running on Windows.

---

Reviewee Checklist

Update the checklist after submitting the PR

• I have reviewed, tested and understand all changes
  If not, why:
• I have filled out the Description and Testing sections above
  If not, why:
• Build and Unit tests are passing
  If not, why:
• Unit test coverage check is passing
  If not, why:
• Integration tests pass locally
  If not, why:
• I have updated integration tests (if needed)
  If not, why:
• I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
  If not, why:
• I have added explanatory comments for complex logic, new classes/methods and new tests
  If not, why:
• I have updated README/documentation (if needed)
  If not, why:
• I have clearly called out breaking changes (if any)
  If not, why:

---

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

• Reviewee checklist has been accurately filled out
• Code changes align with stated purpose in description
• Test coverage adequately validates the changes

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[secrets-manager-ai-code-review]

Claude finished @bob2681312's task in 4m 49s —— View job

---

Review: Fix lock contention

• Read changed files and prior review history
• Analyze correctness of the locking changes
• Attempt build/tests (see note)
• Post review feedback

I re-reviewed the current state of the branch against the long thread of prior feedback. The substantive concurrency bugs raised earlier have all been addressed — this is now in good shape. A few minor items remain.

Note: I couldn't run dotnet build/dotnet test in this environment (sandbox permission denied). The analysis below is from reading the code. CI (dotnet.yml, Windows) is the source of truth for test results.

---

✅ Confirmed fixes since earlier reviews

• Get-or-create race (SecretsManagerCache.GetCachedSecret): now double-checked — a lock-free cache.Get fast path, then cacheLock.WaitAsync outside the try, GetOrCreate + Compact inside the lock, Release() in finally. This closes the multi-instance race and keeps cache hits off the lock (addresses @reyhankoyun's hot-path concern).
• Un-awaited WaitAsync in GetVersion: now awaited, wrapped in try/finally, acquire outside the try. SemaphoreFullException/no-exclusion bug resolved.
• versions.Count check + TrimCacheToSizeLimit(): moved inside versionsLock, matching the item-cache pattern.
• cancellationToken propagation: now threaded through GetSecretString/GetSecretBinary/RefreshNowAsync → GetCachedSecret, and GetVersion(result, cancellationToken).
• refreshNeeded = true under objLock in RefreshNowAsync, and MAX_VERSIONS_CACHE_SIZE constant fix in TrimCacheToSizeLimit.

I also verified refreshNeeded is now read/written exclusively under objLock (RefreshAsync, RefreshNowAsync, IsRefreshNeeded), so its lack of volatile is no longer a visibility concern. GetVersion runs outside GetSecretValue's objLock (called after the finally at SecretCacheObject.cs:220), so reusing a separate versionsLock is self-deadlock-safe.

The two deterministic tests (ConcurrentGetSecretStringOnlyRefreshesOnce → describeCallCount == 1, ConcurrentRefreshNowDoesNotCorruptState → refreshCount == 11) are now genuinely deterministic given the cacheLock/objLock serialization, not just usually-green.

---

🟡 Minor — remaining items

1. The [Trait("Category","Concurrency")] tests still run on every CI build.
Adding the trait was the right first step, but .github/workflows/dotnet.yml runs dotnet test with no filter, so nothing actually excludes the concurrency tests. To honor the earlier "gate them out of the default run" request, the test step would need e.g. --filter "Category!=Concurrency" (and a separate opt-in job to run them). Since they're now deterministic this is low-urgency, but as written the trait has no effect on CI.
(I can't modify files under .github/workflows/ — GitHub App permissions block workflow edits — so this one is for a maintainer.)

2. SemaphoreSlim / nested MemoryCache disposal (follow-up, acknowledged out-of-scope).
SecretsManagerCache.Dispose() disposes cache but not cacheLock; SecretCacheItem never disposes its versions MemoryCache, versionsLock, or the base objLock, and MemoryCache doesn't dispose evicted values. Under churn beyond MaxCacheSize this leaks. You marked this out-of-scope and I agree it needn't block the PR — worth a tracking issue since the nested MemoryCache makes it more material than a bare semaphore.

3. Doc nit — SecretsManagerCache.cs:24 and ISecretsManagerCache.cs:22 both have the pre-existing typo "clide-side" (→ "client-side"). Not introduced here; trivial to fix while nearby.

---

Verdict

The core fix — per-secret SemaphoreSlim instead of a global lock, plus correctly-serialized cold-path creation — is sound and the earlier correctness blockers are resolved. Remaining items are minor/non-blocking. The breaking change to ISecretsManagerCache.GetCachedSecret (now Task<SecretCacheItem>) is correctly called out in the PR body; make sure it lands with an appropriate version bump.