From 2a60a9cf51099250b08a1ebb2ea27cb68e6af6a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Goetz?= <onigoetz@onigoetz.ch>
Date: Tue, 7 Sep 2021 09:59:13 +0200
Subject: [PATCH] fix: idempotency issues when rangeStrategy=update-lockfile is
 used with other rangeStrategy in the same file (#11328)

---
 lib/workers/branch/reuse.spec.ts | 50 ++++++++++++++++++++++++++++++--
 lib/workers/branch/reuse.ts      | 31 ++++++++++++++++++--
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/lib/workers/branch/reuse.spec.ts b/lib/workers/branch/reuse.spec.ts
index 0cec4b645b9874..b860bbf8f90c5e 100644
--- a/lib/workers/branch/reuse.spec.ts
+++ b/lib/workers/branch/reuse.spec.ts
@@ -1,7 +1,7 @@
 import { git, platform } from '../../../test/util';
-import type { RenovateConfig } from '../../config/types';
 import { Pr } from '../../platform';
 import { PrState } from '../../types';
+import type { BranchConfig } from '../types';
 import { shouldReuseExistingBranch } from './reuse';
 
 jest.mock('../../util/git');
@@ -13,12 +13,13 @@ describe('workers/branch/reuse', () => {
       state: PrState.Open,
       title: 'any',
     };
-    let config: RenovateConfig;
+    let config: BranchConfig;
     beforeEach(() => {
       config = {
         branchName: 'renovate/some-branch',
         rebaseLabel: 'rebase',
         rebaseWhen: 'behind-base-branch',
+        upgrades: [],
       };
       jest.resetAllMocks();
     });
@@ -42,6 +43,51 @@ describe('workers/branch/reuse', () => {
       const res = await shouldReuseExistingBranch(config);
       expect(res.reuseExistingBranch).toBe(true);
     });
+
+    it('returns false if does not need rebasing but has upgrades that need lockfile maintenance along with upgrades that do not', async () => {
+      config.upgrades = [
+        {
+          packageFile: 'package.json',
+          rangeStrategy: 'replace',
+          branchName: 'current',
+        },
+        {
+          packageFile: 'package.json',
+          rangeStrategy: 'update-lockfile',
+          branchName: 'current',
+        },
+      ];
+      git.branchExists.mockReturnValueOnce(true);
+      platform.getBranchPr.mockResolvedValueOnce({
+        ...pr,
+        isConflicted: false,
+      });
+      const res = await shouldReuseExistingBranch(config);
+      expect(res.reuseExistingBranch).toBe(false);
+    });
+
+    it('returns true if does not need rebasing and lockfile update is on different packages', async () => {
+      config.upgrades = [
+        {
+          packageFile: 'package.json',
+          rangeStrategy: 'replace',
+          branchName: 'current',
+        },
+        {
+          packageFile: 'subpackage/package.json',
+          rangeStrategy: 'update-lockfile',
+          branchName: 'current',
+        },
+      ];
+      git.branchExists.mockReturnValueOnce(true);
+      platform.getBranchPr.mockResolvedValueOnce({
+        ...pr,
+        isConflicted: false,
+      });
+      const res = await shouldReuseExistingBranch(config);
+      expect(res.reuseExistingBranch).toBe(true);
+    });
+
     it('returns true if unmergeable and cannot rebase', async () => {
       git.branchExists.mockReturnValueOnce(true);
       platform.getBranchPr.mockResolvedValueOnce({
diff --git a/lib/workers/branch/reuse.ts b/lib/workers/branch/reuse.ts
index fdc856d1880b82..8d53647b3e180a 100644
--- a/lib/workers/branch/reuse.ts
+++ b/lib/workers/branch/reuse.ts
@@ -1,8 +1,9 @@
 import { getGlobalConfig } from '../../config/global';
-import type { RenovateConfig } from '../../config/types';
 import { logger } from '../../logger';
 import { platform } from '../../platform';
+import type { RangeStrategy } from '../../types';
 import { branchExists, isBranchModified, isBranchStale } from '../../util/git';
+import type { BranchConfig } from '../types';
 
 type ParentBranch = {
   reuseExistingBranch: boolean;
@@ -10,7 +11,7 @@ type ParentBranch = {
 };
 
 export async function shouldReuseExistingBranch(
-  config: RenovateConfig
+  config: BranchConfig
 ): Promise<ParentBranch> {
   const { branchName } = config;
   // Check if branch exists
@@ -86,5 +87,31 @@ export async function shouldReuseExistingBranch(
     logger.debug(`Branch is not mergeable but can't be rebased`);
   }
   logger.debug(`Branch does not need rebasing`);
+
+  // Branches can get in an inconsistent state if "update-lockfile" is used at the same time as other strategies
+  // On the first execution, everything is executed, but if on a second execution the package.json modification is
+  // skipped but the lockfile update is executed, the lockfile will have a different result than if it was executed
+  // along with the changes to the package.json. Thus ending up with an incomplete branch update
+  // This is why we are skipping branch reuse in this case (#10050)
+  const groupedByPackageFile: Record<string, Set<RangeStrategy>> = {};
+  for (const upgrade of config.upgrades) {
+    groupedByPackageFile[upgrade.packageFile] =
+      groupedByPackageFile[upgrade.packageFile] || new Set();
+    groupedByPackageFile[upgrade.packageFile].add(upgrade.rangeStrategy);
+
+    if (
+      groupedByPackageFile[upgrade.packageFile].size > 1 &&
+      groupedByPackageFile[upgrade.packageFile].has('update-lockfile')
+    ) {
+      logger.debug(
+        `Detected multiple rangeStrategies along with update-lockfile`
+      );
+      return {
+        reuseExistingBranch: false,
+        isModified: false,
+      };
+    }
+  }
+
   return { reuseExistingBranch: true, isModified: false };
 }