fix(execution): throw NonRetriableError when step.invoke target is cancelled - #1707
fix(execution): throw NonRetriableError when step.invoke target is cancelled#1707marsyg wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 56a7c82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Needs attention — 1 issue in 1 file
normalizeMemoizedOp applies to all memoized ops, not just invoke steps. A step.run whose user-returned data happens to contain { _inngest: { status: "Cancelled" } } would be incorrectly rewritten to an error. The likelihood is low but the blast radius is silent data corruption for any affected step. The fix should scope the normalization to invoke ops only (checking op.op === "InvokeFunction" or similar discriminant). Otherwise the logic and error shape are correct.
What this PR does
Adds a normalizeMemoizedOp function that intercepts memoized step state for step.invoke results. When the invoked function was cancelled (detected via _inngest.status === "Cancelled" in the event payload), it rewrites the op to have data: undefined and an error with name: "NonRetriableError", causing the step to reject with a StepError instead of resolving with the raw internal event payload.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<assessment>
`normalizeMemoizedOp` applies to **all** memoized ops, not just invoke steps. A `step.run` whose user-returned data happens to contain `{ _inngest: { status: "Cancelled" } }` would be incorrectly rewritten to an error. The likelihood is low but the blast radius is silent data corruption for any affected step. The fix should scope the normalization to invoke ops only (checking `op.op === "InvokeFunction"` or similar discriminant). Otherwise the logic and error shape are correct.
</assessment>
<file name="packages/inngest/src/components/execution/engine.ts">
<issue location="packages/inngest/src/components/execution/engine.ts:140">
`normalizeMemoizedOp` runs on every step type, but the `_inngest.status` check on the non-event path (lines 152-155) can match user-returned data from `step.run` if it coincidentally contains `{ _inngest: { status: "Cancelled" } }`. This would silently turn a successful step result into an error. Guard the transformation so it only applies to invoke-type ops (e.g. check the op type or limit the fallback status detection to only fire when `isFinishedEvent` is true).
</issue>
</file>
Tag @mendral-app with feedback or questions. View session
|
Hi @amh4r @scottnuma would you be able to review this change? |
Summary
When an invoked child function was cancelled in the Dev UI or via API,
step.invoke(...)previously resolved with the internalinngest/function.finishedevent payload (withdata._inngest.status === "Cancelled") instead of rejecting. This broke TypeScript return type assumptions and caused runtime crashes when trying to parse or use the return value.This PR normalizes memoized step states in
engine.ts(normalizeMemoizedOp) so that cancelled invocations setdata: undefinedanderror: { name: "NonRetriableError", message: "Invoked function was cancelled" }. This ensures:step.invoke(...)rejects with aStepError(NonRetriableError).try { await step.invoke(...) } catch (err).Checklist
Added a docs PR that references this PRN/A Bug fix aligning with existing NonRetriableError step behaviorRelated
Fixes #1694