Scoping: Isolate the ambient scope stacks across execution context flows (closes #23366)

[Ron-Brouwer]

Description

All three ambient scope stacks share a ConcurrentStack instance between unrelated execution flows, so concurrently-running hosted services can observe each other's ambient scope. When that happens Scope.Dispose() throws before it releases anything, stranding the transaction and whatever distributed lock it held until the process restarts.

Related: #23366, which reports the same defect becoming reachable after #22331.

Root cause

Push does (_stack.Value ??= new ConcurrentStack<IScope>()).Push(scope), and Pop never resets _stack.Value to null — popping the last entry leaves the instance in place. So the first scope used on any execution context leaves behind a non-null but empty stack.

AsyncLocal copy-on-write protects the reference, not the instance it points at. Every flow that later branches off that context therefore inherits and mutates the same ConcurrentStack. Because a ConcurrentStack is globally LIFO rather than per-flow, pushes and pops from unrelated hosted services interleave.

This is why the failure is timing-sensitive. If _stack.Value happens to be null when a service starts, that service allocates its own stack and behaves correctly; if something had already used a scope on that context — DistributedJobService.EnsureJobsAsync during start-up, for instance — it silently shares.

Why a failed dispose strands the lock

When the mismatch is detected, Scope.Dispose() throws before Locks.ClearLocks(InstanceId), before Locks.EnsureLocksCleared(InstanceId), and before _scopeProvider.PopAmbientScope(). So a failed dispose leaves the locks held, the transaction uncompleted, and the stack still corrupted for everything that follows. On SQL Server that strands the lock row; on SQLite it leaves an open write transaction.

There is a quieter second symptom. Scope.Database fetches the parent's database when ParentScope != null and calls GetCurrentTransactionIsolationLevel(), which touches Transaction.IsolationLevel. A scope wrongly parented to a foreign scope whose transaction has since completed throws InvalidOperationException: This SqlTransaction has completed. Worth noting because such a scope also does not own its transaction, so its scope.Complete() becomes a no-op against the real one — a correctness exposure, not only a liveness one.

All three stacks need the fix

src/Umbraco.Infrastructure/Scoping/AmbientScopeStack.cs
src/Umbraco.Infrastructure/Scoping/AmbientScopeContextStack.cs
src/Umbraco.Cms.Persistence.EFCore/Scoping/AmbientEFCoreScopeStack.cs

A partial fix is worse than no fix. We deployed a build with only AmbientScopeStack fixed to an affected production site and immediately got a new exception:

System.InvalidOperationException: No AmbientContext was found.
   at Umbraco.Cms.Infrastructure.Scoping.AmbientScopeContextStack.Pop()
   at Umbraco.Cms.Persistence.EFCore.Scoping.EFCoreScopeProvider`1.PopAmbientScopeContext()
   at Umbraco.Cms.Persistence.EFCore.Scoping.EFCoreScope`1.HandleScopeContext()
   at Umbraco.Cms.Persistence.EFCore.Scoping.EFCoreScope`1.Dispose()
   at ...AIUsageHourlyAggregationJob.ProcessMissingHoursAsync()
   at Umbraco.Cms.Infrastructure.HostedServices.RecurringHostedServiceBase.ExecuteAsync()

Whether a scope pushes a scope context depends on whether it sees an ambient scope. Before the fix, flows sharing a stack wrongly saw each other's scopes and became nested children, which do not push a context. Once the scope stack is isolated those scopes correctly become root scopes and start pushing contexts of their own — onto a context stack that is still shared between flows. They then pop each other's entries until one finds it empty and throws. The corruption moves one layer down rather than disappearing.

The change

Identical in all three: when the inherited stack is empty there is no genuine nesting to preserve, so allocating a fresh one lets AsyncLocal copy-on-write isolate the flow. A non-empty stack is real nesting within the same flow and is kept, so a child scope still nests under the current ambient one.

ConcurrentStack<IScope>? stack = _stack.Value;

if (stack is null || stack.IsEmpty)
{
    stack = new ConcurrentStack<IScope>();
    _stack.Value = stack;
}

stack.Push(scope);

Why not restore ExecutionContext.SuppressFlow() instead

#22331 removed SuppressFlow from RecurringHostedServiceBase and simultaneously added an equivalent to RecurringBackgroundJobHostedService.StartAsync. The protection was relocated rather than deleted, so the IRecurringBackgroundJob wrapper stayed safe while external subclasses of the public RecurringHostedServiceBase lost protection they had in 17.4.

Restoring it there would close that regression, but it is not sufficient:

• DistributedBackgroundJobHostedService has never had SuppressFlow in any version, including current main, and it participates in most of the traces we collected.
• AmbientScopeStack.cs is byte-identical across release-17.4.2, release-17.5.3 and main, so the defect is version-invariant rather than a regression.

Fixing the stacks covers every caller at once, including third-party hosted services, instead of requiring each entry point to remember to suppress flow. It is also the only option available to affected sites, since IAmbientScopeStack and AmbientScopeContextStack are internal — the DI registration cannot be overridden from a consumer project, so there is no supported workaround.

Affected versions

Confirmed in production on 17.5.2 and 17.5.3, and present unchanged on main. This PR targets main per the contributing guide, but it cherry-picks cleanly to the v17 branches:

• AmbientScopeStack.cs and AmbientScopeContextStack.cs are byte-identical between main and v17/dev.
• AmbientEFCoreScopeStack.cs differs only by the IEfCoreScope → IEFCoreScope rename, so the v17 variant needs the lower-case spelling in both the fix and the new test.

How to test

Run the new tests:

dotnet test tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj --filter "FullyQualifiedName~AmbientScope"
dotnet test tests/Umbraco.Tests.Integration/Umbraco.Tests.Integration.csproj --filter "FullyQualifiedName~AmbientEFCoreScopeStackTests"

To confirm the tests actually catch the defect, revert the three Push methods to _stack.Value ??= new ConcurrentStack<…>() and re-run. Each stack has one isolation test that fails and one nesting guard that passes either way, so the guards cannot mask an over-correction:

Run	Without the fix	With the fix
Unit, ~AmbientScope	Failed 2, Passed 2	Passed 4
Integration, EF Core stack	Failed 1, Passed 1	Passed 2

Each isolation test reproduces the defect deterministically: it leaves a non-null empty stack on the execution context, starts two flows from it, has both push and wait for each other, then asserts each flow still sees its own entry as ambient.

Wider suites on this branch: 6,393 unit tests and 106 scoping integration tests passing, 0 failures (6 and 3 skipped respectively, unrelated).

For a real-world reproduction, run two hosted services concurrently — DistributedBackgroundJobHostedService plus any RecurringHostedServiceBase subclass that opens a scope — on a site where a scope was used during start-up. Before the fix, one of them eventually throws "The Scope … is not the Ambient Scope …" from Scope.Dispose(), after which every server polling the -347 DistributedJobs lock times out every few seconds indefinitely, because the poll interval and the write-lock timeout are both 5 seconds.

Fixes #23336

[github-actions]

Hi there @Ron-Brouwer, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Copilot]

Pull request overview

This PR fixes a concurrency defect in Umbraco’s ambient scoping infrastructure where unrelated execution flows (notably concurrently running hosted services) could end up sharing the same mutable ConcurrentStack via AsyncLocal, causing scope/context interleaving, invalid ambient state, and (worst case) stranded transactions/locks until process restart.

Changes:

• Update Push(...) in all three ambient stacks to allocate a fresh stack when the inherited stack is null or empty, allowing AsyncLocal copy-on-write to correctly isolate sibling execution flows.
• Add unit tests covering isolation vs. true nesting for AmbientScopeStack and AmbientScopeContextStack.
• Add an EF Core integration test covering the same isolation vs. nesting behavior for AmbientEFCoreScopeStack.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Umbraco.Infrastructure/Scoping/AmbientScopeStack.cs	Prevents sharing an inherited empty ambient scope stack across execution flows by allocating a new stack on Push.
src/Umbraco.Infrastructure/Scoping/AmbientScopeContextStack.cs	Applies the same isolation fix for the ambient scope context stack to avoid cross-flow corruption after scope isolation.
src/Umbraco.Cms.Persistence.EFCore/Scoping/AmbientEFCoreScopeStack.cs	Applies the same isolation fix for EF Core ambient scopes to prevent cross-flow stack sharing.
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Scoping/AmbientScopeStackTests.cs	Adds isolation + nesting tests for both ambient scope and ambient context stacks.
tests/Umbraco.Tests.Integration/Umbraco.Persistence.EFCore/Scoping/AmbientEFCoreScopeStackTests.cs	Adds isolation + nesting tests for EF Core ambient scope stack behavior.

Comments suppressed due to low confidence (1)

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Scoping/AmbientScopeStackTests.cs:114

• The concurrent-flow isolation test in AmbientScopeContextStackTests also leaves pushed contexts on the stack in the Task.Run flows. Since AmbientScopeContextStack uses a static AsyncLocal, this can leak onto ThreadPool execution contexts and cause cross-test contamination. Add a barrier so both flows read AmbientContext before popping, then pop in each flow.

            async Task<IScopeContext?> PushThenReadAmbient(
                IScopeContext context,
                TaskCompletionSource pushed,
                Task otherPushed)
            {
                sut.Push(context);
                pushed.SetResult();
                await otherPushed;
                return sut.AmbientContext;
            }

[emmagarland]

Hi @Ron-Brouwer

Thanks for your PR to fix #23336, where the possibility of a permanently open SQL write transaction was identified.

One of the HQ team will review this as soon as possible; likely @AndyButland when he has capacity to pick this up.

I think this is your first contribution to this repo? That is awesome! 🎉

Best regards

Emma

[Zeegaan]

Great changes, and really love the detailed description, makes it really easy to understand 🎉
And the tests is the cherry on top, great job 🍒