[NEEDS CODE REVIEWER] (time-out related) Fix detect_deletions disable gating and harden timeout handling

[tomerh2001]

Summary

• Fix detect_deletions gating so disabled jobs do not start folder watchers (main.py now checks .enabled).
• Add JobParams.__bool__ so object truthiness consistently reflects job enabled state.
• Add configurable global HTTP timeout via general.request_timeout (and env REQUEST_TIMEOUT) instead of a hardcoded 15s timeout in make_request.
• Add runtime error boundaries so request failures in ARR/download-client job groups are logged and the main loop keeps running instead of crashing.
• Update docs/example config for request_timeout and add regression tests.

Why

• Closes detect_deletions runs even when explicitly disabled (v2.0.0) #329: detect_deletions was starting folder watchers even when disabled; main.py now checks .enabled.
• Fixes decluttarr crashes on slow server when read times out #317: makes the HTTP timeout tunable via request_timeout (env REQUEST_TIMEOUT) and adds per-group error boundaries so one request failure no longer terminates the full run loop.

Validation

• python3 -m pytest -q
• python3 -m pylint main.py src/job_manager.py src/utils/common.py src/settings/_general.py src/settings/_jobs.py src/settings/_user_config.py tests/jobs/test_job_manager.py tests/settings/test_general.py tests/settings/test_jobs.py tests/utils/test_common.py

[AverageCakeSlice]

Can we please get this merged? Would like to use V2 but this timeout issue is preventing me from connecting to my qbittorrent instance

[ManiMatter]

hi @tomerh2001 , 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. Would love to have you on board!

[lolimmlost]

Hey @tomerh2001, thank you for contributing! I am currently reviewing this as part of clearing the open PR backlog with @ManiMatter. Read through against the linked issues — code looks solid, two small asks before I'd approve.

Linkage confirmed:

• ✅ detect_deletions runs even when explicitly disabled (v2.0.0) #329 — main.py now checks .enabled, matches the root cause from the issue exactly. JobParams.__bool__ is a nice belt-and-suspenders touch and the regression test covers it.
• ✅ decluttarr crashes on slow server when read times out #317 — both halves addressed: configurable general.request_timeout (env REQUEST_TIMEOUT), and _run_arr_job_group / _run_download_client_job swallow request errors so one timeout no longer takes down the loop. test_run_jobs_keeps_running_when_one_group_raises_request_error directly exercises the crash scenario.

Ask 1 — auto-close keywords:
Could you update the PR body so the issues close on merge? Right now it says "Fixes behavior reported in #329" and "Addresses crash-loop behavior from timeout failures in #317" — GitHub doesn't parse those phrasings (text between keyword and hash), so issues stay open after merge. Two lines would do it:

Closes #329
Fixes #317

Heads-up — overlap with #346:
#346 also introduces general.request_timeout (same name, same default) and adds per-instance timeout overrides for arr + qbit + sabnzbd, but doesn't fix the crash behavior or the detect_deletions gate. They'll conflict on the general setting. My take: this PR is the more important merge (closes two high-prio bugs), and #346 can rebase on top to add per-instance granularity. Flagging so we can coordinate merge order.

Code-wise: error boundaries are scoped tightly (per ARR group, per download-client job, per qbit cookie refresh), request_timeout defaults to 15 so existing users see no behavior change, and the new tests are focused. Nothing else from me.

[jrhager84]

Hi all, I've been testing this PR extensively and would like to help get it over the line. @lolimmlost, you mentioned in April that you were reviewing this as part of the backlog. Are you still planning to take it? Happy to coordinate either way. @tomerh2001, thanks for the fix. I force-updated your branch to bring it up to date (details below). Your authorship and logic are unchanged. If you'd rather drive the PR yourself, just let me know.

What I changed on the branch:

• Rebased your commit onto the current dev branch. It was 22 commits behind, and the rebase completed cleanly with no conflicts.
• Kept main.py using CRLF line endings to match dev, so the diff only shows the ~15 actual logic changes instead of the whole file. The implementation itself is unchanged.
• Added a commit running black on the files touched by this PR, since the repository already enforces it via pre-commit.

Testing:

• Full test suite passes (206 tests). CI is green on my fork. Two deletion_handler tests fail locally on Windows due to path separator differences. They're pre-existing on dev and unrelated to this change.
• Reproduced decluttarr crashes on slow server when read times out #317 locally using mock *arr servers. Unpatched dev exits with code 1 on a read timeout, while this branch logs the error and continues.
• Verified both YAML (request_timeout) and environment (REQUEST_TIMEOUT) configuration, default timeout behavior when unset, per-instance isolation (a hanging Sonarr doesn't affect Radarr), recovery on the next cycle after the server responds, and the detect_deletions gating fix with both enabled and disabled configurations.
• Ran this on my own TrueNAS against a slow Sonarr, which is the exact decluttarr crashes on slow server when read times out #317 scenario. Three consecutive read timeouts were logged without restarting the container, and the strike system progressed from 1/3 → 2/3 → 3/3 → removal across those cycles. Before this fix, each timeout crash reset the in-memory strike tracker, making that progression impossible.

If anyone following this thread wants to verify the fix before merge, I've published a multi-arch image of this branch at ghcr.io/jrhager84/decluttarr:fix-detect-deletions-timeout-resilience (linux/amd64 and arm64). Point your compose file at it, set REQUEST_TIMEOUT as needed, and leave TEST_RUN: true if you'd prefer an observe-only run. I'd appreciate another confirmation from someone with a slow-server setup.

One scope note: the startup reachability check still exits on timeout by design. The current mitigation is configuring request_timeout. Making startup degrade gracefully on a per-instance basis is intentionally out of scope for this PR. I already have a follow-up ready once this one lands.

[lolimmlost]

Approved. The code looked solid when I reviewed it in April and the two asks were minor (auto-close keywords in the PR body, and coordinating merge order with #346 for the request_timeout overlap).

@jrhager84 — thanks for picking this up and doing the legwork: rebasing, formatting, reproducing the bug, publishing a test image. That kind of thorough validation is exactly what this PR needed to get over the line. Welcome aboard.

One note: the PR body still doesn't have the auto-close keywords. If someone can update it to include Closes #329 and Fixes #317, the issues will close automatically on merge.

[jrhager84]

Approved. The code looked solid when I reviewed it in April and the two asks were minor (auto-close keywords in the PR body, and coordinating merge order with #346 for the request_timeout overlap).

@jrhager84 — thanks for picking this up and doing the legwork: rebasing, formatting, reproducing the bug, publishing a test image. That kind of thorough validation is exactly what this PR needed to get over the line. Welcome aboard.

One note: the PR body still doesn't have the auto-close keywords. If someone can update it to include Closes #329 and Fixes #317, the issues will close automatically on merge.

Good call.

[jrhager84]

Merged. Thanks for the review and the welcome, @lolimmlost.

On the #346 overlap: agreed this was the right one to land first, since it closes both #317 and #329. @eadgbear, this is now in dev, so #346 will need a rebase. The general.request_timeout setting it adds is already here now, so the rebase is mostly dropping that duplicate and keeping your per-instance timeout: override on top of it. Happy to help with the rebase or handle it directly on the branch if you're swamped, just let me know.