Fix: Update of inventory could override app settings being updated (#5565)

BTCPayServer.Data/ApplicationDbContext.cs

@@ -93,7 +93,7 @@ namespace BTCPayServer.Data
             ApplicationUser.OnModelCreating(builder, Database);
             AddressInvoiceData.OnModelCreating(builder);
             APIKeyData.OnModelCreating(builder, Database);
-            AppData.OnModelCreating(builder);
+            AppData.OnModelCreating(builder, Database);
             CustodianAccountData.OnModelCreating(builder, Database);
             //StoredFile.OnModelCreating(builder);
             InvoiceEventData.OnModelCreating(builder);

BTCPayServer.Data/Data/AppData.cs

@@ -1,5 +1,6 @@
 using System;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Newtonsoft.Json;
 
 namespace BTCPayServer.Data
@@ -16,13 +17,20 @@ namespace BTCPayServer.Data
         public string Settings { get; set; }
         public bool Archived { get; set; }
 
-        internal static void OnModelCreating(ModelBuilder builder)
+        internal static void OnModelCreating(ModelBuilder builder, DatabaseFacade databaseFacade)
         {
             builder.Entity<AppData>()
                        .HasOne(o => o.StoreData)
                        .WithMany(i => i.Apps).OnDelete(DeleteBehavior.Cascade);
             builder.Entity<AppData>()
                     .HasOne(a => a.StoreData);
+
+            if (databaseFacade.IsNpgsql())
+            {
+                builder.Entity<AppData>()
+                    .Property(o => o.Settings)
+                    .HasColumnType("JSONB");
+            }
         }
 
         // utility methods

BTCPayServer.Data/Migrations/20231219031609_appssettingstojson.cs

@@ -0,0 +1,26 @@
+using BTCPayServer.Data;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Migrations;
+
+#nullable disable
+
+namespace BTCPayServer.Migrations
+{
+    [DbContext(typeof(ApplicationDbContext))]
+    [Migration("20231219031609_appssettingstojson")]
+    public partial class appssettingstojson : Migration
+    {
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            if (migrationBuilder.IsNpgsql())
+            {
+                migrationBuilder.Sql("ALTER TABLE \"Apps\" ALTER COLUMN \"Settings\" TYPE JSONB USING \"Settings\"::JSONB");
+            }
+        }
+
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+        }
+    }
+}

BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs

@@ -1,4 +1,4 @@
-// <auto-generated />
+// <auto-generated />
 using System;
 using BTCPayServer.Data;
 using Microsoft.EntityFrameworkCore;
@@ -89,7 +89,7 @@ namespace BTCPayServer.Migrations
                         .HasColumnType("TEXT");
 
                     b.Property<string>("Settings")
-                        .HasColumnType("TEXT");
+                        .HasColumnType("JSONB");
 
                     b.Property<string>("StoreDataId")
                         .HasColumnType("TEXT");

BTCPayServer.Tests/AltcoinTests/AltcoinTests.cs

@@ -5,6 +5,7 @@ using System.Linq;
 using System.Threading.Tasks;
 using BTCPayServer.Controllers;
 using BTCPayServer.Data;
+using BTCPayServer.Events;
 using BTCPayServer.HostedServices;
 using BTCPayServer.Hosting;
 using BTCPayServer.Lightning;
@@ -757,24 +758,26 @@ noninventoryitem:
                 vmpos.Template = AppService.SerializeTemplate(MigrationStartupTask.ParsePOSYML(vmpos.Template));
                 Assert.IsType<RedirectToActionResult>(pos.UpdatePointOfSale(app.Id, vmpos).Result);
 
-                //inventoryitem has 1 item available
+                async Task AssertCanBuy(string choiceKey, bool expected)
-                await tester.WaitForEvent<AppInventoryUpdaterHostedService.UpdateAppInventory>(() =>
                 {
-                    Assert.IsType<RedirectToActionResult>(publicApps
+                    var redirect = Assert.IsType<RedirectToActionResult>(await publicApps
-                        .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "inventoryitem").Result);
+                        .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: choiceKey));
-                    return Task.CompletedTask;
+                    if (expected)
-                });
+                        Assert.Equal("UIInvoice", redirect.ControllerName);
+                    else
+                        Assert.NotEqual("UIInvoice", redirect.ControllerName);
+                }
+
+                //inventoryitem has 1 item available
+                await AssertCanBuy("inventoryitem", true);
 
                 //we already bought all available stock so this should fail
                 await Task.Delay(100);
-                Assert.IsType<RedirectToActionResult>(publicApps
+                await AssertCanBuy("inventoryitem", false);
-                    .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "inventoryitem").Result);
 
                 //inventoryitem has unlimited items available
-                Assert.IsType<RedirectToActionResult>(publicApps
+                await AssertCanBuy("noninventoryitem", true);
-                    .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "noninventoryitem").Result);
+                await AssertCanBuy("noninventoryitem", true);
-                Assert.IsType<RedirectToActionResult>(publicApps
-                    .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "noninventoryitem").Result);
 
                 //verify invoices where created
                 invoices = user.BitPay.GetInvoices();
@@ -808,34 +811,8 @@ normal:
   price: 1.0";
                 vmpos.Template = AppService.SerializeTemplate(MigrationStartupTask.ParsePOSYML(vmpos.Template));
                 Assert.IsType<RedirectToActionResult>(pos.UpdatePointOfSale(app.Id, vmpos).Result);
-                try
+                Assert.IsType<RedirectToActionResult>(publicApps
-                {
+                    .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result);
-                    Assert.IsType<RedirectToActionResult>(publicApps
-                        .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result);
-                }
-                catch (IsTypeException)
-                {
-                    TestLogs.LogInformation("This test sometimes fails, so we try to find the issue here...");
-                    TestLogs.LogInformation("Template: " + vmpos.Template);
-                    var retryOk = publicApps.ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result is RedirectToActionResult;
-                    var noChoiceKey = publicApps.ViewPointOfSale(app.Id, PosViewType.Cart, 1).Result is RedirectToActionResult;
-                    TestLogs.LogInformation("RetryOk: " + retryOk);
-                    TestLogs.LogInformation("NoChoiceKey: " + retryOk);
-                    var appService = tester.PayTester.GetService<AppService>();
-                    var found = await appService.GetApp(app.Id, PointOfSaleAppType.AppType);
-                    TestLogs.LogInformation("Found: " + (found != null));
-                    if (found is not null)
-                    {
-                        var settings = found.GetSettings<PointOfSaleSettings>();
-                        TestLogs.LogInformation("settings Found: " + (settings != null));
-                        if (settings is not null)
-                        {
-                            TestLogs.LogInformation("template Found: " + (settings.Template));
-                            TestLogs.LogInformation("parsed template Found: " + (AppService.SerializeTemplate(AppService.Parse(settings.Template, false))));
-                        }
-                    }
-                    throw;
-                }
                 Assert.IsType<RedirectToActionResult>(publicApps
                     .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "normal").Result);
                 invoices = user.BitPay.GetInvoices();

BTCPayServer/HostedServices/AppInventoryUpdaterHostedService.cs

@@ -20,7 +20,6 @@ namespace BTCPayServer.HostedServices
         protected override void SubscribeToEvents()
         {
             Subscribe<InvoiceEvent>();
-            Subscribe<UpdateAppInventory>();
         }
 
         public AppInventoryUpdaterHostedService(EventAggregator eventAggregator, AppService appService, Logs logs) : base(eventAggregator, logs)
@@ -31,77 +30,18 @@ namespace BTCPayServer.HostedServices
 
         protected override async Task ProcessEvent(object evt, CancellationToken cancellationToken)
         {
-            if (evt is UpdateAppInventory updateAppInventory)
+            if (evt is InvoiceEvent invoiceEvent)
-            {
-                //get all apps that were tagged that have manageable inventory that has an item that matches the item code in the invoice
-                var apps = (await _appService.GetApps(updateAppInventory.AppId)).Select(data =>
-                    {
-                        switch (data.AppType)
-                        {
-                            case PointOfSaleAppType.AppType:
-                                var possettings = data.GetSettings<PointOfSaleSettings>();
-                                return (Data: data, Settings: (object)possettings,
-                                    Items: AppService.Parse(possettings.Template));
-                            case CrowdfundAppType.AppType:
-                                var cfsettings = data.GetSettings<CrowdfundSettings>();
-                                return (Data: data, Settings: (object)cfsettings,
-                                    Items: AppService.Parse(cfsettings.PerksTemplate));
-                            default:
-                                return (null, null, null);
-                        }
-                    }).Where(tuple => tuple.Data != null && tuple.Items.Any(item =>
-                                          item.Inventory.HasValue &&
-                                          updateAppInventory.Items.FirstOrDefault(i => i.Id == item.Id) != null));
-                foreach (var app in apps)
-                {
-                    foreach (var cartItem in updateAppInventory.Items)
-                    {
-                        var item = app.Items.FirstOrDefault(item => item.Id == cartItem.Id);
-                        if (item == null) continue;
-                        
-                        if (updateAppInventory.Deduct)
-                        {
-                            item.Inventory -= cartItem.Count;
-                        }
-                        else
-                        {
-                            item.Inventory += cartItem.Count;
-                        }
-                    }
-
-                    switch (app.Data.AppType)
-                    {
-                        case PointOfSaleAppType.AppType:
-                            ((PointOfSaleSettings)app.Settings).Template =
-                                AppService.SerializeTemplate(app.Items);
-                            break;
-                        case CrowdfundAppType.AppType:
-                            ((CrowdfundSettings)app.Settings).PerksTemplate =
-                                AppService.SerializeTemplate(app.Items);
-                            break;
-                        default:
-                            throw new InvalidOperationException();
-                    }
-
-                    app.Data.SetSettings(app.Settings);
-                    await _appService.UpdateOrCreateApp(app.Data);
-                }
-
-
-            }
-            else if (evt is InvoiceEvent invoiceEvent)
             {
                 List<PosCartItem> cartItems = null;
-                bool deduct;
+                int deduct;
                 switch (invoiceEvent.Name)
                 {
                     case InvoiceEvent.Expired:
-
                     case InvoiceEvent.MarkedInvalid:
-                        deduct = false;
+                        deduct = 1;
                         break;
                     case InvoiceEvent.Created:
-                        deduct = true;
+                        deduct = -1;
                         break;
                     default:
                         return;
@@ -112,11 +52,6 @@ namespace BTCPayServer.HostedServices
                 {
                     var appIds = AppService.GetAppInternalTags(invoiceEvent.Invoice);
 
-                    if (!appIds.Any())
-                    {
-                        return;
-                    }
-
                     var items = cartItems?.ToList() ?? new List<PosCartItem>();
                     if (!string.IsNullOrEmpty(invoiceEvent.Invoice.Metadata.ItemCode))
                     {
@@ -128,27 +63,13 @@ namespace BTCPayServer.HostedServices
                         });
                     }
 
-                    _eventAggregator.Publish(new UpdateAppInventory
+                    var changes = items.Select(i => new AppService.InventoryChange(i.Id, i.Count * deduct)).ToArray();
+                    foreach (var appId in appIds)
                     {
-                        Deduct = deduct,
+                        await _appService.UpdateInventory(appId, changes);
-                        Items = items,
+                    }
-                        AppId = appIds
-                    });
-
                 }
             }
         }
-
-        public class UpdateAppInventory
-        {
-            public string[] AppId { get; set; }
-            public List<PosCartItem> Items { get; set; }
-            public bool Deduct { get; set; }
-
-            public override string ToString()
-            {
-                return string.Empty;
-            }
-        }
     }
 }

BTCPayServer/Plugins/PluginService.cs

@@ -37,7 +37,7 @@ namespace BTCPayServer.Plugins
         public IEnumerable<IBTCPayServerPlugin> LoadedPlugins { get; }
         public BTCPayServerEnvironment Env { get; }
         
-        public Version? GetVersionOfPendingInstall(string plugin)
+        public Version GetVersionOfPendingInstall(string plugin)
         {
             var dirName = Path.Combine(_dataDirectories.Value.PluginDir, plugin);
             var manifestFileName = dirName + ".json";

BTCPayServer/Services/Apps/AppService.cs

@@ -15,6 +15,7 @@ using BTCPayServer.Plugins.PointOfSale.Models;
 using BTCPayServer.Services.Invoices;
 using BTCPayServer.Services.Rates;
 using BTCPayServer.Services.Stores;
+using Dapper;
 using Ganss.Xss;
 using Microsoft.EntityFrameworkCore;
 using NBitcoin;
@@ -46,6 +47,7 @@ namespace BTCPayServer.Services.Apps
         private readonly StoreRepository _storeRepository;
         private readonly HtmlSanitizer _HtmlSanitizer;
         public CurrencyNameTable Currencies => _Currencies;
+
         public AppService(
             IEnumerable<AppBaseType> apps,
             ApplicationDbContextFactory contextFactory,
@@ -380,6 +382,42 @@ namespace BTCPayServer.Services.Apps
             return app;
         }
 
+        record AppSettingsWithXmin(string apptype, string settings, uint xmin);
+        public record InventoryChange(string ItemId, int Delta);
+        public async Task UpdateInventory(string appId, InventoryChange[] changes)
+        {
+            await using var ctx = _ContextFactory.CreateContext();
+            // We use xmin to make sure we don't override changes made by another process
+retry:
+            var connection = ctx.Database.GetDbConnection();
+            var row = connection.QueryFirstOrDefault<AppSettingsWithXmin>(
+                "SELECT \"AppType\" AS apptype, \"Settings\" AS settings, xmin FROM \"Apps\" WHERE \"Id\"=@appId", new { appId }
+                );
+            if (row?.settings is null)
+                return;
+            var templatePath = row.apptype switch
+            {
+                CrowdfundAppType.AppType => "PerksTemplate",
+                _ => "Template"
+            };
+            var settings = JObject.Parse(row.settings);
+            var items = JArray.Parse(settings[templatePath]!.Value<string>()!);
+            foreach (var change in changes)
+            {
+                var item = items.FirstOrDefault(i => i["id"]?.Value<string>() == change.ItemId && i["inventory"] is not null);
+                if (item is null)
+                    continue;
+                var inventory = item["inventory"]!.Value<int>();
+                inventory += change.Delta;
+                item["inventory"] = inventory;
+            }
+            settings[templatePath] = items.ToString(Formatting.None);
+            var updated = await connection.ExecuteAsync("UPDATE \"Apps\" SET \"Settings\"=@v::JSONB WHERE \"Id\"=@appId AND xmin=@xmin", new { appId, xmin = (int)row.xmin, v = settings.ToString(Formatting.None) }) == 1;
+            // If we can't update, it means someone else updated the row, so we need to retry
+            if (!updated)
+                goto retry;
+        }
+
         public async Task UpdateOrCreateApp(AppData app)
         {
             await using var ctx = _ContextFactory.CreateContext();

Changelog.md

@@ -31,6 +31,7 @@
 * Do not activate Blazor in Wizard screens (#5435) @NicolasDorier
 * Pull Payment: Display the amount of claims (#5427) @NicolasDorier
 * Dashboard: LND limbo balance had the wrong unit (a 1 BTC limbo balance would show as 0.001 BTC) @NicolasDorier
+* Fix occasional concurrency issue which would result in app settings change not being properly saved (#5565) @NicolasDorier
 
 ### Improvements