Skip to content

Conversation

@lauraneto
Copy link
Contributor

@lauraneto lauraneto commented Jan 5, 2026

Summary

This PR extends the deadlock prevention fix from #21156 to other methods in the HybridCache project that could potentially cause similar deadlocks under concurrent load.

Add ReadLock to database read operations in DocumentCacheService, MediaCacheService, and ContentTypeSeedKeyProvider to prevent SQL Server deadlocks when these read operations compete with concurrent content/media save operations.

Methods updated with ReadLock

DocumentCacheService:

  • GetNodeAsync (cache miss callback)
  • GetByContentType
  • SeedAsync
  • RebuildMemoryCacheByContentTypeAsync

MediaCacheService:

  • GetNodeAsync (cache miss callback)
  • GetByContentType
  • SeedAsync
  • RefreshMemoryCacheAsync
  • RebuildMemoryCacheByContentTypeAsync

ContentTypeSeedKeyProvider:

  • GetSeedKeys

Testing

Deadlocks are race conditions that are difficult to reproduce reliably. They depend on timing, SQL Server lock escalation, and concurrent operation patterns. There are no specific reproduction steps - the issue manifests under heavy concurrent load on SQL Server.

Add ReadLock to database read operations in HybridCache services to prevent
deadlocks during concurrent content/media save operations. This extends the
fix from PR #21156 to other methods that were missing lock protection.

Methods updated with ReadLock:
- DocumentCacheService: GetNodeAsync, GetByContentType, SeedAsync, RebuildMemoryCacheByContentTypeAsync
- MediaCacheService: GetNodeAsync, GetByContentType, SeedAsync, RefreshMemoryCacheAsync, RebuildMemoryCacheByContentTypeAsync
- ContentTypeSeedKeyProvider: GetSeedKeys
@lauraneto lauraneto force-pushed the v17/bugfix/hybrid-cache-deadlock-prevention branch from 47646e5 to 751d70d Compare January 5, 2026 16:35
@lauraneto lauraneto marked this pull request as ready for review January 6, 2026 08:35
Copilot AI review requested due to automatic review settings January 6, 2026 08:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the SQL Server deadlock prevention mechanism from PR #21156 to additional database read operations in the HybridCache project. The changes add ReadLock calls to protect concurrent database reads from conflicting with write operations (such as content/media saves), preventing potential SQL Server deadlocks under heavy load.

Key changes:

  • Added ReadLock protection to cache miss callbacks, seeding operations, and memory cache rebuild methods
  • Ensured consistent lock acquisition pattern across Document, Media, and ContentType seed providers
  • Added necessary import (Umbraco.Cms.Core) for Constants.Locks access in ContentTypeSeedKeyProvider

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
MediaCacheService.cs Added ReadLock to GetNodeAsync cache miss callback, SeedAsync, RefreshMemoryCacheAsync, RebuildMemoryCacheByContentTypeAsync, and GetByContentType methods to protect database read operations
DocumentCacheService.cs Added ReadLock to GetNodeAsync cache miss callback, GetByContentType, SeedAsync, and RebuildMemoryCacheByContentTypeAsync methods to protect database read operations
ContentTypeSeedKeyProvider.cs Added ReadLock to GetSeedKeys method and imported Umbraco.Cms.Core namespace for Constants.Locks access

@lauraneto
Copy link
Contributor Author

@AndyButland Do you have a large database we could test this with?

For reference, these changes were suggested by Claude when @Migaroez asked it to find possible similar issues to #21156 in the hybrid cache project.
Looking at the code, the same database calls are being done, so it could indeed trigger the deadlock issue I was facing, although I cannot actually test/reproduce it.

@lauraneto lauraneto requested a review from AndyButland January 6, 2026 12:51
@AndyButland
Copy link
Contributor

The updates all look sensible to me @lauraneto. I've shared a large test database with you, so if that's able to show the problems on main and not with this PR, I think we can say this update is a definite improvement and we should approve and merge it.

public ISet<Guid> GetSeedKeys()
{
using ICoreScope scope = _scopeProvider.CreateCoreScope();
scope.ReadLock(Constants.Locks.ContentTree);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this lock should be necessary, or, if we do need one, should it be a new lock ID that is dedicated for reads from the cmsContentNu table? What I'm thinking is that it shouldn't be necessary to lock reads for the content tree - which will be umbracoNode, umbracoContent, umbracoDocument etc., to read from a table that's unrelated in the sense of the database schema.

I think the same applies for all the others added in the PR.

So - of course if in the testing you've since done or can do with a large database you see improvements, then probably we should discount my point noted here. But if we can't see any issue, then I'm not sure there's value in adding these locks as they are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants