fix(worker): add missing throttled property in step output#9901
fix(worker): add missing throttled property in step output#9901lqmanh wants to merge 3 commits intonovuhq:nextfrom
throttled property in step output#9901Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
WalkthroughEnd-to-end throttle tests were changed to iterate over all completed and skipped jobs, validating each job's 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/src/app/events/e2e/throttle-events.e2e.ts`:
- Around line 100-106: The assertions assume DB returns completedThrottleJobs in
a deterministic order; instead make the check order-independent by collecting
execution counts from completedThrottleJobs (accessing
job.stepOutput?.executionCount) and asserting the resulting set/array contains
the expected values (e.g., {1,2,3} or includes each expected count) rather than
comparing to index + 1; update the loop/expectations around
completedThrottleJobs in throttle-events.e2e.ts to perform this set membership
check.
♻️ Duplicate comments (1)
apps/api/src/app/events/e2e/throttle-events.e2e.ts (1)
181-195: Same ordering issue as above forexecutionCount.These assertions also assume deterministic ordering from the repository. Apply the same order-independent pattern as the earlier block.
| completedThrottleJobs.forEach((job, index) => { | ||
| expect(job.stepOutput).to.be.ok; | ||
| expect(job.stepOutput?.throttled).to.equal(false); | ||
| expect(job.stepOutput?.threshold).to.equal(3); | ||
| expect(job.stepOutput?.executionCount).to.equal(index + 1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Avoid relying on DB order for executionCount assertions.
find() ordering isn’t guaranteed, so index + 1 can be flaky. Prefer order-independent checks.
🔧 Suggested change
- completedThrottleJobs.forEach((job, index) => {
+ completedThrottleJobs.forEach((job) => {
expect(job.stepOutput).to.be.ok;
expect(job.stepOutput?.throttled).to.equal(false);
expect(job.stepOutput?.threshold).to.equal(3);
- expect(job.stepOutput?.executionCount).to.equal(index + 1);
});
+
+ const executionCounts = completedThrottleJobs
+ .map((job) => job.stepOutput?.executionCount)
+ .filter((count): count is number => typeof count === 'number')
+ .sort((a, b) => a - b);
+ expect(executionCounts).to.deep.equal([1, 2]);🤖 Prompt for AI Agents
In `@apps/api/src/app/events/e2e/throttle-events.e2e.ts` around lines 100 - 106,
The assertions assume DB returns completedThrottleJobs in a deterministic order;
instead make the check order-independent by collecting execution counts from
completedThrottleJobs (accessing job.stepOutput?.executionCount) and asserting
the resulting set/array contains the expected values (e.g., {1,2,3} or includes
each expected count) rather than comparing to index + 1; update the
loop/expectations around completedThrottleJobs in throttle-events.e2e.ts to
perform this set membership check.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/worker/src/app/workflow/usecases/add-job/add-job.usecase.ts (1)
298-310: Good fix — correctly setsthrottled: falseon the non-skip path.This resolves the reported validation error by ensuring
stepOutputis populated regardless of the throttle outcome.One minor note:
handleThrottlehas several early-return paths (lines 843, 850, 856, 863, 869) that return{ shouldSkip: false }withoutexecutionCountorthreshold. In those cases,undefinedvalues will be written to the DB. This is unlikely to cause a runtime failure but may confuse downstream consumers expecting numeric values.Optional: default to safe values
} else { await this.jobRepository.updateOne( { _id: job._id, _environmentId: command.environmentId }, { $set: { stepOutput: { throttled: false, - executionCount: throttleResult.executionCount, - threshold: throttleResult.threshold, + executionCount: throttleResult.executionCount ?? 0, + threshold: throttleResult.threshold ?? 0, }, }, } ); }
What changed? Why was the change needed?
When using
step.throttlevia code-based workflows, this error is raised:Expand for optional sections
Related enterprise PR
Special notes for your reviewer