fix(subscriber): await async calls in MediaSubscriber lifecycle hooks#2745
fix(subscriber): await async calls in MediaSubscriber lifecycle hooks#2745dougrathbone wants to merge 3 commits intoseerr-team:developfrom
Conversation
updateChildRequestStatus (called in beforeUpdate) and updateRelatedMediaRequest (called in afterUpdate) are both async but were invoked without await, making them fire-and-forget. In beforeUpdate, this means PENDING requests are not auto-approved before the media save completes. The afterUpdate hook then runs before those request status changes are persisted, so they are never seen as APPROVED and never promoted to COMPLETED. In afterUpdate, APPROVED requests may not be marked COMPLETED before the response is returned to the caller. Adds integration tests that save a media entity with a status transition and verify the expected final request status, covering both subscriber hooks.
📝 WalkthroughWalkthroughReplaces global repository access with the transaction-scoped EntityManager inside MediaSubscriber and makes subscriber lifecycle helpers awaited; adds integration tests validating MediaRequest status transitions for standard and 4K media status changes. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![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)
server/subscriber/MediaSubscriber.test.ts (1)
17-27: Make the fixtures configurable for 4K coverage.The implementation change also touches the 4K branches, but these helpers only build non-4K data (
statusonly,is4k: false). Making them parameterizable would let this suite cover Line 140 and Line 195 too.♻️ Suggested tweak
async function createMedia( status: MediaStatus, - mediaType = MediaType.MOVIE + mediaType = MediaType.MOVIE, + status4k = MediaStatus.UNKNOWN ): Promise<Media> { const mediaRepo = getRepository(Media); const media = new Media(); media.tmdbId = Math.floor(Math.random() * 900000) + 100000; media.mediaType = mediaType; media.status = status; + media.status4k = status4k; return mediaRepo.save(media); } async function createMovieRequest( requestedBy: User, media: Media, - status: MediaRequestStatus + status: MediaRequestStatus, + is4k = false ): Promise<MediaRequest> { const requestRepo = getRepository(MediaRequest); const req = new MediaRequest({ media, requestedBy, status, type: MediaType.MOVIE, - is4k: false, + is4k, }); return requestRepo.save(req); }Also applies to: 29-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscriber/MediaSubscriber.test.ts` around lines 17 - 27, The test helper createMedia currently only builds non‑4K Media (no way to set is4k), so make it accept an optional is4k boolean (default false) and pass it into the Media entity before saving; update any other fixture helpers referenced (the ones around lines 29–42) similarly to accept an is4k parameter and set media.is4k so tests can exercise the 4K branches (e.g., adjust createMedia and the other helper functions to accept is4k: boolean = false and assign to Media.is4k).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/subscriber/MediaSubscriber.ts`:
- Around line 133-140: The helper methods updateChildRequestStatus and
updateRelatedMediaRequest are using the global datasource repositories (via
getRepository) which causes writes to bypass the transaction on the incoming
event; change their signatures to accept the transaction EntityManager (pass
event.manager from the MediaSubscriber where you call updateChildRequestStatus)
and inside those methods use event.manager.getRepository(...) (or the passed
manager.getRepository) for all reads/writes so the MediaRequest/SeasonRequest
updates occur within the same transaction scope as the Media save.
---
Nitpick comments:
In `@server/subscriber/MediaSubscriber.test.ts`:
- Around line 17-27: The test helper createMedia currently only builds non‑4K
Media (no way to set is4k), so make it accept an optional is4k boolean (default
false) and pass it into the Media entity before saving; update any other fixture
helpers referenced (the ones around lines 29–42) similarly to accept an is4k
parameter and set media.is4k so tests can exercise the 4K branches (e.g., adjust
createMedia and the other helper functions to accept is4k: boolean = false and
assign to Media.is4k).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce70c368-c2ad-4e4f-bb1e-95dbe5f4c228
📒 Files selected for processing (2)
server/subscriber/MediaSubscriber.test.tsserver/subscriber/MediaSubscriber.ts
…and add 4K test coverage Pass the transaction-scoped event.manager into updateChildRequestStatus and updateRelatedMediaRequest instead of resolving repositories via the global getRepository(). This ensures that MediaRequest and SeasonRequest writes participate in the same unit of work as the parent Media save: if the outer save rolls back, the request status changes roll back with it. Also removes the now-unused getRepository import from @server/datasource. Test coverage is extended to include the 4K branches (status4k transitions) for both the beforeUpdate and afterUpdate paths.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/subscriber/MediaSubscriber.ts (1)
19-39: Consider renaming parametereventtomediafor clarity.The parameter
eventis actually aMediaentity, not anUpdateEvent. This naming can cause confusion when reading the code. Additionally, the individual saves in the loop could be batched for efficiency, though this is less critical now that operations are within the same transaction.♻️ Optional: Batch save and rename parameter
private async updateChildRequestStatus( manager: EntityManager, - event: Media, + media: Media, is4k: boolean ) { const requestRepository = manager.getRepository(MediaRequest); const requests = await requestRepository.find({ - where: { media: { id: event.id } }, + where: { media: { id: media.id } }, }); + const toSave: MediaRequest[] = []; for (const request of requests) { if ( request.is4k === is4k && request.status === MediaRequestStatus.PENDING ) { request.status = MediaRequestStatus.APPROVED; - await requestRepository.save(request); + toSave.push(request); } } + if (toSave.length > 0) { + await requestRepository.save(toSave); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscriber/MediaSubscriber.ts` around lines 19 - 39, Rename the parameter `event` to `media` in the updateChildRequestStatus method signature and all its usages to reflect that it is a Media entity (function: updateChildRequestStatus, parameter: event -> media); also improve efficiency by batching saves: collect the modified MediaRequest instances (from requestRepository.find where media.id === media.id and matching is4k/status checks) and call requestRepository.save once with the array instead of awaiting saves inside the loop so the updates stay within the same transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/subscriber/MediaSubscriber.ts`:
- Around line 19-39: Rename the parameter `event` to `media` in the
updateChildRequestStatus method signature and all its usages to reflect that it
is a Media entity (function: updateChildRequestStatus, parameter: event ->
media); also improve efficiency by batching saves: collect the modified
MediaRequest instances (from requestRepository.find where media.id === media.id
and matching is4k/status checks) and call requestRepository.save once with the
array instead of awaiting saves inside the loop so the updates stay within the
same transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3feb4c3e-5601-4d6f-ae19-aba58640f0a7
📒 Files selected for processing (2)
server/subscriber/MediaSubscriber.test.tsserver/subscriber/MediaSubscriber.ts
✅ Files skipped from review due to trivial changes (1)
- server/subscriber/MediaSubscriber.test.ts
- Rename event -> media parameter in updateChildRequestStatus for clarity
- Batch MediaRequest saves: collect changed requests then call
requestRepository.save(toSave) once instead of one save per loop iteration
- Add optional status4k parameter to createMedia test helper
- Add optional is4k parameter to createMovieRequest test helper
- Restore settings.main.localLogin in finally block in authenticatedAgent
- Explicitly set cookie: { secure: false } in test session config
|
Addressed in the latest commit:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/subscriber/MediaSubscriber.ts (1)
138-141: Consider adding error handling in upstream route handlers.Now that
beforeUpdateandafterUpdateproperly await their helper methods, any errors (e.g., bulk save failures) will propagate up to the callingmediaRepository.save(). ThePOST /:id/:statusroute inserver/routes/media.ts(around line 164) lacks a try-catch wrapper, so these errors will crash the handler. TheDELETEroute in the same file demonstrates the correct pattern with proper error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscriber/MediaSubscriber.ts` around lines 138 - 141, The POST /:id/:status route handler currently calls mediaRepository.save() without catching errors that can bubble up from MediaSubscriber.beforeUpdate/afterUpdate; wrap the handler body in a try-catch, call await mediaRepository.save(...) inside the try, and in catch log the error and return an appropriate HTTP error response (e.g., 500 with a JSON error message) similar to the existing DELETE route pattern so upstream failures (bulk save or subscriber errors) don't crash the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/subscriber/MediaSubscriber.ts`:
- Around line 138-141: The POST /:id/:status route handler currently calls
mediaRepository.save() without catching errors that can bubble up from
MediaSubscriber.beforeUpdate/afterUpdate; wrap the handler body in a try-catch,
call await mediaRepository.save(...) inside the try, and in catch log the error
and return an appropriate HTTP error response (e.g., 500 with a JSON error
message) similar to the existing DELETE route pattern so upstream failures (bulk
save or subscriber errors) don't crash the process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44fa986c-22c2-4e13-826a-1a67ad75f79f
📒 Files selected for processing (1)
server/subscriber/MediaSubscriber.ts
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
Hey @dougrathbone, thanks for the PR but I'm noticing quite a lot of scope creep and unnecessary changes.
The core fix which was adding await to the subscriber lifecycle hooks as described in the pr description was already addressed in #2760, which has been merged. So the main motivation for this PR has already been resolved and now it's starting to creep out of scope.
The remaining part which is switching from getRepository() to event.manager for transaction scoping is also unfortunately incomplete.
This is because when the updateChildRequestStatus saves a MediaRequest, it triggers MediaRequestSubscriber.afterUpdate, and that entire subscriber (sendToRadarr, sendToSonarr, updateParentStatus, etc.) still uses global getRepository(). So the writes immediately break out of the transaction scope anyway, and the stated rollback guarantee doesn't actually really hold imo.
In addition, the event to media rename and toSave batching seems like minor cosmetic changes that don't add much value either, imo.
If we want to properly scope both subscribers to use transaction-bound managers, that'd be a larger effort and worth its own pr.
CC: @seerr-team/seerr-core
|
@fallenbagel thanks for the feedback. understand that this might've seemed low value. would you be open to me taking a pass at the bigger manager pr? would love to give it a go? in general i'm just going through the codebase looking for things i can help with as i'm a long time fan (long time listener, first time caller). if other areas might be worth a go i'd love the guidance |
Feel free to open a pr ofcourse! For the time being I'll mark this PR as blocked (so it would be more organised and clear) |
2 participants
Description
MediaSubscriber.beforeUpdatecalledupdateChildRequestStatuswithoutawait, andafterUpdatecalledupdateRelatedMediaRequestwithoutawait. This created a race condition where the subscriber lifecycle could return before the relatedMediaRequeststatus changes were committed to the database -- leaving requests stuck inPENDINGorAPPROVEDinstead of being promoted toCOMPLETEDwhen media became available.A second, related issue: both helpers resolved their repositories via the global
getRepository()from@server/datasourcerather than the transaction-scopedevent.manager. This meant their writes executed outside the event's unit of work. If the outerMediasave failed after those writes committed,MediaRequestandSeasonRequestrows could diverge from their parent media state.Fix:
awaitto bothupdateChildRequestStatusandupdateRelatedMediaRequestcall sites inbeforeUpdateandafterUpdateEntityManageras their first parameter, passingevent.managerfrom the subscribergetRepositoryimport from@server/datasourceeventparameter tomediainupdateChildRequestStatusfor clarityMediaRequestrows inupdateChildRequestStatusinstead of one save per loop iterationAI Disclosure: I used Claude Code to help write the tests and validate the approach. I reviewed and understood all generated code before submitting.
How Has This Been Tested?
Integration tests at
server/subscriber/MediaSubscriber.test.tsusing the real SQLite test database viasetupTestDb(), covering both standard and 4K request paths.Tests:
PENDINGrequest is promoted toCOMPLETEDwhen media transitionsPENDING -> AVAILABLEPENDING4K request is promoted toCOMPLETEDwhen media 4K status transitionsPENDING -> AVAILABLEAPPROVEDrequest is markedCOMPLETEDwhen media transitionsPROCESSING -> AVAILABLEAPPROVED4K request is markedCOMPLETEDwhen media 4K status transitionsPROCESSING -> AVAILABLEFAILEDrequest is markedCOMPLETEDwhen media transitionsPROCESSING -> AVAILABLEScreenshots / Logs (if applicable)
N/A -- backend-only change with no UI impact.
Checklist:
pnpm buildpnpm i18n:extract(no new UI strings added)Summary by CodeRabbit
Release Notes
Tests
Refactor