fix(net): a second proxy stops corrupting the first - #1022
Merged
Conversation
Benchmark Results for generalComparing to e4bbcc2 |
andrewgazelka
force-pushed
the
fix/multiple-proxies
branch
from
July 28, 2026 10:49
45172df to
6b28478
Compare
Benchmark Results for generalComparing to 22baba4 |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1022 +/- ##
==========================================
+ Coverage 51.49% 52.62% +1.12%
==========================================
Files 337 337
Lines 30574 30819 +245
Branches 1167 1194 +27
==========================================
+ Hits 15743 16217 +474
+ Misses 14580 14339 -241
- Partials 251 263 +12
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
andrewgazelka
force-pushed
the
fix/multiple-proxies
branch
2 times, most recently
from
July 28, 2026 11:12
36ac8e1 to
4ca9bd8
Compare
Three defects that are invisible with exactly one proxy and constant with two. All three were diagnosed by TestingPlant in #951, in October 2025. That PR was written against the Bevy main from #882, which #956 reverted to flecs four weeks later, so its hunks reference `Res<'_, Compose>`, `get_resource_mut::<StreamLookup>` and `#[derive(Event)]`, and its target crate `hyperion-proto` was renamed in #1013. This is a reimplementation of the same three fixes against flecs main, not a rebase. The reading was right; only the code had nowhere left to land. Stream ids collided across proxies. Each `hyperion-proxy` numbers its own players from a local `player_id_on = 1`, reset on every reconnect, so proxy-0's player 1 and proxy-1's player 1 were one key in `StreamLookup`. The second connect overwrote the first, the first disconnect destructed the wrong entity, and the second panicked inside a command closure. `StreamLookup` is now keyed on `ConnectionId`, which carries the proxy id. A proxy's death now drops its lookup entries too: no `PlayerDisconnect` is coming for those players, and the entries outlived the entities. Positions desynchronised per proxy. `UpdatePlayerPositions` carried two parallel `Vec`s; `transform_for_proxy` filtered the streams to the receiving proxy and shipped the positions whole, and the proxy zipped them. Every player after the first filtered-out one landed at somebody else's chunk in the BVH, so regional broadcasts reached the wrong people. It is now one `Vec<UpdatePlayerPosition>`, which makes the desync unrepresentable rather than merely fixed. Channel subscribes fanned out. `SubscribeChannelPackets` was listed in `affected_by_proxy` but transformed unconditionally, so a subscribe from a player on proxy-0 was delivered to proxy-1 as well. The asking proxy now rides on `RequestSubscribeChannelPackets` and the reply is filtered on it. Also: a protocol-version mismatch was a `warn!` and a seat on the server. It is now a refusal on the login path, carrying a `login_disconnect` that names both versions, so the player learns what to run rather than failing later against a codec that does not match. Co-authored-by: TestingPlant <TestingPlant@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
andrewgazelka
force-pushed
the
fix/multiple-proxies
branch
from
July 28, 2026 11:16
4ca9bd8 to
77c8184
Compare
Benchmark Results for generalComparing to 3be8426 |
Benchmark Results for generalComparing to 3be8426 |
Benchmark Results for generalComparing to 87c939a |
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.
Three defects that are invisible with exactly one proxy and constant with two, plus a protocol-version refusal.
Credit
All three multi-proxy defects were diagnosed by @TestingPlant in #951, in October 2025. The reading was correct and the fixes below are the ones that PR proposed. Its code had nowhere left to land: it was written against the Bevy main from #882, which #956 reverted to flecs four weeks later, so its hunks reference
Res<'_, Compose>,get_resource_mut::<StreamLookup>()and#[derive(Event)]— none of which exist. It also targetscrates/hyperion-proto, renamed tohyperion-proxy-protoin #1013. This is a reimplementation against flecs main, not a rebase. #951 is being closed with an explanation rather than silently.Stream ids collided across proxies
Each
hyperion-proxynumbers its own players from a locallet mut player_id_on = 1, reset on every reconnect to the game server, and knows nothing of the others.StreamLookupwasFxHashMap<u64, Entity>, so proxy-0's player 1 and proxy-1's player 1 were one key. The second connect silently overwrote the first; the first disconnect destructed the wrong player's entity through a.expect(...), and the second found nothing and panicked inside a command-channel closure.StreamLookupis now keyed onConnectionId, which carries the proxy id.While here: a proxy's death now drops its lookup entries as well as its entities. No
PlayerDisconnectis coming for those players — the connection that would have carried it is the one that died — so the entries outlived the entities they pointed at.Positions desynchronised per proxy
UpdatePlayerPositionscarried two parallelVecs.transform_for_proxyfilteredstreamto the receiving proxy's players and shippedpositionswhole, and the proxy zipped them incache.rs. Every player after the first filtered-out one landed at somebody else's chunk in the regional-broadcast BVH.It is now one
Vec<UpdatePlayerPosition>, one entry per player, on both sides of the wire. That makes the desync unrepresentable rather than patching the instance — a filter over a list of pairs cannot separate a stream from its position.Channel subscribes fanned out to every proxy
SubscribeChannelPacketswas listed inaffected_by_proxy()buttransform_for_proxyreturnedSome(...)for it unconditionally, so a subscribe requested by a player on proxy-0 was also delivered to proxy-1, subscribing its players to a channel nobody had asked them about.RequestSubscribeChannelPacketsnow carries the proxy that asked,intermediate::SubscribeChannelPacketscarries it asreceiver, and the transform returnsNonefor every other proxy. (The wireSubscribeChannelPacketsdoes not carry it: the proxy that receives the message is the receiver, so putting it on the wire would be a second copy of a fact the delivery already establishes.)A protocol mismatch is now a refusal
pre_play.rslogged awarn!and admitted the client. The server speaks exactly protocol 776. A mismatch on the login path now sends alogin_disconnectnaming both versions and then shuts the connection down. Status pings are still answered on any version, because answering the ping is how a client learns which version to be.Tests
Two proxies, throughout.
crates/hyperion/tests/multi_proxy.rsis the widest one: it drives the real proxy-side consumer, so the game server'stransform_for_proxyproducing the bytes andhyperion-proxy'sBufferedEgressconsuming them are checked against each other rather than each against its own idea of the format.net::proxy::tests::colliding_stream_ids_on_two_proxies_are_two_playersruns the realhandle_proxy_messagestwice, on two duplex streams, with both proxies handing the server a player numbered 1.multi_proxy::a_regional_broadcast_reaches_the_players_who_are_actually_thereputs four players interleaved across two proxies, transforms for one, rkyv-encodes, feeds the proxy'sBufferedEgress, and broadcasts locally.net::tests::{shutdown_reaches_one_proxy_as_a_shutdown, a_subscribe_is_delivered_only_to_the_asking_proxy}register twoEgressCommchannels on oneComposeand decode what actually landed on each.net::intermediate::tests::{each_proxy_sees_its_own_players_at_their_own_positions, a_subscribe_reaches_only_the_proxy_that_asked}.net::protocol::pre_play::tests::a_refused_client_is_told_why.Broken each guard and watched it fail
Reverting
StreamLookupto the bareu64key:Restoring the old zip semantics (streams filtered, positions taken in order from the unfiltered list):
Dropping the
receiver != proxy_idcheck:Making the refusal a no-op that reports success, as the
warn!did:All restored; all green.
What this does not do
There is no gate that starts two
hyperion-proxyprocesses.nix/e2e.nix's driver starts exactly one and is shared by every existing e2e gate, so a second proxy there is a change to shared infrastructure rather than to this fix. The tests above put two proxy identities through the real code on both sides of the wire — including the realhandle_proxy_messagesand the realBufferedEgress— but not through two sockets. Worth filing separately.Checks
cargo fmt --all,cargo clippy --workspace --all-targets, andcargo test --workspaceall clean locally.Adversarial review pass
A read-only reviewer went over this diff. Three findings were worth acting on; one of them says a guard I had already "broken and watched fail" was not actually checking what its comment claimed.
The
pre_playtest was tautological. It assertedpayload[1] == LoginDisconnect.to_raw()and claimed to be checking that the refusal goes out uncompressed. It was not: an uncompressed frame is[len][id][body]and a compressed-under-threshold frame is[len][data_len=0][id][body], so byte 1 is0either way — andLoginDisconnectis itself id 0. Swappingsend_uncompressedforsendleft the test green while a real client, which has not been sentlogin_compressionyet, would read the data-length byte as the packet id and desync.It now compares the whole frame against
encode_packet_no_compressionof the same packet. Broken by making the swap:That extra byte is the whole bug. Worth recording that the break-it step is what caught this: the earlier break only replaced the send with a no-op, which any assertion would have caught.
affected_by_proxyis now pinned againsttransform_for_proxy. The two are hand-maintained duplicates of the same fact and nothing checked they agree — and the mismatch is quiet rather than loud, becauseadd_proxy_message's "not affected" branch encodes once againstProxyId::new(0), which is the real id of the first proxy to connect. A variant that does depend on the proxy but is listed as not would route everything as proxy 0. Given this PR's whole subject is a variant listed correctly and transformed wrongly, the inverse deserved a guard. Broken by movingUnicastinto thefalsearm:Send-then-shutdown is safe here, and only here.
Shutdownreaches the proxy asPlayerHandle::shutdown, which callskanal::Sender::close()— and that ends ininternal.queue.clear(), so queued bytes are discarded rather than flushed. This PR adds the first path that tells a client why and then disconnects it. It is safe because a brand-new connection's writer task is parked inrecv(), so the reason is handed straight across and never queues. It would not be safe for an in-play kick with a reason. The constraint is now written at the call site, and the proxy-side fix is ENG-10895.Two things the reviewer found that are outside this change and were filed rather than fixed:
ingress::PendingRemoveis a dead marker component whose system destructs entities without clearingStreamLookup. Nothing adds it today. If revived, the stale entry means the laterPlayerDisconnectdoes not hit its.expect(...)and instead destructs a recycled entity id — the same shape as the bug fixed here.One finding deliberately not acted on:
UpdatePlayerPositionsgrew from 12 to 16 bytes per player on the wire, because rkyv pads the archived struct to align 8. That is the price of making the desync unrepresentable, and it is the trade this PR is arguing for.