From ccdc69e3b012381f49900aa930d1dc876096cfe6 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Tue, 1 Sep 2026 20:47:39 +0200 Subject: [PATCH] Treat an item deleted mid-save as a no-op when saving its images --- .../Item/ItemPersistenceService.cs | 29 ++++--- .../ItemPersistenceServiceSaveImagesTests.cs | 77 +++++++++++++++++++ .../Item/SqliteDbTestFixture.cs | 3 + 3 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs diff --git a/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs b/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs index efff3457a3..c8672e189b 100644 --- a/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs +++ b/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs @@ -176,14 +176,6 @@ public class ItemPersistenceService : IItemPersistenceService var context = await _dbProvider.CreateDbContextAsync(cancellationToken).ConfigureAwait(false); await using (context.ConfigureAwait(false)) { - if (!await context.BaseItems - .AnyAsync(bi => bi.Id == item.Id, cancellationToken) - .ConfigureAwait(false)) - { - _logger.LogWarning("Unable to save ImageInfo for non existing BaseItem"); - return; - } - await context.BaseItemImageInfos .Where(e => e.ItemId == item.Id) .ExecuteDeleteAsync(cancellationToken) @@ -193,7 +185,26 @@ public class ItemPersistenceService : IItemPersistenceService .AddRangeAsync(images, cancellationToken) .ConfigureAwait(false); - await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + try + { + await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + } + catch (DbUpdateException) + { + // Checking that the item exists before writing leaves a gap a scan can delete it + // through, turning the insert into a foreign key violation that fails the whole + // refresh instead of the no-op intended here. Let the insert be the check: it is the + // only point at which the answer cannot go stale. Nothing is orphaned by the delete + // above, because deleting the item cascades to its images anyway. + if (await context.BaseItems + .AnyAsync(bi => bi.Id == item.Id, cancellationToken) + .ConfigureAwait(false)) + { + throw; + } + + _logger.LogWarning("Unable to save ImageInfo for non existing BaseItem {ItemId}", item.Id); + } } } diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs new file mode 100644 index 0000000000..7997c6d771 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs @@ -0,0 +1,77 @@ +using System; +using System.Linq; +using System.Threading.Tasks; +using Jellyfin.Database.Implementations.Entities; +using Jellyfin.Server.Implementations.Item; +using MediaBrowser.Controller; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Model.Entities; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Item; + +public class ItemPersistenceServiceSaveImagesTests : SqliteDbTestFixture +{ + private readonly ItemPersistenceService _service; + + public ItemPersistenceServiceSaveImagesTests() + { + _service = new ItemPersistenceService( + CreateDbContextFactory(), + Mock.Of(), + NullLogger.Instance); + } + + [Fact] + public async Task SaveImagesAsync_ReplacesThePreviousImages() + { + var itemId = Guid.NewGuid(); + Seed(itemId); + + await _service.SaveImagesAsync(CreateItem(itemId, "/first.jpg"), TestContext.Current.CancellationToken); + await _service.SaveImagesAsync(CreateItem(itemId, "/second.jpg"), TestContext.Current.CancellationToken); + + using var context = CreateDbContext(); + var paths = context.BaseItemImageInfos + .Where(e => e.ItemId.Equals(itemId)) + .Select(e => e.Path) + .ToList(); + + Assert.Equal(["/second.jpg"], paths); + } + + [Fact] + public async Task SaveImagesAsync_ItemDeletedFromUnderIt_IsANoOp() + { + // A scan can delete the item between the refresh reading it and the images being written. That + // must not fail the whole refresh, and must not leave the images of an item that is gone. + var itemId = Guid.NewGuid(); + + await _service.SaveImagesAsync(CreateItem(itemId, "/gone.jpg"), TestContext.Current.CancellationToken); + + using var context = CreateDbContext(); + Assert.Empty(context.BaseItemImageInfos.Where(e => e.ItemId.Equals(itemId))); + } + + private static BaseItem CreateItem(Guid itemId, string imagePath) + => new Folder + { + Id = itemId, + ImageInfos = [new ItemImageInfo { Path = imagePath, Type = ImageType.Primary }] + }; + + private void Seed(Guid itemId) + { + using var context = CreateDbContext(); + context.BaseItems.Add(new BaseItemEntity + { + Id = itemId, + Type = "Folder", + IsFolder = true + }); + context.SaveChanges(); + } +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs index 87efa8fea5..cfc9c9496c 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using Emby.Server.Implementations.Data; using Jellyfin.Database.Implementations; using Jellyfin.Database.Implementations.Locking; @@ -58,6 +59,8 @@ public abstract class SqliteDbTestFixture : IDisposable { var factory = new Mock>(); factory.Setup(f => f.CreateDbContext()).Returns(CreateDbContext); + factory.Setup(f => f.CreateDbContextAsync(It.IsAny())) + .ReturnsAsync(CreateDbContext); return factory.Object; }