Support reload in ChainedConfigurationSource

[martincostello]

Add support for reloading configuration to ChainedConfigurationSource.

Fixes #58683.

[svick]

Thank you for the PR.

I think you should also test the case where the chained configuration is not an IConfigurationRoot, i.e. it's a section of another configuration.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[tarekgh]

🤖 Code Review — PR #125122

Review co-authored by @tarekgh and GitHub Copilot

---

Holistic Assessment

Motivation: The PR addresses a real bug (#58683): calling Reload() on an outer ConfigurationRoot that chains an inner IConfigurationRoot doesn't propagate the reload to the inner root's providers. This is clearly broken and needs fixing.

Approach: The fix changes ChainedConfigurationProvider.Load() from a no-op to (_config as IConfigurationRoot)?.Reload(). While this correctly fixes the explicit-reload scenario, it is too broad — Load() is also called during ConfigurationRoot construction and ConfigurationManager.AddSource(), causing unintended side effects on initial build.

Summary: ⚠️ Needs Changes. The core idea is right, but the implementation introduces construction-time reload of the inner root (re-loading all inner providers and firing spurious change notifications) as a side effect of building the outer configuration. A guard (e.g., a _loaded flag to skip the first invocation) is needed to preserve the no-op behavior during initial construction while enabling reload propagation on subsequent calls.

---

Detailed Findings

❌ Construction-time side effect — Inner root gets reloaded during outer construction

ConfigurationRoot constructor calls p.Load() on every provider during construction:

foreach (IConfigurationProvider p in providers)
{
    p.Load();  // triggers Reload() on the inner root!
}

With this change, when the outer ConfigurationRoot is built, it will call ChainedConfigurationProvider.Load() which now calls innerRoot.Reload(). This:

1. Re-loads all inner providers — they were already loaded when the inner root was built, so this is redundant and potentially expensive (file I/O, API calls, etc.)
2. Fires RaiseChanged() on the inner root — any code subscribed to the inner root's change token gets a spurious notification during outer construction

The same issue applies to ConfigurationManager.AddSource() and ReloadSources().

Recommendation: Add a _loaded flag to differentiate initial Load() from subsequent reload calls:

private bool _loaded;

public void Load()
{
    if (_loaded)
    {
        (_config as IConfigurationRoot)?.Reload();
    }
    _loaded = true;
}

⚠️ Double/early notification during outer Reload()

When outerRoot.Reload() is called:

1. It iterates providers and calls chainedProvider.Load() → innerRoot.Reload() → innerRoot.RaiseChanged()
2. The outer root is subscribed to the chained provider's reload token (which delegates to the inner root's token), so the outer's RaiseChanged() may fire during the provider loop
3. Then outerRoot.Reload() calls RaiseChanged() again at the end

This can result in observers seeing partially-updated configuration state mid-reload, and receiving duplicate notifications. This is a pre-existing architectural concern somewhat exacerbated by this change.

⚠️ Recursive chaining risk — Stack overflow with circular chains

If two ConfigurationRoot instances chain each other (A → B → A), calling Load() on A's chained provider will call B.Reload(), which calls Load() on B's chained provider, which calls A.Reload(), leading to a StackOverflowException. While circular chaining is a user error, this change makes it fail catastrophically (stack overflow) rather than silently (no-op). Consider at minimum documenting this, or adding a reentrancy guard.

⚠️ Test coverage gaps

The tests verify the happy path but miss important scenarios:

• No test for construction-time behavior — should verify that building the outer config does NOT trigger inner reload/notifications
• No test for ConfigurationManager — which also calls Load() during AddSource()
• No test for double-notification — verifying that outer Reload() doesn't cause duplicate change notifications on the inner root

💡 Test name typo — ReloadPropagatesto

The test method ChainedConfiguration_ReloadPropagatestoInnerConfigurationRoot has a casing typo — should be ReloadPropagatesTo (capital T).

✅ Correct handling of non-IConfigurationRoot inner config

The as IConfigurationRoot pattern correctly handles the case where _config is an IConfigurationSection — the cast returns null and ?.Reload() is skipped. The test ChainedConfiguration_ReloadDoesNotPropagateToInnerConfigurationSection correctly validates this (addressing @svick's review comment).

✅ Test design — RandomValueConfigurationProvider

The test helper that generates a new Guid on each Load() is a clean way to detect whether reload actually happened.

[martincostello]

FYI the original code was written by Copilot (martincostello#6) which explicitly calls out the reload behaviour here:

Side-effect to be aware of: an explicit Reload() on the outer root fires its change token twice — once when the inner reload propagates up through the already-registered ChangeToken.OnChange listener, and once from the outer Reload()'s own RaiseChanged(). This is consistent with how automatic (file-watcher) reloads already propagate through chains and is not a data-correctness issue.

I don't fully trust Copilot to review itself when it gives differing opinions of its own work, so if a human can tell me which which Copilot is correct, I'll implement any required changes.

[tarekgh]

Hi @martincostello,

In my opinion, I don’t see any conflict in the Copilot feedback. Both the author Copilot and the commenter Copilot are pointing to the same issue: duplicate notifications. This issue wasn’t introduced by this PR, but the changes here make it more noticeable. I haven’t spent much time digging deeper, but it seems there’s a way to fix both the construction and double-fire issue in the code path you’re modifying.

Instead of calling innerConfig.Reload() (which calls Load() on providers and then triggers RaiseChanged()), we could call Load() directly on each inner provider and skip RaiseChanged() on the inner config. We would also update Load() to avoid the duplicate notification, similar to what the Copilot commenter suggested, but with a small tweak.

private bool _loaded;

   public void Load()
   {
       if (!_loaded)
       {
           _loaded = true;
           return;
       }

       if (_config is IConfigurationRoot root)
       {
           foreach (IConfigurationProvider provider in root.Providers)
           {
               provider.Load();
           }
       }
   }

Anyway, having some test for the concerning scenario will be good to have to add confidence to what we are doing. Thanks!

[martincostello]

My point was more around "if you thought that, why did you do it that way in the first place?" with regards to Copilot.

I'll make the changes tomorrow.

[tarekgh]

My point was more around "if you thought that, why did you do it that way in the first place?" with regards to Copilot.

That depends on what you asked Copilot to do 😄 Copilot mentioned that “This is consistent with how automatic (file-watcher) reloads already propagate through chains and is not a data-correctness issue.” If you explicitly instruct it to fix the behavior, it would likely attempt to address it.

I'll make the changes tomorrow.

Thanks for your help fixing the issue!

[jeffhandley]

@martincostello The Coding Agent and the Code review agent are optimized for different jobs, so it’s normal for the review agent to flag things the coding agent didn’t. We've also been iterating in this repo on specializing the review capabilities. They can also use different models, context windows, etc. All around we're seeing favorable results with iterating between the two agents.

[martincostello]

Sure, but from a user perspective it looks like a single AI is giving me (seemingly) contradictory perspectives, which a maintainer may or may not agree is needed. Rather than go on a wild goose chase potentially wasting time on unnecessary changes, I'd rather just action feedback direct from a human (like Tarek has now given me).

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

outdated

[Copilot]

Pull request overview

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