Skip to content

fix(workflows): enable BullMQ retries for workflow-run.process [ENG-1228]#8404

Closed
xernobyl wants to merge 1 commit into
epic/workflows-v1from
fix/ENG-1228_enable-workflow-run-retries
Closed

fix(workflows): enable BullMQ retries for workflow-run.process [ENG-1228]#8404
xernobyl wants to merge 1 commit into
epic/workflows-v1from
fix/ENG-1228_enable-workflow-run-retries

Conversation

@xernobyl

@xernobyl xernobyl commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Pairs with #8395 (ENG-1228), stacked on epic/workflows-v1. Now that send_email execution is idempotent per step (claim-before-send + @@unique([runId, stepId]), in #8395), this drops the attempts: 1 override on enqueueWorkflowRunJob so workflow-run.process inherits the shared retry policy — attempts: 3 + exponential backoff from the queue's defaultJobOptions, same as every other job.

Why: with attempts: 1, maxAttempts was always 1, so the executor's retry/resume machinery and its pool-exhaustion handling never actually fired (every attempt was "final") — a transient failure (SMTP blip, DB pool exhaustion, stalled redelivery) killed the run on the first try and could strand it. With retries enabled and the per-step idempotency from #8395, a transient failure now retries and resumes without re-sending. The deterministic jobId (the run id) keeps a re-enqueue idempotent (e.g. the ENG-1608 reconciler re-dispatching an orphan).

⚠️ Merge ordering: must merge with or after #8395 — retries must never be enabled without the per-step send idempotency, or a retry could double-send. (#8395 is approved.)

How should this be tested?

pnpm --filter @formbricks/jobs run test   # 59 pass

One-line behaviour change (drop the attempts: 1 override). The workflow-run enqueue test now asserts no per-job attempts override, so the job inherits the shared attempts: 3 + backoff policy — which itself is covered by the existing "creates the shared queue with the expected defaults" test. End-to-end (worker running): a send_email step that throws a transient error now retries up to 3× and resumes from the claimed-step log, instead of failing permanently on attempt 1.

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits (the retry-policy inheritance + the feat(workflows): runner executes send_email action [ENG-1228] #8395 pairing)
  • Ran the jobs test + typecheck + lint
  • Checked for warnings, there are none
  • Removed all console.logs (none)
  • Branched off the current base (epic/workflows-v1)
  • My changes don't cause any responsiveness issues (backend job)
  • First PR at Formbricks? — no

Appreciated

  • If a UI change was made: screenshots — N/A
  • Updated the Formbricks Docs — N/A

…228]

Now that send_email execution is idempotent per step (claim-before-send +
@@unique([runId, stepId]), #8395), drop the attempts:1 override on
enqueueWorkflowRunJob so workflow-run.process inherits the shared retry policy
(attempts + exponential backoff from the queue defaultJobOptions). A transient
failure (SMTP blip, DB pool exhaustion, stalled redelivery) now retries and
resumes instead of dying on the first attempt; the deterministic jobId keeps a
re-enqueue idempotent.

Must merge with/after #8395 — retries must never be enabled without the
per-step send idempotency.
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 79547c2e-218c-466a-ab52-ddec235aa70a

📥 Commits

Reviewing files that changed from the base of the PR and between eb064c4 and 6677dbd.

📒 Files selected for processing (2)
  • packages/jobs/src/queue.test.ts
  • packages/jobs/src/queue.ts

Walkthrough

This change removes an explicit attempts: 1 override previously set on BullMQ workflow-run jobs in enqueueWorkflowRun, so retry/backoff behavior is now inherited from the queue's defaultJobOptions. The optional deterministic jobId override behavior is preserved. The corresponding test was updated to assert that only jobId is passed when enqueuing, removing the prior assertion on attempts: 1.

Changes

File Change Summary
packages/jobs/src/queue.ts Removed explicit attempts: 1 override in enqueueWorkflowRun; retry/backoff now inherited from defaultJobOptions
packages/jobs/src/queue.test.ts Updated test to expect only jobId in enqueue options, removed attempts: 1 assertion

Sequence Diagram(s)

Not applicable — this change is a configuration adjustment without new interaction flows.

Related Issues: None provided.

Related PRs: None provided.

Suggested labels: jobs, bug-fix, testing

Suggested reviewers: None specified.

Poem
A rabbit hops through queues at night,
Trimming attempts that once held tight,
Now defaults reign, a simpler flow,
JobId still marks where jobs should go,
Tests confirm this tidy sight. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: enabling BullMQ retries for the workflow-run job.
Description check ✅ Passed The PR description includes the change summary, issue context, testing steps, and checklist sections required by the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

@xernobyl

xernobyl commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Folding this into #8403 — the retry-enablement and the orphan reconciler touch the same packages/jobs/queue surface and only make sense together, so they're now a single PR. The exact commit from here (6677dbd, drop the attempts: 1 override) is cherry-picked onto #8403, which now carries both changes. Closing in favor of #8403.

@xernobyl xernobyl closed this Jul 2, 2026
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.

1 participant