Skip to content

FOUR-21497 - Fix script retry error handling for script microservice #7926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nolanpro wants to merge 4 commits into from
Closed

FOUR-21497 - Fix script retry error handling for script microservice #7926

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98858ca
Fix retries for sync requests
nolanpro Jan 22, 2025
2a74fe5
Make script tasks run synchronously
nolanpro Jan 22, 2025
d27abb2
Start refactoring for async script microservice
nolanpro Jan 25, 2025
ba02a61
WIP
nolanpro Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ProcessMaker/Jobs/ErrorHandling.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use Illuminate\Support\Arr;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Notification;
use ProcessMaker\Exception\ScriptException;
use ProcessMaker\Exception\ScriptTimeoutException;
use ProcessMaker\Models\Process;
use ProcessMaker\Models\ProcessRequest;
use ProcessMaker\Models\ProcessRequestToken;
Expand Down Expand Up @@ -204,4 +206,14 @@ public function setDefaultsFromDataSourceConfig(array $config)
'retry_wait_time' => Arr::get($endpointConfig, 'retry_wait_time', 5),
];
}

public static function convertResponseToException($result)
{
if ($result['status'] === 'error') {
if (str_starts_with($result['message'], 'Command exceeded timeout of')) {
throw new ScriptTimeoutException($result['message']);
}
throw new ScriptException($result['message']);
}
}
}
2 changes: 1 addition & 1 deletion ProcessMaker/Jobs/RunScriptTask.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e
'attempts' => $this->attemptNum,
],
];
$response = $script->runScript($data, $configuration, $token->getId(), $errorHandling->timeout(), 0, $metadata);
$response = $script->runScript($data, $configuration, $token->getId(), $errorHandling->timeout(), 1, $metadata);

@epdl epdl Jan 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nolanpro here we are forcing this to be sync. This is not the idea, we should control this in the callback response.

@nolanpro nolanpro Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@epdl in that case, should we remove ProcessMaker/Jobs/RunScriptTask.php and make this entirely synchronous from core's point of view? Do you see any point in using horizon/queues in core for scripts? Queueing one job that queues another job would make the process error prone


if (!config('script-runner-microservice.enabled') ||
($scriptExecutor && $scriptExecutor->type === ScriptExecutorType::Custom)) {
Expand Down
10 changes: 8 additions & 2 deletions ProcessMaker/ScriptRunners/ScriptMicroserviceRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Log;
use ProcessMaker\Exception\ConfigurationException;
use ProcessMaker\Exception\ScriptException;
use ProcessMaker\GenerateAccessToken;
use ProcessMaker\Jobs\ErrorHandling;
use ProcessMaker\Models\EnvironmentVariable;
use ProcessMaker\Models\Script;
use ProcessMaker\Models\User;
Expand Down Expand Up @@ -92,7 +92,13 @@ public function run($code, array $data, array $config, $timeout, $user, $sync, $

$response->throw();

return $response->json();
$result = $response->json();

if ($sync) {
ErrorHandling::convertResponseToException($result);
}

return $result;
}

private function getEnvironmentVariables(User $user)
Expand Down