update to "new" pack requirements

core/src/main/java/org/geysermc/geyser/GeyserImpl.java

@@ -44,9 +44,6 @@ import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.cloudburstmc.protocol.bedrock.codec.BedrockCodec;
 import org.geysermc.api.Geyser;
-import org.geysermc.geyser.api.command.CommandSource;
-import org.geysermc.geyser.api.util.MinecraftVersion;
-import org.geysermc.geyser.api.util.PlatformType;
 import org.geysermc.cumulus.form.Form;
 import org.geysermc.cumulus.form.util.FormBuilder;
 import org.geysermc.erosion.packet.Packets;
@@ -56,12 +53,19 @@ import org.geysermc.floodgate.crypto.Base64Topping;
 import org.geysermc.floodgate.crypto.FloodgateCipher;
 import org.geysermc.floodgate.news.NewsItemAction;
 import org.geysermc.geyser.api.GeyserApi;
+import org.geysermc.geyser.api.command.CommandSource;
 import org.geysermc.geyser.api.event.EventBus;
 import org.geysermc.geyser.api.event.EventRegistrar;
-import org.geysermc.geyser.api.event.lifecycle.*;
+import org.geysermc.geyser.api.event.lifecycle.GeyserPostInitializeEvent;
+import org.geysermc.geyser.api.event.lifecycle.GeyserPostReloadEvent;
+import org.geysermc.geyser.api.event.lifecycle.GeyserPreInitializeEvent;
+import org.geysermc.geyser.api.event.lifecycle.GeyserPreReloadEvent;
+import org.geysermc.geyser.api.event.lifecycle.GeyserShutdownEvent;
 import org.geysermc.geyser.api.network.AuthType;
 import org.geysermc.geyser.api.network.BedrockListener;
 import org.geysermc.geyser.api.network.RemoteServer;
+import org.geysermc.geyser.api.util.MinecraftVersion;
+import org.geysermc.geyser.api.util.PlatformType;
 import org.geysermc.geyser.command.GeyserCommandManager;
 import org.geysermc.geyser.configuration.GeyserConfiguration;
 import org.geysermc.geyser.entity.EntityDefinitions;
@@ -74,6 +78,7 @@ import org.geysermc.geyser.network.GameProtocol;
 import org.geysermc.geyser.network.netty.GeyserServer;
 import org.geysermc.geyser.registry.BlockRegistries;
 import org.geysermc.geyser.registry.Registries;
+import org.geysermc.geyser.registry.loader.ResourcePackLoader;
 import org.geysermc.geyser.registry.provider.ProviderSupplier;
 import org.geysermc.geyser.scoreboard.ScoreboardUpdater;
 import org.geysermc.geyser.session.GeyserSession;
@@ -85,7 +90,13 @@ import org.geysermc.geyser.skin.SkinProvider;
 import org.geysermc.geyser.text.GeyserLocale;
 import org.geysermc.geyser.text.MinecraftLocale;
 import org.geysermc.geyser.translator.text.MessageTranslator;
-import org.geysermc.geyser.util.*;
+import org.geysermc.geyser.util.AssetUtils;
+import org.geysermc.geyser.util.CooldownUtils;
+import org.geysermc.geyser.util.DimensionUtils;
+import org.geysermc.geyser.util.Metrics;
+import org.geysermc.geyser.util.NewsHandler;
+import org.geysermc.geyser.util.VersionCheckUtils;
+import org.geysermc.geyser.util.WebUtils;
 
 import java.io.File;
 import java.io.FileWriter;
@@ -96,7 +107,14 @@ import java.net.UnknownHostException;
 import java.nio.file.Path;
 import java.security.Key;
 import java.text.DecimalFormat;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executors;
@@ -637,7 +655,7 @@ public class GeyserImpl implements GeyserApi {
             this.erosionUnixListener.close();
         }
 
-        Registries.RESOURCE_PACKS.get().clear();
+        ResourcePackLoader.clear();
 
         bootstrap.getGeyserLogger().info(GeyserLocale.getLocaleStringLog("geyser.core.shutdown.done"));
     }

core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java

@@ -313,14 +313,7 @@ public class UpstreamPacketHandler extends LoggingPacketHandler {
 
         // If a remote pack ends up here, that usually implies that a platform was not able to download the pack
         if (codec instanceof UrlPackCodec urlPackCodec) {
-            // Ensure we don't a. spam console, and b. spam download/check requests
+            ResourcePackLoader.checkPack(urlPackCodec);
-            if (!brokenResourcePacks.containsKey(packet.getPackId())) {
-                brokenResourcePacks.put(packet.getPackId(), "");
-                GeyserImpl.getInstance().getLogger().warning("Received a request for a remote pack that the client should have already downloaded! " +
-                        "Is the pack at the URL " + urlPackCodec.url() + " still available?");
-                // not actually interested in using the download, but this does all the checks we need
-                ResourcePackLoader.downloadPack(urlPackCodec.url(), true);
-            }
         }
 
         data.setChunkIndex(packet.getChunkIndex());

core/src/main/java/org/geysermc/geyser/registry/loader/ResourcePackLoader.java

@@ -31,12 +31,14 @@ import org.geysermc.geyser.GeyserImpl;
 import org.geysermc.geyser.api.event.lifecycle.GeyserLoadResourcePacksEvent;
 import org.geysermc.geyser.api.pack.ResourcePack;
 import org.geysermc.geyser.api.pack.ResourcePackManifest;
+import org.geysermc.geyser.api.pack.UrlPackCodec;
 import org.geysermc.geyser.event.type.GeyserDefineResourcePacksEventImpl;
 import org.geysermc.geyser.pack.GeyserResourcePack;
 import org.geysermc.geyser.pack.GeyserResourcePackManifest;
 import org.geysermc.geyser.pack.SkullResourcePackManager;
 import org.geysermc.geyser.pack.path.GeyserPathPackCodec;
 import org.geysermc.geyser.pack.url.GeyserUrlPackCodec;
+import org.geysermc.geyser.registry.Registries;
 import org.geysermc.geyser.text.GeyserLocale;
 import org.geysermc.geyser.util.FileUtils;
 import org.geysermc.geyser.util.WebUtils;
@@ -48,8 +50,10 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.PathMatcher;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
@@ -58,10 +62,16 @@ import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
 /**
- * Loads {@link ResourcePack}s within a {@link Path} directory, firing the {@link GeyserLoadResourcePacksEvent}.
+ * Loads {@link ResourcePack}s within a {@link Path} directory, firing the {@link GeyserDefineResourcePacksEventImpl}.
  */
 public class ResourcePackLoader implements RegistryLoader<Path, Map<String, ResourcePack>> {
 
+    /**
+     * Used to keep track of remote resource packs that the client rejected.
+     * If a client rejects such a pack, it falls back to the old method, and Geyser serves a cached variant.
+     */
+    private static final Set<UrlPackCodec> brokenPacks = new HashSet<>();
+
     static final PathMatcher PACK_MATCHER = FileSystems.getDefault().getPathMatcher("glob:**.{zip,mcpack}");
 
     private static final boolean SHOW_RESOURCE_PACK_LENGTH_WARNING = Boolean.parseBoolean(System.getProperty("Geyser.ShowResourcePackLengthWarning", "true"));
@@ -100,7 +110,6 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
             resourcePacks.add(skullResourcePack);
         }
 
-
         @SuppressWarnings("deprecation")
         GeyserLoadResourcePacksEvent event = new GeyserLoadResourcePacksEvent(resourcePacks);
         GeyserImpl.getInstance().eventBus().fire(event);
@@ -116,11 +125,9 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
 
         // Load CDN entries
         packMap.putAll(loadRemotePacks());
-
         GeyserDefineResourcePacksEventImpl defineEvent = new GeyserDefineResourcePacksEventImpl(packMap);
-        packMap = defineEvent.getPacks();
 
-        return packMap;
+        return defineEvent.getPacks();
     }
 
     /**
@@ -152,6 +159,14 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
         return new GeyserResourcePack(new GeyserPathPackCodec(path), manifest, contentKey);
     }
 
+    /**
+     * Reads a Resource pack from a URL codec, and returns a resource pack. Unlike {@link ResourcePackLoader#readPack(Path)}
+     * this method reads content keys differently.
+     *
+     * @param codec the URL pack codec with the url to download the pack from
+     * @return a {@link GeyserResourcePack} representation
+     * @throws IllegalArgumentException if there was an error reading the pack.
+     */
     public static GeyserResourcePack readPack(GeyserUrlPackCodec codec) throws IllegalArgumentException {
         Path path = codec.getFallback().path();
         if (!PACK_MATCHER.matches(path)) {
@@ -231,6 +246,15 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
         return packMap;
     }
 
+    public static void checkPack(UrlPackCodec codec) {
+        if (!brokenPacks.contains(codec)) {
+            brokenPacks.add(codec);
+            GeyserImpl.getInstance().getLogger().warning("Received a request for a remote pack that the client should have already downloaded! " +
+                    "Is the pack at the URL " + codec.url() + " still available?");
+            downloadPack(codec.url(), true);
+        }
+    }
+
     public static CompletableFuture<@Nullable Path> downloadPack(String url, boolean checking) throws IllegalArgumentException {
         return WebUtils.checkUrlAndDownloadRemotePack(url, checking).whenCompleteAsync((cachedPath, throwable) -> {
             if (cachedPath == null) {
@@ -239,7 +263,7 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
             }
 
             if (throwable != null) {
-                GeyserImpl.getInstance().getLogger().error("Failed to download resource pack " + url, throwable);
+                GeyserImpl.getInstance().getLogger().error("Failed to download resource pack! ", throwable);
                 return;
             }
 
@@ -248,22 +272,54 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
                 throw new IllegalArgumentException("Invalid pack format! Not a .zip or .mcpack file.");
             }
 
-            try {
+            if (checking) {
-                ZipFile zip = new ZipFile(cachedPath.toFile());
+                try {
-                if (zip.stream().noneMatch(x -> x.getName().contains("manifest.json"))) {
+                    Files.delete(cachedPath);
-                    throw new IllegalArgumentException(url + " does not contain a manifest file.");
+                } catch (IOException e) {
+                    throw new IllegalArgumentException("Could not delete debug pack! " + e.getMessage(), e);
                 }
+            }
 
-                // Check if a "manifest.json" or "pack_manifest.json" file is located directly in the zip... does not work otherwise.
+            try {
-                // (something like MyZip.zip/manifest.json) will not, but will if it's a subfolder (MyPack.zip/MyPack/manifest.json)
+                try (ZipFile zip = new ZipFile(cachedPath.toFile())) {
-                if (zip.getEntry("manifest.json") != null || zip.getEntry("pack_manifest.json") != null) {
+                    if (zip.stream().noneMatch(x -> x.getName().contains("manifest.json"))) {
-                    /*throw new IllegalArgumentException("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
+                        throw new IllegalArgumentException(url + " does not contain a manifest file.");
-                            "This is not supported for remote packs, and will cause Bedrock clients to fall back to request the pack from the server. " +
+                    }
-                          "Please put the pack file in a subfolder, and provide that zip in the URL."); */
+
+//                    // Check if a "manifest.json" or "pack_manifest.json" file is located directly in the zip... does not work otherwise.
+//                    // (something like MyZip.zip/manifest.json) will not, but will if it's a subfolder (MyPack.zip/MyPack/manifest.json)
+//                    if (zip.getEntry("manifest.json") != null || zip.getEntry("pack_manifest.json") != null) {
+//                        GeyserImpl.getInstance().getLogger().debug("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
+//                                "This is not supported for remote packs, and will cause Bedrock clients to fall back to request the pack from the server. " +
+//                                "Please put the pack file in a subfolder, and provide that zip in the URL.");
+//                    }
                 }
             } catch (IOException e) {
                 throw new IllegalArgumentException(GeyserLocale.getLocaleStringLog("geyser.resource_pack.broken", url), e);
             }
         });
     }
+
+    public static void clear() {
+        Registries.RESOURCE_PACKS.get().clear();
+
+        // Now: let's clean up broken remote packs, so we don't cache them
+        Path location = GeyserImpl.getInstance().getBootstrap().getConfigFolder().resolve("cache").resolve("remote_packs");
+        brokenPacks.forEach(codec -> {
+            int hash = codec.url().hashCode();
+            Path packLocation = location.resolve(hash + ".zip");
+            Path packMetadata = packLocation.resolveSibling(hash + ".metadata");
+
+            try {
+                if (packMetadata.toFile().exists()) {
+                    Files.delete(packMetadata);
+                }
+                if (packLocation.toFile().exists()) {
+                    Files.delete(packLocation);
+                }
+            } catch (IOException e) {
+                GeyserImpl.getInstance().getLogger().error("Could not delete broken cached resource packs! " + e);
+            }
+        });
+    }
 }

core/src/main/java/org/geysermc/geyser/util/WebUtils.java

@@ -28,6 +28,7 @@ package org.geysermc.geyser.util;
 import com.fasterxml.jackson.databind.JsonNode;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.geysermc.geyser.GeyserImpl;
+import org.geysermc.geyser.GeyserLogger;
 
 import javax.naming.directory.Attribute;
 import javax.naming.directory.InitialDirContext;
@@ -117,6 +118,7 @@ public class WebUtils {
      * @return Path to the downloaded pack file
      */
     public static CompletableFuture<@Nullable Path> checkUrlAndDownloadRemotePack(String url, boolean force) {
+        GeyserLogger logger = GeyserImpl.getInstance().getLogger();
         return CompletableFuture.supplyAsync(() -> {
             try {
                 HttpURLConnection con = (HttpURLConnection) new URL(url).openConnection();
@@ -128,7 +130,7 @@ public class WebUtils {
 
                 int responseCode = con.getResponseCode();
                 if (responseCode >= 400) {
-                    GeyserImpl.getInstance().getLogger().error(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode));
+                    logger.error(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode));
                     return null;
                 }
 
@@ -136,29 +138,27 @@ public class WebUtils {
                 String type = con.getContentType();
 
                 if (size <= 0) {
-                    GeyserImpl.getInstance().getLogger().error(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size));
+                    logger.error(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size));
                     return null;
                 }
 
-                // This doesn't seem to be a requirement (anymore?) Logging to debug might be interesting though.
+                // This doesn't seem to be a requirement (anymore?). Logging to debug might be interesting though.
                 if (type == null || !type.equals("application/zip")) {
-                    GeyserImpl.getInstance().getLogger().debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type));
+                    logger.debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type));
                 }
 
-                // TODO: add logic here to *not* delete the cached pack (and only at shutdown).
-
                 Path packLocation = REMOTE_PACK_CACHE.resolve(url.hashCode() + ".zip");
                 Path packMetadata = packLocation.resolveSibling(url.hashCode() + ".metadata");
 
-                if (Files.exists(packLocation) && Files.exists(packMetadata)) {
+                if (Files.exists(packLocation) && Files.exists(packMetadata) && !force) {
                     try {
                         List<String> metadataLines = Files.readAllLines(packMetadata, StandardCharsets.UTF_8);
                         int cachedSize = Integer.parseInt(metadataLines.get(0));
                         String cachedEtag = metadataLines.get(1);
                         long cachedLastModified = Long.parseLong(metadataLines.get(2));
 
-                        if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) && cachedLastModified == con.getLastModified() && !force) {
+                        if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) && cachedLastModified == con.getLastModified()) {
-                            GeyserImpl.getInstance().getLogger().debug("Using cached pack for " + url);
+                            logger.debug("Using cached pack for " + url);
                             return packLocation;
                         }
                     } catch (IOException e) {
@@ -166,23 +166,31 @@ public class WebUtils {
                     }
                 }
 
-                InputStream in = con.getInputStream();
+                Path downloadLocation = force ? REMOTE_PACK_CACHE.resolve(url.hashCode() + "_debug") : packLocation;
-                Files.copy(in, packLocation, StandardCopyOption.REPLACE_EXISTING);
+                Files.copy(con.getInputStream(), downloadLocation, StandardCopyOption.REPLACE_EXISTING);
 
-                if (Files.size(packLocation) != size) {
+                // This needs to match as the client fails to download the pack otherwise
+                if (Files.size(downloadLocation) != size) {
                     GeyserImpl.getInstance().getLogger().error(String.format("Size mismatch with resource pack at url: %s. Downloaded pack has %s bytes, expected %s bytes!", url, Files.size(packLocation), size));
-                    Files.delete(packLocation);
+                    Files.delete(downloadLocation);
-                    //return null;
+                    return null;
                 }
 
-                try {
+                // "Force" runs when the client rejected a pack. This is done for diagnosis of the issue.
-                    Files.write(packMetadata, Arrays.asList(String.valueOf(size), con.getHeaderField("ETag"), String.valueOf(con.getLastModified())));
+                if (force) {
-                } catch (IOException e) {
+                    if (Files.size(packLocation) != Files.size(downloadLocation)) {
-                    GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage());
+                        logger.error("The pack size seems to have changed. If you wish to change the pack at the remote URL, restart/reload Geyser. " +
+                                "Changing the pack mid-game can result in clients rejecting the pack, connected clients having different pack, or similar. ");
+                    }
+                } else {
+                    try {
+                        Files.write(packMetadata, Arrays.asList(String.valueOf(size), con.getHeaderField("ETag"), String.valueOf(con.getLastModified())));
+                    } catch (IOException e) {
+                        GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage());
+                    }
                 }
 
-                GeyserImpl.getInstance().getLogger().info("debug: pack downloaded");
+                return downloadLocation;
-                return packLocation;
             } catch (MalformedURLException e) {
                 throw new IllegalArgumentException("Malformed URL: " + url);
             } catch (SocketTimeoutException | ConnectException e) {