fix(#187 AC#2): cancel through verify/install + stale partial + dead code

jayzalowitz · claude · jayzalowitz · commit ccb27086c49a · 2026-05-09T14:23:01.000-04:00
Four findings from Copilot's re-review of the security/correctness fixes:

- **Cancel couldn't stop verify/install**: the runner removed the
  inFlight handle BEFORE verification, so cancelDownload() couldn't
  abort SHA-256 or rename. After verify completed, the runner would
  overwrite the user's cancelled status with complete/failed. Now
  the handle stays in inFlight through verify+install, and the runner
  checks handle.cancelled at every phase boundary (pre-verify,
  post-hash, pre-install, post-rename) and bails without writing
  terminal status if cancel fired. inFlight.delete moved to the
  final boundary.

- **Stale .partial corrupting fresh resume**: a previous attempt that
  failed to delete .partial (e.g., Windows file lock during cancel)
  could cause a brand-new download row to "resume" from stale bytes.
  Now we only resume when bytes_downloaded &gt; 0 on the row; otherwise
  we delete any pre-existing partial before starting fresh.

- **UI showed Pause for verifying/installing but backend rejected it**:
  pauseDownload() returns ok:false for those statuses. Added
  PAUSABLE_STATUSES set (pending + downloading only); the action
  bar shows Cancel-only during verify/install. Cancel now actually
  works through those phases (per the runner change above).

- **ensureDirectory() dead code**: exported but unused. Removed,
  along with the now-unused `dirname` import.

Build clean. 25 route tests passing.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/apps/api/src/embedded-llm/downloader.ts b/apps/api/src/embedded-llm/downloader.ts
@@ -8,7 +8,7 @@ import {
   statSync,
   unlinkSync,
 } from 'node:fs';
-import { dirname, join } from 'node:path';
+import { join } from 'node:path';
 import { homedir } from 'node:os';
 import { Readable } from 'node:stream';
 import { pipeline } from 'node:stream/promises';
@@ -218,13 +218,20 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
   }
 
   const partialPath = partialPathFor(download.target_path);
+  // 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
+  // failed to unlink on Windows. Resuming from those stale bytes
+  // would corrupt the download.
   let resumeFrom = 0;
-  if (existsSync(partialPath)) {
+  if (download.bytes_downloaded > 0 && existsSync(partialPath)) {
     try {
       resumeFrom = statSync(partialPath).size;
     } catch {
       resumeFrom = 0;
     }
+  } else if (existsSync(partialPath)) {
+    try { unlinkSync(partialPath); } catch { /* best effort */ }
   }
 
   const controller = new AbortController();
@@ -336,7 +343,13 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
     return;
   }
 
-  inFlight.delete(download.id);
+  // Bail if the user clicked Cancel during the byte transfer. The
+  // network stream finishes after a controller.abort(), so we may
+  // arrive here with handle.cancelled already true.
+  if (handle.cancelled) {
+    inFlight.delete(download.id);
+    return;
+  }
   await modelDownloadRepository.setStatus(download.id, 'verifying', {
     bytesDownloaded: totalBytes,
   });
@@ -352,10 +365,18 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
     // Stream the file through the hash — multi-GB GGUFs would OOM the
     // API process if we readFile()'d the whole thing into memory.
     const actual = await computeSha256(partialPath);
+    // Cancel can fire mid-hash. If it did, cancelDownload() already
+    // marked the row 'cancelled' and removed the partial — don't
+    // overwrite that with verify's outcome.
+    if (handle.cancelled) {
+      inFlight.delete(download.id);
+      return;
+    }
     if (actual !== expectedHash) {
       // Corrupt download. Delete the partial — user should retry,
       // and we don't want a half-bad GGUF lingering on disk.
       try { unlinkSync(partialPath); } catch { /* best effort */ }
+      inFlight.delete(download.id);
       await modelDownloadRepository.setStatus(download.id, 'failed', {
         error: `sha256 mismatch: expected ${expectedHash}, got ${actual}`,
         bytesDownloaded: 0,
@@ -364,18 +385,26 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
     }
   }
 
+  if (handle.cancelled) {
+    inFlight.delete(download.id);
+    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;
   }
 
+  inFlight.delete(download.id);
+  if (handle.cancelled) return;
   await modelDownloadRepository.setStatus(download.id, 'complete');
   log.info('Model download complete', {
     downloadId: download.id,
@@ -403,15 +432,6 @@ export async function recoverOnBoot(): Promise<void> {
   }
 }
 
-/**
- * Helper used by the dirname-only check in tests. Exported so the
- * route handler doesn't need to import `node:path` separately.
- */
-export function ensureDirectory(filePath: string): void {
-  const dir = dirname(filePath);
-  if (!existsSync(dir)) mkdirSync(dir, { recursive: true });
-}
-
 /**
  * Stream the file through a SHA-256 hash. Multi-GB GGUFs would OOM
  * the API process if we loaded them into memory whole.
diff --git a/apps/web/public/js/components/embedded-llm-card.js b/apps/web/public/js/components/embedded-llm-card.js
@@ -35,7 +35,14 @@ function getCurrentUserId() {
   }
 }
 
+// Statuses that mean "something is in flight" — drives the polling
+// loop and keeps the card showing a progress UI rather than the picker.
 const ACTIVE_STATUSES = new Set(['pending', 'downloading', 'verifying', 'installing']);
+// Subset where the backend can actually pause. pauseDownload() returns
+// ok:false for verifying/installing, so we hide the Pause button there.
+// Cancel still works through verify/install — the runner checks the
+// cancelled flag at phase boundaries.
+const PAUSABLE_STATUSES = new Set(['pending', 'downloading']);
 
 const POLL_INTERVAL_MS = 1000;
 
@@ -94,12 +101,16 @@ function downloadCardHtml(download, modelName) {
   const bytesSoFar = formatBytes(download.bytesDownloaded);
   const totalSize = formatBytes(download.totalBytes);
   const isActive = ACTIVE_STATUSES.has(status);
+  const isPausable = PAUSABLE_STATUSES.has(status);
   const isPaused = status === 'paused';
   const isError = status === 'failed';
 
   const actionButtons = (() => {
     if (isActive) {
-      return `<button class="btn btn-outline btn-sm" data-action="embedded-pause-download" data-download-id="${escapeHtml(download.id)}">Pause</button>
+      const pauseBtn = isPausable
+        ? `<button class="btn btn-outline btn-sm" data-action="embedded-pause-download" data-download-id="${escapeHtml(download.id)}">Pause</button>`
+        : '';
+      return `${pauseBtn}
               <button class="btn btn-outline btn-sm" data-action="embedded-cancel-download" data-download-id="${escapeHtml(download.id)}">Cancel</button>`;
     }
     if (isPaused) {
