From 146b862a3dd9a1e1f6e089cc34d15364fddd021a Mon Sep 17 00:00:00 2001 From: Timon de Groot Date: Tue, 20 Jan 2026 14:32:23 +0100 Subject: [PATCH] Brancher: Handle already cancelled error It doesn't make any sense to fail on brancher already being cancelled or not existing. --- src/Brancher/BrancherHypernodeManager.php | 15 +++- .../Brancher/BrancherHypernodeManagerTest.php | 88 +++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/Brancher/BrancherHypernodeManager.php b/src/Brancher/BrancherHypernodeManager.php index dc93ad8..394c34e 100644 --- a/src/Brancher/BrancherHypernodeManager.php +++ b/src/Brancher/BrancherHypernodeManager.php @@ -337,7 +337,20 @@ public function cancel(string ...$brancherHypernodes): void { foreach ($brancherHypernodes as $brancherHypernode) { $this->log->info(sprintf('Stopping brancher Hypernode %s...', $brancherHypernode)); - $this->hypernodeClient->brancherApp->cancel($brancherHypernode); + try { + $this->hypernodeClient->brancherApp->cancel($brancherHypernode); + } catch (HypernodeApiClientException $e) { + // If the brancher is already cancelled or not found, that's fine - + // our goal was to cancel it anyway + if ($e->getCode() === 404 || str_contains($e->getMessage(), 'has already been cancelled')) { + $this->log->info(sprintf( + 'Brancher Hypernode %s was already cancelled or not found, skipping.', + $brancherHypernode + )); + continue; + } + throw $e; + } } } } diff --git a/tests/Unit/Brancher/BrancherHypernodeManagerTest.php b/tests/Unit/Brancher/BrancherHypernodeManagerTest.php index 719372b..e226410 100644 --- a/tests/Unit/Brancher/BrancherHypernodeManagerTest.php +++ b/tests/Unit/Brancher/BrancherHypernodeManagerTest.php @@ -7,6 +7,7 @@ use Hypernode\Api\Exception\HypernodeApiClientException; use Hypernode\Api\HypernodeClient; use Hypernode\Api\Resource\Logbook\Flow; +use Hypernode\Api\Service\BrancherApp; use Hypernode\Api\Service\Logbook; use Hypernode\Deploy\Brancher\BrancherHypernodeManager; use Hypernode\Deploy\Exception\CreateBrancherHypernodeFailedException; @@ -20,6 +21,7 @@ class BrancherHypernodeManagerTest extends TestCase { private LoggerInterface&MockObject $logger; private HypernodeClient&MockObject $hypernodeClient; + private BrancherApp&MockObject $brancherApp; private Logbook&MockObject $logbook; private TestSshPoller $sshPoller; private BrancherHypernodeManager $manager; @@ -28,7 +30,9 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->hypernodeClient = $this->createMock(HypernodeClient::class); + $this->brancherApp = $this->createMock(BrancherApp::class); $this->logbook = $this->createMock(Logbook::class); + $this->hypernodeClient->brancherApp = $this->brancherApp; $this->hypernodeClient->logbook = $this->logbook; $this->sshPoller = new TestSshPoller(); $this->sshPoller->setMicrotime(1000.0); @@ -258,4 +262,88 @@ private function createFlow(string $name, string $state): Flow 'state' => $state, ]); } + + public function testCancelSucceeds(): void + { + $this->brancherApp->expects($this->once()) + ->method('cancel') + ->with('test-brancher'); + + $this->manager->cancel('test-brancher'); + } + + public function testCancelAlreadyCancelledBrancherContinues(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(400); + $response->method('getBody')->willReturn('["Brancher app test-brancher has already been cancelled."]'); + $exception = new HypernodeApiClientException($response); + + $this->brancherApp->expects($this->once()) + ->method('cancel') + ->with('test-brancher') + ->willThrowException($exception); + + $this->logger->expects($this->exactly(2)) + ->method('info'); + + // Should not throw + $this->manager->cancel('test-brancher'); + } + + public function testCancelNotFoundBrancherContinues(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(404); + $response->method('getBody')->willReturn('Not Found'); + $exception = new HypernodeApiClientException($response); + + $this->brancherApp->expects($this->once()) + ->method('cancel') + ->with('test-brancher') + ->willThrowException($exception); + + $this->logger->expects($this->exactly(2)) + ->method('info'); + + // Should not throw + $this->manager->cancel('test-brancher'); + } + + public function testCancelOtherClientErrorPropagates(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(403); + $response->method('getBody')->willReturn('Forbidden'); + $exception = new HypernodeApiClientException($response); + + $this->brancherApp->expects($this->once()) + ->method('cancel') + ->with('test-brancher') + ->willThrowException($exception); + + $this->expectException(HypernodeApiClientException::class); + + $this->manager->cancel('test-brancher'); + } + + public function testCancelMultipleBranchersPartialFailure(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(400); + $response->method('getBody')->willReturn('["Brancher app brancher-1 has already been cancelled."]'); + $exception = new HypernodeApiClientException($response); + + $this->brancherApp->expects($this->exactly(2)) + ->method('cancel') + ->willReturnCallback(function (string $name) use ($exception) { + if ($name === 'brancher-1') { + throw $exception; + } + // brancher-2 succeeds + }); + + // Should not throw, should continue to second brancher + $this->manager->cancel('brancher-1', 'brancher-2'); + } }