Never let a throwable escape the workflow run
Build package / php-lint (8.2) (push) Successful in 58s
Build package / php-lint (8.3) (push) Successful in 41s
Build package / php-lint (8.4) (push) Successful in 36s
Build package / xml-lint (push) Successful in 12s
Build package / unit-tests (push) Successful in 38s
Build package / package (push) Successful in 58s

Job::start() calls setLastRun() before run() and clears `reserved_at` only
afterwards through setExecutionTime(), which is not in a finally block. Any
uncaught throwable therefore leaves the job reserved, and JobList::getNext()
skips reserved jobs until the reservation is 12 hours stale -- so a single
failing workflow takes the whole app offline for half a day, while last_run
still shows the start of that failed attempt.

Wrap both loops so a failure is logged with its exception and the remaining
users and workflows still run.
This commit is contained in:
Patrick Niebeling
2026-08-13 15:24:16 +02:00
parent 184c39d829
commit 80371d48fb
+24
View File
@@ -52,7 +52,21 @@ class WorkflowRunner {
} }
foreach ($byUser as $userId => $userWorkflows) { foreach ($byUser as $userId => $userWorkflows) {
// Nothing may escape this loop. Job::start() calls setLastRun()
// *before* run() and only clears `reserved_at` afterwards via
// setExecutionTime(), which is not in a finally — so a single
// uncaught throwable leaves the job reserved, and JobList::getNext()
// then skips it until the reservation is 12 hours stale. One bad
// workflow would silently take the whole app offline for half a day.
try {
$this->runForUser((string)$userId, $userWorkflows); $this->runForUser((string)$userId, $userWorkflows);
} catch (Throwable $e) {
$this->logger->error('Workflow run failed for {user}: ' . $e->getMessage(), [
'app' => 'workflow_deck_automation',
'user' => $userId,
'exception' => $e,
]);
}
} }
} }
@@ -78,7 +92,17 @@ class WorkflowRunner {
$this->impersonate($user); $this->impersonate($user);
try { try {
foreach ($workflows as $workflow) { foreach ($workflows as $workflow) {
// Same reasoning as in run(): one broken workflow must not
// stop the user's remaining ones.
try {
$this->runWorkflow($user, $workflow); $this->runWorkflow($user, $workflow);
} catch (Throwable $e) {
$this->logger->error('Workflow {id} failed: ' . $e->getMessage(), [
'app' => 'workflow_deck_automation',
'id' => $workflow->getId(),
'exception' => $e,
]);
}
} }
} finally { } finally {
$this->clearImpersonation(); $this->clearImpersonation();