Skip to content

Commit 78e5035

Browse files
committed
fix(service-print): patch round 3 — bounded cleanup, no raw console URL, the revoke log pinned through the plugin
Claude-Session: https://claude.ai/code/session_01W2XFCTBfK4LabgqovmvSgK Co-authored-by: Claude <noreply@anthropic.com>
1 parent f653bf5 commit 78e5035

7 files changed

Lines changed: 341 additions & 38 deletions

File tree

‎packages/services/service-print/src/chromium-driver.ts‎

Lines changed: 113 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,28 @@ import type { PrintSessionCookie } from './principal-session.js';
99
* `playwright-core` (pinned exact on this package only), in either placement
1010
* {@link ResolvedPrintPlacement} names.
1111
*
12-
* - **local** — `chromium.launch({ executablePath })`, one browser per render,
13-
* closed in a `finally`.
12+
* - **local** — `chromium.launchServer({ executablePath })` and a connection to
13+
* it, one browser process per render. Disposed by closing the connection
14+
* and the server; a process whose close does not settle within
15+
* {@link CLOSE_GRACE_MS} is killed.
1416
* - **remote** — `chromium.connectOverCDP(endpoint)`, one connection per
15-
* render. Closing a connected browser disconnects from it and leaves the
16-
* remote browser running; the context this render opened is closed first,
17-
* so its cookies leave the remote browser with it.
17+
* render. The context this render opened is closed first, so its cookies
18+
* leave the remote browser with it; then the connection is closed, which
19+
* leaves the remote browser running. Each close waits at most
20+
* {@link CLOSE_GRACE_MS}: playwright opens a CDP context with
21+
* `disposeOnDetach`, so dropping the connection disposes the context even
22+
* when the browser no longer answers its close.
23+
*
24+
* ## The budget, and what it bounds
25+
*
26+
* Every step of a render — open, the context, the cookie, the page, the load,
27+
* the ready wait, the fonts and the PDF — runs inside ONE deadline (the
28+
* caller's `timeoutMs`): a browser that stalls on any of them answers
29+
* `TIMEOUT` / `504` when the deadline passes. The cleanup that follows is
30+
* bounded too, by {@link CLOSE_GRACE_MS} per close, so the call returns within
31+
* the budget plus the grace periods even when the browser answers nothing
32+
* more. A browser that arrives after the deadline (an open that settled
33+
* late) is disposed by the abandoned render itself, never kept.
1834
*
1935
* Both placements run the SAME render code after the browser is open — the
2036
* remote placement is not a second driver, only a second way to reach the one
@@ -79,11 +95,40 @@ interface PageHandle {
7995
evaluate(fn: string): Promise<unknown>;
8096
pdf(options: { preferCSSPageSize: boolean; printBackground: boolean }): Promise<Buffer>;
8197
}
98+
/** The subset of playwright-core's browser server this driver uses (local placement). */
99+
interface ServerHandle {
100+
wsEndpoint(): string;
101+
close(): Promise<void>;
102+
kill(): Promise<void>;
103+
}
82104
interface ChromiumLauncher {
83-
launch(options: { executablePath: string; headless: boolean; timeout: number }): Promise<BrowserHandle>;
105+
launchServer(options: { executablePath: string; headless: boolean; timeout: number }): Promise<ServerHandle>;
106+
connect(wsEndpoint: string, options: { timeout: number }): Promise<BrowserHandle>;
84107
connectOverCDP(endpoint: string, options: { timeout: number }): Promise<BrowserHandle>;
85108
}
86109

110+
/** How long each close in a cleanup may take before the driver stops waiting for it. */
111+
export const CLOSE_GRACE_MS = 2_000;
112+
113+
/** Whether `promise` settles within `ms`; it is never awaited past that. */
114+
async function settlesWithin(promise: Promise<unknown>, ms: number): Promise<boolean> {
115+
let timer: ReturnType<typeof setTimeout> | undefined;
116+
const late = new Promise<false>((resolve) => {
117+
timer = setTimeout(() => resolve(false), ms);
118+
});
119+
try {
120+
return await Promise.race([promise.then(() => true, () => true), late]);
121+
} finally {
122+
clearTimeout(timer);
123+
}
124+
}
125+
126+
/** An open browser, and the bounded way to let go of it. */
127+
interface OpenBrowser {
128+
browser: BrowserHandle;
129+
dispose(): Promise<void>;
130+
}
131+
87132
/**
88133
* The ready predicate, evaluated in the page. A string, not a function, so the
89134
* page sees exactly this text and nothing from this module's scope.
@@ -153,6 +198,7 @@ class PlaywrightChromiumDriver implements ChromiumDriver {
153198
constructor(
154199
private readonly resolved: ResolvedPrintPlacement,
155200
private readonly launcher: () => Promise<ChromiumLauncher>,
201+
private readonly closeGraceMs: number,
156202
) {
157203
this.placement = resolved.placement;
158204
}
@@ -164,49 +210,86 @@ class PlaywrightChromiumDriver implements ChromiumDriver {
164210
: `the remote endpoint ${describeEndpoint(this.resolved.target)}`;
165211
}
166212

167-
private async open(timeoutMs: number): Promise<BrowserHandle> {
213+
private async open(timeoutMs: number): Promise<OpenBrowser> {
168214
const chromium = await this.launcher();
169-
return this.placement === 'local'
170-
? chromium.launch({ executablePath: this.resolved.target, headless: true, timeout: timeoutMs })
171-
: chromium.connectOverCDP(this.resolved.target, { timeout: timeoutMs });
215+
const grace = this.closeGraceMs;
216+
if (this.placement === 'remote') {
217+
const browser = await chromium.connectOverCDP(this.resolved.target, { timeout: timeoutMs });
218+
return { browser, dispose: async () => void (await settlesWithin(browser.close(), grace)) };
219+
}
220+
const server = await chromium.launchServer({ executablePath: this.resolved.target, headless: true, timeout: timeoutMs });
221+
let browser: BrowserHandle;
222+
try {
223+
browser = await chromium.connect(server.wsEndpoint(), { timeout: timeoutMs });
224+
} catch (err) {
225+
if (!(await settlesWithin(server.close(), grace))) await settlesWithin(server.kill(), grace);
226+
throw err;
227+
}
228+
return {
229+
browser,
230+
dispose: async () => {
231+
await settlesWithin(browser.close(), grace);
232+
// The process this render launched: closed, or killed when the close
233+
// does not settle.
234+
if (!(await settlesWithin(server.close(), grace))) await settlesWithin(server.kill(), grace);
235+
},
236+
};
172237
}
173238

174239
async probe(timeoutMs: number): Promise<{ version: string }> {
175-
let browser: BrowserHandle;
240+
let opened: OpenBrowser;
176241
try {
177-
browser = await this.open(timeoutMs);
242+
opened = await this.open(timeoutMs);
178243
} catch (err) {
179244
throw new Error(
180245
`Chromium could not be reached at ${this.where()}: ${redact(String((err as Error)?.message ?? err), [this.resolved.target])}`,
181246
);
182247
}
183248
try {
184-
return { version: browser.version() };
249+
return { version: opened.browser.version() };
185250
} finally {
186-
await browser.close().catch(() => undefined);
251+
await opened.dispose();
187252
}
188253
}
189254

190255
async renderPdf(input: ChromiumRenderInput): Promise<Buffer> {
191256
const secrets = [this.resolved.target, input.cookie.value];
192257
const started = Date.now();
193258
const remaining = (): number => Math.max(1, input.timeoutMs - (Date.now() - started));
194-
const handles: { browser?: BrowserHandle; context?: ContextHandle } = {};
195-
// Every step of the render — open, the context, the cookie, the page, the
196-
// load, the ready wait, the fonts and the PDF — runs inside ONE deadline:
197-
// a browser that stalls on any of them answers TIMEOUT within the budget.
259+
const grace = this.closeGraceMs;
260+
const held: { opened?: OpenBrowser; context?: ContextHandle } = {};
261+
let abandoned = false;
262+
/** Let go of everything this render holds, each close bounded. */
263+
const release = async (): Promise<void> => {
264+
const { opened, context } = held;
265+
held.opened = undefined;
266+
held.context = undefined;
267+
// The context first: on the remote placement it holds the session cookie
268+
// inside a browser this process does not own.
269+
if (context) await settlesWithin(context.close(), grace);
270+
if (opened) await opened.dispose();
271+
};
272+
/** After a step settles: an abandoned render releases what it just got, and stops. */
273+
const stillWanted = async (): Promise<void> => {
274+
if (abandoned) {
275+
await release();
276+
throw Object.assign(new Error('the render was abandoned at its deadline'), { name: 'TimeoutError' });
277+
}
278+
};
198279
const render = (async (): Promise<Buffer> => {
199-
handles.browser = await this.open(remaining());
200-
handles.context = await handles.browser.newContext();
201-
await handles.context.addCookies([cookieFor(input.url, input.cookie)]);
202-
const page = await handles.context.newPage();
280+
held.opened = await this.open(remaining());
281+
await stillWanted();
282+
held.context = await held.opened.browser.newContext();
283+
await stillWanted();
284+
await held.context.addCookies([cookieFor(input.url, input.cookie)]);
285+
const page = await held.context.newPage();
203286
await page.goto(input.url, { waitUntil: 'load', timeout: remaining() });
204287
await page.waitForFunction(PRINT_READY_PREDICATE, undefined, { timeout: remaining(), polling: POLL_INTERVAL_MS });
205288
await page.evaluate(FONTS_READY);
206289
return page.pdf({ preferCSSPageSize: true, printBackground: true });
207290
})();
208291
// Once the deadline answers, a late rejection of the abandoned render is
209-
// expected (the cleanup below closes what it was waiting on).
292+
// expected: it is the abandoned render releasing what it opened late.
210293
render.catch(() => undefined);
211294
let timer: ReturnType<typeof setTimeout> | undefined;
212295
const deadline = new Promise<never>((_, reject) => {
@@ -235,23 +318,22 @@ class PlaywrightChromiumDriver implements ChromiumDriver {
235318
);
236319
} finally {
237320
clearTimeout(timer);
238-
// The context first: on the remote placement it holds the session cookie
239-
// inside a browser this process does not own.
240-
if (handles.context) await handles.context.close().catch(() => undefined);
241-
if (handles.browser) await handles.browser.close().catch(() => undefined);
321+
abandoned = true;
322+
await release();
242323
}
243324
}
244325
}
245326

246327
/**
247-
* The driver for a resolved placement. `launcher` exists for tests that
248-
* observe the calls; a deployment never passes it.
328+
* The driver for a resolved placement. `launcher` and `options` exist for
329+
* tests that stage a browser; a deployment passes neither.
249330
*/
250331
export function createChromiumDriver(
251332
resolved: ResolvedPrintPlacement,
252333
launcher: () => Promise<ChromiumLauncher> = loadLauncher,
334+
options: { closeGraceMs?: number } = {},
253335
): ChromiumDriver {
254-
return new PlaywrightChromiumDriver(resolved, launcher);
336+
return new PlaywrightChromiumDriver(resolved, launcher, options.closeGraceMs ?? CLOSE_GRACE_MS);
255337
}
256338

257-
export type { ChromiumLauncher, BrowserHandle, ContextHandle, PageHandle };
339+
export type { ChromiumLauncher, BrowserHandle, ContextHandle, PageHandle, ServerHandle };

‎packages/services/service-print/src/chromium-driver.unit.test.ts‎

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,13 @@ interface Stage {
2929
newContext?: () => Promise<unknown>;
3030
pdf?: () => Promise<Buffer>;
3131
connect?: () => Promise<unknown>;
32+
contextClose?: () => Promise<void>;
33+
browserClose?: () => Promise<void>;
34+
serverClose?: () => Promise<void>;
3235
}
3336

3437
function stagedLauncher(stage: Stage) {
35-
const closed = { context: 0, browser: 0 };
38+
const closed = { context: 0, browser: 0, server: 0, killed: 0 };
3639
const page = {
3740
goto: vi.fn(stage.goto ?? (async () => undefined)),
3841
waitForFunction: vi.fn(async () => undefined),
@@ -44,20 +47,33 @@ function stagedLauncher(stage: Stage) {
4447
newPage: vi.fn(async () => page),
4548
close: vi.fn(async () => {
4649
closed.context++;
50+
if (stage.contextClose) await stage.contextClose();
4751
}),
4852
};
4953
const browser = {
5054
version: () => '141.0.0.0',
5155
newContext: vi.fn(stage.newContext ?? (async () => context)),
5256
close: vi.fn(async () => {
5357
closed.browser++;
58+
if (stage.browserClose) await stage.browserClose();
59+
}),
60+
};
61+
const server = {
62+
wsEndpoint: () => 'ws://127.0.0.1:40001/local-server',
63+
close: vi.fn(async () => {
64+
closed.server++;
65+
if (stage.serverClose) await stage.serverClose();
66+
}),
67+
kill: vi.fn(async () => {
68+
closed.killed++;
5469
}),
5570
};
5671
const launcher = {
57-
launch: vi.fn(async () => browser),
72+
launchServer: vi.fn(async () => server),
73+
connect: vi.fn(stage.connect ?? (async () => browser)),
5874
connectOverCDP: vi.fn(stage.connect ?? (async () => browser)),
5975
} as unknown as ChromiumLauncher;
60-
return { launcher: async () => launcher, closed, page };
76+
return { launcher: async () => launcher, closed, page, browser };
6177
}
6278

6379
async function refusalOf(p: Promise<unknown>): Promise<{ code?: unknown; status?: unknown; message: string }> {
@@ -82,7 +98,7 @@ describe('the driver redacts the session cookie from an engine error', () => {
8298
expect([r.code, r.status]).toEqual(['INTERNAL_ERROR', 500]);
8399
expect(r.message).toContain('[redacted]');
84100
expect(r.message).not.toContain(COOKIE.value);
85-
expect(closed).toEqual({ context: 1, browser: 1 });
101+
expect(closed).toMatchObject({ context: 1, browser: 1, server: 1, killed: 0 });
86102
});
87103

88104
it('a remote endpoint carrying a token is redacted from an engine error, and never named beyond scheme and host', async () => {
@@ -119,3 +135,59 @@ describe('one budget bounds the whole render', () => {
119135
});
120136
}
121137
});
138+
139+
describe('the cleanup after the deadline is bounded, and nothing opened late is kept', () => {
140+
const GRACE = 100;
141+
142+
it('remote: a context and a connection whose close never settles — the call still answers TIMEOUT within budget plus grace', async () => {
143+
const { launcher, closed } = stagedLauncher({
144+
pdf: () => never<Buffer>(),
145+
contextClose: () => never(),
146+
browserClose: () => never(),
147+
});
148+
const driver = createChromiumDriver({ placement: 'remote', target: 'ws://127.0.0.1:9222/x', consoleUrl: 'x' }, launcher, {
149+
closeGraceMs: GRACE,
150+
});
151+
const started = Date.now();
152+
const r = await refusalOf(driver.renderPdf({ url: URL_, cookie: COOKIE, timeoutMs: 300 }));
153+
const elapsed = Date.now() - started;
154+
expect([r.code, r.status]).toEqual(['TIMEOUT', 504]);
155+
expect(elapsed).toBeLessThan(300 + 2 * GRACE + 500);
156+
expect(closed).toMatchObject({ context: 1, browser: 1 });
157+
});
158+
159+
it('local: a browser process whose close never settles is killed, and the call still answers within budget plus grace', async () => {
160+
const { launcher, closed } = stagedLauncher({
161+
pdf: () => never<Buffer>(),
162+
browserClose: () => never(),
163+
serverClose: () => never(),
164+
});
165+
const driver = createChromiumDriver({ placement: 'local', target: '/usr/bin/chromium', consoleUrl: 'x' }, launcher, {
166+
closeGraceMs: GRACE,
167+
});
168+
const started = Date.now();
169+
const r = await refusalOf(driver.renderPdf({ url: URL_, cookie: COOKIE, timeoutMs: 300 }));
170+
const elapsed = Date.now() - started;
171+
expect([r.code, r.status]).toEqual(['TIMEOUT', 504]);
172+
expect(elapsed).toBeLessThan(300 + 4 * GRACE + 500);
173+
expect(closed).toMatchObject({ server: 1, killed: 1 });
174+
});
175+
176+
it('an open that settles after the deadline is closed by the abandoned render, and never given a context', async () => {
177+
let arrive!: () => void;
178+
const arrived = new Promise<void>((r) => (arrive = r));
179+
const ref: { browser?: unknown } = {};
180+
const { launcher, closed, browser } = stagedLauncher({ connect: () => arrived.then(() => ref.browser) });
181+
ref.browser = browser;
182+
const driver = createChromiumDriver({ placement: 'remote', target: 'ws://127.0.0.1:9222/x', consoleUrl: 'x' }, launcher, {
183+
closeGraceMs: GRACE,
184+
});
185+
const r = await refusalOf(driver.renderPdf({ url: URL_, cookie: COOKIE, timeoutMs: 200 }));
186+
expect([r.code, r.status]).toEqual(['TIMEOUT', 504]);
187+
expect(closed.browser).toBe(0);
188+
arrive();
189+
await new Promise((r2) => setTimeout(r2, 50));
190+
expect(closed.browser).toBe(1);
191+
expect(browser.newContext).not.toHaveBeenCalled();
192+
});
193+
});

‎packages/services/service-print/src/config.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,22 @@ describe('resolvePrintPlacement — one engine, two placements', () => {
6767
}
6868
});
6969

70+
it('a console URL is never echoed: credentials with a non-numeric port, and a query, are described by scheme and host or as unparseable', () => {
71+
const unparseable = resolvePrintPlacement({ chromiumPath: '/c', consoleUrl: 'http://print:topsecret@127.0.0.1:abc/_console' }, {});
72+
expect(unparseable.state).toBe('misconfigured');
73+
if (unparseable.state === 'misconfigured') {
74+
expect(unparseable.reason).toContain('(a value that is not a URL)');
75+
expect(unparseable.reason).not.toContain('topsecret');
76+
expect(unparseable.reason).not.toContain('print:');
77+
}
78+
const query = resolvePrintPlacement({ chromiumPath: '/c', consoleUrl: 'http://127.0.0.1:3000/_console?token=topsecret' }, {});
79+
expect(query.state).toBe('misconfigured');
80+
if (query.state === 'misconfigured') {
81+
expect(query.reason).toContain('http://127.0.0.1:3000');
82+
expect(query.reason).not.toContain('topsecret');
83+
}
84+
});
85+
7086
it('a console URL carrying credentials is misconfigured, and the credentials are not echoed', () => {
7187
const r = resolvePrintPlacement({ chromiumPath: '/c', consoleUrl: 'http://print:hunter2-secret@127.0.0.1:3000/_console' }, {});
7288
expect(r.state).toBe('misconfigured');

0 commit comments

Comments
 (0)