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. A download that fails before reading the whole response body
cancels it, including when the temp file cannot be opened or the header
is malformed, so a failed file no longer holds its connection.

Model: opus-5-5

TODO.md

@@ -22,8 +22,13 @@ 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. `streamDecrypt` cancels the response body when decryption or the
-  write fails, so a failed file no longer holds its connection.
+  completes. A download that fails before reading the whole body cancels it, 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,6 +125,30 @@ 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;
@@ -229,22 +253,10 @@ export class ApiClient {
         path: string,
         query?: Record<string, string | number | undefined>,
     ): Promise<T> {
-        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));
-                }
-            }
-        }
+        const url = buildURL(this.apiOrigin, path, query);
         // A GET changes nothing, so it is retried under the full policy.
         return withRetry(async () => {
-            const resp = await this._fetch(url.href, {
+            const resp = await this._fetch(url, {
                 method: "GET",
                 headers: this.headers(),
                 signal: AbortSignal.timeout(this.requestTimeoutMs),
@@ -255,7 +267,7 @@ export class ApiClient {
     }
 
     async postJSON<T>(path: string, body: unknown): Promise<T> {
-        const url = `${this.apiOrigin}${path}`;
+        const url = buildURL(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".
@@ -288,8 +300,8 @@ export class ApiClient {
         opts?: StreamOptions,
     ): Promise<ReadableStream<Uint8Array>> {
         const url = this.isCustomOrigin
-            ? `${this.apiOrigin}/files/download/${fileID}`
-            : `${this.filesOrigin}/?fileID=${fileID}`;
+            ? buildURL(this.apiOrigin, `/files/download/${fileID}`)
+            : buildURL(this.filesOrigin, "/", { fileID });
         return this.streamRequest(url, opts);
     }
 
@@ -331,7 +343,7 @@ export class ApiClient {
     }
 
     async putJSON<T>(path: string, body: unknown): Promise<T> {
-        const url = `${this.apiOrigin}${path}`;
+        const url = buildURL(this.apiOrigin, path);
         // Same replay and redirect rules as `postJSON`, for the same reasons.
         return withRetry(
             async () => {
@@ -367,8 +379,8 @@ export class ApiClient {
         opts?: StreamOptions,
     ): Promise<ReadableStream<Uint8Array>> {
         const url = this.isCustomOrigin
-            ? `${this.apiOrigin}/files/preview/${fileID}`
-            : `${this.thumbsOrigin}/?fileID=${fileID}`;
+            ? buildURL(this.apiOrigin, `/files/preview/${fileID}`)
+            : buildURL(this.thumbsOrigin, "/", { fileID });
         return this.streamRequest(url, opts);
     }
 

src/download/index.ts

@@ -143,12 +143,6 @@ 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();
     }
@@ -257,6 +251,7 @@ const decryptToTemp = async (
     onProgress?: ProgressCallback,
 ): Promise<number> => {
     let bytesWritten = 0;
+    try {
         await stageAtomic(destination, async (handle) => {
             bytesWritten = await streamDecrypt(
                 stream,
@@ -268,6 +263,15 @@ 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,6 +389,93 @@ 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,6 +1372,67 @@ 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);
+    });
 });
 
 // ---------------------------------------------------------------------------