fix: say why a download failed, and what that means for the other archives

android/app/src/main/java/com/microsoft/codepush/react/ArchiveAttemptLog.java

@@ -43,6 +43,9 @@ private static class Attempt {
         String fallbackReason;
         long durationMs = -1;
 
+        /** Whether the server answered this archive's URL with a status instead of the archive. */
+        boolean wasNotServed;
+
         Attempt(String archive) {
             this.archive = archive;
         }
@@ -72,14 +75,25 @@ void recordBundleRestored(long applyDurationMs) {
 
     /**
      * Whether the current archive's attempt got as far as restoring the bundle. A failure
-     * after that point is on the asset side of the archive - and every archive carries the
-     * same bundle patch, so this is what decides whether the patch archive is worth trying
-     * after a failed diff.
+     * after that point is on the asset side of the archive, which the archives do not share -
+     * unlike the bundle patch, which they carry byte for byte.
      */
     boolean currentAttemptRestoredBundle() {
         return current().applyDurationMs >= 0;
     }
 
+    /**
+     * Whether the current archive never arrived, because the server answered its URL with a
+     * status rather than with the archive.
+     *
+     * A verdict on one URL, and the archives are at URLs of their own: a release whose diff
+     * has been cleaned up still has its patch archive. This is the one failure before the
+     * bundle is restored that says nothing about the archives left to try.
+     */
+    boolean currentAttemptWasNotServed() {
+        return current().wasNotServed;
+    }
+
     /**
      * The attempt at the current archive ended in one of the reasons the appliers report.
      *
@@ -104,7 +118,8 @@ void recordFallback(String failureReason) {
      * here would put a value on the wire that no platform reports, so the fallback is
      * reported without a reason.
      */
-    void recordFallbackAfterError() {
+    void recordFallbackAfterError(Throwable error) {
+        current().wasNotServed = CodePushErrorCode.HTTP.equals(CodePushErrorCode.of(error));
         recordFallback(currentAttemptRestoredBundle() ? ArchiveRestoreResult.REASON_PACKAGE_VERIFICATION_FAILED : null);
     }
 

android/app/src/main/java/com/microsoft/codepush/react/CodePushConstants.java

@@ -36,6 +36,24 @@ public class CodePushConstants {
     public static final String DEFAULT_JS_BUNDLE_NAME = "index.android.bundle";
     public static final String DIFF_MANIFEST_FILE_NAME = "hotcodepush.json";
     public static final int DOWNLOAD_BUFFER_SIZE = 1024 * 256;
+
+    /**
+     * How long a download waits for the server to answer at all.
+     *
+     * Kept well above any real handshake, because the only thing this has to catch is a
+     * host that will never answer - a captive portal, a dead route, a network that went
+     * away between the update check and the download.
+     */
+    public static final int DOWNLOAD_CONNECT_TIMEOUT_IN_MS = 10 * 1000;
+
+    /**
+     * How long a download waits for the next bytes of a response already flowing.
+     *
+     * This bounds silence, not the download: a slow connection that keeps delivering is
+     * never cut off, however long the whole archive takes. A connection that delivers
+     * nothing for this long is one the operating system has stopped reporting as dead.
+     */
+    public static final int DOWNLOAD_READ_TIMEOUT_IN_MS = 30 * 1000;
     public static final String DOWNLOAD_FILE_NAME = "download.zip";
     public static final String DOWNLOAD_PROGRESS_EVENT_NAME = "CodePushDownloadProgress";
     public static final String DOWNLOAD_URL_KEY = "downloadUrl";

android/app/src/main/java/com/microsoft/codepush/react/CodePushErrorCode.java

@@ -0,0 +1,86 @@
+package com.microsoft.codepush.react;
+
+import java.net.SocketException;
+import java.net.SocketTimeoutException;
+import java.net.UnknownHostException;
+
+import javax.net.ssl.SSLException;
+
+/**
+ * What kind of failure a download ended in, in the words JS reads it by.
+ *
+ * React Native fills the code of a promise rejected with a bare throwable with
+ * `EUNSPECIFIED`, which left the message the only thing telling one failure apart from
+ * another - and a message is written for a person, not for a report to group by.
+ *
+ * The categories are the ones that ask for different things to happen next, which is why
+ * there are four of them rather than one per way a download can fail. A category no
+ * caller would act differently on is a category that only makes the reports wider.
+ */
+public class CodePushErrorCode {
+
+    /** The network did not carry the download: the socket dropped, timed out, or never opened. */
+    public static final String NETWORK = "CODE_PUSH_NETWORK";
+
+    /** The server answered, with a status that is not a body to install. */
+    public static final String HTTP = "CODE_PUSH_HTTP";
+
+    /**
+     * The downloaded contents do not hash to the release's package hash, or hold no JS
+     * bundle by the name the app looks for. Downloading them again cannot help.
+     */
+    public static final String INTEGRITY = "CODE_PUSH_INTEGRITY";
+
+    /** Nothing here has a word for it, and inventing one would only be a guess. */
+    public static final String UNKNOWN = "CODE_PUSH_UNKNOWN";
+
+    private CodePushErrorCode() {
+    }
+
+    /**
+     * The category of a failure, read through its causes: the download wraps some of what it
+     * catches, and the wrapper is never the part that says what went wrong.
+     */
+    public static String of(Throwable error) {
+        for (Throwable cause = error; cause != null; cause = cause.getCause()) {
+            if (cause instanceof CodePushHttpException) {
+                return HTTP;
+            }
+
+            if (cause instanceof CodePushInvalidUpdateException) {
+                return INTEGRITY;
+            }
+
+            if (isTransportFailure(cause)) {
+                return NETWORK;
+            }
+        }
+
+        return UNKNOWN;
+    }
+
+    /**
+     * Whether the download failed because the network did not carry it.
+     *
+     * A server that answered is not this, however it answered: the connection worked, and
+     * asking a different URL over it is worth doing. A connection that never opened or that
+     * dropped is, and every URL behind it is equally out of reach.
+     */
+    public static boolean isNetworkFailure(Throwable error) {
+        return NETWORK.equals(of(error));
+    }
+
+    private static boolean isTransportFailure(Throwable error) {
+        // `SocketException` is the one that covers the reported majority - the connection
+        // reset and the connection aborted an app being backgrounded mid-download leaves
+        // behind - along with the connection that was refused or had no route.
+        //
+        // A download that stopped short belongs here too. The socket raised nothing for it,
+        // so only the byte count says the network dropped the rest of the body.
+        return error instanceof SocketTimeoutException
+                || error instanceof SocketException
+                || error instanceof UnknownHostException
+                || error instanceof SSLException
+                || error instanceof CodePushIncompleteDownloadException;
+    }
+}

android/app/src/main/java/com/microsoft/codepush/react/CodePushHttpException.java

@@ -0,0 +1,29 @@
+package com.microsoft.codepush.react;
+
+import java.io.IOException;
+
+/**
+ * The server answered a download with a status that is not a body to install.
+ *
+ * An I/O exception rather than one of this package's unchecked ones, because every caller
+ * of a download already handles `IOException` and this is one more way a download does not
+ * arrive - and because the alternative, an unchecked exception, would escape the download
+ * path uncaught.
+ *
+ * The message reads the way the other platform's does, so one release answered with the
+ * same status reads the same in both platforms' reports.
+ */
+public class CodePushHttpException extends IOException {
+
+    private final int mStatusCode;
+
+    public CodePushHttpException(String url, int statusCode) {
+        super("Received " + statusCode + " response from " + url);
+        mStatusCode = statusCode;
+    }
+
+    /** The status the server answered with. */
+    public int getStatusCode() {
+        return mStatusCode;
+    }
+}

android/app/src/main/java/com/microsoft/codepush/react/CodePushIncompleteDownloadException.java

@@ -0,0 +1,17 @@
+package com.microsoft.codepush.react;
+
+import java.io.IOException;
+
+/**
+ * The response stopped before it had delivered the length it declared.
+ *
+ * A type of its own rather than one more unnamed I/O error, because this is a network
+ * failure the socket never raised one for: the bytes simply stopped arriving, and only the
+ * count says so. Everywhere a network failure is acted on has to act on this one too.
+ */
+public class CodePushIncompleteDownloadException extends IOException {
+
+    public CodePushIncompleteDownloadException(long receivedBytes, long declaredBytes) {
+        super("Received " + receivedBytes + " bytes, expected " + declaredBytes);
+    }
+}

android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java

@@ -402,10 +402,15 @@ public void doFrame(long frameTimeNanos) {
                 } catch (CodePushInvalidUpdateException e) {
                     CodePushUtils.log(e);
                     mSettingsManager.saveFailedUpdate(CodePushUtils.convertReadableToJsonObject(updatePackage));
-                    promise.reject(e);
-                } catch (IOException | CodePushUnknownException e) {
+                    promise.reject(CodePushErrorCode.of(e), e.getMessage(), e);
+                } catch (Throwable e) {
+                    // Anything at all, because a promise is waiting on this. A failure that
+                    // leaves this method without settling it leaves JS waiting on a download
+                    // that is no longer running, with nothing to time it out. The narrower
+                    // catch this replaces let two through: a download URL that is not a URL,
+                    // and an archive naming a path outside the folder it unpacks into.
                     CodePushUtils.log(e);
-                    promise.reject(e);
+                    promise.reject(CodePushErrorCode.of(e), e.getMessage(), e);
                 }
             }
         });

android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java

@@ -180,12 +180,15 @@ public JSONObject downloadPackage(JSONObject updatePackage, String expectedBundl
                 return patchAttempt.result();
             }
 
-            // A diff that failed before its bundle was restored failed in the bundle patch,
-            // which the patch archive carries byte for byte - it would fail the same way,
-            // and trying it would only put a second doomed download in front of the full
-            // one. A failure after the restore is on the asset side, which the patch
-            // archive does not share.
-            patchArchiveWorthTrying = patchArchiveWorthTrying && patchAttempt.currentAttemptRestoredBundle();
+            // The patch archive is worth trying when nothing about how the diff failed
+            // implicates it. A diff that failed after restoring its bundle failed on its
+            // asset side, which the patch archive does not share; a diff the server never
+            // served is a verdict on one URL, and the patch archive is at another. Anything
+            // else failed in the bundle patch both archives carry byte for byte, so the
+            // patch archive would fail the same way and trying it would only put a second
+            // doomed download in front of the full one.
+            patchArchiveWorthTrying = patchArchiveWorthTrying
+                    && (patchAttempt.currentAttemptRestoredBundle() || patchAttempt.currentAttemptWasNotServed());
         }
 
         if (patchArchiveWorthTrying) {
@@ -218,8 +221,9 @@ private static String optArchiveDownloadUrl(JSONObject updatePackage, String dow
     /**
      * Installs the update from one of its patch archives.
      *
-     * Every way this can fail ends the same way, with the caller moving on to the next
-     * archive, so none of it is reported to the caller as an error. The ladder cannot loop:
+     * Every verdict on the archive ends the same way, with the caller moving on to the next
+     * one, so none of it is reported to the caller as an error. A network that did not carry
+     * the archive is not a verdict on it and is raised instead. The ladder cannot loop:
      * which archive comes next is the caller's decision alone, and the full archive at its
      * end is downloaded by a call that is not allowed to take the patch path, so it has no
      * failure of its own to fall back from.
@@ -228,10 +232,13 @@ private static String optArchiveDownloadUrl(JSONObject updatePackage, String dow
      *                     whoever asked for the download
      * @return true when the update was installed, false when the caller has to move on to
      *         the next archive
+     * @throws IOException when the network did not carry the archive, which is not a verdict
+     *                     on the archive and so is not a reason to try another one
      */
     private boolean tryDownloadArchivePackage(JSONObject updatePackage, String expectedBundleFileName,
                                               DownloadProgressCallback progressCallback,
-                                              String archiveDownloadUrl, ArchiveAttemptLog patchAttempt) {
+                                              String archiveDownloadUrl, ArchiveAttemptLog patchAttempt)
+            throws IOException {
         try {
             ArchiveRestoreResult patchResult = downloadAndInstallPackage(updatePackage, expectedBundleFileName,
                     progressCallback, archiveDownloadUrl, true, patchAttempt);
@@ -243,11 +250,27 @@ private boolean tryDownloadArchivePackage(JSONObject updatePackage, String expec
             CodePushUtils.log("The " + patchAttempt.currentArchive() + " archive failed ("
                     + patchResult.getFailureReason() + "). Falling back.");
         } catch (Exception | OutOfMemoryError e) {
+            if (CodePushErrorCode.isNetworkFailure(e)) {
+                // The network is what failed, not the archive, and the full archive is behind
+                // the same network - only larger, and started over from nothing. Falling back
+                // here would spend a second download to reach the failure already in hand.
+                CodePushUtils.log("The " + patchAttempt.currentArchive()
+                        + " archive could not be downloaded. Giving up on the download.");
+                if (e instanceof IOException) {
+                    throw (IOException) e;
+                }
+
+                // A network failure read out of a wrapper this package raised, which the
+                // caller catches by its own type rather than by `IOException`.
+                throw new CodePushUnknownException(
+                        "The " + patchAttempt.currentArchive() + " archive could not be downloaded.", e);
+            }
+
             // Applying a patch is the one path that holds a whole bundle in memory, so
             // running out of it is a failure this has to absorb like any other: by the time
             // it lands here the arrays are unreachable, and the full archive is downloaded
             // to disk in chunks rather than held.
-            patchAttempt.recordFallbackAfterError();
+            patchAttempt.recordFallbackAfterError(e);
             CodePushUtils.log(e);
             CodePushUtils.log("The " + patchAttempt.currentArchive()
                     + " archive could not be applied. Falling back.");
@@ -296,6 +319,8 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String
         try {
             URL downloadUrl = new URL(downloadUrlString);
             connection = (HttpURLConnection) (downloadUrl.openConnection());
+            connection.setConnectTimeout(CodePushConstants.DOWNLOAD_CONNECT_TIMEOUT_IN_MS);
+            connection.setReadTimeout(CodePushConstants.DOWNLOAD_READ_TIMEOUT_IN_MS);
 
             if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP &&
                 downloadUrl.toString().startsWith("https")) {
@@ -307,6 +332,15 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String
             }
 
             connection.setRequestProperty("Accept-Encoding", "identity");
+
+            // Read before the body is: an error status carries a body of its own, and
+            // asking `getInputStream()` for it first turns some statuses into a stream and
+            // others into an exception that says nothing about which status it was.
+            int responseCode = connection.getResponseCode();
+            if (responseCode >= 400) {
+                throw new CodePushHttpException(downloadUrlString, responseCode);
+            }
+
             bin = new BufferedInputStream(connection.getInputStream());
 
             // Announced only once the response is flowing, so a connection that fails to
@@ -347,8 +381,11 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String
                 progressCallback.call(new DownloadProgress(totalBytes, receivedBytes));
             }
 
-            if (totalBytes != receivedBytes) {
-                throw new CodePushUnknownException("Received " + receivedBytes + " bytes, expected " + totalBytes);
+            // Only against a length the server declared. `getContentLength()` answers -1
+            // for a body sent without one, which no read total matches - so comparing anyway
+            // would fail every download a server chooses to send that way.
+            if (totalBytes >= 0 && totalBytes != receivedBytes) {
+                throw new CodePushIncompleteDownloadException(receivedBytes, totalBytes);
             }
 
             isZip = ByteBuffer.wrap(header).getInt() == 0x504b0304;
@@ -507,6 +544,14 @@ public void downloadAndReplaceCurrentBundle(String remoteBundleUrl, String bundl
         try {
             downloadUrl = new URL(remoteBundleUrl);
             connection = (HttpURLConnection) (downloadUrl.openConnection());
+            connection.setConnectTimeout(CodePushConstants.DOWNLOAD_CONNECT_TIMEOUT_IN_MS);
+            connection.setReadTimeout(CodePushConstants.DOWNLOAD_READ_TIMEOUT_IN_MS);
+
+            int responseCode = connection.getResponseCode();
+            if (responseCode >= 400) {
+                throw new CodePushHttpException(remoteBundleUrl, responseCode);
+            }
+
             bin = new BufferedInputStream(connection.getInputStream());
             File downloadFile = new File(getCurrentPackageBundlePath(bundleFileName));
             downloadFile.delete();