From f37b1ec62318dac8caba8450463ad8df5b556aa0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ricardo=20Iv=C3=A1n=20Vieitez=20Parra?=
 <3857362+corrideat@users.noreply.github.com>
Date: Fri, 21 Jun 2024 19:14:16 +0000
Subject: [PATCH] Various bugfixes:

- Comments explaining retain / release logic
- Fix #2093
- Fix #2066 (potentially)
---
 frontend/controller/actions/utils.js |  3 +++
 frontend/model/contracts/group.js    | 39 ++++++++++++++++------------
 shared/domains/chelonia/internals.js |  2 +-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/frontend/controller/actions/utils.js b/frontend/controller/actions/utils.js
index 0a4e818b7..e98eabeda 100644
--- a/frontend/controller/actions/utils.js
+++ b/frontend/controller/actions/utils.js
@@ -69,6 +69,9 @@ export const encryptedAction = (
 
       try {
         // Writing to a contract requires being subscribed to it
+        // Since we're only interested in writing and we don't know whether
+        // we're subscribed or should be, we use an ephemeral retain here that
+        // is undone at the end in a finally block.
         await sbp('chelonia/contract/retain', contractID, { ephemeral: true })
         const state = {
           [contractID]: await sbp('chelonia/latestContractState', contractID)
diff --git a/frontend/model/contracts/group.js b/frontend/model/contracts/group.js
index 4751638b6..e760d0994 100644
--- a/frontend/model/contracts/group.js
+++ b/frontend/model/contracts/group.js
@@ -1754,7 +1754,23 @@ sbp('chelonia/defineContract', {
       }
 
       try {
-        await sbp('chelonia/contract/retain', chatRoomID, { ephemeral: true })
+        // We need to be subscribed to the chatroom before writing to it, and
+        // also because of the following check (hasKeysToPerformOperation),
+        // which requires state.
+        // If we're joining the chatroom ourselves (actorID === memberID),
+        // ensure we _remain_ subscribed to the chatroom by not using an
+        // ephemeral call to retain.
+        // If we're _not_ joining the chatroom ourselves (but instead we've
+        // added someone else), we use 'ephemeral: true' because we don't want
+        // to remain subscribed to the chatroom if we're not a member(*).
+        // This used to be done in four steps: unconditional ephemeral retain
+        // here, ended with an ephemeral release in the finally block, and two
+        // conditional persistent retains on postpublish (for the current
+        // device) and the error handler (for other devices). This was too
+        // complex.
+        // (*) Yes, usually we'd be a member of the chatroom in this case, but
+        // we could have left afterwards.
+        await sbp('chelonia/contract/retain', chatRoomID, actorID !== memberID ? { ephemeral: true } : {})
 
         if (!sbp('chelonia/contract/hasKeysToPerformOperation', chatRoomID, 'gi.contracts/chatroom/join')) {
           throw new Error(`Missing keys to join chatroom ${chatRoomID}`)
@@ -1768,29 +1784,18 @@ sbp('chelonia/defineContract', {
         await sbp('gi.actions/chatroom/join', {
           contractID: chatRoomID,
           data: actorID === memberID ? {} : { memberID },
-          encryptionKeyId,
-          ...actorID === memberID && {
-            hooks: {
-              postpublish: () => {
-                sbp('chelonia/contract/retain', chatRoomID)
-              }
-            }
-          }
+          encryptionKeyId
         }).catch(e => {
           if (e.name === 'GIErrorUIRuntimeError' && e.cause?.name === 'GIChatroomAlreadyMemberError') {
-            if (actorID === memberID) {
-              // Increase reference count if we've already joined
-              // Note: this addresses syncing the contract from a new device,
-              // where `retain` in postpublish hasn't been called and the
-              // reference count is zero due to the state being fresh.
-              sbp('chelonia/contract/retain', chatRoomID)
-            }
             return
           }
+
           console.warn(`Unable to join ${memberID} to chatroom ${chatRoomID} for group ${contractID}`, e)
         })
       } finally {
-        await sbp('chelonia/contract/release', chatRoomID, { ephemeral: true })
+        if (actorID !== memberID) {
+          await sbp('chelonia/contract/release', chatRoomID, { ephemeral: true })
+        }
       }
     },
     // eslint-disable-next-line require-await
diff --git a/shared/domains/chelonia/internals.js b/shared/domains/chelonia/internals.js
index f8dcaea78..3054abb3b 100644
--- a/shared/domains/chelonia/internals.js
+++ b/shared/domains/chelonia/internals.js
@@ -796,7 +796,7 @@ export default (sbp('sbp/selectors/register', {
             }
           }
 
-          const previousVolatileState = targetState._volatile
+          const previousVolatileState = targetState?._volatile
           sbp('chelonia/private/queueEvent', v.contractID, ['chelonia/private/postKeyShare', v.contractID, mustResync ? previousVolatileState : null, signingKey])
             .then(() => {
             // The CONTRACT_HAS_RECEIVED_KEYS event is placed on the queue for