Skip to content

Commit 4fdca5f

Browse files
linyowslinyows
authored
Merge pull request #214 from linyows/fix/saveimage-hang-and-icon-bugs
Fix saveImage hang deadlock and harden iconRegexps + webp handling
2 parents 899ccbc + b7342b2 commit 4fdca5f

6 files changed

Lines changed: 332 additions & 31 deletions

File tree

‎package-lock.json‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "rotion",
3-
"version": "3.4.1",
3+
"version": "3.4.2",
44
"license": "MIT",
55
"repository": "linyows/rotion",
66
"description": "This is react components that uses the notion API to display the notion's database and page.",

‎src/exporter/files.test.ts‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import fs from 'fs/promises'
2+
import http from 'http'
23
import { test } from 'uvu'
34
import * as td from 'testdouble'
45
import * as assert from 'uvu/assert'
@@ -362,6 +363,28 @@ const testsIconRegex = [
362363
'<link rel="shortcut icon" href="https://example.com/favicon.ico">',
363364
'https://example.com/favicon.ico',
364365
],
366+
// Unquoted href followed by additional attributes — the regex must
367+
// capture only the URL, not bleed across `>` or whitespace.
368+
[
369+
'<link rel="icon" href=/foo.png class="x">',
370+
'/foo.png',
371+
],
372+
[
373+
'<link rel="icon" type="image/x-icon" href=/bar.ico />',
374+
'/bar.ico',
375+
],
376+
// The leak we observed in cognano builds: nature.com served a link tag
377+
// whose captured href ended with `>` and the saved file became
378+
// `something.png>`, which then crashed sharp with "unsupported image
379+
// format". The fix tightens `[^"]+` URL captures to `[^"\s>]+`.
380+
[
381+
'<link rel="icon" type="image/png" sizes="48x48" href=/static/favicon-48x48.png>',
382+
'/static/favicon-48x48.png',
383+
],
384+
[
385+
'<link rel="icon" href=/foo.svg/>',
386+
'/foo.svg',
387+
],
365388
]
366389
for (const t of testsIconRegex) {
367390
const [tag, path] = t
@@ -490,4 +513,136 @@ test('saveFile writeStream finish is awaited (file is complete on return)', asyn
490513
assert.equal(stats.size, result.size, 'reported size should match actual file size')
491514
})
492515

516+
// --- Stream hang regression tests ---
517+
//
518+
// Reproduce the deadlock observed in cognano builds: a remote server returns
519+
// response headers and a few bytes, then stops sending data. The current
520+
// `await res.end` in saveImage/saveFile hangs forever in this case, because
521+
// `req.setTimeout` aborts the request without emitting the 'end' event that
522+
// the awaited promise listens for. The lock around saveImage is therefore
523+
// never released and downstream workers deadlock.
524+
//
525+
// After the fix, saveImage/saveFile must reject within roughly the configured
526+
// timeout window (default 1500ms) instead of hanging.
527+
528+
async function startStallingServer (): Promise<{ port: number, close: () => Promise<void> }> {
529+
const sockets: import('net').Socket[] = []
530+
const server = http.createServer((_req, res) => {
531+
// Send headers and a few bytes, then deliberately never end.
532+
// No Content-Length, so the client cannot know the body is complete.
533+
res.writeHead(200, { 'Content-Type': 'image/png' })
534+
res.write(Buffer.alloc(64, 0))
535+
})
536+
server.on('connection', (socket) => { sockets.push(socket) })
537+
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve))
538+
const addr = server.address()
539+
if (!addr || typeof addr === 'string') throw new Error('failed to bind test server')
540+
return {
541+
port: addr.port,
542+
close: async () => {
543+
for (const s of sockets) s.destroy()
544+
await new Promise<void>(resolve => server.close(() => resolve()))
545+
},
546+
}
547+
}
548+
549+
test('saveImage rejects (does not hang) when remote stalls mid-response', async () => {
550+
const { port, close } = await startStallingServer()
551+
// Unique URL per run so the existsSync fast-path in saveImage doesn't
552+
// bypass the download (which is what we are exercising).
553+
const url = `http://127.0.0.1:${port}/stall-img-${Date.now()}-${Math.random().toString(36).slice(2)}.png`
554+
try {
555+
const start = Date.now()
556+
const TEST_BUDGET = 8000 // generous: 1500ms ROTION_TIMEOUT default + slack
557+
let outcome: 'resolved' | 'rejected' | 'hang' = 'hang'
558+
559+
await Promise.race([
560+
files.saveImage(url, 'stall-test-img')
561+
.then(() => { outcome = 'resolved' })
562+
.catch(() => { outcome = 'rejected' }),
563+
new Promise<void>(resolve => setTimeout(resolve, TEST_BUDGET)),
564+
])
565+
566+
const elapsed = Date.now() - start
567+
assert.equal(
568+
outcome,
569+
'rejected',
570+
`saveImage should reject when remote stalls (elapsed=${elapsed}ms, outcome=${outcome})`,
571+
)
572+
assert.ok(
573+
elapsed < TEST_BUDGET,
574+
`saveImage should fail fast, took ${elapsed}ms (>= ${TEST_BUDGET}ms budget)`,
575+
)
576+
} finally {
577+
await close()
578+
}
579+
})
580+
581+
test('saveFile rejects (does not hang) when remote stalls mid-response', async () => {
582+
const { port, close } = await startStallingServer()
583+
const url = `http://127.0.0.1:${port}/stall-file-${Date.now()}-${Math.random().toString(36).slice(2)}.bin`
584+
try {
585+
const start = Date.now()
586+
const TEST_BUDGET = 8000
587+
let outcome: 'resolved' | 'rejected' | 'hang' = 'hang'
588+
589+
await Promise.race([
590+
files.saveFile(url, 'stall-test-file')
591+
.then(() => { outcome = 'resolved' })
592+
.catch(() => { outcome = 'rejected' }),
593+
new Promise<void>(resolve => setTimeout(resolve, TEST_BUDGET)),
594+
])
595+
596+
const elapsed = Date.now() - start
597+
assert.equal(
598+
outcome,
599+
'rejected',
600+
`saveFile should reject when remote stalls (elapsed=${elapsed}ms, outcome=${outcome})`,
601+
)
602+
assert.ok(
603+
elapsed < TEST_BUDGET,
604+
`saveFile should fail fast, took ${elapsed}ms (>= ${TEST_BUDGET}ms budget)`,
605+
)
606+
} finally {
607+
await close()
608+
}
609+
})
610+
611+
// --- saveImage webp same-file regression test ---
612+
//
613+
// When the source URL ends in `.webp`, replaceExt(urlPath, '.webp') yields
614+
// the same path as filePath, so the conversion step
615+
// `sharp(filePath).webp().toFile(webpPath)` fails with
616+
// "Cannot use same file for input and output" and the function returns
617+
// without populated width/height. The fix short-circuits the re-encode
618+
// when the downloaded file is already at the target webp path.
619+
620+
test('saveImage downloads .webp URL without "Cannot use same file" error', async () => {
621+
const sharp = (await import('sharp')).default
622+
const webpBuffer = await sharp({
623+
create: { width: 12, height: 8, channels: 3, background: { r: 12, g: 34, b: 56 } },
624+
}).webp().toBuffer()
625+
626+
const server = http.createServer((_req, res) => {
627+
res.writeHead(200, { 'Content-Type': 'image/webp', 'Content-Length': String(webpBuffer.length) })
628+
res.end(webpBuffer)
629+
})
630+
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve))
631+
const addr = server.address()
632+
if (!addr || typeof addr === 'string') throw new Error('failed to bind test server')
633+
const port = addr.port
634+
const url = `http://127.0.0.1:${port}/test-webp-${Date.now()}-${Math.random().toString(36).slice(2)}.webp`
635+
636+
try {
637+
const ipws = await files.saveImage(url, 'webp-saveimage-test')
638+
assert.match(ipws.path, /^\/images\/.*\.webp$/, `expected .webp path, got: ${ipws.path}`)
639+
// The fix should still report metadata (width/height) instead of bailing
640+
// out after the sharp error.
641+
assert.equal(ipws.width, 12, `width should match source, got: ${ipws.width}`)
642+
assert.equal(ipws.height, 8, `height should match source, got: ${ipws.height}`)
643+
} finally {
644+
await new Promise<void>(resolve => server.close(() => resolve()))
645+
}
646+
})
647+
493648
test.run()

‎src/exporter/files.ts‎

Lines changed: 57 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import fs from 'fs'
22
import { mkdir, stat, unlink } from 'node:fs/promises'
3+
import { pipeline } from 'node:stream/promises'
34
import https from 'https'
45
import http from 'http'
6+
import type { IncomingMessage } from 'node:http'
57
import path from 'path'
68
import crypto from 'crypto'
79
import { promisify } from 'util'
@@ -61,11 +63,14 @@ http.get[promisify.custom] = function getAsync (url: any) {
6163
})
6264
}
6365

64-
interface HttpGetResponse {
65-
pipe: Function
66+
// Extends IncomingMessage so it is a proper Readable stream (and works
67+
// directly with `stream.pipeline` without a cast). Overrides `end` with the
68+
// Promise we attach in the custom promisify above (resolved when the
69+
// response emits 'end'), and tightens `statusCode` to non-optional since
70+
// the response is only constructed after headers arrive.
71+
interface HttpGetResponse extends Omit<IncomingMessage, 'end' | 'statusCode'> {
6672
end: Promise<unknown>
6773
statusCode: number
68-
rawHeaders: string[]
6974
}
7075

7176
// https://oembed.com/
@@ -317,14 +322,17 @@ export async function saveFile (fileUrl: string, prefix: string) {
317322
throw new Error(`retry download to ${urlWithoutQuerystring} but failed`)
318323
}
319324
}
325+
// Use stream.pipeline so that source-side errors (request abort,
326+
// socket close, network stall after `req.setTimeout` fires) reject
327+
// and clean up the write stream instead of leaving the
328+
// `writeStream.on('finish')` promise dangling forever.
320329
const writeStream = fs.createWriteStream(filePath)
321-
res.pipe(writeStream)
322-
await res.end
323-
await new Promise<void>((resolve, reject) => {
324-
writeStream.on('finish', resolve)
325-
writeStream.on('error', reject)
326-
})
330+
await pipeline(res, writeStream)
327331
} catch (e) {
332+
// Best-effort cleanup of any partial bytes pipeline managed to write
333+
// before failing — otherwise the next call hits the existsSync()
334+
// fast-path and serves a corrupted cached file.
335+
try { await unlink(filePath) } catch {}
328336
const errorMessage = `saveFile download error -- path: ${filePath}, url: ${fileUrl}, message: ${e}`
329337
if (debug) {
330338
console.log(errorMessage)
@@ -404,14 +412,17 @@ export const saveImage = async (imageUrl: string, prefix: string): Promise<Image
404412
throw new Error(`retry download to ${urlWithoutQuerystring} but failed`)
405413
}
406414
}
415+
// Use stream.pipeline so that source-side errors (request abort,
416+
// socket close, network stall after `req.setTimeout` fires) reject
417+
// and clean up the write stream instead of leaving the
418+
// `writeStream.on('finish')` promise dangling forever.
407419
const writeStream = fs.createWriteStream(filePath)
408-
res.pipe(writeStream)
409-
await res.end
410-
await new Promise<void>((resolve, reject) => {
411-
writeStream.on('finish', resolve)
412-
writeStream.on('error', reject)
413-
})
420+
await pipeline(res, writeStream)
414421
} catch (e) {
422+
// Best-effort cleanup of any partial bytes pipeline managed to write
423+
// before failing — otherwise the next call hits the existsSync()
424+
// fast-path and serves a corrupted cached file.
425+
try { await unlink(filePath) } catch {}
415426
const errorMessage = `saveImage download error -- path: ${filePath}, url: ${imageUrl}, message: ${e}`
416427
if (debug) {
417428
console.log(errorMessage)
@@ -424,6 +435,21 @@ export const saveImage = async (imageUrl: string, prefix: string): Promise<Image
424435
return { path: urlPath }
425436
}
426437

438+
// Source is already webp — re-encoding it would point sharp at the same
439+
// path for input and output ("Cannot use same file for input and output")
440+
// and discard width/height. Just measure metadata and return.
441+
if (ext === '.webp') {
442+
try {
443+
const meta = await sharp(filePath).metadata()
444+
return { path: urlPath, width: meta.width, height: meta.height }
445+
} catch (e) {
446+
if (debug) {
447+
console.log(`sharp.metadata() error -- path: ${urlPath}, message: ${e}`)
448+
}
449+
return { path: urlPath }
450+
}
451+
}
452+
427453
/* Convert HEIC/HEIF to PNG first if needed */
428454
let processFilePath = filePath
429455
let processUrlPath = urlPath
@@ -589,17 +615,23 @@ export const imageRegexps = [
589615
/name="twitter:image"\s+content="([^"]+)"/,
590616
]
591617

618+
// URL captures use `[^"\s>]+?` (lazy) followed by a lookahead requiring
619+
// `"`, whitespace, `>`, or `/>` so that an unquoted href like
620+
// `href=/foo.svg/>` captures `/foo.svg` and not `/foo.svg/`. Without this
621+
// the trailing `/` of a self-closing tag bleeds into the saved filename
622+
// and breaks downstream sharp processing. `[^"]+` is preserved for
623+
// non-URL captures (e.g. `sizes="..."`).
592624
export const iconRegexps = [
593-
/<link\s+href="?([^"]+)"?\s+rel="icon"/,
594-
/<link\s+rel="icon"\s+href="?([^"]+)"?\s*?\/?>/,
595-
/<link\s+rel="icon".*?href="?([^"]+)"?/,
596-
/<link\s+rel="shortcut icon"\s+type="image\/x-icon"\s+href="?([^"]+)"?\s?\/?>/,
597-
/<link\s+rel="shortcut icon"\s+href="?([^"\s>]+)"?\s?\/?>/,
598-
/<link\s+href="?([^"]+)"?\s+rel="(shortcut icon|icon shortcut)"(\s+type="image\/x-icon")?\s?\/?>/,
599-
/<link\s+href="?([^"]+)"?\s+rel="icon"\s+sizes="[^"]+"\s+type="image\/[^"]"\s*\/?>/,
600-
/type="image\/x-icon"\s+href="?([^"]+)"?/,
601-
/rel="icon"\s+href="?([^"]+)"?/,
602-
/rel="shortcut icon"\s+href="?([^"\s>]+)"?/,
625+
/<link\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s+rel="icon"/,
626+
/<link\s+rel="icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s*?\/?>/,
627+
/<link\s+rel="icon".*?href="?([^"\s>]+?(?=["\s>]|\/>))"?/,
628+
/<link\s+rel="shortcut icon"\s+type="image\/x-icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s?\/?>/,
629+
/<link\s+rel="shortcut icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s?\/?>/,
630+
/<link\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s+rel="(shortcut icon|icon shortcut)"(\s+type="image\/x-icon")?\s?\/?>/,
631+
/<link\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?\s+rel="icon"\s+sizes="[^"]+"\s+type="image\/[^"]"\s*\/?>/,
632+
/type="image\/x-icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?/,
633+
/rel="icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?/,
634+
/rel="shortcut icon"\s+href="?([^"\s>]+?(?=["\s>]|\/>))"?/,
603635
]
604636

605637
export const findImage = (html: string): string | null => {

0 commit comments

Comments
 (0)