Performance: Fix thread safety and optimize cache updates in PublishStatusService after content changes

[AndyButland]

Description

In analysis of database operations in a save and publish operation we found more than one call to retrieve the published cultures of a document. Whilst initially the thought was to cache this, I've instead addressed this in the content cache refresher, modifying the behaviour such that it only refreshes the published cultures for a document in the context of an update that affects published cultures. We don't need to do this for example for save or sort operations, only publish and unpublish.

In looking at this I also saw we have a potential concurrent issue with the use of an IDictionary for the cached published cultures. This is demonstrated via an integration test and fixed by using ConcurrentDictionary.

Change Summary

• Optimized ContentCacheRefresher.HandlePublishedAsync to only update publish status when there are actual publish/unpublish changes (and not on Save operations).
• Fixed thread safety issue in PublishStatusService by using ConcurrentDictionary instead of Dictionary for the internal _publishedCultures cache.
• Added thread safety integration tests for PublishStatusService.

Cache Update Optimization

ContentCacheRefresher was calling AddOrUpdateStatusAsync/AddOrUpdateStatusWithDescendantsAsync on every content change, including Save operations. These methods query the database unnecessarily since Save operations don't change publish status.

Added HasPublishStatusUpdates check that only triggers updates when PublishedCultures or UnpublishedCultures are populated in the payload - which only happens during Publish/Unpublish operations, not Save.

Thread Safety Fix

The PublishStatusService maintains an in-memory dictionary of published cultures per document. As a singleton service accessed by multiple concurrent HTTP requests, using a non-thread-safe Dictionary<Guid, ISet<string>> could cause:

• InvalidOperationException (collection modified during enumeration)
• Data corruption from concurrent writes
• Race conditions

Changed to ConcurrentDictionary<Guid, ISet<string>> and updated Remove to use TryRemove.

Testing

The thread safety issue is verified with the provided integration test (this would fail without the changes in this PR PublishStatusService.

For manual testing you can verify with breakpoints that AddOrUpdateStatusAsync/AddOrUpdateStatusWithDescendantsAsync are only called in publish or unpublish operations.

[Copilot]

Pull request overview

This PR addresses thread safety issues in the PublishStatusService and optimizes cache refresh operations to avoid unnecessary database queries.

Changes:

• Replaced Dictionary<Guid, ISet<string>> with ConcurrentDictionary<Guid, ISet<string>> in PublishStatusService to fix thread safety issues in a singleton service
• Optimized ContentCacheRefresher.HandlePublishedAsync to only update publish status when actual publish/unpublish operations occur (not on Save operations)
• Added comprehensive thread safety integration tests covering concurrent reads, writes, and initialization

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
PublishStatusService.cs	Changed internal cache from Dictionary to ConcurrentDictionary and updated Remove method to use TryRemove
ContentCacheRefresher.cs	Added HasPublishStatusUpdates check to skip unnecessary cache updates on Save operations
PublishStatusServiceTests.ThreadSafety.cs	New test file with thread safety tests for concurrent operations
Umbraco.Tests.Integration.csproj	Added project reference for the new thread safety test file

Comments suppressed due to low confidence (2)

src/Umbraco.Core/Services/PublishStatus/PublishStatusService.cs:76

• The ISet<string> values in the ConcurrentDictionary are not thread-safe. While the dictionary operations are now thread-safe, concurrent access to the same ISet<string> instance (e.g., one thread reading via Contains() while another thread is replacing the set via _publishedCultures[documentKey] = publishedCultures in line 123 or 146) could cause issues. Consider using a thread-safe collection for the values, or document that updates happen in a controlled manner (e.g., within database scopes).

        if (_publishedCultures.TryGetValue(documentKey, out ISet<string>? publishedCultures))
        {
            // If "*" is provided as the culture, we consider this as "published in any culture". This aligns
            // with behaviour in Umbraco 13.
            return culture == Constants.System.InvariantCulture || publishedCultures.Contains(culture, StringComparer.InvariantCultureIgnoreCase);
        }

src/Umbraco.Core/Services/PublishStatus/PublishStatusService.cs:88

• The ISet<string>.Count property access is not thread-safe when the set could be replaced concurrently. While less likely to cause crashes than enumeration, this could read inconsistent state if the set is being replaced by another thread. This is related to the same underlying issue where the ISet<string> values are not thread-safe.

        if (_publishedCultures.TryGetValue(documentKey, out ISet<string>? publishedCultures))
        {
            return publishedCultures.Count > 0;
        }

[Copilot]

The thread safety tests use random GUIDs that don't correspond to actual documents in the database. This means AddOrUpdateStatusAsync will query for non-existent documents and cache empty sets. Consider using actual document keys from the test setup (e.g., Textpage.Key, Subpage.Key) to test realistic scenarios where documents exist and have publish status data.

[Zeegaan]

Generally looks good, just had a small suggestion, but this is fine without it 😁