fix: ensure jobs do not retry indefinitely by default, fix undefined values in error messages (#9605)

AlessioGr · web-flow · commit a89d54454a9c · 2024-12-02T22:05:48.000Z
## Fix default retries By default, if no `retries` property has been set, jobs / tasks should not be retried. This was not the case previously, as the `maxRetries` variable was `undefined`, causing jobs to retry endlessly. This PR sets them to `0` by default. Additionally, this fixes some undesirable behavior of the workflow retries property. Workflow retries now act as **maximum**, workflow-level retries. Only tasks that do not have a retry property set will inherit the workflow-level retries. ## Fix error messages Previously, you were able to encounter error messages with undefined values like these: ![CleanShot 2024-11-28 at 15 23 37@2x](https://github.com/user-attachments/assets/81617ca8-11de-4d35-b9bf-cc6c5bc515be) Reason is that it was always using `job.workflowSlug` for the error messages. However, if you queue a task directly, without a workflow, `job.workflowSlug` is undefined and `job.taskSlug` should be used instead. This PR then gets rid of the second undefined value by ensuring that `maxRetries´ is never undefined
diff --git a/docs/jobs-queue/tasks.mdx b/docs/jobs-queue/tasks.mdx
@@ -30,7 +30,7 @@ Simply add a task to the `jobs.tasks` array in your Payload config. A task consi
 | `label` | Define a human-friendly label for this task. |
 | `onFail` | Function to be executed if the task fails. |
 | `onSuccess` | Function to be executed if the task succeeds. |
-| `retries` | Specify the number of times that this step should be retried if it fails. |
+| `retries` | Specify the number of times that this step should be retried if it fails. If this is undefined, the task will either inherit the retries from the workflow or have no retries. If this is 0, the task will not be retried. By default, this is undefined. |
 
 The logic for the Task is defined in the `handler` - which can be defined as a function, or a path to a function. The `handler` will run once a worker picks picks up a Job that includes this task.
 
diff --git a/docs/jobs-queue/workflows.mdx b/docs/jobs-queue/workflows.mdx
@@ -2,7 +2,7 @@
 title: Workflows
 label: Workflows
 order: 30
-desc: A Task is a distinct function declaration that can be run within Payload's Jobs Queue. 
+desc: A Task is a distinct function declaration that can be run within Payload's Jobs Queue.
 keywords: jobs queue, application framework, typescript, node, react, nextjs
 ---
 
@@ -30,6 +30,7 @@ To define a JS-based workflow, simply add a workflow to the `jobs.wokflows` arra
 | `interfaceName` | You can use interfaceName to change the name of the interface that is generated for this workflow. By default, this is "Workflow" + the capitalized workflow slug. |
 | `label` | Define a human-friendly label for this workflow. |
 | `queue` | Optionally, define the queue name that this workflow should be tied to. Defaults to "default". |
+| `retries` | You can define `retries` on the workflow level, which will enforce that the workflow can only fail up to that number of retries. If a task does not have retries specified, it will inherit the retry count as specified on the workflow. You can specify `0` as `workflow` retries, which will disregard all `task` retry specifications and fail the entire workflow on any task failure. You can leave `workflow` retries as undefined, in which case, the workflow will respect what each task dictates as their own retry count. By default this is undefined, meaning workflows retries are defined by their tasks |
 
 Example:
 
diff --git a/packages/payload/src/queues/config/types/taskTypes.ts b/packages/payload/src/queues/config/types/taskTypes.ts
@@ -60,7 +60,14 @@ export type TaskHandlerResults = {
 // Helper type to create correct argument type for the function corresponding to each task.
 export type RunTaskFunctionArgs<TTaskSlug extends keyof TypedJobs['tasks']> = {
   input?: TaskInput<TTaskSlug>
-  retries?: number | RetryConfig
+  /**
+   * Specify the number of times that this task should be retried if it fails for any reason.
+   * If this is undefined, the task will either inherit the retries from the workflow or have no retries.
+   * If this is 0, the task will not be retried.
+   *
+   * @default By default, tasks are not retried and `retries` is `undefined`.
+   */
+  retries?: number | RetryConfig | undefined
 }
 
 export type RunTaskFunction<TTaskSlug extends keyof TypedJobs['tasks']> = (
@@ -76,7 +83,14 @@ export type RunInlineTaskFunction = <TTaskInput extends object, TTaskOutput exte
   taskID: string,
   taskArgs: {
     input?: TTaskInput
-    retries?: number | RetryConfig
+    /**
+     * Specify the number of times that this task should be retried if it fails for any reason.
+     * If this is undefined, the task will either inherit the retries from the workflow or have no retries.
+     * If this is 0, the task will not be retried.
+     *
+     * @default By default, tasks are not retried and `retries` is `undefined`.
+     */
+    retries?: number | RetryConfig | undefined
     // This is the same as TaskHandler, but typed out explicitly in order to improve type inference
     task: (args: { input: TTaskInput; job: RunningJob<any>; req: PayloadRequest }) =>
       | {
@@ -162,8 +176,12 @@ export type TaskConfig<
   outputSchema?: Field[]
   /**
    * Specify the number of times that this step should be retried if it fails.
+   * If this is undefined, the task will either inherit the retries from the workflow or have no retries.
+   * If this is 0, the task will not be retried.
+   *
+   * @default By default, tasks are not retried and `retries` is `undefined`.
    */
-  retries?: number | RetryConfig
+  retries?: number | RetryConfig | undefined
   /**
    * Define a slug-based name for this job. This slug needs to be unique among both tasks and workflows.
    */
diff --git a/packages/payload/src/queues/config/types/workflowJSONTypes.ts b/packages/payload/src/queues/config/types/workflowJSONTypes.ts
@@ -14,6 +14,11 @@ export type WorkflowStep<
    * Each task needs to have a unique ID to track its status
    */
   id: string
+  /**
+   * Specify the number of times that this workflow should be retried if it fails for any reason.
+   *
+   * @default By default, workflows are not retried and `retries` is `0`.
+   */
   retries?: number | RetryConfig
 } & (
   | {
diff --git a/packages/payload/src/queues/config/types/workflowTypes.ts b/packages/payload/src/queues/config/types/workflowTypes.ts
@@ -114,9 +114,14 @@ export type WorkflowConfig<TWorkflowSlugOrInput extends keyof TypedJobs['workflo
    */
   queue?: string
   /**
-   * Specify the number of times that this workflow should be retried if it fails for any reason.
+   * You can define `retries` on the workflow level, which will enforce that the workflow can only fail up to that number of retries. If a task does not have retries specified, it will inherit the retry count as specified on the workflow.
+   *
+   * You can specify `0` as `workflow` retries, which will disregard all `task` retry specifications and fail the entire workflow on any task failure.
+   * You can leave `workflow` retries as undefined, in which case, the workflow will respect what each task dictates as their own retry count.
+   *
+   * @default undefined. By default, workflows retries are defined by their tasks
    */
-  retries?: number | RetryConfig
+  retries?: number | RetryConfig | undefined
   /**
    * Define a slug-based name for this job.
    */
diff --git a/packages/payload/src/queues/operations/runJobs/index.ts b/packages/payload/src/queues/operations/runJobs/index.ts
@@ -171,7 +171,8 @@ export const runJobs = async ({
       workflowHandler = await importHandlerPath<typeof workflowHandler>(workflowConfig.handler)
 
       if (!workflowHandler) {
-        const errorMessage = `Can't find runner while importing with the path ${workflowConfig.handler} in job type ${job.workflowSlug}.`
+        const jobLabel = job.workflowSlug || `Task: ${job.taskSlug}`
+        const errorMessage = `Can't find runner while importing with the path ${workflowConfig.handler} in job type ${jobLabel}.`
         req.payload.logger.error(errorMessage)
 
         await updateJob({
diff --git a/packages/payload/src/queues/operations/runJobs/runJob/getRunTaskFunction.ts b/packages/payload/src/queues/operations/runJobs/runJob/getRunTaskFunction.ts
@@ -107,7 +107,7 @@ export async function handleTaskFailed({
     }
   }
 
-  if (taskStatus && !taskStatus.complete && taskStatus.totalTried >= maxRetries) {
+  if (!taskStatus?.complete && (taskStatus?.totalTried ?? 0) >= maxRetries) {
     state.reachedMaxRetries = true
 
     await updateJob({
@@ -182,9 +182,21 @@ export const getRunTaskFunction = <TIsInline extends boolean>(
           throw new Error(`Task ${taskSlug} not found in workflow ${job.workflowSlug}`)
         }
       }
-      const maxRetries: number =
+      let maxRetries: number =
         typeof retriesConfig === 'object' ? retriesConfig?.attempts : retriesConfig
 
+      if (maxRetries === undefined || maxRetries === null) {
+        // Inherit retries from workflow config, if they are undefined and the workflow config has retries configured
+        if (workflowConfig.retries !== undefined && workflowConfig.retries !== null) {
+          maxRetries =
+            typeof workflowConfig.retries === 'object'
+              ? workflowConfig.retries.attempts
+              : workflowConfig.retries
+        } else {
+          maxRetries = 0
+        }
+      }
+
       const taskStatus: null | SingleTaskStatus<string> = job?.taskStatus?.[taskSlug]
         ? job.taskStatus[taskSlug][taskID]
         : null
diff --git a/packages/payload/src/queues/operations/runJobs/runJob/handleWorkflowError.ts b/packages/payload/src/queues/operations/runJobs/runJob/handleWorkflowError.ts
@@ -23,13 +23,25 @@ export function handleWorkflowError({
 }): {
   hasFinalError: boolean
 } {
+  const jobLabel = job.workflowSlug || `Task: ${job.taskSlug}`
+
   let hasFinalError = state.reachedMaxRetries // If any TASK reached max retries, the job has an error
-  const maxRetries =
-    typeof workflowConfig.retries === 'object'
+  const maxWorkflowRetries: number =
+    (typeof workflowConfig.retries === 'object'
       ? workflowConfig.retries.attempts
-      : workflowConfig.retries
+      : workflowConfig.retries) ?? undefined
+
+  if (
+    maxWorkflowRetries !== undefined &&
+    maxWorkflowRetries !== null &&
+    job.totalTried >= maxWorkflowRetries
+  ) {
+    hasFinalError = true
+    state.reachedMaxRetries = true
+  }
+
   // Now let's handle workflow retries
-  if (!hasFinalError && workflowConfig.retries) {
+  if (!hasFinalError) {
     if (job.waitUntil) {
       // Check if waitUntil is in the past
       const waitUntil = new Date(job.waitUntil)
@@ -38,26 +50,22 @@ export function handleWorkflowError({
         delete job.waitUntil
       }
     }
-    if (job.totalTried >= maxRetries) {
-      state.reachedMaxRetries = true
-      hasFinalError = true
-    } else {
-      // Job will retry. Let's determine when!
-      const waitUntil: Date = calculateBackoffWaitUntil({
-        retriesConfig: workflowConfig.retries,
-        totalTried: job.totalTried ?? 0,
-      })
 
-      // Update job's waitUntil only if this waitUntil is later than the current one
-      if (!job.waitUntil || waitUntil > new Date(job.waitUntil)) {
-        job.waitUntil = waitUntil.toISOString()
-      }
+    // Job will retry. Let's determine when!
+    const waitUntil: Date = calculateBackoffWaitUntil({
+      retriesConfig: workflowConfig.retries,
+      totalTried: job.totalTried ?? 0,
+    })
+
+    // Update job's waitUntil only if this waitUntil is later than the current one
+    if (!job.waitUntil || waitUntil > new Date(job.waitUntil)) {
+      job.waitUntil = waitUntil.toISOString()
     }
   }
 
   req.payload.logger.error({
     err: error,
-    msg: `Error running job ${job.workflowSlug} ${job.taskSlug} id: ${job.id} attempt ${job.totalTried}/${maxRetries}`,
+    msg: `Error running job ${jobLabel} id: ${job.id} attempt ${job.totalTried + 1}${maxWorkflowRetries !== undefined ? '/' + (maxWorkflowRetries + 1) : ''}`,
   })
 
   return {
diff --git a/test/live-preview/payload-types.ts b/test/live-preview/payload-types.ts
diff --git a/test/queues/config.ts b/test/queues/config.ts
diff --git a/test/queues/int.spec.ts b/test/queues/int.spec.ts
diff --git a/test/queues/payload-types.ts b/test/queues/payload-types.ts
