From 95a993db298c41c613553adafd21e185b1fdfe66 Mon Sep 17 00:00:00 2001 From: Marti Markov Date: Thu, 1 Oct 2026 18:35:41 +0100 Subject: [PATCH 1/3] Replace custom item display preferences in one transaction --- .../Users/DisplayPreferencesManager.cs | 2 + ...splayPreferencesManagerFailedWriteTests.cs | 76 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs diff --git a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs index 0e126fe9a0..578119f98e 100644 --- a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs +++ b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs @@ -84,6 +84,7 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager public void SetCustomItemDisplayPreferences(Guid userId, Guid itemId, string client, Dictionary customPreferences) { using var dbContext = _dbContextFactory.CreateDbContext(); + using var transaction = dbContext.Database.BeginTransaction(); dbContext.CustomItemDisplayPreferences.Where(prefs => prefs.UserId.Equals(userId) && prefs.ItemId.Equals(itemId) && prefs.Client == client) @@ -96,6 +97,7 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager } dbContext.SaveChanges(); + transaction.Commit(); } /// diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs new file mode 100644 index 0000000000..70c4b29508 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs @@ -0,0 +1,76 @@ +using System; +using System.Collections.Generic; +using System.Data.Common; +using Jellyfin.Server.Implementations.Tests.Item; +using Jellyfin.Server.Implementations.Users; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Users; + +public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixture +{ + private const string Client = "client"; + + private static readonly Guid _userId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"); + private static readonly Guid _itemId = Guid.Parse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"); + + private readonly InsertFailer _insertFailer; + private readonly DisplayPreferencesManager _manager; + + public DisplayPreferencesManagerFailedWriteTests() + : this(new InsertFailer()) + { + } + + private DisplayPreferencesManagerFailedWriteTests(InsertFailer insertFailer) + : base(insertFailer) + { + _insertFailer = insertFailer; + _manager = new DisplayPreferencesManager(CreateDbContextFactory()); + } + + [Fact] + public void SetCustomItemDisplayPreferences_InsertFails_KeepsThePreviousPreferences() + { + _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["first"] = "1" }); + + _insertFailer.FailNextInsert = true; + Assert.Throws(() => _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["second"] = "2" })); + + Assert.Equal( + new Dictionary { ["first"] = "1" }, + _manager.ListCustomItemDisplayPreferences(_userId, _itemId, Client)); + } + + /// + /// Fails the next insert into CustomItemDisplayPreferences with the error SQLite reports when the unique index rejects a row. + /// + private sealed class InsertFailer : DbCommandInterceptor + { + public bool FailNextInsert { get; set; } + + public override InterceptionResult ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult result) + { + FailInsert(command); + return result; + } + + public override InterceptionResult NonQueryExecuting(DbCommand command, CommandEventData eventData, InterceptionResult result) + { + FailInsert(command); + return result; + } + + private void FailInsert(DbCommand command) + { + if (FailNextInsert && command.CommandText.StartsWith("INSERT INTO \"CustomItemDisplayPreferences\"", StringComparison.Ordinal)) + { + FailNextInsert = false; + throw new SqliteException("SQLite Error 19: 'UNIQUE constraint failed: CustomItemDisplayPreferences.UserId, CustomItemDisplayPreferences.ItemId, CustomItemDisplayPreferences.Client, CustomItemDisplayPreferences.Key'.", 19); + } + } + } +} From 83f5790d6e6b2eea5765b043582e36087d0b7c4c Mon Sep 17 00:00:00 2001 From: Marti Markov Date: Mon, 5 Oct 2026 18:28:56 +0100 Subject: [PATCH 2/3] Return the stored display preferences when another request created them first --- .../Users/DisplayPreferencesManager.cs | 35 +++++++-- ...PreferencesManagerConcurrentCreateTests.cs | 77 +++++++++++++++++++ 2 files changed, 104 insertions(+), 8 deletions(-) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerConcurrentCreateTests.cs diff --git a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs index 578119f98e..87f6b6d139 100644 --- a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs +++ b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs @@ -28,17 +28,31 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager public DisplayPreferences GetDisplayPreferences(Guid userId, Guid itemId, string client) { using var dbContext = _dbContextFactory.CreateDbContext(); - var prefs = dbContext.DisplayPreferences - .Include(pref => pref.HomeSections) - .FirstOrDefault(pref => - pref.UserId.Equals(userId) && pref.Client == client && pref.ItemId.Equals(itemId)); - - if (prefs is null) + var prefs = FindDisplayPreferences(dbContext, userId, itemId, client); + if (prefs is not null) + { + return prefs; + } + + prefs = new DisplayPreferences(userId, itemId, client); + dbContext.DisplayPreferences.Add(prefs); + try { - prefs = new DisplayPreferences(userId, itemId, client); - dbContext.DisplayPreferences.Add(prefs); dbContext.SaveChanges(); } + catch (DbUpdateException) + { + // Another request may have stored the preferences between the lookup and the insert, and the unique index + // rejected this one. Return the stored preferences; if there are none, the insert failed for another reason. + using var retryContext = _dbContextFactory.CreateDbContext(); + var stored = FindDisplayPreferences(retryContext, userId, itemId, client); + if (stored is null) + { + throw; + } + + return stored; + } return prefs; } @@ -115,4 +129,9 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager dbContext.ItemDisplayPreferences.Attach(itemDisplayPreferences).State = EntityState.Modified; dbContext.SaveChanges(); } + + private static DisplayPreferences? FindDisplayPreferences(JellyfinDbContext dbContext, Guid userId, Guid itemId, string client) + => dbContext.DisplayPreferences + .Include(pref => pref.HomeSections) + .FirstOrDefault(pref => pref.UserId.Equals(userId) && pref.Client == client && pref.ItemId.Equals(itemId)); } diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerConcurrentCreateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerConcurrentCreateTests.cs new file mode 100644 index 0000000000..72ca680cf2 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerConcurrentCreateTests.cs @@ -0,0 +1,77 @@ +using System; +using Jellyfin.Database.Implementations.Entities; +using Jellyfin.Server.Implementations.Tests.Item; +using Jellyfin.Server.Implementations.Users; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Users; + +/// +/// Two first-ever requests for the same preferences can both find nothing and both insert; the unique index lets one +/// through. The other has to return the stored preferences instead of failing. +/// +public sealed class DisplayPreferencesManagerConcurrentCreateTests : SqliteDbTestFixture +{ + private const string Client = "client"; + + private static readonly Guid _userId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"); + private static readonly Guid _itemId = Guid.Parse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"); + + private readonly CompetingInsert _competingInsert; + private readonly DisplayPreferencesManager _manager; + + public DisplayPreferencesManagerConcurrentCreateTests() + : this(new CompetingInsert()) + { + } + + private DisplayPreferencesManagerConcurrentCreateTests(CompetingInsert competingInsert) + : base(competingInsert) + { + _competingInsert = competingInsert; + _manager = new DisplayPreferencesManager(CreateDbContextFactory()); + + using var context = CreateDbContext(); + context.Users.Add(new User("user", "auth-provider", "reset-provider") { Id = _userId }); + context.SaveChanges(); + } + + [Fact] + public void GetDisplayPreferences_AnotherRequestStoresThemFirst_ReturnsTheStoredPreferences() + { + var storedId = 0; + _competingInsert.Before = () => + { + using var context = CreateDbContext(); + var stored = new DisplayPreferences(_userId, _itemId, Client); + context.DisplayPreferences.Add(stored); + context.SaveChanges(); + storedId = stored.Id; + }; + + var preferences = _manager.GetDisplayPreferences(_userId, _itemId, Client); + + Assert.NotEqual(0, storedId); + Assert.Equal(storedId, preferences.Id); + using var check = CreateDbContext(); + Assert.Single(check.DisplayPreferences); + } + + /// + /// Stores the other request's row just before the next save, after the manager has looked for the preferences. + /// + private sealed class CompetingInsert : SaveChangesInterceptor + { + public Action? Before { get; set; } + + public override InterceptionResult SavingChanges(DbContextEventData eventData, InterceptionResult result) + { + // Disarmed first, because the competing save goes through this interceptor as well. + var before = Before; + Before = null; + before?.Invoke(); + return result; + } + } +} From f4207fb70bd4ab3bd36a0a2b54599102a6e1128a Mon Sep 17 00:00:00 2001 From: Marti Markov Date: Mon, 5 Oct 2026 19:31:08 +0100 Subject: [PATCH 3/3] Repeat the custom display preferences replace when its save is rejected --- .../Users/DisplayPreferencesManager.cs | 42 +++++++++++----- ...splayPreferencesManagerFailedWriteTests.cs | 49 ++++++++++++++----- 2 files changed, 65 insertions(+), 26 deletions(-) diff --git a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs index 87f6b6d139..dc3c04e63e 100644 --- a/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs +++ b/Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs @@ -13,6 +13,8 @@ namespace Jellyfin.Server.Implementations.Users; /// public sealed class DisplayPreferencesManager : IDisplayPreferencesManager { + private const int MaxSaveAttempts = 3; + private readonly IDbContextFactory _dbContextFactory; /// @@ -97,21 +99,35 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager /// public void SetCustomItemDisplayPreferences(Guid userId, Guid itemId, string client, Dictionary customPreferences) { - using var dbContext = _dbContextFactory.CreateDbContext(); - using var transaction = dbContext.Database.BeginTransaction(); - dbContext.CustomItemDisplayPreferences.Where(prefs => prefs.UserId.Equals(userId) - && prefs.ItemId.Equals(itemId) - && prefs.Client == client) - .ExecuteDelete(); - - foreach (var (key, value) in customPreferences) + // Another request can store one of these keys after this one's delete, and the unique index then rejects the + // insert. Replacing the set again gives the same result, so the replace is repeated. + for (var attempt = 1; ; attempt++) { - dbContext.CustomItemDisplayPreferences - .Add(new CustomItemDisplayPreferences(userId, itemId, client, key, value)); - } + using var dbContext = _dbContextFactory.CreateDbContext(); + using var transaction = dbContext.Database.BeginTransaction(); + dbContext.CustomItemDisplayPreferences.Where(prefs => prefs.UserId.Equals(userId) + && prefs.ItemId.Equals(itemId) + && prefs.Client == client) + .ExecuteDelete(); - dbContext.SaveChanges(); - transaction.Commit(); + foreach (var (key, value) in customPreferences) + { + dbContext.CustomItemDisplayPreferences + .Add(new CustomItemDisplayPreferences(userId, itemId, client, key, value)); + } + + try + { + dbContext.SaveChanges(); + } + catch (DbUpdateException) when (attempt < MaxSaveAttempts) + { + continue; + } + + transaction.Commit(); + return; + } } /// diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs index 70c4b29508..13688c7574 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs @@ -17,18 +17,18 @@ public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixt private static readonly Guid _userId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"); private static readonly Guid _itemId = Guid.Parse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"); - private readonly InsertFailer _insertFailer; + private readonly StatementInterceptor _statements; private readonly DisplayPreferencesManager _manager; public DisplayPreferencesManagerFailedWriteTests() - : this(new InsertFailer()) + : this(new StatementInterceptor()) { } - private DisplayPreferencesManagerFailedWriteTests(InsertFailer insertFailer) - : base(insertFailer) + private DisplayPreferencesManagerFailedWriteTests(StatementInterceptor statements) + : base(statements) { - _insertFailer = insertFailer; + _statements = statements; _manager = new DisplayPreferencesManager(CreateDbContextFactory()); } @@ -37,7 +37,7 @@ public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixt { _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["first"] = "1" }); - _insertFailer.FailNextInsert = true; + _statements.FailInserts = true; Assert.Throws(() => _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["second"] = "2" })); Assert.Equal( @@ -45,12 +45,30 @@ public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixt _manager.ListCustomItemDisplayPreferences(_userId, _itemId, Client)); } - /// - /// Fails the next insert into CustomItemDisplayPreferences with the error SQLite reports when the unique index rejects a row. - /// - private sealed class InsertFailer : DbCommandInterceptor + [Fact] + public void SetCustomItemDisplayPreferences_AnotherRequestStoredAKeyAfterTheDelete_StoresThePreferencesItWasGiven() { - public bool FailNextInsert { get; set; } + _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["first"] = "1" }); + + // The delete misses the row, as it does when another request stores it after this one's delete, so the unique + // index rejects this request's insert. + _statements.SkipNextDelete = true; + _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary { ["first"] = "2" }); + + Assert.Equal( + new Dictionary { ["first"] = "2" }, + _manager.ListCustomItemDisplayPreferences(_userId, _itemId, Client)); + } + + /// + /// Fails inserts into CustomItemDisplayPreferences with the error SQLite reports when the unique index rejects a row, or + /// leaves out the next delete from it. + /// + private sealed class StatementInterceptor : DbCommandInterceptor + { + public bool FailInserts { get; set; } + + public bool SkipNextDelete { get; set; } public override InterceptionResult ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult result) { @@ -60,15 +78,20 @@ public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixt public override InterceptionResult NonQueryExecuting(DbCommand command, CommandEventData eventData, InterceptionResult result) { + if (SkipNextDelete && command.CommandText.StartsWith("DELETE FROM \"CustomItemDisplayPreferences\"", StringComparison.Ordinal)) + { + SkipNextDelete = false; + return InterceptionResult.SuppressWithResult(0); + } + FailInsert(command); return result; } private void FailInsert(DbCommand command) { - if (FailNextInsert && command.CommandText.StartsWith("INSERT INTO \"CustomItemDisplayPreferences\"", StringComparison.Ordinal)) + if (FailInserts && command.CommandText.StartsWith("INSERT INTO \"CustomItemDisplayPreferences\"", StringComparison.Ordinal)) { - FailNextInsert = false; throw new SqliteException("SQLite Error 19: 'UNIQUE constraint failed: CustomItemDisplayPreferences.UserId, CustomItemDisplayPreferences.ItemId, CustomItemDisplayPreferences.Client, CustomItemDisplayPreferences.Key'.", 19); } }