Repository navigation
Conversation
A file ffprobe rejects is marked probe_failed_at, but the scan state never loaded that mark. Without probe metadata the file counts as needing a probe repair on every scan, so each rescan ran ffprobe and the OSHash on the same unchanged bytes again, and failed again. The scan state now loads probe_failed_at. A marked file that never probed successfully skips the probe repair while its size and modification time are the ones that were rejected. Changed bytes drop the mark and are probed as before, and playback still probes the file when it is played.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change stops repeated ffprobe work for unchanged files that ffprobe has already rejected, and it keeps existing probe data when such a file is rescanned for other reasons. No actionable merge-blocking risk remains. CI and the database contract tests were still pending when the PR was written. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely summarizes the main change: preventing repeated probing of unchanged files rejected by ffprobe. Description check The description directly explains the scanner bug, implementation, behavior changes, tests, risks, and validation for the changeset.
-
Full details: Docstring Coverage
-
-
Explanation
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. (1 skipped: 1 unsupported.)
-
✨ Finishing Touches
-
-
-
🧪 Generate unit tests (beta)
-
-
-
- Create a new PR
-
-
-
-
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Quick104
added
priority: P2
Cursor
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
Adds a database contract that a rejected file's probe_failed_at reaches the scan state a library scan reads, and that a rescan of the unchanged file then gets no probe_repair reason.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
Visible effect without evidence: Suggested fix: No diff; update the Evidence section with those captures. Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
|
The recorded Series — Web admin C2/C3 rescans (server Suggested Automated check: Macroscope check run agent (gpt-6-luna). No validation was performed. Posted via Macroscope — v1 validation impact |
A row probed before rejections were tracked keeps its old probe facts without probe_updated_at. Once it is marked rejected, a rescan for a subtitle sidecar no longer carries probe_repair, so the failed probe stopped counting as a repair and the full upsert cleared those facts. A standing rejection now counts like the repair reason when deciding to keep the stored probe metadata.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
An unchanged rejected file can remain unavailable after an ffprobe upgrade: Suggested fix: No diff; add the API response excerpts to the Evidence section. Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
A standing rejection never carries probe_repair, so every reason it has is an identity or subtitle change; otherwise probe_repair is one of the reasons and any second one needs the update. This drops the repeated string literal the changed-line lint flagged.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
Quick104
left a comment
Quick104
left a comment
There was a problem hiding this comment.
Review of c91f195a5. The approach is sound: the folder-scan path now leaves an unchanged rejected file alone, and the probe-fact preservation for a subtitle-triggered rescan holds. I found three gaps, two inline and one here. I'm fixing all three in commits on this branch, so there's no need to change anything yourself.
Rejected extras still re-probe on every scan (low). processExtraFiles (internal/scanner/extras.go, the unchanged check before gatherHints) only counts an extra as unchanged when ProbeUpdatedAt != nil. A damaged trailer in a Trailers/ folder is never probed successfully, so every scan of that folder runs ffprobe and the OSHash on it again, which is the cost this PR removes for primary files. Repair: also count it as unchanged while probeRejectionStands holds for that row.
Also checked, no change needed:
- Native playback still re-probes a marked file before the planner refuses it (
ensurePlaybackProberunsensureProbeRepairbeforePlanPlaybackV3checksProbeRejected). Jellyfin-protocol playback has no such probe, so for a Jellyfin-only household the "playback still probes it" fallback doesn't apply. That is worth one line under Risks. - The Kody thread's fix is in place, and both new DB contract tests passed in CI on this head.
AI-assisted review: Claude Code, claude-opus-5-5, with a cross-provider pass by Codex (gpt-6.1-sol). I checked each finding against the source.
ScanFile builds its scan state from the stored row, and that conversion dropped probe_failed_at, so a targeted scan of an unchanged rejected file still probed it again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The extras unchanged check required a successful probe, so a damaged extra was probed again on every scan. Treat a standing rejection as unchanged, and test the single-file scan state conversion too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Exit status 126 or 127 comes from the shell or the dynamic loader, not from ffprobe refusing the file. Scans now skip marked files, so such a mark would no longer be retried. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
Resolve the db-contracts.txt list conflict by keeping both sides. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review summaryReviewed head: Fit: 9/10 · Readiness: 8/10 — conditional Fit: the change stops the scanner from running ffprobe and the OSHash again on files whose bytes it has already rejected. Rescans cost nothing for those files, no setting is added, and the decision sits in the scanner's scan-state check, where the other "does this file need work" decisions already live. The one trade-off, that a rejection now holds until the file changes or is played, is disclosed under Risks. Readiness: no substantive defect remains after the fixes below, and they are tested and checked live. It is conditional because the required Fixed on this branch
Each fix has a test that fails without it. Needs your call
Other feedback
Validated featuresNo case recorded as passed on the v1.0 board reaches this change. The board lists #1251 C2 and C3 and #1252 C6 as not run. The rehearsal results posted on those issues rescanned a corpus with damaged episodes and should be re-checked after merge, as the PR body says. I updated the Clients and docs
AI-assisted review: Claude Code (claude-opus-5-5), with a cross-provider round by Codex (gpt-6.1-sol). Codex's three findings: the single-file gap (fixed), exit-127 rejections (fixed), and a catalog-seed overwrite case I dropped because it needs a replacement file whose size matches the seed while its mtime equals the old row's. |
|
On the retry question from your review: I'd store the ffprobe version with the rejection and probe the file again when the version changes. That covers an ffprobe upgrade without re-probing broken files on a timer, and because the scan does the re-probe, Jellyfin-only households get a retry path too. It would need a small migration. |
2 participants
Problem
Related issue: N/A
Validation tasks: none recorded as passed on the board. The rehearsal runs reported on #1251 (C2, C3) and #1252 (C6) rescanned a corpus with damaged episodes and should be re-checked after merge; #1250 C1 and the #197 rescan case are not run yet.
A video file that ffprobe rejects is probed again on every scan of its library, even when the file hasn't changed. Each rescan runs ffprobe and the OSHash on the same bytes, gets the same rejection, and logs "scanner: ffprobe failed" again. #1810 added the
probe_failed_atmark and noted this as a remaining limitation in its Risks section.This change skips the probe repair for a rejected file while its size and modification time are the ones that were rejected.
The validation runs for #1251 C2 and C3 and #1252 C6 rescanned a corpus that includes zero-byte, random-byte and truncated episodes. Their outcomes should not change: the damaged files keep their mark and their "Damaged file" state, and only the repeated ffprobe and OSHash work on unchanged files goes away. Those cases should still be re-checked after merge, because the rescan path changes.
On my server, 2 files totalling 10.5 GiB are marked rejected and have never probed successfully. The scanner logged ffprobe failures for 3 files in 24 hours, one of them in two scans 6.5 hours apart. The cost grows with the number of broken files and how often their folders are scanned.
Root cause: the scan state that
scanStateUpdateReasonsreads (scanStateFile) never loadedprobe_failed_at. A row with no probe metadata always counts as needing a playback-critical probe repair (needsCriticalProbeRepairScanState). So a rejected, unchanged file getsprobe_repairon every scan and goes through the full path, ffprobe and OSHash included.Approach
probe_failed_at.scanStateNeedsProbeRepairskips the probe repair when all of these hold:probe_updated_atis null);scanStateUpdateReasonsandshouldSkipStableConfirmedScanStateboth use it, so they agree.shouldPreserveExistingProbeAfterProbeFailurenow treats a standing rejection like the repair reason.probe_failed_atis only set when ffprobe ran, read the file and rejected its content (IsProbeRejection). Timeouts, cancellations, and read or permission errors are never marked, so those files are still retried on every scan.size_changedormtime_changed, which re-probe the file as before, and the upsert drops the mark.ScanFile) build scan state withscanStateFromMediaFile, which now carriesprobe_failed_attoo, so the skip and the probe-fact preservation also apply there.extraFileUnchanged).IsProbeRejectionno longer counts exit status 126 or 127 as a rejection. Those come from the shell or the dynamic loader when ffprobe cannot start, for example when a shared library fails to load, and say nothing about the file.Validation
go test ./internal/scanner/ -count=1: pass.TestScanStateUpdateReasons_SkipsProbeRepairForRejectedUnchangedFilecovers three cases:probe_repairand takes the stable-file skip;mtime_changedandprobe_repair;probe_repair.On
main(52096ba88) the test doesn't compile, because the scan state has noProbeFailedAt: the rejection mark never reaches this decision, which is the bug.TestRescanKeepsProbeFactsOfARejectedUnchangedFileDB, also listed inscripts/ci/db-contracts.txt, scans a rejected row that still has old probe facts, after a new subtitle sidecar appears, with an ffprobe that rejects it. The facts and the rejection must survive, and the subtitle must be recorded. On this branch's earlier head (69a680533) the facts came back empty ("" "" 0).The whole
internal/scannerpackage passes against a migrated local PostgreSQL 18 database, including the existing rejection tests (TestScanRecordsAndClearsProbeRejectionPostgres,TestUpsertKeepsProbeRejectionUntilProbedOrReplacedPostgres).TestScanStateCarriesProbeRejectionDBis a new database contract, listed inscripts/ci/db-contracts.txt. It stores a rejected file, reads it back throughGetScanStateByFolder(the query a library scan uses), and checks that a rescan of the unchanged file gets noprobe_repair. It passes against a migrated local PostgreSQL 18 database, as do the scanner's other scan state database tests (scoped_library_state_test.goandTestFileRepositoryPreservesFirstSeenScanProvenance).go vet ./internal/scanner/andmake lint-changed: clean.Review fixes:
TestScanStateFromMediaFileCarriesProbeRejection,TestExtraFileUnchangedSkipsRejectedExtra, and exit 126 and 127 cases inTestIsProbeRejectionSeparatesRefusedFilesFromProbeTrouble. Each fails without its fix.Live check on an isolated dev server built from
38b6d0b28: a movie library with a random-byte movie, a random-byte trailer extra, and a random-byte file at the library root. ffprobe ran once per file. A second and third library scan, and a single-file scan of the root file, logged no furtherscanner: ffprobe failedand left the marks in place.CI:
c91f195a5passed, including both new DB contract tests.Benchmarks
Not measured. On an unchanged rescan, the change removes one ffprobe run and one OSHash read per rejected file. The scan-state query reads one more column.
Evidence
What a rejected file shows doesn't change: it keeps its "Damaged file" state, and the scanner stops probing it again while it is unchanged.
Two automated evidence notes on this PR describe other states:
69a680533. It is fixed in1e9d2d56cand covered byTestRescanKeepsProbeFactsOfARejectedUnchangedFileDB. With the fix, the file's stored probe metadata matchesmain.The counts above came from read-only checks on my server, which runs my fork at
6ce7fb5cc.Evidence: https://evidence.siloserver.org/r/silo-server/pr-2075/
Risks
probe_failed_atalready exists (fix(playback): mark unreadable files and stop blaming the scan #1810).Checklist
AI Disclosure
IsProbeRejection,MarkProbeFailedand the upsert'sprobe_failed_atrule) before relying on it. The Kody review then found that a rescan for another reason cleared the stored probe metadata of a rejected legacy row. That is fixed and tested.AI-assisted with Claude Opus. I directed the task and designed the work.
Review fixes (c03e1e5, dca9b58, 38b6d0b): Claude Code, claude-opus-5-5, with a cross-provider review by Codex (gpt-6.1-sol).
Note
Stop re-probing unchanged files whose ffprobe rejection still stands
probe_repairupdate reason, and take the stable scan-state skip path inscanner.shouldSkipStableConfirmedScanState. See scanner.go.scanner.extraFileUnchangedpredicate in extras.go.scanner.IsProbeRejectionno longer classifies ffprobe exit statuses 126 and 127 (startup failures) as content rejections; other nonzero exits still use the stderr and bounded-read checks. See probe.go.probe_failed_atto the scan-state projection, row decoder, and single-file converter in scan_state.go, plus database-backed and unit tests registered in db-contracts.txt.probe_failed_aton every scan.Macroscope summarized 7de2231.