Fix Redis TryAcquireAsync handling for disconnected databases - #283
Fix Redis TryAcquireAsync handling for disconnected databases#283teesofttech wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Redis TryAcquireAsync behavior in RedLock when one or more IDatabase instances are disconnected (where StackExchange.Redis may backlog operations instead of failing fast), ensuring disconnected cases surface promptly and correctly without masking real underlying faults.
Changes:
- Update
RedLockAcquire.WaitForAcquireAsyncto properly propagate the synthetic “disconnected database” fault when it becomes decisive and no real faulted/canceled acquire tasks exist. - Adjust the disconnected-database decisiveness logic so a single disconnected database is immediately decisive in the single-server scenario.
- Add CI tests covering: single-db disconnect throws, synthetic disconnect does not mask a real fault, and single-db contention returns
null.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/DistributedLock.Tests/Tests/Redis/RedisDistributedLockTest.cs | Adds targeted CI tests validating TryAcquireAsync outcomes for disconnected and contended Redis database scenarios. |
| src/DistributedLock.Redis/RedLock/RedLockAcquire.cs | Fixes fault propagation for synthetic disconnect faults and refines decisiveness logic for single-database acquisition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
If you can help review this PR as well please @madelson |
| var faultingTasks = tryAcquireTasks.Values.Where(t => t.IsCanceled || t.IsFaulted) | ||
| var faultingTasks = tryAcquireTasks.Values | ||
| .Where(t => t.IsCanceled || t.IsFaulted) | ||
| .ToArray(); |
There was a problem hiding this comment.
Please revert the formatting-only chage
| // A disconnected single database is always decisive. | ||
| if (@this._databases.Count != 1 | ||
| && !((successCount > 0 && RedLockHelper.HasSufficientSuccesses(successCount + 1, @this._databases.Count)) | ||
| || (failCount > 0 && RedLockHelper.HasTooManyFailuresOrFaults(failCount + 1, @this._databases.Count)))) |
There was a problem hiding this comment.
@teesofttech help me think through this: rather than adding @this._databases.Count != 1 can't we just remove the "at least 1" checks?
Let's say we have 2 DBs, 0 failures, and 0 successes. In that case 2 successes or 1 failure is required. Therefore, 1 additional failure is decisive, so we should not return null.
| while (true) | ||
| { | ||
| var completed = TryResolveDisconnectedDatabaseAsFaulted(this) | ||
| ?? await Task.WhenAny(incompleteTasks).ConfigureAwait(false); |
There was a problem hiding this comment.
I think this change is correct but adds meaningful overhead to the system. Previously, we'd never call IsConnected in the common single-server case. Now, we call it every single time even though mostly that's not needed.
Furthermore, calling IsConnected upfront means we can never recover if the connection fails after the first call or if it isn't connected but is about to reconnect.
I feel like the best approach might be to launch a task that just watches for disconnects alongside the connection attempts. I need to think more about this.
For #242