-
Notifications
You must be signed in to change notification settings - Fork 12
Utilize idle tabs in puppeteer for concurrent rendering #3860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234084aa5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let entries = this.#realmPages.get(realm); | ||
| let entryList = entries | ||
| ? [...entries].filter((entry) => !entry.closing && !entry.transitioning) | ||
| : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow same-realm queueing during tab transitions
In #selectEntryForRealm, the entry list for a realm excludes any tab marked transitioning (filter((entry) => !entry.closing && !entry.transitioning)). When a cross‑realm request queues on a busy tab, the method sets transitioning = true for that tab later in this function, so a subsequent request for the original realm no longer sees its own tab to queue behind. If the pool is full (e.g., maxPages = 1 and no standby), the same‑realm request now throws “No standby page available” even though the tab would become free shortly. This only happens under concurrent cross‑realm load, but it turns what could be a queued render into a hard failure. Consider letting same‑realm requests queue on transitioning tabs (or scoping transitioning to cross‑realm selection) to avoid these false “no capacity” errors.
Useful? React with 👍 / 👎.
Host Test Results 1 files ± 0 1 suites ±0 1h 35m 59s ⏱️ + 7m 2s Results for commit 234084a. ± Comparison against base commit 90d8c13. This pull request removes 1 and adds 219 tests. Note that renamed tests count towards both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the prerenderer to enable concurrent rendering across multiple Chrome tabs per server, replacing the previous per-realm serialization model. The changes introduce tab-level queuing and a configurable per-realm tab maximum to optimize resource utilization.
Changes:
- Removed global per-realm serialization, moving concurrency control to tab-level queuing within PagePool
- Introduced
PRERENDER_REALM_TAB_MAXenvironment variable to limit concurrent tabs per realm - Updated RenderRunner to use a lease/release pattern with try-finally blocks for proper resource cleanup
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/realm-server/tests/prerendering-test.ts | Added TestSemaphore and comprehensive tests for concurrent tab usage, queuing behavior, and tab reassignment logic |
| packages/realm-server/prerender/render-runner.ts | Refactored to acquire page leases with release callbacks and ensure cleanup via try-finally blocks; simplified auth change detection |
| packages/realm-server/prerender/prerenderer.ts | Removed per-realm promise chaining and global semaphore acquisition; simplified retry logic by delegating concurrency control to PagePool |
| packages/realm-server/prerender/page-pool.ts | Implemented TabQueue for tab-level queuing, added tab selection logic favoring idle aligned tabs, and introduced transitioning/closing flags to prevent race conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'tab realigned to realm-b when queued request starts', | ||
| ); | ||
| third.release(); | ||
| blocker.release(); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blocker lease is released twice (lines 2263 and 2276). This could cause issues if the release function is not idempotent. While the PagePool's release function appears to check for double-release, this is confusing and may indicate a logic error in the test.
| blocker.release(); |
| shortCircuit = true; | ||
| break; | ||
| } | ||
| if (this.#isAuthError(error)) { | ||
| shortCircuit = true; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value assigned to shortCircuit here is unused.
| shortCircuit = true; | |
| break; | |
| } | |
| if (this.#isAuthError(error)) { | |
| shortCircuit = true; | |
| break; | |
| } | |
| if (this.#isAuthError(error)) { |
| break; | ||
| } | ||
| if (this.#isAuthError(error)) { | ||
| shortCircuit = true; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value assigned to shortCircuit here is unused.
| shortCircuit = true; |
| isolatedHTML = capture.value; | ||
| } else { | ||
| let capErr = this.#captureToError(capture); | ||
| if (!error && capErr) { |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This negation always evaluates to true.
| if (!error && capErr) { | |
| if (capErr) { |
| shortCircuit = true; | ||
| break; | ||
| for (let step of steps) { | ||
| if (shortCircuit) break; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use of variable 'shortCircuit' always evaluates to false.
| if (shortCircuit) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234084aa5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| for (let entry of entries) { | ||
| void this.#closeEntry(entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent oversubscribing contexts during async eviction
In the awaitIdle: false branch, the realm is removed from #realmPages and each entry is closed asynchronously. Because pool sizing (#poolEntryCount/#totalContextCount) only counts entries still in #realmPages, the soon‑to‑be‑closed contexts are no longer counted and #ensureStandbyPool() can spin up new standbys while the old contexts are still running. Under timeout/unusable evictions (the new awaitIdle: false path), this can temporarily exceed the configured maxPages+1 and spike Chrome context count/memory. Consider keeping closing entries in the map (marked closing) or deferring standby creation until closes finish to preserve the capacity limit.
Useful? React with 👍 / 👎.
This PR updates how our prerenderer works so that all chrome tabs are used for rendering, such that a single server can concurrently handle multiple render requests.
PRERENDER_REALM_TAB_MAXenv var to parameter store and infra