diff --git a/src/github/utils/image-downloader.ts b/src/github/utils/image-downloader.ts index a2c427c3..ccd9dbde 100644 --- a/src/github/utils/image-downloader.ts +++ b/src/github/utils/image-downloader.ts @@ -34,6 +34,8 @@ const SIGNED_URL_HOST = "private-user-images.githubusercontent.com"; const SIGNED_URL_PATH_REGEX = /^\/[^/]+\/[^/]*-([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})(?:\.[a-z0-9]+)?$/i; +const DEFAULT_IMAGE_DOWNLOAD_TIMEOUT_MS = 30_000; + function extractSignedUrlAssetGuid(signedUrl: string): string | undefined { let parsed: URL; try { @@ -85,13 +87,19 @@ export type CommentWithImages = | IssueBody | PullRequestBody; +type ImageDownloadOptions = { + timeoutMs?: number; +}; + export async function downloadCommentImages( octokits: Octokits, owner: string, repo: string, comments: CommentWithImages[], + options: ImageDownloadOptions = {}, ): Promise> { const urlToPathMap = new Map(); + const timeoutMs = options.timeoutMs ?? DEFAULT_IMAGE_DOWNLOAD_TIMEOUT_MS; const downloadsDir = "/tmp/github-images"; await fs.mkdir(downloadsDir, { recursive: true }); @@ -241,15 +249,7 @@ export async function downloadCommentImages( try { console.log(`Downloading ${originalUrl}...`); - const imageResponse = await fetch(signedUrl); - if (!imageResponse.ok) { - throw new Error( - `HTTP ${imageResponse.status}: ${imageResponse.statusText}`, - ); - } - - const arrayBuffer = await imageResponse.arrayBuffer(); - const buffer = Buffer.from(arrayBuffer); + const buffer = await fetchImage(signedUrl, timeoutMs); // GitHub user-attachment URLs (/user-attachments/assets/) carry // no file extension, so the URL-based guess silently falls back to @@ -289,6 +289,37 @@ export async function downloadCommentImages( return urlToPathMap; } +async function fetchImage(url: string, timeoutMs: number): Promise { + const controller = new AbortController(); + let timeoutHandle: ReturnType | undefined; + const timeoutPromise = new Promise((_, reject) => { + timeoutHandle = setTimeout(() => { + controller.abort(); + reject(new Error(`Image download timed out after ${timeoutMs}ms`)); + }, timeoutMs); + }); + + try { + const response = await Promise.race([ + fetch(url, { signal: controller.signal }), + timeoutPromise, + ]); + if (!response.ok) { + throw new Error(`HTTP ${response.status}: ${response.statusText}`); + } + + const arrayBuffer = await Promise.race([ + response.arrayBuffer(), + timeoutPromise, + ]); + return Buffer.from(arrayBuffer); + } finally { + if (timeoutHandle !== undefined) { + clearTimeout(timeoutHandle); + } + } +} + function getImageExtension(url: string): string { const urlParts = url.split("/"); const filename = urlParts[urlParts.length - 1]; diff --git a/test/image-downloader.test.ts b/test/image-downloader.test.ts index cc247679..12d4dea7 100644 --- a/test/image-downloader.test.ts +++ b/test/image-downloader.test.ts @@ -148,7 +148,9 @@ describe("downloadCommentImages", () => { mediaType: { format: "full+json" }, }); - expect(fetchSpy).toHaveBeenCalledWith(signedUrl); + expect(fetchSpy).toHaveBeenCalledWith(signedUrl, { + signal: expect.any(AbortSignal), + }); expect(fsWriteFileSpy).toHaveBeenCalledWith( "/tmp/github-images/image-1704067200000-0.png", Buffer.from(mockArrayBuffer), @@ -481,8 +483,12 @@ describe("downloadCommentImages", () => { ); expect(fetchSpy).toHaveBeenCalledTimes(2); - expect(fetchSpy).toHaveBeenNthCalledWith(1, signedUrl1); - expect(fetchSpy).toHaveBeenNthCalledWith(2, signedUrl2); + expect(fetchSpy).toHaveBeenNthCalledWith(1, signedUrl1, { + signal: expect.any(AbortSignal), + }); + expect(fetchSpy).toHaveBeenNthCalledWith(2, signedUrl2, { + signal: expect.any(AbortSignal), + }); expect(result.get(imageUrl1)).toBe( "/tmp/github-images/image-1704067200000-0.png", ); @@ -523,7 +529,9 @@ describe("downloadCommentImages", () => { comments, ); - expect(fetchSpy).toHaveBeenCalledWith(signedUrl); + expect(fetchSpy).toHaveBeenCalledWith(signedUrl, { + signal: expect.any(AbortSignal), + }); expect(result.get(imageUrl)).toBe( "/tmp/github-images/image-1704067200000-0.png", ); @@ -766,6 +774,95 @@ describe("downloadCommentImages", () => { ); }); + test("should skip an image when the fetch times out", async () => { + const mockOctokit = createMockOctokit(); + const imageUrl = assetUrl(GUID_1); + const signedUrl = signedUrlFor(GUID_1, ".png"); + let signal: AbortSignal | null | undefined; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.getComment = jest.fn().mockResolvedValue({ + data: { + body_html: ``, + }, + }); + + fetchSpy = spyOn(global, "fetch"); + fetchSpy.mockImplementation((_input: unknown, init?: RequestInit) => { + signal = init?.signal; + return new Promise(() => {}); + }); + + const result = await downloadCommentImages( + mockOctokit, + "owner", + "repo", + [ + { + type: "issue_comment", + id: "445", + body: `Stalled image: ![stalled](${imageUrl})`, + }, + ], + { timeoutMs: 5 }, + ); + + expect(result.size).toBe(0); + expect(signal?.aborted).toBe(true); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to download"), + expect.objectContaining({ + message: "Image download timed out after 5ms", + }), + ); + }); + + test("should time out while reading a response body", async () => { + const mockOctokit = createMockOctokit(); + const imageUrl = assetUrl(GUID_1); + const signedUrl = signedUrlFor(GUID_1, ".png"); + let signal: AbortSignal | null | undefined; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.getComment = jest.fn().mockResolvedValue({ + data: { + body_html: ``, + }, + }); + + fetchSpy = spyOn(global, "fetch"); + fetchSpy.mockImplementation((_input: unknown, init?: RequestInit) => { + signal = init?.signal; + return Promise.resolve({ + ok: true, + arrayBuffer: () => new Promise(() => {}), + } as Response); + }); + + const result = await downloadCommentImages( + mockOctokit, + "owner", + "repo", + [ + { + type: "issue_comment", + id: "446", + body: `Stalled body: ![stalled](${imageUrl})`, + }, + ], + { timeoutMs: 5 }, + ); + + expect(result.size).toBe(0); + expect(signal?.aborted).toBe(true); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to download"), + expect.objectContaining({ + message: "Image download timed out after 5ms", + }), + ); + }); + test("should handle API errors gracefully", async () => { const mockOctokit = createMockOctokit(); const imageUrl = assetUrl(GUID_1); @@ -936,7 +1033,9 @@ describe("downloadCommentImages", () => { mediaType: { format: "full+json" }, }); - expect(fetchSpy).toHaveBeenCalledWith(signedUrl); + expect(fetchSpy).toHaveBeenCalledWith(signedUrl, { + signal: expect.any(AbortSignal), + }); expect(fsWriteFileSpy).toHaveBeenCalledWith( "/tmp/github-images/image-1704067200000-0.png", Buffer.from(mockArrayBuffer),