diff --git a/core/src/main/java/com/minekube/connect/watch/SessionProposal.java b/core/src/main/java/com/minekube/connect/watch/SessionProposal.java index a4eafe5f3..309c43882 100644 --- a/core/src/main/java/com/minekube/connect/watch/SessionProposal.java +++ b/core/src/main/java/com/minekube/connect/watch/SessionProposal.java @@ -105,6 +105,17 @@ public boolean hasBedrockPrincipalV2() { return bedrockPrincipalV2; } + /** + * The id of the proposed session, as a plain string. + * + *
Protobuf is an {@code implementation} dependency of this module, so platform modules
+ * (Spigot, Velocity, ...) cannot resolve the generated types at compile time. Anything they need
+ * to log or correlate has to be exposed here rather than through {@link #getSession()}.
+ */
+ public String getSessionId() {
+ return session == null ? "" : session.getId();
+ }
+
@Override
public String toString() {
return "SessionProposal[sessionId=" + (session == null ? "" : session.getId())
diff --git a/spigot/src/main/java/com/minekube/connect/addon/data/SpigotDataAddon.java b/spigot/src/main/java/com/minekube/connect/addon/data/SpigotDataAddon.java
index ac480ae76..97dffe7c3 100644
--- a/spigot/src/main/java/com/minekube/connect/addon/data/SpigotDataAddon.java
+++ b/spigot/src/main/java/com/minekube/connect/addon/data/SpigotDataAddon.java
@@ -52,13 +52,23 @@ public void onInject(Channel channel, boolean toServer) {
LocalSession.context(channel, ctx -> {
// we have to add the packet blocker in the data handler, otherwise ProtocolSupport breaks
channel.pipeline().addBefore(
- packetHandlerName, "connect_data_handler",
+ packetHandlerName, SpigotDataHandler.HANDLER_NAME,
new SpigotDataHandler(ctx,
packetHandlerName,
config,
logger,
bedrockIdentityEnforcer)
);
+ // Only a session where Connect owns the login (i.e. not passthrough) is injected here
+ // at all, which is exactly the case the stall watchdog can attribute.
+ if (channel.pipeline().get(SpigotLoginStallWatchdog.HANDLER_NAME) == null) {
+ SpigotLoginStallWatchdog.install(channel.pipeline(), new SpigotLoginStallWatchdog(
+ logger,
+ playerName(ctx),
+ sessionId(ctx),
+ ctx.getEndpointId()
+ ));
+ }
if (channel.pipeline().get(SpigotChatSessionPacketFilter.HANDLER_NAME) == null) {
channel.pipeline().addBefore(
packetHandlerName,
@@ -69,6 +79,19 @@ public void onInject(Channel channel, boolean toServer) {
});
}
+ /**
+ * The identity the connector logs for a session: the signed Bedrock principal is never rendered.
+ */
+ private static String playerName(LocalSession.Context ctx) {
+ return ctx.getSessionProposal().hasBedrockPrincipalV2()
+ ? " A packet-level login plugin (PacketEvents-style) sits upstream of this pipeline
+ * position and clears the frame buffer when it cancels a packet, so {@code LOGIN_START} can vanish
+ * before the connector's own data handler sees it. That handler's fake offline-login cycle is
+ * driven from a single site inside its {@code LOGIN_START} branch, so it then never runs: the
+ * client receives nothing and the only line anywhere is the server's own cause-blind
+ * {@code Took too long to log in}. This watchdog sees the same packets the connector's login path
+ * sees and, when a handshake was observed but no {@code LOGIN_START} follows within
+ * {@link #LOGIN_START_WINDOW_MILLIS}, emits exactly one default-verbosity line naming the session
+ * and the probable cause.
+ *
+ * Log-first by design: this handler never consumes, replaces, delays or fabricates a packet and
+ * never closes a channel, so a healthy session is unaffected. A kick that names the cause would
+ * change user-visible behaviour and is deliberately not part of this change.
+ *
+ * Packets are identified by class name - the convention introduced by
+ * {@link SpigotChatSessionPacketFilter} - instead of the server-internals reflection in
+ * {@link com.minekube.connect.util.ClassNames}, which keeps this handler independent of the running
+ * server version and unit-testable with an {@code EmbeddedChannel}. The names mirror the candidates
+ * {@code ClassNames} resolves for its {@code HANDSHAKE_PACKET} and {@code LOGIN_START_PACKET}; keep
+ * both in sync when a Minecraft version moves them again.
+ *
+ * Only non-passthrough sessions reach this handler: for passthrough (offline-mode) endpoints
+ * Connect deliberately injects no addons at all, and it owns no login decision to attribute there.
+ */
+final class SpigotLoginStallWatchdog extends ChannelInboundHandlerAdapter {
+ static final String HANDLER_NAME = "connect_login_stall_watchdog";
+
+ /**
+ * Bounded window after the handshake in which {@code LOGIN_START} is expected. Chosen to land
+ * well before the server's own login timeout (~30 s / 600 ticks) so the cause-naming line is the
+ * first one the operator reads, and far above the milliseconds a healthy handshake-to-login-start
+ * gap takes.
+ */
+ static final long LOGIN_START_WINDOW_MILLIS = Duration.ofSeconds(10).toMillis();
+
+ // Mojang-mapped names on 1.20.5+, obfuscated fallbacks before that (see ClassNames).
+ private static final String HANDSHAKE_PACKET =
+ "net.minecraft.network.protocol.handshake.ClientIntentionPacket";
+ private static final String LEGACY_HANDSHAKE_PACKET = "PacketHandshakingInSetProtocol";
+ private static final String LOGIN_START_PACKET =
+ "net.minecraft.network.protocol.login.ServerboundHelloPacket";
+ private static final String LEGACY_LOGIN_START_PACKET = "PacketLoginInStart";
+
+ private final ConnectLogger logger;
+ private final String playerName;
+ private final String sessionId;
+ private final String endpointId;
+
+ private ScheduledFuture> deadline;
+ private boolean handshakeSeen;
+ private boolean loginStartSeen;
+ private boolean reported;
+ private int packetsSeen;
+
+ SpigotLoginStallWatchdog(
+ ConnectLogger logger,
+ String playerName,
+ String sessionId,
+ String endpointId) {
+ this.logger = logger;
+ this.playerName = playerName;
+ this.sessionId = sessionId;
+ this.endpointId = endpointId;
+ }
+
+ /**
+ * Installs the watchdog at its production position: immediately upstream of the
+ * connector's {@code connect_data_handler}.
+ *
+ * The position is the point of this handler. {@link SpigotDataHandler} consumes
+ * {@code LOGIN_START} (it does not forward it), so a watchdog installed downstream of it would
+ * miss every healthy login and report a stall for each session. Anchoring on the data handler's
+ * name keeps the order correct regardless of the order of the surrounding pipeline edits.
+ */
+ static void install(ChannelPipeline pipeline, SpigotLoginStallWatchdog watchdog) {
+ pipeline.addBefore(SpigotDataHandler.HANDLER_NAME, HANDLER_NAME, watchdog);
+ }
+
+ @Override
+ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
+ packetsSeen++;
+ if (isPacket(msg, HANDSHAKE_PACKET, LEGACY_HANDSHAKE_PACKET)) {
+ armDeadline(ctx);
+ } else if (isPacket(msg, LOGIN_START_PACKET, LEGACY_LOGIN_START_PACKET)) {
+ loginStartSeen = true;
+ cancelDeadline();
+ }
+ super.channelRead(ctx, msg);
+ }
+
+ @Override
+ public void channelInactive(ChannelHandlerContext ctx) throws Exception {
+ cancelDeadline();
+ super.channelInactive(ctx);
+ }
+
+ @Override
+ public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
+ cancelDeadline();
+ super.handlerRemoved(ctx);
+ }
+
+ private void armDeadline(ChannelHandlerContext ctx) {
+ handshakeSeen = true;
+ if (deadline != null) {
+ return; // one window per session
+ }
+ deadline = ctx.executor().schedule(
+ this::reportMissingLoginStart, LOGIN_START_WINDOW_MILLIS, TimeUnit.MILLISECONDS);
+ }
+
+ private void cancelDeadline() {
+ if (deadline != null) {
+ deadline.cancel(false);
+ deadline = null;
+ }
+ }
+
+ /**
+ * Runs on the session's event loop once the window elapsed without a {@code LOGIN_START}.
+ */
+ private void reportMissingLoginStart() {
+ if (!handshakeSeen || loginStartSeen || reported) {
+ return;
+ }
+ reported = true;
+ deadline = null;
+ logger.warn(
+ "Connect tunneled login stalled: no LOGIN_START reached the connector within {} ms"
+ + " player={} session={} endpoint={} packets={} - probable cause: a"
+ + " packet-level login plugin in the pipeline consumed the login start"
+ + " before connect_data_handler, so Connect's offline-login cycle never ran"
+ + " and the client is left without a login response; note: connect-player"
+ + " never fires for passthrough sessions, so a connect-player exemption is"
+ + " not a fix on an offline-mode endpoint",
+ LOGIN_START_WINDOW_MILLIS, playerName, sessionId, endpointId, packetsSeen);
+ }
+
+ private static boolean isPacket(Object msg, String modernName, String legacyName) {
+ if (msg == null) {
+ return false;
+ }
+ String className = msg.getClass().getName();
+ return className.equals(modernName) || className.equals(legacyName)
+ || className.endsWith("." + legacyName);
+ }
+}
diff --git a/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java b/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java
index 79bdaf822..e62772d90 100644
--- a/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java
+++ b/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java
@@ -169,6 +169,17 @@ public void onAdd(Object object) {
return false;
}
+ /**
+ * Whether Connect injects its addons for - and so owns the login of - the given session.
+ *
+ * Passthrough (offline-mode) endpoints are deliberately left alone: Connect owns no login
+ * decision there, so nothing it adds (including the login-stall watchdog) can observe such a
+ * session, and the {@code connect-player} channel attribute is never set for it either.
+ */
+ static boolean injectsAddons(LocalSession.Context context) {
+ return !context.getPlayer().getAuth().isPassthrough();
+ }
+
public void injectClient(ChannelFuture future) {
try { // fix for reloads
future.channel().pipeline().remove("connect-init");
@@ -185,7 +196,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
@Override
public void channelActive(ChannelHandlerContext childCtx)
throws Exception {
- if (context.getPlayer().getAuth().isPassthrough()) {
+ if (!injectsAddons(context)) {
if (bedrockIdentityEnforcer != null && packetHandlerName != null) {
addPassthroughAdmissionHandler(childCtx.channel(), context);
trackPassthroughClient(childCtx.channel());
diff --git a/spigot/src/test/java/com/minekube/connect/addon/data/SpigotLoginStallWatchdogTest.java b/spigot/src/test/java/com/minekube/connect/addon/data/SpigotLoginStallWatchdogTest.java
new file mode 100644
index 000000000..a56e85227
--- /dev/null
+++ b/spigot/src/test/java/com/minekube/connect/addon/data/SpigotLoginStallWatchdogTest.java
@@ -0,0 +1,227 @@
+package com.minekube.connect.addon.data;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.minekube.connect.api.logger.ConnectLogger;
+import com.minekube.connect.util.MessageFormatter;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.channel.embedded.EmbeddedChannel;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+import net.minecraft.network.protocol.handshake.ClientIntentionPacket;
+import net.minecraft.network.protocol.login.ServerboundHelloPacket;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Guard for the connector-side attribution of a tunneled login that never completes.
+ *
+ * Measured shape of the defect (Paper 26.3 + connect-spigot 0.15.15 + a PacketEvents listener
+ * that cancels {@code LOGIN_START}): the handshake reaches the connector's pipeline position,
+ * {@code LOGIN_START} never does, so the connector's fake offline-login cycle never runs and the
+ * only line anywhere is the server's cause-blind {@code Took too long to log in}. These tests drive
+ * the real {@link SpigotLoginStallWatchdog} through an {@link EmbeddedChannel} at its production
+ * position - with a handler at {@code connect_data_handler} that consumes {@code LOGIN_START}
+ * exactly like {@link SpigotDataHandler} does - so the position, the window and the wording are all
+ * under test, and a healthy login cannot be reported as a stall.
+ */
+class SpigotLoginStallWatchdogTest {
+ private static final String PACKET_HANDLER = "packet_handler";
+ private static final String PLAYER = "coconut";
+ private static final String SESSION = "8b1f2c34-5d6e-4f70-891a-b2c3d4e5f607";
+ private static final String ENDPOINT = "hce2e1535984129";
+
+ private final RecordingLogger logger = new RecordingLogger();
+
+ @Test
+ void watchesEveryLoginStartTheDataHandlerConsumes() {
+ Fixture fixture = new Fixture(logger);
+ List