Skip to content

[NEEDS CODE REVIEWER] Handle failed /sync/maindata requests in check_connected#342

Open
jamtur01 wants to merge 2 commits into
ManiMatter:devfrom
jamtur01:fix/handle-sync-maindata-500
Open

[NEEDS CODE REVIEWER] Handle failed /sync/maindata requests in check_connected#342
jamtur01 wants to merge 2 commits into
ManiMatter:devfrom
jamtur01:fix/handle-sync-maindata-500

Conversation

@jamtur01
Copy link
Copy Markdown

@jamtur01 jamtur01 commented Apr 18, 2026 •
edited
Loading

Previously, an HTTP error from the qBit API (e.g. a 500 from rdt-client) would crash decluttarr entirely. Now the error is caught and treated as a disconnected client, so the cycle is skipped and retried next run.

Here's an example of what happens:

decluttarr   | ERROR   | HTTP error occurred: 500 Server Error: Internal Server Error for url: http://somerdtclient:6500/api/v2/sync/maindata
decluttarr   | Traceback (most recent call last):
decluttarr   |   File "/app/src/utils/common.py", line 79, in make_request
decluttarr   |     response.raise_for_status()
decluttarr   |   File "/usr/local/lib/python3.10/site-packages/requests/models.py", line 1026, in raise_for_status
decluttarr   |     raise HTTPError(http_error_msg, response=self)
decluttarr   | requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://somerdtclient:6500/api/v2/sync/maindata
decluttarr   | Traceback (most recent call last):
decluttarr   |   File "/app/main.py", line 79, in <module>
decluttarr   |     asyncio.run(main())
decluttarr   |   File "/usr/local/lib/python3.10/asyncio/runners.py", line 44, in run
decluttarr   |     return loop.run_until_complete(main)
decluttarr   |   File "/usr/local/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
decluttarr   |     return future.result()
decluttarr   |   File "/app/main.py", line 67, in main
decluttarr   |     await job_manager.run_jobs(arr)
decluttarr   |   File "/app/src/job_manager.py", line 27, in run_jobs
decluttarr   |     await self.removal_jobs()
decluttarr   |   File "/app/src/job_manager.py", line 73, in removal_jobs
decluttarr   |     if not await self._download_clients_connected():
decluttarr   |   File "/app/src/job_manager.py", line 130, in _download_clients_connected
decluttarr   |     if not await self._check_client_connection_status(clients):
decluttarr   |   File "/app/src/job_manager.py", line 139, in _check_client_connection_status
decluttarr   |     if not await client.check_connected():
decluttarr   |   File "/app/src/settings/_download_clients_qbit.py", line 233, in check_connected
decluttarr   |     await make_request(
decluttarr   |   File "/app/src/utils/common.py", line 79, in make_request
decluttarr   |     response.raise_for_status()
decluttarr   |   File "/usr/local/lib/python3.10/site-packages/requests/models.py", line 1026, in raise_for_status
decluttarr   |     raise HTTPError(http_error_msg, response=self)
decluttarr   | requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://somerdtclient:6500/api/v2/sync/maindata
decluttarr exited with code 1

This closes #348

@jamtur01
Handle failed /sync/maindata requests in check_connected
6fd6f8e 
Previously, an HTTP error from the qBit API (e.g. a 500 from
rdt-client) would crash decluttarr entirely. Now the error is
caught and treated as a disconnected client, so the cycle is
skipped and retried next run.
@ManiMatter
Copy link
Copy Markdown
Owner

ManiMatter commented Apr 18, 2026

hi, I am truly sorry I haven't looked into your PR in such a long time. I do appreciate very much that you took the time to contribute.

Unfortunately, I don't have the time to look into it still.
To overcome me being the bottleneck, I am looking to open this repo up to other people who help maintain it, and contributors can review each others code / merge.

Would you be willing to act as a formal contributor? If yes, I will add you, and if I find others (from open PRs), hopefully you can review each others PR and they can be merged.

Thanks for letting me know, and apologies again for my radio silence.

@ManiMatter ManiMatter changed the title Handle failed /sync/maindata requests in check_connected [NEEDS CODE REVIEWER] Handle failed /sync/maindata requests in check_connected Apr 21, 2026
@lolimmlost
Copy link
Copy Markdown
Collaborator

lolimmlost commented Apr 30, 2026

Hey @jamtur01, Thanks for contributing! I am going through the open PRs with @ManiMatter to clear the backlog.

Fix is clean and scoped:
Catching HTTPError in check_connected and treating it as a disconnected client is the right shape — removal_jobs will skip just this client and retry next cycle instead of taking the whole app down. 17/10 in one file, no collateral.

Ask — issue reference:
Is there a specific issue this fixes? Didn't find one in the open list. If not, would you mind opening a bug issue with that traceback so we have something to attach Closes #N to in the PR body? Useful for the changelog and for users searching for the rdt-client/500 error.

Heads-up — relationship to #333:
#333 wraps removal_jobs in a broader try/except RequestException as a top-level safety net. Your handling is more granular — flags just this client as disconnected and lets the rest of the cycle continue — so they're complementary rather than competing. Both should land. Flagging in case of merge-order conflicts.

Otherwise all checks out!

@jamtur01 jamtur01 mentioned this pull request Apr 30, 2026
Open
lolimmlost
lolimmlost approved these changes May 10, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@lolimmlost lolimmlost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took it for a spin — fix holds up. Approving.

Smoke tested all four paths against check_connected:

  • ✅ HTTPError (500) → returns False, warning logged, no crash
  • ✅ Connected status → returns True
  • ✅ Disconnected status → returns False
  • ✅ Malformed/missing server_state → returns False (broad except Exception catches the KeyError cleanly)

Full test suite passes (192/192).

Follow-up worth flagging (not blocking this PR):
SABnzbd's check_connected in _download_clients_sabnzbd.py:122 has the same unguarded pattern — make_request + response.json() + dict access, no try/except. A 500 from SAB will crash decluttarr the same way. Happy to open a follow-up PR mirroring this fix once #342 lands so the diffs stay easy to review.

LGTM.

@lolimmlost lolimmlost mentioned this pull request May 10, 2026
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lolimmlost lolimmlost lolimmlost approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HTTP error from the qBit API crashes decluttarr

3 participants

@jamtur01 @ManiMatter @lolimmlost