Skip to content

Conversation

@CahidArda
Copy link
Collaborator

We now return status: 400 in response to auth failures. The text of the message reads:

Failed to authenticate Workflow request. If this is unexpected, see the caveat https://upstash.com/docs/workflow/basics/caveats#avoid-non-deterministic-code-outside-context-run

The caveat is being updated with upstash/docs#336. See the preview

Also did the following in the CI:

  • added an endpoint which returns a json in context.call
  • added workflowStarts so that we can disable checking worklfow start in auth/fail endpoint

new Response(JSON.stringify({ workflowRunId }), {
onStepFinish: (workflowRunId: string, finishCondition: FinishCondition) => {
if (finishCondition === "auth-fail") {
console.error(AUTH_FAIL_MESSAGE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can log with debug?.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we have both? If we only have debug, then the user won't understand what's happening if they get this error uninentionally

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug.log is async. onStepFinish would have to be async. it's a breaking change. I think we should do it later.


await qstash.checkWorkflowStart(messageId)
try {
await qstash.checkWorkflowStart(messageId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the case shouldWorkflowStart is false, but the workflow starts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did check this case today, and realised that checkWorkflowStart doesn't work as I expect at all :D

I will remove it altogether in #32 after we merge this. I didn't want to cause conflicts.

@CahidArda CahidArda merged commit 55fb82e into main Nov 29, 2024
17 checks passed
@CahidArda CahidArda deleted the return-400-on-auth-fail branch November 29, 2024 14:37
@CahidArda CahidArda changed the title Return 400 open auth failure Return 400 on auth failure Dec 4, 2024
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.

4 participants