Skip to content

Commit

Permalink
test: fix race condition if multiple invites are sent (#863)
Browse files Browse the repository at this point in the history
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]: #856
  • Loading branch information
EvanHahn committed Sep 26, 2024
1 parent ab66b21 commit dfaceb2
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 6 deletions.
10 changes: 8 additions & 2 deletions src/member-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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(
Expand All @@ -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)
Expand Down
15 changes: 11 additions & 4 deletions test-e2e/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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))
})
})()
)
}

Expand Down

0 comments on commit dfaceb2

Please sign in to comment.