Have asynchronous execution so that the bot stays responsive for new requests while executing commands

bench.js

@@ -1,54 +1,13 @@
-const cp = require("child_process")
 const path = require("path")
 const fs = require("fs")
+const Mutex = require("async-mutex").Mutex
+const libCollector = require("./collector")
 
 function errorResult(message, error) {
   return { isError: true, message, error }
 }
 
-let cwd = process.cwd()
-
-const Mutex = require("async-mutex").Mutex
 const mutex = new Mutex()
-var shell = require("shelljs")
-
-var libCollector = require("./collector")
-
-function BenchContext(app, config) {
-  var self = this
-  self.app = app
-  self.config = config
-
-  self.runTask = function (cmd, title) {
-    let stdout = "",
-      stderr = "",
-      error = true
-
-    try {
-      if (title) {
-        app.log({ title, msg: `Running task on directory ${process.cwd()}` })
-      }
-      // We prefer to run the command in a synchronously so that there's less
-      // risk of having the Node.js process interfere or deprioritize the
-      // process' execution.
-      // Previously we've used cp.spawn for capturing the processes' streams
-      // but, again, having it execute directly in the shell reduces the
-      // likelihood of friction or overhead due to Node.js APIs.
-      const result = shell.exec(cmd, { silent: false })
-      stderr = result.stderr
-      error = result.code !== 0
-      stdout = result.stdout
-    } catch (err) {
-      error = true
-      app.log.fatal({
-        msg: "Caught exception in command execution",
-        error: err,
-      })
-    }
-
-    return { stdout, stderr, error }
-  }
-}
 
 //::node::import::native::sr25519::transfer_keep_alive::paritydb::small
 
@@ -87,54 +46,60 @@ var BenchConfigs = {
 
 const prepareBranch = async function (
   { contributor, owner, repo, branch, baseBranch, getPushDomain },
-  { benchContext },
+  { runner },
 ) {
-  const gitDirectory = path.join(cwd, "git")
-  shell.mkdir(gitDirectory)
-
+  const gitDirectory = path.join(__dirname, "git")
   const repositoryPath = path.join(gitDirectory, repo)
+
   var { url } = await getPushDomain()
-  benchContext.runTask(`git clone ${url}/${owner}/${repo} ${repositoryPath}`)
-  shell.cd(repositoryPath)
+  await runner.run(
+    `mkdir -p ${gitDirectory}; git clone ${url}/${owner}/${repo} ${repositoryPath}`,
+  )
+
+  try {
+    process.chdir(repositoryPath)
+  } catch (error) {
+    return errorResult(`Failed to enter directory ${repositoryPath}`, error)
+  }
 
-  var { error } = benchContext.runTask("git add . && git reset --hard HEAD")
+  var { error, stderr } = await runner.run("git add . && git reset --hard HEAD")
   if (error) return errorResult(stderr)
 
-  var { error, stdout } = benchContext.runTask("git rev-parse HEAD")
+  var { error, stdout, stderr } = await runner.run("git rev-parse HEAD")
   if (error) return errorResult(stderr)
   const detachedHead = stdout.trim()
 
   // Check out to the detached head so that any branch can be deleted
-  var { error, stderr } = benchContext.runTask(`git checkout ${detachedHead}`)
+  var { error, stderr } = await runner.run(`git checkout ${detachedHead}`)
   if (error) return errorResult(stderr)
 
   // Recreate PR remote
-  benchContext.runTask("git remote remove pr")
+  await runner.run("git remote remove pr")
   var { url } = await getPushDomain()
-  var { error, stderr } = benchContext.runTask(
+  var { error, stderr } = await runner.run(
     `git remote add pr ${url}/${contributor}/${repo}.git`,
   )
   if (error)
     return errorResult(`Failed to add remote reference to ${owner}/${repo}`)
 
   // Fetch and recreate the PR's branch
-  benchContext.runTask(`git branch -D ${branch}`)
-  var { error, stderr } = benchContext.runTask(
+  await runner.run(`git branch -D ${branch}`)
+  var { error, stderr } = await runner.run(
     `git fetch pr ${branch} && git checkout --track pr/${branch}`,
     `Checking out ${branch}...`,
   )
   if (error) return errorResult(stderr)
 
   // Fetch and merge master
-  var { error, stderr } = benchContext.runTask(
+  var { error, stderr } = await runner.run(
     `git pull origin ${baseBranch}`,
     `Merging branch ${baseBranch}`,
   )
   if (error) return errorResult(stderr)
 }
 
-function benchBranch(app, config) {
-  app.log("Waiting our turn to run benchBranch...")
+function benchBranch(runner, config) {
+  runner.log("Waiting our turn to run benchBranch...")
 
   return mutex.runExclusive(async function () {
     try {
@@ -149,14 +114,13 @@ function benchBranch(app, config) {
       }
 
       const collector = new libCollector.Collector()
-      var benchContext = new BenchContext(app, config)
       var { title, benchCommand } = benchConfig
-      app.log(`Started benchmark "${title}."`)
+      runner.log(`Started benchmark "${title}."`)
 
-      var error = await prepareBranch(config, { benchContext })
+      var error = await prepareBranch(config, { runner })
       if (error) return error
 
-      var { stderr, error, stdout } = benchContext.runTask(
+      var { stderr, error, stdout } = await runner.run(
         benchCommand,
         `Benching branch ${config.branch}...`,
       )
@@ -423,8 +387,8 @@ function checkAllowedCharacters(command) {
   return true
 }
 
-function benchmarkRuntime(app, config) {
-  app.log("Waiting our turn to run benchmarkRuntime...")
+function benchmarkRuntime(runner, config) {
+  runner.log("Waiting our turn to run benchmarkRuntime...")
 
   return mutex.runExclusive(async function () {
     try {
@@ -477,76 +441,73 @@ function benchmarkRuntime(app, config) {
         return errorResult(`Missing required flags: ${missing.toString()}`)
       }
 
-      var benchContext = new BenchContext(app, config)
       var { title } = benchConfig
-      app.log(
-        `Started ${config.id} benchmark "${title}." (command: ${benchCommand})`,
+      runner.log(
+        `Started ${config.id} benchmark "${title}" (command: ${benchCommand})`,
       )
 
-      var error = await prepareBranch(config, { benchContext })
+      var error = await prepareBranch(config, { runner })
       if (error) return error
 
       const outputFile = benchCommand.match(/--output(?:=|\s+)(".+?"|\S+)/)[1]
-      var { stdout, stderr } = benchContext.runTask(
+      var { stdout, stderr, error } = await runner.run(
         benchCommand,
         `Running for branch ${config.branch}, ${
           outputFile ? `outputFile: ${outputFile}` : ""
         }: ${benchCommand}`,
       )
+      if (error) {
+        return errorResult(stderr)
+      }
+
       let extraInfo = ""
 
-      var { stdout: gitStatus, stderr: gitStatusError } =
-        benchContext.runTask("git status --short")
-      app.log(`Git status after execution: ${gitStatus || gitStatusError}`)
+      var { stdout: gitStatus, stderr: gitStatusError } = await runner.run(
+        "git status --short",
+      )
+      runner.log(`Git status after execution: ${gitStatus || gitStatusError}`)
 
       if (outputFile) {
         if (process.env.DEBUG) {
-          app.log({
+          runner.log({
             context: "Output file",
             msg: fs.readFileSync(outputFile).toString(),
           })
         } else {
           try {
-            var last = benchContext.runTask(
+            var last = await runner.run(
               `git add ${outputFile} && git commit -m "${benchCommand}"`,
             )
             if (last.error) {
               extraInfo = `ERROR: Unable to commit file ${outputFile}`
-              app.log.fatal({
-                msg: extraInfo,
-                stdout: last.stdout,
-                stderr: last.stderr,
-              })
+              runner.logFatalError(
+                { stdout: last.stdout, stderr: last.stderr },
+                extraInfo,
+              )
             } else {
               const target = `${config.contributor}/${config.repo}`
               const { url, token } = await config.getPushDomain()
-              var last = benchContext.runTask(
+              var last = await runner.run(
                 `git remote set-url pr ${url}/${target}.git && git push pr HEAD`,
                 `Pushing ${outputFile} to ${config.branch}`,
               )
               if (last.error) {
                 extraInfo = `ERROR: Unable to push ${outputFile}`
-                app.log.fatal({
-                  msg: extraInfo,
-                  stdout: last.stdout,
-                  stderr: last.stderr,
-                })
+                runner.logFatalError(
+                  { stdout: last.stdout, stderr: last.stderr },
+                  extraInfo,
+                )
               }
             }
           } catch (error) {
             extraInfo =
               "NOTE: Caught exception while trying to push commits to the repository"
-            app.log.fatal({ msg: extraInfo, error })
+            runner.logFatalError(error, extraInfo)
           }
         }
       }
 
-      return {
-        title,
-        output: stdout ? stdout : stderr,
-        extraInfo,
-        benchCommand,
-      }
+      return { title, output: stdout || stderr, extraInfo, benchCommand }
     } catch (error) {
       return errorResult("Caught exception in benchmarkRuntime", error)
     }

index.js

@@ -1,7 +1,8 @@
 const { createAppAuth } = require("@octokit/auth-app")
 const assert = require("assert")
 const fs = require("fs")
-const shell = require("shelljs")
+const path = require("path")
+const { Runner } = require("./runner")
 
 var { benchBranch, benchmarkRuntime } = require("./bench")
 
@@ -31,14 +32,16 @@ for (const event of ["uncaughtException", "unhandledRejection"]) {
 }
 
 module.exports = (app) => {
+  const runner = new Runner(app)
+
+  appFatalLogger = runner.log.fatal
+
   if (process.env.DEBUG) {
-    app.log("Running in debug mode")
+    runner.log("Running in debug mode")
   }
 
-  appFatalLogger = app.log.fatal
-
   const baseBranch = process.env.BASE_BRANCH || "master"
-  app.log.debug(`base branch: ${baseBranch}`)
+  runner.log.debug(`base branch: ${baseBranch}`)
 
   const appId = parseInt(process.env.APP_ID)
   assert(appId)
@@ -102,18 +105,19 @@ module.exports = (app) => {
       let pr = await context.octokit.pulls.get({ owner, repo, pull_number })
       const contributor = pr.data.head.user.login
       const branch = pr.data.head.ref
-      app.log.debug(`branch: ${branch}`)
-
-      var { stdout: toolchain, code: toolchainError } = shell.exec(
-        "rustup show active-toolchain --verbose",
-        { silent: false },
-      )
-      if (toolchainError) {
-        await context.octokit.issues.createComment(
-          context.issue({
-            body: "ERROR: Failed to query the currently active Rust toolchain",
-          }),
-        )
+      runner.log.debug(`branch: ${branch}`)
+
+      var result = await runner.run("rustup show active-toolchain --verbose")
+      var { stdout: toolchain, error, stderr } = result
+      if (error) {
+        const msg = "ERROR: Failed to query the currently active Rust toolchain"
+        if (process.env.DEBUG) {
+          runner.logFatalError({ stderr, toolchain }, msg)
+        } else {
+          await context.octokit.issues.createComment(
+            context.issue({ body: msg }),
+          )
+        }
         return
       } else {
         toolchain = toolchain.trim()
@@ -122,7 +126,7 @@ module.exports = (app) => {
       const initialInfo = `Starting benchmark for branch: ${branch} (vs ${baseBranch})\n\nToolchain: \n${toolchain}\n\n Comment will be updated.`
       let comment_id = undefined
       if (process.env.DEBUG) {
-        app.log(initialInfo)
+        runner.log(initialInfo)
       } else {
         const issueComment = context.issue({ body: initialInfo })
         const issue_comment = await context.octokit.issues.createComment(
@@ -144,20 +148,20 @@ module.exports = (app) => {
 
       let report
       if (action == "runtime" || action == "xcm") {
-        report = await benchmarkRuntime(app, config)
+        report = await benchmarkRuntime(runner, config)
       } else {
-        report = await benchBranch(app, config)
+        report = await benchBranch(runner, config)
       }
       if (process.env.DEBUG) {
         console.log(report)
         return
       }
 
       if (report.isError) {
-        app.log.error(report.message)
+        runner.log.error(report.message)
 
         if (report.error) {
-          app.log.error(report.error)
+          runner.log.error(report.error)
         }
 
         const output = `${report.message}${
@@ -218,18 +222,25 @@ ${extraInfo}
         body,
       })
     } catch (error) {
-      app.log.fatal({
-        error,
-        repo,
-        owner,
-        pull_number,
+      runner.logFatalError(error, {
         msg: "Caught exception in issue_comment's handler",
+        payload: context.payload,
       })
-      await context.octokit.issues.createComment(
-        context.issue({
-          body: `Exception caught: \`${error.message}\`\n${error.stack}`,
-        }),
-      )
+      if (!process.env.DEBUG) {
+        await context.octokit.issues.createComment(
+          context.issue({
+            body: `Exception caught: \`${error.message}\`\n${error.stack}`,
+          }),
+        )
+      }
     }
   })
+
+  if (fs.existsSync(path.join(__dirname, "payload.json"))) {
+    app.receive(
+      JSON.parse(
+        fs.readFileSync(path.join(__dirname, "payload.json")).toString(),
+      ),
+    )
+  }
 }