fix(proxy): queue play packets when reentering configuration#1791
fix(proxy): queue play packets when reentering configuration#1791AlexProgrammerDE wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Reverts a regression from b1a1b8b by restoring the full play-packet queue (both inbound and outbound) when a player connection reenters configuration state, rather than installing only the outbound queue handler. This ensures inbound play packets from the client are also queued during the transitional reconfiguration period.
Changes:
- In
MinecraftConnection.setState, drop the conditional that installed onlyPlayPacketQueueOutboundHandlerforPLAY → CONFIGtransitions onConnectedPlayerconnections, and unconditionally calladdPlayPacketQueueHandler(). Removes the now-unusedpreviousStatelocal andConnectedPlayerimport. - In
ConnectedPlayer.switchToConfigState, replaceaddPlayPacketQueueOutboundHandler()withaddPlayPacketQueueHandler()so the inbound queue is installed alongside the outbound one when the client is moved into config state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java | Removes the PLAY → CONFIG special case and always installs the full play packet queue when entering CONFIG; cleans up unused import and local. |
| proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java | Switches switchToConfigState from outbound-only queue handler to the full inbound+outbound queue handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
31f2997 to
789a654
Compare
|
not sure why we'd be queing incoming play packets there, sounds like that was always a bad idea, should we not just be discarding them? |
|
For context, #1088 is the PR that originally introduced the packet queue here, e.g. if (state == StateRegistry.CONFIG) {
// Activate the play packet queue
addPlayPacketQueueHandler();
} else if (this.channel.pipeline().get(Connections.PLAY_PACKET_QUEUE) != null) {
// Remove the queue
this.channel.pipeline().remove(Connections.PLAY_PACKET_QUEUE);
}in |
|
I realized what we were doing was unidiomatic and did a big refactor internally now to use Velocity more idiomatically, so this isn't needed for me anymore, so will close. We were chaining velocity but after some consideration, it's not worth the trouble of patching velocity to make it work. We now just use one velocity + a plugin, which is way simpler and better. |
|
Might still be worth looking into - chaining Velocity instances might not be explicitly supported but adding Velocity into the chain should not break the vanilla MC protocol.
|
|
The patch which this is partially reverting fixed a case of chaining, which is why I'm dubious on touching stuff here without further considerations |
|
chaining velocity is itself a very complex issue because of nested configuration phases and our loads of patches were very annoying to maintain. For anyone trying to make this work, I wish you good luck, it's a pretty annoying issue. |
|
Hey @AlexProgrammerDE, would you be able to see if your issue is recreatable with the PR attached to this PR? |
Summary
Restore the full play-packet queue when a player connection reenters configuration state from play state.
This keeps the existing serverbound forwarding guards from the previous reconfiguration change, but avoids switching the client-side connection to outbound-only queueing during reconfiguration. Reentering configuration is a transitional protocol state, so both inbound and outbound play packets should be handled consistently until the configuration acknowledgement completes.
Root cause
The current code installs only
PlayPacketQueueOutboundHandlerwhen a play-state client is moved back into configuration. That protects clientbound play packets, but leaves the inbound side without the normal configuration-state queueing used elsewhere. In nested or rapid server-switch flows, this can leave the switch stuck waiting for reconfiguration to complete.Reconfiguration flow
sequenceDiagram actor Player participant Velocity participant Server as Backend server Player->>Velocity: In PLAY on current backend Server-->>Velocity: New backend login succeeds Velocity->>Player: StartUpdatePacket Note over Player,Velocity: Client starts PLAY to CONFIG reconfiguration Velocity->>Velocity: pendingConfigurationSwitch = true alt Outbound-only queueing Velocity->>Velocity: Install PlayPacketQueueOutboundHandler only Note over Velocity: Clientbound PLAY packets are queued,<br/>but serverbound PLAY packets are not buffered Player-->>Velocity: Serverbound PLAY packet during transition Velocity--xVelocity: Packet reaches the transitional CONFIG path Server--xVelocity: Switch can stall before configuration completes Velocity--xPlayer: Client remains at reconfiguring until timeout else Full play-packet queueing Velocity->>Velocity: Install full play-packet queue Note over Velocity: PLAY packets in both directions are queued,<br/>while CONFIG packets continue through Player->>Velocity: FinishedUpdatePacket Velocity->>Server: Forward FinishedUpdatePacket Velocity->>Velocity: Backend enters CONFIG Server->>Velocity: JoinGame after config completes Velocity->>Player: JoinGame and switch packets Velocity->>Velocity: Remove play-packet queues Player->>Velocity: In PLAY on new backend endValidation
git diff --checkJAVA_HOME=/home/alex/.gradle/jdks/eclipse_adoptium-21-amd64-linux.2 ./gradlew check --console=plainDisclosure
This PR was made by codex to fix a bug I have when configuration transitions happen.
Regression was introduced as part of b1a1b8b.