Fix concurrent Lazy get and set - #9868
Conversation
🔗 Linked Issue RequiredThanks for the contribution! Please link a GitHub issue to this PR by adding |
|
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. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The locking is coherent and regression coverage exercises concurrent reads and writes; only a minor documentation nit remains.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/lazy/lazy.go — Public API comments must begin with the documented symbol name (cli/azd/AGENTS.md:253-257); this… |
What changed in this PR
Makes Lazy initialization, reads, and writes concurrency-safe.
Changes:
- Replaces separate mutexes with one
RWMutex. - Adds initialization and concurrency regression tests.
| File | Description |
|---|---|
cli/azd/pkg/lazy/lazy.go |
Synchronizes lazy state access. |
cli/azd/pkg/lazy/lazy_test.go |
Covers bypass, recovery, and concurrent access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c47e419 to
308e61f
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The concurrency regression test cannot detect the former race in standard CI because race detection is not enabled.
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/lazy/lazy_test.go — This regression test has no behavioral assertion and only detects the former data race when run… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/lazy/lazy.go — Public API comments must begin with the documented symbol name (cli/azd/AGENTS.md:253-257); this… View resolved comment |
|
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. |
|
Could this deadlock when the initializer calls
|
308e61f to
0b59494
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The unified lock removes the data race while preserving lazy initialization and override behavior.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/lazy/lazy_test.go — This regression test has no behavioral assertion and only detects the former data race when run… View resolved comment |
Just to check, the |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|


Lazy was using two separate locks for get and set operations, which allowed access to variables that were intended to be protected. The original intent appeared to be doing the same work a sync.RWMutex does, so I just swapped over to that.
In reality, this type's probably isn't as high performance as this, and we could have just used a single sync.Mutex, but this preserves the intention of the original, and still uses the stdlib.