Add ab-av1 worker websocket and shared CRF claiming#19
Open
mjc wants to merge 49 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a websocket-based worker flow for CRF-search, including token authentication, session tracking, atomic work claiming, transfer streaming, dashboard and workers UI updates, diagnostics output, and supporting config/documentation. ChangesWorker Websocket Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant WorkerSocket
participant WorkerChannel
participant WorkerSessions
participant Media
Worker->>WorkerSocket: connect(token)
WorkerSocket-->>Worker: assign worker_id
Worker->>WorkerChannel: join("workers:crf_search")
Worker->>WorkerChannel: announce(payload)
WorkerChannel->>WorkerSessions: register(attrs)
WorkerChannel-->>Worker: accepted(protocol_version)
Worker->>WorkerChannel: pull_work
WorkerChannel->>Media: claim_next_video_for_crf_search
Media-->>WorkerChannel: video or nil
WorkerChannel->>WorkerSessions: assign_video
WorkerChannel-->>Worker: work_assigned or no_work
Worker->>WorkerChannel: crf_search_result / crf_search_completed
WorkerChannel->>WorkerSessions: clear_video
WorkerChannel-->>Worker: event_ack
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
mjc
commented
Jul 7, 2026
mjc
commented
Jul 7, 2026
mjc
commented
Jul 7, 2026
mjc
commented
Jul 7, 2026
mjc
commented
Jul 7, 2026
mjc
commented
Jul 7, 2026
There was a problem hiding this comment.
Pull request overview
Adds a Phoenix WebSocket/Channel surface for remote ab-av1 CRF-search workers, and unifies CRF job dispatching behind an atomic “claim” so local Broadway and remote workers don’t double-claim the same analyzed video.
Changes:
- Introduces authenticated worker WebSocket endpoint (
/workers/socket) with protocol validation, session tracking, heartbeat, and job assignment replies. - Adds an atomic DB claim path for CRF-search work and switches the local CRF Broadway producer to use it.
- Extends diagnostics + dashboard UI/docs/tests to surface worker connection info and validate protocol/session behavior.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/channel_case.ex | Adds a ChannelCase helper for channel tests with DB sandbox setup. |
| test/reencodarr/broadway_producers_test.exs | Adds tests asserting atomic claim transitions and prevents redispatch. |
| test/reencodarr/ab_av1/worker_sessions_test.exs | Adds tests for worker session lifecycle, expiry, and diagnostics output. |
| test/reencodarr/ab_av1/worker_protocol_test.exs | Adds tests for building job_assigned payloads. |
| test/reencodarr_web/live/dashboard_v2_live_test.exs | Tests dashboard display of worker WebSocket token fingerprint and URL. |
| test/reencodarr_web/channels/worker_channel_test.exs | End-to-end channel tests for auth, announce, work claiming, heartbeat, and disconnect requeue. |
| lib/reencodarr/media/video_queries.ex | Implements atomic “claim next CRF-search video” query + state transition. |
| lib/reencodarr/media.ex | Exposes claim_next_video_for_crf_search/0 from the Media context. |
| lib/reencodarr/diagnostics.ex | Adds worker session listing to diagnostics output. |
| lib/reencodarr/crf_searcher/broadway/producer.ex | Switches Broadway dispatch to atomic claim to avoid double-claiming. |
| lib/reencodarr/application.ex | Starts the WorkerSessions GenServer under the main supervision tree. |
| lib/reencodarr/ab_av1/worker_sessions.ex | New GenServer/ETS-backed worker session registry with expiry/touch/assignment. |
| lib/reencodarr/ab_av1/worker_protocol.ex | New helpers for protocol validation and server reply payloads. |
| lib/reencodarr_web/live/dashboard_live.ex | Displays worker socket URL and token fingerprint (without exposing the token). |
| lib/reencodarr_web/endpoint.ex | Mounts the worker socket at /workers/socket. |
| lib/reencodarr_web/channels/worker_socket.ex | Implements token-authenticated socket connect and worker socket ID. |
| lib/reencodarr_web/channels/worker_channel.ex | Implements worker channel join/announce/heartbeat/work request/disconnect behavior. |
| docs/ab_av1_worker_websocket.md | Documents initial worker WebSocket contract and payload shapes. |
| config/runtime.exs | Adds runtime worker-token config (env var or derived fallback). |
| .serena/memories/tech_stack.md | Adds project “memory” notes (tech stack). |
| .serena/memories/task_completion.md | Adds project “memory” notes (finish gates). |
| .serena/memories/suggested_commands.md | Adds project “memory” notes (common commands). |
| .serena/memories/memory_maintenance.md | Adds project “memory” notes (maintenance guidance). |
| .serena/memories/core.md | Adds project “memory” notes (repo structure/entry point). |
| .serena/memories/conventions.md | Adds project “memory” notes (key conventions). |
| .gitignore | Ignores Serena cache directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
needs more type specs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@config/runtime.exs`:
- Around line 23-32: The worker token derivation in runtime config uses a raw
SHA-256 hash of secret_key_base concatenated with a label, which should be
replaced with an HMAC-based derivation. Update the worker_token logic in the
runtime config to use :crypto.mac(:hmac, :sha256, secret_key_base,
"ab-av1-worker-token") instead of :crypto.hash/2 with string concatenation,
while keeping the existing Base.url_encode64 formatting and the surrounding
System.get_env("REENCODARR_WORKER_TOKEN") / SECRET_KEY_BASE fallback behavior.
In `@docs/ab_av1_worker_websocket.md`:
- Around line 28-32: The websocket docs example reply is missing the protocol
version field, so update the expected payload in the documentation to match what
WorkerProtocol.accepted/1 actually returns. Locate the “Expected reply payload”
example in the ab_av1 worker websocket doc and change it to include
protocol_version alongside accepted, keeping the documented contract aligned
with the server response.
In `@lib/reencodarr_web/channels/worker_channel.ex`:
- Around line 55-67: Add a catch-all handle_in/3 clause in WorkerChannel to
handle unexpected worker events instead of letting FunctionClauseError crash the
channel process. Keep the existing handle_in("heartbeat", ...),
handle_in("request_work", ...), and handle_in("pull_work", ...) clauses, then
add a fallback that matches any other event and replies with a structured
WorkerProtocol.error-style response so unknown messages are safely rejected
without dropping the socket.
In `@lib/reencodarr_web/channels/worker_socket.ex`:
- Around line 13-18: The token check in worker_socket’s connect/3 uses a normal
pattern match on the fetched :worker_token, which can leak timing information.
Update the authentication branch in connect/3 to compare the provided token
against the stored token with Plug.Crypto.secure_compare/2, and keep the
assign(socket, :worker_id, worker_id()) path only when that constant-time
comparison succeeds.
In `@lib/reencodarr/ab_av1/worker_sessions.ex`:
- Around line 98-137: The `handle_call/3` clauses for `{:touch, ...}`,
`{:assign_video, ...}`, and `{:clear_video, ...}` in `worker_sessions` duplicate
the same lookup/update/reply flow. Extract that shared logic into a private
helper such as `update_session_reply/3` (or similar) that takes
`server_worker_id`, `state`, and an update function, and have each clause
delegate to it while preserving the existing `lookup_session/1`,
`put_session/1`, and `{:error, :unknown_worker_session}` behavior.
- Around line 71-96: The `handle_call({:register, attrs}, _from, state)` path is
crashing on missing keys because it and `build_session/2` rely on
`Map.fetch!/2`, which can take down the GenServer and destroy the private
ETS-backed sessions. Add upfront validation for all required fields in
`register/1` or make `build_session/2` return a safe error instead of raising,
and ensure malformed attrs reply with an error rather than crashing. Use the
`handle_call({:register, ...})` and `build_session/2` symbols to locate the
unsafe fetches and guard them consistently.
In `@lib/reencodarr/media/video_queries.ex`:
- Around line 191-202: The initial candidate lookup in
claim_next_video_for_crf_search_in_tx/1 only fetches one Video id, which leaves
claim_next_video_for_crf_search_in_tx/2 with no rest list to retry when the
guarded update loses contention. Change the query to select a small ordered
batch of analyzed Video ids instead of a single row, so the recursive fallback
in claim_next_video_for_crf_search_in_tx/2 can continue to the next-best
candidate when the first claim fails.
In `@test/reencodarr_web/live/dashboard_v2_live_test.exs`:
- Around line 85-95: The dashboard_v2_live_test test mutates process-wide
Application config while the test module is async, which can leak the temporary
worker token into concurrent tests. Make this test module or the specific "shows
the worker websocket token when configured" test non-async, or refactor the
token lookup used by the LiveView so it can be injected/isolated instead of
relying on Application.get_env/put_env during the test. Keep the restore logic
in on_exit, but ensure no other async test can observe the transient
worker_token value.
In `@test/reencodarr/ab_av1/worker_protocol_test.exs`:
- Around line 1-27: Add unit test coverage in WorkerProtocolTest for the
untested WorkerProtocol.parse_announcement/1 and WorkerProtocol.error/1 paths.
Add cases that exercise invalid/untrusted announcement payloads and verify
parse_announcement/1 handles them as expected, including the missing catch-all
behavior noted in worker_protocol.ex, and add assertions that error/1 maps
worker failures to the correct protocol payload. Use the existing
work_assigned/2 test as a pattern and keep the new tests focused on the public
protocol API used by worker_channel.ex.
In `@test/reencodarr/ab_av1/worker_sessions_test.exs`:
- Around line 12-85: The same worker session attrs map is duplicated across all
four tests, so extract it into a shared helper/fixture and reuse it. Update the
tests in WorkerSessionsTest to call a local helper like worker_session_attrs/1,
or move the shared data into test/support/fixtures.ex if that is the standard
test fixture location, so future changes to the session fields happen in one
place.
- Around line 1-86: Add tests in WorkerSessionsTest to cover the untested error
branches: verify WorkerSessions.register/1 rejects a second registration with
the same server_worker_id or client_worker_id and returns {:error,
:duplicate_worker_id}, and add separate assertions that touch/1, assign_video/2,
and clear_video/1 each return {:error, :unknown_worker_session} when called with
a non-existent worker session identifier. Use the existing WorkerSessions
setup/reset pattern and the same register/list helpers to keep the new tests
consistent with the current module.
In `@test/reencodarr/broadway_producers_test.exs`:
- Around line 119-146: Add a test for the CRF claim path that asserts the same
`:video_mutated` PubSub broadcast behavior already covered for the analyzer
claim flow. Extend `test/reencodarr/broadway_producers_test.exs` near the
existing `CrfProducer.handle_demand/2` tests by invoking
`claim_next_video_for_crf_search` indirectly through `CrfProducer`, then verify
the broadcast payload alongside the state transition. Use
`VideoQueries.claim_next_video_for_crf_search/1` and the existing broadcast
assertion helpers/pattern from the analyzer coverage as the reference for
parity.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7cd47dea-01fd-4913-821a-94612a8358b9
📒 Files selected for processing (26)
.gitignore.serena/memories/conventions.md.serena/memories/core.md.serena/memories/memory_maintenance.md.serena/memories/suggested_commands.md.serena/memories/task_completion.md.serena/memories/tech_stack.mdconfig/runtime.exsdocs/ab_av1_worker_websocket.mdlib/reencodarr/ab_av1/worker_protocol.exlib/reencodarr/ab_av1/worker_sessions.exlib/reencodarr/application.exlib/reencodarr/crf_searcher/broadway/producer.exlib/reencodarr/diagnostics.exlib/reencodarr/media.exlib/reencodarr/media/video_queries.exlib/reencodarr_web/channels/worker_channel.exlib/reencodarr_web/channels/worker_socket.exlib/reencodarr_web/endpoint.exlib/reencodarr_web/live/dashboard_live.extest/reencodarr/ab_av1/worker_protocol_test.exstest/reencodarr/ab_av1/worker_sessions_test.exstest/reencodarr/broadway_producers_test.exstest/reencodarr_web/channels/worker_channel_test.exstest/reencodarr_web/live/dashboard_v2_live_test.exstest/support/channel_case.ex
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/reencodarr/ab_av1/worker_protocol.ex (1)
662-663: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve explicit
falsevalues in payload lookup.
Enum.find_value/2skipsfalse, so fields like"chosen" => falseand"retriable" => falseare parsed asnil. Use key-presence lookup instead.🐛 Proposed fix
defp fetch_any(payload, keys) do - Enum.find_value(keys, fn key -> Map.get(payload, key) end) + Enum.find_value(keys, fn key -> + if Map.has_key?(payload, key), do: {:ok, Map.fetch!(payload, key)} + end) + |> case do + {:ok, value} -> value + nil -> nil + end end🤖 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 `@lib/reencodarr/ab_av1/worker_protocol.ex` around lines 662 - 663, The payload lookup in fetch_any/2 drops explicit false values because Enum.find_value/2 treats false as absent, so fields like “chosen” and “retriable” can be parsed as nil. Update fetch_any/2 to search by key presence instead of truthiness, using the existing payload Map access logic so it returns the stored value even when it is false.lib/reencodarr/ab_av1/worker_sessions.ex (1)
145-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBroadcast after scheduled stale-session expiration.
The manual
expire_stalepath broadcasts when sessions expire, but the scheduled sweep drops expired sessions silently. Worker/session UI subscribers can keep showing expired workers until another event occurs.🐛 Proposed fix
def handle_info(:expire_stale, state) do - _ = expire_stale_sessions(timeout_seconds()) + {expired_sessions, _} = expire_stale_sessions(timeout_seconds()) + + if expired_sessions != [] do + broadcast_sessions() + end + schedule_expire_stale() {:noreply, state} end🤖 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 `@lib/reencodarr/ab_av1/worker_sessions.ex` around lines 145 - 149, The scheduled stale-session sweep in handle_info(:expire_stale, state) expires sessions silently, unlike the manual path. Update the expire_stale_sessions flow used by WorkerSessions so it returns the expired workers/sessions (or otherwise exposes what changed), then broadcast the same removal/refresh event after the scheduled sweep before rescheduling with schedule_expire_stale(). Keep the fix localized around expire_stale_sessions and handle_info(:expire_stale, state) so UI subscribers are notified whenever stale sessions are removed.lib/reencodarr_web/live/dashboard_live.ex (1)
54-55: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid keeping the raw worker token in socket assigns.
worker_tokenis stored in assigns solely to render its fingerprint. Keeping the secret in long-lived process state widens exposure (crash dumps, assigns inspection). Prefer computing the fingerprint (and aworker_token_configured?boolean) at mount and assigning only those.🤖 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 `@lib/reencodarr_web/live/dashboard_live.ex` around lines 54 - 55, The dashboard LiveView currently keeps the raw worker token in socket assigns just to derive display state. Update the mount/assignment logic in the DashboardLive module so it computes the token fingerprint and a worker_token_configured? flag up front, then stores only those derived values (plus worker_socket_url) in assigns. Remove any direct worker_token assign and make sure the render path uses the derived fingerprint/configured state instead of the secret itself.
🤖 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 `@lib/reencodarr/ab_av1/worker_protocol.ex`:
- Around line 111-114: Update the public completion result contract to include
:failed everywhere it is exposed: extend the result type in WorkerProtocol.t and
the completion_result() type alias so they match parse_completion_result/1
returning {:ok, :failed}. Make sure any related Completion.t() or
protocol/documentation references use the same union so callers and Dialyzer see
:failed as a valid completion outcome alongside the existing :ok, :cancelled,
:shutdown, and {:error, term()} cases.
---
Outside diff comments:
In `@lib/reencodarr_web/live/dashboard_live.ex`:
- Around line 54-55: The dashboard LiveView currently keeps the raw worker token
in socket assigns just to derive display state. Update the mount/assignment
logic in the DashboardLive module so it computes the token fingerprint and a
worker_token_configured? flag up front, then stores only those derived values
(plus worker_socket_url) in assigns. Remove any direct worker_token assign and
make sure the render path uses the derived fingerprint/configured state instead
of the secret itself.
In `@lib/reencodarr/ab_av1/worker_protocol.ex`:
- Around line 662-663: The payload lookup in fetch_any/2 drops explicit false
values because Enum.find_value/2 treats false as absent, so fields like “chosen”
and “retriable” can be parsed as nil. Update fetch_any/2 to search by key
presence instead of truthiness, using the existing payload Map access logic so
it returns the stored value even when it is false.
In `@lib/reencodarr/ab_av1/worker_sessions.ex`:
- Around line 145-149: The scheduled stale-session sweep in
handle_info(:expire_stale, state) expires sessions silently, unlike the manual
path. Update the expire_stale_sessions flow used by WorkerSessions so it returns
the expired workers/sessions (or otherwise exposes what changed), then broadcast
the same removal/refresh event after the scheduled sweep before rescheduling
with schedule_expire_stale(). Keep the fix localized around
expire_stale_sessions and handle_info(:expire_stale, state) so UI subscribers
are notified whenever stale sessions are removed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be54cfbb-23af-42b1-9f8b-b36e59caebca
📒 Files selected for processing (14)
lib/reencodarr/ab_av1/worker_protocol.exlib/reencodarr/ab_av1/worker_sessions.exlib/reencodarr/crf_searcher/broadway/producer.exlib/reencodarr/media/video_queries.exlib/reencodarr_web/channels/worker_channel.exlib/reencodarr_web/components/layouts/app.html.heexlib/reencodarr_web/live/dashboard_live.exlib/reencodarr_web/live/workers_live.exlib/reencodarr_web/router.extest/reencodarr/ab_av1/worker_protocol_test.exstest/reencodarr/broadway_producers_test.exstest/reencodarr_web/channels/worker_channel_test.exstest/reencodarr_web/live/dashboard_v2_live_test.exstest/reencodarr_web/live/workers_live_test.exs
💤 Files with no reviewable changes (1)
- test/reencodarr/broadway_producers_test.exs
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/reencodarr/ab_av1/worker_sessions.ex (1)
340-357: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRelease the claimed video when expiring stale sessions
lib/reencodarr/ab_av1/worker_sessions.ex:340-357drops expired sessions withoutrequeue_active_video/1, so a crashed or partitioned worker can leave its video claimed forever.cancel/1anddrain/0already requeue before dropping; stale expiry needs the same cleanup.🤖 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 `@lib/reencodarr/ab_av1/worker_sessions.ex` around lines 340 - 357, expire_stale_sessions/1 currently drops stale sessions without releasing the claimed video, unlike cancel/1 and drain/0. Update the stale-session cleanup path in WorkerSessions to call requeue_active_video/1 for each expired session before drop_session/1, using the same session/server_worker_id data already available in expire_stale_sessions/1. Ensure the expiration flow mirrors the existing cancellation/drain cleanup so claimed videos are returned before the session is removed.lib/reencodarr_web/channels/worker_channel.ex (2)
360-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate CRF-search finalization logic.
choose_result_from_report/2(Lines 360-374) andfinish_successful_crf_search/4(Lines 376-391) both mark the chosen VMAF, callMedia.mark_as_crf_searched/1, and resolve CRF-search failures. Any future change to this finalization sequence (e.g., adding a new side effect) risks being applied to only one of the two call sites.♻️ Suggested consolidation
- defp choose_result_from_report(video, results) do - chosen_crf = - Enum.find_value(results, fn result -> - if Map.get(result, :chosen, false), do: Map.get(result, :crf) - end) || Map.get(List.first(results) || %{}, :crf) - - case chosen_crf do - nil -> :ok - crf -> _ = Media.mark_vmaf_as_chosen(video.id, crf) - end - - _ = Media.mark_as_crf_searched(video) - _ = Media.resolve_crf_search_failures(video.id) - :ok - end + defp choose_result_from_report(video, results) do + chosen_crf = + Enum.find_value(results, fn result -> + if Map.get(result, :chosen, false), do: Map.get(result, :crf) + end) || Map.get(List.first(results) || %{}, :crf) + + finalize_crf_search(video, chosen_crf) + endAnd reuse the same
finalize_crf_search/2helper fromfinish_successful_crf_search/4.🤖 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 `@lib/reencodarr_web/channels/worker_channel.ex` around lines 360 - 391, There is duplicated CRF-search finalization logic in choose_result_from_report/2 and finish_successful_crf_search/4. Extract the shared post-selection steps into a single helper such as finalize_crf_search/2 that handles mark_vmaf_as_chosen, Media.mark_as_crf_searched, and Media.resolve_crf_search_failures, then call that helper from both functions; keep finish_successful_crf_search/4 responsible only for the choose-best-vmaf fallback and clear_assigned_video/2.
215-247: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWorker gets stranded if the assigned file is missing.
If
File.exists?(video.path)isfalseat Line 237, no:stream_transfer_chunkmessage is sent, soopen_transfer_stream/1and itshandle_transfer_failure/3path never run. The worker receivesjob_assignedbut notransfer_started/transfer_failedever arrives, leaving it stuck waiting indefinitely for a video the server knows doesn't exist on disk. The existingFile.openerror path inopen_transfer_stream/1(Lines 414-430) already handles this correctly (records failure, notifies client, clears assignment) — simply relying on that instead of pre-checking existence closes this gap.🐛 Proposed fix
- if File.exists?(video.path) do - send(self(), :stream_transfer_chunk) - end + send(self(), :stream_transfer_chunk)🤖 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 `@lib/reencodarr_web/channels/worker_channel.ex` around lines 215 - 247, The assign_claimed_work/3 flow in WorkerChannel leaves the worker waiting forever when video.path is missing because the File.exists? pre-check prevents open_transfer_stream/1 from running. Remove the existence gate and always trigger the transfer startup path so open_transfer_stream/1 can hit its existing File.open failure handling and handle_transfer_failure/3 logic, which will report the failure and clear the assignment. Keep the fix localized to assign_claimed_work/3 and the transfer-stream startup path so the worker always gets either transfer_started or transfer_failed.lib/reencodarr/diagnostics.ex (1)
668-699: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactor out the shared worker-status lookup.
worker_session_state/1andReencodarrWeb.WorkersLive.worker_status/1both derive the same nil/lookup/missing status, but format"idle"/"Idle"differently. Move the status derivation into one shared helper and let each caller handle display text. If this list can grow, batch theMedia.get_video/1lookups too.🤖 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 `@lib/reencodarr/diagnostics.ex` around lines 668 - 699, The worker status logic is duplicated between format_worker_sessions/1’s worker_session_state/1 and ReencodarrWeb.WorkersLive.worker_status/1, with only display casing differing. Extract the nil/lookup/missing state derivation into a shared helper that returns a normalized status, then have each caller map that result to its own display text (e.g. idle vs Idle). If worker_sessions can grow, also consider batching the Reencodarr.Media.get_video/1 lookups instead of resolving each session one by one.Source: Coding guidelines
🤖 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 `@config/runtime.exs`:
- Around line 23-28: Update parse_bool_env in runtime.exs so it no longer
defaults every unrecognized string to true; only explicitly accepted truthy
values should enable the worker websocket feature, and any typo, unexpected
casing, or unknown input should fall back to false. Mirror the stricter pattern
used by parse_check_origin by making the function enumerate the allowed values
and keep the behavior safe by default.
In `@lib/reencodarr_web/live/dashboard_live.ex`:
- Around line 1053-1075: The dashboard is rendering the full worker token in the
Token section, which exposes a live credential in the HTML. Update the view
logic in DashboardLive to stop displaying the raw token by default, keep showing
only the fingerprint from `@worker_token_state`, and add an explicit reveal/copy
interaction for the token instead of embedding it in the page.
In `@lib/reencodarr_web/live/workers_live.ex`:
- Around line 126-133: The worker status rendering is doing one
Media.get_video/1 lookup per worker, which creates repeated DB hits and
duplicates the same pattern used in
Reencodarr.Diagnostics.worker_session_state/1. Refactor worker_status/1 to use a
shared helper that batches active video state lookups for all workers in one
query, then have both WorkersLive and Diagnostics consume that preloaded map
instead of calling Media.get_video/1 per worker.
In `@lib/reencodarr/ab_av1/worker_sessions.ex`:
- Around line 138-148: The progress setters in WorkerSessions are causing
redundant full-session PubSub broadcasts by flowing through update_session_reply
and put_session/1, which always calls broadcast_sessions(). Update
set_transfer_progress and set_crf_search_progress to avoid triggering the full
session list broadcast for these progress-only changes, using a non-broadcasting
session update path while keeping the existing WorkerChannel progress events for
transfer/crf search updates.
- Around line 20-21: Add missing type specs to the new public API in
WorkerSessions: `set_transfer_progress/2`, `set_crf_search_progress/2`,
`cancel/1`, and `drain/0` should all have explicit `@spec` declarations matching
their actual inputs and return values. Also tighten the `session` type by
replacing the generic `map() | nil` fields for `transfer_progress` and
`crf_search_progress` with `WorkerProtocol.TransferProgress.t() | nil` and
`WorkerProtocol.CrfSearchProgress.t() | nil` so the contract matches the new
protocol types.
- Around line 307-318: The requeue_active_video/1 flow in WorkerSessions
currently ignores failures from Reencodarr.Media.mark_as_analyzed/1, which can
leave the video in the wrong state while still returning :ok. Update the
%Reencodarr.Media.Video{} branch to explicitly handle the return value from
mark_as_analyzed/1, branching on {:error, reason} to log or surface the failure
instead of discarding it, and keep the nil case unchanged.
In `@test/reencodarr/ab_av1/worker_sessions_test.exs`:
- Around line 79-104: The cancel/drain test is using videos already in :analyzed
state, so it never verifies the requeue path. Update the setup in
WorkerSessions.cancel/1 and WorkerSessions.drain/0 coverage to create or
transition the fixtures into an active claimed state such as :crf_searching
before assignment, then keep the assertions on Media.get_video/1 so the test
actually proves requeue_active_video and mark_as_analyzed are invoked.
---
Outside diff comments:
In `@lib/reencodarr_web/channels/worker_channel.ex`:
- Around line 360-391: There is duplicated CRF-search finalization logic in
choose_result_from_report/2 and finish_successful_crf_search/4. Extract the
shared post-selection steps into a single helper such as finalize_crf_search/2
that handles mark_vmaf_as_chosen, Media.mark_as_crf_searched, and
Media.resolve_crf_search_failures, then call that helper from both functions;
keep finish_successful_crf_search/4 responsible only for the choose-best-vmaf
fallback and clear_assigned_video/2.
- Around line 215-247: The assign_claimed_work/3 flow in WorkerChannel leaves
the worker waiting forever when video.path is missing because the File.exists?
pre-check prevents open_transfer_stream/1 from running. Remove the existence
gate and always trigger the transfer startup path so open_transfer_stream/1 can
hit its existing File.open failure handling and handle_transfer_failure/3 logic,
which will report the failure and clear the assignment. Keep the fix localized
to assign_claimed_work/3 and the transfer-stream startup path so the worker
always gets either transfer_started or transfer_failed.
In `@lib/reencodarr/ab_av1/worker_sessions.ex`:
- Around line 340-357: expire_stale_sessions/1 currently drops stale sessions
without releasing the claimed video, unlike cancel/1 and drain/0. Update the
stale-session cleanup path in WorkerSessions to call requeue_active_video/1 for
each expired session before drop_session/1, using the same
session/server_worker_id data already available in expire_stale_sessions/1.
Ensure the expiration flow mirrors the existing cancellation/drain cleanup so
claimed videos are returned before the session is removed.
In `@lib/reencodarr/diagnostics.ex`:
- Around line 668-699: The worker status logic is duplicated between
format_worker_sessions/1’s worker_session_state/1 and
ReencodarrWeb.WorkersLive.worker_status/1, with only display casing differing.
Extract the nil/lookup/missing state derivation into a shared helper that
returns a normalized status, then have each caller map that result to its own
display text (e.g. idle vs Idle). If worker_sessions can grow, also consider
batching the Reencodarr.Media.get_video/1 lookups instead of resolving each
session one by one.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b870b971-eef4-4a6b-b848-f9977fbdf133
📒 Files selected for processing (17)
config/config.exsconfig/runtime.exsdocs/distributed_worker_protocol.mdlib/reencodarr/ab_av1/worker_config.exlib/reencodarr/ab_av1/worker_protocol.exlib/reencodarr/ab_av1/worker_sessions.exlib/reencodarr/diagnostics.exlib/reencodarr_web/channels/worker_channel.exlib/reencodarr_web/channels/worker_socket.exlib/reencodarr_web/live/dashboard_live.exlib/reencodarr_web/live/workers_live.extest/reencodarr/ab_av1/worker_config_test.exstest/reencodarr/ab_av1/worker_protocol_test.exstest/reencodarr/ab_av1/worker_sessions_test.exstest/reencodarr_web/channels/worker_channel_test.exstest/reencodarr_web/live/dashboard_v2_live_test.exstest/reencodarr_web/live/workers_live_test.exs
Comment on lines
+23
to
+28
| parse_bool_env = fn | ||
| "false" -> false | ||
| "0" -> false | ||
| "no" -> false | ||
| _ -> true | ||
| end |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
parse_bool_env fails open on unrecognized values.
Only "false", "0", "no" are treated as false; any other string (including typos like "flase" or unexpected casing) is treated as true. Since this gates whether the worker websocket feature is enabled at all, a misconfigured env var silently turns it on. Contrast with parse_check_origin below, which explicitly enumerates "true"/"false".
🛡️ Proposed fix — default to false for unrecognized values
parse_bool_env = fn
- "false" -> false
- "0" -> false
- "no" -> false
- _ -> true
+ "true" -> true
+ "1" -> true
+ "yes" -> true
+ _ -> false
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parse_bool_env = fn | |
| "false" -> false | |
| "0" -> false | |
| "no" -> false | |
| _ -> true | |
| end | |
| parse_bool_env = fn | |
| "true" -> true | |
| "1" -> true | |
| "yes" -> true | |
| _ -> false | |
| end |
🤖 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 `@config/runtime.exs` around lines 23 - 28, Update parse_bool_env in
runtime.exs so it no longer defaults every unrecognized string to true; only
explicitly accepted truthy values should enable the worker websocket feature,
and any typo, unexpected casing, or unknown input should fall back to false.
Mirror the stricter pattern used by parse_check_origin by making the function
enumerate the allowed values and keep the behavior safe by default.
Comment on lines
+126
to
+133
| defp worker_status(%{active_video_id: nil}), do: "Idle" | ||
|
|
||
| defp worker_status(%{active_video_id: video_id}) do | ||
| case Media.get_video(video_id) do | ||
| %Media.Video{state: state} -> Atom.to_string(state) | ||
| nil -> "missing" | ||
| end | ||
| end |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how often worker session broadcasts fire (e.g., per progress update vs per state change)
rg -n 'broadcast_sessions' lib/reencodarr/ab_av1/worker_sessions.ex -B5 -A2
rg -n 'broadcast_sessions\(' lib/reencodarr -nRepository: mjc/reencodarr
Length of output: 2615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first, then inspect the small slices around the worker status logic.
ast-grep outline lib/reencodarr_web/live/workers_live.ex --view expanded || true
ast-grep outline lib/reencodarr/diagnostics.ex --view expanded || true
printf '\n--- workers_live.ex ---\n'
sed -n '1,220p' lib/reencodarr_web/live/workers_live.ex | cat -n
printf '\n--- diagnostics.ex ---\n'
sed -n '1,220p' lib/reencodarr/diagnostics.ex | cat -n
printf '\n--- search for worker_status helpers ---\n'
rg -n 'worker_status|worker_session_state|get_video\(' lib/reencodarr_web/live/workers_live.ex lib/reencodarr/diagnostics.ex lib/reencodarr -nRepository: mjc/reencodarr
Length of output: 17025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- diagnostics helper around worker_session_state ---\n'
sed -n '650,700p' lib/reencodarr/diagnostics.ex | cat -n
printf '\n--- worker_sessions update/broadcast paths ---\n'
sed -n '1,360p' lib/reencodarr/ab_av1/worker_sessions.ex | cat -n | sed -n '1,360p'
printf '\n--- focused search for state/progress update calls that broadcast ---\n'
rg -n 'broadcast_sessions\(\)|set_state|progress|update_session|touch_session|active_video_id' lib/reencodarr/ab_av1/worker_sessions.ex lib/reencodarr -nRepository: mjc/reencodarr
Length of output: 36940
Batch worker status lookups. worker_status/1 does one Media.get_video/1 per worker on every 5s refresh and on each worker-session update, so the render path grows linearly with connected workers. The same lookup pattern also exists in Reencodarr.Diagnostics.worker_session_state/1; a shared helper that preloads all active video states in one query would remove the duplication and the per-worker DB hit.
🤖 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 `@lib/reencodarr_web/live/workers_live.ex` around lines 126 - 133, The worker
status rendering is doing one Media.get_video/1 lookup per worker, which creates
repeated DB hits and duplicates the same pattern used in
Reencodarr.Diagnostics.worker_session_state/1. Refactor worker_status/1 to use a
shared helper that batches active video state lookups for all workers in one
query, then have both WorkersLive and Diagnostics consume that preloaded map
instead of calling Media.get_video/1 per worker.
Comment on lines
+20
to
+21
| transfer_progress: map() | nil, | ||
| crf_search_progress: map() | nil, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing @spec on new public API; consider typed progress fields.
set_transfer_progress/2, set_crf_search_progress/2, cancel/1, and drain/0 have no type specs, unlike the sibling WorkerConfig/WorkerProtocol modules added in this PR. The PR comments explicitly ask for more type specs. Also, transfer_progress/crf_search_progress in the session type (lines 20-21) are typed as generic map() | nil — using WorkerProtocol.TransferProgress.t() | nil and WorkerProtocol.CrfSearchProgress.t() | nil would make the contract precise.
Also applies to: 50-65
🤖 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 `@lib/reencodarr/ab_av1/worker_sessions.ex` around lines 20 - 21, Add missing
type specs to the new public API in WorkerSessions: `set_transfer_progress/2`,
`set_crf_search_progress/2`, `cancel/1`, and `drain/0` should all have explicit
`@spec` declarations matching their actual inputs and return values. Also
tighten the `session` type by replacing the generic `map() | nil` fields for
`transfer_progress` and `crf_search_progress` with
`WorkerProtocol.TransferProgress.t() | nil` and
`WorkerProtocol.CrfSearchProgress.t() | nil` so the contract matches the new
protocol types.
Comment on lines
+138
to
+148
| def handle_call({:set_transfer_progress, server_worker_id, progress}, _from, state) do | ||
| update_session_reply(server_worker_id, state, fn session -> | ||
| %{session | transfer_progress: progress} | ||
| end) | ||
| end | ||
|
|
||
| def handle_call({:set_crf_search_progress, server_worker_id, progress}, _from, state) do | ||
| update_session_reply(server_worker_id, state, fn session -> | ||
| %{session | crf_search_progress: progress} | ||
| end) | ||
| end |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Every progress update broadcasts the full session list.
set_transfer_progress/set_crf_search_progress go through update_session_reply → put_session/1, which unconditionally calls broadcast_sessions() (line 324). Transfer/CRF-search progress can update frequently (per chunk / per tick), so each tick re-broadcasts the entire sorted session list over PubSub to every dashboard/workers LiveView, triggering a full assign(socket, :workers, sessions) re-render each time (see workers_live.ex handle_info({:worker_sessions_updated, ...})). This looks redundant with the dedicated :transfer_progress/:crf_search_progress events WorkerChannel already broadcasts separately for the same data.
Consider skipping the broadcast_sessions() call for progress-only updates (e.g., a variant of put_session that doesn't broadcast), relying on the existing per-event progress broadcasts instead.
Also applies to: 320-326
🤖 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 `@lib/reencodarr/ab_av1/worker_sessions.ex` around lines 138 - 148, The
progress setters in WorkerSessions are causing redundant full-session PubSub
broadcasts by flowing through update_session_reply and put_session/1, which
always calls broadcast_sessions(). Update set_transfer_progress and
set_crf_search_progress to avoid triggering the full session list broadcast for
these progress-only changes, using a non-broadcasting session update path while
keeping the existing WorkerChannel progress events for transfer/crf search
updates.
Comment on lines
+307
to
+318
| defp requeue_active_video(%{active_video_id: nil}), do: :ok | ||
|
|
||
| defp requeue_active_video(%{active_video_id: video_id}) do | ||
| case Reencodarr.Media.get_video(video_id) do | ||
| %Reencodarr.Media.Video{} = video -> | ||
| _ = Reencodarr.Media.mark_as_analyzed(video) | ||
| :ok | ||
|
|
||
| nil -> | ||
| :ok | ||
| end | ||
| end |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mark_as_analyzed delegates to VideoStateMachine and check its return contract
ast-grep run --pattern 'def mark_as_analyzed($_) do $$$ end' --lang elixir lib/reencodarr/media.ex
rg -n 'def mark_as_analyzed' -A 15 lib/reencodarr/media.exRepository: mjc/reencodarr
Length of output: 793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the state machine implementation and relevant callers.
fd -a 'video_state_machine.ex' lib
fd -a 'video_state_machine*.ex' lib
rg -n 'def mark_as_analyzed|mark_as_analyzed\(' lib/reencodarr -A 6 -B 4Repository: mjc/reencodarr
Length of output: 6224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the state transition implementation and any cleanup paths that handle errors.
sed -n '1,260p' lib/reencodarr/media/video_state_machine.ex
printf '\n---\n'
sed -n '260,460p' lib/reencodarr/media/video_state_machine.ex
printf '\n---\n'
sed -n '168,190p' lib/reencodarr/ab_av1/crf_search.ex
printf '\n---\n'
sed -n '890,905p' lib/reencodarr/ab_av1/crf_search.exRepository: mjc/reencodarr
Length of output: 17474
Handle Media.mark_as_analyzed/1 failures in lib/reencodarr/ab_av1/worker_sessions.ex:309-313. requeue_active_video/1 ignores the {:error, reason} return, so cancel/drain can drop the session while the video remains in its previous state. Log or branch on the error instead of always returning :ok.
🤖 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 `@lib/reencodarr/ab_av1/worker_sessions.ex` around lines 307 - 318, The
requeue_active_video/1 flow in WorkerSessions currently ignores failures from
Reencodarr.Media.mark_as_analyzed/1, which can leave the video in the wrong
state while still returning :ok. Update the %Reencodarr.Media.Video{} branch to
explicitly handle the return value from mark_as_analyzed/1, branching on
{:error, reason} to log or surface the failure instead of discarding it, and
keep the nil case unchanged.
Source: Path instructions
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 (4)
docs/distributed_worker_protocol.md (1)
18-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the work-request event names consistent.
pull_work/request_workare introduced here, but neither appears in the later "Worker Events" list. That makes the protocol contract ambiguous for client authors.🤖 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 `@docs/distributed_worker_protocol.md` around lines 18 - 19, The work-request event names are inconsistent between the protocol steps and the “Worker Events” list, which makes the contract ambiguous. Update the distributed worker protocol documentation so the same request event name(s) used in the “Client requests work” section are also defined in the “Worker Events” list, and ensure the later event list explicitly includes the chosen symbol(s) such as pull_work and request_work for easy reference.test/reencodarr_web/channels/worker_channel_test.exs (2)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
announce_payload/1helper for consistency.This test hardcodes the announce map inline while every other test uses
announce_payload/1(defined at Line 566). Using the helper here keeps the default payload in one place and avoids drift if the default shape changes.♻️ Suggested diff
- assert_reply push(socket, "announce", %{ - "worker_id" => "abav1-dev", - "protocol_version" => 1, - "version" => "0.10.0", - "capabilities" => %{"crf_search" => true} - }), + assert_reply push(socket, "announce", announce_payload()), :ok, %{accepted: true, protocol_version: 1}🤖 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 `@test/reencodarr_web/channels/worker_channel_test.exs` around lines 42 - 49, The announce request in the worker channel test is hardcoded inline instead of using the shared announce_payload/1 helper, which can drift from the default payload used elsewhere. Update the assertion in the worker_channel_test case to build the payload through announce_payload/1, then override any test-specific fields if needed, so the test stays aligned with the helper defined in the same module.
30-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated connect/join/announce boilerplate across tests.
The
connect(WorkerSocket, %{"token" => token})→subscribe_and_join(socket, "workers:crf_search")→announcepush sequence is duplicated near-verbatim across ~10 tests. Extracting a private helper (e.g.connect_and_announce/1, returning the joined+announced socket) would reduce duplication and make each test body focus on its actual assertions.♻️ Example helper extraction
defp connect_and_announce(overrides \\ []) do token = "test-worker-token" Application.put_env(:reencodarr, :worker_token, token) {:ok, socket} = connect(WorkerSocket, %{"token" => token}) {:ok, _join_payload, socket} = subscribe_and_join(socket, "workers:crf_search") assert_reply push(socket, "announce", announce_payload(overrides)), :ok, %{accepted: true, protocol_version: 1} socket end🤖 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 `@test/reencodarr_web/channels/worker_channel_test.exs` around lines 30 - 524, The worker channel tests repeat the same connect, join, and announce setup in many cases, so extract that flow into a shared private helper in WorkerChannelTest. Create a helper such as connect_and_announce/1 that handles setting the worker token, calling connect/2 on WorkerSocket, subscribe_and_join/2 for "workers:crf_search", and the announce push via announce_payload/1, then return the ready socket for each test to use. Update the existing tests to call the helper and keep only the scenario-specific assertions so the suite is shorter and easier to maintain.test/reencodarr/ab_av1/worker_protocol_test.exs (1)
214-227: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGlobal env mutation under
async: truecan make this test flaky.
Application.put_env/3changes a node-wide setting, so this case can race with any other async test that reads:worker_chunk_size_bytes. Mark this moduleasync: falseor move the env-mutating case into a dedicated non-async test module.🤖 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 `@test/reencodarr/ab_av1/worker_protocol_test.exs` around lines 214 - 227, The test in WorkerProtocol mutates a global Application env value with Application.put_env/3, which can race with other async tests. Move this env-changing case out of the async test module by either marking the worker_protocol_test module async: false or placing the chunk_size_bytes test into a dedicated non-async module, and keep the existing cleanup around :worker_chunk_size_bytes in the test body.
🤖 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 `@docs/distributed_worker_protocol.md`:
- Around line 18-19: The work-request event names are inconsistent between the
protocol steps and the “Worker Events” list, which makes the contract ambiguous.
Update the distributed worker protocol documentation so the same request event
name(s) used in the “Client requests work” section are also defined in the
“Worker Events” list, and ensure the later event list explicitly includes the
chosen symbol(s) such as pull_work and request_work for easy reference.
In `@test/reencodarr_web/channels/worker_channel_test.exs`:
- Around line 42-49: The announce request in the worker channel test is
hardcoded inline instead of using the shared announce_payload/1 helper, which
can drift from the default payload used elsewhere. Update the assertion in the
worker_channel_test case to build the payload through announce_payload/1, then
override any test-specific fields if needed, so the test stays aligned with the
helper defined in the same module.
- Around line 30-524: The worker channel tests repeat the same connect, join,
and announce setup in many cases, so extract that flow into a shared private
helper in WorkerChannelTest. Create a helper such as connect_and_announce/1 that
handles setting the worker token, calling connect/2 on WorkerSocket,
subscribe_and_join/2 for "workers:crf_search", and the announce push via
announce_payload/1, then return the ready socket for each test to use. Update
the existing tests to call the helper and keep only the scenario-specific
assertions so the suite is shorter and easier to maintain.
In `@test/reencodarr/ab_av1/worker_protocol_test.exs`:
- Around line 214-227: The test in WorkerProtocol mutates a global Application
env value with Application.put_env/3, which can race with other async tests.
Move this env-changing case out of the async test module by either marking the
worker_protocol_test module async: false or placing the chunk_size_bytes test
into a dedicated non-async module, and keep the existing cleanup around
:worker_chunk_size_bytes in the test body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5407a3ae-29c7-4e16-b535-957c0573a60a
📒 Files selected for processing (7)
config/config.exsconfig/runtime.exsdocs/distributed_worker_protocol.mdlib/reencodarr/ab_av1/worker_config.exlib/reencodarr/ab_av1/worker_protocol.extest/reencodarr/ab_av1/worker_protocol_test.exstest/reencodarr_web/channels/worker_channel_test.exs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the ab-av1 worker websocket path on the Phoenix endpoint with token auth, protocol validation, session tracking, and job assignment replies.
Also switches local CRF Broadway dispatch to the same atomic claim path so remote websocket workers and local Broadway cannot claim the same analyzed video twice.
Verification:
Summary by CodeRabbit