Stop polling when backend state is unavailable - #470
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughStrict content identity filtering, scoped URL credential redaction, bounded polling, distinct release retries, and playback identity metadata were added. Episode player templates now pass separate show and episode TMDB identifiers, with tests covering the new filtering, retry, observability, redaction, and metadata behavior. ChangesResolution reliability and playback identity
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 5 medium |
🟢 Metrics 138 complexity · 0 duplication
Metric Results Complexity 138 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_release_retry.py (1)
17-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer unpacking over list concatenation.
♻️ Suggested fix
- _attach_retry_candidates(params, selected, [selected, duplicate] + distinct) + _attach_retry_candidates(params, selected, [selected, duplicate, *distinct])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_release_retry.py` around lines 17 - 33, Update test_retry_candidates_are_distinct_and_capped to pass the retry candidates by unpacking the selected and duplicate rows alongside distinct, instead of concatenating lists, while preserving the assertion that only the first five distinct candidates are retained.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@repo/plugin.video.nzbdav/resources/lib/filter.py`:
- Around line 567-574: Update _media_type_matches and the filtering flow used by
_prepare_picker_rows so season-pack rows or season-only candidates bypass the
strict _episode_identity_matches check when both season and episode are set.
Preserve strict episode-marker validation for individual episode candidates,
while allowing whole-season titles such as S01.COMPLETE to remain available for
_prepend_pack.
In `@repo/plugin.video.nzbdav/resources/lib/http_util.py`:
- Around line 44-53: Update redact_url() to apply the scoped getnzb credential
redaction to query parameters as well as parts.path, covering getnzb URLs with
?i=...&r=... and the existing .nzb& form. Keep i/r redaction limited to
getnzb-looking URLs, while preserving ordinary application query parameters and
existing _REDACT_PARAM_NAMES behavior.
---
Nitpick comments:
In `@tests/test_release_retry.py`:
- Around line 17-33: Update test_retry_candidates_are_distinct_and_capped to
pass the retry candidates by unpacking the selected and duplicate rows alongside
distinct, instead of concatenating lists, while preserving the assertion that
only the first five distinct candidates are retained.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 34bfd276-da4f-4edf-998c-ea437039d2e6
📒 Files selected for processing (11)
repo/plugin.video.nzbdav/resources/lib/filter.pyrepo/plugin.video.nzbdav/resources/lib/http_util.pyrepo/plugin.video.nzbdav/resources/lib/resolver.pyrepo/plugin.video.nzbdav/resources/lib/resolver_flow.pyrepo/plugin.video.nzbdav/resources/lib/resolver_pollloop.pyrepo/plugin.video.nzbdav/resources/lib/resolver_retry.pyrepo/plugin.video.nzbdav/resources/lib/router_play.pytests/test_filter.pytests/test_http_util.pytests/test_poll_observability.pytests/test_release_retry.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🪛 ast-grep (0.45.0)
repo/plugin.video.nzbdav/resources/lib/filter.py
[warning] 579-579: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _YEAR_RE.findall(candidate_title)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 535-540: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(
r"(?<![A-Za-z0-9])"
+ r"[\W_]+".join(re.escape(word) for word in words)
+ r"(?![A-Za-z0-9])",
re.I,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🪛 GitHub Check: Codacy Static Code Analysis
repo/plugin.video.nzbdav/resources/lib/filter.py
[warning] 544-544: repo/plugin.video.nzbdav/resources/lib/filter.py#L544
Method _episode_identity_matches has a cyclomatic complexity of 9 (limit is 8)
repo/plugin.video.nzbdav/resources/lib/router_play.py
[warning] 491-491: repo/plugin.video.nzbdav/resources/lib/router_play.py#L491
Method _attach_retry_candidates has a cyclomatic complexity of 10 (limit is 8)
🪛 Ruff (0.16.0)
tests/test_release_retry.py
[warning] 31-31: Consider [selected, duplicate, *distinct] instead of concatenation
Replace with [selected, duplicate, *distinct]
(RUF005)
🔇 Additional comments (11)
repo/plugin.video.nzbdav/resources/lib/filter.py (1)
518-621: LGTM! Strict identity filter logic and its rejection/acceptance paths match the provided test cases (test_strict_identity_filter_rejects_wrong_title_episode_and_extras,test_strict_identity_filter_rejects_episode_and_wrong_year_for_movie).repo/plugin.video.nzbdav/resources/lib/router_play.py (1)
331-345: LGTM! Retry-candidate attachment and identity-based picker filtering are wired consistently across all play/search selection paths, and the dedupe/cap logic matchestest_retry_candidates_are_distinct_and_capped.Also applies to: 441-441, 472-514, 827-827, 853-856, 911-911, 933-933
tests/test_filter.py (1)
11-11: LGTM!Also applies to: 38-71
repo/plugin.video.nzbdav/resources/lib/http_util.py (1)
127-134: LGTM!_redact_url_spancorrectly scopes fullredact_url()redaction (userinfo + getnzb path credentials) to just the matched embedded URL span, confirmed bytest_redact_text_scopes_getnzb_path_credentials_to_url.Also applies to: 152-152
tests/test_http_util.py (1)
232-248: LGTM!Also applies to: 306-333
repo/plugin.video.nzbdav/resources/lib/resolver.py (1)
96-97: LGTM!Also applies to: 444-448, 494-497
repo/plugin.video.nzbdav/resources/lib/resolver_pollloop.py (1)
84-135: LGTM! Bounded observation-outage tracking correctly requires a continuous 60s streak of total backend-observation failure, surfaces a local-only timeout without cancelling the remote job, and matches the accompanyingtest_poll_observability.pycoverage.Also applies to: 241-254, 271-271, 286-297, 321-321
tests/test_poll_observability.py (1)
15-125: LGTM!repo/plugin.video.nzbdav/resources/lib/resolver_retry.py (1)
1-73: LGTM! The dead-gated retry sequencing correctly avoids triggering a fallback release attempt on an ambiguous bounded-observation timeout (since that path never marks the URL dead), only on a provably terminal failure — matchestest_release_retry_runs_only_after_provably_dead_attempt/test_release_retry_stops_after_nonterminal_failure.repo/plugin.video.nzbdav/resources/lib/resolver_flow.py (1)
201-218: LGTM!tests/test_release_retry.py (1)
35-81: LGTM!
| # Some Newznab indexers return download URLs shaped like | ||
| # ``/getnzb/id.nzb&i=ACCOUNT&r=APIKEY``. Because there is no ``?``, both | ||
| # credentials are parsed as part of the URL path rather than query params. | ||
| # Restrict the short ``i``/``r`` names to getnzb-looking paths so ordinary | ||
| # application URLs using those names are not over-redacted. | ||
| _GETNZB_PATH_CRED_RE = re.compile( | ||
| r"([&;](?:i|r))=([^&\s\"'<>]+)", | ||
| re.IGNORECASE, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "_REDACT_PARAM_NAMES" repo/plugin.video.nzbdav/resources/lib/http_util.py -B2 -A10Repository: Appz4Fun/nzbdavkodi
Length of output: 1244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' repo/plugin.video.nzbdav/resources/lib/http_util.py | cat -n
printf '\n--- occurrences ---\n'
rg -n "_GETNZB_PATH_CRED_RE|redact_url|_REDACT_PARAM_NAMES|getnzb" repo/plugin.video.nzbdav/resources/lib
printf '\n--- read-only parser probe ---\n'
python3 - <<'PY'
import urllib.parse as up
url = "https://example.com/getnzb/id.nzb?i=ACCOUNT&r=APIKEY"
parts = up.urlparse(url)
print("scheme", parts.scheme)
print("netloc", parts.netloc)
print("path", parts.path)
print("query", parts.query)
print("parsed qsl keys:", [k for k, v in up.parse_qsl(parts.query, keep_blank_values=True)])
PYRepository: Appz4Fun/nzbdavkodi
Length of output: 11415
Redact i/r when they appear as getnzb query parameters.
redact_url() only applies _GETNZB_PATH_CRED_RE to parts.path; a ?-delimited getnzb URL such as .../getnzb/id.nzb?i=ACCOUNT&r=APIKEY keeps those values in parts.query, and i/r are not included in _REDACT_PARAM_NAMES, so they would round-trip unredacted. Handle the same scoped getnzb/.nzb& case for query params as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@repo/plugin.video.nzbdav/resources/lib/http_util.py` around lines 44 - 53,
Update redact_url() to apply the scoped getnzb credential redaction to query
parameters as well as parts.path, covering getnzb URLs with ?i=...&r=... and the
existing .nzb& form. Keep i/r redaction limited to getnzb-looking URLs, while
preserving ordinary application query parameters and existing
_REDACT_PARAM_NAMES behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
repo/plugin.video.nzbdav/resources/lib/resolver_resume.py (1)
168-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply playback identity exactly once in each no-proxy path.
_resolve_direct_no_proxyreceivesparamsbut never uses them, so handle-based direct playback loses the new metadata/unique-ID handoff. Conversely, the player path invokes the metadata lookup twice.Proposed fix
def _resolve_direct_no_proxy( handle, stream_url, stream_headers, monitor_key, resume_seconds, params=None ): @@ li = _resolver._make_playable_listitem(bust_url, stream_headers) + _resolver._apply_playback_identity(li, params) _apply_resume_start_offset(li, resume_seconds) @@ li = _resolver._make_playable_listitem(bust_url, stream_headers) _resolver._apply_playback_identity(li, params) - _resolver._apply_playback_identity(li, params) _apply_resume_start_offset(li, resume_seconds)Also applies to: 306-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@repo/plugin.video.nzbdav/resources/lib/resolver_resume.py` around lines 168 - 180, Update _resolve_direct_no_proxy to use its params argument when applying playback identity, ensuring handle-based direct playback receives the metadata and unique-ID handoff. Inspect the corresponding player path around the additionally flagged lines and remove the duplicate metadata lookup so identity is applied exactly once per no-proxy path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@repo/plugin.video.nzbdav/resources/lib/resolver_resume.py`:
- Around line 168-180: Update _resolve_direct_no_proxy to use its params
argument when applying playback identity, ensuring handle-based direct playback
receives the metadata and unique-ID handoff. Inspect the corresponding player
path around the additionally flagged lines and remove the duplicate metadata
lookup so identity is applied exactly once per no-proxy path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: eb2ead97-aa71-474c-a4c3-a365a213d1c1
📒 Files selected for processing (11)
repo/plugin.video.nzbdav/resources/lib/filter.pyrepo/plugin.video.nzbdav/resources/lib/player_installer.pyrepo/plugin.video.nzbdav/resources/lib/resolver.pyrepo/plugin.video.nzbdav/resources/lib/resolver_flow.pyrepo/plugin.video.nzbdav/resources/lib/resolver_metadata.pyrepo/plugin.video.nzbdav/resources/lib/resolver_resume.pyrepo/plugin.video.nzbdav/resources/lib/router_play.pyrepo/plugin.video.nzbdav/resources/players/nzbdav.jsontests/test_player_installer.pytests/test_resolver.pytests/test_resolver_metadata.py
🚧 Files skipped from review as they are similar to previous changes (3)
- repo/plugin.video.nzbdav/resources/lib/resolver_flow.py
- repo/plugin.video.nzbdav/resources/lib/filter.py
- repo/plugin.video.nzbdav/resources/lib/router_play.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🪛 ast-grep (0.45.0)
repo/plugin.video.nzbdav/resources/lib/resolver_metadata.py
[info] 90-90: use jsonify instead of json.dumps for JSON output
Context: json.dumps(request)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_player_installer.py
[info] 137-137: use jsonify instead of json.dumps for JSON output
Context: json.dumps(PLAYER_JSON)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_resolver_metadata.py
[info] 27-27: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"result": {"files": []}})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (10)
repo/plugin.video.nzbdav/resources/lib/resolver.py (4)
96-97: LGTM!
380-386: LGTM!
451-455: LGTM!
501-504: LGTM!repo/plugin.video.nzbdav/resources/lib/resolver_metadata.py (1)
12-188: LGTM!repo/plugin.video.nzbdav/resources/lib/player_installer.py (1)
35-54: LGTM!repo/plugin.video.nzbdav/resources/players/nzbdav.json (1)
6-8: LGTM!tests/test_player_installer.py (1)
135-165: LGTM!tests/test_resolver.py (1)
2650-2654: LGTM!Also applies to: 2736-2736, 2794-2794, 3274-3274, 3548-3548, 3614-3617, 3693-3693
tests/test_resolver_metadata.py (1)
13-91: LGTM!
|
@aryan-j Let me know when this fixes your issue, and if you can give me a nzb and I can test it locally and will merge. it in. Thanks for getting into the source code and helping with this more advanced type of issue -- its some of the hardest to code for! |
2 participants
Summary
QueuedstateEvidence
A real S02E01 request had one terminal missing-article failure followed by a second job whose status/history/WebDAV calls all returned HTTP 500. Kodi retained
Queued; the remote second job later completed. The configured one-hour download timeout did not bound this observation outage, and cancellation also returned HTTP 500.Service-log follow-up identified the shared HTTP 500 source as intermittent NZBDav SQLite Error 14 (
unable to open database file) affecting its API and WebDAV layers. That service-storage issue is operationally separate from this add-on-side bounded failure handling.Codacy remediation
The original check reported six exact annotations: one ErrorProne observation-state comparison warning, four cyclomatic-complexity findings, and one method-length finding. The observation tracker now uses an explicit numeric sentinel and helper; new identity, retry-selection, and poll-state logic was decomposed below Codacy's thresholds without changing behavior.
Validation
/bin/bashThis draft is the canonical review target and supersedes #469.