Isolate nested IoC scope bindings - #9869
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
c47e419 to
308e61f
Compare
11477fd to
12ba01b
Compare
🔗 Linked Issue RequiredThanks for the contribution! Please link a GitHub issue to this PR by adding |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
All reviewed changes are covered by isolation tests, with no unresolved issues.
Review tier: Balanced
Findings: None
What changed in this PR
Isolates nested IoC registrations while preserving inherited singleton instances.
Changes:
- Clones binding maps when creating child containers.
- Adds regression tests for parent, child, and sibling scope isolation.
| File | Description |
|---|---|
cli/azd/pkg/ioc/container.go |
Prevents child registrations from mutating parent bindings. |
cli/azd/pkg/ioc/container_test.go |
Verifies scope isolation and singleton inheritance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
12ba01b to
64792d1
Compare
Richard Park (richardpark-msft)
added
the
skip-governance
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Marina He (hemarina)
left a comment
Marina He (hemarina)
left a comment
There was a problem hiding this comment.
The underlying golobby/container type is map[reflect.Type]map[string]*binding. Previously, maps.Copy(current, parent.inner) only copied the outer map — each current[type] still pointed at the same inner map[string]*binding as the parent, so any bind() call (e.g. RegisterInstance) mutated that shared map in place, leaking overrides into the parent/siblings.
Cloning the inner map per type (maps.Clone(bindings)) isolates overrides correctly, while un-overridden entries still share the same *binding pointer — preserving singleton caching/sharing across parent/child (as Test_NewNestedContainer_InheritsParent verifies). This is the minimal correct fix, not over- or under-cloning.
Test coverage: Good — Test_Container_LayerEnvironmentManagerOverridesAreIsolated is a solid regression test matching the real infra-provider scenario. Worth calling out explicitly in the PR description: the existing assertion in Test_Container_Singleton_Instance_Register_Resolve (NotSame -> Same) wasn't just "updated" — it was asserting the bug's symptom as correct behavior before this fix. A sentence noting that would help reviewers not read it as an unrelated test change.
Optional nit: A short doc-comment on NewNestedContainer stating the invariant ("bindings are isolated per scope on override; unmodified singleton bindings remain shared with the parent") would help this not silently regress again.
64792d1 to
068a5d0
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is sound, and the test assertion concern is non-blocking.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
cli/azd/pkg/ioc/container_test.go — This assertion only compares the two values created by the test, so it passes even if either scope… |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Shared singleton bindings leave a critical cross-scope leakage path unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
cli/azd/pkg/ioc/container.go — maps.Clone isolates only the name map; every value remains the same *binding. In… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
cli/azd/pkg/ioc/container_test.go — This assertion only compares the two values created by the test, so it passes even if either scope… View resolved comment |
| for registeredType, bindings := range parent.inner { | ||
| // clone bindings as well, otherwise changes in child scopes will | ||
| // actually modify the map for the parent! | ||
| current[registeredType] = maps.Clone(bindings) |
…e instance ends up being the same at all levels.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The tests must assert resolution errors to reliably verify singleton inheritance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
cli/azd/pkg/ioc/container_test.go — These unchecked resolutions can both fail and leave typed nil pointers, allowing Same to pass… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
cli/azd/pkg/ioc/container.go — maps.Clone isolates only the name map; every value remains the same *binding. In… View comment |
Suppressed comments (2)
cli/azd/pkg/ioc/container_test.go:211
- The
child firstcase resolves both variables fromchildScope, so it never verifies that the parent observes the inherited singleton after the child resolves it. Resolve the child first and then resolve the second value fromrootContainer; checking both errors also prevents two failed resolutions from satisfying the identity assertion.
var rootResolved *singletonService
childScope.Resolve(&rootResolved)
var childResolved *singletonService
childScope.Resolve(&childResolved)
cli/azd/pkg/ioc/container_test.go:234
- Despite the test name, the root is resolved before either child, making this block repeat the preceding root-first path rather than covering child overrides resolved before the root. Move the root resolution after the two child resolutions so the intended ordering is exercised.
var rootInstanceResolved *singletonService
err = rootContainer.Resolve(&rootInstanceResolved)
| var rootResolved *singletonService | ||
| rootContainer.Resolve(&rootResolved) | ||
|
|
||
| var childResolved *singletonService | ||
| childScope.Resolve(&childResolved) |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
IoC scopes and context lifetimes in azd: history and lessons learnedThis change sits in an area where azd has accumulated several years of lessons about dependency lifetimes, command scopes, and context cancellation. The recurring theme has been that three lifetimes must remain distinct:
Many of the regressions in this area happened when an object from one lifetime was accidentally retained by another. The original nested containerThe custom Internally, the upstream container is effectively: map[reflect.Type]map[string]*bindingThe original implementation created a child by copying entries from the parent's outer map: for key, value := range parent.inner {
current[key] = value
}This looked like a copy, but Command scopes made isolation important#3173 introduced customizable cmdContainer, err := cb.container.NewScope()
ioc.RegisterInstance(cmdContainer, ctx)
ioc.RegisterInstance(cmdContainer, cmd)
ioc.RegisterInstance(cmdContainer, args)This established an important intended contract: the root owns process-wide services, while the child owns the current command's context and inputs. However, because the nested container shared its inner registration maps, registering an already-known type in the child could replace the parent's entry too. Creating a scope did not always provide registration isolation even though callers reasonably assumed that it did. #3246 later simplified the wrapper by removing unused lifetime and parent fields, but did not change these copy semantics. #7064 eventually modernized the loop to Singleton versus scoped
|
Victor Vazquez (vhvb1989)
left a comment
Victor Vazquez (vhvb1989)
left a comment
There was a problem hiding this comment.
This ioc container has been historically complicated 😂🫠
I asked copilot to bring the Historical evolution for this component - I am not 100% sure anymore if what this PR says changes in child scopes modify the parter might be expected in some scenario but blocking you right now and acting as a bug for you.
I might need to expend more time on this then what I want/wish 😁🐿️ - so maybe we should just let it be;
This PR fixes a bug in how we created nested scopes. The shallow copy we did, prior to this PR, made it possible for child scopes to affect sibling or root scopes when rebinding a type.