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..a925a58291ae 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; @@ -526,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++; } @@ -1023,39 +1011,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; @@ -1086,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++; } @@ -1344,38 +1321,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; @@ -1406,15 +1367,133 @@ 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++; } } + /// + /// 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); + + if (_logger.IsEnabled(LogLevel.Debug)) + { + _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); + 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); + } + }); + } + + 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(int[] nodeIds) + { + if (nodeIds.Length == 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..878bcecf6b47 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DatabaseCacheRepositoryTests.cs @@ -1,6 +1,9 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; 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,9 +11,12 @@ 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; +using Umbraco.Cms.Tests.Integration.Attributes; using Umbraco.Cms.Tests.Integration.Testing; using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; @@ -27,8 +33,17 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) builder.Services.AddUnique(); } + // 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(); + private IDocumentCacheService DocumentCacheService => GetRequiredService(); + + private ISqlContext SqlContext => GetRequiredService(); + private IContentPublishingService ContentPublishingService => GetRequiredService(); private IMediaTypeService MediaTypeService => GetRequiredService(); @@ -107,6 +122,59 @@ public async Task GetMediaSourcesAsync_With_Empty_Keys_Returns_Nothing() Assert.That(sources, Is.Empty); } + [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 + // 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() {