fix(agenda): clear failReason and failedAt on successful job completion#39758
fix(agenda): clear failReason and failedAt on successful job completion#39758Jashk120 wants to merge 2 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughClears stale failure fields on successful job completion and refactors how job updates are sent to MongoDB: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
Jashk120
force-pushed
the
fix/agenda-clear-failreason-on-success-v2
branch
from
1eb1337 to
b4e02ea
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/agenda/src/Agenda.ts (1)
495-496: Remove the new implementation comment.The code is already self-explanatory here. As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agenda/src/Agenda.ts` around lines 495 - 496, Remove the redundant implementation comment that precedes the debug call; delete the line "// Update the job and process the resulting data" so only the debug statement debug('job already has _id, calling findOneAndUpdate() using _id as query'); remains in the method where job update is handled (around the code using findOneAndUpdate in Agenda.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/agenda/src/Agenda.ts`:
- Around line 483-493: The loop over Object.entries(props) is unconditionally
turning any undefined prop into an $unset, which will mistakenly remove fields
like lastRunAt set temporarily by repeatEvery(..., { skipImmediate: true });
change the logic in the block that builds $set/$unset (the code using props,
$set, $unset, update) to only unset the specific failure-related fields
(failReason and failedAt) or use an explicit delete-list instead of unsetting
every undefined prop; locate the builder that iterates props (the for (const
[key, value] of Object.entries(props)) block) and replace the unconditional
undefined => $unset behavior with either (a) only add key to $unset if key is in
['failReason','failedAt'] and value === undefined, or (b) maintain an explicit
array of keysToRemove and only unset those keys when present.
---
Nitpick comments:
In `@packages/agenda/src/Agenda.ts`:
- Around line 495-496: Remove the redundant implementation comment that precedes
the debug call; delete the line "// Update the job and process the resulting
data" so only the debug statement debug('job already has _id, calling
findOneAndUpdate() using _id as query'); remains in the method where job update
is handled (around the code using findOneAndUpdate in Agenda.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903604b2-795c-48e5-bd61-23e017a7b5d6
📒 Files selected for processing (2)
packages/agenda/src/Agenda.tspackages/agenda/src/Job.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/agenda/src/Job.tspackages/agenda/src/Agenda.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/agenda/src/Job.tspackages/agenda/src/Agenda.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/agenda/src/Job.tspackages/agenda/src/Agenda.ts
🔇 Additional comments (1)
packages/agenda/src/Job.ts (1)
209-211: Success-path cleanup looks right.This clears the stale failure markers before persisting, so a recovered job no longer carries
failReason/failedAtinto later runs.
|
@cubic-dev-ai @coderabbitai review |
@Jashk120 I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
1 participant
Proposed changes
failReasonandfailedAtfields are never cleared when a job runs successfullyafter a previous failure. This causes stale failure data to persist indefinitely
in
rocketchat_cron.Two changes:
Job.ts— clearfailReasonandfailedAtin the success path ofjobCallbackAgenda.ts— filterundefinedvalues from$setinto$unsetin_updateJobso MongoDB actually removes the fields instead of ignoring them
Issue(s)
Closes #39755
Steps to test
failReasonandfailedAton a job document inrocketchat_cronSteps to test or reproduce
Before fix — job with lastFinishedAt set still showing stale failReason:
After fix — both fields cleared after successful run:
Summary by CodeRabbit