fix(cache): forward the token in multi-key RemoveAsync, clear the CI build warnings - #163
Merged
Merged
Conversation
cosmin-staicu
requested review from
alinahornet,
cosminvlad,
litheon,
lucianaparaschivei and
razvalex
as code owners
September 9, 2026 11:56
There was a problem hiding this comment.
🟢 Approval recommended
The token-forwarding fix is correct and the warning cleanups make cancellation/termination behavior explicit without changing the intended shutdown semantics.
Pull request overview
This PR fixes a cancellation-token propagation defect in MultilayerCache.RemoveAsync(CacheKey[], token) and removes recurring CI analyzer warnings by making cancellation/termination behavior explicit in background loops and updating test fakes to a non-obsolete StackExchange.Redis exception constructor.
Changes:
- Forward the provided
CancellationTokenwhen building entry options for multi-keyRemoveAsync. - Make periodic-timer loops explicitly opt out of token propagation (
CancellationToken.None) to ensure clean shutdown via timer disposal. - Update tests and test project settings to avoid obsolete APIs / experimental warnings, and document the change in the changelog.
File summaries
| File | Description |
|---|---|
| tests/UiPath.Caching.Tests/UiPath.Caching.Tests.csproj | Suppresses SER007 for test-only usage of experimental RedisErrorKind needed by the non-obsolete exception ctor. |
| tests/UiPath.Caching.Tests/Broadcast/RedisStreamSubjectWriterTests.cs | Updates test fake to use non-obsolete RedisServerException constructor. |
| tests/UiPath.Caching.Tests/Broadcast/RedisStreamsTopicTests.cs | Updates test fakes to use non-obsolete RedisServerException constructor. |
| src/UiPath.Caching/Redis/RedisPlannedMaintenance.cs | Forces probe task delegate to always run by passing CancellationToken.None to Task.Run, with rationale comment. |
| src/UiPath.Caching/MultilayerCache.cs | Fixes defect: forwards cancellation token when building multi-key remove options. |
| src/UiPath.Caching/CacheMemoryMonitor.cs | Makes periodic timer wait explicitly non-cancelable to avoid OCE; relies on timer disposal to terminate. |
| src/UiPath.Caching/Broadcast/Redis/RedisStreamHealthMaintainer.cs | Same explicit non-cancelable waits for timer/semaphore to avoid OCE and ensure shutdown by disposal. |
| CHANGELOG.md | Documents the bug fix and the warning-cleanup rationale. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…build warnings CI annotated eight analyzer warnings on every pull request, none of them owned by the branch that surfaced them. One was a real defect. - MultilayerCache.RemoveAsync(CacheKey[], token) built each entry's options without the token, so an already-cancelled call did the work and only noticed at the removal. It forwards the token now, like the single-key overload does. - The periodic-timer loops in CacheMemoryMonitor and RedisStreamHealthMaintainer, and the planned-maintenance probe task, now pass CancellationToken.None explicitly, each with the reason. Disposal is what ends those loops, and propagating a token would trade a clean exit for an OperationCanceledException. The probe delegate must always be scheduled: its finally disposes the linked token source and clears InProgress, so skipping it would block every later probe. - Test fakes use the non-obsolete RedisServerException constructor. Its RedisErrorKind argument is experimental (SER007), suppressed in the test project alongside the SER305/SER306 suppressions already there. Not changed: the null-forgiving operator the analyzer flagged in RedisStreamsTopic. Removing it makes the compiler report CS8604, because RedisKey converts to a nullable string, so the operator is load-bearing and the finding is a false positive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fwLrS3Sbcen8v6iRkUaFB Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
cosmin-staicu
force-pushed
the
chore/fix-sonar-warnings
branch
from
September 9, 2026 12:05
b565223 to
38c790a
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, consistent with existing APIs (including BuildEntryOptions cancellation behavior), and the updated call sites preserve intended operational semantics while resolving the described defect and warnings.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
alinahornet
approved these changes
Sep 9, 2026
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
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.
Why
CI annotates eight analyzer warnings on every pull request. None belong to the branch that surfaces them, so they get carried from PR to PR and read as noise. One of the eight is a real defect.
The defect
MultilayerCache.RemoveAsync(CacheKey[], token)built each entry's options without the token, while the single-key overload passes it.CacheEntryBuilder.BuildEntryOptionscallsThrowIfCancellationRequested, so an already-cancelled multi-key removal did the key-strategy and topic-key work anyway and only noticed at the removal itself. Now it forwards the token.The rest, no behavior change
CacheMemoryMonitorandRedisStreamHealthMaintainerpassCancellationToken.Noneexplicitly. Disposal is what ends them:Dispose/StopAsyncdisposes the timer, which completes the pending wait withfalse. Propagating the token would swap a clean exit for anOperationCanceledException. Each site says so in a comment. The health maintainer's semaphore wait has a zero timeout, so it never blocks either.CancellationToken.NonetoTask.Rundeliberately. Its token is already on aCancelAftertimer, and a token that won the race against scheduling would skip the delegate, leaking the linked source and leavingInProgressstuck true, which blocks every later probe. The delegate must always run so itsfinallyclears the flag.RedisServerExceptionconstructor. ItsRedisErrorKindargument is experimental (SER007), suppressed in the test project next to theSER305/SER306suppressions already there. The production paths match on message text, soRedisErrorKind.Noneis faithful to what these fakes exercise.One finding rejected
The analyzer wants the null-forgiving operator gone from
RedisStreamsTopicline 120, saying the compiler already knows the expression is non-null. It does not: removing it makes the compiler reportCS8604, becauseRedisKeyconverts to a nullablestringand the telemetry method takes a non-nullable one. The operator is load-bearing, so the code is unchanged.Verification
Solution builds with zero warnings, where it previously emitted eight. Full suite passes on both frameworks, 1636 tests on .NET 10 and 1615 on .NET 8.
🤖 Generated with Claude Code
https://claude.ai/code/session_017fwLrS3Sbcen8v6iRkUaFB