From dfaceb29186c695b04f037ffa31321c7588d1270 Mon Sep 17 00:00:00 2001
From: Evan Hahn <me@evanhahn.com>
Date: Thu, 26 Sep 2024 11:57:21 -0500
Subject: [PATCH] test: fix race condition if multiple invites are sent (#863)

We have a test helper, `invite`, that invites people to projects. It
works like this:

1. Tell the invitee(s) to accept any invite they receive.
2. Send the invite.

This is fine for most of our tests, where only one invite will be sent
to each manager. But we have [at least one test][0] where multiple
invites are sent.

That can cause a race condition. Imagine the following scenario where
`invite` is called twice:

1. Tell the invitee to accept any invite they receive (listener 1).
2. Tell the invitee to accept any invite they receive (listener 2).
3. Send the first invite.
4. Both listeners fire, sending one valid "accept" response and one that
   gets dropped.
4. Send the second invite. There are no listeners, and the test hangs.

This commit changes `invite`. Now, it works like this:

1. Generate an invite ID.
2. Tell the invitee(s) to accept any invite with that ID.
3. Send the invite.

As far as I know, this isn't a problem today. But an [upcoming test
change][1] causes this to happen much more reliably, so I think this is
worth fixing. (I think it's worth fixing even without that upcoming
change. Also, I tested this with those changes and it does, indeed, fix
hung tests.)

[0]: https://github.com/digidem/comapeo-core/blob/d2e5590aa749b690e5c07c3b64791db5e403ee29/test-e2e/sync.js#L530
[1]: https://github.com/digidem/comapeo-core/pull/856
---
 src/member-api.js | 10 ++++++++--
 test-e2e/utils.js | 15 +++++++++++----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/member-api.js b/src/member-api.js
index 98706b6f..e6cea8cc 100644
--- a/src/member-api.js
+++ b/src/member-api.js
@@ -90,6 +90,7 @@ export class MemberApi extends TypedEmitter {
    * @param {import('./roles.js').RoleIdForNewInvite} opts.roleId
    * @param {string} [opts.roleName]
    * @param {string} [opts.roleDescription]
+   * @param {Buffer} [opts.__testOnlyInviteId] Hard-code the invite ID. Only for tests.
    * @returns {Promise<(
    *   typeof InviteResponse_Decision.ACCEPT |
    *   typeof InviteResponse_Decision.REJECT |
@@ -98,7 +99,12 @@ export class MemberApi extends TypedEmitter {
    */
   async invite(
     deviceId,
-    { roleId, roleName = ROLES[roleId]?.name, roleDescription }
+    {
+      roleId,
+      roleName = ROLES[roleId]?.name,
+      roleDescription,
+      __testOnlyInviteId,
+    }
   ) {
     assert(isRoleIdForNewInvite(roleId), 'Invalid role ID for new invite')
     assert(
@@ -121,7 +127,7 @@ export class MemberApi extends TypedEmitter {
 
       abortSignal.throwIfAborted()
 
-      const inviteId = crypto.randomBytes(32)
+      const inviteId = __testOnlyInviteId || crypto.randomBytes(32)
       const projectId = projectKeyToId(this.#projectKey)
       const projectInviteId = projectKeyToProjectInviteId(this.#projectKey)
       const project = await this.#dataTypes.project.getByDocId(projectId)
diff --git a/test-e2e/utils.js b/test-e2e/utils.js
index 99c48e03..e5695537 100644
--- a/test-e2e/utils.js
+++ b/test-e2e/utils.js
@@ -7,13 +7,13 @@ import { fork } from 'node:child_process'
 import { createRequire } from 'node:module'
 import { fileURLToPath } from 'node:url'
 import * as v8 from 'node:v8'
+import { pEvent } from 'p-event'
 
 import { MapeoManager, roles } from '../src/index.js'
 import { kManagerReplicate, kRPC } from '../src/mapeo-manager.js'
-import { once } from 'node:events'
 import { generate } from '@mapeo/mock-data'
 import { valueOf } from '../src/utils.js'
-import { randomInt } from 'node:crypto'
+import { randomBytes, randomInt } from 'node:crypto'
 import { temporaryFile, temporaryDirectory } from 'tempy'
 import fsPromises from 'node:fs/promises'
 import { kSyncState } from '../src/sync/sync-api.js'
@@ -100,18 +100,25 @@ export async function invite({
   const promises = []
 
   for (const invitee of invitees) {
+    const inviteId = randomBytes(32)
     promises.push(
       invitorProject.$member.invite(invitee.deviceId, {
         roleId,
         roleName,
+        __testOnlyInviteId: inviteId,
       })
     )
     promises.push(
-      once(invitee.invite, 'invite-received').then(async ([invite]) => {
+      (async () => {
+        const invite = await pEvent(
+          invitee.invite,
+          'invite-received',
+          (invite) => Buffer.from(invite.inviteId, 'hex').equals(inviteId)
+        )
         await (reject
           ? invitee.invite.reject(invite)
           : invitee.invite.accept(invite))
-      })
+      })()
     )
   }