Conversation
# Conflicts: # bridge-core/src/main/java/gg/modl/minecraft/bridge/reporter/ModlBackendReplayUploader.java # platforms/spigot/src/main/java/gg/modl/minecraft/spigot/bridge/BridgeComponent.java # platforms/spigot/src/test/java/gg/modl/minecraft/spigot/bridge/BridgeComponentTest.java
cleanup 1
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static boolean shouldReconnectAfterClose(int code) { | ||
| return code != 1008 && code != 1013; | ||
| } |
There was a problem hiding this comment.
This treats close code 1013 as a terminal close. That code means the server is temporarily unavailable and asks clients to try again later, so a backend deploy or overload response will set running = false in handleClose and realtime invalidations will never resume until the plugin restarts. This should reconnect with backoff instead of stopping the client.
| static boolean shouldReconnectAfterClose(int code) { | |
| return code != 1008 && code != 1013; | |
| } | |
| static boolean shouldReconnectAfterClose(int code) { | |
| return code != 1008; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/src/main/java/gg/modl/minecraft/core/realtime/MinecraftRealtimeClient.java
Line: 278-280
Comment:
**Reconnect stops permanently**
This treats close code `1013` as a terminal close. That code means the server is temporarily unavailable and asks clients to try again later, so a backend deploy or overload response will set `running = false` in `handleClose` and realtime invalidations will never resume until the plugin restarts. This should reconnect with backoff instead of stopping the client.
```suggestion
static boolean shouldReconnectAfterClose(int code) {
return code != 1008;
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (!localEnabled || startupResponse == null) return false; | ||
| if (!Boolean.TRUE.equals(startupResponse.getRealtimeEnabled())) return false; | ||
| String url = startupResponse.getRealtimeUrl(); | ||
| if (url == null || url.trim().isEmpty()) return false; | ||
| return !parseTopics(startupResponse.getRealtimeTopics(), normalize(startupResponse.getServerInstanceId())).isEmpty(); |
There was a problem hiding this comment.
canStart accepts any non-empty realtime URL, but connectOnce passes that URI directly to WebSocketClient. If startup returns an https:// or http:// endpoint by mistake, the realtime client starts and then fails/retries forever instead of rejecting the startup payload and staying cleanly on polling. Validate ws/wss here before starting the client.
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/src/main/java/gg/modl/minecraft/core/realtime/MinecraftRealtimeClient.java
Line: 88-92
Comment:
**Validate realtime scheme**
`canStart` accepts any non-empty realtime URL, but `connectOnce` passes that URI directly to `WebSocketClient`. If startup returns an `https://` or `http://` endpoint by mistake, the realtime client starts and then fails/retries forever instead of rejecting the startup payload and staying cleanly on polling. Validate `ws`/`wss` here before starting the client.
How can I resolve this? If you propose a fix, please make it concise.| @SuppressWarnings("unchecked") | ||
| public static Map<String, Object> loadMap(Path file) throws IOException { | ||
| try (InputStream input = Files.newInputStream(file)) { | ||
| Object loaded = new Yaml().load(input); | ||
| if (loaded == null) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| return (Map<String, Object>) loaded; | ||
| } | ||
| } |
There was a problem hiding this comment.
This unchecked cast lets a scalar or list YAML root throw ClassCastException. BridgeConfig.load is called from bridge startup under a catch that only handles IOException, so a malformed bridge-config.yml can escape the fallback path and abort bridge initialization instead of logging and using defaults. Check the loaded root type and return an empty map or throw IOException for unsupported roots.
| @SuppressWarnings("unchecked") | |
| public static Map<String, Object> loadMap(Path file) throws IOException { | |
| try (InputStream input = Files.newInputStream(file)) { | |
| Object loaded = new Yaml().load(input); | |
| if (loaded == null) { | |
| return Collections.emptyMap(); | |
| } | |
| return (Map<String, Object>) loaded; | |
| } | |
| } | |
| @SuppressWarnings("unchecked") | |
| public static Map<String, Object> loadMap(Path file) throws IOException { | |
| try (InputStream input = Files.newInputStream(file)) { | |
| Object loaded = new Yaml().load(input); | |
| if (loaded == null) { | |
| return Collections.emptyMap(); | |
| } | |
| if (!(loaded instanceof Map)) { | |
| throw new IOException("YAML root must be a map: " + file); | |
| } | |
| return (Map<String, Object>) loaded; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bridge-core/src/main/java/gg/modl/minecraft/bridge/resource/BridgeYamlResource.java
Line: 30-39
Comment:
**Malformed config crashes**
This unchecked cast lets a scalar or list YAML root throw `ClassCastException`. `BridgeConfig.load` is called from bridge startup under a catch that only handles `IOException`, so a malformed `bridge-config.yml` can escape the fallback path and abort bridge initialization instead of logging and using defaults. Check the loaded root type and return an empty map or throw `IOException` for unsupported roots.
```suggestion
@SuppressWarnings("unchecked")
public static Map<String, Object> loadMap(Path file) throws IOException {
try (InputStream input = Files.newInputStream(file)) {
Object loaded = new Yaml().load(input);
if (loaded == null) {
return Collections.emptyMap();
}
if (!(loaded instanceof Map)) {
throw new IOException("YAML root must be a map: " + file);
}
return (Map<String, Object>) loaded;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| new ThreadPoolExecutor.CallerRunsPolicy() | ||
| ); |
There was a problem hiding this comment.
The lookup pool uses a bounded queue with CallerRunsPolicy, so once two lookups are running and 64 are queued, the login caller performs lookupRemote itself. That method can spend up to five seconds on connect and five seconds on read, which means a login event thread or login executor worker can block on public IP enrichment instead of failing that optional enrichment fast. Under a join burst or slow ipwho.is, login verification can stall even though this code was moved behind an executor.
| new ThreadPoolExecutor.CallerRunsPolicy() | |
| ); | |
| new ThreadPoolExecutor.AbortPolicy() | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/src/main/java/gg/modl/minecraft/core/util/IpApiClient.java
Line: 195-196
Comment:
**Caller thread blocks**
The lookup pool uses a bounded queue with `CallerRunsPolicy`, so once two lookups are running and 64 are queued, the login caller performs `lookupRemote` itself. That method can spend up to five seconds on connect and five seconds on read, which means a login event thread or login executor worker can block on public IP enrichment instead of failing that optional enrichment fast. Under a join burst or slow `ipwho.is`, login verification can stall even though this code was moved behind an executor.
```suggestion
new ThreadPoolExecutor.AbortPolicy()
);
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -93,13 +103,15 @@ private void onPlayerJoin(ServerPlayerEntity player) { | |||
| PlayerLoginRequest request = new PlayerLoginRequest( | |||
| uuid.toString(), playerName, | |||
| ipAddress, skinHash, platform.getServerName(), ipInfo); | |||
| request.setServerInstanceId(StartupClient.getServerInstanceId()); | |||
| return new Object[]{request, ipInfo, skinHash}; | |||
| }).thenCompose(data -> { | |||
| PlayerLoginRequest request = (PlayerLoginRequest) data[0]; | |||
| @SuppressWarnings("unchecked") | |||
| Map<String, Object> ipInfo = (Map<String, Object>) data[1]; | |||
| String skinHash = (String) data[2]; | |||
| return getHttpClient().playerLogin(request) | |||
| .orTimeout(LOGIN_TIMEOUT_SECONDS, TimeUnit.SECONDS) | |||
| .thenAccept(response -> { | |||
| loginCache.cacheLoginResult(uuid, response, ipInfo, skinHash); | |||
| ListenerHelper.handlePendingIpLookups( | |||
| @@ -114,39 +126,45 @@ private void onPlayerJoin(ServerPlayerEntity player) { | |||
| LoginHandler.LoginResult errorResult = LoginHandler.handleLoginError(error); | |||
| if (errorResult instanceof LoginHandler.LoginResult.Denied) { | |||
| LoginHandler.LoginResult.Denied denied = (LoginHandler.LoginResult.Denied) errorResult; | |||
| server.execute(() -> platform.kickPlayer( | |||
| platform.getAbstractPlayer(uuid, false), denied.getMessage())); | |||
| kickForLoginFailure(uuid, denied.getMessage()); | |||
| } else { | |||
| completeJoin(uuid, playerName, null); | |||
| kickForLoginFailure(uuid, "Unable to verify ban status. Login temporarily restricted for safety."); | |||
| } | |||
| return null; | |||
| }).get(LOGIN_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS); | |||
| } catch (Exception e) { | |||
| platform.getLogger().warning("Async login timed out for " + playerName + ": " + e.getMessage()); | |||
| completeJoin(uuid, playerName, null); | |||
| } | |||
| }); | |||
| }); | |||
| }); | |||
There was a problem hiding this comment.
The executor task only creates the CompletableFuture chain and then returns. The IP lookup, Mojang lookup, backend login request, and kick/complete callbacks keep running on other executors and are not tracked by this listener, so the new queue and shutdown() do not limit or cancel active login checks. A burst of joins can still create many concurrent backend login requests, and a plugin shutdown can still leave callbacks kicking or completing players through disabled services. Keep the whole login check inside the executor task, or store and cancel the returned future on shutdown.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platforms/fabric-121/src/main/java/gg/modl/minecraft/fabric/v1_21_1/FabricListener.java
Line: 89-135
Comment:
**Login work escapes executor**
The executor task only creates the `CompletableFuture` chain and then returns. The IP lookup, Mojang lookup, backend login request, and kick/complete callbacks keep running on other executors and are not tracked by this listener, so the new queue and `shutdown()` do not limit or cancel active login checks. A burst of joins can still create many concurrent backend login requests, and a plugin shutdown can still leave callbacks kicking or completing players through disabled services. Keep the whole login check inside the executor task, or store and cancel the returned future on shutdown.
How can I resolve this? If you propose a fix, please make it concise.
| private static Set<Topic> allowedStartupTopics() { | ||
| Set<Topic> topics = new HashSet<>(); | ||
| topics.add(Topic.TOPIC_MINECRAFT_PERMISSIONS); | ||
| topics.add(Topic.TOPIC_MINECRAFT_PUNISHMENT_TYPES); | ||
| topics.add(Topic.TOPIC_MINECRAFT_STAFF_NOTIFICATIONS); | ||
| topics.add(Topic.TOPIC_MINECRAFT_PUNISHMENTS); | ||
| topics.add(Topic.TOPIC_MINECRAFT_PLAYER_NOTIFICATIONS); | ||
| topics.add(Topic.TOPIC_MINECRAFT_STAFF_2FA); | ||
| topics.add(Topic.TOPIC_MINECRAFT_MIGRATION_TASKS); | ||
| topics.add(Topic.TOPIC_MINECRAFT_STAT_WIPES); | ||
| return topics; |
There was a problem hiding this comment.
This allowlist filters the startup topics before buildSubscribe() sends the subscription, but it omits the active-staff topic even though this client now handles ACTIVE_STAFF_PUSH. If the backend advertises active staff updates, they are dropped by parseTopics() and the websocket never subscribes to them. With the periodic poll removed while realtime is connected, staff cache and permission state can stay stale until a reconnect or baseline fetch.
Rule Used: THE CENTRAL RULE everything else serves: maintain ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/src/main/java/gg/modl/minecraft/core/realtime/MinecraftRealtimeClient.java
Line: 474-484
Comment:
**Active staff unsubscribed**
This allowlist filters the startup topics before `buildSubscribe()` sends the subscription, but it omits the active-staff topic even though this client now handles `ACTIVE_STAFF_PUSH`. If the backend advertises active staff updates, they are dropped by `parseTopics()` and the websocket never subscribes to them. With the periodic poll removed while realtime is connected, staff cache and permission state can stay stale until a reconnect or baseline fetch.
**Rule Used:** THE CENTRAL RULE everything else serves: maintain ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=21719055-65fa-440f-b434-eba8d358d917))
How can I resolve this? If you propose a fix, please make it concise.| public void applyActiveStaffMember(SyncResponse.ActiveStaffMember staffMember) { | ||
| processActiveStaffMember(staffMember); |
There was a problem hiding this comment.
The baseline sync treats activeStaffMembers as the authoritative full set and evicts cached staff profiles that are no longer present, but the realtime push path only updates each pushed member. When a staff member is removed, loses staff state, or disappears from the active set while the websocket is connected, this method has no matching eviction step, so profile.getStaffMember() and cached staff permissions can remain set until the next reconnect baseline.
Rule Used: THE CENTRAL RULE everything else serves: maintain ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/src/main/java/gg/modl/minecraft/core/service/sync/SyncService.java
Line: 609-610
Comment:
**Staff removals persist**
The baseline sync treats `activeStaffMembers` as the authoritative full set and evicts cached staff profiles that are no longer present, but the realtime push path only updates each pushed member. When a staff member is removed, loses staff state, or disappears from the active set while the websocket is connected, this method has no matching eviction step, so `profile.getStaffMember()` and cached staff permissions can remain set until the next reconnect baseline.
**Rule Used:** THE CENTRAL RULE everything else serves: maintain ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=21719055-65fa-440f-b434-eba8d358d917))
How can I resolve this? If you propose a fix, please make it concise.1 participant
Confidence Score: 3/5
This should be fixed before merging.
Focus on
MinecraftRealtimeClient.javaandSyncService.java.Important Files Changed
Comments Outside Diff (1)
core/src/main/java/gg/modl/minecraft/core/impl/http/ModlHttpClientV2Impl.java, line 824-827 (link)When debug mode is enabled this logs the full header map, including
X-API-Key. Server logs are commonly shared for support, so this can expose the API key for every request. Log a redacted copy of the headers instead of the raw request headers.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "realtime push consume + proto v3 client" | Re-trigger Greptile
Context used: