Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a global in-process database writer to serialize SQLite write operations at runtime, reducing lock contention and simplifying previously write-serialization logic in webhook processing.
Changes:
- Add
Reencodarr.DbWriter(GenServer) to serialize runtime SQLite mutations and apply retry behavior centrally. - Route multiple write-heavy code paths (Media, VideoUpsert, VideoStateMachine, Services, VideoFailure, webhooks) through
DbWriter. - Adjust SQLite/pool tuning (smaller repo pool, reduced busy timeout) and add focused DbWriter tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/reencodarr/db_writer.ex |
New GenServer-based writer that serializes write execution and wraps writes with retry logic. |
test/reencodarr/db_writer_test.exs |
Adds tests covering inline behavior in test env, nesting, enqueue, and transaction semantics. |
lib/reencodarr_web/webhook_processor.ex |
Simplifies webhook processing to enqueue work onto the global DbWriter. |
lib/reencodarr/media.ex |
Routes many write paths through DbWriter and adds labeled wrappers for writes/transactions. |
lib/reencodarr/media/video_upsert.ex |
Wraps upsert/batch_upsert in DbWriter and removes per-transaction retry helper usage. |
lib/reencodarr/media/video_state_machine.ex |
Routes Repo updates through DbWriter instead of direct retry helper. |
lib/reencodarr/media/video_queries.ex |
Wraps “claim for analysis” state transition in DbWriter transaction. |
lib/reencodarr/media/video_failure.ex |
Routes failure insert/update through DbWriter. |
lib/reencodarr/services.ex |
Routes config create/update/delete through DbWriter. |
lib/reencodarr/sync.ex |
Removes explicit Repo transactions around some upsert flows (now handled elsewhere). |
lib/reencodarr/core/retry.ex |
Tweaks transient SQLite retry log message format. |
lib/reencodarr/application.ex |
Adds Reencodarr.DbWriter to supervision tree. |
config/config.exs |
Reduces SQLite busy timeout default now that writes are serialized. |
config/dev.exs / config/runtime.exs |
Reduces repo pool size defaults to reflect serialized writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
lib/reencodarr_web/webhook_processor.ex:12
WebhookProcessoris still started under supervision as aGenServer, but it no longer queues/handles any messages (it just delegates toDbWriter.enqueue/2). This leaves an extra always-idle process and can be misleading to future maintainers. Consider converting this to a plain module (or keep the process only if you need it for compatibility/health checks) and update the supervision tree accordingly.
use GenServer
alias Reencodarr.DbWriter
def start_link(opts) do
GenServer.start_link(__MODULE__, opts, name: __MODULE__)
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0215401 to
1a4b86e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lib/reencodarr_web/webhook_processor.ex:12
WebhookProcessorno longer handles any GenServer messages (it just forwards toDbWriter.enqueue/2), but it is still started as a supervised GenServer and keeps an idle process around. Consider converting this to a plain module (removeuse GenServer,start_link/1, and the child entry) or, if it must remain a child for compatibility, document why it needs to run and/or add minimal callbacks to justify the process.
use GenServer
alias Reencodarr.DbWriter
def start_link(opts) do
GenServer.start_link(__MODULE__, opts, name: __MODULE__)
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test "run uses writer_timeout without consuming Repo timeout options" do | ||
| previous_env = Application.get_env(:reencodarr, :env) | ||
| Application.put_env(:reencodarr, :env, :dev) | ||
| on_exit(fn -> Application.put_env(:reencodarr, :env, previous_env) end) | ||
| test_pid = self() |
There was a problem hiding this comment.
Application.put_env(:reencodarr, :env, :dev) mutates global application config and can leak into other tests running concurrently (this repo has many async: true tests). This can make the suite flaky. Prefer a test-only switch that forces non-inline mode without touching global env (e.g., an option like DbWriter.run(..., inline?: false) / mode: :writer) or isolate the behavior behind dedicated test config instead of runtime mutation.
| test "enqueue logs async failures with the writer label" do | ||
| previous_env = Application.get_env(:reencodarr, :env) | ||
| Application.put_env(:reencodarr, :env, :dev) | ||
| on_exit(fn -> Application.put_env(:reencodarr, :env, previous_env) end) |
There was a problem hiding this comment.
This test also changes :reencodarr, :env globally. Even with async: false for this module, ExUnit can still run other async tests in parallel, so this can cause cross-test interference. Consider avoiding Application.put_env/3 here by injecting a writer/inline mode override into DbWriter.enqueue/2 for tests.
Summary
Validation