fix: getAvailableUrl returns remote when lazy-deleted

src/index.ts

@@ -271,19 +271,7 @@ export default class DownloadQueue {
         if (existingTasks.some(task => task.id === spec.id)) {
           return;
         }
-        if (spec.finished) {
-          // Once in a while we think the spec is finished but the file isn't
-          // on disk. This can happen when XCode installs a new build, and
-          // sometimes through TestFlight.
-          const exists = await RNFS.exists(spec.path);
-
-          if (exists) {
-            return;
-          }
-
-          spec.finished = false; // We're not really finished, it seems.
-          await this.kvfs.write(this.keyFromId(spec.id), spec);
-        }
+        await this.reconcileFinishStateWithFile(spec);
         return this.start(spec);
       })
     );
@@ -613,7 +601,8 @@ export default class DownloadQueue {
 
   /**
    * Gets a remote or local url, preferring to the local path when possible. If
-   * the local file hasn't yet been downloaded, returns the remote url.
+   * the local file hasn't yet been downloaded, returns the remote url. Also
+   * returns the remote url if the record is being lazy-deleted.
    * @param url The remote URL to check for local availability
    * @returns A local file path if the URL has already been downloaded, else url
    */
@@ -622,7 +611,7 @@ export default class DownloadQueue {
 
     const spec = this.specs.find(spec => spec.url === url);
 
-    if (!spec || !spec.finished) {
+    if (!spec || !spec.finished || spec.createTime <= 0) {
       return url;
     }
 
@@ -812,6 +801,10 @@ export default class DownloadQueue {
   private async reviveTask(task: DownloadTask) {
     const spec = this.specs.find(spec => spec.id === task.id);
 
+    if (spec) {
+      await this.reconcileFinishStateWithFile(spec);
+    }
+
     // Don't revive finished tasks or ones that already have lazy deletes in
     // progress.
     if (spec && !spec.finished && spec.createTime > 0) {
@@ -827,9 +820,27 @@ export default class DownloadQueue {
           this.handlers?.onBegin?.(spec.url, task.totalBytes);
           break;
         case "DONE":
-          this.handlers?.onBegin?.(spec.url, task.totalBytes);
-          this.handlers?.onDone?.(spec.url, spec.path);
-          shouldAddTask = false;
+          {
+            const exists = await RNFS.exists(spec.path);
+
+            if (exists) {
+              spec.finished = true;
+              await this.kvfs.write(this.keyFromId(spec.id), spec);
+              this.handlers?.onBegin?.(spec.url, task.totalBytes);
+              this.handlers?.onDone?.(spec.url, spec.path);
+              shouldAddTask = false;
+            } else {
+              // If the spec thinks we're not done but the OS does, yet we
+              // can't find the file on disk, we'll leave shouldAddTask = true so
+              // that we begin the download again.
+              // Downloader docs say every task needs to be paused or stopped.
+              // So we stop here.
+              task.stop();
+              // Since the file is missing from disk, yet the downloader thinks
+              // it's done, we restart the download.
+              this.start(spec);
+            }
+          }
           break;
         case "STOPPED":
           this.start(spec);
@@ -858,7 +869,7 @@ export default class DownloadQueue {
         task.stop();
       }
     } else {
-      if (["DOWNLOADING", "PAUSED"].includes(task.state)) {
+      if (this.isTaskDownloading(task)) {
         task.stop();
 
         if (spec && !spec.finished) {
@@ -876,6 +887,31 @@ export default class DownloadQueue {
     }
   }
 
+  /**
+   * Makes sure, if a spec thinks it's finished, that the file which backs it
+   * actually exists. If that file doesn't exist, we set finished === false.
+   * @returns true if spec is finished and the file exists
+   */
+  private async reconcileFinishStateWithFile(spec: Spec) {
+    if (spec.finished) {
+      // Once in a while we think the spec is finished but the file isn't
+      // on disk. This can happen when XCode installs a new build, and
+      // sometimes through TestFlight.
+      const exists = await RNFS.exists(spec.path);
+
+      if (exists) {
+        return;
+      }
+
+      spec.finished = false; // We're not really finished, it seems.
+      await this.kvfs.write(this.keyFromId(spec.id), spec);
+    }
+  }
+
+  private isTaskDownloading(task: DownloadTask) {
+    return ["DOWNLOADING", "PAUSED"].includes(task.state);
+  }
+
   private extensionFromUri(uri: string) {
     const path = this.urlToPath?.(uri);
 

test/index.spec.ts

@@ -383,6 +383,7 @@ describe("DownloadQueue", () => {
       task.state = "DONE";
       task.totalBytes = 8675309;
       (checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
+      (exists as jest.Mock).mockReturnValue(true);
 
       await kvfs.write("/mydomain/foo", {
         id: "foo",
@@ -405,6 +406,33 @@ describe("DownloadQueue", () => {
       expect(download).not.toHaveBeenCalled();
     });
 
+    it("revives done specs from previous launches with missing files", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+        onDone: jest.fn(),
+      };
+
+      task.state = "DONE";
+      task.totalBytes = 8675309;
+      (checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
+      (exists as jest.Mock).mockReturnValue(false);
+
+      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 });
+
+      // Because it's done downloading, we don't expect resume()
+      expect(task.resume).not.toHaveBeenCalled();
+      expect(handlers.onBegin).not.toHaveBeenCalled();
+      expect(handlers.onDone).not.toHaveBeenCalled();
+      expect(download).toHaveBeenCalledTimes(1);
+    });
+
     it("restarts stopped specs from previous launches", async () => {
       const queue = new DownloadQueue();
       const handlers: DownloadQueueHandlers = {
@@ -1942,6 +1970,70 @@ describe("DownloadQueue", () => {
       );
     });
 
+    it("should give you back the remote url when spec is lazy-deleted", async () => {
+      const queue = new DownloadQueue();
+      const fooTask = createBasicTask();
+      let fooPath = "tbd";
+
+      (download as jest.Mock).mockImplementation(
+        (spec: { id: string; url: string; destination: string }) => {
+          if (spec.url === "http://foo.com/a.mp3") {
+            fooPath = spec.destination;
+            return Object.assign(fooTask, {
+              done: jest.fn((handler: DoneHandler) => {
+                fooTask._done = handler;
+                return fooTask;
+              }),
+            });
+          }
+          return {
+            ...task,
+            id: spec.id,
+            path: spec.destination,
+          };
+        }
+      );
+
+      await queue.init({ domain: "mydomain" });
+      await queue.addUrl("http://foo.com/a.mp3");
+      await queue.addUrl("http://boo.com/a.mp3");
+
+      // Pretend we've downloaded only foo
+      (exists as jest.Mock).mockImplementation(path => path === fooPath);
+
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      await fooTask._done!();
+      await queue.removeUrl("http://foo.com/a.mp3", Date.now() + 50000);
+
+      const [fooU, booU] = await Promise.all([
+        queue.getAvailableUrl("http://foo.com/a.mp3"),
+        queue.getAvailableUrl("http://boo.com/a.mp3"),
+      ]);
+
+      expect(fooU).toBe("http://foo.com/a.mp3"); // Should give us remote URL
+      expect(booU).toBe("http://boo.com/a.mp3");
+
+      const restartedQueue = new DownloadQueue();
+
+      await restartedQueue.init({ domain: "mydomain" });
+      const [fooUR, statuses] = await Promise.all([
+        restartedQueue.getAvailableUrl("http://foo.com/a.mp3"),
+        restartedQueue.getQueueStatus(),
+      ]);
+
+      // We should be sure that the lazy-deleted status is reported as !complete
+      expect(fooUR).toBe("http://foo.com/a.mp3"); // Should give us remote URL
+      expect(statuses.length).toBe(1); // only boo should be left
+      expect(statuses).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({
+            url: "http://boo.com/a.mp3",
+            complete: false,
+          }),
+        ])
+      );
+    });
+
     it("should call handlers for all cases", async () => {
       const handlers: DownloadQueueHandlers = {
         onBegin: jest.fn(),