From 837597cd558ef815035b8119b05df9eef1db0d42 Mon Sep 17 00:00:00 2001
From: Florent <florent.git@zeteo.me>
Date: Wed, 3 Jan 2024 20:47:53 +0100
Subject: [PATCH] Fix deadlock with webhooks on document load #799 (#812)

---
 app/server/lib/Sharing.ts | 14 ++++++++++----
 test/server/lib/DocApi.ts | 31 +++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts
index 77743e63..d9936264 100644
--- a/app/server/lib/Sharing.ts
+++ b/app/server/lib/Sharing.ts
@@ -22,6 +22,7 @@ import {ActionHistory, asActionGroup, getActionUndoInfo} from './ActionHistory';
 import {ActiveDoc} from './ActiveDoc';
 import {makeExceptionalDocSession, OptDocSession} from './DocSession';
 import {WorkCoordinator} from './WorkCoordinator';
+import {summarizeAction} from 'app/common/ActionSummarizer';
 
 // Describes the request to apply a UserActionBundle. It includes a Client (so that broadcast
 // message can set `.fromSelf` property), and methods to resolve or reject the promise for when
@@ -246,14 +247,14 @@ export class Sharing {
 
     try {
 
-      const isCalculate = (userActions.length === 1 && SYSTEM_ACTIONS.has(userActions[0][0] as string));
+      const isSystemAction = (userActions.length === 1 && SYSTEM_ACTIONS.has(userActions[0][0] as string));
       // `internal` is true if users shouldn't be able to undo the actions. Applies to:
       // - Calculate/UpdateCurrentTime because it's not considered as performed by a particular client.
       // - Adding attachment metadata when uploading attachments,
       //   because then the attachment file may get hard-deleted and redo won't work properly.
       // - Action was rejected but it had some side effects (e.g. NOW() or UUID() formulas).
       const internal =
-        isCalculate ||
+        isSystemAction ||
         userActions.every(a => a[0] === "AddRecord" && a[1] === "_grist_Attachments") ||
         !!failure;
 
@@ -305,7 +306,7 @@ export class Sharing {
 
       // If the document has shut down in the meantime, and this was just a "Calculate" action,
       // return a trivial result.  This is just to reduce noisy warnings in migration tests.
-      if (this._activeDoc.isShuttingDown && isCalculate) {
+      if (this._activeDoc.isShuttingDown && isSystemAction) {
         return {
           actionNum: localActionBundle.actionNum,
           retValues: [],
@@ -336,7 +337,12 @@ export class Sharing {
       }
       await this._activeDoc.processActionBundle(ownActionBundle);
 
-      const actionSummary = await this._activeDoc.handleTriggers(localActionBundle);
+      // Don't trigger webhooks for single Calculate actions, this causes a deadlock on document load.
+      // See gh issue #799
+      const isSingleCalculateAction = userActions.length === 1 && userActions[0][0] === 'Calculate';
+      const actionSummary = !isSingleCalculateAction ?
+        await this._activeDoc.handleTriggers(localActionBundle) :
+        summarizeAction(localActionBundle);
 
       await this._activeDoc.updateRowCount(sandboxActionBundle.rowCount, docSession);
 
diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts
index 3b242402..cc54f0a8 100644
--- a/test/server/lib/DocApi.ts
+++ b/test/server/lib/DocApi.ts
@@ -4599,6 +4599,37 @@ function testDocApi() {
         await unsubscribe1();
       });
 
+      it('should not block document load (gh issue #799)', async function () {
+        // Create a test document.
+        const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id;
+        const docId = await userApi.newDoc({name: 'testdoc5'}, ws1);
+        const doc = userApi.getDocAPI(docId);
+        // Before #799, formula of this type would block document load because of a deadlock
+        // and make this test fail.
+        const formulaEvaluatedAtDocLoad = 'NOW()';
+
+        await axios.post(`${serverUrl}/api/docs/${docId}/apply`, [
+          ['ModifyColumn', 'Table1', 'C', {isFormula: true, formula: formulaEvaluatedAtDocLoad}],
+        ], chimpy);
+
+        const unsubscribeWebhook1 = await autoSubscribe('probe', docId);
+
+        // Create a first row.
+        await doc.addRows("Table1", {
+          A: [1],
+        });
+
+        await doc.forceReload();
+
+        // Create a second row after document reload.
+        // This should not timeout.
+        await doc.addRows("Table1", {
+          A: [2],
+        });
+
+        await unsubscribeWebhook1();
+      });
+
       it("should monitor failures", async () => {
         const webhook3 = await subscribe('probe', docId);
         const webhook4 = await subscribe('probe', docId);