Hybrid Cache: Resolve start-up errors with mis-matched types (#20554)

* Be consistent in use of GetOrCreateAsync overload in exists and retrieval.
Ensure nullability of ContentCacheNode is consistent in exists and retrieval.

* Applied suggestion from code review.

* Move seeding to Umbraco application starting rather than started, ensuring an initial request is served.

* Tighten up hybrid cache exists check with locking around check and remove, and use of cancellation token.

Author: AndyButland

src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs

@@ -1,4 +1,3 @@
-
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration.Models;
@@ -74,7 +73,7 @@ public static IUmbracoBuilder AddUmbracoHybridCache(this IUmbracoBuilder builder
         builder.AddNotificationAsyncHandler<ContentTypeDeletedNotification, CacheRefreshingNotificationHandler>();
         builder.AddNotificationAsyncHandler<MediaTypeRefreshedNotification, CacheRefreshingNotificationHandler>();
         builder.AddNotificationAsyncHandler<MediaTypeDeletedNotification, CacheRefreshingNotificationHandler>();
-        builder.AddNotificationAsyncHandler<UmbracoApplicationStartedNotification, SeedingNotificationHandler>();
+        builder.AddNotificationAsyncHandler<UmbracoApplicationStartingNotification, SeedingNotificationHandler>();
         builder.AddCacheSeeding();
         return builder;
     }

src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs

@@ -1,3 +1,4 @@
+using System.Collections.Concurrent;
 using Microsoft.Extensions.Caching.Hybrid;
 
 namespace Umbraco.Cms.Infrastructure.HybridCache.Extensions;
@@ -7,19 +8,24 @@ namespace Umbraco.Cms.Infrastructure.HybridCache.Extensions;
 /// </summary>
 internal static class HybridCacheExtensions
 {
+    // Per-key semaphores to ensure the GetOrCreateAsync + RemoveAsync sequence
+    // executes atomically for a given cache key.
+    private static readonly ConcurrentDictionary<string, SemaphoreSlim> _keyLocks = new();
+
     /// <summary>
     /// Returns true if the cache contains an item with a matching key.
     /// </summary>
     /// <param name="cache">An instance of <see cref="Microsoft.Extensions.Caching.Hybrid.HybridCache"/></param>
     /// <param name="key">The name (key) of the item to search for in the cache.</param>
+    /// <param name="token">The cancellation token.</param>
     /// <returns>True if the item exists already. False if it doesn't.</returns>
     /// <remarks>
     /// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191
     /// Will never add or alter the state of any items in the cache.
     /// </remarks>
-    public static async Task<bool> ExistsAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key)
+    public static async Task<bool> ExistsAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token)
     {
-        (bool exists, _) = await TryGetValueAsync<T>(cache, key);
+        (bool exists, _) = await TryGetValueAsync<T>(cache, key, token).ConfigureAwait(false);
         return exists;
     }
 
@@ -29,34 +35,55 @@ public static async Task<bool> ExistsAsync<T>(this Microsoft.Extensions.Caching.
     /// <typeparam name="T">The type of the value of the item in the cache.</typeparam>
     /// <param name="cache">An instance of <see cref="Microsoft.Extensions.Caching.Hybrid.HybridCache"/></param>
     /// <param name="key">The name (key) of the item to search for in the cache.</param>
+    /// <param name="token">The cancellation token.</param>
     /// <returns>A tuple of <see cref="bool"/> and the object (if found) retrieved from the cache.</returns>
     /// <remarks>
     /// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191
     /// Will never add or alter the state of any items in the cache.
     /// </remarks>
-    public static async Task<(bool Exists, T? Value)> TryGetValueAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key)
+    public static async Task<(bool Exists, T? Value)> TryGetValueAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token)
     {
         var exists = true;
 
-        T? result = await cache.GetOrCreateAsync<object, T>(
-            key,
-            null!,
-            (_, _) =>
-            {
-                exists = false;
-                return new ValueTask<T>(default(T)!);
-            },
-            new HybridCacheEntryOptions(),
-            null,
-            CancellationToken.None);
-
-        // In checking for the existence of the item, if not found, we will have created a cache entry with a null value.
-        // So remove it again.
-        if (exists is false)
+        // Acquire a per-key semaphore so that GetOrCreateAsync and the possible RemoveAsync
+        // complete without another thread retrieving/creating the same key in-between.
+        SemaphoreSlim sem = _keyLocks.GetOrAdd(key, _ => new SemaphoreSlim(1, 1));
+
+        await sem.WaitAsync().ConfigureAwait(false);
+
+        try
         {
-            await cache.RemoveAsync(key);
+            T? result = await cache.GetOrCreateAsync<T?>(
+                key,
+                cancellationToken =>
+                {
+                    exists = false;
+                    return default;
+                },
+                new HybridCacheEntryOptions(),
+                null,
+                token).ConfigureAwait(false);
+
+            // In checking for the existence of the item, if not found, we will have created a cache entry with a null value.
+            // So remove it again. Because we're holding the per-key lock there is no chance another thread
+            // will observe the temporary entry between GetOrCreateAsync and RemoveAsync.
+            if (exists is false)
+            {
+                await cache.RemoveAsync(key).ConfigureAwait(false);
+            }
+
+            return (exists, result);
         }
+        finally
+        {
+            sem.Release();
 
-        return (exists, result);
+            // Only remove the semaphore mapping if it still points to the same instance we used.
+            // This avoids removing another thread's semaphore or corrupting the map.
+            if (_keyLocks.TryGetValue(key, out SemaphoreSlim? current) && ReferenceEquals(current, sem))
+            {
+                _keyLocks.TryRemove(key, out _);
+            }
+        }
     }
 }

src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs

@@ -1,4 +1,4 @@
-using Microsoft.Extensions.Options;
+using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.Events;
@@ -9,7 +9,7 @@
 
 namespace Umbraco.Cms.Infrastructure.HybridCache.NotificationHandlers;
 
-internal sealed class SeedingNotificationHandler : INotificationAsyncHandler<UmbracoApplicationStartedNotification>
+internal sealed class SeedingNotificationHandler : INotificationAsyncHandler<UmbracoApplicationStartingNotification>
 {
     private readonly IDocumentCacheService _documentCacheService;
     private readonly IMediaCacheService _mediaCacheService;
@@ -24,7 +24,7 @@ public SeedingNotificationHandler(IDocumentCacheService documentCacheService, IM
         _globalSettings = globalSettings.Value;
     }
 
-    public async Task HandleAsync(UmbracoApplicationStartedNotification notification,
+    public async Task HandleAsync(UmbracoApplicationStartingNotification notification,
         CancellationToken cancellationToken)
     {
 

src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs

@@ -205,7 +205,7 @@ public async Task SeedAsync(CancellationToken cancellationToken)
 
                 var cacheKey = GetCacheKey(key, false);
 
-                var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode>(cacheKey);
+                var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode?>(cacheKey, cancellationToken).ConfigureAwait(false);
                 if (existsInCache is false)
                 {
                     uncachedKeys.Add(key);
@@ -278,7 +278,7 @@ public async Task<bool> HasContentByIdAsync(int id, bool preview = false)
             return false;
         }
 
-        return await _hybridCache.ExistsAsync<ContentCacheNode>(GetCacheKey(keyAttempt.Result, preview));
+        return await _hybridCache.ExistsAsync<ContentCacheNode?>(GetCacheKey(keyAttempt.Result, preview), CancellationToken.None);
     }
 
     public async Task RefreshContentAsync(IContent content)

src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs

@@ -133,7 +133,7 @@ public async Task<bool> HasContentByIdAsync(int id)
             return false;
         }
 
-        return await _hybridCache.ExistsAsync<ContentCacheNode>($"{keyAttempt.Result}");
+        return await _hybridCache.ExistsAsync<ContentCacheNode?>($"{keyAttempt.Result}", CancellationToken.None);
     }
 
     public async Task RefreshMediaAsync(IMedia media)
@@ -170,7 +170,7 @@ public async Task SeedAsync(CancellationToken cancellationToken)
 
                 var cacheKey = GetCacheKey(key, false);
 
-                var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode>(cacheKey);
+                var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode?>(cacheKey, CancellationToken.None);
                 if (existsInCache is false)
                 {
                     uncachedKeys.Add(key);

tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs

@@ -7,6 +7,7 @@
 using Umbraco.Cms.Core.PublishedCache;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Sync;
+using Umbraco.Cms.Tests.Common.Builders;
 using Umbraco.Cms.Tests.Common.Testing;
 using Umbraco.Cms.Tests.Integration.Testing;
 using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services;
@@ -25,10 +26,10 @@ protected override void CustomTestSetup(IUmbracoBuilder builder)
 
     private IPublishedContentCache PublishedContentHybridCache => GetRequiredService<IPublishedContentCache>();
 
-    private IContentEditingService ContentEditingService => GetRequiredService<IContentEditingService>();
-
     private IContentPublishingService ContentPublishingService => GetRequiredService<IContentPublishingService>();
 
+    private IDocumentCacheService DocumentCacheService => GetRequiredService<IDocumentCacheService>();
+
     private const string NewName = "New Name";
     private const string NewTitle = "New Title";
 
@@ -460,6 +461,61 @@ public async Task Can_Not_Get_Deleted_Published_Content_By_Key(bool preview)
         Assert.IsNull(textPage);
     }
 
+    [Test]
+    public async Task Can_Get_Published_Content_By_Id_After_Previous_Check_Where_Not_Found()
+    {
+        // Arrange
+        var testPageKey = Guid.NewGuid();
+
+        // Act & Assert
+        // - assert we cannot get the content that doesn't yet exist from the cache
+        var testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey);
+        Assert.IsNull(testPage);
+
+        testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey);
+        Assert.IsNull(testPage);
+
+        // - create and publish the content
+        var testPageContent = ContentEditingBuilder.CreateBasicContent(ContentType.Key, testPageKey);
+        var createResult = await ContentEditingService.CreateAsync(testPageContent, Constants.Security.SuperUserKey);
+        Assert.IsTrue(createResult.Success);
+        var publishResult = await ContentPublishingService.PublishAsync(testPageKey, CultureAndSchedule, Constants.Security.SuperUserKey);
+        Assert.IsTrue(publishResult.Success);
+
+        // - assert we can now get the content from the cache
+        testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey);
+        Assert.IsNotNull(testPage);
+    }
+
+    [Test]
+    public async Task Can_Get_Published_Content_By_Id_After_Previous_Exists_Check()
+    {
+        // Act
+        var hasContentForTextPageCached = await DocumentCacheService.HasContentByIdAsync(PublishedTextPageId);
+        Assert.IsTrue(hasContentForTextPageCached);
+        var textPage = await PublishedContentHybridCache.GetByIdAsync(PublishedTextPageId);
+
+        // Assert
+        AssertPublishedTextPage(textPage);
+    }
+
+    [Test]
+    public async Task Can_Do_Exists_Check_On_Created_Published_Content()
+    {
+        var testPageKey = Guid.NewGuid();
+        var testPageContent = ContentEditingBuilder.CreateBasicContent(ContentType.Key, testPageKey);
+        var createResult = await ContentEditingService.CreateAsync(testPageContent, Constants.Security.SuperUserKey);
+        Assert.IsTrue(createResult.Success);
+        var publishResult = await ContentPublishingService.PublishAsync(testPageKey, CultureAndSchedule, Constants.Security.SuperUserKey);
+        Assert.IsTrue(publishResult.Success);
+
+        var testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey);
+        Assert.IsNotNull(testPage);
+
+        var hasContentForTextPageCached = await DocumentCacheService.HasContentByIdAsync(testPage.Id);
+        Assert.IsTrue(hasContentForTextPageCached);
+    }
+
     private void AssertTextPage(IPublishedContent textPage)
     {
         Assert.Multiple(() =>