From b750dde0ed202afe3f690fe6aeee52fb34ae5963 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 8 Jul 2026 15:19:31 +0200 Subject: [PATCH 1/2] Batch the content cache deletes following a structural content type update. --- .../Configuration/Models/NuCacheSettings.cs | 22 ++ .../Persistence/DatabaseCacheRepository.cs | 207 +++++++++++------- .../DatabaseCacheRepositoryTests.cs | 59 +++++ 3 files changed, 212 insertions(+), 76 deletions(-) diff --git a/src/Umbraco.Core/Configuration/Models/NuCacheSettings.cs b/src/Umbraco.Core/Configuration/Models/NuCacheSettings.cs index 3450383d5cff..c6844f6c209b 100644 --- a/src/Umbraco.Core/Configuration/Models/NuCacheSettings.cs +++ b/src/Umbraco.Core/Configuration/Models/NuCacheSettings.cs @@ -26,6 +26,12 @@ public class NuCacheSettings /// internal const bool StaticUsePagedSqlQuery = true; + /// + /// The default number of content items whose stale NuCache rows are deleted per batch during a + /// content type rebuild. Matches the SQL parameter limit ceiling the delete is capped to. + /// + internal const int StaticContentTypeRebuildDeleteBatchSize = 2000; + /// /// The serializer type that nucache uses to persist documents in the database. /// @@ -43,4 +49,20 @@ public class NuCacheSettings /// [DefaultValue(StaticUsePagedSqlQuery)] public bool UsePagedSqlQuery { get; set; } = true; + + /// + /// Gets or sets the number of content items whose stale NuCache rows are deleted per batch during a + /// content type structural rebuild. + /// + /// + /// Deleting in batches avoids a single unbounded DELETE that can escalate to a table lock, bloat the + /// transaction log, or exceed the command timeout on sites with a lot of content. The matching node ids + /// are read from the source tables once, then their rows are deleted in batches of this size, so the + /// value only bounds the per-statement (and, for a deferred rebuild, per-transaction) footprint — it + /// does not cause repeated scans. The effective size is capped at the SQL parameter limit + /// (); lower it if brief lock escalation on + /// cmsContentNu during a rebuild is a concern. + /// + [DefaultValue(StaticContentTypeRebuildDeleteBatchSize)] + public int ContentTypeRebuildDeleteBatchSize { get; set; } = StaticContentTypeRebuildDeleteBatchSize; } diff --git a/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs b/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs index 233765c30d14..1d433069793d 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs @@ -445,44 +445,26 @@ private void RebuildContentDbCache(IContentCacheDataSerializer serializer, int g Guid contentObjectType = Constants.ObjectTypes.Document; - long total = 0; Dictionary? contentTypeVariations = null; Dictionary? languageMap = null; Dictionary>? propertyInfoByContentType = null; - // Delete and pre-fetch in one step, then release locks before the paging loop. - executeStep(() => - { - RemoveByObjectType(contentObjectType, contentTypeIds); - - Sql countSql = Sql() - .SelectCount() - .From() - .InnerJoin().On((n, c) => n.NodeId == c.NodeId) - .Where(x => x.NodeObjectType == contentObjectType); - - if (contentTypeIds.Count > 0) - { - countSql = countSql.WhereIn(x => x.ContentTypeId, contentTypeIds); - } + // Delete stale rows in batches (each its own step); the returned count of affected nodes is the + // number to repopulate, so no separate count query is needed. + long total = RemoveByObjectTypeInBatches(contentObjectType, contentTypeIds, executeStep); - total = Database.ExecuteScalar(countSql); - - if (total == 0) - { - return; - } + if (total == 0) + { + return; + } + executeStep(() => + { contentTypeVariations = GetContentTypeVariations(contentTypeIds); languageMap = GetLanguageMap(); propertyInfoByContentType = GetPropertyInfoByContentType(contentTypeIds); }); - if (total == 0) - { - return; - } - long processed = 0; long pageIndex = 0; @@ -1023,39 +1005,22 @@ private void RebuildMediaDbCache(IContentCacheDataSerializer serializer, int gro Guid mediaObjectType = Constants.ObjectTypes.Media; - long total = 0; Dictionary>? propertyAliasesByContentType = null; - executeStep(() => - { - RemoveByObjectType(mediaObjectType, contentTypeIds); - - Sql countSql = Sql() - .SelectCount() - .From() - .InnerJoin().On((n, c) => n.NodeId == c.NodeId) - .Where(x => x.NodeObjectType == mediaObjectType); - - if (contentTypeIds.Count > 0) - { - countSql = countSql.WhereIn(x => x.ContentTypeId, contentTypeIds); - } - - total = Database.ExecuteScalar(countSql); - - if (total == 0) - { - return; - } - - propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds); - }); + // Delete stale rows in batches (each its own step); the returned count of affected nodes is the + // number to repopulate, so no separate count query is needed. + long total = RemoveByObjectTypeInBatches(mediaObjectType, contentTypeIds, executeStep); if (total == 0) { return; } + executeStep(() => + { + propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds); + }); + long processed = 0; long pageIndex = 0; @@ -1344,38 +1309,22 @@ private void RebuildMemberDbCache(IContentCacheDataSerializer serializer, int gr Guid memberObjectType = Constants.ObjectTypes.Member; - long total = 0; Dictionary>? propertyAliasesByContentType = null; - executeStep(() => - { - RemoveByObjectType(memberObjectType, contentTypeIds); - - Sql countSql = Sql() - .SelectCount() - .From() - .InnerJoin().On((n, c) => n.NodeId == c.NodeId) - .Where(x => x.NodeObjectType == memberObjectType); - - if (contentTypeIds.Count > 0) - { - countSql = countSql.WhereIn(x => x.ContentTypeId, contentTypeIds); - } - - total = Database.ExecuteScalar(countSql); - if (total == 0) - { - return; - } - - propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds); - }); + // Delete stale rows in batches (each its own step); the returned count of affected nodes is the + // number to repopulate, so no separate count query is needed. + long total = RemoveByObjectTypeInBatches(memberObjectType, contentTypeIds, executeStep); if (total == 0) { return; } + executeStep(() => + { + propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds); + }); + long processed = 0; long pageIndex = 0; @@ -1415,6 +1364,112 @@ private void RebuildMemberDbCache(IContentCacheDataSerializer serializer, int gr } } + /// + /// Deletes the cmsContentNu rows for the given object type in batches, running each batch through + /// the supplied delegate. + /// + /// + /// Batching avoids a single unbounded DELETE that can escalate to a table lock, bloat the transaction + /// log, and (for a content type backing a lot of content) exceed the command timeout. When the caller's + /// opens a fresh scope per step (the deferred rebuild path), each batch + /// commits in its own transaction, so the locks and log space it uses are released between batches + /// instead of accumulating across the whole delete. When it runs inline (the immediate path) the batches + /// share the ambient transaction, but each DELETE statement is still individually bounded. + /// + /// The number of content nodes affected, i.e. the number that will need repopulating. + private long RemoveByObjectTypeInBatches(Guid objectType, IReadOnlyCollection contentTypeIds, Action executeStep) + { + // A full clear (no content type filter) is only used by full rebuilds, where the table has typically + // already been truncated, so keep it as a single statement. + if (contentTypeIds.Count == 0) + { + long allCount = 0; + executeStep(() => + { + RemoveByObjectType(objectType, contentTypeIds); + allCount = CountByObjectType(objectType, contentTypeIds); + }); + return allCount; + } + + // The node ids come from the (stable) source tables, not cmsContentNu, so fetching them once up front + // is safe: concurrent foreground saves during a deferred rebuild only add/remove rows we either + // correctly skip (new rows are preserved) or harmlessly no-op on (already-removed rows). + List nodeIds = []; + executeStep(() => nodeIds = GetNodeIdsByContentTypes(objectType, contentTypeIds)); + + var batchSize = Math.Clamp(_nucacheSettings.Value.ContentTypeRebuildDeleteBatchSize, 1, Constants.Sql.MaxParameterCount); + var totalBatches = (int)Math.Ceiling(nodeIds.Count / (double)batchSize); + + _logger.LogDebug( + "Rebuild: deleting cmsContentNu rows for object type {ObjectType} — {NodeCount} node(s) in {BatchCount} batch(es) of up to {BatchSize}.", + objectType, + nodeIds.Count, + totalBatches, + batchSize); + + var batchNumber = 0; + foreach (IEnumerable batch in nodeIds.InGroupsOf(batchSize)) + { + var nodeIdBatch = batch.ToArray(); + var currentBatchNumber = ++batchNumber; + executeStep(() => + { + DeleteContentNuByNodeIds(nodeIdBatch); + _logger.LogDebug( + "Rebuild: deleted cmsContentNu batch {BatchNumber}/{BatchCount} ({NodeCount} node(s)) for object type {ObjectType}.", + currentBatchNumber, + totalBatches, + nodeIdBatch.Length, + objectType); + }); + } + + return nodeIds.Count; + } + + private List GetNodeIdsByContentTypes(Guid objectType, IReadOnlyCollection contentTypeIds) + { + Sql sql = Sql() + .Select(x => x.NodeId) + .From() + .InnerJoin().On((n, c) => n.NodeId == c.NodeId) + .Where(x => x.NodeObjectType == objectType) + .WhereIn(x => x.ContentTypeId, contentTypeIds); + + return Database.Fetch(sql); + } + + private long CountByObjectType(Guid objectType, IReadOnlyCollection contentTypeIds) + { + Sql sql = Sql() + .SelectCount() + .From() + .InnerJoin().On((n, c) => n.NodeId == c.NodeId) + .Where(x => x.NodeObjectType == objectType); + + if (contentTypeIds.Count > 0) + { + sql = sql.WhereIn(x => x.ContentTypeId, contentTypeIds); + } + + return Database.ExecuteScalar(sql); + } + + private void DeleteContentNuByNodeIds(IReadOnlyCollection nodeIds) + { + if (nodeIds.Count == 0) + { + return; + } + + Sql sql = Sql() + .Delete() + .WhereIn(x => x.NodeId, nodeIds); + + Database.Execute(sql); + } + private void RemoveByObjectType(Guid objectType, IReadOnlyCollection contentTypeIds) { // If the provided contentTypeIds collection is empty, remove all records for the provided object type. diff --git a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs index 3c9eb3123dd0..a2a0cf5e157f 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs @@ -1,6 +1,8 @@ +using Microsoft.Extensions.DependencyInjection; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Notifications; using Umbraco.Cms.Core.PublishedCache; @@ -8,6 +10,8 @@ using Umbraco.Cms.Core.Sync; using Umbraco.Cms.Infrastructure.HybridCache.Persistence; using Umbraco.Cms.Infrastructure.HybridCache.Serialization; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Builders.Extensions; using Umbraco.Cms.Tests.Common.Testing; @@ -25,10 +29,17 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) builder.AddNotificationHandler(); builder.AddNotificationHandler(); builder.Services.AddUnique(); + + // Force several delete batches over the handful of fixture documents so the batched delete loop is exercised. + builder.Services.PostConfigure(options => options.ContentTypeRebuildDeleteBatchSize = 2); } private IDatabaseCacheRepository DatabaseCacheRepository => GetRequiredService(); + private IDocumentCacheService DocumentCacheService => GetRequiredService(); + + private ISqlContext SqlContext => GetRequiredService(); + private IContentPublishingService ContentPublishingService => GetRequiredService(); private IMediaTypeService MediaTypeService => GetRequiredService(); @@ -107,6 +118,54 @@ public async Task GetMediaSourcesAsync_With_Empty_Keys_Returns_Nothing() Assert.That(sources, Is.Empty); } + [Test] + public void Rebuild_Deletes_Stale_Rows_In_Batches_And_Repopulates() + { + // Arrange — populate the cache for the document type, then mark every row for the type as stale. + // If the batched delete failed to remove a row, the repopulation's insert-where-not-exists would + // skip it and the stale marker would survive — so this proves the delete actually runs (with a + // batch size of 2 forcing multiple batches over the fixture's documents). + DocumentCacheService.Rebuild([ContentType.Id]); + + const string staleMarker = "STALE"; + var contentTypeNodeIds = new[] { Textpage.Id, Subpage.Id, Subpage2.Id, Subpage3.Id }; + + using (var scope = ScopeProvider.CreateScope()) + { + ScopeAccessor.AmbientScope!.Database.Execute( + SqlContext.Sql() + .Update(u => u.Set(x => x.Data, staleMarker)) + .WhereIn(x => x.NodeId, contentTypeNodeIds)); + scope.Complete(); + } + + // Act + DocumentCacheService.Rebuild([ContentType.Id]); + + // Assert — every document of the type has a refreshed (non-stale) row, and none was left behind. + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + var dtos = ScopeAccessor.AmbientScope!.Database.Fetch( + SqlContext.Sql() + .Select() + .From() + .WhereIn(x => x.NodeId, contentTypeNodeIds)); + + Assert.Multiple(() => + { + Assert.That(dtos, Is.Not.Empty, "Expected the cache to be repopulated for the content type."); + Assert.That( + dtos.Select(x => x.NodeId).Distinct(), + Is.EquivalentTo(contentTypeNodeIds), + "Every document of the type should have a cache row after the rebuild."); + Assert.That( + dtos.Any(x => x.Data == staleMarker), + Is.False, + "The batched delete should have removed the stale rows before repopulation."); + }); + } + } + [Test] public void GetContentByContentTypeKey_With_Keys_Returns_Matching_Nodes() { From 2255acc84ec0402ce920356ae4ce5036bd6b28be Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 9 Jul 2026 10:06:12 +0200 Subject: [PATCH 2/2] Addressed code review feedback. --- .../Persistence/DatabaseCacheRepository.cs | 52 ++++++++++++++----- .../DatabaseCacheRepositoryTests.cs | 13 ++++- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs b/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs index 1d433069793d..a925a58291ae 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs @@ -508,10 +508,16 @@ private void RebuildContentDbCache(IContentCacheDataSerializer serializer, int g pageComplete = true; }); + // The break IS reachable: the executeStep delegate's early return (a page yielding no nodes — e.g. + // content removed concurrently, leaving the total row count stale) leaves pageComplete false, which + // guards against looping indefinitely. SonarLint cannot see through the delegate, so it incorrectly + // reports the condition as always false. +#pragma warning disable S2583 // Conditionally executed code should be reachable if (!pageComplete) { break; } +#pragma warning restore S2583 pageIndex++; } @@ -1051,10 +1057,16 @@ private void RebuildMediaDbCache(IContentCacheDataSerializer serializer, int gro pageComplete = true; }); + // The break IS reachable: the executeStep delegate's early return (a page yielding no nodes — e.g. + // content removed concurrently, leaving the total row count stale) leaves pageComplete false, which + // guards against looping indefinitely. SonarLint cannot see through the delegate, so it incorrectly + // reports the condition as always false. +#pragma warning disable S2583 // Conditionally executed code should be reachable if (!pageComplete) { break; } +#pragma warning restore S2583 pageIndex++; } @@ -1355,10 +1367,16 @@ private void RebuildMemberDbCache(IContentCacheDataSerializer serializer, int gr pageComplete = true; }); + // The break IS reachable: the executeStep delegate's early return (a page yielding no nodes — e.g. + // content removed concurrently, leaving the total row count stale) leaves pageComplete false, which + // guards against looping indefinitely. SonarLint cannot see through the delegate, so it incorrectly + // reports the condition as always false. +#pragma warning disable S2583 // Conditionally executed code should be reachable if (!pageComplete) { break; } +#pragma warning restore S2583 pageIndex++; } @@ -1401,12 +1419,15 @@ private long RemoveByObjectTypeInBatches(Guid objectType, IReadOnlyCollection batch in nodeIds.InGroupsOf(batchSize)) @@ -1416,12 +1437,15 @@ private long RemoveByObjectTypeInBatches(Guid objectType, IReadOnlyCollection { DeleteContentNuByNodeIds(nodeIdBatch); - _logger.LogDebug( - "Rebuild: deleted cmsContentNu batch {BatchNumber}/{BatchCount} ({NodeCount} node(s)) for object type {ObjectType}.", - currentBatchNumber, - totalBatches, - nodeIdBatch.Length, - objectType); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug( + "Rebuild: deleted cmsContentNu batch {BatchNumber}/{BatchCount} ({NodeCount} node(s)) for object type {ObjectType}.", + currentBatchNumber, + totalBatches, + nodeIdBatch.Length, + objectType); + } }); } @@ -1456,9 +1480,9 @@ private long CountByObjectType(Guid objectType, IReadOnlyCollection content return Database.ExecuteScalar(sql); } - private void DeleteContentNuByNodeIds(IReadOnlyCollection nodeIds) + private void DeleteContentNuByNodeIds(int[] nodeIds) { - if (nodeIds.Count == 0) + if (nodeIds.Length == 0) { return; } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs index a2a0cf5e157f..878bcecf6b47 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; @@ -15,6 +16,7 @@ using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Builders.Extensions; using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Attributes; using Umbraco.Cms.Tests.Integration.Testing; using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; @@ -29,10 +31,12 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) builder.AddNotificationHandler(); builder.AddNotificationHandler(); builder.Services.AddUnique(); + } - // Force several delete batches over the handful of fixture documents so the batched delete loop is exercised. + // Applied via [ConfigureBuilder] to only the batched-delete test, so the tiny batch size doesn't change the + // number of SQL statements other tests in this fixture issue. + public static void ConfigureSmallDeleteBatchSize(IUmbracoBuilder builder) => builder.Services.PostConfigure(options => options.ContentTypeRebuildDeleteBatchSize = 2); - } private IDatabaseCacheRepository DatabaseCacheRepository => GetRequiredService(); @@ -119,8 +123,13 @@ public async Task GetMediaSourcesAsync_With_Empty_Keys_Returns_Nothing() } [Test] + [ConfigureBuilder(ActionName = nameof(ConfigureSmallDeleteBatchSize))] public void Rebuild_Deletes_Stale_Rows_In_Batches_And_Repopulates() { + // Guard: the [ConfigureBuilder] override must be in effect, otherwise the default (large) batch size + // would delete every row in one statement and this test would no longer exercise the batching loop. + Assert.That(GetRequiredService>().Value.ContentTypeRebuildDeleteBatchSize, Is.EqualTo(2)); + // Arrange — populate the cache for the document type, then mark every row for the type as stale. // If the batched delete failed to remove a row, the repopulation's insert-where-not-exists would // skip it and the stale marker would survive — so this proves the delete actually runs (with a