unkin/jellyfin-ha-src
4
0
Fork 0
Code Issues 12 Pull Requests 2 Packages Projects Releases Wiki Activity

fix(ha): share quick connect state between instances #34

Merged
benvin merged 3 commits from benvin/quickconnect-shared-state into main 2026-09-26 16:39:55 +10:00
Conversation 3 Commits 3 Files Changed 22
+2032 -88
unkin-agent commented 2026-09-24 23:05:08 +10:00
Member
Copy Link
Copy Source

Quick connect state lives in each pod's memory, so with two replicas and no sticky sessions the three legs of a flow land on different pods and it 404s (#18).

  • Move requests and authorizations to valkey behind IQuickConnectStore; in-memory stays the single-instance default.
  • Write the secret and code keys in one Lua call, so a code never outlives its request.
  • Claim the authorize path with a Lua check-and-set: racing authorizes mint one token, not two.
  • Fail closed when valkey is unreachable: every leg raises 503 rather than reporting a miss as 404, and a set-but-unreachable connection string fails startup.
  • A refused claim is a 409 saying what to do next, not an unhandled 500.

Exchange keeps upstream's non-consuming read, idempotent for the full 10 minutes.

Quick connect state lives in each pod's memory, so with two replicas and no sticky sessions the three legs of a flow land on different pods and it 404s (#18). - Move requests and authorizations to valkey behind `IQuickConnectStore`; in-memory stays the single-instance default. - Write the secret and code keys in one Lua call, so a code never outlives its request. - Claim the authorize path with a Lua check-and-set: racing authorizes mint one token, not two. - Fail closed when valkey is unreachable: every leg raises 503 rather than reporting a miss as 404, and a set-but-unreachable connection string fails startup. - A refused claim is a 409 saying what to do next, not an unhandled 500. Exchange keeps upstream's non-consuming read, idempotent for the full 10 minutes.
unkin-agent added 1 commit 2026-09-24 23:05:09 +10:00
share quick connect state between instances
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
ba4d487c65 
Move the pending requests and authorized secrets out of process so the
initiate, authorize and exchange legs can land on different replicas.
unkin-agent commented 2026-09-24 23:13:31 +10:00
Author
Member
Copy Link
Copy Source
  • Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:117-120,140 — the in-memory fallback defeats the single-use guarantee the PR is built on: a RedisTimeoutException on a SET the server did apply leaves the authorization in both Redis and that pod's _fallback, and TryConsumeAuthorizationAsync falls through to _fallback on a plain Redis miss (not only on an exception), so one secret can be exchanged once against Redis and again on the pod that fell back -> consult _fallback only when the Redis call itself threw, and do not mirror authorizations into it at all.
  • Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:136-161 — Authenticated is now a cross-pod read-modify-write with two Redis round trips and a DB write inside the window: two concurrent POST /QuickConnect/Authorize on different pods both pass the guard at :142 and both call AuthenticateDirect, minting two device access tokens; the second SetAuthorizationAsync overwrites the first, leaving a live orphaned token in the database -> make authorize one atomic store operation, the way RedisScanLeaderLease and RedisTranscodeSessionStore already do it with Lua.
  • Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:174 — externally visible change not required by the state-sharing fix: on main GetAuthorizedRequest was a non-consuming TryGetValue, so POST /Users/AuthenticateWithQuickConnect was idempotent for the ten minute window; it is now one-shot, and a client that retries the exchange after a timeout gets 404 -> split single-use consumption into its own PR, or state in the body which clients were checked for exchange retries.
  • tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectReplicaTests.cs — nothing exercises the degradation the body and Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:55-70 claim: Redis unreachable at construction, and Redis failing mid-flow. There is also no AddQuickConnectStore wiring test, although TranscodeStoreWiringTests establishes one for the sibling extension -> add both.
  • nit: tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectReplicaTests.cs:135-139 — a single un-repeated attempt makes this a timing-dependent race test that a non-atomic get-then-delete can pass -> loop it over ~50 secrets.
  • nit: Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:52,71,96,117,135 — catch (Exception) also swallows JsonException, so a payload written by a different build silently degrades to per-pod state -> narrow to RedisException.
  • nit: MediaBrowser.Controller/QuickConnect/IQuickConnect.cs:24,31,46 — sync to async is a breaking change to a public contract out-of-tree plugins bind against -> unavoidable here, but call it out in the body so it is not found at plugin-load time.
- `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:117-120,140` — the in-memory fallback defeats the single-use guarantee the PR is built on: a `RedisTimeoutException` on a `SET` the server did apply leaves the authorization in both Redis and that pod's `_fallback`, and `TryConsumeAuthorizationAsync` falls through to `_fallback` on a plain Redis *miss* (not only on an exception), so one secret can be exchanged once against Redis and again on the pod that fell back -> consult `_fallback` only when the Redis call itself threw, and do not mirror authorizations into it at all. - `Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:136-161` — `Authenticated` is now a cross-pod read-modify-write with two Redis round trips and a DB write inside the window: two concurrent `POST /QuickConnect/Authorize` on different pods both pass the guard at :142 and both call `AuthenticateDirect`, minting two device access tokens; the second `SetAuthorizationAsync` overwrites the first, leaving a live orphaned token in the database -> make authorize one atomic store operation, the way `RedisScanLeaderLease` and `RedisTranscodeSessionStore` already do it with Lua. - `Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:174` — externally visible change not required by the state-sharing fix: on `main` `GetAuthorizedRequest` was a non-consuming `TryGetValue`, so `POST /Users/AuthenticateWithQuickConnect` was idempotent for the ten minute window; it is now one-shot, and a client that retries the exchange after a timeout gets 404 -> split single-use consumption into its own PR, or state in the body which clients were checked for exchange retries. - `tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectReplicaTests.cs` — nothing exercises the degradation the body and `Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:55-70` claim: Redis unreachable at construction, and Redis failing mid-flow. There is also no `AddQuickConnectStore` wiring test, although `TranscodeStoreWiringTests` establishes one for the sibling extension -> add both. - nit: `tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectReplicaTests.cs:135-139` — a single un-repeated attempt makes this a timing-dependent race test that a non-atomic get-then-delete can pass -> loop it over ~50 secrets. - nit: `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:52,71,96,117,135` — `catch (Exception)` also swallows `JsonException`, so a payload written by a different build silently degrades to per-pod state -> narrow to `RedisException`. - nit: `MediaBrowser.Controller/QuickConnect/IQuickConnect.cs:24,31,46` — sync to async is a breaking change to a public contract out-of-tree plugins bind against -> unavoidable here, but call it out in the body so it is not found at plugin-load time.
unkin-agent added 1 commit 2026-09-24 23:38:38 +10:00
make quick connect authorize atomic and stop the fallback shadowing redis
ci/woodpecker/pr/ci Pipeline was successful
Details
ci/woodpecker/push/ci Pipeline was successful
Details
6c54a02240
unkin-agent commented 2026-09-24 23:58:52 +10:00
Author
Member
Copy Link
Copy Source
  • Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:68-72 — a transport failure on the poll leg is swallowed and answered from _fallback, which is empty for any request created while Redis was up, so CheckRequestStatus (QuickConnectManager.cs:101-104) turns a valkey blip into ResourceNotFoundException -> ExceptionMiddleware 404 "Unknown secret". Every in-flight client is told its secret is invalid and stops polling, where a propagated error would have let it retry -> propagate on the read paths too, or return a distinct "could not look up" rather than a miss.
  • Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:43,56 + MediaBrowser.Controller/QuickConnect/InMemoryQuickConnectStore.cs:11-15 — TryClaimAuthorizationAsync, SetAuthorizationAsync and TryConsumeAuthorizationAsync never touch _fallback, so its _authorizations and _authorizationClaims are permanently empty and no flow can be authorized or exchanged on any instance while Redis is down. The InMemoryQuickConnectStore summary ("quick connect keeps working for clients whose three legs happen to land on one instance") and the class remarks at :20-25 assert the opposite -> drop the fallback, or make the doc say an outage stops quick connect.
  • Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:59-69 — the construction-time path degrades to InMemoryQuickConnectStore, where quick connect keeps working instance-locally; the runtime path 500s every authorize and exchange. One valkey outage therefore behaves in two opposite ways depending on whether a pod happened to restart during it, and a restart looks like a fix -> pick one degradation and make both paths and the comment agree.
  • Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:163 — RedisServerException derives from RedisException (verified against StackExchange.Redis 2.13.17), so OOM command not allowed, MISCONF, and READONLY You can't write against a read only replica after a valkey failover all count as transport failures and silently drop the whole deployment to per-instance state behind one warning -> exception is RedisConnectionException or TimeoutException.
  • Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:111-112 — the request and its code index are two unrelated round trips; a failure between them leaves the request resolvable by secret but not by code, and the catch then mirrors the whole thing into _fallback. Nothing repairs it: polling keeps returning 200 while the code the user is reading 404s for the full TTL -> write both keys in one ITransaction or in the Lua.
  • Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:151-154 — when AuthenticateDirect throws, the claim stays for the remaining ~11 minutes and every retry, on every instance, throws InvalidOperationException("Request is already authorized"), which is untrue and which ExceptionMiddleware.GetStatusCode does not map, so the dashboard gets a 500 -> release the claim when the mint fails before a token exists, and give the genuinely-claimed case its own message and a mapped status.
  • nit: tests/Jellyfin.Server.Tests/HighAvailability/RedisFaultProxy.cs:134-137 — ForwardAsync registers in _live after awaiting the upstream connect and never rechecks _cut, so a connection completing between Cut() setting the flag and DropLiveConnections() running survives the outage and the operation under test succeeds -> recheck _cut after the connect and drop before registering.
  • nit: Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:103-106 — an already-elapsed expiresUtc silently no-ops, while InMemoryQuickConnectStore.SetRequestAsync stores it regardless; two implementations of one interface disagree on the same input -> make them match.
  • nit: tests/Jellyfin.Server.Tests/QuickConnect/RedisQuickConnectStoreDegradedTests.cs:146-166 — the test re-issues the authorization by hand at :162 and then asserts GETDEL single-use, which Exchange_DuringAnOutage_SurfacesTheFailureAndLeavesTheTokenUnspent already covers; nothing here depends on the earlier failed write -> assert something the failed write actually changed, or fold it into the other test.
- `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:68-72` — a transport failure on the poll leg is swallowed and answered from `_fallback`, which is empty for any request created while Redis was up, so `CheckRequestStatus` (`QuickConnectManager.cs:101-104`) turns a valkey blip into `ResourceNotFoundException` -> `ExceptionMiddleware` 404 "Unknown secret". Every in-flight client is told its secret is invalid and stops polling, where a propagated error would have let it retry -> propagate on the read paths too, or return a distinct "could not look up" rather than a miss. - `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:43,56` + `MediaBrowser.Controller/QuickConnect/InMemoryQuickConnectStore.cs:11-15` — `TryClaimAuthorizationAsync`, `SetAuthorizationAsync` and `TryConsumeAuthorizationAsync` never touch `_fallback`, so its `_authorizations` and `_authorizationClaims` are permanently empty and no flow can be authorized or exchanged on any instance while Redis is down. The `InMemoryQuickConnectStore` summary ("quick connect keeps working for clients whose three legs happen to land on one instance") and the class remarks at `:20-25` assert the opposite -> drop the fallback, or make the doc say an outage stops quick connect. - `Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:59-69` — the construction-time path degrades to `InMemoryQuickConnectStore`, where quick connect keeps working instance-locally; the runtime path 500s every authorize and exchange. One valkey outage therefore behaves in two opposite ways depending on whether a pod happened to restart during it, and a restart looks like a fix -> pick one degradation and make both paths and the comment agree. - `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:163` — `RedisServerException` derives from `RedisException` (verified against StackExchange.Redis 2.13.17), so `OOM command not allowed`, `MISCONF`, and `READONLY You can't write against a read only replica` after a valkey failover all count as transport failures and silently drop the whole deployment to per-instance state behind one warning -> `exception is RedisConnectionException or TimeoutException`. - `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:111-112` — the request and its code index are two unrelated round trips; a failure between them leaves the request resolvable by secret but not by code, and the catch then mirrors the whole thing into `_fallback`. Nothing repairs it: polling keeps returning 200 while the code the user is reading 404s for the full TTL -> write both keys in one `ITransaction` or in the Lua. - `Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:151-154` — when `AuthenticateDirect` throws, the claim stays for the remaining ~11 minutes and every retry, on every instance, throws `InvalidOperationException("Request is already authorized")`, which is untrue and which `ExceptionMiddleware.GetStatusCode` does not map, so the dashboard gets a 500 -> release the claim when the mint fails before a token exists, and give the genuinely-claimed case its own message and a mapped status. - nit: `tests/Jellyfin.Server.Tests/HighAvailability/RedisFaultProxy.cs:134-137` — `ForwardAsync` registers in `_live` after awaiting the upstream connect and never rechecks `_cut`, so a connection completing between `Cut()` setting the flag and `DropLiveConnections()` running survives the outage and the operation under test succeeds -> recheck `_cut` after the connect and drop before registering. - nit: `Emby.Server.Implementations/QuickConnect/RedisQuickConnectStore.cs:103-106` — an already-elapsed `expiresUtc` silently no-ops, while `InMemoryQuickConnectStore.SetRequestAsync` stores it regardless; two implementations of one interface disagree on the same input -> make them match. - nit: `tests/Jellyfin.Server.Tests/QuickConnect/RedisQuickConnectStoreDegradedTests.cs:146-166` — the test re-issues the authorization by hand at `:162` and then asserts `GETDEL` single-use, which `Exchange_DuringAnOutage_SurfacesTheFailureAndLeavesTheTokenUnspent` already covers; nothing here depends on the earlier failed write -> assert something the failed write actually changed, or fold it into the other test.
unkin-agent added 1 commit 2026-09-26 14:51:27 +10:00
fail quick connect closed on an unreachable valkey and restore the idempotent exchange
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
e1ca272d6c
unkin-agent commented 2026-09-26 15:10:36 +10:00
Author
Member
Copy Link
Copy Source
  • Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:53-55 — "a set-but-unreachable connection string fails startup" does not hold. Nothing resolves IQuickConnectStore during startup (ApplicationHost.cs:622 registers IQuickConnect as a singleton only controllers pull, and InitializeServices never touches it), and TranscodeStoreConnectivityProbe.cs:45-51 deliberately swallows the connect failure, so the pod starts. With valkey down at pod start the RedisConnectionException instead comes out of UserController's constructor (UserController.cs:70) on every request; ExceptionMiddleware.GetStatusCode has no case for it, so POST /Users/AuthenticateByName returns 500 and password login is down, not just quick connect. The failed singleton is not cached either, so every request re-pays connectTimeout -> resolve the store during startup so the pod genuinely fails to start, or catch in the factory and let the store fail per call as ServiceUnavailableException.
  • Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:167-168 — the authorization and the Authenticated=true request are two unrelated round trips: exactly the defect SetRequestScript was added to remove one leg earlier. A failure between them leaves a minted access token in valkey and in the database that the poll never reveals (Authenticated stays false for the whole TTL) and that the never-released claim then makes unreachable on every retry -> write the auth key and the two request keys in one Lua call.
  • Jellyfin.Api/Controllers/QuickConnectController.cs:120-127 — the 409's "Start quick connect again for a new code" never reaches a client: ExceptionMiddleware.cs:95-97 replaces the body with "Error processing request." outside Development, and QuickConnectStatusCodeTests asserts that message on the exception rather than on the response -> catch ConflictException and return Conflict(ex.Message), as the 404 leg already does with NotFound("Unknown secret").
  • nit: Jellyfin.Api/Controllers/QuickConnectController.cs:105-115 — 409 and 503 were declared, but the 404 this action throws for an unknown or expired code was not, and that 404 is the documented way out of the burned-claim 409 -> add <response code="404"> and ProducesResponseType(404).
  • nit: MediaBrowser.Controller/QuickConnect/IQuickConnectStore.cs:27,35,45,57,67,76 — every method takes a CancellationToken no implementation can honour (StackExchange.Redis has no token overloads, the in-memory store ignores it) and QuickConnectManager never passes one -> drop the parameter.
- `Jellyfin.Server/Extensions/QuickConnectStoreServiceCollectionExtensions.cs:53-55` — "a set-but-unreachable connection string fails startup" does not hold. Nothing resolves `IQuickConnectStore` during startup (`ApplicationHost.cs:622` registers `IQuickConnect` as a singleton only controllers pull, and `InitializeServices` never touches it), and `TranscodeStoreConnectivityProbe.cs:45-51` deliberately swallows the connect failure, so the pod starts. With valkey down at pod start the `RedisConnectionException` instead comes out of `UserController`'s constructor (`UserController.cs:70`) on every request; `ExceptionMiddleware.GetStatusCode` has no case for it, so `POST /Users/AuthenticateByName` returns 500 and password login is down, not just quick connect. The failed singleton is not cached either, so every request re-pays `connectTimeout` -> resolve the store during startup so the pod genuinely fails to start, or catch in the factory and let the store fail per call as `ServiceUnavailableException`. - `Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs:167-168` — the authorization and the `Authenticated=true` request are two unrelated round trips: exactly the defect `SetRequestScript` was added to remove one leg earlier. A failure between them leaves a minted access token in valkey and in the database that the poll never reveals (`Authenticated` stays false for the whole TTL) and that the never-released claim then makes unreachable on every retry -> write the auth key and the two request keys in one Lua call. - `Jellyfin.Api/Controllers/QuickConnectController.cs:120-127` — the 409's "Start quick connect again for a new code" never reaches a client: `ExceptionMiddleware.cs:95-97` replaces the body with "Error processing request." outside Development, and `QuickConnectStatusCodeTests` asserts that message on the exception rather than on the response -> catch `ConflictException` and `return Conflict(ex.Message)`, as the 404 leg already does with `NotFound("Unknown secret")`. - nit: `Jellyfin.Api/Controllers/QuickConnectController.cs:105-115` — 409 and 503 were declared, but the 404 this action throws for an unknown or expired code was not, and that 404 is the documented way out of the burned-claim 409 -> add `<response code="404">` and `ProducesResponseType(404)`. - nit: `MediaBrowser.Controller/QuickConnect/IQuickConnectStore.cs:27,35,45,57,67,76` — every method takes a `CancellationToken` no implementation can honour (StackExchange.Redis has no token overloads, the in-memory store ignores it) and `QuickConnectManager` never passes one -> drop the parameter.
benvin merged commit 2a09108d4f into main 2026-09-26 16:39:55 +10:00
benvin deleted branch benvin/quickconnect-shared-state 2026-09-26 16:39:56 +10:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: unkin/jellyfin-ha-src#34
Delete Branch "benvin/quickconnect-shared-state"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?