Skip to content

fix(net): a second proxy stops corrupting the first - #1022

Merged
andrewgazelka merged 1 commit into
mainfrom
fix/multiple-proxies
Jul 28, 2026
Merged

fix(net): a second proxy stops corrupting the first#1022
andrewgazelka merged 1 commit into
mainfrom
fix/multiple-proxies

Conversation

@andrewgazelka

@andrewgazelka andrewgazelka commented Jul 28, 2026

Copy link
Copy Markdown
Member

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 targets crates/hyperion-proto, renamed to hyperion-proxy-proto in #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-proxy numbers its own players from a local let mut player_id_on = 1, reset on every reconnect to the game server, and knows nothing of the others. StreamLookup was FxHashMap<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.

StreamLookup is now keyed on ConnectionId, which carries the proxy id.

While here: a proxy's death now drops its lookup entries as well as its entities. No PlayerDisconnect is 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

UpdatePlayerPositions carried two parallel Vecs. transform_for_proxy filtered stream to the receiving proxy's players and shipped positions whole, and the proxy zipped them in cache.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

SubscribeChannelPackets was listed in affected_by_proxy() but transform_for_proxy returned Some(...) 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.

RequestSubscribeChannelPackets now carries the proxy that asked, intermediate::SubscribeChannelPackets carries it as receiver, and the transform returns None for every other proxy. (The wire SubscribeChannelPackets does 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.rs logged a warn! and admitted the client. The server speaks exactly protocol 776. A mismatch on the login path now sends a login_disconnect naming 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.rs is the widest one: it drives the real proxy-side consumer, so the game server's transform_for_proxy producing the bytes and hyperion-proxy's BufferedEgress consuming 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_players runs the real handle_proxy_messages twice, 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_there puts four players interleaved across two proxies, transforms for one, rkyv-encodes, feeds the proxy's BufferedEgress, and broadcasts locally.
  • net::tests::{shutdown_reaches_one_proxy_as_a_shutdown, a_subscribe_is_delivered_only_to_the_asking_proxy} register two EgressComm channels on one Compose and 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 StreamLookup to the bare u64 key:

test net::proxy::tests::colliding_stream_ids_on_two_proxies_are_two_players ... FAILED
panicked at crates/hyperion/src/net/proxy.rs:555:9:
assertion `left != right` failed: two proxies' player 1 must be two entities, not one
  left: Entity(527)
 right: Entity(527)

Restoring the old zip semantics (streams filtered, positions taken in order from the unfiltered list):

test a_regional_broadcast_reaches_the_players_who_are_actually_there ... FAILED
panicked at crates/hyperion/tests/multi_proxy.rs:126:5:
assertion `left == right` failed: only the player at the broadcast's chunk should have received
it; stream 3 is a hundred chunks away and is only reachable if it inherited another player's position
  left: [1, 3]
 right: [1]

test each_proxy_places_its_own_players ... FAILED
panicked at crates/hyperion/tests/multi_proxy.rs:162:5:
proxy 1's only player is at chunk 100, not at the origin where proxy 0's player is

test net::intermediate::tests::each_proxy_sees_its_own_players_at_their_own_positions ... FAILED
assertion `left == right` failed: player 30 was sent to another player's chunk
  left: 10
 right: 30

Dropping the receiver != proxy_id check:

test net::intermediate::tests::a_subscribe_reaches_only_the_proxy_that_asked ... FAILED
panicked at crates/hyperion/src/net/intermediate.rs:451:9:
a proxy that never asked must not be told to subscribe anyone

test net::tests::a_subscribe_is_delivered_only_to_the_asking_proxy ... FAILED
panicked at crates/hyperion/src/net/mod.rs:837:9:
a proxy that never asked must not be told to subscribe anyone

Making the refusal a no-op that reports success, as the warn! did:

test net::protocol::pre_play::tests::a_refused_client_is_told_why ... FAILED
panicked at crates/hyperion/src/net/protocol/pre_play.rs:715:14:
the refused client's proxy must be sent the reason: Empty

All restored; all green.

What this does not do

There is no gate that starts two hyperion-proxy processes. 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 real handle_proxy_messages and the real BufferedEgress — but not through two sockets. Worth filing separately.

Checks

cargo fmt --all, cargo clippy --workspace --all-targets, and cargo test --workspace all 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_play test was tautological. It asserted payload[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 is 0 either way — and LoginDisconnect is itself id 0. Swapping send_uncompressed for send left the test green while a real client, which has not been sent login_compression yet, would read the data-length byte as the packet id and desync.

It now compares the whole frame against encode_packet_no_compression of the same packet. Broken by making the swap:

test net::protocol::pre_play::tests::a_refused_client_is_told_why ... FAILED
assertion `left == right` failed: the refusal must be the uncompressed login_disconnect frame, byte for byte
  left: [94, 0, 0, 91, 123, 34, 116, 101, 120, 116, ...]
 right: [93, 0, 91, 123, 34, 116, 101, 120, 116, ...]

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_proxy is now pinned against transform_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, because add_proxy_message's "not affected" branch encodes once against ProxyId::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 moving Unicast into the false arm:

test net::intermediate::tests::every_variant_transforms_into_its_own_wire_variant ... FAILED
assertion `left == right` failed: Unicast says it does not depend on the proxy, but transforms differently for two of them
  left: Some("Unicast")
 right: None

Send-then-shutdown is safe here, and only here. Shutdown reaches the proxy as PlayerHandle::shutdown, which calls kanal::Sender::close() — and that ends in internal.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 in recv(), 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:

  • ENG-10896ingress::PendingRemove is a dead marker component whose system destructs entities without clearing StreamLookup. Nothing adds it today. If revived, the stale entry means the later PlayerDisconnect does not hit its .expect(...) and instead destructs a recycled entity id — the same shape as the bug fixed here.
  • ENG-10876 — no e2e gate runs two proxy processes, which is why all three defects survived.

One finding deliberately not acted on: UpdatePlayerPositions grew 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.

@github-actions github-actions Bot added the fix label Jul 28, 2026
@github-actions

Copy link
Copy Markdown

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  19.3 ns ...  19.4 ns ]      +0.68%
ray_intersection/aabb_size_1                       [  18.6 ns ...  18.6 ns ]      +0.07%
ray_intersection/aabb_size_10                      [  18.7 ns ...  18.7 ns ]      -0.29%
ray_intersection/ray_distance_1                    [   1.3 ns ...   1.3 ns ]      +0.29%
ray_intersection/ray_distance_5                    [   1.3 ns ...   1.4 ns ]      +6.35%
ray_intersection/ray_distance_20                   [   1.3 ns ...   1.3 ns ]      -0.02%
overlap/no_overlap                                 [  15.9 ns ...  15.9 ns ]      +0.35%
overlap/partial_overlap                            [  15.9 ns ...  15.9 ns ]      -0.11%
overlap/full_containment                           [  14.5 ns ...  14.5 ns ]      +0.24%
point_containment/inside                           [   5.4 ns ...   5.4 ns ]      +0.10%
point_containment/outside                          [   5.7 ns ...   5.7 ns ]      +0.43%
point_containment/boundary                         [   5.4 ns ...   5.4 ns ]      -0.04%

Comparing to e4bbcc2

@andrewgazelka
andrewgazelka force-pushed the fix/multiple-proxies branch from 45172df to 6b28478 Compare July 28, 2026 10:49
@github-actions

Copy link
Copy Markdown

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  18.6 ns ...  18.6 ns ]      +0.06%
ray_intersection/aabb_size_1                       [  19.0 ns ...  19.0 ns ]      -0.32%
ray_intersection/aabb_size_10                      [  18.7 ns ...  18.6 ns ]      -0.31%
ray_intersection/ray_distance_1                    [   1.3 ns ...   1.3 ns ]      +0.32%
ray_intersection/ray_distance_5                    [   1.3 ns ...   1.3 ns ]      -0.03%
ray_intersection/ray_distance_20                   [   1.3 ns ...   1.3 ns ]      -0.10%
overlap/no_overlap                                 [  16.2 ns ...  16.0 ns ]      -1.20%
overlap/partial_overlap                            [  15.9 ns ...  15.9 ns ]      +0.25%
overlap/full_containment                           [  14.6 ns ...  14.6 ns ]      -0.07%
point_containment/inside                           [   5.4 ns ...   5.4 ns ]      -0.18%
point_containment/outside                          [   5.7 ns ...   5.7 ns ]      +0.07%
point_containment/boundary                         [   5.4 ns ...   5.4 ns ]      -0.04%

Comparing to 22baba4

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.46865% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.62%. Comparing base (3be8426) to head (77c8184).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
crates/hyperion/src/net/proxy.rs 85.10% 14 Missing ⚠️
crates/hyperion/src/net/mod.rs 86.36% 9 Missing ⚠️
crates/hyperion/src/net/protocol/pre_play.rs 83.33% 7 Missing and 2 partials ⚠️
crates/hyperion/src/egress/mod.rs 33.33% 4 Missing ⚠️
crates/hyperion/src/net/intermediate.rs 94.73% 2 Missing and 2 partials ⚠️
crates/hyperion/src/egress/channel.rs 50.00% 1 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
crates/hyperion-proxy/src/cache.rs 23.68% <100.00%> (+23.68%) ⬆️
crates/hyperion/src/simulation/lookup.rs 40.74% <ø> (ø)
crates/hyperion/src/egress/channel.rs 26.66% <50.00%> (-0.23%) ⬇️
crates/hyperion/src/egress/mod.rs 66.07% <33.33%> (-3.02%) ⬇️
crates/hyperion/src/net/intermediate.rs 98.04% <94.73%> (+1.71%) ⬆️
crates/hyperion/src/net/mod.rs 33.18% <86.36%> (+21.69%) ⬆️
crates/hyperion/src/net/protocol/pre_play.rs 26.49% <83.33%> (+8.40%) ⬆️
crates/hyperion/src/net/proxy.rs 37.35% <85.10%> (+37.35%) ⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrewgazelka
andrewgazelka force-pushed the fix/multiple-proxies branch 2 times, most recently from 36ac8e1 to 4ca9bd8 Compare July 28, 2026 11:12
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
andrewgazelka force-pushed the fix/multiple-proxies branch from 4ca9bd8 to 77c8184 Compare July 28, 2026 11:16
@andrewgazelka
andrewgazelka merged commit 5a486fd into main Jul 28, 2026
10 of 11 checks passed
@andrewgazelka
andrewgazelka deleted the fix/multiple-proxies branch July 28, 2026 11:17
@github-actions

Copy link
Copy Markdown

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  17.4 ns ...  17.4 ns ]      +0.18%
ray_intersection/aabb_size_1                       [  17.5 ns ...  17.5 ns ]      +0.15%
ray_intersection/aabb_size_10                      [  18.0 ns ...  18.1 ns ]      +0.12%
ray_intersection/ray_distance_1                    [   1.5 ns ...   1.5 ns ]      +0.03%
ray_intersection/ray_distance_5                    [   1.5 ns ...   1.5 ns ]      +0.14%
ray_intersection/ray_distance_20                   [   1.5 ns ...   1.5 ns ]      +0.14%
overlap/no_overlap                                 [  15.6 ns ...  15.6 ns ]      +0.33%
overlap/partial_overlap                            [  15.6 ns ...  15.6 ns ]      +0.15%
overlap/full_containment                           [  14.8 ns ...  14.8 ns ]      +0.00%
point_containment/inside                           [   6.0 ns ...   6.0 ns ]      +0.07%
point_containment/outside                          [   6.1 ns ...   6.1 ns ]      -0.01%
point_containment/boundary                         [   6.1 ns ...   6.1 ns ]      -0.09%

Comparing to 3be8426

@github-actions

Copy link
Copy Markdown

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  24.1 ns ...  24.1 ns ]      -0.25%
ray_intersection/aabb_size_1                       [  24.0 ns ...  24.0 ns ]      -0.05%
ray_intersection/aabb_size_10                      [  24.1 ns ...  24.0 ns ]      -0.25%
ray_intersection/ray_distance_1                    [   0.9 ns ...   0.9 ns ]      -0.01%
ray_intersection/ray_distance_5                    [   0.8 ns ...   0.8 ns ]      -0.10%
ray_intersection/ray_distance_20                   [   0.8 ns ...   0.8 ns ]      -0.07%
overlap/no_overlap                                 [  14.1 ns ...  14.0 ns ]      -0.26%
overlap/partial_overlap                            [  14.1 ns ...  14.1 ns ]      -0.14%
overlap/full_containment                           [  13.5 ns ...  13.5 ns ]      +0.21%
point_containment/inside                           [   6.0 ns ...   6.0 ns ]      +0.05%
point_containment/outside                          [   6.2 ns ...   6.2 ns ]      -0.21%
point_containment/boundary                         [   6.2 ns ...   6.2 ns ]      -0.09%

Comparing to 3be8426

@github-actions

Copy link
Copy Markdown

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  18.7 ns ...  18.7 ns ]      +0.03%
ray_intersection/aabb_size_1                       [  20.6 ns ...  20.6 ns ]      -0.32%
ray_intersection/aabb_size_10                      [  19.0 ns ...  19.0 ns ]      +0.18%
ray_intersection/ray_distance_1                    [   1.3 ns ...   1.3 ns ]      +0.14%
ray_intersection/ray_distance_5                    [   1.3 ns ...   1.3 ns ]      +0.16%
ray_intersection/ray_distance_20                   [   1.3 ns ...   1.3 ns ]      +0.29%
overlap/no_overlap                                 [  15.6 ns ...  15.6 ns ]      +0.01%
overlap/partial_overlap                            [  15.7 ns ...  15.8 ns ]      +0.51%
overlap/full_containment                           [  14.7 ns ...  14.5 ns ]      -1.23%*
point_containment/inside                           [   5.4 ns ...   5.4 ns ]      -0.11%
point_containment/outside                          [   5.7 ns ...   5.7 ns ]      +0.30%
point_containment/boundary                         [   5.4 ns ...   5.4 ns ]      +0.27%

Comparing to 87c939a

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant