fix: don't delete project info on initial sync fail (#1055)

* fix: don't delete project info on initial sync fail

* fix: invitee should set deviceInfo

* chore: remove dead code

* fix: Add timeout to inviter initial sync

* chore: clean up try catch for setting up project on invitee

* chore: run formatter

* chore: Change name / approach for initial sync timeout

* fix force close of peer connections

The previous behaviour when stopping LocalDiscovery server with force: true (which is used in tests to disconnect peers) was to disconnect after a timeout of 1ms. This was causing a race condition in this test https://github.com/digidem/comapeo-core/blob/f7e59063deb32fef129eb2a6a06ec9ac8b28e4c3/test-e2e/manager-invite.js#L668 because the timers were being run before abort signals had been created. On my macbook this test was failing randomly about 75% of the time.

This change results in calling localDiscovery.stop({ force: true }) immediately disconnecting sockets in the same tick.

* fix: catch disconnect errors for tests

---------

Co-authored-by: Mauve Signweaver <[email protected]>
Co-authored-by: Gregor MacLennan <[email protected]>

Author: 3 people

src/discovery/local-discovery.js

@@ -276,15 +276,25 @@ export class LocalDiscovery extends TypedEmitter {
     const { port } = getAddress(this.#server)
     this.#server.close()
     const closePromise = once(this.#server, 'close')
-    await pTimeout(closePromise, {
-      milliseconds: force ? (timeout === 0 ? 1 : timeout) : Infinity,
-      fallback: () => {
-        for (const socket of this.#noiseConnections.values()) {
-          socket.destroy()
-        }
-        return pTimeout(closePromise, { milliseconds: 500 })
-      },
-    })
+
+    const forceClose = () => {
+      for (const socket of this.#noiseConnections.values()) {
+        socket.destroy()
+      }
+      return pTimeout(closePromise, { milliseconds: 500 })
+    }
+
+    if (!force) {
+      await closePromise
+    } else if (timeout === 0) {
+      // If timeout is 0, we force-close immediately
+      await forceClose()
+    } else {
+      await pTimeout(closePromise, {
+        milliseconds: timeout,
+        fallback: forceClose,
+      })
+    }
     this.#log(`stopped for ${port}`)
   }
 }

src/errors.js

@@ -37,14 +37,6 @@ export class ProjectDetailsSendFailError extends Error {
   }
 }
 
-export class InviteInitialSyncFailError extends Error {
-  /** @param {string} [message] */
-  constructor(message = 'Failed to sync config for invite') {
-    super(message)
-    this.name = 'InviteInitialSyncFailError'
-  }
-}
-
 export class RPCDisconnectBeforeSendingError extends Error {
   /** @param {string} [message] */
   constructor(message = 'RPC disconnected before sending') {

src/mapeo-manager.js

@@ -643,23 +643,9 @@ export class MapeoManager extends TypedEmitter {
 
     // Any errors from here we need to remove project from db because it has not
     // been fully added and synced
+    let project = null
     try {
-      // 4. Write device info into project
-      const project = await this.getProject(projectPublicId)
-
-      try {
-        const deviceInfo = this.getDeviceInfo()
-        if (hasSavedDeviceInfo(deviceInfo)) {
-          await project[kSetOwnDeviceInfo](deviceInfo)
-        }
-      } catch (e) {
-        // Can ignore an error trying to write device info
-        this.#l.log(
-          'ERROR: failed to write project %h deviceInfo %o',
-          projectKey,
-          e
-        )
-      }
+      project = await this.getProject(projectPublicId)
 
       /** @type {import('drizzle-orm').InferInsertModel<typeof projectSettingsTable>} */
       const settingsDoc = {
@@ -677,21 +663,39 @@ export class MapeoManager extends TypedEmitter {
       }
 
       await this.#db.insert(projectSettingsTable).values([settingsDoc])
-
-      // 5. Wait for initial project sync
-      if (waitForSync) {
-        await this.#waitForInitialSync(project)
-      }
-
       this.#activeProjects.set(projectPublicId, project)
     } catch (e) {
+      // Only happens if getProject or the the DB insert fails
       this.#l.log('ERROR: could not add project', e)
       this.#db
         .delete(projectKeysTable)
         .where(eq(projectKeysTable.projectId, projectId))
         .run()
       throw e
     }
+
+    try {
+      const deviceInfo = this.getDeviceInfo()
+      if (hasSavedDeviceInfo(deviceInfo)) {
+        await project[kSetOwnDeviceInfo](deviceInfo)
+      }
+    } catch (e) {
+      // Can ignore an error trying to write device info
+      this.#l.log(
+        'ERROR: failed to write project %h deviceInfo %o',
+        projectKey,
+        e
+      )
+    }
+
+    // 5. Wait for initial project sync
+    if (waitForSync) {
+      try {
+        await this.#waitForInitialSync(project)
+      } catch (e) {
+        this.#l.log('ERROR: could not do initial project sync', e)
+      }
+    }
     this.#l.log('Added project %h, public ID: %S', projectKey, projectPublicId)
     return projectPublicId
   }

src/mapeo-project.js

@@ -376,6 +376,7 @@ export class MapeoProject extends TypedEmitter {
         deviceInfo: this.#dataTypes.deviceInfo,
         project: this.#dataTypes.projectSettings,
       },
+      logger: this.#l,
     })
 
     this.#blobStore = new BlobStore({

src/member-api.js

@@ -12,20 +12,16 @@ import {
   projectKeyToProjectInviteId,
   projectKeyToPublicId,
 } from './utils.js'
+import { Logger } from './logger.js'
 import { keyBy } from './lib/key-by.js'
 import { abortSignalAny } from './lib/ponyfills.js'
 import timingSafeEqual from 'string-timing-safe-equal'
 import { isHostnameIpAddress } from './lib/is-hostname-ip-address.js'
 import { ErrorWithCode, getErrorMessage } from './lib/error.js'
-import {
-  InviteAbortedError,
-  InviteInitialSyncFailError,
-  ProjectDetailsSendFailError,
-} from './errors.js'
+import { InviteAbortedError, ProjectDetailsSendFailError } from './errors.js'
 import { wsCoreReplicator } from './lib/ws-core-replicator.js'
 import {
   BLOCKED_ROLE_ID,
-  FAILED_ROLE_ID,
   MEMBER_ROLE_ID,
   ROLES,
   isRoleIdForNewInvite,
@@ -71,6 +67,7 @@ export class MemberApi extends TypedEmitter {
   #getReplicationStream
   #waitForInitialSyncWithPeer
   #dataTypes
+  #l
 
   /** @type {Map<string, { abortController: AbortController }>} */
   #outboundInvitesByDevice = new Map()
@@ -90,6 +87,7 @@ export class MemberApi extends TypedEmitter {
    * @param {Object} opts.dataTypes
    * @param {Pick<DeviceInfoDataType, 'getByDocId' | 'getMany'>} opts.dataTypes.deviceInfo
    * @param {Pick<ProjectDataType, 'getByDocId'>} opts.dataTypes.project
+   * @param {Logger} [opts.logger]
    */
   constructor({
     deviceId,
@@ -103,8 +101,10 @@ export class MemberApi extends TypedEmitter {
     getReplicationStream,
     waitForInitialSyncWithPeer,
     dataTypes,
+    logger,
   }) {
     super()
+    this.#l = Logger.create('member-api', logger)
     this.#ownDeviceId = deviceId
     this.#roles = roles
     this.#coreOwnership = coreOwnership
@@ -128,6 +128,7 @@ export class MemberApi extends TypedEmitter {
    * @param {string} [opts.roleName]
    * @param {string} [opts.roleDescription]
    * @param {Buffer} [opts.__testOnlyInviteId] Hard-code the invite ID. Only for tests.
+   * @param {number} [opts.initialSyncTimeoutMs=5000]
    * @returns {Promise<(
    *   typeof InviteResponse_Decision.ACCEPT |
    *   typeof InviteResponse_Decision.REJECT |
@@ -141,6 +142,7 @@ export class MemberApi extends TypedEmitter {
       roleName = ROLES[roleId]?.name,
       roleDescription,
       __testOnlyInviteId,
+      initialSyncTimeoutMs = 5000,
     }
   ) {
     assert(isRoleIdForNewInvite(roleId), 'Invalid role ID for new invite')
@@ -205,38 +207,37 @@ export class MemberApi extends TypedEmitter {
         case InviteResponse_Decision.DECISION_UNSPECIFIED:
           return InviteResponse_Decision.REJECT
         case InviteResponse_Decision.ACCEPT:
-          // Technically we can assign after sending project details
-          // This lets us test role removal
-          await this.#roles.assignRole(deviceId, roleId)
-
           try {
             await this.#rpc.sendProjectJoinDetails(deviceId, {
               inviteId,
               projectKey: this.#projectKey,
               encryptionKeys: this.#encryptionKeys,
             })
           } catch {
-            try {
-              // Mark them as "failed" so we can retry the flow
-              await this.#roles.assignRole(deviceId, FAILED_ROLE_ID)
-            } catch (e) {
-              console.error(e)
-            }
             throw new ProjectDetailsSendFailError()
           }
+          await this.#roles.assignRole(deviceId, roleId)
 
           try {
-            await this.#waitForInitialSyncWithPeer(deviceId, abortSignal)
-          } catch {
-            // Mark them as "failed" so we can retry the flow
-            await this.#roles.assignRole(deviceId, FAILED_ROLE_ID)
-            throw new InviteInitialSyncFailError()
+            let abortSync = new AbortController().signal
+            if (initialSyncTimeoutMs) {
+              abortSync = AbortSignal.timeout(initialSyncTimeoutMs)
+            }
+
+            await this.#waitForInitialSyncWithPeer(deviceId, abortSync)
+          } catch (e) {
+            this.#l.log('ERROR: Could not initial sync with peer', e)
           }
 
           return inviteResponse.decision
         default:
           throw new ExhaustivenessError(inviteResponse.decision)
       }
+    } catch (err) {
+      if (err instanceof Error && err.name === 'RPCDisconnectBeforeAckError') {
+        throw new InviteAbortedError()
+      }
+      throw err
     } finally {
       this.#outboundInvitesByDevice.delete(deviceId)
     }

src/roles.js

@@ -12,7 +12,6 @@ export const COORDINATOR_ROLE_ID = 'f7c150f5a3a9a855'
 export const MEMBER_ROLE_ID = '012fd2d431c0bf60'
 export const BLOCKED_ROLE_ID = '9e6d29263cba36c9'
 export const LEFT_ROLE_ID = '8ced989b1904606b'
-export const FAILED_ROLE_ID = 'a24eaca65ab5d5d0'
 export const NO_ROLE_ID = '08e4251e36f6e7ed'
 
 /**
@@ -28,7 +27,6 @@ const ROLE_IDS = new Set(
     MEMBER_ROLE_ID,
     BLOCKED_ROLE_ID,
     LEFT_ROLE_ID,
-    FAILED_ROLE_ID,
     NO_ROLE_ID,
   ])
 )
@@ -53,7 +51,6 @@ const ROLE_IDS_ASSIGNABLE_TO_ANYONE = new Set(
     MEMBER_ROLE_ID,
     BLOCKED_ROLE_ID,
     LEFT_ROLE_ID,
-    FAILED_ROLE_ID,
   ])
 )
 const isRoleIdAssignableToAnyone = setHas(ROLE_IDS_ASSIGNABLE_TO_ANYONE)
@@ -129,34 +126,6 @@ const BLOCKED_ROLE = {
   },
 }
 
-/**
- * This role is used for devices that failed to sync after accepting an invite and being added
- * @type {Role<typeof FAILED_ROLE_ID>}
- */
-const FAILED_ROLE = {
-  roleId: FAILED_ROLE_ID,
-  name: 'Failed',
-  docs: mapObject(currentSchemaVersions, (key) => {
-    return [
-      key,
-      {
-        readOwn: false,
-        writeOwn: false,
-        readOthers: false,
-        writeOthers: false,
-      },
-    ]
-  }),
-  roleAssignment: [],
-  sync: {
-    auth: 'blocked',
-    config: 'blocked',
-    data: 'blocked',
-    blobIndex: 'blocked',
-    blob: 'blocked',
-  },
-}
-
 /**
  * This is the role assumed for a device when no role record can be found. This
  * can happen when an invited device did not manage to sync with the device that
@@ -250,7 +219,6 @@ export const ROLES = {
     },
   },
   [NO_ROLE_ID]: NO_ROLE,
-  [FAILED_ROLE_ID]: FAILED_ROLE,
 }
 
 /**
@@ -412,11 +380,6 @@ export class Roles extends TypedEmitter {
       if (deviceId !== this.#ownDeviceId) {
         throw new Error('Cannot assign LEFT role to another device')
       }
-    } else if (roleId === FAILED_ROLE_ID) {
-      const ownRole = await this.getRole(this.#ownDeviceId)
-      if (!ownRole.roleAssignment.includes(COORDINATOR_ROLE_ID)) {
-        throw new Error('Lacks permission to assign role ' + roleId)
-      }
     } else {
       const ownRole = await this.getRole(this.#ownDeviceId)
       if (!ownRole.roleAssignment.includes(roleId)) {

test-e2e/sync.js

@@ -901,6 +901,7 @@ test('no sync capabilities === no namespaces sync apart from auth', async (t) =>
   const disconnect1 = connectPeers(managers)
 
   const projectId = await invitor.createProject({ name: 'Mapeo' })
+
   await invite({
     invitor,
     invitees: [blocked],