Skip to content

fix(flow-producer): reject deduplication and debounce options (#2780) - #4121

Open
mohanrajvenkatesan23-04 wants to merge 5 commits into
taskforcesh:masterfrom
mohanrajvenkatesan23-04:fix/issue-2780-flow-debounce-error
Open

fix(flow-producer): reject deduplication and debounce options (#2780)#4121
mohanrajvenkatesan23-04 wants to merge 5 commits into
taskforcesh:masterfrom
mohanrajvenkatesan23-04:fix/issue-2780-flow-debounce-error

Conversation

@mohanrajvenkatesan23-04

Copy link
Copy Markdown
Contributor

Why

FlowProducer.add() and addBulk() silently dropped opts.deduplication / opts.debounce. A dedup hit returns the existing job's id rather than the freshly generated one, which orphans the flow's children — there is no safe way to reconcile the two ids inside the multi-exec transaction.

Per maintainer @roggervalf on #2780:

"actually this won't work for a root parent either using flow producer because the parent ids should be known in each addition before running the transaction. Dedup logic can retrieve a different id than the one being generated from the producer."
"this functionality is not supported... We should throw an error in this case."

Closes #2780.

What

  • src/classes/flow-producer.ts: file-private assertNoDeduplication(flow) helper called at the top of add() and addBulk(). Throws a clear error naming the offending queue/job.
  • tests/flow.test.ts: three regression tests covering deduplication on add(), debounce on add(), and deduplication mixed into addBulk().

I left the FlowJob type alone for now — removing debounce/deduplication from the root type would be a TypeScript breaking change that warrants a separate decision. The runtime check covers JS users and any TS escape hatch.

Test plan

  • tests/flow.test.ts adds three new cases under "when deduplication or debounce is provided to a flow"
  • CI runs the existing flow test suite against redis@7-alpine, valkey@8, and dragonflydb@latest

FlowProducer cannot honour deduplication or debounce: parent ids must be
known before multi.exec() commits, but a dedup hit returns the existing
job's id rather than the freshly generated one — orphaning the flow's
children. The FlowJob type currently still permits these opts at the
root, and they were silently dropped at runtime.

Throw a clear error from add() and addBulk() instead. Per maintainer
guidance on taskforcesh#2780: "we should throw an error in this case".

- Add assertNoDeduplication() helper in flow-producer.ts
- Call it at the top of add() and addBulk()
- Add three regression tests in tests/flow.test.ts covering both opts
  on add() and the bulk path

Closes taskforcesh#2780
@manast
manast requested a review from Copilot July 15, 2026 18:07
@manast
manast requested review from roggervalf and removed request for Copilot July 15, 2026 18:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR makes FlowProducer fail fast when deduplication or debounce options are provided in flow jobs, because these options can cause the returned job id to differ from the id generated inside the flow transaction—breaking parent/child linking and orphaning children.

Changes:

  • Add a runtime guard (assertNoDeduplication) in FlowProducer.add() and addBulk() that throws a descriptive error when opts.deduplication/opts.debounce is present.
  • Add regression tests ensuring add() rejects both deduplication and debounce, and addBulk() rejects flows containing deduplication.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/classes/flow-producer.ts Adds a guard to reject deduplication/debounce options in flows with a clear error message.
tests/flow.test.ts Adds tests covering rejection behavior for flow creation with deduplication/debounce.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/classes/flow-producer.ts
Comment thread tests/flow.test.ts
…low tree

- assertNoDeduplication now walks children recursively instead of only
  checking the root, so a deduplication or debounce option set on any
  nested node is rejected with the same error. FlowChildJob omits these
  options at the type level, but JavaScript callers can still set them
  and addChildren would pass them straight through to Job.addJob.
- add a test covering nested child and grandchild nodes.
- add an addBulk test for debounce, matching the existing deduplication
  case so a regression in either option is caught.
@mohanrajvenkatesan23-04

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Pushed a follow-up commit addressing both points:

  • src/classes/flow-producer.tsassertNoDeduplication now walks the full tree instead of only checking the root. It accepts FlowJob | FlowChildJob and recurses into children, so a deduplication or debounce option on any nested node is rejected with the same error. FlowChildJob omits both options at the type level, but JavaScript callers (and TS escape hatches) can still set them, and addChildren() would otherwise pass them straight through to Job.addJob.
  • tests/flow.test.ts — added an addBulk() case for debounce mirroring the existing deduplication case, plus a new test covering nested child and grandchild nodes for both options.

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.

[Bug]: Debounced job ID not being returned on new flow job creation

3 participants