start: don't try to delete broken packs while we are still delivering them

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

@@ -77,16 +77,15 @@ import java.nio.channels.SeekableByteChannel;
 import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.Map;
 import java.util.OptionalInt;
-import java.util.Set;
 import java.util.UUID;
 
 public class UpstreamPacketHandler extends LoggingPacketHandler {
 
     private boolean networkSettingsRequested = false;
     private final Deque<String> packsToSent = new ArrayDeque<>();
-    private final Set<UUID> brokenResourcePacks = new HashSet<>();
+    private final Map<UUID, String> brokenResourcePacks = new HashMap<>();
     private final CompressionStrategy compressionStrategy;
 
     private SessionLoadResourcePacksEventImpl resourcePackLoadEvent;
@@ -315,8 +314,8 @@ 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
-            if (!brokenResourcePacks.contains(packet.getPackId())) {
+            if (!brokenResourcePacks.containsKey(packet.getPackId())) {
-                brokenResourcePacks.add(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

core/src/main/java/org/geysermc/geyser/pack/url/GeyserUrlPackCodec.java

@@ -88,9 +88,9 @@ public class GeyserUrlPackCodec extends UrlPackCodec {
             try {
                 final Path downloadedPack = ResourcePackLoader.downloadPack(url, false).whenComplete((pack, throwable) -> {
                     if (throwable != null) {
-                        GeyserImpl.getInstance().getLogger().error("Failed to download pack from " + url, throwable);
+                        GeyserImpl.getInstance().getLogger().error("Failed to download pack from " + url + " due to " + throwable.getMessage());
                         if (GeyserImpl.getInstance().getConfig().isDebugMode()) {
-                            throwable.printStackTrace();
+                            GeyserImpl.getInstance().getLogger().error("full error: " + throwable);
                         }
                     }
                 }).join();

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

@@ -231,8 +231,8 @@ public class ResourcePackLoader implements RegistryLoader<Path, Map<String, Reso
         return packMap;
     }
 
-    public static CompletableFuture<@Nullable Path> downloadPack(String url, boolean force) throws IllegalArgumentException {
+    public static CompletableFuture<@Nullable Path> downloadPack(String url, boolean checking) throws IllegalArgumentException {
-        return WebUtils.checkUrlAndDownloadRemotePack(url, force).whenCompleteAsync((cachedPath, throwable) -> {
+        return WebUtils.checkUrlAndDownloadRemotePack(url, checking).whenCompleteAsync((cachedPath, throwable) -> {
             if (cachedPath == null) {
                 // already warned about in WebUtils
                 return;
@@ -257,9 +257,9 @@ 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) {
-                    throw new IllegalArgumentException("The remote resource pack from " + url + " contains a manifest.json file at the root of the zip file. " +
+                    /*throw new IllegalArgumentException("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.");
+                          "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);

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

@@ -113,7 +113,7 @@ public class WebUtils {
      * If it is, it will download the pack file and return a path to it
      *
      * @param url The URL to check
-     * @param force If true, the pack will be downloaded even if it is cached
+     * @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
      */
     public static CompletableFuture<@Nullable Path> checkUrlAndDownloadRemotePack(String url, boolean force) {
@@ -124,7 +124,7 @@ public class WebUtils {
                 con.setConnectTimeout(10000);
                 con.setReadTimeout(10000);
                 con.setRequestProperty("User-Agent", "Geyser-" + GeyserImpl.getInstance().getPlatformType().platformName() + "/" + GeyserImpl.VERSION);
-                con.setInstanceFollowRedirects(false); // TODO verify
+                con.setInstanceFollowRedirects(true);
 
                 int responseCode = con.getResponseCode();
                 if (responseCode >= 400) {
@@ -140,11 +140,13 @@ public class WebUtils {
                     return null;
                 }
 
+                // 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().error(String.format("Invalid application type from remote pack URL: %s (type: %s)", url, type));
+                    GeyserImpl.getInstance().getLogger().debug(String.format("Application type from remote pack URL: %s (type: %s)", url, type));
-                    return null;
                 }
 
+                // 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");
 
@@ -170,7 +172,7 @@ public class WebUtils {
                 if (Files.size(packLocation) != 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);
-                    return null;
+                    //return null;
                 }
 
                 try {
@@ -179,6 +181,7 @@ public class WebUtils {
                     GeyserImpl.getInstance().getLogger().error("Failed to write cached pack metadata: " + e.getMessage());
                 }
 
+                GeyserImpl.getInstance().getLogger().info("debug: pack downloaded");
                 return packLocation;
             } catch (MalformedURLException e) {
                 throw new IllegalArgumentException("Malformed URL: " + url);