-
Notifications
You must be signed in to change notification settings - Fork 223
[dev] [tofikwest] tofik/update-risk-assesstment-part #1961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR SummaryModernizes vendor risk assessment end-to-end with versioned, centralized data and improved UX.
Written by Cursor Bugbot for commit a66d3ee. This will update automatically on new commits. Configure here. |
Graphite Automations"Auto-assign PRs to Author" took an action on this PR • (01/02/26)1 reviewer was added to this PR based on Mariano Fuentes's automation. |
apps/api/src/trigger/vendor/vendor-risk-assessment-monthly-schedule.ts
Outdated
Show resolved
Hide resolved
apps/api/src/trigger/vendor/backfill-vendor-risk-assessment-tasks.ts
Outdated
Show resolved
Hide resolved
…hance task handling
| }); | ||
| return await run(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Advisory lock fallback causes double execution on error
The withAdvisoryLock function has an overly broad outer catch block that catches errors from all three operations: lock acquisition, run() execution, and unlock. The intent was to only fall back to running without a lock when lock acquisition fails. However, if run() throws an error, the finally block executes (releasing the lock), then the error bubbles up to the outer catch, which logs a warning and calls run() again. This causes the database operations inside run() (version incrementing and GlobalVendors updates) to execute twice, potentially causing data inconsistencies, double version increments, or compounding errors.
| vendorName: v.vendorName, | ||
| vendorWebsite: v.vendorWebsite ?? null, | ||
| // Keep website canonical so downstream (Trigger task) uses the same GlobalVendors key. | ||
| vendorWebsite: normalizeWebsite(v.vendorWebsite ?? null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Website normalization mismatch causes silent research skip
There's a mismatch between extractDomain used for filtering (which adds a protocol if missing, accepting "example.com") and normalizeWebsite used for batch creation (which rejects URLs without protocols, returning null). When a vendor has a protocol-less website like "example.com", it passes filtering but gets vendorWebsite: null in the batch payload. The task then checks the DB for a valid website and continues, but at line 484 in vendor-risk-assessment-task.ts, payload.vendorWebsite being null causes research to be silently skipped even when needsResearch is true. This results in tasks completing without performing the intended research.
Additional Locations (1)
|
🎉 This PR is included in version 1.72.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This is an automated pull request to merge tofik/update-risk-assesstment-part into dev.
It was created by the [Auto Pull Request] action.