Replace Cowboy with Bandit - #5090
Open
IvanIvanoff wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces Cowboy/Cowlib/PlugCowboy with Bandit: endpoint configs switch to Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application / Supervisor
participant Endpoint as Phoenix Endpoint
participant Bandit as Bandit / ThousandIsland
participant Client as HTTP Client
App->>Endpoint: stop endpoint / initiate shutdown
Endpoint->>Bandit: close listening socket (suspend new accept)
Note right of Bandit: in-flight requests continue\nuntil finished or shutdown_timeout
Client->>Bandit: in-flight request (started before suspend)
Bandit-->>Client: response if completed before timeout
Client->>Bandit: new request (after suspend)
Bandit-->>Client: connection refused / reset
par wait for in-flight or timeout
Bandit->>Bandit: track active connection pids
end
Bandit->>App: shutdown completed (or forced after timeout)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/dev.exs (1)
14-20:⚠️ Potential issue | 🟠 MajorMove response compression configuration to
http_options.With
Bandit.PhoenixAdapter, the entries insidehttp:are passed directly to Bandit. Response compression must be configured underhttp_options, not at the top level of thehttp:key, ascompressis not a recognized top-level option inBandit.options/0.Suggested fix
config :sanbase, SanbaseWeb.Endpoint, http: [ - compress: true, + http_options: [compress: true], port: port, thousand_island_options: [ read_timeout: 100_000 ] ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/dev.exs` around lines 14 - 20, Move the response compression option out of the top-level http map and into http_options for the SanbaseWeb.Endpoint config: remove compress: true from inside the http: map and add it under http_options (e.g., http_options: [compress: true, thousand_island_options: [...]]), keeping the existing thousand_island_options intact; update the SanbaseWeb.Endpoint config so Bandit receives compress via http_options rather than as a top-level http key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sanbase_web/connection_drainer.ex`:
- Around line 37-57: The current case branch assumes ThousandIsland.suspend/1
and ThousandIsland.connection_pids/1 always return {:ok, ...} and will crash on
:error; update the {:ok, server_pid} branch in the safe_bandit_pid handling to
use a with that calls ThousandIsland.suspend(server_pid) and then
ThousandIsland.connection_pids(server_pid), handling {:ok, _} and matching
:error by logging/skipping draining instead of hard-matching; on success keep
the IO.puts messages and call wait_for_drain(server_pid), but if either call
returns :error, log the failure and do not call wait_for_drain.
In `@test/sanbase_web/connection_drainer_test.exs`:
- Around line 13-20: Replace the sleep rendezvous in SlowPlug.call/2 with an
explicit test-process handshake: have SlowPlug.call/2 send a message like
{:slow_plug_started, self()} (or {:slow_plug_started, test_pid} if you pass test
PID via conn private assigns) to the test process as soon as handling begins,
then block on a receive to continue; in the test (connection_drainer_test), wait
for that {:slow_plug_started, _} message after triggering the request (instead
of relying on Process.sleep/1_000) so the drainer is guaranteed to start
draining only after the plug has actually begun handling the request—use symbols
SlowPlug.call/2, request_started, and Req.get!/1 to locate where to add the
send/receive handshake.
- Around line 7-21: Move the nested SlowPlug module out of the test file into
its own top-level support file: create a new module SlowPlug with the same
functions init/1 and call/2 (keeping the Process.sleep,
put_resp_header/put_resp_content_type/send_resp behavior) and `@moduledoc` false,
remove the nested module from connection_drainer_test.exs, and update the test
to reference the standalone SlowPlug (no alias needed if named the same); ensure
the new support file is picked up by the test suite (compiled by test farm) so
tests continue to run.
---
Outside diff comments:
In `@config/dev.exs`:
- Around line 14-20: Move the response compression option out of the top-level
http map and into http_options for the SanbaseWeb.Endpoint config: remove
compress: true from inside the http: map and add it under http_options (e.g.,
http_options: [compress: true, thousand_island_options: [...]]), keeping the
existing thousand_island_options intact; update the SanbaseWeb.Endpoint config
so Bandit receives compress via http_options rather than as a top-level http
key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 420a168e-55c9-40d5-929d-870144bc2d4c
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
config/config.exsconfig/dev.exsconfig/runtime.exslib/sanbase/application/application.exlib/sanbase_web/connection_drainer.exmix.exstest/sanbase_web/connection_drainer_test.exs
Comment on lines
+37
to
+57
| case safe_bandit_pid(endpoint) do | ||
| {:ok, server_pid} -> | ||
| # Stop accepting new connections | ||
| :ok = ThousandIsland.suspend(server_pid) | ||
|
|
||
| {:ok, running_connections} = ThousandIsland.connection_pids(server_pid) | ||
|
|
||
| IO.puts( | ||
| "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Stopped accepting new connections. Waiting for #{length(running_connections)} connections to finish." | ||
| ) | ||
|
|
||
| # Wait until the connections are all finished. | ||
| # If it takes more time, the `:shutdown` timeout will kick in | ||
| # and kill this process. This way we have a balance between | ||
| # waiting for most connections to finish, but not waiting too long | ||
| # or getting stuck. | ||
| wait_for_drain(server_pid) | ||
|
|
||
| IO.puts( | ||
| "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Finished draining connections." | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the documented return values of ThousandIsland.suspend/1andThousandIsland.connection_pids/1, and should callers handle :error?
💡 Result:
ThousandIsland.suspend/1 returns :ok on success or :error if the server is not in a usable state (e.g., if the listener PID is unavailable). ThousandIsland.connection_pids/1 returns {:ok, [pid]} listing active connection processes (with a note that the list may be inconsistent due to concurrency) or :error if the acceptor pool supervisor PID is unavailable. Callers should handle :error in both cases, as it indicates the server may not be running properly or the requested component is missing.
Citations:
- 1: https://hexdocs.pm/thousand_island/ThousandIsland.html
- 2: https://github.com/mtrudel/thousand_island/blob/main/lib/thousand_island.ex
Use with to handle potential :error returns from ThousandIsland.suspend/1 and ThousandIsland.connection_pids/1.
Both functions can return :error if the server is unavailable. Hard-matching these with pattern matching would crash the shutdown callback. Use with to chain operations and gracefully skip draining if either call fails.
Suggested fix
case safe_bandit_pid(endpoint) do
{:ok, server_pid} ->
- # Stop accepting new connections
- :ok = ThousandIsland.suspend(server_pid)
-
- {:ok, running_connections} = ThousandIsland.connection_pids(server_pid)
-
- IO.puts(
- "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Stopped accepting new connections. Waiting for #{length(running_connections)} connections to finish."
- )
-
- # Wait until the connections are all finished.
- # If it takes more time, the `:shutdown` timeout will kick in
- # and kill this process. This way we have a balance between
- # waiting for most connections to finish, but not waiting too long
- # or getting stuck.
- wait_for_drain(server_pid)
-
- IO.puts(
- "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Finished draining connections."
- )
+ with :ok <- ThousandIsland.suspend(server_pid),
+ {:ok, running_connections} <- ThousandIsland.connection_pids(server_pid) do
+ IO.puts(
+ "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Stopped accepting new connections. Waiting for #{length(running_connections)} connections to finish."
+ )
+
+ wait_for_drain(server_pid)
+
+ IO.puts(
+ "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Finished draining connections."
+ )
+ else
+ :error ->
+ IO.puts(
+ "[#{DateTime.utc_now(:second)}][ConnectionDrainer] No HTTP server found, nothing to drain."
+ )
+ end
_ ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase_web/connection_drainer.ex` around lines 37 - 57, The current case
branch assumes ThousandIsland.suspend/1 and ThousandIsland.connection_pids/1
always return {:ok, ...} and will crash on :error; update the {:ok, server_pid}
branch in the safe_bandit_pid handling to use a with that calls
ThousandIsland.suspend(server_pid) and then
ThousandIsland.connection_pids(server_pid), handling {:ok, _} and matching
:error by logging/skipping draining instead of hard-matching; on success keep
the IO.puts messages and call wait_for_drain(server_pid), but if either call
returns :error, log the failure and do not call wait_for_drain.
Comment on lines
+7
to
+21
| defmodule SlowPlug do | ||
| @moduledoc false | ||
| import Plug.Conn | ||
|
|
||
| def init(opts), do: opts | ||
|
|
||
| def call(conn, _opts) do | ||
| Process.sleep(1_000) | ||
|
|
||
| conn | ||
| |> put_resp_header("connection", "close") | ||
| |> put_resp_content_type("text/plain") | ||
| |> send_resp(200, "ok") | ||
| end | ||
| end |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move SlowPlug into a separate support file.
Keeping a second module nested inside this test file violates the repo rule against multiple modules per file and makes the helper harder to reuse from other shutdown cases.
As per coding guidelines, "Never nest multiple modules in the same file as it can cause cyclic dependencies and compilation errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanbase_web/connection_drainer_test.exs` around lines 7 - 21, Move the
nested SlowPlug module out of the test file into its own top-level support file:
create a new module SlowPlug with the same functions init/1 and call/2 (keeping
the Process.sleep, put_resp_header/put_resp_content_type/send_resp behavior) and
`@moduledoc` false, remove the nested module from connection_drainer_test.exs, and
update the test to reference the standalone SlowPlug (no alias needed if named
the same); ensure the new support file is picked up by the test suite (compiled
by test farm) so tests continue to run.
Comment on lines
+13
to
+20
| def call(conn, _opts) do | ||
| Process.sleep(1_000) | ||
|
|
||
| conn | ||
| |> put_resp_header("connection", "close") | ||
| |> put_resp_content_type("text/plain") | ||
| |> send_resp(200, "ok") | ||
| end |
Contributor
There was a problem hiding this comment.
Replace the sleep-based rendezvous with a real “request entered the plug” signal.
request_started is sent before Req.get!/1, so Line 66 is the only thing making this request “in-flight”. On a slower runner the drainer can still suspend the listener before the request reaches SlowPlug, which makes the test flaky. Have SlowPlug.call/2 notify the test process when handling starts and wait on that message instead.
Also applies to: 51-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanbase_web/connection_drainer_test.exs` around lines 13 - 20, Replace
the sleep rendezvous in SlowPlug.call/2 with an explicit test-process handshake:
have SlowPlug.call/2 send a message like {:slow_plug_started, self()} (or
{:slow_plug_started, test_pid} if you pass test PID via conn private assigns) to
the test process as soon as handling begins, then block on a receive to
continue; in the test (connection_drainer_test), wait for that
{:slow_plug_started, _} message after triggering the request (instead of relying
on Process.sleep/1_000) so the drainer is guaranteed to start draining only
after the plug has actually begun handling the request—use symbols
SlowPlug.call/2, request_started, and Req.get!/1 to locate where to add the
send/receive handshake.
IvanIvanoff
force-pushed
the
migrate-bandit
branch
from
April 7, 2026 12:07
de232d6 to
eb7d3a6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/sanbase_web/connection_drainer.ex (1)
38-64:⚠️ Potential issue | 🟠 MajorUse
withto handle potential:errorreturns from ThousandIsland APIs.The hard pattern matches on Lines 41 and 43 (
":ok = ThousandIsland.suspend(server_pid)"and"{:ok, running_connections} = ThousandIsland.connection_pids(server_pid)") will crash if ThousandIsland returns:errorwhen the server is unavailable. Usewithfor graceful handling:Suggested fix using `with`
case safe_bandit_pid(endpoint) do {:ok, server_pid} -> - # Stop accepting new connections - :ok = ThousandIsland.suspend(server_pid) - - {:ok, running_connections} = ThousandIsland.connection_pids(server_pid) - - IO.puts( - "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Stopped accepting new connections. Waiting for #{length(running_connections)} connections to finish." - ) - - # Wait until the connections are all finished. - # If it takes more time, the `:shutdown` timeout will kick in - # and kill this process. This way we have a balance between - # waiting for most connections to finish, but not waiting too long - # or getting stuck. - wait_for_drain(server_pid) - - IO.puts( - "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Finished draining connections." - ) + with :ok <- ThousandIsland.suspend(server_pid), + {:ok, running_connections} <- ThousandIsland.connection_pids(server_pid) do + IO.puts( + "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Stopped accepting new connections. Waiting for #{length(running_connections)} connections to finish." + ) + + wait_for_drain(server_pid) + + IO.puts( + "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Finished draining connections." + ) + else + :error -> + IO.puts( + "[#{DateTime.utc_now(:second)}][ConnectionDrainer] Server unavailable during drain, skipping." + ) + end _ ->As per coding guidelines, "Use
withfor chaining operations that return{:ok, _}or{:error, _}tuples."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase_web/connection_drainer.ex` around lines 38 - 64, The current drain logic in the case branch for safe_bandit_pid uses hard pattern matches on ThousandIsland.suspend(server_pid) and ThousandIsland.connection_pids(server_pid) which will crash on :error; replace those sequential calls with a with-based chain (e.g., with {:ok, server_pid} from safe_bandit_pid(...), {:ok, _} <- ThousandIsland.suspend(server_pid), {:ok, running_connections} <- ThousandIsland.connection_pids(server_pid) do ...) so you can handle {:error, _} returns gracefully (log via IO.puts or a logger and skip wait_for_drain or return {:error, reason}); keep wait_for_drain(server_pid) only inside the successful with block and ensure the fallback clause logs the error instead of crashing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sanbase_web/connection_drainer.ex`:
- Around line 67-75: The resume_listener/1 function currently hard-matches :ok =
ThousandIsland.resume(server_pid) which will crash if ThousandIsland.resume/1
returns :error; change it to handle both :ok and :error (similar to terminate/2)
by calling ThousandIsland.resume(server_pid) inside a case/with and treating
:error as a no-op (or log it) so init does not crash; use the existing
safe_bandit_pid/1 to get server_pid and handle the {:ok, server_pid} branch by
matching on the resume result instead of forcing :ok.
In `@test/sanbase_web/connection_drainer_test.exs`:
- Around line 37-49: start_endpoint!/1 currently calls Supervisor.start_link to
launch the Bandit child but never registers the supervisor with the test
process, causing a resource leak; replace the Supervisor.start_link call and
manual child_spec registration with start_supervised!/1 so the supervisor is
tied to the test process and automatically stopped after each test.
Specifically, in start_endpoint!/1 update the code path that constructs
bandit_child and calls Supervisor.start_link to instead call
start_supervised!(bandit_child) (or start_supervised!([{Bandit, bandit_opts, id:
{endpoint, :http}}]) as appropriate), then continue using
Bandit.PhoenixAdapter.bandit_pid(endpoint, :http) and
ThousandIsland.listener_info(bandit_pid) unchanged.
---
Duplicate comments:
In `@lib/sanbase_web/connection_drainer.ex`:
- Around line 38-64: The current drain logic in the case branch for
safe_bandit_pid uses hard pattern matches on ThousandIsland.suspend(server_pid)
and ThousandIsland.connection_pids(server_pid) which will crash on :error;
replace those sequential calls with a with-based chain (e.g., with {:ok,
server_pid} from safe_bandit_pid(...), {:ok, _} <-
ThousandIsland.suspend(server_pid), {:ok, running_connections} <-
ThousandIsland.connection_pids(server_pid) do ...) so you can handle {:error, _}
returns gracefully (log via IO.puts or a logger and skip wait_for_drain or
return {:error, reason}); keep wait_for_drain(server_pid) only inside the
successful with block and ensure the fallback clause logs the error instead of
crashing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b0abf27-7c17-4e63-bed8-15d80328316b
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
config/config.exsconfig/dev.exsconfig/runtime.exslib/sanbase/application/application.exlib/sanbase_web/connection_drainer.exmix.exstest/sanbase_web/connection_drainer_test.exs
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/sanbase/application/application.ex
- config/runtime.exs
- mix.exs
Comment on lines
+37
to
+49
| defp start_endpoint!(plug) do | ||
| endpoint = | ||
| String.to_atom("connection_drainer_test_endpoint_#{System.unique_integer([:positive])}") | ||
|
|
||
| bandit_opts = [plug: plug, port: 0, startup_log: false] | ||
| bandit_child = Supervisor.child_spec({Bandit, bandit_opts}, id: {endpoint, :http}) | ||
|
|
||
| {:ok, _endpoint_sup} = | ||
| Supervisor.start_link([bandit_child], strategy: :one_for_one, name: endpoint) | ||
|
|
||
| {:ok, bandit_pid} = Bandit.PhoenixAdapter.bandit_pid(endpoint, :http) | ||
| {:ok, {_ip, port}} = ThousandIsland.listener_info(bandit_pid) | ||
| {endpoint, bandit_pid, port} |
Contributor
There was a problem hiding this comment.
Potential resource leak: Supervisor started in start_endpoint!/1 is never stopped.
The start_endpoint!/1 function starts a supervisor via Supervisor.start_link/2 but doesn't register it with start_supervised!/1. This means the supervisor and Bandit server may not be properly cleaned up between tests, potentially causing port conflicts or resource leaks.
Suggested fix using start_supervised!
defp start_endpoint!(plug) do
endpoint =
String.to_atom("connection_drainer_test_endpoint_#{System.unique_integer([:positive])}")
bandit_opts = [plug: plug, port: 0, startup_log: false]
bandit_child = Supervisor.child_spec({Bandit, bandit_opts}, id: {endpoint, :http})
- {:ok, _endpoint_sup} =
- Supervisor.start_link([bandit_child], strategy: :one_for_one, name: endpoint)
+ endpoint_sup =
+ start_supervised!(
+ Supervisor.child_spec(
+ %{
+ id: endpoint,
+ start: {Supervisor, :start_link, [[bandit_child], [strategy: :one_for_one, name: endpoint]]},
+ type: :supervisor
+ },
+ []
+ )
+ )
{:ok, bandit_pid} = Bandit.PhoenixAdapter.bandit_pid(endpoint, :http)
{:ok, {_ip, port}} = ThousandIsland.listener_info(bandit_pid)
{endpoint, bandit_pid, port}
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanbase_web/connection_drainer_test.exs` around lines 37 - 49,
start_endpoint!/1 currently calls Supervisor.start_link to launch the Bandit
child but never registers the supervisor with the test process, causing a
resource leak; replace the Supervisor.start_link call and manual child_spec
registration with start_supervised!/1 so the supervisor is tied to the test
process and automatically stopped after each test. Specifically, in
start_endpoint!/1 update the code path that constructs bandit_child and calls
Supervisor.start_link to instead call start_supervised!(bandit_child) (or
start_supervised!([{Bandit, bandit_opts, id: {endpoint, :http}}]) as
appropriate), then continue using Bandit.PhoenixAdapter.bandit_pid(endpoint,
:http) and ThousandIsland.listener_info(bandit_pid) unchanged.
IvanIvanoff
force-pushed
the
migrate-bandit
branch
from
April 9, 2026 08:53
eb7d3a6 to
20bb209
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/sanbase_web/connection_drainer_test.exs (3)
53-55:⚠️ Potential issue | 🟠 MajorManage the server supervisor with ExUnit lifecycle helpers.
Starting it with
Supervisor.start_link/2here leaves cleanup to test success paths; crashes/timeouts can leak listeners and make async runs flaky.🧪 Proposed fix
- {:ok, sup} = Supervisor.start_link([{Bandit, bandit_opts}], strategy: :one_for_one) + sup = + start_supervised!( + Supervisor.child_spec( + %{ + id: make_ref(), + start: {Supervisor, :start_link, [[{Bandit, bandit_opts}], [strategy: :one_for_one]]}, + type: :supervisor + }, + [] + ) + ) [{_id, server_pid, _type, _modules}] = Supervisor.which_children(sup)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/connection_drainer_test.exs` around lines 53 - 55, The test starts the Bandit supervisor directly with Supervisor.start_link/2 (and then inspects children via Supervisor.which_children/1 and ThousandIsland.listener_info/1), which can leak listeners on failures; replace that manual start with ExUnit's supervised helpers so the supervisor is automatically terminated on test exit — e.g., in the test setup call start_supervised!/1 (or start_supervised/1) with the same spec used for Supervisor.start_link([{Bandit, bandit_opts}], strategy: :one_for_one) and then continue to call Supervisor.which_children/1 and ThousandIsland.listener_info/1 against the supervised pid; ensure the change is placed in the test setup or the test body so ExUnit will clean up the supervisor on crashes/timeouts.
19-21:⚠️ Potential issue | 🟠 MajorReplace sleep-based request coordination with an explicit plug-entry signal.
:request_startedis sent before the request is actually handled, so shutdown can race ahead and make these assertions nondeterministic (including the forced-timeout test).🛠️ Proposed stabilization
- def call(conn, _opts) do + def call(conn, opts) do + if test_pid = Keyword.get(opts, :test_pid) do + send(test_pid, :slow_plug_started) + end Process.sleep(1_000) @@ - {sup, _server_pid, port} = start_server!(SlowPlug) + {sup, _server_pid, port} = start_server!({SlowPlug, test_pid: self()}) @@ - request_task = - Task.async(fn -> - send(test_pid, :request_started) - Req.get!("http://127.0.0.1:#{port}/") - end) - - assert_receive :request_started, 1_000 - # Let Bandit accept the connection before initiating shutdown - Process.sleep(100) + request_task = Task.async(fn -> Req.get!("http://127.0.0.1:#{port}/") end) + assert_receive :slow_plug_started, 1_000 @@ - {sup, _server_pid, port} = start_server!(SlowPlug) + {sup, _server_pid, port} = start_server!({SlowPlug, test_pid: self()}) @@ - _request_task = - Task.async(fn -> - send(test_pid, :request_started) - Req.get("http://127.0.0.1:#{port}/") - end) - - assert_receive :request_started, 1_000 - Process.sleep(100) + _request_task = Task.async(fn -> Req.get("http://127.0.0.1:#{port}/") end) + assert_receive :slow_plug_started, 1_000 @@ - {sup, _server_pid, port} = start_server!(SlowPlug, shutdown_timeout: 200) + {sup, _server_pid, port} = start_server!({SlowPlug, test_pid: self()}, shutdown_timeout: 200) @@ - Process.sleep(100) + assert_receive :slow_plug_started, 1_000Also applies to: 65-74, 97-104, 130-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/connection_drainer_test.exs` around lines 19 - 21, The test plug's sleep-based coordination in call(conn, _opts) (and the similar blocks at the other occurrences) should be replaced with an explicit plug-entry signal: instead of Process.sleep(1_000), send a deterministic message from the plug (e.g., send(test_pid, {:plug_entered, self()}) or similar) and have the test wait/receive that message; do not rely on :request_started since it fires before request handling. Update the tests to inject/obtain the test process PID and assert against the explicit {:plug_entered, pid} signal so shutdown timing becomes deterministic.
13-39: 🛠️ Refactor suggestion | 🟠 MajorExtract
SlowPlugandFastPluginto separate support files.Keeping multiple modules inside this test file violates the repo rule and reduces reusability across shutdown tests.
♻️ Proposed refactor
-defmodule SanbaseWeb.ConnectionDrainingTest do +defmodule SanbaseWeb.ConnectionDrainingTest do @@ - defmodule SlowPlug do - `@moduledoc` false - import Plug.Conn - - def init(opts), do: opts - - def call(conn, _opts) do - Process.sleep(1_000) - - conn - |> put_resp_content_type("text/plain") - |> send_resp(200, "ok") - end - end - - defmodule FastPlug do - `@moduledoc` false - import Plug.Conn - - def init(opts), do: opts - - def call(conn, _opts) do - conn - |> put_resp_content_type("text/plain") - |> send_resp(200, "ok") - end - end + # Use SanbaseWeb.TestSupport.SlowPlug / SanbaseWeb.TestSupport.FastPlugAs per coding guidelines, "Never nest multiple modules in the same file as it can cause cyclic dependencies and compilation errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/connection_drainer_test.exs` around lines 13 - 39, Extract the nested modules SlowPlug and FastPlug out of the test file into separate support modules so they are top-level and reusable; create dedicated support files that define SlowPlug and FastPlug (keeping the same module names and public functions init/1 and call/2), remove their definitions from test/sanbase_web/connection_drainer_test.exs, and ensure the test suite loads these support files (so tests can reference SlowPlug and FastPlug unchanged) to comply with the rule against multiple modules in one file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/sanbase_web/connection_drainer_test.exs`:
- Around line 53-55: The test starts the Bandit supervisor directly with
Supervisor.start_link/2 (and then inspects children via
Supervisor.which_children/1 and ThousandIsland.listener_info/1), which can leak
listeners on failures; replace that manual start with ExUnit's supervised
helpers so the supervisor is automatically terminated on test exit — e.g., in
the test setup call start_supervised!/1 (or start_supervised/1) with the same
spec used for Supervisor.start_link([{Bandit, bandit_opts}], strategy:
:one_for_one) and then continue to call Supervisor.which_children/1 and
ThousandIsland.listener_info/1 against the supervised pid; ensure the change is
placed in the test setup or the test body so ExUnit will clean up the supervisor
on crashes/timeouts.
- Around line 19-21: The test plug's sleep-based coordination in call(conn,
_opts) (and the similar blocks at the other occurrences) should be replaced with
an explicit plug-entry signal: instead of Process.sleep(1_000), send a
deterministic message from the plug (e.g., send(test_pid, {:plug_entered,
self()}) or similar) and have the test wait/receive that message; do not rely on
:request_started since it fires before request handling. Update the tests to
inject/obtain the test process PID and assert against the explicit
{:plug_entered, pid} signal so shutdown timing becomes deterministic.
- Around line 13-39: Extract the nested modules SlowPlug and FastPlug out of the
test file into separate support modules so they are top-level and reusable;
create dedicated support files that define SlowPlug and FastPlug (keeping the
same module names and public functions init/1 and call/2), remove their
definitions from test/sanbase_web/connection_drainer_test.exs, and ensure the
test suite loads these support files (so tests can reference SlowPlug and
FastPlug unchanged) to comply with the rule against multiple modules in one
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40f9cdf3-a864-46da-8b9d-7fbd4a497934
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
config/config.exsconfig/dev.exsconfig/runtime.exslib/sanbase/application/application.exlib/sanbase_web/connection_drainer.exmix.exstest/sanbase_web/connection_drainer_test.exs
💤 Files with no reviewable changes (1)
- lib/sanbase_web/connection_drainer.ex
🚧 Files skipped from review as they are similar to previous changes (3)
- config/runtime.exs
- mix.exs
- config/dev.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
1 participant
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.
Changes
Ticket
Checklist:
Summary by CodeRabbit
Infrastructure Updates
Tests