fail quick connect closed on an unreachable valkey and restore the idempotent exchange

Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs

@@ -141,7 +141,7 @@ namespace Emby.Server.Implementations.QuickConnect
 
             if (result.Authenticated)
             {
-                throw new InvalidOperationException("Request is already authorized");
+                throw new ConflictException("Request is already authorized");
             }
 
             // Change the time on the request so it expires one minute into the future. It can't expire immediately as otherwise some clients wouldn't ever see that they have been authenticated.
@@ -150,7 +150,7 @@ namespace Emby.Server.Implementations.QuickConnect
             // The guard above is a read on shared state, so it cannot settle a race between instances; the claim can.
             if (!await _store.TryClaimAuthorizationAsync(result.Secret, ExpiryOf(result)).ConfigureAwait(false))
             {
-                throw new InvalidOperationException("Request is already authorized");
+                throw await RefusedClaimAsync(result.Secret).ConfigureAwait(false);
             }
 
             var authenticationResult = await _sessionManager.AuthenticateDirect(new AuthenticationRequest
@@ -177,7 +177,7 @@ namespace Emby.Server.Implementations.QuickConnect
         {
             AssertActive();
 
-            var result = await _store.TryConsumeAuthorizationAsync(secret).ConfigureAwait(false);
+            var result = await _store.GetAuthorizationAsync(secret).ConfigureAwait(false);
             if (result is null)
             {
                 throw new ResourceNotFoundException("Unable to find request");
@@ -188,6 +188,20 @@ namespace Emby.Server.Implementations.QuickConnect
 
         private static DateTime ExpiryOf(QuickConnectResult request) => request.DateAdded.AddMinutes(Timeout);
 
+        /// <summary>
+        /// Explains a refused claim. The claim outlives a failed mint on purpose, so it can mean either
+        /// that the request is authorized or that authorizing it did not finish; the two are told apart
+        /// by re-reading the request rather than reported as the same thing.
+        /// </summary>
+        private async Task<ConflictException> RefusedClaimAsync(string secret)
+        {
+            var current = await _store.GetRequestBySecretAsync(secret).ConfigureAwait(false);
+
+            return current?.Authenticated == true
+                ? new ConflictException("Request is already authorized")
+                : new ConflictException("Request is being authorized elsewhere, or an earlier attempt to authorize it did not complete. Start quick connect again for a new code.");
+        }
+
         private string GenerateSecureRandom(int length = 32)
         {
             Span<byte> bytes = stackalloc byte[length];

Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs

@@ -3,6 +3,7 @@ using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Extensions.Json;
+using MediaBrowser.Common.Extensions;
 using MediaBrowser.Controller.Authentication;
 using MediaBrowser.Controller.QuickConnect;
 using MediaBrowser.Model.QuickConnect;
@@ -13,20 +14,27 @@ namespace Emby.Server.Implementations.QuickConnect;
 
 /// <summary>
 /// A Redis-backed <see cref="IQuickConnectStore"/> that lets the initiate, authorize and exchange legs
-/// of a quick connect flow land on different instances. Expiry is the key TTL, an authorization is
-/// claimed with a Lua check-and-set and consumed with <c>GETDEL</c>, so only one instance can ever mint
-/// a given secret's access token and only one can ever hand it out.
+/// of a quick connect flow land on different instances. Expiry is the key TTL and an authorization is
+/// claimed with a Lua check-and-set, so only one instance can ever mint a given secret's access token.
 /// </summary>
 /// <remarks>
-/// A pending request survives an unreachable Redis through a process-local fallback, because a second
-/// copy of it is harmless. An authorization has none: a second copy of it is a second access token, and
-/// a write whose response timed out may well have been applied, so a transport failure on that path is
-/// surfaced rather than degraded.
+/// There is no local fallback: a call Redis did not answer is inconclusive, and reporting it as a miss
+/// would tell a polling client its secret is invalid. Quick connect is unavailable for as long as Redis
+/// is, which password login is not.
 /// </remarks>
 public sealed class RedisQuickConnectStore : IQuickConnectStore
 {
     private const string KeyPrefix = "jellyfin:quickconnect:";
 
+    /// <summary>
+    /// Lua script writing the two keys a request is resolvable by in one step, so it can never be
+    /// reachable by its secret while the code the user is reading off the screen resolves to nothing.
+    /// </summary>
+    private const string SetRequestScript = @"
+redis.call('SET', KEYS[1], ARGV[1], 'PX', ARGV[3])
+redis.call('SET', KEYS[2], ARGV[2], 'PX', ARGV[3])
+return 1";
+
     /// <summary>
     /// Lua script for the atomic claim of the sole right to authorize a request: the request has to
     /// exist and not already be authorized, and the claim marker is taken with <c>SET NX</c>, so of two
@@ -40,7 +48,6 @@ if redis.call('SET', KEYS[2], '1', 'NX', 'PX', ARGV[1]) then return 1 end
 return 0";
 
     private readonly IDatabase _db;
-    private readonly InMemoryQuickConnectStore _fallback;
     private readonly ILogger<RedisQuickConnectStore> _logger;
 
     /// <summary>
@@ -53,41 +60,23 @@ return 0";
         ArgumentNullException.ThrowIfNull(redis);
 
         _db = redis.GetDatabase();
-        _fallback = new InMemoryQuickConnectStore();
         _logger = logger;
     }
 
     /// <inheritdoc />
     public async Task<QuickConnectResult?> GetRequestBySecretAsync(string secret, CancellationToken cancellationToken = default)
     {
-        RedisValue raw;
-        try
-        {
-            raw = await _db.StringGetAsync(RequestKey(secret)).ConfigureAwait(false);
-        }
-        catch (Exception ex) when (IsTransportFailure(ex))
-        {
-            LogDegraded(ex);
-            return await _fallback.GetRequestBySecretAsync(secret, cancellationToken).ConfigureAwait(false);
-        }
+        var raw = await CallAsync(() => _db.StringGetAsync(RequestKey(secret))).ConfigureAwait(false);
 
-        // A miss is an answer rather than a transport failure, so the fallback is not consulted for it.
+        // Deserialization is outside the guard: a malformed stored value is a fault of its own, not Redis
+        // being unavailable.
         return raw.HasValue ? JsonSerializer.Deserialize<QuickConnectResult>(raw.ToString(), JsonDefaults.Options) : null;
     }
 
     /// <inheritdoc />
     public async Task<QuickConnectResult?> GetRequestByCodeAsync(string code, CancellationToken cancellationToken = default)
     {
-        RedisValue secret;
-        try
-        {
-            secret = await _db.StringGetAsync(CodeKey(code)).ConfigureAwait(false);
-        }
-        catch (Exception ex) when (IsTransportFailure(ex))
-        {
-            LogDegraded(ex);
-            return await _fallback.GetRequestByCodeAsync(code, cancellationToken).ConfigureAwait(false);
-        }
+        var secret = await CallAsync(() => _db.StringGetAsync(CodeKey(code))).ConfigureAwait(false);
 
         return secret.HasValue
             ? await GetRequestBySecretAsync(secret.ToString(), cancellationToken).ConfigureAwait(false)
@@ -105,17 +94,11 @@ return 0";
             return;
         }
 
-        try
-        {
-            var json = JsonSerializer.Serialize(request, JsonDefaults.Options);
-            await _db.StringSetAsync(RequestKey(request.Secret), json, ttl).ConfigureAwait(false);
-            await _db.StringSetAsync(CodeKey(request.Code), request.Secret, ttl).ConfigureAwait(false);
-        }
-        catch (Exception ex) when (IsTransportFailure(ex))
-        {
-            LogDegraded(ex);
-            await _fallback.SetRequestAsync(request, expiresUtc, cancellationToken).ConfigureAwait(false);
-        }
+        var json = JsonSerializer.Serialize(request, JsonDefaults.Options);
+        await CallAsync(() => _db.ScriptEvaluateAsync(
+            SetRequestScript,
+            keys: new RedisKey[] { RequestKey(request.Secret), CodeKey(request.Code) },
+            values: new RedisValue[] { json, request.Secret, (long)ttl.TotalMilliseconds })).ConfigureAwait(false);
     }
 
     /// <inheritdoc />
@@ -127,10 +110,10 @@ return 0";
             return false;
         }
 
-        var claimed = (long?)await _db.ScriptEvaluateAsync(
+        var claimed = (long?)await CallAsync(() => _db.ScriptEvaluateAsync(
             ClaimAuthorizationScript,
             keys: new RedisKey[] { RequestKey(secret), ClaimKey(secret) },
-            values: new RedisValue[] { (long)ttl.TotalMilliseconds }).ConfigureAwait(false);
+            values: new RedisValue[] { (long)ttl.TotalMilliseconds })).ConfigureAwait(false);
 
         return claimed == 1;
     }
@@ -145,23 +128,19 @@ return 0";
         }
 
         var json = JsonSerializer.Serialize(authenticationResult, JsonDefaults.Options);
-        await _db.StringSetAsync(AuthorizationKey(secret), json, ttl).ConfigureAwait(false);
+        await CallAsync(() => _db.StringSetAsync(AuthorizationKey(secret), json, ttl)).ConfigureAwait(false);
     }
 
     /// <inheritdoc />
-    public async Task<AuthenticationResult?> TryConsumeAuthorizationAsync(string secret, CancellationToken cancellationToken = default)
+    public async Task<AuthenticationResult?> GetAuthorizationAsync(string secret, CancellationToken cancellationToken = default)
     {
-        var raw = await _db.StringGetDeleteAsync(AuthorizationKey(secret)).ConfigureAwait(false);
+        var raw = await CallAsync(() => _db.StringGetAsync(AuthorizationKey(secret))).ConfigureAwait(false);
 
         return raw.HasValue
             ? JsonSerializer.Deserialize<AuthenticationResult>(raw.ToString(), JsonDefaults.Options)
             : null;
     }
 
-    // Deliberately excludes a malformed stored value, which is a fault of its own rather than a reason
-    // to answer from this instance.
-    private static bool IsTransportFailure(Exception exception) => exception is RedisException or TimeoutException;
-
     private static string RequestKey(string secret) => KeyPrefix + "request:" + secret;
 
     private static string CodeKey(string code) => KeyPrefix + "code:" + code;
@@ -170,6 +149,16 @@ return 0";
 
     private static string AuthorizationKey(string secret) => KeyPrefix + "auth:" + secret;
 
-    private void LogDegraded(Exception exception)
-        => _logger.LogWarning(exception, "Quick connect request state could not be shared through Redis; falling back to this instance only.");
+    private async Task<T> CallAsync<T>(Func<Task<T>> call)
+    {
+        try
+        {
+            return await call().ConfigureAwait(false);
+        }
+        catch (Exception exception) when (exception is RedisException or RedisCommandException or TimeoutException)
+        {
+            _logger.LogError(exception, "Quick connect state could not be reached in Redis.");
+            throw new ServiceUnavailableException("Quick connect is temporarily unavailable.", exception);
+        }
+    }
 }