-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: browser checks for run execution #734
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
Changes from 4 commits
2a302c8
6d75520
96339f5
b2f6211
fe4a12c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -221,6 +221,12 @@ export class BrowserPool { | |
| return undefined; | ||
| } | ||
|
|
||
| // Return undefined for failed slots | ||
| if (poolInfo.status === "failed") { | ||
| logger.log('debug', `Browser ${id} has failed status`); | ||
| return undefined; | ||
| } | ||
|
|
||
| return poolInfo.browser || undefined; | ||
| }; | ||
|
|
||
|
|
@@ -607,8 +613,13 @@ export class BrowserPool { | |
| * @returns true if successful, false if slot wasn't reserved | ||
| */ | ||
| public upgradeBrowserSlot = (id: string, browser: RemoteBrowser): boolean => { | ||
| if (!this.pool[id] || this.pool[id].status !== "reserved") { | ||
| logger.log('warn', `Cannot upgrade browser ${id}: slot not reserved`); | ||
| if (!this.pool[id]) { | ||
| logger.log('warn', `Cannot upgrade browser ${id}: slot does not exist in pool`); | ||
| return false; | ||
| } | ||
|
|
||
| if (this.pool[id].status !== "reserved") { | ||
| logger.log('warn', `Cannot upgrade browser ${id}: slot not in reserved state (current: ${this.pool[id].status})`); | ||
| return false; | ||
|
Comment on lines
+616
to
623
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Good guard: only upgrade reserved slots; add an 'initializing' step for richer introspection The stricter validation is appropriate. For better observability, consider setting status="initializing" when initialization starts (in controller) so getBrowserStatus reflects progress beyond "reserved". Add a setter and use it from the controller:
+ public setBrowserStatus = (id: string, status: "reserved" | "initializing" | "ready" | "failed"): boolean => {
+ const info = this.pool[id];
+ if (!info) return false;
+ info.status = status;
+ return true;
+ };
+ browserPool.setBrowserStatus(id, "initializing");🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
@@ -629,4 +640,17 @@ export class BrowserPool { | |
| this.deleteRemoteBrowser(id); | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Gets the current status of a browser slot. | ||
| * | ||
| * @param id browser ID to check | ||
| * @returns the status or null if browser doesn't exist | ||
| */ | ||
| public getBrowserStatus = (id: string): "reserved" | "initializing" | "ready" | "failed" | null => { | ||
| if (!this.pool[id]) { | ||
| return null; | ||
| } | ||
| return this.pool[id].status || null; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -242,7 +242,7 @@ const initializeBrowserAsync = async (id: string, userId: string) => { | |||||||||||||||||
| logger.log('warn', `No client connected to browser ${id} within timeout, proceeding with dummy socket`); | ||||||||||||||||||
| resolve(null); | ||||||||||||||||||
| } | ||||||||||||||||||
| }, 10000); | ||||||||||||||||||
| }, 15000); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| namespace.on('error', (error: any) => { | ||||||||||||||||||
|
|
@@ -272,21 +272,25 @@ const initializeBrowserAsync = async (id: string, userId: string) => { | |||||||||||||||||
| browserSession = new RemoteBrowser(dummySocket, userId, id); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| logger.log('debug', `Starting browser initialization for ${id}`); | ||||||||||||||||||
| await browserSession.initialize(userId); | ||||||||||||||||||
| logger.log('debug', `Browser initialization completed for ${id}`); | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+275
to
278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Log additions are good; set status='initializing' here for better introspection Emit initializing state before await initialize so worker logs report real progress. If you add BrowserPool.setBrowserStatus as suggested: + browserPool.setBrowserStatus(id, "initializing");
await browserSession.initialize(userId);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| const upgraded = browserPool.upgradeBrowserSlot(id, browserSession); | ||||||||||||||||||
| if (!upgraded) { | ||||||||||||||||||
| throw new Error('Failed to upgrade reserved browser slot'); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (socket) { | ||||||||||||||||||
| socket.emit('ready-for-run'); | ||||||||||||||||||
| } else { | ||||||||||||||||||
| setTimeout(async () => { | ||||||||||||||||||
| try { | ||||||||||||||||||
| logger.log('info', `Starting execution for browser ${id} with dummy socket`); | ||||||||||||||||||
| logger.log('info', `Browser ${id} with dummy socket is ready for execution`); | ||||||||||||||||||
| } catch (error: any) { | ||||||||||||||||||
| logger.log('error', `Error executing run for browser ${id}: ${error.message}`); | ||||||||||||||||||
| logger.log('error', `Error with dummy socket browser ${id}: ${error.message}`); | ||||||||||||||||||
| } | ||||||||||||||||||
| }, 100); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -299,10 +303,12 @@ const initializeBrowserAsync = async (id: string, userId: string) => { | |||||||||||||||||
| if (socket) { | ||||||||||||||||||
| socket.emit('error', { message: error.message }); | ||||||||||||||||||
| } | ||||||||||||||||||
| throw error; | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+306
to
307
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rethrowing here creates an unhandled promise rejection at the call site initializeBrowserAsync is invoked without await/catch in createRemoteBrowserForRun, so rethrowing will surface as an unhandled rejection and can crash the process depending on Node settings. Fix by catching at the call site: - initializeBrowserAsync(id, userId);
+ void initializeBrowserAsync(id, userId).catch((err) => {
+ logger.log('error', `Failed to initialize browser ${id}: ${err?.message || err}`);
+ try { browserPool.failBrowserSlot(id); } catch (e) {
+ logger.log('warn', `Failed to mark slot ${id} as failed: ${e}`);
+ }
+ });Optionally, if you prefer not to rethrow inside initializeBrowserAsync, remove the throws and rely on logging + failBrowserSlot there; but catching at the call site is safer and explicit. Also applies to: 312-313 🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| } catch (error: any) { | ||||||||||||||||||
| logger.log('error', `Error setting up browser ${id}: ${error.message}`); | ||||||||||||||||||
| browserPool.failBrowserSlot(id); | ||||||||||||||||||
| throw error; | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -215,7 +215,8 @@ async function triggerIntegrationUpdates(runId: string, robotMetaId: string): Pr | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Modified processRunExecution function - only add browser reset | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function processRunExecution(job: Job<ExecuteRunData>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const BROWSER_INIT_TIMEOUT = 30000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const BROWSER_INIT_TIMEOUT = 60000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const BROWSER_PAGE_TIMEOUT = 45000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const data = job.data; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('info', `Processing run execution job for runId: ${data.runId}, browserId: ${data.browserId}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -244,15 +245,28 @@ async function processRunExecution(job: Job<ExecuteRunData>) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let browser = browserPool.getRemoteBrowser(browserId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const browserWaitStart = Date.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let lastLogTime = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (!browser && (Date.now() - browserWaitStart) < BROWSER_INIT_TIMEOUT) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('debug', `Browser ${browserId} not ready yet, waiting...`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const currentTime = Date.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const browserStatus = browserPool.getBrowserStatus(browserId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (browserStatus === null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Browser slot ${browserId} does not exist in pool`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (currentTime - lastLogTime > 10000) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('info', `Browser ${browserId} not ready yet (status: ${browserStatus}), waiting... (${Math.round((currentTime - browserWaitStart) / 1000)}s elapsed)`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lastLogTime = currentTime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await new Promise(resolve => setTimeout(resolve, 2000)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| browser = browserPool.getRemoteBrowser(browserId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+251
to
264
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Early-exit when status is 'failed' to avoid spinning until timeout If BrowserPool begins to persist 'failed' (recommended), this loop should stop immediately rather than wait until timeout. Apply: - const browserStatus = browserPool.getBrowserStatus(browserId);
+ const browserStatus = browserPool.getBrowserStatus(browserId);
if (browserStatus === null) {
throw new Error(`Browser slot ${browserId} does not exist in pool`);
}
+ if (browserStatus === "failed") {
+ throw new Error(`Browser ${browserId} initialization failed`);
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!browser) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Browser ${browserId} not found in pool after timeout`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const finalStatus = browserPool.getBrowserStatus(browserId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Browser ${browserId} not found in pool after ${BROWSER_INIT_TIMEOUT/1000}s timeout (final status: ${finalStatus})`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('info', `Browser ${browserId} found and ready for execution`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -273,14 +287,22 @@ async function processRunExecution(job: Job<ExecuteRunData>) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let currentPage = browser.getCurrentPage(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const pageWaitStart = Date.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (!currentPage && (Date.now() - pageWaitStart) < 30000) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('debug', `Page not ready for browser ${browserId}, waiting...`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let lastPageLogTime = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (!currentPage && (Date.now() - pageWaitStart) < BROWSER_PAGE_TIMEOUT) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const currentTime = Date.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (currentTime - lastPageLogTime > 5000) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('info', `Page not ready for browser ${browserId}, waiting... (${Math.round((currentTime - pageWaitStart) / 1000)}s elapsed)`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lastPageLogTime = currentTime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| currentPage = browser.getCurrentPage(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!currentPage) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`No current page available for browser ${browserId} after timeout`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`No current page available for browser ${browserId} after ${BROWSER_PAGE_TIMEOUT/1000}s timeout`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log('info', `Starting workflow execution for run ${data.runId}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
🛠️ Refactor suggestion
'failed' status check is currently unreachable given failBrowserSlot deletes the entry
getRemoteBrowser returns undefined for status "failed", but failBrowserSlot immediately deletes slots instead of persisting a failed status. As a result, worker logs will see finalStatus=null, never "failed".
To expose meaningful final statuses and enable earlier bailouts:
Proposed minimal change to persist "failed" briefly:
If you go with this, also have the worker break early when it sees status === "failed". I’ve added a suggested worker diff in its review.