medusajs / medusa

The world's most flexible commerce platform.
https://medusajs.com
MIT License
26.36k stars 2.69k forks source link

[Bug]: The error should be re-thrown when maxRetries is set to a value greater than 0 in a workflow step #10201

Closed noubase closed 2 weeks ago

noubase commented 2 weeks ago

Package.json file

{
  "name": "medusa-starter-default",
  "version": "0.0.1",
  "description": "A starter for Medusa projects.",
  "author": "Medusa (https://medusajs.com)",
  "license": "MIT",
  "keywords": [
    "sqlite",
    "postgres",
    "typescript",
    "ecommerce",
    "headless",
    "medusa"
  ],
  "scripts": {
    "build": "medusa build",
    "seed": "medusa exec ./src/scripts/seed.ts",
    "start": "medusa start",
    "dev": "medusa develop",
    "test:integration:http": "TEST_TYPE=integration:http NODE_OPTIONS=--experimental-vm-modules jest --silent=false --runInBand --forceExit",
    "test:integration:modules": "TEST_TYPE=integration:modules NODE_OPTIONS=--experimental-vm-modules jest --silent --runInBand --forceExit",
    "test:unit": "TEST_TYPE=unit NODE_OPTIONS=--experimental-vm-modules jest --silent --runInBand --forceExit"
  },
  "dependencies": {
    "@dnd-kit/modifiers": "^7.0.0",
    "@medusajs/admin-sdk": "2.0.4",
    "@medusajs/cli": "2.0.4",
    "@medusajs/framework": "2.0.4",
    "@medusajs/medusa": "2.0.4",
    "@mikro-orm/core": "5.9.7",
    "@mikro-orm/knex": "5.9.7",
    "@mikro-orm/migrations": "5.9.7",
    "@mikro-orm/postgresql": "5.9.7",
    "awilix": "^8.0.1",
    "axios-retry": "^4.5.0",
    "contentful-management": "11.35.1",
    "contentful-rich-text-html-parser": "^1.5.13",
    "lodash": "^4.17.21",
    "pg": "^8.13.1",
    "react-icons": "^5.3.0",
    "xmlrpc": "^1.3.2",
    "@directus/sdk": "^18.0.0"
  },
  "devDependencies": {
    "@mikro-orm/cli": "5.9.7",
    "@swc/core": "1.5.7",
    "@swc/jest": "^0.2.36",
    "@types/jest": "^29.5.13",
    "@types/lodash": "^4.17.12",
    "@types/node": "^20.0.0",
    "@types/react": "^18.3.2",
    "@types/react-dom": "^18.2.25",
    "jest": "^29.7.0",
    "medusa-test-utils": "rc",
    "prop-types": "^15.8.1",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "ts-node": "^10.9.2",
    "typescript": "^5.6.2",
    "vite": "^5.2.11",
    "@medusajs/test-utils": "latest"
  },
  "engines": {
    "node": ">=20"
  },
  "packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
}

Node.js version

v2

Database and its version

16.2

Operating system name and version

Mac OS X

Browser name

No response

What happended?

route.ts

export async function GET(
    req: MedusaRequest,
    res: MedusaResponse
): Promise<void> {

  const result = await withErrorWorkflow(req.scope).run({input: 'some input', throwOnError: true})
  console.log(`Workflow result [${JSON.stringify(result.result)}], errors: [${JSON.stringify(result.errors)}]`);
  res.json(result)
}
import {createStep, createWorkflow, StepResponse, WorkflowResponse} from "@medusajs/workflows-sdk";

const errorStep = createStep<string, string, string>(
    {name: 'error-step', maxRetries: 2, retryInterval: 1},
    async (input, context) => {
      if (true)
        throw new Error(`ERROR`)
      return new StepResponse(input, input)
    },
    async (input, context) => {
      console.warn(`Compensating in error step [${input}]`)
    }
)

const firstStep = createStep<string, string, string>(
    {name: 'first-step'},
    async (input, context) => {
      return new StepResponse(input, input)
    },
    async (input, context) => {
      console.warn(`Compensating in first step [${input}]`)
    }
)

const withErrorWorkflow = createWorkflow<string, string, any[]>(
    {name: "with-error-workflow"},

    function (input) {
      firstStep(input)
      const result = errorStep(input)
      return new WorkflowResponse(result)
    }
)

export default withErrorWorkflow

Console output

Workflow result [undefined], errors: [[]]
Compensating in error step [undefined]
Compensating in first step [some input]
error:   with-error-workflow:error-step:invoke - ERROR
Error: ERROR
...

The workflow engine should re-throw an error that occurs in a step when maxRetries is set to a value greater than 0.

Expected behavior

Error should be re-thrown when using maxRetries in a workflow step

Actual behavior

The error is simply logged to the error output.

Link to reproduction repo

on need

noubase commented 2 weeks ago

Additionally, it seems that setting maxRetries > 0 makes a step asynchronous, as the workflow returns a result immediately.

adrien2p commented 2 weeks ago

Hey @noubase ,

your workflow becomes async because you are setting the retryInterval which is a timer and therefore means that it is not synchronous anymore. also, about throwing on each error, by design an execution needs to be finished before we throw the errors, just throwing them when they happen would prevent the workflow from continuing its execution since throwing stop immediately unless being catched. Which is what we do and then report them back to you.

I hope it is clear, let me know if it is not

kasperkristensen commented 2 weeks ago

Hi @noubase, I am closing the issue as it is working as expected, as Adrien describes. If you are still facing other issues then don't hesitate to open a new issue.

noubase commented 2 weeks ago

your workflow becomes async because you are setting the retryInterval which is a timer and therefore means that it is not synchronous anymore.

This is very confusing and counterintuitive because there’s already an async?: boolean field in the step configuration. So currently, there’s no way to configure a step to retry synchronously three times with a 100 ms interval, is that correct?

carlos-r-l-rodrigues commented 2 weeks ago

your workflow becomes async because you are setting the retryInterval which is a timer and therefore means that it is not synchronous anymore.

This is very confusing and counterintuitive because there’s already an async?: boolean field in the step configuration. So currently, there’s no way to configure a step to retry synchronously three times with a 100 ms interval, is that correct?

if you want to do that synchronously the second argument of the step you have the attempt number in the context. You can just check if it is a retry and "sleep" there.