Improve analyzer error handling and database retry logic #12
Conversation
- Change Pipeline.process_single_video to return {path, reason} error tuples for detailed error messages
- Add retry_state_transition helper with exponential backoff (10 retries, 50-500ms delays) for database lock handling
- Skip state transitions if video already in target/advanced state to prevent invalid transition errors
- Fix .iex.exs to use list_configs() and remove non-existent start/pause functions
- Extract mark_video_as_analyzed helper to satisfy Credo nesting depth requirements
There was a problem hiding this comment.
Pull request overview
This PR improves error handling and database concurrency management in the analyzer Broadway pipeline, while also fixing developer tooling issues in the IEx console.
Key Changes:
- Added state transition guards to prevent invalid transitions and reduce error log spam
- Introduced retry logic with exponential backoff for SQLite busy errors during state transitions
- Fixed
.iex.exshelper functions to use correct API methods and remove non-existent calls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/reencodarr/analyzer/broadway.ex |
Added state guards before transitions and retry logic for database busy errors; extracted into reusable retry_state_transition helper |
.iex.exs |
Fixed IEx helpers: removed no-op start/pause calls for Analyzer/Encoder, corrected list_services() to list_configs(), removed non-existent get_service_by_id call |
- Add output_extension/1 helper to determine output format based on input - MP4 inputs now encode to MP4, all others default to MKV - Update regex patterns to match both .mkv and .mp4 extensions - Update progress parser to handle dynamic extensions - All tests passing (877 tests, 0 failures)
The batch mediainfo processor was extracting the 'media' key twice: 1. Once in process_videos_with_mediainfo (line 157) 2. Again in process_video_with_mediainfo (line 179-184) This caused the validate_mediainfo check to fail because it was looking for a 'track' key in already-extracted media data, resulting in 'invalid video info' errors for MP4 files. Fixed by removing the premature extraction on line 157 and letting process_video_with_mediainfo handle the structure properly.
Now that MP4 encoding output format is properly supported, remove the temporary skip logic that was blocking MP4 files from: - File validation in analyzer pipeline - Encode queue processing MP4 files will now be analyzed and encoded like any other video format.
- Replace generic 'invalid video info' with actual validation error reasons - Refactor filter_valid_videos to preserve error details for each file - Fix unused variable warnings in media_test.exs
When file validation detects issues, handle them immediately: - Missing files: Delete the video record from the database - Empty files: Delete the empty file, delete the video record, and trigger a rescan in Sonarr/Radarr so they can re-download This cleanup happens early in the pipeline before any other processing, ensuring we don't waste time on files that can't be processed.
Vintage films (pre-2009) that have been remastered to HDR still benefit from film grain synthesis to preserve the authentic film aesthetic. Previously, grain was only applied to SDR vintage content. Now it applies to both SDR and HDR content from before 2009.
The service_id field on videos is stored as a string, but the Sonarr/Radarr API functions expect integers. This was causing the refresh and rename operations to fail silently after encoding completed. Now properly parses string service_id to integer before calling the API, with error handling for invalid IDs and missing service type/id.
When no videos have been processed recently, the performance monitor was still logging throughput statistics based on stale data within the 2-minute window. Now it checks for recent activity before attempting to adjust or log performance metrics, silently resetting the timer when the queue is idle.
- credo: 1.7.13 → 1.7.14 - ecto_sql: 3.13.2 → 3.13.3 - phoenix: 1.8.1 → 1.8.3 - phoenix_live_reload: 1.6.1 → 1.6.2 - phoenix_live_view: 1.1.17 → 1.1.18 - swoosh: 1.19.8 → 1.19.9 - plug: 1.18.1 → 1.19.1 (transitive) Also fixes new credo warnings for using length/1 when checking for empty lists - replaced with Enum.empty? or pattern matching.
As per PR feedback, replace the custom retry_state_transition/4 function with the existing Reencodarr.Core.Retry.retry_on_db_busy/2 that provides exponential backoff, jitter, and consistent error handling across the codebase.
| <!-- All Failures --> | ||
| <%= case Map.get(@video_failures, video.id) do %> | ||
| <% failures when is_list(failures) and length(failures) > 0 -> %> | ||
| <% failures when is_list(failures) and failures != [] -> %> |
There was a problem hiding this comment.
This guard pattern when is_list(failures) and failures != [] is inconsistent with the codebase's standard idiom for checking non-empty lists. The PR description mentions converting length() > 0 checks to not Enum.empty?() for consistency, but this change uses a different pattern (!= []). For consistency with the rest of the codebase changes in this PR, this should use when is_list(failures) and not Enum.empty?(failures) instead.
| <% failures when is_list(failures) and failures != [] -> %> | |
| <% failures when is_list(failures) and not Enum.empty?(failures) -> %> |
| if Enum.empty?(invalid_audio_tracks) do | ||
| changeset | ||
| else | ||
| add_error(changeset, :audio_tracks, "audio tracks must have valid channel information") |
There was a problem hiding this comment.
The condition has been inverted from if length(invalid_audio_tracks) > 0 to if Enum.empty?(invalid_audio_tracks), which swaps the error and success branches. Now the success case (empty list of invalid tracks) is in the do block and the error case is in the else block. This inverted logic is less intuitive. Consider using if not Enum.empty?(invalid_audio_tracks) to keep the error handling in the main branch for better readability.
| if Enum.empty?(invalid_audio_tracks) do | |
| changeset | |
| else | |
| add_error(changeset, :audio_tracks, "audio tracks must have valid channel information") | |
| if not Enum.empty?(invalid_audio_tracks) do | |
| add_error(changeset, :audio_tracks, "audio tracks must have valid channel information") | |
| else | |
| changeset |
| """ | ||
| @spec execute_batch_mediainfo([String.t()]) :: {:ok, map()} | {:error, term()} | ||
| def execute_batch_mediainfo(paths) when is_list(paths) and length(paths) > 0 do | ||
| def execute_batch_mediainfo([_ | _] = paths) do |
There was a problem hiding this comment.
The guard pattern has changed from when is_list(paths) and length(paths) > 0 to when [_ | _] = paths. However, this creates a subtle bug: the new pattern [_ | _] = paths is a match operation that will always succeed and bind the result to paths, rather than a proper guard. The correct non-empty list pattern in a guard should be [_ | _] without the assignment. The function will now match any value (not just non-empty lists) because the pattern match in the guard always succeeds.
| if Enum.empty?(all_files) do | ||
| 0 | ||
| else | ||
| batch_upsert_videos(all_files, service_type) | ||
| length(all_files) |
There was a problem hiding this comment.
The condition logic has been inverted (from if length(all_files) > 0 to if Enum.empty?(all_files)), which changes the branches that execute. However, the else branch now executes when there ARE files to process, and the if branch executes when there are NO files. This inverted logic is harder to read and reason about. It would be clearer to keep the positive condition if not Enum.empty?(all_files) so the main processing logic remains in the do block rather than the else block.
| if Enum.empty?(all_files) do | |
| 0 | |
| else | |
| batch_upsert_videos(all_files, service_type) | |
| length(all_files) | |
| if not Enum.empty?(all_files) do | |
| batch_upsert_videos(all_files, service_type) | |
| length(all_files) | |
| else | |
| 0 |
| def refresh_and_rename_from_video(%{service_type: nil}), | ||
| do: {:error, "No service type for video"} | ||
|
|
||
| def refresh_and_rename_from_video(%{service_id: nil}), | ||
| do: {:error, "No service_id for video"} |
There was a problem hiding this comment.
These new function clauses handle string-to-integer parsing for service_id, but they don't handle the new error clauses at lines 395-399 that return errors for nil service_type or service_id. When a video has nil service_type or service_id, the error tuples will be returned but there's no clear error handling path in the callers. This could lead to unexpected error propagation. Consider whether these error returns should trigger specific logging or failure tracking, or if the function should have a catch-all clause that logs unexpected patterns.
| def refresh_and_rename_from_video(%{service_type: nil}), | |
| do: {:error, "No service type for video"} | |
| def refresh_and_rename_from_video(%{service_id: nil}), | |
| do: {:error, "No service_id for video"} | |
| def refresh_and_rename_from_video(video = %{service_type: nil}) do | |
| Logger.error("refresh_and_rename_from_video: No service type for video: #{inspect(video)}") | |
| {:error, "No service type for video"} | |
| end | |
| def refresh_and_rename_from_video(video = %{service_id: nil}) do | |
| Logger.error("refresh_and_rename_from_video: No service_id for video: #{inspect(video)}") | |
| {:error, "No service_id for video"} | |
| end |
| if Enum.empty?(specific_errors) do | ||
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code})" | ||
| else | ||
| error_details = Enum.join(specific_errors, "; ") | ||
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code}): #{error_details}" |
There was a problem hiding this comment.
The condition has been inverted from if length(specific_errors) > 0 to if Enum.empty?(specific_errors), which swaps the branches. Now the "no specific errors" case is in the do block and the "has specific errors" case is in the else block. This inverted logic makes the code less intuitive to read. Consider using if not Enum.empty?(specific_errors) to keep the positive condition in the main branch for better readability.
| if Enum.empty?(specific_errors) do | |
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code})" | |
| else | |
| error_details = Enum.join(specific_errors, "; ") | |
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code}): #{error_details}" | |
| if not Enum.empty?(specific_errors) do | |
| error_details = Enum.join(specific_errors, "; ") | |
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code}): #{error_details}" | |
| else | |
| "#{base_message} (FFmpeg exit #{ffmpeg_exit_code})" |
| {:req, "~> 0.5"}, | ||
| {:stream_data, "~> 1.2.0", only: :test}, | ||
| {:swoosh, "~> 1.5"}, | ||
| {:swoosh, "~> 1.19.9"}, |
There was a problem hiding this comment.
The version constraints for these dependencies have been changed from flexible ranges to exact minor versions (e.g., ~> 1.7 → ~> 1.7.14). This is overly restrictive and contradicts Elixir's semantic versioning conventions. The ~> operator already ensures compatible versions within the same minor version, so specifying the patch version defeats this purpose and will prevent automatic bug fixes and security patches.
Recommend reverting to flexible version constraints (e.g., ~> 1.7, ~> 3.13, ~> 1.8, ~> 1.6, ~> 1.1, ~> 1.19) to allow patch version updates while maintaining compatibility.
| {:swoosh, "~> 1.19.9"}, | |
| {:swoosh, "~> 1.19"}, |
Changes
Testing
All 877 tests pass. Manual testing shows:
Related Issues
Fixes Broadway infinite loop issues and improves error visibility for debugging analyzer failures.