Start implementing a way to retry failed connections on startup

src/Bridge.ts

@@ -23,7 +23,10 @@
 } from "./config/Config";
 import { BridgeWidgetApi } from "./widgets/BridgeWidgetApi";
 import { CommentProcessor } from "./CommentProcessor";
-import { ConnectionManager } from "./ConnectionManager";
+import {
+  ConnectionCreationError,
+  ConnectionManager,
+} from "./ConnectionManager";
 import { GetIssueResponse, GetIssueOpts } from "./gitlab/Types";
 import { GithubInstance } from "./github/GithubInstance";
 import { IBridgeStorageProvider } from "./stores/StorageProvider";
@@ -1036,6 +1039,10 @@
         try {
           await connManager.createConnectionsForRoomId(roomId, false);
         } catch (ex) {
+          if (ex instanceof ConnectionCreationError && ex.retryable) {
+            // Failed to create connection, but the error was transiet and we should retry creating the room.
+            allRooms.push(roomId);
+          }
           log.error(`Unable to create connection for ${roomId}`, ex);
           continue;
         }
@@ -1482,7 +1489,7 @@
       );
       // This might be a reply to a notification
       try {
         const evContent = replyEvent.content as any;
         const splitParts: string[] =
           evContent["uk.half-shot.matrix-hookshot.github.repo"]?.name.split(
             "/",

src/ConnectionManager.ts

@@ -3,10 +3,10 @@
  */
 
 import {
+  APIRoomStateEvent,
   Appservice,
   Intent,
   MatrixError,
-  PowerLevelsEvent,
   StateEvent,
 } from "matrix-bot-sdk";
 import { ApiError, ErrCode } from "./api";
@@ -42,17 +42,33 @@
 import { MessageSenderClient } from "./MatrixSender";
 import { UserTokenStore } from "./tokens/UserTokenStore";
 import BotUsersManager, { BotUser } from "./managers/BotUsersManager";
-import { retry, retryMatrixErrorFilter } from "./PromiseUtil";
 import Metrics from "./Metrics";
 import EventEmitter from "events";
 import { HoundConnection } from "./Connections/HoundConnection";
 import { OpenProjectConnection } from "./Connections/OpenProjectConnection";
+import { GrantRejectedError } from "./grants/GrantCheck";
 
 const log = new Logger("ConnectionManager");
 
 const GET_STATE_ATTEMPTS = 5;
 const GET_STATE_TIMEOUT_MS = 1000;
 
+/**
+ * Represents a failure to create a connection.
+ */
+export class ConnectionCreationError extends Error {
+  constructor(
+    public readonly retryable: boolean,
+    inner: Error,
+    roomId: string,
+  ) {
+    super(
+      `Failed to create connections for ${roomId} (retryable: ${retryable})`,
+      { cause: inner },
+    );
+  }
+}
+
 export class ConnectionManager extends EventEmitter {
   private connections: IConnection[] = [];
   public readonly enabledForProvisioning: Record<
@@ -176,9 +192,8 @@
         `User ${state.sender} is disallowed to manage state for ${serviceType} in ${roomId}`,
       );
       return false;
-    } else {
-      return true;
     }
+    return false;
   }
 
   /**
@@ -271,7 +286,8 @@
   }
 
   /**
-   * This is called ONLY when we spot new state in a room and want to create a connection for it.
+   * This is called when we spot a new state even in a room and want to create a connection
+   * or called for each room we are joined to on startup.
    * @param roomId
    * @param state
    * @param rollbackBadState
@@ -311,7 +327,7 @@
       log.error(
         `Failed to find a bot in room '${roomId}' for service type '${connectionType.ServiceCategory}' when creating connection for state`,
       );
-      throw Error("Could not find a bot to handle this connection");
+      return;
     }
 
     if (
@@ -345,6 +361,13 @@
       await connection.ensureGrant?.(state.sender);
       return connection;
     } catch (ex) {
+      if (ex instanceof GrantRejectedError && ex.cause) {
+        // The grant was rejected BUT it had an inner cause, which means it was a networking failure.
+        throw new ConnectionCreationError(true, ex, roomId);
+      }
+      if (ex instanceof ConnectionCreationError) {
+        throw ex;
+      }
       log.error(
         `Not creating connection for state ${roomId}/${state.type}`,
         ex,
@@ -379,21 +402,30 @@
         "m.room.tombstone",
         "",
       );
+      log.debug(`Room ${roomId} has a tombstone, so not creating connection.`);
       return;
     } catch (ex) {
-      if (ex instanceof MatrixError === false || ex.errcode !== "M_NOT_FOUND") {
-        throw ex;
+      if (ex instanceof MatrixError) {
+        if (ex.errcode === "M_NOT_FOUND") {
+          // Good, that's what we want.
+        } else if (ex.errcode === "M_FORBIDDEN") {
+          // We're not in the room, surprising since we seem to have a bot?
+          throw new ConnectionCreationError(false, ex, roomId);
+        }
+      } else {
+        // All other errors are retryable, as far as we're concerned.
+        throw new ConnectionCreationError(true, ex, roomId);
       }
-      // No tombstone, so room is valid.
     }
 
-    // This endpoint can be heavy, wrap it in pillows.
-    const state = await retry(
-      () => botUser.intent.underlyingClient.getRoomState(roomId),
-      GET_STATE_ATTEMPTS,
-      GET_STATE_TIMEOUT_MS,
-      retryMatrixErrorFilter,
-    );
+    // Note: Fetching state is expensive, sometimes the response can be a 5XX.
+    let state: APIRoomStateEvent[];
+    try {
+      state = await botUser.intent.underlyingClient.getRoomState(roomId);
+    } catch (ex) {
+      // We've already firmly established we can access the room, so all other errors are probably server issues.
+      throw new ConnectionCreationError(true, ex, roomId);
+    }
 
     for (const event of state) {
       try {
@@ -407,7 +439,19 @@
           this.push(conn);
         }
       } catch (ex) {
-        log.error(`Failed to create connection for ${roomId}:`, ex);
+        // Passthrough any creation failures
+        if (ex instanceof ConnectionCreationError) {
+          log.warn(
+            `Room ${roomId} did not create connection ${event.type}, but will retry.`,
+            ex,
+          );
+          if (ex.retryable) {
+            // If we can retry creating this state connection, then throw now
+            // and the caller will reshedule
+            throw ex;
+          }
+        } // Else, just log and give up.
+        log.warn(`Room ${roomId} did not create connection ${event.type}`, ex);
       }
     }
   }

src/Connections/GenericHook.ts

@@ -9,7 +9,13 @@ import { Logger } from "matrix-appservice-bridge";
 import { MessageSenderClient } from "../MatrixSender";
 import markdownit from "markdown-it";
 import { MatrixEvent } from "../MatrixEvent";
-import { Appservice, Intent, StateEvent, UserID } from "matrix-bot-sdk";
+import {
+  Appservice,
+  Intent,
+  MatrixError,
+  StateEvent,
+  UserID,
+} from "matrix-bot-sdk";
 import { ApiError, ErrCode } from "../api";
 import { BaseConnection, removeConnectionState } from "./BaseConnection";
 import { BridgeConfigGenericWebhooks } from "../config/sections";
@@ -25,6 +31,7 @@ import {
   WebhookTransformer,
 } from "../generic/WebhookTransformer";
 import { GetConnectionsResponseItem } from "../widgets/Api";
+import { ConnectionCreationError } from "../ConnectionManager";
 
 export interface GenericHookConnectionState extends IConnectionState {
   /**
@@ -242,12 +249,19 @@ export class GenericHookConnection
       throw Error("Generic webhooks are not configured");
     }
     // Generic hooks store the hookId in the account data
-    const acctData =
-      await intent.underlyingClient.getSafeRoomAccountData<GenericHookAccountData>(
-        GenericHookConnection.CanonicalEventType,
-        roomId,
-        {},
-      );
+    let acctData: Record<string, string> = {};
+    try {
+      acctData =
+        await intent.underlyingClient.getRoomAccountData<GenericHookAccountData>(
+          GenericHookConnection.CanonicalEventType,
+          roomId,
+        );
+    } catch (ex) {
+      if (ex instanceof MatrixError && ex.errcode === "M_NOT_FOUND") {
+        acctData = {};
+      }
+      throw new ConnectionCreationError(true, ex, roomId);
+    }
     const state = this.validateState(event.content);
     // hookId => stateKey
     let hookId = Object.entries(acctData).find(
@@ -262,12 +276,16 @@ export class GenericHookConnection
       if (config.generic.requireExpiryTime && !state.expirationDate) {
         throw new Error("Expiration date must be set");
       }
-      await GenericHookConnection.ensureRoomAccountData(
-        roomId,
-        intent,
-        hookId,
-        event.stateKey,
-      );
+      try {
+        await GenericHookConnection.ensureRoomAccountData(
+          roomId,
+          intent,
+          hookId,
+          event.stateKey,
+        );
+      } catch (ex) {
+        throw new ConnectionCreationError(true, ex, roomId);
+      }
     }
 
     return new GenericHookConnection(

src/Errors.ts

@@ -33,3 +33,11 @@ export class TokenError extends Error {
     super(code);
   }
 }
+
+export class ConnectionConfigurationError extends Error {
+  constructor(connectionId: string, fieldName: string, message: string) {
+    super(
+      `Unable to create connection ${connectionId} due to invalid field ${fieldName}: ${message}`,
+    );
+  }
+}

src/grants/GrantCheck.ts

@@ -15,8 +15,11 @@ export class GrantRejectedError extends Error {
   constructor(
     public readonly roomId: string,
     public readonly connectionId: string,
+    public readonly innerError?: Error,
   ) {
-    super(`No grant exists for ${roomId}/${connectionId}. Rejecting`);
+    super(`No grant exists for ${roomId}/${connectionId}. Rejecting`, {
+      cause: innerError,
+    });
   }
 }
 
@@ -84,7 +87,7 @@ export class GrantChecker<cId extends ConnectionId = ConnectionId> {
         }
       } else {
         log.warn(`Failed to check grant in ${roomId}/${connectionId}`, ex);
-        throw new GrantRejectedError(roomId, connId);
+        throw new GrantRejectedError(roomId, connId, ex);
       }
     }
   }