fix(1370): stat the partial instead of asserting it (#1392)

artokun · claude · web-flow · commit ba7bf2e816bf · 2026-08-11T03:32:35.000-06:00
* wip(1370): claim the unverified resumable-partial claim Refs #1370 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1370): stat the partial instead of asserting it `download_model action:"status"` told users: cancelled — the partial was left on disk and can be resumed by re-issuing the download (it picks up where it left off) Nothing ever checked. The string was selected from `status === "cancelled"` plus two flags; no code stat'd the file. The reporter paused a 33 GB download BECAUSE of that sentence, scanned their whole install tree, found no partial of any kind, and restarted from zero. The message was not stale. It was never an observation. What makes it worse is that the neighbouring branches were already careful. A ComfyUI-Manager dispatch correctly says there is NO local partial. A reclaimed-dead writer correctly says one MAY exist and was left untouched. `afterCancelAdvice("local")` correctly hedges with "resumes any .partial the dead writer left, or restarts cleanly". Only the ordinary cancel stated a fact about the filesystem without consulting it — surrounded by code that knew better. Both cancelled branches now report what is on disk: the partial's SIZE when there is one, so "resumable" is something the user can weigh against restarting, or its absence, so they can decide before spending the bandwidth rather than after. A cancel that leaves nothing is not an error; telling someone it left something is. SEVERAL CANDIDATE NAMES, not one. The staged file is `.<basename>.partial` in the download cache, and a cancelled job knows its destination by routes of differing reliability — `filename`, the landed path, the destination key, the URL's last segment. A few stats are cheap, and guessing one wrong would reintroduce the same false claim inverted: reporting "nothing to resume" over a 30 GB partial that is sitting right there. A zero-byte partial reports as absent, because resuming from it saves nothing and calling it resumable is the original bug in miniature. Only rows that could HAVE a local partial are checked — a Manager dispatch never writes one and already says so. DELIBERATELY NOT IN SCOPE: why the reporter's bytes were missing. Candidates include a Manager-side dispatch whose `via_manager` flag was stripped (#1197) and local downloads being routed through Manager at all (#1374). Those are separate issues, and this message has to be honest under every one of them rather than depending on which is true. Mutation-tested: counting a zero-byte file as resumable, and dropping the basename extraction, each kill a test. Fixes #1370 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1370): derive the partial path the way the WRITER does, not the way it sounds Caught while writing my own review prompt, before anyone else saw it. The fix I had just committed was wrong in the same shape as the bug it fixes, pointing the other way. I looked for `.<destination filename>.partial` in the cache dir, because that is what "the partial for this download" sounds like. The writer stages under `.<sha256(cacheIdentity)[0:32]><ext>.partial` — keyed by the CACHE identity (cachePathForUrl), not the destination. So the lookup would have missed every real partial and told users "NO partial was found on disk" for downloads that had one. That is worse than the original defect. The old message at least erred toward "your bytes are safe"; mine would have told someone with a 30 GB resumable partial to start over. MY TESTS COULD NOT HAVE CAUGHT IT, because the fixtures created files under the name I was searching for. They encoded my belief about the naming rather than the naming — the third time that exact shape has cost me today, after an index-based removeWidget double and an empty-graph object_info fixture. The fixtures now build their files from `stagedPartialPathForUrl`, the same function production uses, so the two cannot disagree; one test pins that the staged name is hashed and does NOT contain the destination filename. SCOPE, stated in the message rather than papered over: cacheIdentity folds in representation-affecting request headers and cloud credentials, which a job record deliberately does not keep. So this reproduces the staged path exactly for an unauthenticated public download — the reporter's case, and the common one — and cannot for an authenticated variant. A miss is therefore reported as "none found under this URL's staged name", explicitly not as "none exists". Having just shipped one over-claim I am not going to hide a smaller one. Also drops the multi-candidate search: with the path derived correctly there is exactly one place to look, and trying several names could only ever match a DIFFERENT download's partial and report its bytes as resumable. Mutation-tested: reverting to the filename-based guess kills a test. Refs #1370 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1370): the staged partial is HIDDEN, and there is now one definition of its path Codex round 2, and this is the second time I got the same path wrong. Round 1: searched `.<destination filename>.partial` — wrong KEY, since the writer stages by cache identity. Round 2 (my "fix"): derived the cache path correctly and DROPPED THE LEADING DOT, so it looked for `hash.ext.partial` while the writer writes `.hash.ext.partial`. Still missed every real partial. Still would have told a user holding 30 GB of resumable bytes to start over — the inverted false claim this issue is about, shipped in the commit that claimed to prevent it. BOTH TIMES MY TESTS AGREED WITH ME, because the fixtures were built by calling the same helper under test. A fixture derived from the code under test cannot falsify it. The previous version even pinned the name with `/[0-9a-f]{32}\.safetensors\.partial$/`, which matches equally well with and without the dot. So the fix is structural rather than another careful correction: ONE exported `stagedPartialPathForTarget`, and the WRITER now calls it. Not two derivations that agree today — the same function, so they cannot disagree. A test asserts the writer routes through it and that the hand-rolled expression exists in exactly one place; another asserts the leading dot on its own, separately from the hash, because the combined regex is what hid it. Mutation-tested: dropping the dot fails 2 tests, and re-inlining the path at the writer fails the wiring test. Refs #1370 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
diff --git a/src/__tests__/tools/download-cancel-partial.test.ts b/src/__tests__/tools/download-cancel-partial.test.ts
@@ -0,0 +1,131 @@
+// #1370 — `download_model action:"status"` told users "the partial was left on disk and
+// can be resumed by re-issuing the download". Nothing ever stat'd the file. The reporter
+// paused a 33 GB download BECAUSE of that sentence, found no partial anywhere, and
+// restarted from zero.
+//
+// THE FIXTURES HERE ARE BUILT FROM THE PRODUCTION PATH FUNCTION, NOT FROM A NAME I TYPED.
+// My first version of this file created `.<destination filename>.partial` and searched for
+// the same string, so it passed while production stages under
+// `.<sha256(cacheIdentity)[0:32]><ext>.partial` — keyed by the CACHE identity, not the
+// destination. The lookup would have missed every real partial and reported "no partial
+// found" for downloads that had one: this issue's own bug inverted, and worse, because the
+// original at least erred toward "your bytes are safe".
+//
+// A test whose fixture encodes the belief under test cannot fail for the reason it exists.
+// Deriving the path from `stagedPartialPathForUrl` is what makes the two impossible to
+// disagree.
+
+import { describe, expect, it, afterEach } from "vitest";
+import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises";
+import { basename, dirname } from "node:path";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+import { findResumablePartial, stagedPartialPathForUrl } from "../../services/download-cache.js";
+
+const URL_A = "https://huggingface.co/Comfy-Org/flux2-dev/resolve/main/flux2_dev_fp8mixed.safetensors";
+const URL_B = "https://huggingface.co/Comfy-Org/other/resolve/main/mistral_3_small.safetensors";
+
+const dirs: string[] = [];
+afterEach(async () => {
+  for (const d of dirs.splice(0)) await rm(d, { recursive: true, force: true });
+  delete process.env.COMFYUI_DOWNLOAD_CACHE_DIR;
+});
+
+async function useTempCache(): Promise<void> {
+  const dir = await mkdtemp(join(tmpdir(), "dl-partial-"));
+  dirs.push(dir);
+  process.env.COMFYUI_DOWNLOAD_CACHE_DIR = dir;
+}
+
+/** Stage a partial exactly where the writer would put it for this URL. */
+async function stagePartialFor(url: string, bytes: number): Promise<string> {
+  const p = stagedPartialPathForUrl(url);
+  await mkdir(dirname(p), { recursive: true });
+  await writeFile(p, Buffer.alloc(bytes, 1));
+  return p;
+}
+
+describe("findResumablePartial reports what is ACTUALLY staged (#1370)", () => {
+  it("finds the partial the writer would have staged for this URL", async () => {
+    await useTempCache();
+    const staged = await stagePartialFor(URL_A, 4096);
+    const found = await findResumablePartial(URL_A);
+    expect(found?.bytes).toBe(4096);
+    expect(found?.path).toBe(staged);
+  });
+
+  it("the staged name is HASHED and HIDDEN — both details cost a round", async () => {
+    // Two separate mistakes, one per review round:
+    //   1. searched `.<destination filename>.partial` — wrong KEY (the writer stages by
+    //      cache identity, not destination);
+    //   2. derived the cache path correctly and dropped the LEADING DOT, so it looked for
+    //      `hash.ext.partial` while the writer wrote `.hash.ext.partial`.
+    // Each time the lookup found nothing and would have told a user holding 30 GB of
+    // resumable bytes to start over. The dot is asserted separately because the previous
+    // version of this test used `/[0-9a-f]{32}\.safetensors\.partial$/`, which matches
+    // happily with OR without it.
+    await useTempCache();
+    const p = stagedPartialPathForUrl(URL_A);
+    const name = basename(p);
+    expect(p, "must not be keyed on the destination filename").not.toMatch(/flux2_dev_fp8mixed/);
+    expect(name.startsWith("."), `staged file must be hidden, got ${name}`).toBe(true);
+    expect(name).toMatch(/^\.[0-9a-f]{32}\.safetensors\.partial$/);
+  });
+
+  it("WIRING: the WRITER stages through the same function the lookup reads", async () => {
+    // The root cause of both rounds was two parallel derivations of one path, each
+    // confident and one wrong. A test that only compares the helper against itself cannot
+    // see that — my fixtures were built by calling the helper under test, so they agreed
+    // with every wrong version of it.
+    //
+    // This asserts the writer does not hand-roll the path any more. It is the only check
+    // here that could have failed while all the others passed.
+    const { readFileSync } = await import("node:fs");
+    const src = readFileSync(new URL("../../services/download-cache.ts", import.meta.url), "utf8")
+      .replace(/\/\*[\s\S]*?\*\//g, "")
+      .replace(/\/\/.*/g, "");
+    expect(src).toMatch(/const partial = stagedPartialPathForTarget\(target\);/);
+    // …and the hand-rolled expression exists in exactly ONE place: the shared helper.
+    const handRolled = src.match(/join\(cacheDir\(\), `\.\$\{basename\([a-z]+\)\}\.partial`\)/g) ?? [];
+    expect(handRolled.length, "the staged path must be built in exactly one place").toBe(1);
+  });
+
+  it("returns NULL when nothing is staged — the reporter's case", async () => {
+    await useTempCache();
+    expect(await findResumablePartial(URL_A)).toBeNull();
+  });
+
+  it("does not report ANOTHER download's partial as this one's", async () => {
+    // The staged name is keyed on the URL, so two downloads never share one. Reporting a
+    // neighbour's bytes as resumable would be a new false claim, not a fix for the old one.
+    await useTempCache();
+    await stagePartialFor(URL_B, 8192);
+    expect(await findResumablePartial(URL_A)).toBeNull();
+    expect((await findResumablePartial(URL_B))?.bytes).toBe(8192);
+  });
+
+  it("a ZERO-BYTE partial is reported as absent", async () => {
+    // Resuming from it saves nothing, and calling it resumable restates this bug in
+    // miniature: a claim of retained bytes where there are none.
+    await useTempCache();
+    await stagePartialFor(URL_A, 0);
+    expect(await findResumablePartial(URL_A)).toBeNull();
+  });
+
+  it("ignores the COMPLETED cache entry — only the staged .partial is resumable", async () => {
+    await useTempCache();
+    const partial = stagedPartialPathForUrl(URL_A);
+    const completed = partial.replace(/\.partial$/, "");
+    await mkdir(dirname(completed), { recursive: true });
+    await writeFile(completed, Buffer.alloc(8192, 1));
+    expect(await findResumablePartial(URL_A)).toBeNull();
+  });
+
+  it("a missing or unparseable URL yields null rather than throwing", async () => {
+    await useTempCache();
+    expect(await findResumablePartial(undefined)).toBeNull();
+    expect(await findResumablePartial("")).toBeNull();
+    expect(await findResumablePartial("not a url")).toBeNull();
+  });
+});
diff --git a/src/services/download-cache.ts b/src/services/download-cache.ts
@@ -2347,7 +2347,7 @@ async function downloadIntoCache(
     // resumes from the byte it left off on the next call, rather than
     // restarting from zero. (See streamUrlToFile for the Range + flags
     // handshake.) Cleanup on terminal failure stays unchanged.
-    const partial = join(cacheDir(), `.${basename(target)}.partial`);
+    const partial = stagedPartialPathForTarget(target);
     const rejectedMarker = `${partial}.rejected`;
 
     /**
@@ -3302,3 +3302,77 @@ export async function downloadWithCache(
     return { targetPath: options.targetPath, usedCache: false };
   }
 }
+
+/**
+ * The staged `.partial` path for a cache target. THE ONE DEFINITION (#1370).
+ *
+ * It is a HIDDEN file — `.<basename>.partial`, leading dot — in the cache dir. That dot
+ * cost two rounds. My first lookup searched for `.<destination filename>.partial` (wrong
+ * key: the writer stages by CACHE identity, not destination). The correction derived the
+ * cache path properly and then dropped the leading dot, so it looked for `hash.ext.partial`
+ * while the writer wrote `.hash.ext.partial` — still missing every real partial, still
+ * reporting "no partial found" to someone holding 30 GB of resumable bytes.
+ *
+ * Both times my tests agreed with me, because the fixtures were built by calling the same
+ * helper being tested. A fixture derived from the code under test cannot falsify it.
+ *
+ * So there is now exactly one expression, and the WRITER uses it too. Not a parallel
+ * derivation that happens to agree today — the same function, so they cannot disagree.
+ */
+export function stagedPartialPathForTarget(target: string): string {
+  return join(cacheDir(), `.${basename(target)}.partial`);
+}
+
+/**
+ * Where a download's resumable `.partial` is staged, derived the way the writer derives it
+ * (#1370).
+ *
+ * MY FIRST VERSION OF THIS GUESSED THE NAME AND GUESSED WRONG. It looked for
+ * `.<destination filename>.partial`, because that is what "the partial for this download"
+ * sounds like. The writer stages under `.<sha256(cacheIdentity)[0:32]><ext>.partial` —
+ * keyed by the CACHE identity, not the destination — so the lookup would have missed every
+ * time and reported "no partial was found" for downloads that had one. That is the same
+ * false claim this issue is about, pointing the other way, and it would have been worse:
+ * the original at least erred toward "your bytes are safe".
+ *
+ * The tests missed it because the fixtures created files under the name I was searching
+ * for. They encoded my belief about the naming rather than the naming, which is the third
+ * time that shape has cost me today. Building the path from `cachePathForUrl` — the same
+ * function the writer uses — is what makes the two definitions impossible to disagree.
+ *
+ * SCOPE, stated because the caller has to phrase its answer around it: `cacheIdentity`
+ * folds in representation-affecting request headers and cloud credentials, which a job
+ * record deliberately does not keep. So this reproduces the staged path exactly for an
+ * unauthenticated public download (the reporter's case, and the common one) and cannot for
+ * an authenticated variant. A miss therefore means "none found under this URL's staged
+ * name", never "none exists" — and the caller must not upgrade it to the latter.
+ */
+export function stagedPartialPathForUrl(url: string): string {
+  return stagedPartialPathForTarget(cachePathForUrl(url));
+}
+
+/**
+ * Stat the staged `.partial` for a URL. Returns null when there is nothing usable there.
+ *
+ * A zero-byte partial reports as absent: resuming from it saves nothing, and calling it
+ * resumable would restate this issue's bug in miniature — a claim of retained bytes where
+ * there are none.
+ */
+export async function findResumablePartial(
+  url: string | undefined,
+): Promise<{ path: string; bytes: number } | null> {
+  if (typeof url !== "string" || !url.trim()) return null;
+  let candidate: string;
+  try {
+    candidate = stagedPartialPathForUrl(url.trim());
+  } catch {
+    return null;
+  }
+  try {
+    const st = await stat(candidate);
+    if (st.isFile() && st.size > 0) return { path: candidate, bytes: st.size };
+  } catch {
+    // ENOENT is the common, expected answer.
+  }
+  return null;
+}
diff --git a/src/tools/model-management.ts b/src/tools/model-management.ts
@@ -26,6 +26,7 @@ import {
   type DownloadJob,
 } from "../services/download-jobs.js";
 import { readDownloadProgress } from "../services/download-progress.js";
+import { findResumablePartial } from "../services/download-cache.js";
 import { errorToToolResult, ModelError } from "../utils/errors.js";
 import {
   downloadCivitaiModelAction,
@@ -222,6 +223,39 @@ function stillWritingClause(route: DownloadRoute): string {
  * re-issue, while the stale note said doing that corrupts the file (codex
  * review). One function, so they cannot drift again.
  */
+/**
+ * What is ACTUALLY on disk for a cancelled download (#1370).
+ *
+ * The row this replaces said "the partial was left on disk and can be resumed by
+ * re-issuing the download (it picks up where it left off)" on the strength of
+ * `status === "cancelled"` alone. A reporter paused a 33 GB download because of that
+ * sentence, found nothing on disk, and restarted from zero. The sentence was not stale —
+ * it was never checked.
+ *
+ * Both answers are useful and neither is a failure: a partial means "re-issue and you keep
+ * these bytes", and no partial means "re-issue and you start over" — which is exactly the
+ * fact someone needs BEFORE spending the bandwidth, not after. The size is included
+ * because "resumable" without a number is not something you can weigh against restarting.
+ */
+function describePartial(partial: { path: string; bytes: number } | null): string {
+  if (!partial) {
+    return (
+      `no resumable partial was found for this URL, so re-issuing very likely starts from ` +
+      `the beginning. That is not an error — but it is worth knowing before you re-spend ` +
+      `the bandwidth on a large file. (The staged file is keyed by the download's cache ` +
+      `identity, which folds in auth headers this record deliberately does not keep, so ` +
+      `for an AUTHENTICATED download this means "none found under the unauthenticated ` +
+      `key", not "none exists".)`
+    );
+  }
+  const gb = partial.bytes / 1024 ** 3;
+  const size = gb >= 1 ? `${gb.toFixed(2)} GB` : `${(partial.bytes / 1024 ** 2).toFixed(1)} MB`;
+  return (
+    `a partial of ${size} is on disk (${partial.path}) and re-issuing the same download ` +
+    `resumes from it.`
+  );
+}
+
 function afterCancelAdvice(route: DownloadRoute): string {
   switch (route) {
     case "manager":
@@ -968,6 +1002,31 @@ async function statusAction(args: {
         // listing actually contains a collision (two writers, one file).
         const idCounts = new Map<string, number>();
         for (const j of list) idCounts.set(j.id, (idCounts.get(j.id) ?? 0) + 1);
+        // #1370 — LOOK, then say. The cancelled row claimed "the partial was left on disk
+        // and can be resumed by re-issuing the download" purely from `status === "cancelled"`;
+        // nothing stat'd the file. A reporter paused a 33 GB download BECAUSE of that
+        // sentence, found no partial anywhere, and restarted from zero.
+        //
+        // Stat'd up front because the row builder below is synchronous, and only for rows
+        // that could have one: a Manager dispatch never writes a local partial and already
+        // says so.
+        //
+        // Keyed by the job's URL, because that is what the WRITER keys the staged file on
+        // (cachePathForUrl). My first version searched for `.<destination filename>.partial`
+        // — what "the partial for this download" sounds like, and not what is on disk. It
+        // would have reported "no partial" for every download that had one: the same false
+        // claim inverted, and pointing the more damaging way, since the original at least
+        // erred toward "your bytes are safe".
+        const partials = new Map<string, { path: string; bytes: number } | null>();
+        await Promise.all(
+          list
+            .filter((j) => j.status === "cancelled" && !j.viaManager)
+            .map(async (j) => {
+              partials.set(`${j.id}\n${j.trayId}`, await findResumablePartial(j.url));
+            }),
+        );
+        const partialFor = (j: DownloadJob): { path: string; bytes: number } | null =>
+          partials.get(`${j.id}\n${j.trayId}`) ?? null;
         const collidingIds = [...idCounts.entries()].filter(([, n]) => n > 1).map(([k]) => k);
         const lines = list.map((j) => {
           const p = readDownloadProgress(j.progressId ?? j.trayId);
@@ -1027,10 +1086,10 @@ async function statusAction(args: {
                         // left by a cancel (none may exist).
                         (j.viaManager
                           ? `\n    cancelled — the previous session's writer was confirmed GONE (its process no longer exists), so a later session closed its stale record; no live transfer was aborted. This was a remote ComfyUI-Manager dispatch: the host MAY still be fetching server-side (no Manager recall API) and there is NO local partial to resume — check list_local_models to see whether the file landed; re-issuing starts a NEW dispatch.`
-                          : `\n    cancelled — the previous session's writer was confirmed GONE (its process no longer exists), so a later session closed its stale record; no live transfer was aborted. Any .partial the dead writer left was untouched — re-issue the download to resume from it, or to restart cleanly if there is none.`)
+                          : `\n    cancelled — the previous session's writer was confirmed GONE (its process no longer exists), so a later session closed its stale record; no live transfer was aborted. ${describePartial(partialFor(j))}`)
                       : j.viaManager
                         ? `\n    cancelled — this was a remote ComfyUI-Manager dispatch, so there is NO local partial to resume, and the host MAY still be fetching server-side (there's no Manager recall API). Re-issuing starts a NEW server-side dispatch (a duplicate, not a resume). Check list_local_models to see whether the file landed before deciding.`
-                        : `\n    cancelled — the partial was left on disk and can be resumed by re-issuing the download (it picks up where it left off)`) +
+                        : `\n    cancelled — ${describePartial(partialFor(j))}`) +
                     // A recovery-critical note from cancellation cleanup (e.g. a previous
                     // destination file preserved under a .bak path because it couldn't be
                     // restored) — surface it so the user can recover, not mask it.
