Skip to content
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.

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

Omit duplicate wf error in first invocation & not found in cleanup #16

Merged
merged 10 commits into from
Nov 20, 2024

Conversation

CahidArda
Copy link
Collaborator

@CahidArda CahidArda commented Nov 6, 2024

There are cases where network errors cause workflow runs to fail.

This PR handles three of those cases:

  1. when the workflow is first created, it is possible for request to fail even though it is delivered to QStash. So QStash creates the workflow but the SDK retries. Solution is to omit this error.
  2. when the SDK attempts to delete the workflow run after it completes, the request may fail even though it was delivered to QStash. Solution is to omit this error if it occurs.
  3. The workflow endpoint can get an error after scheduling the next step. In this case, the scheduled request may complete the workflow and finish it while the request (which failed) retries after the workflow is finished. We omit the workflow-canceled error to fix this issue.

In both cases, the debugger will log warnings.

Copy link

linear bot commented Nov 6, 2024

@CahidArda
Copy link
Collaborator Author

this won't be merged right away. We will get a canary release from this branch to verify the solution. If all looks good, we will merge to main.

@CahidArda
Copy link
Collaborator Author

we should update client.trigger in this PR too #18

@CahidArda CahidArda force-pushed the DX-1407-omit-trigger-and-cleanup-errors branch from b5c0212 to 51b71eb Compare November 20, 2024 09:49
src/workflow-requests.ts Outdated Show resolved Hide resolved
src/workflow-requests.ts Outdated Show resolved Hide resolved
@CahidArda
Copy link
Collaborator Author

can't pass the tests because they are too flaky. we had passed the tests in 51b71eb, and after that change we only bumped qstash and updated how we check omit error cases.

The unit tests pass. I ran the integration test which fails locally and it works. We can go ahead without the CI.

@CahidArda CahidArda merged commit e37c9b0 into main Nov 20, 2024
16 of 17 checks passed
@CahidArda CahidArda deleted the DX-1407-omit-trigger-and-cleanup-errors branch November 20, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants