Skip to content

fix: Prevent log out bug on tvOS#1875

Closed
PhilippeWeidmann wants to merge 3 commits intojellyfin:mainfrom
PhilippeWeidmann:fix/logout-keychain
Closed

fix: Prevent log out bug on tvOS#1875
PhilippeWeidmann wants to merge 3 commits intojellyfin:mainfrom
PhilippeWeidmann:fix/logout-keychain

Conversation

@PhilippeWeidmann
Copy link

@PhilippeWeidmann PhilippeWeidmann commented Dec 25, 2025

Context

As mentioned in my comment here, the issue comes from the fact that Swiftfin isn’t able to restore its entire cache from an access token alone, as it requires both a ServerModel and a UserModel.

tvOS is more aggressive with cache cleaning, which results in frequent disconnects, but this could theoretically also happen on iOS.


Description of the fix

Previously, the accessToken was a simple string, with the token stored in the Keychain.

The token has been migrated to an object called AccessToken. This object contains the minimum data required to restore a session: userId, username, token, and associatedServerURLs.

On each app launch, UserSessionRestorationHelper is called to retrieve a UserSession.

Happy path
The database exists, so we use the existing code path and call getCurrentUserSession().

Unhappy path
The database has been deleted, so we attempt to restore the user and server information from the Keychain.

This approach isn’t perfect, but it gets the job done. One drawback is that it introduces a double source of truth—Keychain vs. Core Data—which must be kept in sync. This means ensuring the Keychain is updated whenever the UserModel is added, modified, or deleted.

I split the fix in 3 commits:

  • New AccessToken object
  • Restoration from AccessToken
  • One small fix to delete the token on logout

Scenarios tested

  • Token migration from master to this branch without disconnect
  • Normal app boot path
  • Restore from Keychain with one account and one server URL
  • Restore from Keychain with multiple accounts and multiple server URLs (same server ID)

@JPKribs JPKribs added bug Something isn't working tvOS Impacts tvOS labels Dec 25, 2025
@lebojo

This comment has been minimized.

@JPKribs
Copy link
Member

JPKribs commented Dec 25, 2025

I don't think this is the correct approach. This would still mean that each time core store is wiped, we would lose all of the settings and customizations that the users have made.

Additionally, this would only preserve the last logged in user. If multiple users were set up on the Apple TV, every other user would be lost. Additionally, if anyone uses the setting to log their user out on background, there would be no user ID to pull from in this scenario.

I'll defer to @LePips but I believe the only real solution of this would be move everything into user defaults for Apple TV (and likely iOS as well for development parity). Part of what makes that complicated is 1) decoupling multi-user settings from core store and 2) 500kb limitations.

The former would require a lot of revisiting for the multi user support. Core store allows us to key items to the user ID so, enabling multiple users would change every one of our settings from purely value to UserID: Value. Either that, or customization settings would have to be device wide instead of per user.

The size limitation is manageable but we'd likely need limitations to help users avoid getting into trouble with many users/servers. As the person who added it, I would also argue that the custom device profiles might make more sense as an iOS exclusive option as iOS can use core store without issue. Since that setting is fairly variable in size depending on how it's configured, I think it's the most likely setting that would get people into trouble.

So, I think this would end up being a much larger task that requires a lot of testing and we will likely need to build a migration from core store to user defaults.

The most ideal solution would be Apple allowing us to actually store non-UserDefaults data on the Apple TV. But that's wishful thinking haha


Edit: I've updated my original post, to the best of my understanding, to explain the scenario: #776

Primarily, I think we should push for a more permanent solution where information is stored in the right place for tvOS as opposed to moving data into the current, wrong place for a recovery.

@PhilippeWeidmann
Copy link
Author

PhilippeWeidmann commented Dec 25, 2025

Additionally, this would only preserve the last logged in user

This should save/restore every logged in user but maybe I'm missing something ?
Is it possible to add user without a token ?

@rud

This comment has been minimized.

@sjorge
Copy link

sjorge commented Dec 26, 2025

This would still mean that each time core store is wiped, we would lose all of the settings and customizations that the users have made.

I know the server can store client settings, perhaps swiftfin can make use of that somehow?

Login/Logout -> Update DB -> Update keychain
Settings changed -> Update DB -> Push to server

Startup: Local DB OK -> use that
Startup: Local DB missing? -> have keychain to restore from -> restore -> fetch settings from server ?

Although that seems rather complex :(

@LePips
Copy link
Member

LePips commented Dec 27, 2025

I understand what this is attempting to do and think it's clever for restoring the servers and users, but this is still not the correct approach to this problem. As stated, this would still lose all of the other StoredValue data that goes to SQL and I need us to just remove tvOSs dependency on SQL entirely, rather than a solution that would keep getting its store wiped and keep needing to restore. I can't take a solution that only does server/user restoration, as I would require that prior to its next update anyways.

I've taken a look at this in #1876, where StoredValue data can go to SQL or Defaults. This might be larger than I originally anticipated, as we would then need to make changes with the manual usage of the SQL data stack.

@LePips LePips closed this Dec 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working tvOS Impacts tvOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants