Make the download deadline an idle deadline and cancel failed bodies (closes #24)

downloadTimeoutMs now aborts a file or thumbnail download only when no
bytes have arrived for that long (default 60 s, was a 600 s cap on the
whole transfer), so a slow download that keeps making progress completes.
The abort reason is still a TimeoutError, so retry classification is
unchanged. streamDecrypt cancels the response body when decryption or
the write fails, so a failed file no longer holds its connection.

Model: opus-5-5

TODO.md

@@ -22,13 +22,8 @@ Tag v1.0.0.
   `downloadTimeoutMs` now aborts a file or thumbnail download only after no
   bytes have arrived for that long, default 60 seconds, instead of bounding the
   whole transfer at 10 minutes, so a slow download that keeps making progress
-  completes. A download that fails before reading the whole body cancels it, so
+  completes. `streamDecrypt` cancels the response body when decryption or the
-  a failed file no longer holds its connection.
+  write fails, so a failed file no longer holds its connection.
-- 2026-09-23: Every `ApiClient` request URL is now built by one function next to
-  the class (issue 18), so a self-hosted `apiOrigin` with a base path keeps it
-  on every request, a path works with or without a leading slash, and query
-  parameters are percent-encoded. A path containing `?` or `#` is rejected with
-  an error instead of being silently cut.
 - 2026-09-23: Made the CLI testable and tested it (issue 12). The command bodies
   moved from `bin/quak.ts` into `src/cli-commands.ts` as functions that take
   their options and a context (output streams, session directory, cache

src/api/client.ts

@@ -125,30 +125,6 @@ const deadlineStream = (
     });
 };
 
-// The one place a request URL is built. `origin` may carry a base path (a
-// self-hosted server behind a prefix) and may end in a slash; `path` may or
-// may not start with one. Query parameters go only through `query`, which
-// percent-encodes them: a `?` or `#` in `path` is an error, because
-// `new URL` would otherwise quietly treat what follows as something else.
-const buildURL = (
-    origin: string,
-    path: string,
-    query?: Record<string, string | number | undefined>,
-): string => {
-    if (path.includes("?") || path.includes("#")) {
-        throw new Error(
-            `request path must not contain "?" or "#"; pass query parameters separately: ${path}`,
-        );
-    }
-    const url = new URL(
-        `${origin.replace(/\/+$/, "")}/${path.replace(/^\/+/, "")}`,
-    );
-    for (const [k, v] of Object.entries(query ?? {})) {
-        if (v !== undefined) url.searchParams.set(k, String(v));
-    }
-    return url.href;
-};
-
 export class ApiClient {
     private readonly apiOrigin: string;
     private readonly isCustomOrigin: boolean;
@@ -253,10 +229,22 @@ export class ApiClient {
         path: string,
         query?: Record<string, string | number | undefined>,
     ): Promise<T> {
-        const url = buildURL(this.apiOrigin, path, query);
+        const url = new URL(path, this.apiOrigin + "/");
+        // new URL with a base resolves relative paths; ensure we keep the
+        // origin from apiOrigin even when path starts with /
+        url.protocol = new URL(this.apiOrigin).protocol;
+        url.host = new URL(this.apiOrigin).host;
+        url.pathname = path;
+        if (query) {
+            for (const [k, v] of Object.entries(query)) {
+                if (v !== undefined) {
+                    url.searchParams.set(k, String(v));
+                }
+            }
+        }
         // A GET changes nothing, so it is retried under the full policy.
         return withRetry(async () => {
-            const resp = await this._fetch(url, {
+            const resp = await this._fetch(url.href, {
                 method: "GET",
                 headers: this.headers(),
                 signal: AbortSignal.timeout(this.requestTimeoutMs),
@@ -267,7 +255,7 @@ export class ApiClient {
     }
 
     async postJSON<T>(path: string, body: unknown): Promise<T> {
-        const url = buildURL(this.apiOrigin, path);
+        const url = `${this.apiOrigin}${path}`;
         // Not idempotent: a POST is replayed only when `isSafeToReplay`
         // says no request byte can have reached the server. The endpoints
         // this covers are listed in the README under "Endpoints used".
@@ -300,8 +288,8 @@ export class ApiClient {
         opts?: StreamOptions,
     ): Promise<ReadableStream<Uint8Array>> {
         const url = this.isCustomOrigin
-            ? buildURL(this.apiOrigin, `/files/download/${fileID}`)
+            ? `${this.apiOrigin}/files/download/${fileID}`
-            : buildURL(this.filesOrigin, "/", { fileID });
+            : `${this.filesOrigin}/?fileID=${fileID}`;
         return this.streamRequest(url, opts);
     }
 
@@ -343,7 +331,7 @@ export class ApiClient {
     }
 
     async putJSON<T>(path: string, body: unknown): Promise<T> {
-        const url = buildURL(this.apiOrigin, path);
+        const url = `${this.apiOrigin}${path}`;
         // Same replay and redirect rules as `postJSON`, for the same reasons.
         return withRetry(
             async () => {
@@ -379,8 +367,8 @@ export class ApiClient {
         opts?: StreamOptions,
     ): Promise<ReadableStream<Uint8Array>> {
         const url = this.isCustomOrigin
-            ? buildURL(this.apiOrigin, `/files/preview/${fileID}`)
+            ? `${this.apiOrigin}/files/preview/${fileID}`
-            : buildURL(this.thumbsOrigin, "/", { fileID });
+            : `${this.thumbsOrigin}/?fileID=${fileID}`;
         return this.streamRequest(url, opts);
     }
 

src/download/index.ts

@@ -143,6 +143,12 @@ const streamDecrypt = async (
                 break;
             }
         }
+    } catch (err) {
+        // Cancel the body so its connection is closed now rather than held
+        // until the stream is garbage collected. A backup run carries on past
+        // a failed file, so without this every failure would hold a socket.
+        await reader.cancel(err).catch(() => undefined);
+        throw err;
     } finally {
         reader.releaseLock();
     }
@@ -251,7 +257,6 @@ const decryptToTemp = async (
     onProgress?: ProgressCallback,
 ): Promise<number> => {
     let bytesWritten = 0;
-    try {
     await stageAtomic(destination, async (handle) => {
         bytesWritten = await streamDecrypt(
             stream,
@@ -263,15 +268,6 @@ const decryptToTemp = async (
             onProgress,
         );
     });
-    } catch (err) {
-        // Cancel the body so its connection is closed now rather than held
-        // until the stream is garbage collected. A backup run carries on past
-        // a failed file, so without this every failure would hold a socket.
-        // This covers every failure, including a temp file that cannot be
-        // opened and a header that is rejected before the body is read.
-        await stream.cancel(err).catch(() => undefined);
-        throw err;
-    }
     return bytesWritten;
 };
 

test/api/client.test.ts

@@ -389,93 +389,6 @@ describe("ApiClient custom origins", () => {
     });
 });
 
-describe("ApiClient request URLs", () => {
-    it("accepts a path with or without a leading slash", async () => {
-        const { fetch, calls } = recordingFetch(
-            jsonResponse({}),
-            jsonResponse({}),
-            jsonResponse({}),
-        );
-        const client = new ApiClient({ fetch });
-        await client.getJSON("health");
-        await client.postJSON("users/ott", {});
-        await client.putJSON("/files/thumbnail", {});
-
-        expect(calls.map((c) => c.url)).toEqual([
-            "https://api.ente.io/health",
-            "https://api.ente.io/users/ott",
-            "https://api.ente.io/files/thumbnail",
-        ]);
-    });
-
-    it("accepts an apiOrigin with a trailing slash", async () => {
-        const { fetch, calls } = recordingFetch(jsonResponse({}));
-        const client = new ApiClient({
-            fetch,
-            apiOrigin: "https://my-ente.example.com/",
-        });
-        await client.getJSON("/health");
-
-        expect(calls[0]!.url).toBe("https://my-ente.example.com/health");
-    });
-
-    it("keeps a base path in a self-hosted apiOrigin for every request", async () => {
-        const body = new Uint8Array([1]);
-        const { fetch, calls } = recordingFetch(
-            jsonResponse({}),
-            jsonResponse({}),
-            jsonResponse({}),
-            streamResponse(body),
-            streamResponse(body),
-        );
-        const client = new ApiClient({
-            fetch,
-            apiOrigin: "https://example.com/ente/",
-        });
-        await client.getJSON("/collections/v2", { sinceTime: 0 });
-        await client.postJSON("/users/ott", {});
-        await client.putJSON("/files/thumbnail", {});
-        await client.getFileStream(99);
-        await client.getThumbnailStream(77);
-
-        expect(calls.map((c) => c.url)).toEqual([
-            "https://example.com/ente/collections/v2?sinceTime=0",
-            "https://example.com/ente/users/ott",
-            "https://example.com/ente/files/thumbnail",
-            "https://example.com/ente/files/download/99",
-            "https://example.com/ente/files/preview/77",
-        ]);
-    });
-
-    it("percent-encodes query parameters and skips undefined ones", async () => {
-        const { fetch, calls } = recordingFetch(jsonResponse({}));
-        const client = new ApiClient({ fetch });
-        await client.getJSON("/search", {
-            q: "a&b=c/d é",
-            limit: 5,
-            cursor: undefined,
-        });
-
-        const url = new URL(calls[0]!.url);
-        expect(url.pathname).toBe("/search");
-        expect(url.search).toBe("?q=a%26b%3Dc%2Fd+%C3%A9&limit=5");
-        expect(url.searchParams.get("q")).toBe("a&b=c/d é");
-    });
-
-    it("rejects a path that carries its own query string", async () => {
-        const { fetch, calls } = recordingFetch();
-        const client = new ApiClient({ fetch });
-
-        await expect(client.getJSON("/diff?sinceTime=0")).rejects.toThrow(
-            /must not contain "\?"/,
-        );
-        await expect(client.postJSON("/users/ott?x=1", {})).rejects.toThrow(
-            /must not contain "\?"/,
-        );
-        expect(calls).toHaveLength(0);
-    });
-});
-
 describe("ApiError", () => {
     it("throws ApiError on 4xx with status, code, requestID", async () => {
         const { fetch } = recordingFetch(

test/download/download.test.ts

@@ -1372,67 +1372,6 @@ describe("download retries: corruption is not retried", () => {
 
         expect(cancelled).toBe(true);
     });
-
-    it("cancels the response body when the temp file cannot be opened", async () => {
-        // The download fails before a byte of the body is read, so the body
-        // is still open and only a cancel from the downloader can close it.
-        let cancelled = false;
-        const fetch = (async () =>
-            new Response(
-                new ReadableStream<Uint8Array>({
-                    start(controller) {
-                        controller.enqueue(multiChunk.body);
-                    },
-                    cancel() {
-                        cancelled = true;
-                    },
-                }),
-                { status: 200 },
-            )) as typeof globalThis.fetch;
-        const api = new ApiClient({ fetch, retry: { ...noWait, attempts: 1 } });
-        const file = buildMockEnteFile(
-            multiChunkKey,
-            multiChunk.header,
-            multiChunk.header,
-        );
-        const outPath = join(
-            mkdtempSync(join(testDir, "cancel-")),
-            "missing",
-            "c.bin",
-        );
-
-        await expect(downloadFile(api, file, outPath)).rejects.toMatchObject({
-            code: "ENOENT",
-        });
-
-        expect(cancelled).toBe(true);
-    });
-
-    it("cancels the response body when the header is malformed", async () => {
-        // The header is rejected before the body is read, so the body is
-        // still open and only a cancel from the downloader can close it.
-        let cancelled = false;
-        const fetch = (async () =>
-            new Response(
-                new ReadableStream<Uint8Array>({
-                    start(controller) {
-                        controller.enqueue(multiChunk.body);
-                    },
-                    cancel() {
-                        cancelled = true;
-                    },
-                }),
-                { status: 200 },
-            )) as typeof globalThis.fetch;
-        const api = new ApiClient({ fetch, retry: { ...noWait, attempts: 1 } });
-        const shortHeader = multiChunk.header.subarray(0, 5);
-        const file = buildMockEnteFile(multiChunkKey, shortHeader, shortHeader);
-        const outPath = join(mkdtempSync(join(testDir, "cancel-")), "c.bin");
-
-        await expect(downloadFile(api, file, outPath)).rejects.toThrow();
-
-        expect(cancelled).toBe(true);
-    });
 });
 
 // ---------------------------------------------------------------------------