Add timeout to gh merge queue query

Summary:
When checking for the merge queue support, we've seen cases of the command hanging. Let's just add a timeout to kill the spawned proccess. This makes sure we keep this reasonable.

I went with 10 seconds. It's a very simple query, so if it takes more than 1-2 seconds something is wrong.

Worst case scenario: the timeout hits when it shouldn't and the repo is detected as not supporting merge queue, which is a minor feature. It would still work otherwise.

Reviewed By: muirdm

Differential Revision: D76067308

fbshipit-source-id: ab14eecc86629f72273f58095661a0ece57c4c3e

Author: evangrayk

addons/isl-server/src/github/githubCodeReviewProvider.ts

@@ -90,6 +90,7 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider {
         const data = await this.query<MergeQueueSupportQueryData, MergeQueueSupportQueryVariables>(
           MergeQueueSupportQuery,
           {},
+          10_000,
         ).catch(err => {
           this.logger.info('failed to detect merge queue support', err);
           return undefined;
@@ -232,8 +233,8 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider {
     );
   }
 
-  private query<D, V>(query: string, variables: V): Promise<D | undefined> {
-    return queryGraphQL<D, V>(query, variables, this.codeReviewSystem.hostname);
+  private query<D, V>(query: string, variables: V, timeoutMs?: number): Promise<D | undefined> {
+    return queryGraphQL<D, V>(query, variables, this.codeReviewSystem.hostname, timeoutMs);
   }
 
   public dispose() {

addons/isl-server/src/github/queryGraphQL.ts

@@ -13,6 +13,7 @@ export default async function queryGraphQL<TData, TVariables>(
   query: string,
   variables: TVariables,
   hostname: string,
+  timeoutMs?: number,
 ): Promise<TData> {
   if (Object.prototype.hasOwnProperty.call(variables, 'query')) {
     throw Error('cannot have a variable named query');
@@ -38,12 +39,29 @@ export default async function queryGraphQL<TData, TVariables>(
   args.push('--hostname', hostname);
   args.push('-f', `query=${query}`);
 
+  let timedOut = false;
+
   try {
-    const {stdout} = await ejeca('gh', args, {
+    const proc = ejeca('gh', args, {
       env: {
         ...((await Internal.additionalGhEnvVars?.()) ?? {}),
       },
     });
+
+    // TODO: move this into ejeca itself
+    let timeoutId: NodeJS.Timeout | undefined;
+    if (timeoutMs != null && timeoutMs > 0) {
+      timeoutId = setTimeout(() => {
+        proc.kill('SIGTERM', {forceKillAfterTimeout: 5_000});
+        timedOut = true;
+      }, timeoutMs);
+      proc.on('exit', () => {
+        clearTimeout(timeoutId);
+      });
+    }
+
+    const {stdout} = await proc;
+
     const json = JSON.parse(stdout);
 
     if (Array.isArray(json.errors)) {
@@ -64,6 +82,9 @@ export default async function queryGraphQL<TData, TVariables>(
         throw new Error(`NotAuthenticatedError: ${(error as Error).stack}`);
       }
     }
+    if (timedOut) {
+      throw new Error(`TimedOutError: ${(error as Error).stack}`);
+    }
     throw error;
   }
 }