fix: resume revived paused tasks and guard unknown download sizes

README.md

@@ -20,7 +20,7 @@ Automatically download files from urls, even in the background, and keep them lo
 ## Install
 
 First install peer dependencies:
-* [@kesha-antonov/react-native-background-downloader](https://github.com/kesha-antonov/react-native-background-downloader#readme). Be sure to follow the sneakily-hidden [extra iOS step for AppDelegate.m](https://github.com/kesha-antonov/react-native-background-downloader#ios---extra-mandatory-step) or else your background tasks will be canceled by the OS.
+* [@fivecar/react-native-background-downloader](https://github.com/fivecar/react-native-background-downloader#readme). Be sure to follow the [iOS AppDelegate configuration step](https://github.com/fivecar/react-native-background-downloader#-installation) (or add the Expo config plugin) or else your background tasks will be canceled by the OS.
 * [react-native-fs](https://github.com/itinance/react-native-fs#readme)
 * [@react-native-async-storage/async-storage](https://github.com/react-native-async-storage/async-storage#readme)
 

src/index.ts

@@ -69,6 +69,10 @@ export interface DownloadQueueStatus {
 
 export interface DownloadQueueHandlers {
   onBegin?: (url: string, totalBytes: number) => void;
+  /**
+   * `fractionWritten` is 0 when the total size is unknown (e.g. the server
+   * didn't send a Content-Length, in which case `totalBytes` is -1 or 0).
+   */
   onProgress?: (
     url: string,
     fractionWritten: number,
@@ -265,6 +269,42 @@ export default class DownloadQueue {
     this.active = startActive;
     this.isPausedByUser = !startActive;
 
+    // Evaluate the network gate before reviving tasks or starting downloads. If
+    // the current connection isn't allowed (e.g. wifi-only while on cellular),
+    // onNetInfoChanged() calls pauseAllInternal(), which flips this.active off.
+    // That way revivals don't resume PAUSED tasks and fresh starts get paused
+    // in begin() -- so nothing briefly downloads on a forbidden network.
+    this.wouldAutoPause = false;
+
+    if (
+      activeNetworkTypes.length > 0 &&
+      (!netInfoAddEventListener || !netInfoFetchState)
+    ) {
+      throw new Error(
+        "If you pass `activeNetworkTypes`, you must also pass both `netInfoAddEventListener` and `netInfoFetchState`"
+      );
+    }
+    if (netInfoAddEventListener && !netInfoFetchState) {
+      throw new Error(
+        "If you pass `netInfoAddEventListener`, you must also pass `netInfoFetchState`"
+      );
+    }
+    this.activeNetworkTypes = activeNetworkTypes;
+    this.netInfoFetchState = netInfoFetchState;
+    if (netInfoAddEventListener) {
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      const state = await this.netInfoFetchState!();
+
+      await this.onNetInfoChanged(state);
+      this.netInfoUnsubscriber = netInfoAddEventListener(
+        (state: DownloadQueueNetInfoState) => {
+          // The caller's listener signature is synchronous (it comes from
+          // NetInfo.addEventListener), so we can't await this here.
+          void this.onNetInfoChanged(state);
+        }
+      );
+    }
+
     // First revive tasks that were working in the background
     if (existingTasks.length > 0) {
       await Promise.all(existingTasks.map(task => this.reviveTask(task)));
@@ -321,37 +361,6 @@ export default class DownloadQueue {
       now
     );
 
-    this.wouldAutoPause = false;
-
-    if (
-      activeNetworkTypes.length > 0 &&
-      (!netInfoAddEventListener || !netInfoFetchState)
-    ) {
-      throw new Error(
-        "If you pass `activeNetworkTypes`, you must also pass both `netInfoAddEventListener` and `netInfoFetchState`"
-      );
-    }
-    if (netInfoAddEventListener && !netInfoFetchState) {
-      throw new Error(
-        "If you pass `netInfoAddEventListener`, you must also pass `netInfoFetchState`"
-      );
-    }
-    this.activeNetworkTypes = activeNetworkTypes;
-    this.netInfoFetchState = netInfoFetchState;
-    if (netInfoAddEventListener) {
-      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-      const state = await this.netInfoFetchState!();
-
-      await this.onNetInfoChanged(state);
-      this.netInfoUnsubscriber = netInfoAddEventListener(
-        (state: DownloadQueueNetInfoState) => {
-          // The caller's listener signature is synchronous (it comes from
-          // NetInfo.addEventListener), so we can't await this here.
-          void this.onNetInfoChanged(state);
-        }
-      );
-    }
-
     this.inited = true;
   }
 
@@ -753,7 +762,10 @@ export default class DownloadQueue {
         if (!this.active) {
           void task.pause();
         }
-        const fraction = bytesDownloaded / bytesTotal;
+        // bytesTotal can be -1 (unknown Content-Length) or 0, in which case we
+        // can't compute a meaningful fraction. Report 0 rather than a negative
+        // or NaN value, while still passing the raw byte counts through.
+        const fraction = bytesTotal > 0 ? bytesDownloaded / bytesTotal : 0;
         this.handlers?.onProgress?.(url, fraction, bytesDownloaded, bytesTotal);
       })
       // eslint-disable-next-line @typescript-eslint/no-misused-promises
@@ -939,6 +951,11 @@ export default class DownloadQueue {
         this.addTask(spec.url, task);
         if (!this.active && task.state === "DOWNLOADING") {
           await task.pause();
+        } else if (this.active && task.state === "PAUSED") {
+          // A revived task can come back PAUSED (e.g. paused by the OS while
+          // backgrounded). If the queue is active, resume it so it doesn't
+          // stay paused forever -- nothing else in init will restart it.
+          await task.resume();
         }
       } else {
         await task.stop();

test/index.spec.ts

@@ -376,7 +376,7 @@ describe("DownloadQueue", () => {
       expect(createDownloadTask).not.toHaveBeenCalled();
     });
 
-    it("revives paused specs from previous launches", async () => {
+    it("resumes revived paused specs when the queue is active", async () => {
       const queue = new DownloadQueue();
       const handlers: DownloadQueueHandlers = {
         onBegin: jest.fn(),
@@ -394,16 +394,82 @@ describe("DownloadQueue", () => {
       });
       await queue.init({ domain: "mydomain", handlers });
 
-      // PAUSED tasks are left alone (not auto-resumed) upon revival.
+      // Since the queue is active, a revived PAUSED task must be resumed --
+      // otherwise it would stay paused forever.
+      expect(task.resume).toHaveBeenCalled();
       expect(task.pause).not.toHaveBeenCalled();
+      expect(handlers.onBegin).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        task.bytesTotal
+      );
+      expect(createDownloadTask).not.toHaveBeenCalled();
+    });
+
+    it("leaves revived paused specs paused when init'd inactive", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+      };
+
+      task.state = "PAUSED";
+      task.bytesTotal = 8675309;
+      (getExistingDownloadTasks as jest.Mock).mockReturnValue([task]);
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: Date.now() - 1000,
+      });
+      await queue.init({ domain: "mydomain", handlers, startActive: false });
+
+      // When init'd inactive, a revived PAUSED task is already paused and must
+      // be left alone.
       expect(task.resume).not.toHaveBeenCalled();
+      expect(task.pause).not.toHaveBeenCalled();
       expect(handlers.onBegin).toHaveBeenCalledWith(
         "http://foo.com/a.mp3",
         task.bytesTotal
       );
       expect(createDownloadTask).not.toHaveBeenCalled();
     });
 
+    it("doesn't resume revived paused specs when the network forbids downloads", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+      };
+
+      task.state = "PAUSED";
+      task.bytesTotal = 8675309;
+      (getExistingDownloadTasks as jest.Mock).mockReturnValue([task]);
+
+      // We're on cellular, but the queue is wifi-only. init() must evaluate the
+      // network gate before reviving tasks, so a revived PAUSED task is not
+      // resumed onto metered data.
+      (fetch as jest.Mock).mockResolvedValueOnce({
+        isConnected: true,
+        type: "cellular",
+      });
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: Date.now() - 1000,
+      });
+      await queue.init({
+        domain: "mydomain",
+        handlers,
+        activeNetworkTypes: ["wifi"],
+        netInfoAddEventListener: addEventListener,
+        netInfoFetchState: fetch,
+      });
+
+      expect(task.resume).not.toHaveBeenCalled();
+      expect(createDownloadTask).not.toHaveBeenCalled();
+    });
+
     it("revives done specs from previous launches", async () => {
       const queue = new DownloadQueue();
       const handlers: DownloadQueueHandlers = {
@@ -985,10 +1051,10 @@ describe("DownloadQueue", () => {
 
       await relaunchQueue.init({ domain: "mydomain" });
       // Revival should reattach to the existing task rather than starting a
-      // new download, and shouldn't touch pause/resume for an already-paused
-      // task.
+      // new download. Since the relaunched queue is active, a revived PAUSED
+      // task is resumed so it doesn't stay stuck.
       expect(createDownloadTask).toHaveBeenCalledTimes(1);
-      expect(task.resume).not.toHaveBeenCalled();
+      expect(task.resume).toHaveBeenCalled();
     });
 
     it("should pause revived url when relaunched inactive", async () => {
@@ -2435,6 +2501,41 @@ describe("DownloadQueue", () => {
       expect(handlers.onError).toHaveBeenCalledTimes(1);
     });
 
+    it("reports fraction 0 when total size is unknown", async () => {
+      const handlers: DownloadQueueHandlers = {
+        onProgress: jest.fn(),
+      };
+      const queue = new DownloadQueue();
+
+      Object.assign(task, {
+        id: "foo",
+        begin: jest.fn(handler => {
+          task._begin = handler;
+          return task;
+        }),
+        progress: jest.fn(handler => {
+          task._progress = handler;
+          return task;
+        }),
+      });
+
+      await queue.init({ domain: "mydomain", handlers });
+      await queue.addUrl("http://foo.com/a.mp3");
+
+      // The downloader reports bytesTotal === -1 when Content-Length is unknown.
+      // We must not divide by it, which would yield a negative fraction.
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      task._progress!({ bytesDownloaded: 420, bytesTotal: -1 });
+
+      expect(handlers.onProgress).toHaveBeenCalledTimes(1);
+      expect(handlers.onProgress).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        0,
+        420,
+        -1
+      );
+    });
+
     it("should call done handler with local path when finished", async () => {
       const queue = new DownloadQueue();
       const handlers: DownloadQueueHandlers = {