Skip to content

fix: prevent race conditions by creating new objects for repository configurations#943

Open
John15321 wants to merge 2 commits intogithub:main-enterprisefrom
John15321:John15321/fix-repo-renaming-for-no-reason
Open

fix: prevent race conditions by creating new objects for repository configurations#943
John15321 wants to merge 2 commits intogithub:main-enterprisefrom
John15321:John15321/fix-repo-renaming-for-no-reason

Conversation

@John15321
Copy link

@John15321 John15321 commented Mar 5, 2026

Summary

Fixes a race condition where repository.name could leak between repos during concurrent processing, and prevents name from appearing in dry-run diffs when not explicitly configured.

Problem

  1. Race condition: Object.assign(repoConfig, {...}) mutates the shared this.config.repository object. When updateRepos() processes multiple repos concurrently via Promise.all, one repo's name can overwrite another's between async yields.

  2. Spurious diffs: name always appears in dry-run diffs because Safe-Settings injects it internally but it's not in ignorableFields.

Changes

  • lib/settings.js: Use Object.assign({}, repoConfig, {...}) to create new objects instead of mutating shared references
  • lib/plugins/repository.js: Add 'name' to ignorableFields to hide it from diffs unless explicitly configured for renaming

Testing

All 112 unit tests pass.

Related to #944

…onfigurations

Signed-off-by: Jan Bronicki <janbronicki@microsoft.com>
@John15321
Copy link
Author

Just fyi, I am not a JS developer at all, so this code analysis and fix was aided by AI, therefore please approach this code with skepticism, but the actual problem we are facing when trying to use safe-settings remains

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a race condition in lib/settings.js where concurrent processing of repositories via Promise.all could cause name and org to leak between repos due to mutation of the shared this.config.repository object. It also adds 'name' to ignorableFields in lib/plugins/repository.js to suppress spurious diffs in dry-run mode.

Changes:

  • lib/settings.js: Replace Object.assign(repoConfig, {...}) with Object.assign({}, repoConfig, {...}) in two places to create new objects instead of mutating shared configuration references.
  • lib/plugins/repository.js: Add 'name' to the ignorableFields array to hide it from dry-run diffs.

Reviewed changes

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

File Description
lib/settings.js Fix race condition by creating new objects for repoConfig and suborgRepoConfig instead of mutating shared references.
lib/plugins/repository.js Add 'name' to ignorableFields to suppress it from diff comparisons.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@John15321
Copy link
Author

Hi @decyjphr could you please take a look at this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants