PROD-2201: fix interrupted asset download poisoning the pull cache

src/lib/downloaders/download-assets.ts

@@ -39,12 +39,20 @@ export async function downloadAllAssets(guid: string): Promise<void> {
   // Helper function to check if asset needs download based on dateModified
   function shouldDownloadAsset(
     apiAsset: any,
-    localInfo: { dateModified?: string; exists: boolean }
+    localInfo: { dateModified?: string; exists: boolean },
+    binaryExists: boolean
   ): { shouldDownload: boolean; reason: string } {
     if (!localInfo.exists) {
       return { shouldDownload: true, reason: "new file" };
     }
 
+    // Guard against a poisoned cache: metadata JSON exists but its binary was
+    // never written (e.g. an interrupted pull). Change detection would
+    // otherwise skip forever, and a later push fails with "asset file not found".
+    if (!binaryExists) {
+      return { shouldDownload: true, reason: "missing binary" };
+    }
+
     if (!localInfo.dateModified || !apiAsset.dateModified) {
       return { shouldDownload: true, reason: "missing date info" };
     }
@@ -118,7 +126,12 @@ export async function downloadAllAssets(guid: string): Promise<void> {
     for (const asset of allAssets) {
       const assetJsonPath = path.join(fileOps.getDataFolderPath("assets"), `${asset.mediaID}.json`);
       const localInfo = getLocalAssetInfo(assetJsonPath);
-      const downloadDecision = shouldDownloadAsset(asset, localInfo);
+      // Assets with an originUrl have a binary on disk; verify it exists so a
+      // metadata-only cache entry doesn't cause a permanent skip.
+      const binaryExists = asset.originUrl
+        ? fs.existsSync(path.join(fileOps.getDataFolderPath("assets"), getAssetFilePath(asset.originUrl)))
+        : true;
+      const downloadDecision = shouldDownloadAsset(asset, localInfo, binaryExists);
 
       if (downloadDecision.shouldDownload) {
         downloadableAssets.push({ asset, reason: downloadDecision.reason });
@@ -153,16 +166,16 @@ export async function downloadAllAssets(guid: string): Promise<void> {
       const downloadPromises = batch.map(async (item) => {
         const { asset, reason } = item;
         try {
-          // Export asset JSON metadata
-          fileOps.exportFiles(`assets`, asset.mediaID.toString(), asset);
-
           // Download actual file if it has an originUrl
           if (asset.originUrl) {
             const filePath = getAssetFilePath(asset.originUrl);
             const assetFilesPath = path.join(fileOps.getDataFolderPath("assets"), filePath);
             const success = await fileOps.downloadFile(asset.originUrl, assetFilesPath);
 
             if (success) {
+              // Write the metadata JSON only after the binary lands on disk, so
+              // an interrupted pull can't leave a metadata-only (poisoned) entry.
+              fileOps.exportFiles(`assets`, asset.mediaID.toString(), asset);
               const sizeDisplay = asset.size ? formatFileSize(asset.size) : "";
               logger.asset.downloaded(asset);
               return { success: true, asset };
@@ -171,7 +184,8 @@ export async function downloadAllAssets(guid: string): Promise<void> {
               throw new Error("Download failed");
             }
           } else {
-            // Asset without downloadable file - just metadata
+            // Asset without downloadable file - just metadata, safe to write now
+            fileOps.exportFiles(`assets`, asset.mediaID.toString(), asset);
             logger.warning("Asset without downloadable file", asset);
             // logger.asset.downloaded(asset);
             return { success: true, asset };
@@ -192,9 +206,6 @@ export async function downloadAllAssets(guid: string): Promise<void> {
         if (result.success) {
           totalSuccessfullyDownloaded++;
         }
-
-        // Update progress (include skipped assets in total processed)
-        const totalProcessed = totalAttemptedToProcess + skippableAssets.length;
       }
     }
 

src/lib/downloaders/tests/download-assets.test.ts

@@ -1,18 +1,30 @@
 import { resetState, setState, state } from "core/state";
+import * as fs from "fs";
+
+const mockDataFolderPath = "/tmp/agility-mock-assets";
+
+let exportFilesMock: jest.Mock;
+let downloadFileMock: jest.Mock;
 
 jest.mock("core/fileOperations", () => ({
   fileOperations: jest.fn().mockImplementation(() => ({
     createFolder: jest.fn(),
-    exportFiles: jest.fn(),
-    downloadFile: jest.fn(),
-    getDataFolderPath: jest.fn().mockReturnValue("/tmp/agility-mock-assets"),
+    exportFiles: exportFilesMock,
+    downloadFile: downloadFileMock,
+    getDataFolderPath: jest.fn().mockReturnValue(mockDataFolderPath),
   })),
 }));
 
 jest.mock("lib/shared/get-all-channels", () => ({
   getAllChannels: jest.fn().mockResolvedValue([{ channel: "website" }]),
 }));
 
+jest.mock("fs", () => ({
+  ...jest.requireActual("fs"),
+  existsSync: jest.fn(),
+  readFileSync: jest.fn(),
+}));
+
 import { downloadAllAssets } from "lib/downloaders/download-assets";
 
 function makeMockLogger() {
@@ -28,8 +40,33 @@ function makeMockLogger() {
   };
 }
 
+function makeAsset(
+  overrides: Partial<{ mediaID: number; originUrl: string; dateModified: string; fileName: string; size: number }> = {}
+) {
+  const fileName = overrides.fileName ?? "file1.jpg";
+  return {
+    mediaID: 1,
+    originUrl: `https://cdn.agilitycms.com/guid/assets/${fileName}`,
+    dateModified: "2024-01-01T00:00:00.000Z",
+    fileName,
+    size: 1024,
+    ...overrides,
+  };
+}
+
+const UNCHANGED_DATE = "2024-01-01T00:00:00.000Z";
+
+const jsonPathFor = (mediaID: number) => `${mockDataFolderPath}/${mediaID}.json`;
+const binaryPathFor = (fileName: string) => `${mockDataFolderPath}/${fileName}`;
+
 beforeEach(() => {
   resetState();
+  exportFilesMock = jest.fn();
+  downloadFileMock = jest.fn();
+
+  (fs.existsSync as jest.Mock).mockReset();
+  (fs.readFileSync as jest.Mock).mockReset();
+
   jest.spyOn(console, "log").mockImplementation(() => {});
   jest.spyOn(console, "warn").mockImplementation(() => {});
   jest.spyOn(console, "error").mockImplementation(() => {});
@@ -39,8 +76,6 @@ afterEach(() => {
   jest.restoreAllMocks();
 });
 
-// ─── downloadAllAssets guard clause ───────────────────────────────────────────
-
 describe("downloadAllAssets", () => {
   describe("guard clause: no logger for GUID", () => {
     it("returns early without throwing when getLoggerForGuid returns null", async () => {
@@ -89,4 +124,114 @@ describe("downloadAllAssets", () => {
       await expect(downloadAllAssets("test-guid-u")).resolves.toBeUndefined();
     });
   });
+
+  describe("PROD-2201: missing-binary re-download (skip-decision fix)", () => {
+    it("re-downloads an asset whose metadata is unchanged but binary is missing on disk", async () => {
+      const asset = makeAsset({ mediaID: 1, dateModified: UNCHANGED_DATE, fileName: "file1.jpg" });
+      const logger = makeMockLogger();
+
+      jest.spyOn(require("core/state"), "getLoggerForGuid").mockReturnValue(logger);
+      jest.spyOn(require("core/state"), "getApiClient").mockReturnValue({
+        assetMethods: {
+          getMediaList: jest.fn().mockResolvedValue({ totalCount: 1, assetMedias: [asset] }),
+        },
+      });
+
+      (fs.existsSync as jest.Mock).mockImplementation((filePath: string) => {
+        if (filePath === jsonPathFor(asset.mediaID)) return true; // metadata JSON present
+        if (filePath === binaryPathFor(asset.fileName)) return false; // binary missing
+        return false;
+      });
+      (fs.readFileSync as jest.Mock).mockReturnValue(JSON.stringify({ dateModified: UNCHANGED_DATE }));
+
+      downloadFileMock.mockResolvedValue(true);
+
+      await downloadAllAssets("test-guid-u");
+
+      expect(downloadFileMock).toHaveBeenCalledWith(asset.originUrl, binaryPathFor(asset.fileName));
+      expect(logger.asset.skipped).not.toHaveBeenCalled();
+    });
+
+    it("skips an asset whose metadata is unchanged and binary is present on disk", async () => {
+      const asset = makeAsset({ mediaID: 2, dateModified: UNCHANGED_DATE, fileName: "file2.jpg" });
+      const logger = makeMockLogger();
+
+      jest.spyOn(require("core/state"), "getLoggerForGuid").mockReturnValue(logger);
+      jest.spyOn(require("core/state"), "getApiClient").mockReturnValue({
+        assetMethods: {
+          getMediaList: jest.fn().mockResolvedValue({ totalCount: 1, assetMedias: [asset] }),
+        },
+      });
+
+      (fs.existsSync as jest.Mock).mockImplementation((filePath: string) => {
+        if (filePath === jsonPathFor(asset.mediaID)) return true; // metadata JSON present
+        if (filePath === binaryPathFor(asset.fileName)) return true; // binary present
+        return false;
+      });
+      (fs.readFileSync as jest.Mock).mockReturnValue(JSON.stringify({ dateModified: UNCHANGED_DATE }));
+
+      await downloadAllAssets("test-guid-u");
+
+      expect(downloadFileMock).not.toHaveBeenCalled();
+      expect(logger.asset.skipped).toHaveBeenCalledWith(asset);
+    });
+  });
+
+  describe("PROD-2201: metadata-written-after-binary (ordering fix)", () => {
+    it("does not write metadata JSON when downloadFile fails", async () => {
+      const asset = makeAsset({ mediaID: 3, fileName: "file3.jpg" });
+      const logger = makeMockLogger();
+
+      jest.spyOn(require("core/state"), "getLoggerForGuid").mockReturnValue(logger);
+      jest.spyOn(require("core/state"), "getApiClient").mockReturnValue({
+        assetMethods: {
+          getMediaList: jest.fn().mockResolvedValue({ totalCount: 1, assetMedias: [asset] }),
+        },
+      });
+
+      // Local metadata doesn't exist yet, so the asset is treated as a new file
+      // and always queued for download regardless of dateModified/binary checks.
+      (fs.existsSync as jest.Mock).mockReturnValue(false);
+
+      downloadFileMock.mockResolvedValue(false);
+
+      await downloadAllAssets("test-guid-u");
+
+      expect(downloadFileMock).toHaveBeenCalledWith(asset.originUrl, binaryPathFor(asset.fileName));
+      expect(exportFilesMock).not.toHaveBeenCalledWith("assets", asset.mediaID.toString(), asset);
+      expect(logger.asset.error).toHaveBeenCalled();
+    });
+
+    it("writes metadata JSON only after downloadFile resolves successfully", async () => {
+      const asset = makeAsset({ mediaID: 4, fileName: "file4.jpg" });
+      const logger = makeMockLogger();
+
+      jest.spyOn(require("core/state"), "getLoggerForGuid").mockReturnValue(logger);
+      jest.spyOn(require("core/state"), "getApiClient").mockReturnValue({
+        assetMethods: {
+          getMediaList: jest.fn().mockResolvedValue({ totalCount: 1, assetMedias: [asset] }),
+        },
+      });
+
+      (fs.existsSync as jest.Mock).mockReturnValue(false);
+
+      const callOrder: string[] = [];
+      downloadFileMock.mockImplementation(async () => {
+        await new Promise((resolve) => setTimeout(resolve, 5));
+        callOrder.push("downloadFile");
+        return true;
+      });
+      exportFilesMock.mockImplementation((...args: any[]) => {
+        if (args[0] === "assets" && args[1] === asset.mediaID.toString()) {
+          callOrder.push("exportFiles");
+        }
+      });
+
+      await downloadAllAssets("test-guid-u");
+
+      expect(exportFilesMock).toHaveBeenCalledWith("assets", asset.mediaID.toString(), asset);
+      expect(callOrder).toEqual(["downloadFile", "exportFiles"]);
+      expect(logger.asset.downloaded).toHaveBeenCalledWith(asset);
+    });
+  });
 });