From 1b03ac05158051c94f5ad87098054943fa43912b Mon Sep 17 00:00:00 2001
From: Evan Hahn <me@evanhahn.com>
Date: Mon, 23 Sep 2024 18:33:17 +0000
Subject: [PATCH] test: fix race condition if multiple invites are sent

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 75627555a..f359fb040 100644
--- a/src/member-api.js
+++ b/src/member-api.js
@@ -89,6 +89,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 |
@@ -97,7 +98,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(
@@ -120,7 +126,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 5c8e40ec9..3102a32c9 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))
-      })
+      })()
     )
   }