fix(#187 AC#2 r3 follow-up): install idempotency + legacy partial migration

jayzalowitz · claude · jayzalowitz · commit f5e567eea6c9 · 2026-05-09T14:53:48.000-04:00
Three findings from Copilot's review of PR #249: - **Install destroys good shared file on rename failure**: the previous "unlink target → renameSync" sequence had a window where, if rename threw or got cancelled mid-flight, the host was left with no GGUF at all — even though a perfectly good copy had been deleted moments before. Since the final path is now content-addressable (SHA-256 verified before this point), an existing target IS the model. New install path: if target exists → just discard our partial, treat as installed; otherwise renameSync directly without unlinking; if the rename fails AND target now exists (race lost to another row), treat as installed. - **Legacy non-namespaced partials orphaned by migration**: PR #247 shipped with `<target>.partial`; PR #249 namespaces by row id. Existing pre-#249 partials would sit on disk indefinitely. Added unconditional cleanup at the top of runDownload — if a legacy `<target>.partial` exists, delete it before the row's namespaced partial is created. - **Stale comment in cancelDownload**: docstring still mentioned `<target_path>.partial`. Updated to the namespaced format. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/apps/api/src/embedded-llm/downloader.ts b/apps/api/src/embedded-llm/downloader.ts
@@ -182,8 +182,9 @@ export async function pauseDownload(downloadId: string): Promise<boolean> {
 }
 
 /**
- * Cancel and clean up. Aborts in-flight transfer, deletes the
- * `<target_path>.partial` file, marks the row 'cancelled'.
+ * Cancel and clean up. Aborts in-flight transfer, deletes this row's
+ * partial (`<target_path>.<downloadId>.partial`), marks the row
+ * 'cancelled'. The shared final GGUF at `<target_path>` is left alone.
  */
 export async function cancelDownload(downloadId: string): Promise<boolean> {
   const row = await modelDownloadRepository.findById(downloadId);
@@ -242,6 +243,15 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
   }
 
   const partialPath = partialPathFor(download.target_path, download.id);
+  // Migration: PR #247 wrote to a non-namespaced `<target>.partial`.
+  // After this PR, partials are namespaced by download row id. Any
+  // pre-namespacing partial on disk is orphan junk from before the
+  // upgrade — clean it up unconditionally so it doesn't waste space
+  // forever.
+  const legacyPartial = `${download.target_path}.partial`;
+  if (existsSync(legacyPartial)) {
+    try { unlinkSync(legacyPartial); } catch { /* best effort */ }
+  }
   // Only resume from the partial if THIS row had progress recorded.
   // A fresh row (bytes_downloaded === 0) finding a partial on disk
   // means the previous attempt didn't clean up — e.g., a cancel that
@@ -414,17 +424,34 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
     return;
   }
   await modelDownloadRepository.setStatus(download.id, 'installing');
-  // Atomic rename — partial → final. Same filesystem so this is O(1).
-  try {
-    if (existsSync(download.target_path)) unlinkSync(download.target_path);
-    renameSync(partialPath, download.target_path);
-  } catch (err) {
-    inFlight.delete(download.id);
-    if (handle.cancelled) return;
-    await modelDownloadRepository.setStatus(download.id, 'failed', {
-      error: `install (rename) failed: ${err instanceof Error ? err.message : String(err)}`,
-    });
-    return;
+  // The final target is shared across users (content-addressable —
+  // we just verified SHA-256 above, so any existing file at this
+  // path is by definition the same model). If another download
+  // already installed it (concurrent same-model fetch from a
+  // different user/row), skip the rename entirely and just discard
+  // our partial. The previous "unlink target → rename" sequence had
+  // a window where a renameSync failure could leave the host with
+  // no GGUF after we'd already deleted the good one.
+  if (existsSync(download.target_path)) {
+    try { unlinkSync(partialPath); } catch { /* best effort */ }
+  } else {
+    try {
+      renameSync(partialPath, download.target_path);
+    } catch (err) {
+      // Race fallback: someone else won between our existsSync and
+      // our rename. If the target now exists, treat as installed —
+      // otherwise this is a real install failure.
+      if (existsSync(download.target_path)) {
+        try { unlinkSync(partialPath); } catch { /* best effort */ }
+      } else {
+        inFlight.delete(download.id);
+        if (handle.cancelled) return;
+        await modelDownloadRepository.setStatus(download.id, 'failed', {
+          error: `install (rename) failed: ${err instanceof Error ? err.message : String(err)}`,
+        });
+        return;
+      }
+    }
   }
 
   inFlight.delete(download.id);
