From 6b80afe9341743bcca7b9b9eb903af55e5ca6be7 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Tue, 14 Jul 2026 13:38:34 -0500 Subject: [PATCH] Verify all destination remotes before pushing artifact push:artifact only cloned and fetched from the first destination git URL, then pushed to the rest blind. Because every run generates a fresh commit SHA, any drift between destinations (branch left over on one remote, concurrent builds, a partial push failure) made later pushes fail with a non-fast-forward rejection that only a force push could clear. This hit PR/build branches constantly while main survived because merges serialize its pushes. Now the branch tip is fetched from every destination before the artifact is built. If tips differ but are ancestor-related, the build bases on the most advanced tip so every remote can fast-forward. If tips have truly diverged, the command aborts with an error naming each remote and its tip before anything is built. A push failure on one remote no longer skips the remaining remotes, which was how the drift started in the first place. Co-Authored-By: Claude Fable 5 --- src/Command/Push/PushArtifactCommand.php | 124 ++++++++-- .../Commands/Push/PushArtifactCommandTest.php | 227 ++++++++++++++++-- 2 files changed, 318 insertions(+), 33 deletions(-) diff --git a/src/Command/Push/PushArtifactCommand.php b/src/Command/Push/PushArtifactCommand.php index 858240fae..924ea56f5 100644 --- a/src/Command/Push/PushArtifactCommand.php +++ b/src/Command/Push/PushArtifactCommand.php @@ -111,7 +111,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int ]); $this->checklist->addItem('Preparing artifact directory'); - $this->cloneSourceBranch($outputCallback, $artifactDir, $destinationGitUrls[0], $sourceGitBranch); + $this->cloneSourceBranch($outputCallback, $artifactDir, $destinationGitUrls, $sourceGitBranch); $this->checklist->completePreviousItem(); } @@ -175,8 +175,10 @@ private function determineDestinationGitUrls(): array /** * Prepare a directory to build the artifact. + * + * @param string[] $vcsUrls */ - private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, string $vcsUrl, string $vcsPath): void + private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, array $vcsUrls, string $vcsPath): void { $fs = $this->localMachineHelper->getFilesystem(); @@ -185,39 +187,63 @@ private function cloneSourceBranch(Closure $outputCallback, string $artifactDir, $outputCallback('out', "Initializing Git in $artifactDir"); $this->localMachineHelper->checkRequiredBinariesExist(['git']); + $printOutput = $this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL; $process = $this->localMachineHelper->execute([ 'git', 'clone', '--depth=1', - $vcsUrl, + $vcsUrls[0], $artifactDir, - ], $outputCallback, null, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + ], $outputCallback, null, $printOutput); if (!$process->isSuccessful()) { throw new AcquiaCliException('Failed to clone repository from the Cloud Platform: {message}', ['message' => $process->getErrorOutput()]); } - $process = $this->localMachineHelper->execute([ - 'git', - 'fetch', - '--depth=1', - '--update-head-ok', - $vcsUrl, - $vcsPath . ':' . $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); - if (!$process->isSuccessful()) { - // Remote branch does not exist. Just create it locally. This will create - // the new branch off of the current commit. + + // Fetch the branch tip from every destination so out-of-sync remotes + // are detected before the artifact is built and pushed. + $tips = []; + foreach ($vcsUrls as $vcsUrl) { + $outputCallback('out', "Fetching $vcsPath from $vcsUrl"); + $process = $this->localMachineHelper->execute([ + 'git', + 'fetch', + '--depth=1', + $vcsUrl, + $vcsPath, + ], $outputCallback, $artifactDir, $printOutput); + if (!$process->isSuccessful()) { + // The branch does not exist on this remote yet. The push will + // create it. + continue; + } + $process = $this->localMachineHelper->execute([ + 'git', + 'rev-parse', + 'FETCH_HEAD', + ], null, $artifactDir, false); + if ($process->isSuccessful() && trim($process->getOutput()) !== '') { + $tips[$vcsUrl] = trim($process->getOutput()); + } + } + + if ($tips === []) { + // The branch does not exist on any remote. Just create it locally. + // This will create the new branch off of the current commit. $process = $this->localMachineHelper->execute([ 'git', 'checkout', '-b', $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + ], $outputCallback, $artifactDir, $printOutput); } else { + $baseTip = $this->determineBaseTip($tips, $vcsPath, $artifactDir); $process = $this->localMachineHelper->execute([ 'git', 'checkout', + '-B', $vcsPath, - ], $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); + $baseTip, + ], $outputCallback, $artifactDir, $printOutput); } if (!$process->isSuccessful()) { throw new AcquiaCliException("Could not checkout $vcsPath branch locally: {message}", ['message' => $process->getErrorOutput() . $process->getOutput()]); @@ -392,12 +418,69 @@ private function generateCommitMessage(string $commitHash): array|string return "Automated commit by Acquia CLI (source commit: $commitHash)"; } + /** + * Pick the branch tip to base the artifact on. + * + * Ensures every destination can fast-forward to the artifact commit. If + * the tips differ but are ancestor-related, the most advanced tip wins. + * Truly diverged tips abort the push before anything is built. + * + * @param array $tips + */ + private function determineBaseTip(array $tips, string $vcsPath, string $artifactDir): string + { + $uniqueTips = array_values(array_unique($tips)); + if (count($uniqueTips) === 1) { + return $uniqueTips[0]; + } + + // The tips differ. Deepen the shallow history so ancestry between + // them can be established. + foreach (array_keys($tips) as $vcsUrl) { + $this->localMachineHelper->execute([ + 'git', + 'fetch', + '--deepen=50', + $vcsUrl, + $vcsPath, + ], null, $artifactDir, false); + } + foreach ($uniqueTips as $candidate) { + foreach ($uniqueTips as $other) { + if ($other === $candidate) { + continue; + } + $process = $this->localMachineHelper->execute([ + 'git', + 'merge-base', + '--is-ancestor', + $other, + $candidate, + ], null, $artifactDir, false); + if (!$process->isSuccessful()) { + continue 2; + } + } + return $candidate; + } + + $remoteTips = []; + foreach ($tips as $vcsUrl => $tip) { + $remoteTips[] = "$vcsUrl ($tip)"; + } + throw new AcquiaCliException('The destination git repositories are out of sync for the {branch} branch: {tips}. Reconcile them (e.g. delete the stale artifact branch from the out-of-date remote or push the desired tip to it) and try again.', [ + 'branch' => $vcsPath, + 'tips' => implode(', ', $remoteTips), + ]); + } + /** * Push the artifact. */ private function pushArtifact(Closure $outputCallback, string $artifactDir, array $vcsUrls, string $destGitBranch): void { $this->localMachineHelper->checkRequiredBinariesExist(['git']); + $failures = []; foreach ($vcsUrls as $vcsUrl) { $outputCallback('out', "Pushing changes to Acquia Git ($vcsUrl)"); $args = [ @@ -408,9 +491,14 @@ private function pushArtifact(Closure $outputCallback, string $artifactDir, arra ]; $process = $this->localMachineHelper->execute($args, $outputCallback, $artifactDir, ($this->output->getVerbosity() > OutputInterface::VERBOSITY_NORMAL)); if (!$process->isSuccessful()) { - throw new AcquiaCliException("Unable to push artifact: {message}", ['message' => $process->getOutput() . $process->getErrorOutput()]); + // Keep pushing to the remaining remotes so a single failure + // does not leave them further out of sync. + $failures[] = "$vcsUrl: " . $process->getOutput() . $process->getErrorOutput(); } } + if ($failures !== []) { + throw new AcquiaCliException("Unable to push artifact: {message}", ['message' => implode(PHP_EOL, $failures)]); + } } /** diff --git a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php index d5b738c18..118e89a84 100644 --- a/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php +++ b/tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php @@ -6,6 +6,7 @@ use Acquia\Cli\Command\CommandBase; use Acquia\Cli\Command\Push\PushArtifactCommand; +use Acquia\Cli\Exception\AcquiaCliException; use Acquia\Cli\Tests\Commands\Pull\PullCommandTestBase; use PHPUnit\Framework\Attributes\DataProvider; use Prophecy\Argument; @@ -169,6 +170,127 @@ public function testPushArtifactWithArgs(): void $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); } + public function testPushArtifactToDivergedRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + touch(Path::join($this->projectDir, 'composer.json')); + mkdir(Path::join($this->projectDir, 'docroot')); + $this->createMockGitConfigFile(); + $fs = $this->prophet->prophesize(Filesystem::class); + $localMachineHelper->getFilesystem()->willReturn($fs); + $this->mockExecuteGitStatus(false, $localMachineHelper, $this->projectDir); + $this->mockGetLocalCommitHash($localMachineHelper, $this->projectDir, 'abc123'); + $localMachineHelper->checkRequiredBinariesExist(['git']) + ->shouldBeCalled(); + $artifactDir = Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + $this->mockCloneShallow($localMachineHelper, 'master', $destinationGitUrls, $artifactDir, true, [ + $destinationGitUrls[0] => 'sha1', + $destinationGitUrls[1] => 'sha2', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + $this->mockGitMergeBase($localMachineHelper, 'sha2', 'sha1', false); + $this->mockGitMergeBase($localMachineHelper, 'sha1', 'sha2', false); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessageMatches('/out of sync/'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + + public function testPushArtifactWithBranchMissingOnFirstRemote(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, true, true, [ + $destinationGitUrls[0] => null, + $destinationGitUrls[1] => 'secondremotesha', + ]); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactWithRemoteBehind(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, true, true, [ + $destinationGitUrls[0] => 'newsha', + $destinationGitUrls[1] => 'oldsha', + ]); + $this->mockGitDeepen($localMachineHelper, 'master', $destinationGitUrls); + $this->mockGitMergeBase($localMachineHelper, 'oldsha', 'newsha', true); + $this->mockGitCheckoutBase($localMachineHelper, 'master', 'newsha'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactWithNewBranchOnAllRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'feature-1-build', $destinationGitUrls, 'feature-1-build:feature-1-build', true, true, true, true, [ + $destinationGitUrls[0] => null, + $destinationGitUrls[1] => null, + ]); + $this->executeCommand([ + '--destination-git-branch' => 'feature-1-build', + '--destination-git-urls' => $destinationGitUrls, + ]); + + $output = $this->getDisplay(); + + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example1/cli.git)', $output); + $this->assertStringContainsString('Pushing changes to Acquia Git (https://github.com/example2/cli.git)', $output); + } + + public function testPushArtifactPushFailureStillPushesRemainingRemotes(): void + { + $destinationGitUrls = [ + 'https://github.com/example1/cli.git', + 'https://github.com/example2/cli.git', + ]; + $localMachineHelper = $this->mockLocalMachineHelper(); + $this->setUpPushArtifact($localMachineHelper, 'master', $destinationGitUrls, 'master:master', true, true, false); + $artifactDir = Path::join(sys_get_temp_dir(), 'acli-push-artifact'); + $this->mockGitPush($destinationGitUrls, $localMachineHelper, $artifactDir, 'master:master', true, [$destinationGitUrls[0]]); + + $this->expectException(AcquiaCliException::class); + $this->expectExceptionMessageMatches('~https://github\.com/example1/cli\.git~'); + $this->executeCommand([ + '--destination-git-branch' => 'master', + '--destination-git-urls' => $destinationGitUrls, + ]); + } + public function testPushArtifactNoPush(): void { $applications = $this->mockRequest('getApplications'); @@ -243,7 +365,7 @@ public function testPushArtifactNoClone(): void $this->assertStringNotContainsString('Adding and committing changed files', $output); $this->assertStringNotContainsString('Pushing changes to Acquia Git (site@svn-3.hosted.acquia-sites.com:site.git)', $output); } - protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $destGitRef = 'master:master', bool $clone = true, bool $commit = true, bool $push = true, bool $printOutput = true): void + protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $destGitRef = 'master:master', bool $clone = true, bool $commit = true, bool $push = true, bool $printOutput = true, ?array $tips = null): void { touch(Path::join($this->projectDir, 'composer.json')); mkdir(Path::join($this->projectDir, 'docroot')); @@ -264,7 +386,7 @@ protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string if ($clone) { $this->mockLocalGitConfig($localMachineHelper, $artifactDir, $printOutput); - $this->mockCloneShallow($localMachineHelper, $vcsPath, $vcsUrls[0], $artifactDir, $printOutput); + $this->mockCloneShallow($localMachineHelper, $vcsPath, $vcsUrls, $artifactDir, $printOutput, $tips); } if ($commit) { $this->mockGitAddCommit($localMachineHelper, $artifactDir, $commitHash, $printOutput); @@ -274,35 +396,110 @@ protected function setUpPushArtifact(ObjectProphecy $localMachineHelper, string } } - protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $vcsPath, string $vcsUrl, string $artifactDir, bool $printOutput = true): void + /** + * @param array|null $tips + * Map of vcs url to the branch tip sha on that remote. A null value + * means the branch does not exist on that remote. Defaults to every + * url sharing the same tip. + */ + protected function mockCloneShallow(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls, string $artifactDir, bool $printOutput = true, ?array $tips = null): void { + if ($tips === null) { + $tips = array_fill_keys($vcsUrls, 'mainbranchsha'); + } $process = $this->prophet->prophesize(Process::class); $process->isSuccessful()->willReturn(true)->shouldBeCalled(); $localMachineHelper->execute([ 'git', 'clone', '--depth=1', - $vcsUrl, + $vcsUrls[0], $artifactDir, ], Argument::type('callable'), null, $printOutput) ->willReturn($process->reveal())->shouldBeCalled(); - $localMachineHelper->execute([ - 'git', - 'fetch', - '--depth=1', - '--update-head-ok', - $vcsUrl, - $vcsPath . ':' . $vcsPath, - ], Argument::type('callable'), Argument::type('string'), $printOutput) - ->willReturn($process->reveal())->shouldBeCalled(); + + $revParseProcesses = []; + foreach ($vcsUrls as $vcsUrl) { + $tip = $tips[$vcsUrl]; + $fetchProcess = $this->mockProcess($tip !== null); + $localMachineHelper->execute([ + 'git', + 'fetch', + '--depth=1', + $vcsUrl, + $vcsPath, + ], Argument::type('callable'), Argument::type('string'), $printOutput) + ->willReturn($fetchProcess->reveal())->shouldBeCalled(); + if ($tip !== null) { + $revParseProcess = $this->mockProcess(); + $revParseProcess->getOutput()->willReturn($tip . PHP_EOL); + $revParseProcesses[] = $revParseProcess->reveal(); + } + } + if ($revParseProcesses !== []) { + $localMachineHelper->execute([ + 'git', + 'rev-parse', + 'FETCH_HEAD', + ], null, Argument::type('string'), false) + ->willReturn(...$revParseProcesses)->shouldBeCalled(); + } + + $uniqueTips = array_values(array_unique(array_filter($tips, static fn ($tip) => $tip !== null))); + if ($uniqueTips === []) { + $localMachineHelper->execute([ + 'git', + 'checkout', + '-b', + $vcsPath, + ], Argument::type('callable'), Argument::type('string'), $printOutput) + ->willReturn($process->reveal())->shouldBeCalled(); + } elseif (count($uniqueTips) === 1) { + $this->mockGitCheckoutBase($localMachineHelper, $vcsPath, $uniqueTips[0], $printOutput); + } + // Multiple distinct tips: the test mocks deepen, merge-base, and + // checkout calls itself. + } + + protected function mockGitCheckoutBase(ObjectProphecy $localMachineHelper, string $vcsPath, string $baseTip, bool $printOutput = true): void + { + $process = $this->mockProcess(); $localMachineHelper->execute([ 'git', 'checkout', + '-B', $vcsPath, + $baseTip, ], Argument::type('callable'), Argument::type('string'), $printOutput) ->willReturn($process->reveal())->shouldBeCalled(); } + protected function mockGitDeepen(ObjectProphecy $localMachineHelper, string $vcsPath, array $vcsUrls): void + { + foreach ($vcsUrls as $vcsUrl) { + $localMachineHelper->execute([ + 'git', + 'fetch', + '--deepen=50', + $vcsUrl, + $vcsPath, + ], null, Argument::type('string'), false) + ->willReturn($this->mockProcess()->reveal())->shouldBeCalled(); + } + } + + protected function mockGitMergeBase(ObjectProphecy $localMachineHelper, string $ancestor, string $descendant, bool $isAncestor): void + { + $localMachineHelper->execute([ + 'git', + 'merge-base', + '--is-ancestor', + $ancestor, + $descendant, + ], null, Argument::type('string'), false) + ->willReturn($this->mockProcess($isAncestor)->reveal())->shouldBeCalled(); + } + protected function mockLocalGitConfig(ObjectProphecy $localMachineHelper, string $artifactDir, bool $printOutput = true): void { $process = $this->prophet->prophesize(Process::class); @@ -406,10 +603,10 @@ protected function mockReadComposerJson(ObjectProphecy $localMachineHelper, stri ->willReturn($composerJson); } - protected function mockGitPush(array $gitUrls, ObjectProphecy $localMachineHelper, string $artifactDir, string $destGitRef, bool $printOutput): void + protected function mockGitPush(array $gitUrls, ObjectProphecy $localMachineHelper, string $artifactDir, string $destGitRef, bool $printOutput, array $failingUrls = []): void { - $process = $this->mockProcess(); foreach ($gitUrls as $gitUrl) { + $process = $this->mockProcess(!in_array($gitUrl, $failingUrls, true)); $localMachineHelper->execute([ 'git', 'push',