Degrade unreachable instances at startup instead of exiting the whole app - #366
Conversation
|
@lolimmlost — any chance you can review (and test if you're able) to make sure I didn't miss anything? It also sets up a follow-up PR I have that adds qBit 5.2 API-key auth — with graceful degradation in place, a bad or rotated key degrades instead of crash-looping the container. |
350dae1 to
18e2e51
Compare
|
Is anybody available to review this? My qBit API key branch builds on this degradation logic so a bad or rotated key degrades the client instead of taking down the whole app in a crash loop. Tagging @ManiMatter for visibility. @lolimmlost or @Dark3clipse, you guys around today? Thanks! |
lolimmlost
left a comment
lolimmlost
left a comment
There was a problem hiding this comment.
Reviewed the full diff. Architecture is sound and the fail-closed removal guard is the most important piece -- correctly skips downloads whose client is degraded rather than deleting them without being able to verify protection status.
Key points verified:
- Transient vs definitive classification is correct across all three client types. The qBit bad-password detection (HTTP 200 + "Fails.") as definitive is a good catch -- avoids retry-loop IP bans.
- _ignore_degraded_client_downloads uses the real group_by_download_id shape in its test, so the guard can't silently drift if the grouped dict changes.
- ready_only=True on get_download_client_by_name returns (None, None) for degraded clients, and all downstream call sites already handle the None case. Backward compatible since the default is False.
- Log dedup via str(e) != self.last_error keeps logs clean on repeated transient failures without hiding new failure modes.
- _exit_if_all_failed_definitively re-evaluates after each retry cycle, catching the edge case where a transient unit turns definitive on retry (e.g. slow qBit finally answers and reveals a bad key).
- Watcher setup on rejoin via setup_for_arr is a clean extraction from the existing get_folders_to_watch.
Test coverage is thorough -- all classification paths, recovery, log dedup, fail-closed guard, exit conditions, and watcher rejoin are exercised.
One minor note (not blocking): is_definitive_setup_error checks exc.cause but not context. All current raises use explicit "from e" so this is fine, but worth keeping in mind if future error paths use bare raise.
LGTM.
| for candidate in (exc, exc.__cause__): | ||
| if getattr(candidate, "definitive", False): | ||
| return True | ||
| if isinstance(candidate, requests.exceptions.HTTPError): |
There was a problem hiding this comment.
Non-blocking: is_definitive_setup_error checks exc.__cause__ but not __context__. All current raises use explicit from e so this works, but if a future error path uses a bare raise inside an except block, the implicit chaining would land on __context__ instead and bypass the definitive check. Easy fix if it ever matters -- just add getattr(candidate, '__context__', None) to the loop.
There was a problem hiding this comment.
Fair point. I'll take a look 👍
Previously any single instance failing its startup check terminated decluttarr via wait_and_exit(), taking healthy instances down with it and causing container crash-loops on slow servers (ManiMatter#317 follow-up). Now each unit (arr, qBittorrent, SABnzbd) records a readiness state: - Transient failures (timeout, connection, 5xx, unknown) degrade the instance; setup is re-attempted every timer cycle and the instance rejoins automatically (including its detect_deletions watchers). - Definitive config errors (401/403, wrong username/password, bad SABnzbd api key, non-English UI, client version too old) degrade the instance with a per-cycle ERROR + tip; they are not retried since they cannot heal without user action (and retrying a bad password would get the IP banned by qBittorrent). - The app exits when nothing is configured, or when every configured unit has failed definitively - re-checked each cycle, not only at launch. Degraded download clients are skipped everywhere a job would call them (bandwidth checks, obsolete-tagging, bad-file handling) via a ready_only lookup. Removal jobs additionally fail closed: a download whose configured client is degraded is left untouched rather than deleted, since its protection status (protected tag, private/public tracker) cannot be verified while the client is down. Wrong-arr-type and arr-version-too-old keep their existing log-and-continue behavior. Repeated identical setup failures log a single-line skip instead of the full error block each cycle. Also fixes main.py handing the deletion watchers to a throwaway WatcherManager, which left terminate() stopping an instance that owned no observers.
18e2e51 to
cf225b8
Compare
2 participants
Summary
Today, if any single instance (Sonarr/Radarr/qBittorrent/SABnzbd) fails its startup check, decluttarr calls
wait_and_exit()and the whole container goes down — taking healthy instances with it, and on a slow or briefly-unreachable server causing a crash-loop where it never gets past startup. This makes startup degrade per-instance instead.Behavior
Each unit now records a readiness state at setup:
detect_deletionswatchers).Safety: degraded download clients fail closed
A degraded download client is skipped everywhere a job would call it. Critically, removal jobs fail closed: a download whose configured client is degraded is left untouched rather than deleted, because its protection status (protected tag, private/public tracker) can't be verified while the client is down. This prevents a misconfigured or unreachable qBittorrent from causing protected or private torrents to be removed. Downloads on healthy clients — and on clients not configured in decluttarr — are unaffected.
Why
This complements #333: that made the runtime resilient to request errors; this extends the same resilience to startup, so a single slow or misconfigured instance no longer crash-loops the container — the exact pain reported in #317. It also fixes a latent bug where
main.pyhanded the deletion watchers to a throwawayWatcherManager, soterminate()stopped an instance that owned no observers.Validation
tests/deletion_handlerfailures are pre-existing Windows path-separator issues that reproduce identically ondev).Notes
Closes #…— this is proactive hardening rather than a fix for a specific open report, though it is the startup half of the decluttarr crashes on slow server when read times out #317 / [NEEDS CODE REVIEWER] (time-out related) Fix detect_deletions disable gating and harden timeout handling #333 resilience story.