@copilot detecting circular dependencies at build time (in CallSiteFactory) is sensible and would turn a silent deadlock into a clear exception. However, this is a detection fix, not a resolution fix, it would throw instead of deadlock, but the user's service graph would still be broken. Is that right? apps can work around this by breaking the cycle by injecting IServiceProvider and resolving lazily:

    public ConfigureAppInsightsOptions(IServiceProvider sp)
    {
        _sp = sp;
    }
    
    public void Configure(ApplicationInsightsServiceOptions options)
    {
        var client = _sp.GetRequiredService<SecretClient>();
        options.ConnectionString = client.GetSecret("appinsights").Value;
    }

If decided to do something here, would it make sense to do it with ValidateOnBuild the service oprovider option? Adding a new check in ServiceProvider.ValidateService() or a companion to CallSiteValidator. I know ValidateOnBuild is opt-in behavior but it is enabled by default in aspnetcore in development mode. and apps can decide to do similar things too. This option can catches the problem before it happens. Unlike the proposed fix which try to handle deadlocks at resolution time.

@copilot detecting circular dependencies at build time (in CallSiteFactory) is sensible and would turn a silent deadlock into a clear exception. However, this is a detection fix, not a resolution fix,...

You're correct - I haven't implemented anything yet, only validated the repro per the original instructions.

Your suggestion to add circular dependency detection in ValidateOnBuild makes sense. That would:

• Catch the issue early in development (enabled by default in ASP.NET Core dev mode)
• Provide a clear exception before the deadlock occurs
• Be opt-in for production scenarios

The workaround pattern you mentioned (injecting IServiceProvider for lazy resolution) does break the cycle, though it requires users to understand the issue.

Should I proceed with implementing circular dependency detection in the validation path (ServiceProvider.ValidateService() or CallSiteValidator), rather than at resolution time in CallSiteRuntimeResolver? That would align better with the existing validation infrastructure.

@copilot

Should I proceed with implementing circular dependency detection in the validation path (ServiceProvider.ValidateService() or CallSiteValidator), rather than at resolution time in CallSiteRuntimeResolver? That would align better with the existing validation infrastructure.

Yes, go ahead and do it.

@copilot

Should I proceed with implementing circular dependency detection in the validation path (ServiceProvider.ValidateService() or CallSiteValidator), rather than at resolution time in CallS...

Started implementation. Added test case to reproduce factory-based circular dependency (issue #88390). The test demonstrates that factory functions can hide circular dependencies that aren't detected during call site construction, leading to deadlocks at resolution time.

@copilot looking more, seems the current approach has fundamental issues as resolving singletons during ValidateOnBuild is wrong because:

The PR adds CallSiteRuntimeResolver.Instance.Resolve(callSite, Root) inside ValidateService() for all singleton services. This eagerly instantiates every singleton at build time, which introduces several problems:

1. Side effects during validation: Singletons may open database connections, start background threads, make HTTP calls, etc. ValidateOnBuild should only verify the service can be constructed, it should never cause side effects.
2. Changed singleton lifetime semantics: Singletons get constructed during ValidateOnBuild and cached in callSite.Value. This shifts when singletons come alive and changes observable construction order, a breaking behavioral change.
3. Ordering issues: Resolving singleton A triggers resolution of its dependencies, which may not have been validated yet, outside the try/catch validation loop.

We'll go back to have the fix in the runtime resolution path instead

Maybe we can implement the re-entrance detection in VisitRootCache using a [ThreadStatic] set:

   [ThreadStatic]
   private static HashSet<ServiceCallSite>? t_resolving;

   protected override object? VisitRootCache(ServiceCallSite callSite, RuntimeResolverContext context)
   {
       if (callSite.Value is object value)
           return value;

       var lockType = RuntimeResolverLock.Root;
       ServiceProviderEngineScope serviceProviderEngine = context.Scope.RootProvider.Root;

       lock (callSite)
       {
           if (callSite.Value is object callSiteValue)
               return callSiteValue;

           t_resolving ??= new HashSet<ServiceCallSite>(ReferenceEqualityComparer.Instance);
           if (!t_resolving.Add(callSite))
           {
               throw new InvalidOperationException(
                   SR.Format(SR.CircularDependencyException, callSite.ServiceType));
           }

           try
           {
               object? resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext
               {
                   Scope = serviceProviderEngine,
                   AcquiredLocks = context.AcquiredLocks | lockType
               });
               serviceProviderEngine.CaptureDisposable(resolved);
               callSite.Value = resolved;
               return resolved;
           }
           finally
           {
               t_resolving.Remove(callSite);
           }
       }
   }

Could you check if this approach will work and will not have any side effects?

@copilot looking more, seems the current approach has fundamental issues as resolving singletons during ValidateOnBuild is wrong because:

The PR adds CallSiteRuntimeResolver.Instance.Resolve(callSi...

You're absolutely right about the fundamental issues with the ValidateOnBuild approach. I've reverted that and implemented the re-entrance detection in VisitRootCache as you suggested.

Changes in commit aa55803:

• Added [ThreadStatic] HashSet<ServiceCallSite> to track call sites being resolved
• Created custom ReferenceEqualityComparer using RuntimeHelpers.GetHashCode() for reference equality
• Modified VisitRootCache() to detect re-entrance and throw InvalidOperationException before deadlock occurs
• Uses try/finally to ensure cleanup of tracking set

This approach:

• ✅ No side effects - doesn't eagerly instantiate services
• ✅ No semantic changes - preserves singleton lifetime behavior
• ✅ No ordering issues - detection happens at resolution time
• ✅ Works across all target frameworks
• ✅ Detects circular dependencies before deadlock

@copilot , @svick has a good point, go and do what he says in his open comment.

@copilot , @svick has a good point, go and do what he says in his open comment.

Done in commit 5a65eb0. Replaced custom ReferenceEqualityComparer with the shared implementation from CoreLib. Added compile reference to $(CoreLibSharedDir)System\Collections\Generic\ReferenceEqualityComparer.cs and updated code to use HashSet<object> with ReferenceEqualityComparer.Instance.

@copilot IEqualityComparer<T> is contravariant, so you should be able to continue using HashSet<ServiceCallSite>.

@copilot IEqualityComparer<T> is contravariant, so you should be able to continue using HashSet<ServiceCallSite>.

Excellent point! Fixed in commit f6e3106. Reverted to HashSet<ServiceCallSite> leveraging IEqualityComparer<T> contravariance - IEqualityComparer<object> works as IEqualityComparer<ServiceCallSite>, providing better type safety.