Automatically download newer pack versions from urls, properly get rid of old packs

core/src/main/java/org/geysermc/geyser/event/type/GeyserDefineResourcePacksEventImpl.java

@@ -35,9 +35,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.UUID;
 
+@Getter
 public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePacksEvent {
 
-    @Getter
     private final Map<String, ResourcePack> packs;
 
     public GeyserDefineResourcePacksEventImpl(Map<String, ResourcePack> packMap) {

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

@@ -305,7 +305,7 @@ public class UpstreamPacketHandler extends LoggingPacketHandler {
 
         // If a remote pack ends up here, that usually implies that a client was not able to download the pack
         if (codec instanceof UrlPackCodec urlPackCodec) {
-            ResourcePackLoader.testUrlPack(session, urlPackCodec);
+            ResourcePackLoader.testRemotePack(session, urlPackCodec, packet.getPackId().toString(), packet.getPackVersion());
         }
 
         data.setChunkIndex(packet.getChunkIndex());

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

@@ -47,6 +47,7 @@ import org.geysermc.geyser.text.GeyserLocale;
 import org.geysermc.geyser.util.FileUtils;
 import org.geysermc.geyser.util.WebUtils;
 
+import java.io.File;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileSystems;
@@ -55,6 +56,7 @@ import java.nio.file.Path;
 import java.nio.file.PathMatcher;
 import java.util.*;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
@@ -126,10 +128,9 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
             }
         }
 
-        // Load CDN entries
         packMap.putAll(loadRemotePacks());
         GeyserDefineResourcePacksEventImpl defineEvent = new GeyserDefineResourcePacksEventImpl(packMap);
-
+        GeyserImpl.getInstance().eventBus().fire(defineEvent);
         return defineEvent.getPacks();
     }
 
@@ -234,7 +235,7 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
                 GeyserUrlPackCodec codec = new GeyserUrlPackCodec(url);
                 ResourcePack pack = codec.create();
                 packMap.put(pack.manifest().header().uuid().toString(), pack);
-            } catch (Exception e) {
+            } catch (Throwable e) {
                 instance.getLogger().error(GeyserLocale.getLocaleStringLog("geyser.resource_pack.broken", url));
                 instance.getLogger().error(e.getMessage());
                 if (instance.getLogger().isDebug()) {
@@ -243,6 +244,9 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
             }
         }
 
+        // After loading the new resource packs: let's clean up the old
+        cleanupRemotePacks();
+
         return packMap;
     }
 
@@ -253,19 +257,76 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
      *
      * @param codec the codec of the resource pack that wasn't successfully downloaded by a Bedrock client.
      */
-    public static void testUrlPack(GeyserSession session, UrlPackCodec codec) {
+    public static void testRemotePack(GeyserSession session, UrlPackCodec codec, String packId, String packVersion) {
         if (CACHED_FAILED_PACKS.getIfPresent(codec.url()) == null) {
-            CACHED_FAILED_PACKS.put(codec.url(), codec);
-            GeyserImpl.getInstance().getLogger().warning("""
-                A Bedrock client (%s, playing on %s / %s) was not able to download the resource pack at %s. Is it still available? Running check now.
-           """.formatted(session.bedrockUsername(), session.getClientData().getDeviceOs().name(), session.getClientData().getDeviceId(), codec.url()));
-            downloadPack(codec.url(), true);
+            String url = codec.url();
+            CACHED_FAILED_PACKS.put(url, codec);
+            GeyserImpl.getInstance().getLogger().warning(
+                "A Bedrock client (%s, playing on %s / %s) was not able to download the resource pack at %s. Checking for changes now:"
+                    .formatted(session.bedrockUsername(), session.getClientData().getDeviceOs().name(), session.getClientData().getDeviceId(), codec.url())
+            );
+
+            downloadPack(codec.url(), true).whenComplete((pathPackCodec, e) -> {
+                if (e != null) {
+                    GeyserImpl.getInstance().getLogger().error(GeyserLocale.getLocaleStringLog("geyser.resource_pack.broken", url), e);
+                    if (GeyserImpl.getInstance().getLogger().isDebug()) {
+                        e.printStackTrace();
+                        if (pathPackCodec != null) {
+                            deleteFile(pathPackCodec.path());
+                        }
+                        return;
+                    }
+                }
+
+                if (pathPackCodec == null) {
+                    return; // Already warned about
+                }
+
+                ResourcePack newPack = ResourcePackLoader.readPack(pathPackCodec.path());
+                UUID newUUID = newPack.manifest().header().uuid();
+                if (newUUID.toString().equals(packId)) {
+                    GeyserImpl.getInstance().getLogger().info("Detected a new resource pack version (%s, old version %s) for pack at %s!"
+                            .formatted(packVersion, newPack.manifest().header().version().toString(), url));
+                } else {
+                    GeyserImpl.getInstance().getLogger().info("Detected a new resource pack at the url %s!".formatted(url));
+                }
+
+                // This should be safe to do as we're not directly using registries to read packs.
+                // Instead, they're cached per-session in the SessionLoadResourcePacks event
+                Registries.RESOURCE_PACKS.get().remove(packId);
+                Registries.RESOURCE_PACKS.get().put(newUUID.toString(), newPack);
+
+                if (codec instanceof GeyserUrlPackCodec geyserUrlPackCodec && geyserUrlPackCodec.getFallback() != null) {
+                    // Other implementations could, in theory, not have a fallback
+                    Path path = geyserUrlPackCodec.getFallback().path();
+                    try {
+                        GeyserImpl.getInstance().getScheduledThread().schedule(() -> {
+                            deleteFile(path);
+                            CACHED_FAILED_PACKS.invalidate(packId);
+                        }, 5, TimeUnit.MINUTES);
+                    } catch (RejectedExecutionException exception) {
+                        // No scheduling here, probably because we're shutting down?
+                        deleteFile(path);
+                    }
+                }
+            });
+        }
+    }
+
+    private static void deleteFile(Path path) {
+        if (path.toFile().exists()) {
+            try {
+                Files.delete(path);
+            } catch (IOException e) {
+                GeyserImpl.getInstance().getLogger().error("Unable to delete old pack! " + e.getMessage());
+                e.printStackTrace();
+            }
         }
     }
 
     public static CompletableFuture<@Nullable PathPackCodec> downloadPack(String url, boolean testing) throws IllegalArgumentException {
         return CompletableFuture.supplyAsync(() -> {
-            Path path = WebUtils.checkUrlAndDownloadRemotePack(url, testing);
+            Path path = WebUtils.downloadRemotePack(url, testing);
 
             // Already warned about these above
             if (path == null) {
@@ -274,7 +335,7 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
 
             // Check if the pack is a .zip or .mcpack file
             if (!PACK_MATCHER.matches(path)) {
-                throw new IllegalArgumentException("Invalid pack format! Not a .zip or .mcpack file.");
+                throw new IllegalArgumentException("Invalid pack format from url %s! Not a .zip or .mcpack file.".formatted(url));
             }
 
             try {
@@ -286,7 +347,7 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
                     // 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) {
-                        if (testing) {
+                        if (GeyserImpl.getInstance().getLogger().isDebug()) {
                             GeyserImpl.getInstance().getLogger().info("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
                                     "This may not work for remote packs, and could 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.");
@@ -297,39 +358,34 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
                 throw new IllegalArgumentException(GeyserLocale.getLocaleStringLog("geyser.resource_pack.broken", url), e);
             }
 
-            if (testing) {
-                try {
-                    Files.delete(path);
-                    return null;
-                } catch (IOException e) {
-                    throw new IllegalStateException("Could not delete debug pack! " + e.getMessage(), e);
-                }
-            }
-
             return new GeyserPathPackCodec(path);
         });
     }
 
     public static void clear() {
         Registries.RESOURCE_PACKS.get().clear();
+        CACHED_FAILED_PACKS.invalidateAll();
 
-        // 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");
-        for (UrlPackCodec codec : CACHED_FAILED_PACKS.asMap().values()) {
-            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);
+    public static void cleanupRemotePacks() {
+        File cacheFolder = GeyserImpl.getInstance().getBootstrap().getConfigFolder().resolve("cache").resolve("remote_packs").toFile();
+        if (!cacheFolder.exists()) {
+            return;
         }
-                if (packLocation.toFile().exists()) {
-                    Files.delete(packLocation);
+
+        int count = 0;
+        final long expireTime = (((long) 1000 * 60 * 60)); // one hour
+        for (File imageFile : Objects.requireNonNull(cacheFolder.listFiles())) {
+            if (imageFile.lastModified() < System.currentTimeMillis() - expireTime) {
+                //noinspection ResultOfMethodCallIgnored
+                imageFile.delete();
+                count++;
             }
-            } catch (IOException e) {
-                GeyserImpl.getInstance().getLogger().error("Could not delete broken cached resource packs! " + e);
         }
+
+        if (count > 0) {
+            GeyserImpl.getInstance().getLogger().debug(String.format("Removed %d cached resource pack files as they are no longer in use!", count));
         }
     }
 }

core/src/main/java/org/geysermc/geyser/skin/SkinProvider.java

@@ -29,9 +29,6 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import it.unimi.dsi.fastutil.bytes.ByteArrays;
-import lombok.AllArgsConstructor;
-import lombok.Getter;
-import lombok.NoArgsConstructor;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.geysermc.geyser.GeyserImpl;
@@ -56,7 +53,9 @@ import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
-import java.util.*;
+import java.util.Map;
+import java.util.Objects;
+import java.util.UUID;
 import java.util.concurrent.*;
 import java.util.function.Predicate;
 
@@ -168,7 +167,7 @@ public class SkinProvider {
                 if (count > 0) {
                     GeyserImpl.getInstance().getLogger().debug(String.format("Removed %d cached image files as they have expired", count));
                 }
-            }, 10, 1440, TimeUnit.MINUTES);
+            }, 10, 1, TimeUnit.DAYS);
         }
     }
 

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

@@ -108,7 +108,8 @@ public class WebUtils {
      * @param force If true, the pack will be downloaded even if it is cached to a separate location.
      * @return Path to the downloaded pack file, or null if it was unable to be loaded
      */
-    public static @Nullable Path checkUrlAndDownloadRemotePack(String url, boolean force) {
+    @SuppressWarnings("ResultOfMethodCallIgnored")
+    public static @Nullable Path downloadRemotePack(String url, boolean force) {
         GeyserLogger logger = GeyserImpl.getInstance().getLogger();
         try {
             HttpURLConnection con = (HttpURLConnection) new URL(url).openConnection();
@@ -120,69 +121,79 @@ public class WebUtils {
 
             int responseCode = con.getResponseCode();
             if (responseCode >= 400) {
-                throw new IllegalStateException(String.format("Invalid response code from remote pack URL: %s (code: %d)", url, responseCode));
+                throw new IllegalStateException(String.format("Invalid response code from remote pack at URL: %s (code: %d)", url, responseCode));
             }
 
             int size = con.getContentLength();
             String type = con.getContentType();
 
             if (size <= 0) {
-                throw new IllegalArgumentException(String.format("Invalid size from remote pack URL: %s (size: %d)", url, size));
+                throw new IllegalArgumentException(String.format("Invalid content length received from remote pack at URL: %s (size: %d)", url, size));
             }
 
-            // 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 as it might be interesting though.
             if (type == null || !type.equals("application/zip")) {
-                logger.debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type));
+                logger.debug(String.format("Application type from remote pack at URL: %s (type: %s)", url, type));
             }
 
-            Path packLocation = REMOTE_PACK_CACHE.resolve(url.hashCode() + ".zip");
-            Path packMetadata = packLocation.resolveSibling(url.hashCode() + ".metadata");
+            Path packMetadata = REMOTE_PACK_CACHE.resolve(url.hashCode() + ".metadata");
+            Path downloadLocation;
 
             // If we downloaded this pack before, reuse it if the ETag matches.
-            if (Files.exists(packLocation) && Files.exists(packMetadata) && !force) {
+            if (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));
+                    List<String> metadata = Files.readAllLines(packMetadata, StandardCharsets.UTF_8);
+                    int cachedSize = Integer.parseInt(metadata.get(0));
+                    String cachedEtag = metadata.get(1);
+                    long cachedLastModified = Long.parseLong(metadata.get(2));
+                    downloadLocation = REMOTE_PACK_CACHE.resolve(metadata.get(3));
 
-                    if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) && cachedLastModified == con.getLastModified()) {
-                        logger.debug("Using cached pack for " + url);
-                        return packLocation;
+                    if (cachedSize == size && cachedEtag.equals(con.getHeaderField("ETag")) &&
+                            cachedLastModified == con.getLastModified() && downloadLocation.toFile().exists()) {
+                        logger.info("Using cached pack (%s) for %s.".formatted(downloadLocation.getFileName(), url));
+                        downloadLocation.toFile().setLastModified(System.currentTimeMillis());
+                        packMetadata.toFile().setLastModified(System.currentTimeMillis());
+                        return downloadLocation;
                     }
                 } catch (IOException e) {
-                    GeyserImpl.getInstance().getLogger().error("Failed to read cached pack metadata: " + e.getMessage());
+                    GeyserImpl.getInstance().getLogger().error("Failed to read cached pack metadata! " + e);
+                    try {
+                        Files.delete(packMetadata);
+                    } catch (Exception exception) {
+                        GeyserImpl.getInstance().getLogger().error("Failed to delete pack metadata!", exception);
+                    }
                 }
             }
 
-            Path downloadLocation = force ? REMOTE_PACK_CACHE.resolve(url.hashCode() + "_debug") : packLocation;
+            downloadLocation = REMOTE_PACK_CACHE.resolve(url.hashCode() + "_" + System.currentTimeMillis() + ".zip");
             Files.copy(con.getInputStream(), downloadLocation, StandardCopyOption.REPLACE_EXISTING);
 
             // This needs to match as the client fails to download the pack otherwise
             long downloadSize = Files.size(downloadLocation);
             if (downloadSize != size) {
                 Files.delete(downloadLocation);
-                throw new IllegalStateException("Size mismatch with resource pack at url: %s. Downloaded pack has %s bytes, expected %s bytes"
+                throw new IllegalStateException("Size mismatch with resource pack at url: %s. Downloaded pack has %s bytes, expected %s bytes!"
                         .formatted(url, downloadSize, size));
             }
 
-            // "Force" runs when the client rejected a pack. This is done for diagnosis of the issue.
-            if (force) {
-                // Check whether existing pack's size matches the newly downloaded packs' size
-                if (Files.size(packLocation) != Files.size(downloadLocation)) {
-                    logger.error("""
-                            The pack size seems to have changed (%s, expected %s). If you wish to change the pack at the remote URL, restart/reload Geyser.
-                            Changing the pack while Geyser is running can result in unexpected issues.
-                            """.formatted(Files.size(packLocation), Files.size(downloadLocation)));
-                }
-            } else {
             try {
-                    Files.write(packMetadata, Arrays.asList(String.valueOf(size), con.getHeaderField("ETag"), String.valueOf(con.getLastModified())));
+                Files.write(
+                        packMetadata,
+                        Arrays.asList(
+                                String.valueOf(size),
+                                con.getHeaderField("ETag"),
+                                String.valueOf(con.getLastModified()),
+                                downloadLocation.getFileName().toString()
+                        ));
+                packMetadata.toFile().setLastModified(System.currentTimeMillis());
             } catch (IOException e) {
                 GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage());
-                }
+                Files.delete(packMetadata);
+                Files.delete(downloadLocation);
+                return null;
             }
 
+            downloadLocation.toFile().setLastModified(System.currentTimeMillis());
             return downloadLocation;
         } catch (MalformedURLException e) {
             throw new IllegalArgumentException("Unable to download resource pack from malformed URL %s! ".formatted(url));