Send packets through the per-connection queue by default

[leroysquad]

Summary

• The per-connection send queue stays in this branch: short writes are retried, MaxPendingBytes disconnects a connection that would grow a non-empty queue past the cap, and shutdown no longer waits on the caller.
• SendQueueEnabled ships off. A 1000-bot run, a slow real client, and a play session on this head are still required before the default can flip. Those numbers are not in this pull request.
• Shutdown completes the writer and returns. The socket FIN runs after the drain finishes, or after 250 ms, whichever is first. Close still cancels the drain and is the hard bound. A disconnect reason is written before the FIN when the backlog drains inside that 250 ms window. A 1 MiB backlog on a client that reads slowly does not.
• Server-list query sockets always call Shutdown(), including when the queue is on.
• One accepted packet that is already at or above the cap is not added to the pending-byte count, so the packets behind it are judged on their own size.

Test plan

• Shutdown_Should_DeliverQueuedBytes_When_SendQueueEnabled expects the queued reason on the wire
• Shutdown_Should_ReturnImmediately_When_PeerNeverReads asserts shutdown returns within 50 ms
• 1000-bot capture on this head, with overflow disconnect counts, before the default can flip
• Throttled real client at default view distance
• Play session on a queue-on server

[leroysquad]

Same-machine retest on the Ryzen 9 5950X, /probe profiler 60, queue off vs queue on. Large runs are 1000 players and about 131k chunks.

• Large, queue off: 1-minute TPS 0.13 and 0.15. DoOperationSendSingleBuffer was 49% of main-thread samples. Mean tick 5.3s and 7.7s.
• Large, queue on: 1-minute TPS 0.37 and 0.66. That send method was 0% of main-thread samples. Mean tick 2.8s and 1.6s.
• Small world, 512 chunks: 1-minute TPS about 8.5 with the queue off, 24.1 with the queue on.

The large world is still well under 20 TPS. The gameplay thread is no longer inside the socket send.

[Zaldaryon]

Reviewed head 7deca3d. The same-machine results show the send method falling from 49% to 0% of main-thread samples, large-world one-minute TPS rising from 0.13/0.15 to 0.37/0.66, and small-world TPS rising from about 8.5 to 24.1. Local bootstrap, both Release builds, and the smoke test passed. The two inline findings block changing the shipped default. indev advanced to ece5d42 after #343 merged; please rebase onto current indev before requesting review again.

[leroysquad]

Addressed the two inline notes in c05adda.

Short writes: StratumSendQueue.SendAsync keeps the int from Socket.SendAsync(ReadOnlyMemory<byte>, SocketFlags, CancellationToken) and loops until the whole buffer is accepted. A non-positive count, or any send exception other than cancellation, disconnects the connection. Cancellation stops the drain.

Per-connection cap: Performance.Network.MaxPendingBytes defaults to 8 MiB. Chunk scheduling already slows that client at OutboundPressurePendingBytesHardLimit (1 MiB) and still has a minimum budget there, so the disconnect threshold sits above that line. An enqueue that would grow a non-empty queue past MaxPendingBytes is refused and the connection is disconnected. An empty queue still accepts one packet, so a single large send is kept. The caller stays unblocked, so the gameplay thread does not sit on the slow socket, and the refused packet is not left on the queue, so the length-prefixed stream stays intact.

SendQueueEnabled stays true.

Still open on this PR: a slow-reader load test, and a rebase onto ece5d42 (#343). Rebase would rewrite the published commits, so this update is a new commit on the existing branch. GitHub reports the PR mergeable against current indev.

[Pixnop]

Requesting changes. The two things c05adda adds do answer Zaldaryon's review as far as the code goes: the byte cap with a defined overflow policy, and the loop on the SendAsync count. I would take both. What I cannot take yet is the default flip, for one concrete bug and for lack of evidence.

Kick reasons are lost with the queue on. DisconnectPlayer sends the disconnect packet, then CloseConnection calls TcpSocket.Shutdown(Both) straight away. With the queue off, the send has already put the bytes in the kernel buffer, so the client gets the reason and then the FIN. With the queue on, Send only enqueues. Shutdown is not patched to wait for the drain, and Close completes the channel and cancels the token, so the drain finds a dead socket. Reproduced in a harness built from your StratumSendQueue.cs and the generated TcpNetConnection send, shutdown and close code, on loopback: queue off, 200 of 200 reasons delivered; queue on, 0 of 200 with no gap between send and shutdown, 179 of 200 with a 20 µs gap. It is worst where almost nothing sits between the two calls: wrong version, wrong password, whitelist, ban, server full. Those players would see a bare connection loss where they should see the reason. The queue needs a bounded flush before shutdown (drain what is queued, with a short timeout) before it can be the default.

Evidence. The comment this PR deletes set the bar itself: soak on a community server before flipping the default. What is on the thread is 60-second profiler runs with 1000 bots, taken at 7deca3d, before the cap and the overflow disconnect existed. Bots do not parse entity or attribute packets, which is where the previous flush crashed real clients on join. Nothing in the repo runs this path either: the scenarios join through a dummy connection, and the smoke test connects no client. Before the flip I would want three runs on the final code: the 1000-bot capture again with the count of overflow disconnects, a throttled real client (a few Mbit/s) joining at default view distance to show it loads slowly and is not kicked at 8 MiB, and a real play session on a queue-on server.

Smaller:

• Each accepted TCP connection now allocates the 64 KiB coalesce buffer and a drain task before the peer has sent a byte. A connection that never identifies has no ping timeout, so a scanner holding sockets open costs 64 KiB each where it used to cost 4. Rent the buffer on the first small packet.
• The overflow warning names no address and no player, and the player gets no reason. One more field in that line tells an admin whom the cap hit and which key to raise.
• The PR body still says the change is a config default only. It should mention MaxPendingBytes and the disconnect policy.

A suggestion: split it. Land the cap, the short-write loop and the flush on disconnect with the default still off. That part is safe to merge once the flush is in. Then flip the default in a follow-up that carries the three runs above.

[Zaldaryon]

Requesting changes. The byte cap and complete-send loop address the two previous inline findings, and I resolved those threads.

The default-on queue still drops disconnect reasons. DisconnectPlayer enqueues the reason, then Close completes the channel and immediately cancels the token awaited by DrainAsync. A cancelled drain can exit before the queued frame reaches the socket. Pixnop's reproduction delivered 0/200 reasons with no gap and 179/200 after 20 microseconds, versus 200/200 with the queue off. This affects wrong-version/password, whitelist, ban, and full-server responses. Add a bounded flush before shutdown and a regression test on the queue-enabled disconnect path.

The queue also allocates a 64 KiB coalesce buffer and drain task before an unauthenticated peer sends a packet. The overflow warning identifies neither player nor address, and the body does not describe the byte limit or disconnect policy.

Before enabling this by default, provide final-code load evidence with overflow counts, a throttled real-client join at default view distance, and a real play session. The previous local build and smoke evidence was for 7deca3d, not this head. GitHub reports no checks for c05adda. Rebase onto current indev ece5d42 before the next review.

[Zaldaryon]

Since my review, #347 merged and advanced indev from ece5d42 to c5c3aee. Please rebase onto c5c3aee when addressing the disconnect drain issue and load tests; the earlier base SHA is no longer current.

[leroysquad]

e974389 flushes queued bytes for 250 ms at the start of Shutdown and Close, before the cancellation token is cancelled. SendQueueDisconnectScenarios writes a payload and calls Shutdown immediately. That scenario was not executed in this pass.

The coalesce buffer and the drain task are created on the first enqueued packet. The overflow warning names the player, the address, and Performance.Network.MaxPendingBytes.

The numbers already on the thread are still the 7deca3d captures: large world 1-minute TPS 0.13 and 0.15 with the queue off, 0.37 and 0.66 with the queue on; small world about 8.5 to 24.1. The send method went from 49% of main-thread samples to 0%. I did not start another 1000-bot capture, a throttled client join, or a play session. I did not rebase onto c5c3aee. SendQueueEnabled stays true.

[leroysquad]

Rebased onto current indev (c5c3aee). New head: 56f7e92.

[Zaldaryon]

The disconnect flush fix and SendQueueDisconnectScenarios pass on this head on Windows and Linux. One blocking validation gap remains: the final-code 1000-bot capture, throttled real-client join at default view distance, and play session are still open in the PR body. The available counts came from 7deca3d, before the byte cap and shutdown flush, so they do not establish the behavior of this default-on queue. Please run the current head under load and report queue overflows and disconnects, then record the real-client join and play results.

Local validation on head 56f7e92603a6426808e917b9873d0fb4c73588ea: Windows and Linux bootstraps with ILSpy 10.1.0.8386 passed; both Release builds had 0 errors; both smoke runs reached WorldReady; SendQueueDisconnectScenarios passed 1/1 on each OS; git diff --check passed. GitHub has no build or test checks.

PR #344 has since merged into indev at 66a8530718e254a5b055908140dcff7c425bfd16. This head still records base c5c3aee. Please rebase onto current indev and rerun the gates with the requested load evidence before approval.

[Pixnop]

Requesting changes. The blocker from my last review is fixed. Shutdown and Close now call FlushPending(250) before Shutdown(Both) and before cts.Cancel, and since FlushPending, Enqueue and the lazy start all go through startGate, a packet that races the flush is either drained or dropped, never left behind. On 56f7e92 I ran the bootstrap and the Release build (0 errors, no warning in the touched files), the full scenario suite (50 of 50) and the new scenario 30 times (30 of 30). The scenario does guard the bug: removing the flush from Shutdown turns it red with an empty read, and so does completing the channel without waiting for the drain. My smaller points are handled too: the drain and the 64 KiB buffer start on the first packet, the overflow line names the player, the address and the key, and the body describes the cap and the disconnect policy. The rollback and the short-write loop read correctly, and a peer that never reads is cut off once, on the ninth 1 MiB packet, with later sends dropped.

What still blocks:

1. The default flip still has no evidence. SendQueueEnabled is still true (StratumConfig.cs:640), and every number on the thread is from 7deca3d, before the cap and the flush. The three runs Zaldaryon and I asked for are still open. My suggestion stands: land the cap, the short-write loop and the flush with the default off, and flip it in a follow-up that carries the runs.

2. Shutdown now blocks the main thread for up to 250 ms. FlushPending ends in task.Wait(250) on the caller (StratumSendQueue.cs:110), and ConnectedClient.CloseConnection calls Shutdown synchronously from DisconnectPlayer. When the drain is stuck in socket.SendAsync, the caller waits the full 250 ms. I measured it on loopback: with a peer that never reads and about 3 MiB offered, Shutdown blocked 250 ms. With a reader throttled to about 4 Mbit/s and a 1 MiB backlog, Shutdown blocked 250 ms and the reason still did not arrive (315738 of 1048726 bytes received). 1 MiB is the chunk scheduler's hard limit, so a slow client in the middle of loading is enough. The callers run on the main thread: the ping-timeout kick in PingTimerTick, kicks and bans, "joined again, killing previous client", and the Stop loop, which disconnects clients one after another before the save. Each stuck client is a 250 ms tick stall, and they add up within one pass. With the queue off, Shutdown never waits. Please do not wait on the caller: complete the writer and run TcpSocket.Shutdown(Both) as a continuation of the drain, bounded by Task.WhenAny(drainTask, Task.Delay(250)), then return. The Close that CloseConnection queues one second later cancels the token and stays the hard bound. A scenario with a peer that never reads and a backlog larger than the socket buffer, asserting that Shutdown returns within a few milliseconds, would hold it.

3. Server-list query sockets are no longer closed. After answering a ServerQuery, ProcessNetMessage calls connection.Shutdown() only when the queue is off (ServerMain.cs.patch:1163, from c7c83f6, when Shutdown still dropped the queued answer). With this PR's default the server never closes those sockets. No ConnectedClient exists on that path, so no ping timeout applies, and the answer is small enough to be coalesced: one 6-byte frame starts the drain, allocates the 64 KiB buffer (StratumSendQueue.cs:210) and keeps it parked until the peer closes. A peer that sends one query per socket and holds the sockets open costs about 68 KiB each instead of about 4 KiB, which undoes the lazy start. Vanilla closes them ("Query complete"), and so does indev with its default. Now that Shutdown flushes, drop the guard and always call Shutdown(). With today's synchronous flush that would make the packet-parsing thread wait on the drain, which the fix in item 2 avoids. This one is from reading the code, I did not send a real query.

4. A single large send is not kept. The config comment (StratumConfig.cs:650-656) and the body say an empty queue still accepts one packet. It does, but the packet stays counted in pendingBytes until the drain dequeues it on the thread pool (StratumSendQueue.cs:205), later still when coalesced bytes sit ahead of it (line 201). Any Enqueue in that window sees queued > maxPendingBytes && queued - length > 0 (line 133) and disconnects, and the large packet is lost with the connection. On join, HandleRequestJoin calls SendServerAssets and then SendPlayerEntity on the same thread with nothing in between that waits for the drain, so an assets packet at or above the cap kicks the joining player with the overflow warning whenever the main thread gets there first. At 8 MiB that depends on the mod set, which nobody has measured. With a lowered MaxPendingBytes, which EnsureSane accepts down to CoalesceLimitBytes (64 KiB by default), it takes far fewer mods. Count an accepted packet at or above the cap apart from pendingBytes, so the packets behind it are judged on their own bytes.

Non-blocking:

• SendQueueDisconnectScenarios gets its queue only from the shipped default. With the default back to false it fails with "SendQueueEnabled did not attach a queue" (line 34) before it reaches the flush, so it cannot ship with the split as it is. Seed Performance.Network.SendQueueEnabled = true through an AtlasDataFiles fixture, the way the *DisabledScenarios classes do, with stratum.json at the current config version.
• That scenario is the only test on the queue path. Removing the flush from Close, disabling the cap, replacing the short-write loop with offset = length, or starting the drain eagerly all stay green. A scenario with a peer that never reads and a lowered MaxPendingBytes would cover the cap, the rollback that keeps frames whole, the single disconnect and the oversized first packet.
• Turning the queue on also turns on the outbound-pressure chunk throttle. ApplyStratumOutboundPressureBudget (ServerSystemSendChunks.cs.patch:444-470) reads PendingSendCount and PendingSendBytes, which are always 0 with the queue off, so it never fired before. The queue-off and queue-on captures had it off in one and on in the other. In the final runs, please report chunks sent per tick and skippedPressure next to TPS, add one queue-on pass with ChunkSending.OutboundPressureEnabled=false, and say in the body that the flip activates the throttle.
• The body says a reason queued before CloseConnection is written before the FIN. That holds when the backlog drains within 250 ms: in the throttled test above, 64 KiB and 256 KiB backlogs got the reason through, 1 MiB did not. That is the bound I asked for, the body should just say so.
• TcpNetConnection.cs.patch is not what scripts/extract-patches.sh writes: the Shutdown hunk has two lines of context, line 101 is a bare empty line where a context line belongs, and the index line still names indev's blob (34c0f3d, the real one is 5280080). It applies fine, but the next extract run will rewrite it. Please regenerate it.
• The head is one commit behind indev (66a8530, #344, which only touches ServerSystemEntitySimulation.cs.patch) and merges cleanly, so a maintainer can run /rebase.

[leroysquad]

Pushed 3acd1f9 for the four blocking notes on 56f7e92.

• SendQueueEnabled is false again. The cap, the short-write loop, and the shutdown path stay. The default flip waits for a 1000-bot run, a slow real client, and a play session on this head.
• Shutdown completes the writer and returns. The FIN runs after the drain, or after 250 ms, on that task. Close still cancels and is the hard bound. A new scenario asserts Shutdown returns within 50 ms when the peer never reads.
• Server-list queries always call Shutdown().
• One accepted packet at or above MaxPendingBytes is not added to the pending-byte count, so the packets behind it are judged on their own size.

The disconnect scenario seeds the queue on, because the shipped default is off. I have not run the scenario suite on this commit.

[leroysquad]

@Pixnop @Zaldaryon the four notes on 56f7e92 are in 3acd1f9. The queue ships off. Shutdown returns immediately and closes the socket after the drain. Query sockets always shut down. One oversized packet is not counted in the pending bytes. Ready for another look.

[Zaldaryon]

The updates in 3acd1f9 address the core design concerns: keeping SendQueueEnabled false by default allows the queue infrastructure, byte cap, and short-write loop to land safely; decoupling Shutdown from synchronous caller blocking eliminates main-thread stalls; query sockets now always shut down; and the oversized initial packet exemption avoids cascading disconnects.

What still blocks approval:

1. The branch is based on c5c3aee. Current indev is at a3966e2 (#344, #352, #353 merged). On Windows with ilspycmd 10.1.0.8386, scripts/bootstrap.ps1 against c5c3aee fails to apply 12 other patches. On a3966e2, bootstrap.ps1 and the Release build pass with 0 errors. Please rebase onto current indev.
2. As noted in the comments, the scenario suite has not been executed on 3acd1f9. Running the scenario gate and confirming the disconnect scenarios pass on the updated head is required.
3. The load test and profiler evidence (1000-bot capture, slow-reader load test, play session, and outbound pressure metrics) remain open before the feature toggle can be enabled by default.

Non-blocking:

• In TcpNetConnection.cs.patch, the index line and hunk context should be normalized by re-running scripts/extract-patches.ps1.

[Pixnop]

Requesting changes, for two small things. All four blockers from my last review are addressed in 3acd1f9, and I checked each one on that head. The queue ships off (StratumConfig.cs:639). Shutdown no longer waits: ScheduleShutdown completes the writer under startGate and returns, and the FIN runs after Task.WhenAny(drain, Task.Delay(250)). With the scenario's own setup (256 KiB backlog, 1 KiB SO_SNDBUF, peer never reads), Shutdown returned in 0.003 ms at the median and 0.773 ms at worst over 100 runs. With the caller made to wait again, the same runs take 249 to 252 ms. My loopback harness from last time now reads 0 ms in all five measurements where 56f7e92 gave 1, 161 and 250 ms. A peer that never reads gets its FIN at the 250 ms bound, and reason delivery is what the body now says: 64 KiB and 256 KiB backlogs get the reason through, 1 MiB does not. For the query sockets I sent real queries this time, to this build booted headless: with the queue on, every socket got the 111-byte answer and was closed by the server (51 of 51), the same with the queue off, and with the old guard put back, 0 of 51 were closed within 3 s. Packets behind an oversized packet are now judged on their own bytes: at a 128 KiB cap, one 256 KiB packet followed by eight 1 KiB packets to a reading peer gave 0 disconnects in 600 trials, against 100 of 100 with the 56f7e92 rule. The fixture seeds the queue at ConfigVersion 4, as asked.

On 3acd1f9 I ran the bootstrap, the Release build (0 errors, no warning in the touched files) and the full scenario suite (51 of 51). The two disconnect scenarios passed 30 of 30, in a French locale and under the invariant culture. Both scenarios earn their place: making the caller wait again turns Shutdown_Should_ReturnImmediately_When_PeerNeverReads red (251 ms), and running the FIN before the drain turns Shutdown_Should_DeliverQueuedBytes_When_SendQueueEnabled red. I also merged it into a3966e2 with the one conflicting line resolved by hand: bootstrap, build and 55 of 55.

What still blocks:

1. The branch no longer merges into indev. 3acd1f9 edits the ProcessNetMessage hunk header in ServerMain.cs.patch by hand (line 1125, +3895,62), but #352 had already regenerated that file on indev and moved the same header to +3909,63. GitHub reports a conflict, and git merge-tree stops on exactly that line. 56f7e92 still merged cleanly, so my note that a maintainer could run /rebase no longer holds: /rebase stops on a conflict. Please rebase onto a3966e2 and regenerate ServerMain.cs.patch and TcpNetConnection.cs.patch with scripts/extract-patches.sh rather than fixing the header by hand (+3909,62 would be the value). That also clears what is still stale from my last review: the index lines (TcpNetConnection.cs.patch names 34c0f3d, the real blob is a99c8d5; ServerMain.cs.patch names a654aca, the real one is d68c94a), the two lines of context on the Shutdown hunk, the bare empty line at patch line 116, and the Close hunk start, +329 where a regeneration writes +330. Then run the scenario gate on the new head and post the result, which also answers Zaldaryon's two points.

2. An oversized packet is still refused when anything is queued ahead of it. The exemption only applies while pendingBytes is 0 (StratumSendQueue.cs:149 and 160), and small packets stay counted until the drain dequeues them on the thread pool (Release, line 275). HandleRequestJoin enqueues the channels packet, LevelInitialize, LevelProgress and WorldMetaData on the main thread just before SendServerAssets, with nothing in between that waits for the drain. So an assets packet at or above the cap still kicks the joining player whenever the drain has not caught up, which is the outcome of item 4 coming from the other side. At a 128 KiB cap, a 1 KiB packet immediately followed by a 256 KiB packet was disconnected in 8 of 600 trials. During play it is not even a race: while the drain sits in SendAsync on a slow client, chunk packets wait in the channel, and any packet at or above the cap disconnects that client every time. The pendingBytes > 0 checks protect nothing, since the packets queued behind an oversized one can already fill the cap. Gate the exemption on exemptInFlight alone and drop both checks. Retention stays bounded by the cap plus one oversized packet (plus the one already being sent, as today). Please update the comment at StratumConfig.cs:649-654 to match. A scenario with a lowered MaxPendingBytes and a peer that never reads (one small packet, then one at the cap, connection stays up) would hold it.

Non-blocking:

• The title still says "by default", and so does the subject of f7ab355. A squash merge would record a default flip this PR no longer makes, so please rename it, for example "Bound the per-connection send queue and flush it on shutdown".
• Shutdown_Should_ReturnImmediately_When_PeerNeverReads relies on 256 KiB being more than loopback absorbs, and never checks that the drain is still stuck when Shutdown returns. Here a peer that never reads absorbs 176640 of the 262148 bytes, so the scenario catches the old wait with about 83 KiB to spare. Absorption grows with the client's receive buffer: at an effective 196608 it takes 259609 bytes, at 262144 the whole frame, and from there the old blocking code also returns in under 50 ms. Set client.ReceiveBufferSize = 4096 before Connect (line 79; absorption drops to 8192 bytes) and offer a few MiB. After Shutdown returns, assert that the private drainTask is not complete (PendingSendBytes cannot serve, because a large packet is released before SendAsync), then read and assert EOF within about 250 ms plus a margin, which also covers the FIN on timeout.
• The two scenarios catch the Shutdown wait and a FIN before the drain, and nothing else. Putting the query guard back, reverting the oversized rule to the 56f7e92 one, disabling the cap or starting the drain eagerly all stay green. The scenario in item 2 would cover the cap and the exemption. A ServerQuery sent through ProcessNetMessage on the harness connection with the queue on, asserting the answer and then EOF, would cover the query fix, which I could only check on a booted server. I drop the short-write loop from my last list: in practice SendAsync on a TCP socket only completes once the whole buffer is accepted, so no real-socket scenario reaches it.
• The outbound-pressure point from my last review moves to the follow-up that flips the default, together with the three runs.