Activity notification intent test: name each intent by its activity, not by arrival order.

The mocha test "stores intent before dispatch, preserves pending failures
and suppresses deferred or disabled capture" failed about one run in four
with "notification hook did not settle". Traced with in-memory lease,
recovery and gate events: the two activities' insert hooks dispatch
concurrently, and when the second activity reached the subscriber first,
intentIds[0] held ITS intent - the one the test makes fail on purpose. The
test then waited for that intent to complete, which never happens, while
the one-second recovery scan kept retrying it. The application behaved as
designed; the test assumed an order the hooks never promised.

Intents are now recorded against their activity. The checks that counted
subscriber calls also allowed for the recovery scan, which legitimately
retries a pending intent on its own: the recipient is denied before the
replacement subscriber is installed, calls are compared with a snapshot,
the final retry waits out a scan that holds the lease at that instant, and
exactly one delivery is required whichever of the two made it.

The test passed 12 of 12 runs in isolation afterwards; it failed 1 of 4
before.

Thanks to xet7 !

server/lib/tests/activityNotificationIntent.tests.js

@@ -19,7 +19,7 @@ describe('Activity notification intent hooks', function () {
   this.timeout(15000);
   it('stores intent before dispatch, preserves pending failures and suppresses deferred or disabled capture', async function () {
     if (!Meteor.isAppTest) this.skip();
-    const ids = Array.from({ length: 4 }, () => Random.id()), intentIds = [];
+    const ids = Array.from({ length: 4 }, () => Random.id()), intentIds = [], intentOf = {};
     const flags = getFeatureFlags(), previousFlags = { ...flags };
     const oldRules = RulesHelper.executeRules, oldUsers = Notifications.getUsers, oldServices = { ...activityNotificationServices };
     let release;
@@ -40,7 +40,7 @@ describe('Activity notification intent hooks', function () {
         calls++;
         const row = await intents.findOne({ 'activity._id': params.activityId });
         assert.ok(row, 'intent exists before subscriber starts');
-        assert.equal(row.state, 'pending'); intentIds.push(row._id);
+        assert.equal(row.state, 'pending'); intentIds.push(row._id); intentOf[params.activityId] = row._id;
         assert.ok(await Activities.findOneAsync(params.activityId));
         await gate;
         if (params.activityId === ids[1]) throw new Error('subscriber write failed');
@@ -49,29 +49,50 @@ describe('Activity notification intent hooks', function () {
       const activity = id => ({ _id: id, activityType: 'createCard', cardId: Random.id() });
       await Activities.insertAsync(activity(ids[0]));
       await Activities.insertAsync(activity(ids[1]));
-      await until(() => calls === 2 && intentIds.length === 2);
-      assert.equal(await intents.countDocuments({ _id: { $in: intentIds }, state: 'pending' }), 2);
+      // The two hooks dispatch concurrently, so either may reach the subscriber
+      // first: name each intent by its activity, never by arrival order. The
+      // recovery scan (every second) also retries ids[1]'s failing delivery
+      // once the gate opens, so later checks must not count on `calls` alone.
+      await until(() => intentOf[ids[0]] && intentOf[ids[1]]);
+      const completedIntent = intentOf[ids[0]], failingIntent = intentOf[ids[1]];
+      assert.equal(await intents.countDocuments({ _id: { $in: [completedIntent, failingIntent] }, state: 'pending' }), 2);
       release();
-      await until(async () => (await intents.findOne({ _id: intentIds[0] }))?.state === 'completed');
-      assert.equal((await intents.findOne({ _id: intentIds[0] })).activity, undefined);
-      assert.equal((await intents.findOne({ _id: intentIds[1] })).state, 'pending');
-      await until(async () => !await ActivityNotificationLeases.rawCollection().findOne({ _id: intentIds[1] }));
-      const interrupted = await intents.findOne({ _id: intentIds[1] });
+      await until(async () => (await intents.findOne({ _id: completedIntent }))?.state === 'completed');
+      assert.equal((await intents.findOne({ _id: completedIntent })).activity, undefined);
+      assert.equal((await intents.findOne({ _id: failingIntent })).state, 'pending');
+      await until(async () => !await ActivityNotificationLeases.rawCollection().findOne({ _id: failingIntent }));
+      const interrupted = await intents.findOne({ _id: failingIntent });
       const savedPlan = await ActivityNotificationPlans.rawCollection().findOne({ 'plan.activityId': ids[1] });
       assert.equal(savedPlan.plan.recipients[0].email.html, 'saved');
+      // Deny the recipient BEFORE the replacement subscriber is installed: the
+      // recovery scan retries this pending intent on its own, and must not be
+      // able to complete it through the new subscriber in between.
+      await Meteor.users.rawCollection().updateOne({ _id: ids[0] }, { $set: { loginDisabled: true } });
       activityNotificationServices.prepareEmail = () => assert.fail('saved plan must not rerender');
       activityNotificationServices.prepareTray = () => assert.fail('saved plan must not reselect services');
       activityNotificationServices.email = async job => {
         calls++; assert.equal(job.html, 'saved'); return idFor(job.userId, job.eventId);
       };
-      await Meteor.users.rawCollection().updateOne({ _id: ids[0] }, { $set: { loginDisabled: true } });
       const retry = () => deliverStoredActivityNotifications(interrupted.activity, interrupted.dispatchUserId,
         () => assert.fail('saved plan must not reselect recipients'));
-      await assert.rejects(retry(), /recipient-denied/);
-      assert.equal(calls, 2);
+      // A scan holding the lease at that instant answers sync-busy; that is the
+      // lease working, so try again rather than fail on the timing.
+      const retryPastScan = async () => {
+        for (;;) {
+          try { return await retry(); } catch (error) {
+            if (error.code !== 'sync-busy') throw error;
+            await new Promise(resolve => setTimeout(resolve, 10));
+          }
+        }
+      };
+      const before = calls;
+      await assert.rejects(retryPastScan(), /recipient-denied/);
+      assert.equal(calls, before, 'a denied recipient receives nothing');
       await Meteor.users.rawCollection().updateOne({ _id: ids[0] }, { $set: { loginDisabled: false } });
-      await retry();
-      assert.equal((await intents.findOne({ _id: intentIds[1] })).state, 'completed');
+      await retryPastScan();
+      assert.equal((await intents.findOne({ _id: failingIntent })).state, 'completed');
+      // Exactly one delivery, whether this retry or a concurrent scan made it.
+      assert.equal(calls, before + 1);
 
       const saved = activity(ids[2]); saved.createdAt = new Date(1000); saved.modifiedAt = saved.createdAt;
       await withSyncActivityDeferred(saved, () => Activities.insertAsync(saved));
@@ -79,7 +100,7 @@ describe('Activity notification intent hooks', function () {
       flags.disableNotifications = true;
       await Activities.insertAsync(activity(ids[3]));
       assert.equal(await intents.countDocuments({ 'activity._id': ids[3] }), 0);
-      assert.equal(calls, 3);
+      assert.equal(calls, before + 1, 'deferred and disabled activities reach no subscriber');
     } finally {
       release();
       Notifications.getUsers = oldUsers; Object.assign(activityNotificationServices, oldServices);