Skip to content

Fix socket drainer reporting an extra round and waiting after the last batch - #6897

Open
ramiscaleai wants to merge 1 commit into
phoenixframework:mainfrom
ramiscaleai:socket-drainer-rounds
Open

ramiscaleai wants to merge 1 commit into
phoenixframework:mainfrom
ramiscaleai:socket-drainer-rounds

Conversation

@ramiscaleai

Copy link
Copy Markdown

Phoenix.Socket.PoolDrainer computes the number of rounds as div(total, size) + 1. Stream.chunk_every/2 only produces ceil(total / size) batches, so when the number of connections is an exact multiple of :batch_size the drainer believes there is one more round than there really is. Two effects:

  • The [:phoenix, :socket_drain] telemetry event, and the log line built from it, report a wrong number of rounds, e.g. "round 2 of 3" for a drain made of two batches.
  • The sleep between batches is guarded by index < rounds, so the drainer also sleeps :batch_interval after the last batch, when there is nothing left to do. The example in the Phoenix.Endpoint.socket/3 docs (150k connections with the default :batch_size of 10_000 and :batch_interval of 2_000) is such a case: the drainer needs 15 x 2s = 30s, the whole default :shutdown, instead of 14 x 2s = 28s.

The fix computes rounds with a ceiling division and reports length(pids) as the count of each batch (the special case for the last batch is no longer needed).

Tests

Nothing exercised PoolDrainer.terminate/2, so this adds test/phoenix/socket/pool_drainer_test.exs. It drains an isolated PoolSupervisor pool and checks:

  • every connection receives phx_drain;
  • a partial last batch (5 connections, batches of 2: 3 rounds, counts 2/2/1);
  • an exact multiple (4 connections, batches of 2: 2 rounds);
  • no extra wait after the last batch;
  • no events when there are no connections.
mix test test/phoenix/socket/pool_drainer_test.exs

Before the fix, the exact multiple test fails (rounds: 3, expected 2) and the wait test fails (5001ms elapsed for a 5000ms interval): 5 tests, 2 failures. After the fix: 5 tests, 0 failures. The rest of test/phoenix gives the same results with and without this change.

I ran this locally on Elixir 1.14 / OTP 25, in a scratch copy of the repository with a small compatibility shim (mix.exs requires 1.15). lib/ and the test file are identical to this PR; CI covers the supported versions.

AI disclosure: Claude Code (an AI coding agent) found this bug, wrote the fix and the tests, and drafted this description. The tests above were run locally.

🤖 Generated with Claude Code

PoolDrainer computed the number of rounds as div(total, size) + 1, which is
one too many when the number of connections is a multiple of the batch size.
The :socket_drain telemetry/log then reported e.g. "round 2 of 3" for a drain
with two batches, and the drainer slept for :batch_interval after the last
batch. Use a ceiling division and report the real size of each batch.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant