Skip to content

Commit f514b0d

Browse files
committed
fix: always run session end hooks on all shutdown paths
Ensure |onBeforeSessionEnd|, |onSessionEnd| and |onAfterSessionEnd| are always executed. - Route |SECURITY_VIOLATION| through endSession() - Trigger `onSessionEnd` on launch failures - Preserve shutdown reason across lifecycle
1 parent 8ccf2d4 commit f514b0d

File tree

4 files changed

+36
-15
lines changed

4 files changed

+36
-15
lines changed

api/src/services/cdp/cdp.service.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ import {
6969
SessionContextType,
7070
categorizeError,
7171
} from "./errors/launch-errors.js";
72-
import { BasePlugin } from "./plugins/core/base-plugin.js";
72+
import { BasePlugin, ShutdownReason } from "./plugins/core/base-plugin.js";
7373
import { PluginManager } from "./plugins/core/plugin-manager.js";
7474
import { isSimilarConfig, validateLaunchConfig, validateTimezone } from "./utils/validation.js";
7575
import { TargetInstrumentationManager } from "./instrumentation/target-manager.js";
@@ -376,7 +376,7 @@ export class CDPService extends EventEmitter {
376376
`[CDPService] Blocked response from file protocol: ${response.url()}`,
377377
);
378378
page.close().catch(() => {});
379-
this.shutdown();
379+
this.endSession(ShutdownReason.SECURITY_VIOLATION);
380380
}
381381
});
382382
}
@@ -436,7 +436,7 @@ export class CDPService extends EventEmitter {
436436
if (url.startsWith("file://")) {
437437
this.logger.error(`[CDPService] Blocked request to file protocol: ${url}`);
438438
page.close().catch(() => {});
439-
this.shutdown();
439+
this.endSession(ShutdownReason.SECURITY_VIOLATION);
440440
} else {
441441
await request.continue({ headers });
442442
}
@@ -456,16 +456,16 @@ export class CDPService extends EventEmitter {
456456
}
457457

458458
@traceable
459-
public async shutdown(): Promise<void> {
459+
public async shutdown(reason: ShutdownReason): Promise<void> {
460460
this.shuttingDown = true;
461-
this.logger.info(`[CDPService] Shutting down and cleaning up resources`);
461+
this.logger.info(`[CDPService] Shutting down and cleaning up resources (reason: ${reason})`);
462462

463463
try {
464464
if (this.browserInstance) {
465465
await this.pluginManager.onBrowserClose(this.browserInstance);
466466
}
467467

468-
await this.pluginManager.onShutdown();
468+
await this.pluginManager.onShutdown(reason);
469469

470470
this.removeAllHandlers();
471471
await this.browserInstance?.close();
@@ -527,7 +527,7 @@ export class CDPService extends EventEmitter {
527527
return await this.launchInternal(config);
528528
} catch (error) {
529529
try {
530-
await this.pluginManager.onShutdown();
530+
await this.pluginManager.onShutdown(ShutdownReason.LAUNCH_FAILURE);
531531
await this.shutdownHook();
532532
} catch (e) {
533533
this.logger.warn(
@@ -609,7 +609,7 @@ export class CDPService extends EventEmitter {
609609
);
610610
await executeBestEffort(
611611
this.logger,
612-
async () => this.shutdown(),
612+
async () => this.shutdown(ShutdownReason.RELAUNCH),
613613
"Error during shutdown before launch",
614614
);
615615
}
@@ -1278,21 +1278,24 @@ export class CDPService extends EventEmitter {
12781278
try {
12791279
return await this.launch(sessionConfig);
12801280
} catch (error) {
1281+
// If launch fails, ensure we still notify plugins about session end to allow for proper cleanup
1282+
await this.pluginManager.onBeforeSessionEnd(sessionConfig);
1283+
await this.pluginManager.onSessionEnd(sessionConfig);
12811284
await this.pluginManager.onAfterSessionEnd(sessionConfig);
12821285
throw error;
12831286
}
12841287
}
12851288

12861289
@traceable
1287-
public async endSession(): Promise<void> {
1290+
public async endSession(reason: ShutdownReason = ShutdownReason.SESSION_END): Promise<void> {
12881291
this.logger.info("Ending current session and resetting to default configuration.");
12891292
const sessionConfig = this.currentSessionConfig!;
12901293

12911294
this.sessionContext = await this.getBrowserState().catch(() => null);
12921295

12931296
try {
12941297
await this.pluginManager.onBeforeSessionEnd(sessionConfig);
1295-
await this.shutdown();
1298+
await this.shutdown(reason);
12961299
await this.pluginManager.onSessionEnd(sessionConfig);
12971300
this.currentSessionConfig = null;
12981301
this.sessionContext = null;

api/src/services/cdp/plugins/core/base-plugin.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,23 @@ import type { Browser, Page } from "puppeteer-core";
22
import type { CDPService } from "../../cdp.service.js";
33
import type { BrowserLauncherOptions } from "../../../../types/browser.js";
44

5+
/**
6+
* Reason why the browser is being shut down.
7+
* Plugins can use this to decide what cleanup actions to take.
8+
*/
9+
export enum ShutdownReason {
10+
/** Normal session end — user or system ended the session gracefully */
11+
SESSION_END = "session_end",
12+
/** Security violation detected (e.g. file:// protocol access) */
13+
SECURITY_VIOLATION = "security_violation",
14+
/** Browser is being relaunched (closing old instance before new launch) */
15+
RELAUNCH = "relaunch",
16+
/** Browser launch failed — cleanup before retry */
17+
LAUNCH_FAILURE = "launch_failure",
18+
/** Switching to a different browser mode (e.g. CDP → Selenium) */
19+
MODE_SWITCH = "mode_switch",
20+
}
21+
522
export interface PluginOptions {
623
name: string;
724
[key: string]: any;
@@ -33,7 +50,7 @@ export abstract class BasePlugin {
3350
public async onPageUnload(page: Page): Promise<void> {}
3451
public async onBrowserClose(browser: Browser): Promise<void> {}
3552
public async onBeforePageClose(page: Page): Promise<void> {}
36-
public async onShutdown(): Promise<void> {}
53+
public async onShutdown(reason: ShutdownReason): Promise<void> {}
3754
public async onSessionEnd(sessionConfig: BrowserLauncherOptions): Promise<void> {}
3855
}
3956

api/src/services/cdp/plugins/core/plugin-manager.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Browser, Page } from "puppeteer-core";
22
import { CDPService } from "../../cdp.service.js";
3-
import { BasePlugin } from "./base-plugin.js";
3+
import { BasePlugin, ShutdownReason } from "./base-plugin.js";
44
import { FastifyBaseLogger } from "fastify";
55
import { BrowserLauncherOptions } from "../../../../types/browser.js";
66

@@ -190,10 +190,10 @@ export class PluginManager {
190190
/**
191191
* Notify all plugins about shutdown
192192
*/
193-
public async onShutdown(): Promise<void> {
193+
public async onShutdown(reason: ShutdownReason): Promise<void> {
194194
const promises = Array.from(this.plugins.values()).map(async (plugin) => {
195195
try {
196-
await plugin.onShutdown();
196+
await plugin.onShutdown(reason);
197197
} catch (error) {
198198
this.logger.error(`Error in plugin ${plugin.name}.onShutdown: ${error}`);
199199
}

api/src/services/session.service.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { BrowserLauncherOptions, OptimizeBandwidthOptions } from "../types/index
1111
import { IProxyServer, ProxyServer } from "../utils/proxy.js";
1212
import { getBaseUrl, getUrl } from "../utils/url.js";
1313
import { CDPService } from "./cdp/cdp.service.js";
14+
import { ShutdownReason } from "./cdp/plugins/core/base-plugin.js";
1415
import { CookieData } from "./context/types.js";
1516
import { FileService } from "./file.service.js";
1617
import { SeleniumService } from "./selenium.service.js";
@@ -224,7 +225,7 @@ export class SessionService {
224225
};
225226

226227
if (isSelenium) {
227-
await this.cdpService.shutdown();
228+
await this.cdpService.shutdown(ShutdownReason.MODE_SWITCH);
228229
await this.seleniumService.launch(browserLauncherOptions);
229230

230231
Object.assign(this.activeSession, {

0 commit comments

Comments
 (0)