Treat an item deleted mid-save as a no-op when saving its images

3 files changed, 100 insertions, 9 deletions

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<IServerApplicationHost>(),
+            NullLogger<ItemPersistenceService>.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<IDbContextFactory<JellyfinDbContext>>();
         factory.Setup(f => f.CreateDbContext()).Returns(CreateDbContext);
+        factory.Setup(f => f.CreateDbContextAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(CreateDbContext);
 
         return factory.Object;
     }