fix(flow): parent job cannot be replaced (python) (#2417)

taskforcesh · Feb 13, 2024 · 2696ef8 · 2696ef8
1 parent f2e11b4
commit 2696ef8
Show file tree

Hide file tree

Showing 10 changed files with 101 additions and 34 deletions.
diff --git a/python/bullmq/error_code.py b/python/bullmq/error_code.py
@@ -8,3 +8,4 @@ class ErrorCode(Enum):
     JobPendingDependencies = -4
     ParentJobNotExist = -5
     JobLockMismatch = -6
+    ParentJobCannotBeReplaced = -7
diff --git a/python/bullmq/scripts.py b/python/bullmq/scripts.py
@@ -573,6 +573,8 @@ def finishedErrors(self, code: int, jobId: str, command: str, state: str) -> Typ
             return TypeError(f"Missing key for parent job {jobId}.{command}")
         elif code == ErrorCode.JobLockMismatch.value:
             return TypeError(f"Lock mismatch for job {jobId}. Cmd {command} from {state}")
+        elif code == ErrorCode.ParentJobCannotBeReplaced.value:
+            return TypeError(f"The parent job of job {jobId} cannot be replaced. {command}")
         else:
             return TypeError(f"Unknown code {str(code)} error for {jobId}.{command}")
 

diff --git a/src/classes/scripts.ts b/src/classes/scripts.ts
@@ -444,6 +444,10 @@ export class Scripts {
         return new Error(
           `Lock mismatch for job ${jobId}. Cmd ${command} from ${state}`,
         );
+      case ErrorCode.ParentJobCannotBeReplaced:
+        return new Error(
+          `The parent job of job ${jobId} cannot be replaced. ${command}`,
+        );
       default:
         return new Error(`Unknown code ${code} error for ${jobId}. ${command}`);
     }

diff --git a/src/commands/addDelayedJob-6.lua b/src/commands/addDelayedJob-6.lua
@@ -57,6 +57,7 @@ local parentData
 -- Includes
 --- @include "includes/addDelayMarkerIfNeeded"
 --- @include "includes/getOrSetMaxEvents"
+--- @include "includes/handleDuplicatedJob"
 --- @include "includes/isQueuePaused"
 --- @include "includes/storeJob"
 --- @include "includes/updateExistingJobsParent"
@@ -77,17 +78,12 @@ if args[2] == "" then
     jobId = jobCounter
     jobIdKey = args[1] .. jobId
 else
-    -- Refactor to: handleDuplicateJob.lua
     jobId = args[2]
     jobIdKey = args[1] .. jobId
     if rcall("EXISTS", jobIdKey) == 1 then
-        updateExistingJobsParent(parentKey, parent, parentData,
-                                 parentDependenciesKey, completedKey, jobIdKey,
-                                 jobId, timestamp)
-        rcall("XADD", eventsKey, "MAXLEN", "~", maxEvents, "*", "event",
-              "duplicated", "jobId", jobId)
-
-        return jobId .. "" -- convert to string
+        return handleDuplicatedJob(jobIdKey, jobId, parentKey, parent,
+            parentData, parentDependenciesKey, completedKey, eventsKey,
+            maxEvents, timestamp)
     end
 end
 

diff --git a/src/commands/addParentJob-4.lua b/src/commands/addParentJob-4.lua
@@ -49,9 +49,10 @@ local parent = args[8]
 local parentData
 
 -- Includes
+--- @include "includes/getOrSetMaxEvents"
+--- @include "includes/handleDuplicatedJob"
 --- @include "includes/storeJob"
 --- @include "includes/updateExistingJobsParent"
---- @include "includes/getOrSetMaxEvents"
 
 if parentKey ~= nil then
     if rcall("EXISTS", parentKey) ~= 1 then return -5 end
@@ -72,13 +73,9 @@ else
     jobId = args[2]
     jobIdKey = args[1] .. jobId
     if rcall("EXISTS", jobIdKey) == 1 then
-        updateExistingJobsParent(parentKey, parent, parentData,
-                                 parentDependenciesKey, completedKey, jobIdKey,
-                                 jobId, timestamp)
-        rcall("XADD", eventsKey, "MAXLEN", "~", maxEvents, "*", "event",
-              "duplicated", "jobId", jobId)
-
-        return jobId .. "" -- convert to string
+        return handleDuplicatedJob(jobIdKey, jobId, parentKey, parent,
+            parentData, parentDependenciesKey, completedKey, eventsKey,
+            maxEvents, timestamp)
     end
 end
 

diff --git a/src/commands/addPrioritizedJob-7.lua b/src/commands/addPrioritizedJob-7.lua
@@ -54,11 +54,12 @@ local parent = args[8]
 local parentData
 
 -- Includes
+--- @include "includes/addJobWithPriority"
 --- @include "includes/storeJob"
+--- @include "includes/getOrSetMaxEvents"
+--- @include "includes/handleDuplicatedJob"
 --- @include "includes/isQueuePaused"
---- @include "includes/addJobWithPriority"
 --- @include "includes/updateExistingJobsParent"
---- @include "includes/getOrSetMaxEvents"
 
 if parentKey ~= nil then
     if rcall("EXISTS", parentKey) ~= 1 then return -5 end
@@ -79,14 +80,9 @@ else
     jobId = args[2]
     jobIdKey = args[1] .. jobId
     if rcall("EXISTS", jobIdKey) == 1 then
-        updateExistingJobsParent(parentKey, parent, parentData,
-                                 parentDependenciesKey, completedKey, jobIdKey,
-                                 jobId, timestamp)
-
-        rcall("XADD", eventsKey, "MAXLEN", "~", maxEvents, "*", "event",
-              "duplicated", "jobId", jobId)
-
-        return jobId .. "" -- convert to string
+        return handleDuplicatedJob(jobIdKey, jobId, parentKey, parent,
+            parentData, parentDependenciesKey, completedKey, eventsKey,
+            maxEvents, timestamp)
     end
 end
 

diff --git a/src/commands/addStandardJob-7.lua b/src/commands/addStandardJob-7.lua
@@ -61,6 +61,7 @@ local parentData
 --- @include "includes/addJobInTargetList"
 --- @include "includes/getOrSetMaxEvents"
 --- @include "includes/getTargetQueueList"
+--- @include "includes/handleDuplicatedJob"
 --- @include "includes/storeJob"
 --- @include "includes/updateExistingJobsParent"
 
@@ -84,14 +85,9 @@ else
     jobId = args[2]
     jobIdKey = args[1] .. jobId
     if rcall("EXISTS", jobIdKey) == 1 then
-        updateExistingJobsParent(parentKey, parent, parentData,
-                                 parentDependenciesKey, KEYS[5], jobIdKey,
-                                 jobId, timestamp)
-
-        rcall("XADD", eventsKey, "MAXLEN", "~", maxEvents, "*", "event",
-              "duplicated", "jobId", jobId)
-
-        return jobId .. "" -- convert to string
+        return handleDuplicatedJob(jobIdKey, jobId, parentKey, parent,
+            parentData, parentDependenciesKey, KEYS[5], eventsKey,
+            maxEvents, timestamp)
     end
 end
 

diff --git a/src/commands/includes/handleDuplicatedJob.lua b/src/commands/includes/handleDuplicatedJob.lua
@@ -0,0 +1,26 @@
+--[[
+  Function to handle the case when job is duplicated.
+]]
+
+-- Includes
+--- @include "updateExistingJobsParent"
+
+local function handleDuplicatedJob(jobKey, jobId, currentParentKey, currentParent,
+  parentData, parentDependenciesKey, completedKey, eventsKey, maxEvents, timestamp)
+  local existedParentKey = rcall("HGET", jobKey, "parentKey")
+
+  if not existedParentKey then
+    updateExistingJobsParent(currentParentKey, currentParent, parentData,
+      parentDependenciesKey, completedKey, jobKey,
+      jobId, timestamp)
+  else
+    if currentParentKey ~= nil and currentParentKey ~= existedParentKey
+      and (rcall("EXISTS", existedParentKey) == 1) then
+      return -7
+    end
+  end
+  rcall("XADD", eventsKey, "MAXLEN", "~", maxEvents, "*", "event",
+    "duplicated", "jobId", jobId)
+
+  return jobId .. "" -- convert to string
+end
diff --git a/src/enums/error-code.ts b/src/enums/error-code.ts
@@ -5,4 +5,5 @@ export enum ErrorCode {
   JobPendingDependencies = -4,
   ParentJobNotExist = -5,
   JobLockMismatch = -6,
+  ParentJobCannotBeReplaced = -7,
 }
diff --git a/tests/test_flow.ts b/tests/test_flow.ts
@@ -1318,6 +1318,54 @@ describe('flows', () => {
       await worker.close();
       await flow.close();
     });
+
+    describe('when job already have a parent', async () => {
+      it('throws an error', async () => {
+        const flow = new FlowProducer({ connection, prefix });
+        await flow.add({
+          queueName,
+          name: 'tue',
+          opts: {
+            jobId: 'tue',
+          },
+          children: [
+            {
+              name: 'mon',
+              queueName,
+              opts: {
+                jobId: 'mon',
+              },
+            },
+          ],
+        });
+
+        await queue.add(
+          'wed',
+          {},
+          {
+            jobId: 'wed',
+          },
+        );
+
+        await expect(
+          queue.add(
+            'mon',
+            {},
+            {
+              jobId: 'mon',
+              parent: {
+                id: 'wed',
+                queue: `${prefix}:${queueName}`,
+              },
+            },
+          ),
+        ).to.be.rejectedWith(
+          `The parent job of job ${prefix}:${queueName}:wed cannot be replaced. addJob`,
+        );
+
+        await flow.close();
+      });
+    });
   });
 
   describe('when custom prefix is set in flow producer', async () => {