Skip to content

Commit 1a519a2

Browse files
fix(capture): hand libvips' worker pool back after the renders
`sharp.concurrency(1)` is process-global and was set once, for the whole life of the process. The bound is right for the rasterize loop -- a native abort in libvips cannot be caught, so the renders must not overlap -- but its scope was every later sharp caller in the process, none of which asked for captioning, all of them pinned to one thread from then on. Now the host's value is read first and restored in a `finally` around the rasterize loop, so a skipped SVG cannot cost the process its threads either. The vision requests below are network work and gain nothing from a pinned pool. The mock had to grow the getter half of sharp's API -- `concurrency()` with no argument reports the current value -- since save-and-restore is untestable without it. Verified as a real guard: dropping only the restore fails both new cases. Raised by Rames Jusso in review of #3561 and concurred by Magi. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 4cdc0d9 commit 1a519a2

2 files changed

Lines changed: 90 additions & 41 deletions

File tree

packages/cli/src/capture/contentExtractor.test.ts

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ const { generateContentMock, clientOptions, sharpState } = vi.hoisted(() => ({
1919
inFlight: 0,
2020
maxInFlight: 0,
2121
concurrencyCalls: [] as number[],
22+
// Stands in for the host core count sharp reports before anything touches it, so a failure
23+
// to restore shows up as a wrong value rather than as a coincidental match with 1.
24+
HOST_CONCURRENCY: 8,
25+
concurrency: 8,
2226
renders: [] as string[],
2327
},
2428
}));
@@ -42,9 +46,15 @@ vi.mock("sharp", () => {
4246
return chain;
4347
};
4448
const sharp = Object.assign(pipeline, {
45-
concurrency: (value: number) => {
46-
sharpState.concurrencyCalls.push(value);
47-
return value;
49+
// Real `sharp.concurrency()` is a getter when called with no argument and a process-global
50+
// setter otherwise. The mock has to be both, or code that saves and restores the host value
51+
// cannot be tested at all.
52+
concurrency: (value?: number) => {
53+
if (typeof value === "number") {
54+
sharpState.concurrencyCalls.push(value);
55+
sharpState.concurrency = value;
56+
}
57+
return sharpState.concurrency;
4858
},
4959
});
5060
return { default: sharp };
@@ -623,6 +633,7 @@ describe("captionImagesWithGemini — SVG rasterization", () => {
623633
sharpState.inFlight = 0;
624634
sharpState.maxInFlight = 0;
625635
sharpState.concurrencyCalls.length = 0;
636+
sharpState.concurrency = sharpState.HOST_CONCURRENCY;
626637
sharpState.renders.length = 0;
627638
clientOptions.length = 0;
628639
});
@@ -678,9 +689,11 @@ describe("captionImagesWithGemini — SVG rasterization", () => {
678689
expect(Object.keys(captions)).toHaveLength(6);
679690
});
680691

681-
it("bounds libvips' own worker pool as well", async () => {
692+
it("bounds libvips' own worker pool for the renders, then hands it back", async () => {
682693
// Left at its default the pool sizes itself to the host's core count, so serializing the
683-
// loop alone still leaves one render fanning out across every core.
694+
// loop alone still leaves one render fanning out across every core. But `sharp.concurrency`
695+
// is process-global: leaving it at 1 pins every later sharp caller in this process, none of
696+
// which asked for captioning, so the bound has to be given back when the renders are done.
684697
const dir = makeProjectWithSvgs(1);
685698
dirs.push(dir);
686699
vi.stubEnv("OPENROUTER_API_KEY", "or-key");
@@ -697,7 +710,33 @@ describe("captionImagesWithGemini — SVG rasterization", () => {
697710

698711
await captionImagesWithGemini(dir, () => {}, []);
699712

700-
expect(sharpState.concurrencyCalls).toEqual([1]);
713+
expect(sharpState.concurrencyCalls).toEqual([1, sharpState.HOST_CONCURRENCY]);
714+
expect(sharpState.concurrency).toBe(sharpState.HOST_CONCURRENCY);
715+
});
716+
717+
it("hands the worker pool back even when a rasterize throws", async () => {
718+
// The restore is in a `finally`, because a skipped SVG must not cost the rest of the process
719+
// its threads.
720+
const dir = makeProjectWithSvgs(2);
721+
dirs.push(dir);
722+
vi.stubEnv("OPENROUTER_API_KEY", "or-key");
723+
vi.stubGlobal(
724+
"fetch",
725+
vi.fn(
726+
async () =>
727+
new Response(JSON.stringify({ choices: [{ message: { content: "A glyph." } }] }), {
728+
status: 200,
729+
headers: { "content-type": "application/json" },
730+
}),
731+
),
732+
);
733+
// Unreadable, which is how an exotic SVG behaves through sharp.
734+
rmSync(join(dir, "assets", "svgs", "logo-0.svg"));
735+
mkdirSync(join(dir, "assets", "svgs", "logo-0.svg"));
736+
737+
await captionImagesWithGemini(dir, () => {}, []);
738+
739+
expect(sharpState.concurrency).toBe(sharpState.HOST_CONCURRENCY);
701740
});
702741

703742
it("keeps captioning the rest when one SVG cannot be rasterized", async () => {

packages/cli/src/capture/contentExtractor.ts

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -509,10 +509,13 @@ export async function captionImagesWithGemini(
509509
reportOutcome();
510510
return geminiCaptions;
511511
}
512-
// Bound libvips' worker pool. Left at its default it sizes itself to the host's core
513-
// count, and several concurrent SVG renders then multiply that — the combination
514-
// corrupted the heap in production (see the serialized rasterize loop below).
515-
sharp.concurrency(1);
512+
// libvips' worker pool sizes itself to the host's core count, and several concurrent SVG
513+
// renders then multiply that — the combination corrupted the heap in production (see the
514+
// serialized rasterize loop below). `sharp.concurrency` is process-global and outlives this
515+
// function, so remember the host's value and bound the pool only around the renders that
516+
// need it: captioning must not leave every later sharp caller in this process — none of
517+
// which asked for captioning — pinned to a single thread for the rest of its life.
518+
const hostConcurrency = sharp.concurrency();
516519
progress("design", `Rasterizing + captioning ${svgFiles.length} SVGs via vision API...`);
517520
const SVG_BATCH = 20;
518521
const SVG_RENDER_SIZE = 256; // px — enough resolution for Gemini to read wordmarks, small enough to keep payload sub-MB
@@ -534,38 +537,45 @@ export async function captionImagesWithGemini(
534537
// the concurrency has to go rather than be handled. Rasterizing is local and cheap;
535538
// the vision request is the slow leg and stays parallel, so throughput barely moves.
536539
const rasterized: { relPath: string; pngBase64: string | null }[] = [];
537-
for (const { relPath } of batch) {
538-
const filePath = join(assetsDir, relPath);
539-
try {
540-
// Flatten against a contrasting background — white-on-white SVGs render invisible to Vision.
541-
const svgSource = readFileSync(filePath, "utf-8");
542-
const lightFillHits = (
543-
svgSource.match(/fill\s*=\s*["'](#fff(fff)?|white|#[ef][ef][ef]|#[ef]{6})["']/gi) ||
544-
[]
545-
).length;
546-
const darkFillHits = (
547-
svgSource.match(/fill\s*=\s*["'](#000(000)?|black|#[0-3]{6}|#[0-3]{3})["']/gi) || []
548-
).length;
549-
const bg =
550-
lightFillHits > darkFillHits
551-
? { r: 32, g: 32, b: 32 } // dark slate behind light glyphs
552-
: { r: 255, g: 255, b: 255 }; // white behind dark glyphs (default)
553-
const pngBuffer = await sharp(filePath)
554-
.resize({
555-
width: SVG_RENDER_SIZE,
556-
height: SVG_RENDER_SIZE,
557-
fit: "inside",
558-
withoutEnlargement: false,
559-
})
560-
.flatten({ background: bg })
561-
.png()
562-
.toBuffer();
563-
rasterized.push({ relPath, pngBase64: pngBuffer.toString("base64") });
564-
} catch {
565-
// exotic SVG features may break sharp; skip caption rather than block
566-
svgsSkipped++;
567-
rasterized.push({ relPath, pngBase64: null });
540+
sharp.concurrency(1);
541+
try {
542+
for (const { relPath } of batch) {
543+
const filePath = join(assetsDir, relPath);
544+
try {
545+
// Flatten against a contrasting background — white-on-white SVGs render invisible to Vision.
546+
const svgSource = readFileSync(filePath, "utf-8");
547+
const lightFillHits = (
548+
svgSource.match(/fill\s*=\s*["'](#fff(fff)?|white|#[ef][ef][ef]|#[ef]{6})["']/gi) ||
549+
[]
550+
).length;
551+
const darkFillHits = (
552+
svgSource.match(/fill\s*=\s*["'](#000(000)?|black|#[0-3]{6}|#[0-3]{3})["']/gi) || []
553+
).length;
554+
const bg =
555+
lightFillHits > darkFillHits
556+
? { r: 32, g: 32, b: 32 } // dark slate behind light glyphs
557+
: { r: 255, g: 255, b: 255 }; // white behind dark glyphs (default)
558+
const pngBuffer = await sharp(filePath)
559+
.resize({
560+
width: SVG_RENDER_SIZE,
561+
height: SVG_RENDER_SIZE,
562+
fit: "inside",
563+
withoutEnlargement: false,
564+
})
565+
.flatten({ background: bg })
566+
.png()
567+
.toBuffer();
568+
rasterized.push({ relPath, pngBase64: pngBuffer.toString("base64") });
569+
} catch {
570+
// exotic SVG features may break sharp; skip caption rather than block
571+
svgsSkipped++;
572+
rasterized.push({ relPath, pngBase64: null });
573+
}
568574
}
575+
} finally {
576+
// Hand the pool back even if a rasterize threw: the vision requests below are network
577+
// work that gains nothing from a pinned pool, and the process outlives this capture.
578+
sharp.concurrency(hostConcurrency);
569579
}
570580
const results = await Promise.allSettled(
571581
rasterized.map(async ({ relPath, pngBase64 }) => {

0 commit comments

Comments
 (0)