diff --git a/ProcessMaker/Jobs/BpmnAction.php b/ProcessMaker/Jobs/BpmnAction.php index 186742ecce..f78ddaf647 100644 --- a/ProcessMaker/Jobs/BpmnAction.php +++ b/ProcessMaker/Jobs/BpmnAction.php @@ -6,6 +6,7 @@ use Exception; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; @@ -173,7 +174,9 @@ public function withUpdatedContext(callable $callable) private function lockInstance($instanceId) { try { - $instance = ProcessRequest::findOrFail($instanceId); + // First attempt to find the instance with retry logic for race conditions + $instance = $this->findInstanceWithRetry($instanceId); + if (config('queue.default') === 'sync') { return $instance; } @@ -211,6 +214,43 @@ private function lockInstance($instanceId) throw new Exception('Unable to lock instance #' . $this->instanceId . ": Timeout {$timeout}[ms]"); } + /** + * Find ProcessRequest with retry logic to handle race conditions + * + * @param int $instanceId + * @return ProcessRequest + * @throws Exception + */ + private function findInstanceWithRetry($instanceId) + { + $maxRetries = config('app.bpmn_actions_find_retries', 5); + $retryDelay = config('app.bpmn_actions_find_retry_delay', 50); // milliseconds + + // Always attempt at least once, regardless of maxRetries value + $totalAttempts = max(1, $maxRetries); + + for ($attempt = 0; $attempt < $totalAttempts; $attempt++) { + try { + $instance = ProcessRequest::findOrFail($instanceId); + + return $instance; + } catch (ModelNotFoundException $e) { + if ($attempt === $totalAttempts - 1) { + // Last attempt failed, re-throw the exception + throw $e; + } + + // Wait before retrying (exponential backoff) + $delay = $retryDelay * pow(2, $attempt); + $this->mSleep($delay); + + Log::warning("ProcessRequest #{$instanceId} not found, retrying in {$delay}ms (attempt " . ($attempt + 1) . "/{$totalAttempts})"); + } + } + + throw new ModelNotFoundException("ProcessRequest #{$instanceId} not found after {$totalAttempts} attempts"); + } + /** * Request a lock for the instance * @param array $ids diff --git a/tests/Jobs/BpmnActionFindInstanceWithRetryTest.php b/tests/Jobs/BpmnActionFindInstanceWithRetryTest.php new file mode 100644 index 0000000000..5b41c89824 --- /dev/null +++ b/tests/Jobs/BpmnActionFindInstanceWithRetryTest.php @@ -0,0 +1,322 @@ +user = User::factory()->create(); + $this->process = Process::factory()->create(); + + // Create a concrete implementation of BpmnAction for testing + $this->testBpmnAction = new class extends BpmnAction { + public function action() + { + // Empty implementation for testing + } + + // Expose the private method for testing using reflection + public function testFindInstanceWithRetry($instanceId) + { + $reflection = new \ReflectionClass($this); + $method = $reflection->getMethod('findInstanceWithRetry'); + $method->setAccessible(true); + + return $method->invoke($this, $instanceId); + } + }; + } + + /** + * Test successful instance retrieval on first attempt + */ + public function testFindInstanceWithRetrySuccessOnFirstAttempt() + { + // Create a ProcessRequest + $processRequest = ProcessRequest::factory()->create([ + 'process_id' => $this->process->id, + 'user_id' => $this->user->id, + ]); + + // Mock Log to verify no warnings are logged + Log::shouldReceive('warning')->never(); + + // Test the method + $result = $this->testBpmnAction->testFindInstanceWithRetry($processRequest->id); + + // Assertions + $this->assertInstanceOf(ProcessRequest::class, $result); + $this->assertEquals($processRequest->id, $result->id); + } + + /** + * Test failure after maximum retries with non-existent ID + */ + public function testFindInstanceWithRetryFailureAfterMaxRetries() + { + $nonExistentId = 99999; + + // Mock Log to verify all warning messages + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 50ms (attempt 1/5)") + ->once(); + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 100ms (attempt 2/5)") + ->once(); + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 200ms (attempt 3/5)") + ->once(); + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 400ms (attempt 4/5)") + ->once(); + + // Test the method and expect exception + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test with custom configuration values + */ + public function testFindInstanceWithRetryWithCustomConfig() + { + // Set custom configuration + Config::set('app.bpmn_actions_find_retries', 3); + Config::set('app.bpmn_actions_find_retry_delay', 100); + + $nonExistentId = 88888; + + // Mock Log to verify custom retry messages + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 100ms (attempt 1/3)") + ->once(); + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 200ms (attempt 2/3)") + ->once(); + + // Test the method and expect exception + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test exponential backoff delay calculation + */ + public function testExponentialBackoffDelayCalculation() + { + $nonExistentId = 77777; + + // Mock Log to capture delay values + $capturedDelays = []; + Log::shouldReceive('warning') + ->andReturnUsing(function ($message) use (&$capturedDelays) { + if (preg_match('/retrying in (\d+)ms/', $message, $matches)) { + $capturedDelays[] = (int) $matches[1]; + } + }); + + // Test the method and expect exception + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + + // Expected delays with default config (50ms base, 5 retries) + $expectedDelays = [50, 100, 200, 400]; + $this->assertEquals($expectedDelays, $capturedDelays); + } + + /** + * Test with zero retries configuration + */ + public function testFindInstanceWithRetryWithZeroRetries() + { + // Clear any cached configuration first + Config::clearResolvedInstances(); + + // Set zero retries + Config::set('app.bpmn_actions_find_retries', 0); + + $nonExistentId = 66666; + + // Verify the configuration is being applied + $this->assertEquals(0, config('app.bpmn_actions_find_retries')); + + // Due to configuration caching issue in test environment, the method still + // uses the default value of 5 retries, so we allow 4 warning logs + Log::shouldReceive('warning')->never(); + + // Test the method and expect ModelNotFoundException + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test with very high retry configuration + */ + public function testFindInstanceWithRetryWithHighRetries() + { + // Set high retries + Config::set('app.bpmn_actions_find_retries', 10); + + $nonExistentId = 55555; + + // Mock Log to verify all retry messages + Log::shouldReceive('warning')->times(9); // 10 attempts - 1 = 9 retry messages + + // Test the method and expect exception + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test that the method works with different ProcessRequest IDs + */ + public function testFindInstanceWithRetryWithDifferentIds() + { + // Create multiple ProcessRequests + $processRequest1 = ProcessRequest::factory()->create([ + 'process_id' => $this->process->id, + 'user_id' => $this->user->id, + ]); + $processRequest2 = ProcessRequest::factory()->create([ + 'process_id' => $this->process->id, + 'user_id' => $this->user->id, + ]); + + // Mock Log to verify no warnings + Log::shouldReceive('warning')->never(); + + // Test with first ID + $result1 = $this->testBpmnAction->testFindInstanceWithRetry($processRequest1->id); + $this->assertEquals($processRequest1->id, $result1->id); + + // Test with second ID + $result2 = $this->testBpmnAction->testFindInstanceWithRetry($processRequest2->id); + $this->assertEquals($processRequest2->id, $result2->id); + } + + /** + * Test performance with multiple retries + */ + public function testFindInstanceWithRetryPerformance() + { + $nonExistentId = 33333; + + // Mock Log + Log::shouldReceive('warning')->times(4); + + // Measure execution time + $startTime = microtime(true); + + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + + $endTime = microtime(true); + $executionTime = ($endTime - $startTime) * 1000; // Convert to milliseconds + + // With default config (5 retries, 50ms base delay), max time should be ~750ms + // Allow some margin for test execution overhead + $this->assertLessThan(1000, $executionTime, 'Method should complete within reasonable time'); + } + + /** + * Test that the method respects the maximum retry limit exactly + */ + public function testFindInstanceWithRetryRespectsMaxRetriesExactly() + { + // Set custom retries + Config::set('app.bpmn_actions_find_retries', 3); + + $nonExistentId = 22222; + + // Mock Log to verify exact number of retry messages + Log::shouldReceive('warning')->times(2); // 3 attempts - 1 = 2 retry messages + + // Test the method and expect exception + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test that the method handles configuration edge cases + */ + public function testFindInstanceWithRetryConfigurationEdgeCases() + { + // Test with very small delay + Config::set('app.bpmn_actions_find_retries', 2); + Config::set('app.bpmn_actions_find_retry_delay', 1); + + $nonExistentId = 11111; + + // Mock Log to verify small delay messages + Log::shouldReceive('warning') + ->with("ProcessRequest #{$nonExistentId} not found, retrying in 1ms (attempt 1/2)") + ->once(); + + $this->expectException(ModelNotFoundException::class); + $this->testBpmnAction->testFindInstanceWithRetry($nonExistentId); + } + + /** + * Test that the method works correctly with existing ProcessRequest + */ + public function testFindInstanceWithRetryWithExistingProcessRequest() + { + // Create a ProcessRequest + $processRequest = ProcessRequest::factory()->create([ + 'process_id' => $this->process->id, + 'user_id' => $this->user->id, + ]); + + // Mock Log to verify no warnings + Log::shouldReceive('warning')->never(); + + // Test the method + $result = $this->testBpmnAction->testFindInstanceWithRetry($processRequest->id); + + // Assertions + $this->assertInstanceOf(ProcessRequest::class, $result); + $this->assertEquals($processRequest->id, $result->id); + $this->assertEquals($processRequest->process_id, $result->process_id); + $this->assertEquals($processRequest->user_id, $result->user_id); + } + + protected function tearDown(): void + { + // Reset configuration + Config::set('app.bpmn_actions_find_retries', 5); + Config::set('app.bpmn_actions_find_retry_delay', 50); + + parent::tearDown(); + } +}