refactor!: remove the EventEmitter interface from SessionPool#3643
Open
barjin wants to merge 11 commits into
Open
refactor!: remove the EventEmitter interface from SessionPool#3643barjin wants to merge 11 commits into
EventEmitter interface from SessionPool#3643barjin wants to merge 11 commits into
Conversation
…able Previously, calling retire() bumped errorScore to maxErrorScore but a subsequent markGood() (e.g. the automatic markGood after a successful requestHandler that explicitly retired the session) could decrement the score back below the threshold, making the session usable again. Track retirement in a dedicated _retired flag checked by isUsable() so retire() is a true terminal state.
Replace the global EVENT_SESSION_RETIRED listener and the per-controller browserSessionIds map with a check at the per-request cleanup hook: if the session ended the request unusable, retire the browser controller. The previous mechanism tore down browsers eagerly mid-flight; the new one lets the in-flight request finish on the doomed browser and retires it once the request is done. Same outcome, no global event subscription needed.
SessionPool no longer extends EventEmitter and no longer fires a sessionRetired event. The Session->SessionPool back-reference, the sessionPool constructor option on Session, and the EVENT_SESSION_RETIRED constant are gone with it. The only consumer of that event was the browser crawler, which now retires browsers via the per-request context pipeline cleanup. Custom createSessionFunction implementations that manually constructed Session instances should drop the sessionPool argument.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a breaking refactor that removes the EventEmitter-based event system from SessionPool and updates crawler logic to retire browser instances based on session usability at the end of request processing. This simplifies the SessionPool/Session relationship and reduces cross-package coupling, as described in the PR metadata and #3617 context.
Changes:
- Remove
SessionPool extends EventEmitterand dropEVENT_SESSION_RETIRED/sessionPoolback-reference fromSession. - Update
BrowserCrawlerto retire the current browser controller in a deferred cleanup whensession.isUsable()isfalse. - Update tests and upgrade docs to reflect the new
Session/SessionPoolconstruction patterns.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/core/session_pool/session.test.ts | Updates Session tests to not require a SessionPool and removes event-emission assertions. |
| test/core/session_pool/session_pool.test.ts | Adjusts SessionPool tests for sessions no longer holding a sessionPool reference. |
| test/core/crawlers/puppeteer_crawler.test.ts | Updates custom createSessionFunction to construct Session without sessionPool. |
| test/core/crawlers/browser_crawler.test.ts | Updates browser-retirement test to rely on crawler end-of-request retirement behavior. |
| packages/core/src/session_pool/session.ts | Removes EventEmitter validation, removes sessionPool option, adds _retired terminal state affecting isUsable(). |
| packages/core/src/session_pool/session_pool.ts | Removes EventEmitter inheritance and updates default session creation + load path accordingly. |
| packages/core/src/session_pool/index.ts | Stops exporting ./events.js. |
| packages/core/src/session_pool/events.ts | Deletes EVENT_SESSION_RETIRED. |
| packages/browser-crawler/src/internals/browser-crawler.ts | Removes session-retired listener mechanism and retires browser controllers during deferred cleanup when session becomes unusable. |
| packages/basic-crawler/src/internals/basic-crawler.ts | Updates default createSessionFunction signature and removes setMaxListeners call on SessionPool. |
| docs/upgrading/upgrading_v4.md | Documents the breaking change and new recommended patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bail early when the session is already retired so subsequent retire() calls don't keep inflating errorScore and usageCount. Without this guard, the auto markGood() the crawler invokes after a successful requestHandler that explicitly retired the session triggers _maybeSelfRetire() -> retire() and double-bumps both counters.
The previous block still referred to the removed sessionRetired event emission. Rewrite it to describe the current behaviour: a terminal retire state, idempotent, with markGood/markBad unable to bring the session back.
The terminal _retired flag was not part of SessionState, so retired sessions could resurrect after a SessionPool persist/restore cycle: a previously retired session would be rebuilt without the flag, and since the auto markGood() following a retire() leaves errorScore below maxErrorScore, isUsable() would return true again. Thread retired through SessionState, expose a public Session.retired getter, accept retired in the constructor (ow shape + destructuring), and emit it from getState() so the flag survives the round-trip.
The leading SessionPool argument was only useful to pass into the Session constructor, which no longer keeps a back-reference to the pool. CreateSession now takes a single options object — same shape as before, just without the redundant first parameter. Merge the two SessionPool-related sections in the v4 upgrade guide into a single 'createSessionFunction signature has changed' entry covering both the merged-options behaviour and the dropped argument.
The JSDoc on the createSessionFunction option still said the function receives a SessionPool instance — that argument was just removed.
Match the naming of the other Session predicates (isBlocked, isExpired, isMaxUsageCountReached, isUsable). The persisted SessionState.retired field stays as-is — it's a noun-style state field consistent with errorScore, usageCount, etc.
…method" This reverts commit e1b4efc.
janbuchar
reviewed
May 12, 2026
janbuchar
approved these changes
May 12, 2026
… session Address PR review: spell out the leak-prevention rationale (non-incognito controllers carry cookies/storage across pages, so a no-longer-usable session would taint whichever session lands on the controller next).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes
extends EventEmitterfromSessionPool. The event system was originally only used to retire browsers on expiringSessions.BrowserCrawlernow retires the browser at the end ofRequestprocessing, if theSessionproves to be retired. This leads to a simpler API.This refactor simplifies the
SessionPoolinterface and allows us to drop theSessionPoolreferences from different parts of the codebase.Related to (prerequisite of) #3617