fix(subscriber): await request status updates#3223
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMediaSubscriber now logs failures during beforeUpdate and afterUpdate request-status updates, and afterUpdate uses optional chaining when comparing against prior persisted media status fields. ChangesMediaSubscriber Error Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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.
Pull request overview
Updates the TypeORM MediaSubscriber to prevent unhandled promise rejections during request-status synchronization when Media entities update, avoiding process termination on Node versions where unhandled rejections are fatal.
Changes:
- Await
updateChildRequestStatusinbeforeUpdateand wrap intry/catchwith logging. - Await
updateRelatedMediaRequestinafterUpdateand wrap intry/catchwith logging. - Add
@server/loggerusage to record failures instead of letting rejections escape.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/subscriber/MediaSubscriber.ts`:
- Around line 188-222: In MediaSubscriber.afterUpdate, the standard and 4K
update paths are incorrectly wrapped in one shared try/catch, so a failure in
one branch prevents the other branch from running. Split the two
updateRelatedMediaRequest calls into separate guarded blocks (or separate
try/catch handling) so the non-4K and 4K updates fail independently, while
keeping the existing logger.error context for each branch.
- Around line 130-152: Split the standard and 4K child-status updates in
MediaSubscriber.beforeUpdate into separate guarded branches so a failure in one
does not prevent the other from running, and add an early check for
event.databaseEntity before accessing its fields. Make sure the logic around
updateChildRequestStatus, event.entity.status/status4k, and
event.databaseEntity.id safely handles cases where databaseEntity is undefined
on some TypeORM update paths, and keep the existing error logging scoped to each
branch.
🪄 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: 82634918-443e-441a-8349-def189f45e57
📒 Files selected for processing (1)
server/subscriber/MediaSubscriber.ts
af927f4 to
14fea58
Compare
updateChildRequestStatus and updateRelatedMediaRequest were called from beforeUpdate/afterUpdate without being awaited or wrapped in error handling. Since Seerr registers no unhandledRejection handler, any rejection from either call falls through to Node's default behaviour, which terminates the process on Node 15+. Await both calls and wrap them in try/catch so a failure in the request-sync side effect is logged instead of crashing the server, and can't affect the primary media save.
14fea58 to
6df7026
Compare
Description
Similar to #2760, while working on a fix for availability sync job, I noticed that
MediaSubscriber'sbeforeUpdateandafterUpdatecallupdateChildRequestStatusandupdateRelatedMediaRequestwithout awaiting them, and neither method has its own error handling. Since Seerr has no globalunhandledRejectionhandler, any rejection from either call is silently lost on Node 15+, and an unhandled rejection with no handler terminates the process bydefault. Any transient failure in this request-sync path (a DB error, a constraint violation, anything) can crash the running server, not just fail one status update.
This surfaced while working on unrelated availability sync fixes: a test that inserted a real MediaRequest and triggered a status change would intermittently report an unhandled rejection after the test had already completed. The screenshot below shows that failure; the underlying risk is the same in production, just without a test runner around to catch it.
Both calls are now awaited and wrapped in try/catch, so a failure in the request-sync side effect is logged instead of taking down the process, and can't interfere with the primary media save.
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Summary by CodeRabbit