fix: Ensure that file watchers are closed before Electron exits (#22606)

* Adding prototype before-quit handler to handle async teardown.

* Getting binary builds

* Let's try this then

* Working the async changes backwards, hope I got them all. Unit tests will be a disaster currently.

* Actually getting build artifacts for testing

* Moving logic to server interactive/run

* fix: Fix some tests

* fix: Revert changes to circle config

* fix: Fix some tests

* fix: Fix more tests

* fix: Remove dead code comment

* fix: Fix ProjectDataSource tests

* fix: Add comment prefix

* fix: Remove comment and unnecessary async

* fix: Build Mac binary

* Reverting run changes

* Addressing PR comments. Cleaning up a few naming quibbles I had.

* Addressing TODO regarding onLoadError watcher cleanup.

* Correcting catch

* Fighting some unref errors on these catches

* Reverting making this private en lieu of binding

* Should have left these as instances, whoops

* Removing unnecessary test that was previously skipped

* Adding a couple cheap unit tests for the new interactive mode behavior

Co-authored-by: Tyler Biethman <[email protected]>
Co-authored-by: Tyler Biethman <[email protected]>

Author: astone123

circle.yml

@@ -37,6 +37,7 @@ macWorkflowFilters: &darwin-workflow-filters
     or:
     - equal: [ develop, << pipeline.git.branch >> ]
     - equal: [ linux-arm64, << pipeline.git.branch >> ]
+    - equal: [ 'tbiethman/UNIFY-1816-prototype', << pipeline.git.branch >> ]
     - matches:
           pattern: "-release$"
           value: << pipeline.git.branch >>
@@ -128,7 +129,7 @@ commands:
       - run:
           name: Check current branch to persist artifacts
           command: |
-            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "issue-22147-nohoist" ]]; then
+            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "tbiethman/UNIFY-1816-prototype" ]]; then
               echo "Not uploading artifacts or posting install comment for this branch."
               circleci-agent step halt
             fi

packages/data-context/src/DataContext.ts

@@ -429,10 +429,14 @@ export class DataContext {
   }
 
   async destroy () {
-    const destroy = util.promisify(this.coreData.servers.gqlServer?.destroy || (() => {}))
+    let destroyGqlServer = () => Promise.resolve()
+
+    if (this.coreData.servers.gqlServer?.destroy) {
+      destroyGqlServer = util.promisify(this.coreData.servers.gqlServer.destroy)
+    }
 
     return Promise.all([
-      destroy(),
+      destroyGqlServer(),
       this._reset(),
     ])
   }

packages/data-context/src/actions/ProjectActions.ts

@@ -93,7 +93,7 @@ export class ProjectActions {
       d.app.browserStatus = 'closed'
     })
 
-    this.ctx.lifecycleManager.clearCurrentProject()
+    await this.ctx.lifecycleManager.clearCurrentProject()
     resetIssuedWarnings()
     await this.api.closeActiveProject()
   }
@@ -128,13 +128,13 @@ export class ProjectActions {
     await this.updateProjectList(() => this.api.insertProjectToCache(projectRoot))
 
     await this.clearCurrentProject()
-    this.ctx.lifecycleManager.setCurrentProject(projectRoot)
+    await this.ctx.lifecycleManager.setCurrentProject(projectRoot)
   }
 
   // Temporary: remove after other refactor lands
-  setCurrentProjectAndTestingTypeForTestSetup (projectRoot: string) {
-    this.ctx.lifecycleManager.clearCurrentProject()
-    this.ctx.lifecycleManager.setCurrentProject(projectRoot)
+  async setCurrentProjectAndTestingTypeForTestSetup (projectRoot: string) {
+    await this.ctx.lifecycleManager.clearCurrentProject()
+    await this.ctx.lifecycleManager.setCurrentProject(projectRoot)
     this.ctx.lifecycleManager.setCurrentTestingType('e2e')
     // @ts-expect-error - we are setting this as a convenience for our integration tests
     this.ctx._modeOptions = {}
@@ -416,7 +416,7 @@ export class ProjectActions {
 
     this.ctx.actions.project.setSpecs(specs)
 
-    this.ctx.project.startSpecWatcher({
+    await this.ctx.project.startSpecWatcher({
       projectRoot,
       testingType,
       specPattern,

packages/data-context/src/data/ProjectConfigManager.ts

@@ -27,7 +27,7 @@ type ProjectConfigManagerOptions = {
   handlers: IpcHandler[]
   hasCypressEnvFile: boolean
   eventRegistrar: EventRegistrar
-  onError: (cypressError: CypressError, title?: string | undefined) => void
+  onError: (cypressError: CypressError) => void
   onInitialConfigLoaded: (initialConfig: Cypress.ConfigOptions) => void
   onFinalConfigLoaded: (finalConfig: FullConfig, options: OnFinalConfigLoadedOptions) => Promise<void>
   refreshLifecycle: () => Promise<void>
@@ -106,7 +106,7 @@ export class ProjectConfigManager {
       this._state = 'loadingConfig'
 
       // Clean things up for a new load
-      this.closeWatchers()
+      await this.closeWatchers()
       this._cachedLoadConfig = undefined
       this._cachedFullConfig = undefined
 
@@ -137,7 +137,7 @@ export class ProjectConfigManager {
       }
 
       this._state = 'errored'
-      this.closeWatchers()
+      await this.closeWatchers()
 
       throw error
     } finally {
@@ -177,7 +177,7 @@ export class ProjectConfigManager {
         this._eventsIpc.cleanupIpc()
       }
 
-      this.closeWatchers()
+      await this.closeWatchers()
 
       throw error
     } finally {
@@ -310,9 +310,9 @@ export class ProjectConfigManager {
     )
   }
 
-  onLoadError = (error: any) => {
-    this.closeWatchers()
-    this.options.onError(error, 'Cypress configuration error')
+  onLoadError = async (error: any) => {
+    await this.closeWatchers()
+    this.options.onError(error)
   }
 
   private watchFiles (paths: string[]) {
@@ -495,17 +495,18 @@ export class ProjectConfigManager {
     return true
   }
 
-  private closeWatchers () {
-    for (const watcher of this._watchers.values()) {
-      // We don't care if there's an error while closing the watcher,
-      // the watch listener on our end is already removed synchronously by chokidar
-      watcher.close().catch((e) => {})
-    }
+  private async closeWatchers () {
+    await Promise.all(Array.from(this._watchers).map((watcher) => {
+      return watcher.close().catch((error) => {
+        // Watcher close errors are ignored, we cannot meaningfully handle these
+      })
+    }))
+
     this._watchers = new Set()
     this._pathToWatcherRecord = {}
   }
 
-  destroy () {
+  async destroy () {
     if (this._eventsIpc) {
       this._eventsIpc.cleanupIpc()
     }
@@ -514,6 +515,6 @@ export class ProjectConfigManager {
     this._cachedLoadConfig = undefined
     this._cachedFullConfig = undefined
     this._registeredEventsTarget = undefined
-    this.closeWatchers()
+    await this.closeWatchers()
   }
 }

packages/data-context/src/data/ProjectLifecycleManager.ts

@@ -89,23 +89,18 @@ export class ProjectLifecycleManager {
 
   constructor (private ctx: DataContext) {
     this._eventRegistrar = new EventRegistrar()
+
     if (ctx.coreData.currentProject) {
-      this.setCurrentProject(ctx.coreData.currentProject)
+      this._setCurrentProject(ctx.coreData.currentProject)
     }
 
-    process.on('exit', this.onProcessExit)
-
     return autoBindDebug(this)
   }
 
   get git () {
     return this.ctx.coreData.currentProjectGitInfo
   }
 
-  private onProcessExit = () => {
-    this.resetInternalState()
-  }
-
   async getProjectId (): Promise<string | null> {
     try {
       const contents = await this.ctx.project.getConfig()
@@ -185,8 +180,8 @@ export class ProjectLifecycleManager {
     return this._configManager?.eventProcessPid
   }
 
-  clearCurrentProject () {
-    this.resetInternalState()
+  async clearCurrentProject () {
+    await this.resetInternalState()
     this._initializedProject = undefined
     this._projectRoot = undefined
   }
@@ -215,13 +210,7 @@ export class ProjectLifecycleManager {
       handlers: getServerPluginHandlers(),
       hasCypressEnvFile: this._projectMetaState.hasCypressEnvFile,
       eventRegistrar: this._eventRegistrar,
-      onError: (cypressError, title) => {
-        if (this.ctx.isRunMode && this._pendingInitialize) {
-          this._pendingInitialize.reject(cypressError)
-        } else {
-          this.ctx.onError(cypressError, title)
-        }
-      },
+      onError: this.onLoadError,
       onInitialConfigLoaded: (initialConfig: Cypress.ConfigOptions) => {
         this._cachedInitialConfig = initialConfig
 
@@ -391,23 +380,10 @@ export class ProjectLifecycleManager {
     return this._configManager.initializeConfig()
   }
 
-  /**
-   * When we set the current project, we need to cleanup the
-   * previous project that might have existed. We use this as the
-   * single location we should use to set the `projectRoot`, because
-   * we can call it from legacy code and it'll be a no-op if the `projectRoot`
-   * is already the same, otherwise it'll do the necessary cleanup
-   */
-  setCurrentProject (projectRoot: string) {
-    if (this._projectRoot === projectRoot) {
-      return
-    }
-
+  private _setCurrentProject (projectRoot: string) {
     this._projectRoot = projectRoot
     this._initializedProject = undefined
 
-    this.resetInternalState()
-
     this._configManager = this.createConfigManager()
 
     // Preemptively load these so that they are available when we need them later
@@ -443,6 +419,23 @@ export class ProjectLifecycleManager {
     }
   }
 
+  /**
+   * When we set the current project, we need to cleanup the
+   * previous project that might have existed. We use this as the
+   * single location we should use to set the `projectRoot`, because
+   * we can call it from legacy code and it'll be a no-op if the `projectRoot`
+   * is already the same, otherwise it'll do the necessary cleanup
+   */
+  async setCurrentProject (projectRoot: string) {
+    if (this._projectRoot === projectRoot) {
+      return
+    }
+
+    await this.resetInternalState()
+
+    this._setCurrentProject(projectRoot)
+  }
+
   /**
    * Handles pre-initialization checks. These will display warnings or throw with errors if catastrophic.
    * Returns false, if we're not ready to initialize due to needing to migrate
@@ -551,14 +544,14 @@ export class ProjectLifecycleManager {
     }
   }
 
-  private resetInternalState () {
+  private async resetInternalState () {
     if (this._configManager) {
-      this._configManager.destroy()
+      await this._configManager.destroy()
       this._configManager = undefined
     }
 
-    this.ctx.coreData.currentProjectGitInfo?.destroy()
-    this.ctx.project.destroy()
+    await this.ctx.coreData.currentProjectGitInfo?.destroy()
+    await this.ctx.project.destroy()
     this._currentTestingType = null
     this._cachedInitialConfig = undefined
     this._cachedFullConfig = undefined
@@ -580,9 +573,9 @@ export class ProjectLifecycleManager {
     return this._configManager.getConfigFileContents()
   }
 
-  reinitializeCypress () {
+  async reinitializeCypress () {
     resetPluginHandlers()
-    this.resetInternalState()
+    await this.resetInternalState()
   }
 
   registerEvent (event: string, callback: Function) {
@@ -717,10 +710,8 @@ export class ProjectLifecycleManager {
     }
   }
 
-  destroy () {
-    this.resetInternalState()
-    // @ts-ignore
-    process.removeListener('exit', this.onProcessExit)
+  async destroy () {
+    await this.resetInternalState()
   }
 
   isTestingTypeConfigured (testingType: TestingType): boolean {
@@ -798,8 +789,8 @@ export class ProjectLifecycleManager {
    * for run mode
    */
   onLoadError = (err: CypressError) => {
-    if (this.ctx.isRunMode && this._configManager) {
-      this._configManager.onLoadError(err)
+    if (this.ctx.isRunMode && this._pendingInitialize) {
+      this._pendingInitialize.reject(err)
     } else {
       this.ctx.onError(err, 'Cypress configuration error')
     }

packages/data-context/src/index.ts

@@ -23,13 +23,11 @@ export { globalPubSub }
 
 let ctx: DataContext | null = null
 
-// globalPubSub.on('cleanup', clearCtx)
-
-/**
- * Shouldn't ever be called from runtime code, primarily for test situations where we need to
- */
-export function clearCtx () {
-  ctx = null
+export async function clearCtx () {
+  if (ctx) {
+    await ctx.destroy()
+    ctx = null
+  }
 }
 
 export function hasCtx () {

packages/data-context/src/sources/GitDataSource.ts

@@ -156,18 +156,18 @@ export class GitDataSource {
     return this.#currentBranch
   }
 
-  destroy () {
+  async destroy () {
     this.#destroyed = true
     if (this.#intervalTimer) {
       clearTimeout(this.#intervalTimer)
     }
 
-    this.#destroyWatcher(this.#gitBaseDirWatcher)
+    await this.#destroyWatcher(this.#gitBaseDirWatcher)
   }
 
-  #destroyWatcher (watcher?: chokidar.FSWatcher) {
+  async #destroyWatcher (watcher?: chokidar.FSWatcher) {
     // Can't do anything actionable with these errors
-    watcher?.close().catch((e) => {})
+    await watcher?.close().catch((e) => {})
   }
 
   async #loadAndWatchCurrentBranch () {

packages/data-context/src/sources/ProjectDataSource.ts

@@ -289,15 +289,15 @@ export class ProjectDataSource {
     return matched
   }
 
-  startSpecWatcher ({
+  async startSpecWatcher ({
     projectRoot,
     testingType,
     specPattern,
     configSpecPattern,
     excludeSpecPattern,
     additionalIgnorePattern,
   }: FindSpecs<string[]>) {
-    this.stopSpecWatcher()
+    await this.stopSpecWatcher()
 
     // Early return the spec watcher if we're in run mode, we do not want to
     // init a lot of files watchers that are unneeded
@@ -455,16 +455,16 @@ export class ProjectDataSource {
     return false
   }
 
-  destroy () {
-    this.stopSpecWatcher()
+  async destroy () {
+    await this.stopSpecWatcher()
   }
 
-  stopSpecWatcher () {
+  async stopSpecWatcher () {
     if (!this._specWatcher) {
       return
     }
 
-    this._specWatcher.close().catch(() => {})
+    await this._specWatcher.close().catch(() => {})
     this._specWatcher = null
   }