fix: preserve orphaned process electron (#29515)

cli/CHANGELOG.md

@@ -5,6 +5,7 @@ _Released 5/21/2024 (PENDING)_
 
 **Bugfixes:**
 
+- Fixed an issue where orphaned Electron processes were inadvertently terminating the browser's CRI client. Fixes [#28397](https://github.com/cypress-io/cypress/issues/28397). Fixed in [#29515](https://github.com/cypress-io/cypress/pull/29515).
 - Fixed an issue where Cypress was unable to search in the Specs list for files or folders containing numbers. Fixes [#29034](https://github.com/cypress-io/cypress/issues/29034).
 - Fixed an issue where Cypress would use the wrong URL to upload Test Replay recordings when it wasn't able to determine the upload URL. It now displays an error when the upload URL cannot be determined, rather than a "Request Entity Too Large" error. Addressed in [#29512](https://github.com/cypress-io/cypress/pull/29512).
 

packages/server/lib/browsers/electron.ts

@@ -54,6 +54,7 @@ const _getAutomation = async function (win, options: BrowserLaunchOpts, parent)
   const port = getRemoteDebuggingPort()
 
   if (!browserCriClient) {
+    debug(`browser CRI is not set. Creating...`)
     browserCriClient = await BrowserCriClient.create({
       hosts: ['127.0.0.1'],
       port,
@@ -68,6 +69,7 @@ const _getAutomation = async function (win, options: BrowserLaunchOpts, parent)
   const pageCriClient = await browserCriClient.attachToTargetUrl('about:blank')
 
   const sendClose = async () => {
+    debug(`sendClose called, browserCriClient is set? ${!!browserCriClient}`)
     if (browserCriClient) {
       const gracefulShutdown = true
 
@@ -443,10 +445,15 @@ export = {
    * Clear instance state for the electron instance, this is normally called on kill or on exit, for electron there isn't any state to clear.
    */
   clearInstanceState (options: GracefulShutdownOptions = {}) {
-    debug('closing remote interface client', { options })
-    // Do nothing on failure here since we're shutting down anyway
-    browserCriClient?.close(options.gracefulShutdown).catch(() => {})
-    browserCriClient = null
+    // in the case of orphaned browser clients, we should preserve the CRI client as it is connected
+    // to the same host regardless of window
+    debug('clearInstanceState called with options', { options })
+    if (!options.shouldPreserveCriClient) {
+      debug('closing remote interface client')
+      // Do nothing on failure here since we're shutting down anyway
+      browserCriClient?.close(options.gracefulShutdown).catch(() => {})
+      browserCriClient = null
+    }
   },
 
   connectToNewSpec (browser: Browser, options: ElectronOpts, automation: Automation) {
@@ -534,7 +541,7 @@ export = {
       allPids: [mainPid],
       browserWindow: win,
       kill (this: BrowserInstance) {
-        clearInstanceState({ gracefulShutdown: true })
+        clearInstanceState({ gracefulShutdown: true, shouldPreserveCriClient: this.isOrphanedBrowserProcess })
 
         if (this.isProcessExit) {
           // if the process is exiting, all BrowserWindows will be destroyed anyways

packages/server/lib/browsers/index.ts

@@ -24,12 +24,14 @@ interface KillOptions {
   isProcessExit?: boolean
   nullOut?: boolean
   unbind?: boolean
+  isOrphanedBrowserProcess?: boolean
 }
 
 const kill = (options: KillOptions = {}) => {
   options = _.defaults({}, options, {
     instance,
     isProcessExit: false,
+    isOrphanedBrowserProcess: false,
     unbind: true,
     nullOut: true,
   })
@@ -60,7 +62,7 @@ const kill = (options: KillOptions = {}) => {
     debug('killing browser process')
 
     instanceToKill.isProcessExit = options.isProcessExit
-
+    instanceToKill.isOrphanedBrowserProcess = options.isOrphanedBrowserProcess
     instanceToKill.kill()
   })
 }
@@ -180,7 +182,7 @@ export = {
 
     const _instance = await browserLauncher.open(browser, options.url, options, automation, ctx.coreData.servers.cdpSocketServer)
 
-    debug('browser opened')
+    debug(`browser opened for launch ${thisLaunchAttempt}`)
 
     // in most cases, we'll kill any running browser instance before launching
     // a new one when we call `await kill()` early in this function.
@@ -204,8 +206,11 @@ export = {
     // this browser, it means it has been orphaned and should be terminated.
     //
     // https://github.com/cypress-io/cypress/issues/24377
-    if (thisLaunchAttempt !== launchAttempt) {
-      await kill({ instance: _instance, nullOut: false })
+    const isOrphanedBrowserProcess = thisLaunchAttempt !== launchAttempt
+
+    if (isOrphanedBrowserProcess) {
+      debug(`killing process because launch attempt: ${thisLaunchAttempt} does not match current launch attempt: ${launchAttempt}`)
+      await kill({ instance: _instance, isOrphanedBrowserProcess, nullOut: false })
 
       return null
     }

packages/server/lib/browsers/types.ts

@@ -27,6 +27,7 @@ export type BrowserInstance = EventEmitter & {
    * TODO: remove need for this
    */
   isProcessExit?: boolean
+  isOrphanedBrowserProcess?: boolean
 }
 
 export type BrowserLauncher = {
@@ -52,4 +53,5 @@ export type BrowserLauncher = {
 
 export type GracefulShutdownOptions = {
   gracefulShutdown?: boolean
+  shouldPreserveCriClient?: boolean
 }

packages/server/test/unit/browsers/browsers_spec.js

@@ -209,6 +209,7 @@ describe('lib/browsers/index', () => {
       browsers._setInstance(null)
 
       expect(browserInstance1.kill).to.be.calledOnce
+      expect(browserInstance1.isOrphanedBrowserProcess).to.be.true
       expect(currentInstance).to.equal(browserInstance2)
     })
 
@@ -262,6 +263,7 @@ describe('lib/browsers/index', () => {
       browsers._setInstance(null)
 
       expect(browserInstance1.kill).to.be.calledOnce
+      expect(browserInstance1.isOrphanedBrowserProcess).to.be.true
       expect(currentInstance).to.equal(browserInstance2)
     })
   })

packages/server/test/unit/browsers/electron_spec.js

@@ -262,6 +262,33 @@ describe('lib/browsers/electron', () => {
     })
   })
 
+  context('.kill', () => {
+    beforeEach(async function () {
+      await electron._getAutomation({}, { onError: () => {} }, {})
+
+      await this.stubForOpen()
+
+      sinon.stub(electron, '_getBrowserCriClient').returns(this.browserCriClient)
+    })
+
+    it('does not terminate the browserCriClient if the instance is an orphaned process', async function () {
+      const instance = await electron.open('electron', this.url, this.options, this.automation)
+
+      instance.isOrphanedBrowserProcess = true
+      instance.kill()
+
+      expect(this.browserCriClient.close).not.to.be.called
+    })
+
+    it('terminates the browserCriClient otherwise', async function () {
+      const instance = await electron.open('electron', this.url, this.options, this.automation)
+
+      instance.kill()
+
+      expect(this.browserCriClient.close).to.be.called
+    })
+  })
+
   context('._launch', () => {
     beforeEach(() => {
       sinon.stub(menu, 'set')