fix: Cleanup on stopping agent (#382)

* ♻️ Dry up start and exec commands

Both of these commands start and stop in the same way, and all
commands can benefit from handling termination events

* fix: 🐛 Suppress error when a PID file does not exist

* fix: 🐛 Cleanup PID file when start command is terminated (#340)

When sigterm and friends are recieved, the server gets stopped but leaves behind
the PID file which causes future runs to think there is already a running
agent. With the recent adjustments to how termination events are handled, we can
safely cleanup that file in the stop method.

* fix: 🐛 Catch error from child process event and stop agent

* fix: 🐛 Add process handlers to static snapshot command

src/commands/exec.ts

@@ -32,9 +32,6 @@ export default class Exec extends PercyCommand {
     }),
   }
 
-  // helps prevent exiting before the agent service has stopped
-  private exiting = false
-
   async run() {
     await super.run()
 
@@ -49,29 +46,15 @@ export default class Exec extends PercyCommand {
     }
 
     if (this.percyWillRun()) {
-      const configuration = new ConfigurationService().applyFlags(flags)
-      await this.agentService.start(configuration)
-      this.logStart()
+      await this.start(flags)
     }
 
     // Even if Percy will not run, continue to run the subprocess
     const spawnedProcess = spawn(command, argv, { stdio: 'inherit' })
     spawnedProcess.on('exit', (code) => this.stop(code))
-
-    // Receiving any of these events should stop the agent and exit
-    process.on('SIGHUP', () => this.stop())
-    process.on('SIGINT', () => this.stop())
-    process.on('SIGTERM', () => this.stop())
-  }
-
-  private async stop(exitCode?: number | null) {
-    if (this.exiting) { return }
-    this.exiting = true
-
-    if (this.percyWillRun()) {
-      await this.agentService.stop()
-    }
-
-    process.exit(exitCode || 0)
+    spawnedProcess.on('error', (error) => {
+      this.logger.error(error)
+      this.stop(1)
+    })
   }
 }

src/commands/percy-command.ts

@@ -1,6 +1,7 @@
-import {Command} from '@oclif/command'
+import { Command } from '@oclif/command'
 import * as winston from 'winston'
-import {AgentService} from '../services/agent-service'
+import { AgentService } from '../services/agent-service'
+import ConfigurationService from '../services/configuration-service'
 import ProcessService from '../services/process-service'
 import logger from '../utils/logger'
 
@@ -12,6 +13,9 @@ export default class PercyCommand extends Command {
   logger: winston.Logger
   percyToken: string
 
+  // helps prevent exiting before the agent service has stopped
+  private exiting = false
+
   constructor(argv: string[], config: any) {
     super(argv, config)
 
@@ -42,4 +46,28 @@ export default class PercyCommand extends Command {
   logStart() {
     this.logger.info('percy has started.')
   }
+
+  async start(flags: any) {
+    if (this.percyWillRun()) {
+      const configuration = new ConfigurationService().applyFlags(flags)
+      await this.agentService.start(configuration)
+      this.logStart()
+
+      // Receiving any of these events should stop the agent and exit
+      process.on('SIGHUP', () => this.stop())
+      process.on('SIGINT', () => this.stop())
+      process.on('SIGTERM', () => this.stop())
+    }
+  }
+
+  async stop(exitCode?: number | null) {
+    if (this.exiting) { return }
+    this.exiting = true
+
+    if (this.percyWillRun()) {
+      await this.agentService.stop()
+    }
+
+    process.exit(exitCode || 0)
+  }
 }

src/commands/snapshot.ts

@@ -84,8 +84,8 @@ export default class Snapshot extends PercyCommand {
       this.exit(1)
     }
 
-    await this.agentService.start(configuration)
-    this.logStart()
+    // start agent service and attach process handlers
+    await this.start(flags)
 
     const staticSnapshotService = new StaticSnapshotService(configuration['static-snapshots'])
 
@@ -97,6 +97,6 @@ export default class Snapshot extends PercyCommand {
 
      // stop the static snapshot and agent services
     await staticSnapshotService.stop()
-    await this.agentService.stop()
+    await this.stop()
   }
 }

src/commands/start.ts

@@ -42,51 +42,29 @@ export default class Start extends PercyCommand {
     // If Percy is disabled or is missing a token, gracefully exit here
     if (!this.percyWillRun()) { this.exit(0) }
 
-    const {flags} = this.parse(Start)
+    const { flags } = this.parse(Start)
 
     if (flags.detached) {
-      this.runDetached()
+      this.runDetached(flags)
     } else {
-      await this.runAttached()
+      await this.start(flags)
     }
 
     await healthCheck(flags.port!)
   }
 
-  private async runAttached() {
-    const {flags} = this.parse(Start)
-
-    process.on('SIGHUP', async () => {
-      await this.agentService.stop()
-      process.exit(0)
-    })
-
-    process.on('SIGINT', async () => {
-      await this.agentService.stop()
-      process.exit(0)
-    })
-
-    process.on('SIGTERM', async () => {
-      await this.agentService.stop()
-      process.exit(0)
-    })
-
-    const configuration = new ConfigurationService().applyFlags(flags)
-    await this.agentService.start(configuration)
-    this.logStart()
+  async stop(exitCode?: any) {
+    this.processService.cleanup()
+    await super.stop(exitCode)
   }
 
-  private runDetached() {
-    const {flags} = this.parse(Start)
-
-    const pid = this.processService.runDetached(
-      [
-        path.resolve(`${__dirname}/../../bin/run`),
-        'start',
-        '-p', String(flags.port!),
-        '-t', String(flags['network-idle-timeout']),
-      ],
-    )
+  private runDetached(flags: any) {
+    const pid = this.processService.runDetached([
+      path.resolve(`${__dirname}/../../bin/run`),
+      'start',
+      '-p', String(flags.port!),
+      '-t', String(flags['network-idle-timeout']),
+    ])
 
     if (pid) {
       this.logStart()

src/services/process-service.ts

@@ -33,12 +33,21 @@ export default class ProcessService {
   kill() {
     if (this.isRunning()) {
       const pid = this.getPid()
-      fs.unlinkSync(ProcessService.PID_PATH)
+      this.cleanup()
 
       process.kill(pid, 'SIGHUP')
     }
   }
 
+  cleanup() {
+    try {
+      fs.unlinkSync(ProcessService.PID_PATH)
+    } catch (e) {
+      // it's fine when the file doesn't exist, raise errors otherwise
+      if (e.code !== 'ENOENT') { throw e }
+    }
+  }
+
   private writePidFile(pid: number) {
     fs.writeFileSync(ProcessService.PID_PATH, pid)
   }