fix: isolate tag fetch failures to prevent sync abort on network errors#2681
Open
gitdeathbot wants to merge 3 commits intoseerr-team:developfrom
Open
fix: isolate tag fetch failures to prevent sync abort on network errors#2681gitdeathbot wants to merge 3 commits intoseerr-team:developfrom
gitdeathbot wants to merge 3 commits intoseerr-team:developfrom
Conversation
When tagRequests is enabled and Radarr/Sonarr is momentarily unreachable, getTags() would throw and escape the outer try-catch in sendToRadarr/sendToSonarr. This caused the error to propagate to the afterInsert subscriber catch, which only logged it - leaving the request stuck in APPROVED state with no Retry button and the media status permanently in PENDING with no externalServiceId written. Wrap the tagRequests block in its own try-catch in both sendToRadarr and sendToSonarr. A tag fetch failure now logs a warning and allows the sync to continue - the media is still added to the arr, just without the user tag.
📝 WalkthroughWalkthroughWraps Radarr and Sonarr tag fetch/create flows in guarded try/catch blocks, adds dual-format tag label lookup (legacy " Changes
Sequence Diagram(s)sequenceDiagram
participant Subscriber as MediaRequestSubscriber
participant Radarr as Radarr API
participant Sonarr as Sonarr API
participant Repo as seasonRequestRepository
Subscriber->>Radarr: getTags()
alt tags fetched
Radarr-->>Subscriber: tags list
Subscriber->>Radarr: find tag ("userId - " / "userId-")
alt tag exists
Radarr-->>Subscriber: tagId
else not found
Subscriber->>Radarr: createTag(label)
Radarr-->>Subscriber: new tagId or error
end
else fetch/create error
Radarr-->>Subscriber: error (logged), continue without tags
end
Subscriber->>Sonarr: getTags()
alt tags fetched
Sonarr-->>Subscriber: tags list
Subscriber->>Sonarr: find tag ("userId - " / "userId-")
alt tag exists
Sonarr-->>Subscriber: tagId
else not found
Subscriber->>Sonarr: createTag(label)
Sonarr-->>Subscriber: new tagId or error
end
else fetch/create error
Sonarr-->>Subscriber: error (logged), continue without tags
end
Subscriber->>Repo: approve child seasons (await save)
Repo-->>Subscriber: save complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
Author
|
fixes #2664 |
![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/MediaRequestSubscriber.ts (1)
303-355: Extract the Arr requester-tag flow into a shared helper.These Radarr and Sonarr blocks are now effectively the same algorithm with different clients and log metadata. Pulling the lookup/create/append/warn path into one helper would reduce drift the next time the tag format, fallback order, or warning fields change.
Also applies to: 668-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscriber/MediaRequestSubscriber.ts` around lines 303 - 355, Extract the duplicated Radarr/Sonarr requester-tag logic into a shared helper (e.g., ensureRequesterTag or addRequesterTag) that accepts the client (radarr/sonarr), server settings (radarrSettings/sonarrSettings), the tags array, the request entity (entity), logger, and sanitizeDisplayName; inside the helper implement the lookup fallback (id + ' - ' then id + '-' ), create tag if missing, append tag id to tags only if not present, and emit the same info/warn logs and return success/failure (or the tag id) so callers in MediaRequestSubscriber can call ensureRequesterTag(...) for both Radarr and Sonarr blocks instead of duplicating lines around getTags/createTag, logging, and the tags push.
🤖 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/MediaRequestSubscriber.ts`:
- Around line 307-314: The tag lookup currently prefers the legacy label "<id> -
" before the canonical "<id>-", causing new requests to attach to deprecated
tags; update the Radarr block (the userTag assignment using radarrTags.find and
entity.requestedBy.id) to first search for labels starting with
entity.requestedBy.id + '-' (no space) and only if not found fall back to
entity.requestedBy.id + ' - ' (with space), and apply the exact same swap to the
Sonarr lookup block (the analogous userTag/sonarrTags find logic around lines
~672-679) so canonical tags are preferred before legacy ones.
---
Nitpick comments:
In `@server/subscriber/MediaRequestSubscriber.ts`:
- Around line 303-355: Extract the duplicated Radarr/Sonarr requester-tag logic
into a shared helper (e.g., ensureRequesterTag or addRequesterTag) that accepts
the client (radarr/sonarr), server settings (radarrSettings/sonarrSettings), the
tags array, the request entity (entity), logger, and sanitizeDisplayName; inside
the helper implement the lookup fallback (id + ' - ' then id + '-' ), create tag
if missing, append tag id to tags only if not present, and emit the same
info/warn logs and return success/failure (or the tag id) so callers in
MediaRequestSubscriber can call ensureRequesterTag(...) for both Radarr and
Sonarr blocks instead of duplicating lines around getTags/createTag, logging,
and the tags push.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78214159-f662-4564-a35d-8e6499d58d7c
📒 Files selected for processing (1)
server/subscriber/MediaRequestSubscriber.ts
fallenbagel
reviewed
Mar 11, 2026
Collaborator
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
Please re-edit your pr description to follow the pr template:
https://github.com/seerr-team/seerr/blob/develop/.github%2FPULL_REQUEST_TEMPLATE.md
This is not related to the tag bugfix, but rather an existing build issue for prettier
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/MediaRequestSubscriber.ts`:
- Around line 958-961: Replace the non-async entity.seasons.forEach callback
with an async-aware loop so awaits actually run: iterate with for (const season
of entity.seasons) { season.status = MediaRequestStatus.APPROVED; await
seasonRequestRepository.save(season); } (or map to promises and await
Promise.all) and ensure the enclosing function (the one that calls this code,
e.g., updateParentStatus) is declared async so awaits are valid; keep references
to entity.seasons, season.status, MediaRequestStatus.APPROVED and
seasonRequestRepository.save when making the change.
🪄 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: 6217a836-30e3-4cf0-af67-c03517d9a667
📒 Files selected for processing (1)
server/subscriber/MediaRequestSubscriber.ts
Comment on lines
958
to
961
| entity.seasons.forEach((season) => { | ||
| season.status = MediaRequestStatus.APPROVED; | ||
| seasonRequestRepository.save(season); | ||
| await seasonRequestRepository.save(season); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("server/subscriber/MediaRequestSubscriber.ts")
lines = path.read_text().splitlines()
start, end = 953, 962
for lineno in range(start, end + 1):
print(f"{lineno}: {lines[lineno - 1]}")
block = "\n".join(lines[start - 1 : end])
has_for_each = "entity.seasons.forEach(" in block
has_await_save = "await seasonRequestRepository.save(season);" in block
has_async_callback = "entity.seasons.forEach(async" in block
if has_for_each and has_await_save and not has_async_callback:
raise SystemExit(
"\nVerification failed: found `await` inside a non-async `forEach` callback."
)
if has_for_each:
raise SystemExit(
"\nVerification failed: child season approvals still use `forEach`, so async saves are not awaited correctly."
)
print("\nVerification passed.")
PYRepository: seerr-team/seerr
Length of output: 480
Replace forEach with a for...of loop; the await on line 959 is invalid and still wouldn't wait.
Line 959 uses await inside the non-async callback opened on line 957, which is a syntax error. Even if that callback were changed to async, forEach would not wait for all season saves to finish before updateParentStatus() returns.
Suggested fix
- entity.seasons.forEach((season) => {
- season.status = MediaRequestStatus.APPROVED;
- await seasonRequestRepository.save(season);
- });
+ for (const season of entity.seasons) {
+ season.status = MediaRequestStatus.APPROVED;
+ await seasonRequestRepository.save(season);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entity.seasons.forEach((season) => { | |
| season.status = MediaRequestStatus.APPROVED; | |
| seasonRequestRepository.save(season); | |
| await seasonRequestRepository.save(season); | |
| }); | |
| for (const season of entity.seasons) { | |
| season.status = MediaRequestStatus.APPROVED; | |
| await seasonRequestRepository.save(season); | |
| } |
🧰 Tools
🪛 Biome (2.4.9)
[error] 960-960: await is only allowed within async functions and at the top levels of modules.
(parse)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/subscriber/MediaRequestSubscriber.ts` around lines 958 - 961, Replace
the non-async entity.seasons.forEach callback with an async-aware loop so awaits
actually run: iterate with for (const season of entity.seasons) { season.status
= MediaRequestStatus.APPROVED; await seasonRequestRepository.save(season); } (or
map to promises and await Promise.all) and ensure the enclosing function (the
one that calls this code, e.g., updateParentStatus) is declared async so awaits
are valid; keep references to entity.seasons, season.status,
MediaRequestStatus.APPROVED and seasonRequestRepository.save when making the
change.
Collaborator
There was a problem hiding this comment.
This PR needs a rebase as this is already fixed in the current develop so this entire fix of adding await here should be removed with merge conflict resolution
There was a problem hiding this comment.
@fallenbagel, thanks for the clarification!
@gitdeathbot — noted: the await-in-forEach fix already exists in develop, so when you rebase this PR onto develop you should simply drop this hunk during merge conflict resolution rather than carrying it forward. My original suggestion can be disregarded for the purposes of this PR.
🧠 Learnings used
Learnt from: gauthier-th
Repo: seerr-team/seerr PR: 2810
File: server/routes/issue.ts:278-279
Timestamp: 2026-04-12T14:02:31.091Z
Learning: In seerr-team/seerr, `IssueCommentSubscriber.ts` (server/subscriber/IssueCommentSubscriber.ts) re-fetches the IssueComment entity inside `sendIssueCommentNotification` using `getRepository(IssueComment).findOneOrFail({ where: { id: entity.id }, relations: { issue: { comments: true } } })` and does NOT rely on the subscriber event entity's relation data. Do not flag missing relation loading on the event entity in this subscriber.
Learnt from: fallenbagel
Repo: seerr-team/seerr PR: 2731
File: server/lib/i18n/extractMessages.ts:71-71
Timestamp: 2026-03-20T01:39:15.788Z
Learning: In seerr-team/seerr, the server-side i18n extraction script (`server/lib/i18n/extractMessages.ts`) intentionally calls `saveMessages()` without a `.catch()` handler, mirroring the pre-existing pattern in the frontend extractor. This is acceptable because Node.js 15+ exits with a non-zero code on unhandled promise rejections by default, ensuring CI correctly detects failures. Do not flag this as an issue in future reviews.
Learnt from: fallenbagel
Repo: seerr-team/seerr PR: 2810
File: server/routes/issue.ts:388-395
Timestamp: 2026-04-07T08:22:22.643Z
Learning: Repo seerr-team/seerr — Maintainers consider TOCTOU race conditions on issue delete (count-then-delete without a transaction/lock in server/routes/issue.ts) as overkill and irrelevant for this media request app. Do not suggest wrapping comment-count checks and deletes in a DB transaction or pessimistic locking for this code path.
Learnt from: Rubeanie
Repo: seerr-team/seerr PR: 2606
File: server/lib/availabilitySync.ts:998-1005
Timestamp: 2026-03-07T07:29:25.941Z
Learning: In `server/lib/availabilitySync.ts`, `seasonExistsInPlex()` and `seasonExistsInJellyfin()` intentionally return `true` (fail-open) when the episode fetch succeeds but returns an empty array. This is a deliberate design choice consistent with the broader fail-open pattern used throughout the class (e.g., catch blocks also return `true`), to avoid false negatives that would incorrectly mark media as deleted.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/subscriber/MediaRequestSubscriber.ts (1)
957-960:⚠️ Potential issue | 🔴 CriticalReplace this
forEachwith an async-aware loop.Line 959 is a parse error:
awaitis not valid inside this non-asynccallback. Even if you mark the callbackasync,forEachstill will not wait for the season saves beforeupdateParentStatus()returns.Suggested fix
- entity.seasons.forEach((season) => { - season.status = MediaRequestStatus.APPROVED; - await seasonRequestRepository.save(season); - }); + for (const season of entity.seasons) { + season.status = MediaRequestStatus.APPROVED; + await seasonRequestRepository.save(season); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscriber/MediaRequestSubscriber.ts` around lines 957 - 960, The code is using entity.seasons.forEach with an await inside a callback which is invalid and not waited on; replace the forEach block with an async-aware loop (e.g., a for...of loop or Promise.all) that awaits seasonRequestRepository.save for each season and only calls updateParentStatus() after all saves complete; locate the block referencing entity.seasons, season.status = MediaRequestStatus.APPROVED, and seasonRequestRepository.save and change to a loop that properly awaits each save (or map to promises and await Promise.all) before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/subscriber/MediaRequestSubscriber.ts`:
- Around line 957-960: The code is using entity.seasons.forEach with an await
inside a callback which is invalid and not waited on; replace the forEach block
with an async-aware loop (e.g., a for...of loop or Promise.all) that awaits
seasonRequestRepository.save for each season and only calls updateParentStatus()
after all saves complete; locate the block referencing entity.seasons,
season.status = MediaRequestStatus.APPROVED, and seasonRequestRepository.save
and change to a loop that properly awaits each save (or map to promises and
await Promise.all) before proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8fa49a1-ccdb-4e1b-af33-40def7481287
📒 Files selected for processing (1)
server/subscriber/MediaRequestSubscriber.ts
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Collaborator
|
@gitdeathbot Would it be possible for you to resolve the merge conflicts? Or should we take over this PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When tagRequests is enabled and Radarr/Sonarr is momentarily unreachable, getTags() would throw and escape the outer try-catch in sendToRadarr/sendToSonarr. This caused the error to propagate to the afterInsert subscriber catch, which only logged it - leaving the request stuck in APPROVED state with no Retry button and the media status permanently in PENDING with no externalServiceId written.
Wrap the tagRequests block in its own try-catch in both sendToRadarr and sendToSonarr. A tag fetch failure now logs a warning and allows the sync to continue - the media is still added to the arr, just without the user tag.
fixes #2664
How Has This Been Tested?
Wrapping existing code, as is, in a try/catch - this is not a functionality change.
Screenshots / Logs (if applicable)
Checklist:
AI wrote the description, the commit notes, etc. AI was used to quickly parse the existing project to find the functions that needed to be wrapped in try/catch. AI did tab complete things.
pnpm buildpnpm i18n:extractSummary by CodeRabbit