fix: handle multiple proxies - #951
Open
TestingPlant wants to merge 3 commits into
Open
Conversation
Benchmark Results for generalComparing to 313503c |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #951 +/- ##
==========================================
+ Coverage 23.22% 23.24% +0.01%
==========================================
Files 157 157
Lines 15494 15532 +38
Branches 486 489 +3
==========================================
+ Hits 3599 3610 +11
- Misses 11794 11821 +27
Partials 101 101
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
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
added a commit
that referenced
this pull request
Jul 28, 2026
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
added a commit
that referenced
this pull request
Jul 28, 2026
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
added a commit
that referenced
this pull request
Jul 28, 2026
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
added a commit
that referenced
this pull request
Jul 28, 2026
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 `Vec`s. `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-10896** — `ingress::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. Co-authored-by: TestingPlant <TestingPlant@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
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.
This adds some fixes to support multiple proxies properly.