Remove useless identity contracts after leaving group

[Silver-IT]

Summary of Changes

• Closes Unsubscribe from identity contracts after leaving group #1672

[cypress]

Passing run #1992 ↗︎

0

110

8

0

0

Details:

Merge a658d7a into 7aba1d6...
Project: group-income	Commit: 2eaf3fc7da ℹ️
Status: Passed	Duration: 11:06 💡
Started: Mar 18, 2024 12:54 PM	Ended: Mar 18, 2024 1:05 PM

Review all test suite changes for PR #1885 ↗︎

[corrideat]

This looks like a good first step, although I'm not sure about the approach taken.

The main issue is that this is reading and acting upon information found in a different contracts, i.e., group A is reading information about group B. I know this is sometimes unavoidable, but in general it should not be done because it can introduce all sorts of rare race conditions (this is why I refactored many of the complex actions into simpler actions with side effects). I think a good model to use is that the (Chelonia) state for a certain client should always be the same, no matter the order events are processed or how they end up being processed. I'm not convinced that this is the case here, because there could be several groups syncing in the background and this could remove a member's contract from another group.

I've thought about this issue (I mean removing identity contracts) in the past, and I came to the conclusion that the 'best' approach would probably involve some kind of tagged reference counting. So, for example, each contract has a set of subscription 'reasons' (which could be the name of the contract that requested it) and once that set is empty, the contract is removed.

The second challenge with implementing this (in general, this isn't about the approach used here) is that we rely on the identity contract state being present to display (past) information. For example, if you have a group with A, B and C, and then B leaves, you still want (probably) to use B's profile information in the chatroom. By removing the profile entirely, this will no longer be the case.

Solving this second issue is trickier, but I suppose we could use some kind of cache layer for removed profile pictures.

A third issue (and maybe distinct from this issue, which is about removing contracts) is fetching unnecessary information. Let's go back to the previous example of a group with A, B and C. Let's say now that B leaves the group at time t, and the group contract was at height GH and B's contract at height BH. Now, if C were to, for example, log in from a new device, they'd sync the contracts for A, B, C and G in their entirety, even though contract B should only be synced up to BH. I'm mentioning this issue because of trying to always build the same state, but I realise it's difficult to tackle.

[Silver-IT]

@corrideat, I totally agree that every process in a certain contract should not rely on another contract's state as much as possible. But we need them sometimes. So if it's needed, we should wait until those event queues are empty.
So, I think I should update this PR to process removing identity contracts after the necessary event queues are empty.

What do you think, @corrideat?

[corrideat]

I think that that could work, but I also see potential for deadlocks if there are several groups doing this.

What do you think about the other issues mentioned? (For example, that the profile will no longer be available, which affects chats)

[Silver-IT]

What do you think about the other issues mentioned? (For example, that the profile will no longer be available, which affects chats)

We don't remove the identity contracts which is used for any other purposes, such as the identity contracts which is used for direct messages, and the identity contracts of another group members. In my updates of this PR, identityContractsMapToKeepSyncing is used for this purpose.

However, the removing identity contracts should not be done until the necessary (all the contracts invocation queue?) invocation queues are empty. @corrideat, what do you recommend for this? Do you think setInterval should be used for waiting the those queued invocations to be fully executed?

[corrideat]

We don't remove the identity contracts which is used for any other purposes, such as the identity contracts which is used for direct messages, and the identity contracts of another group members

Right, but I'm talking about this situation:

For example, if you have a group with A, B and C, and then B leaves, you still want (probably) to use B's profile information in the chatroom. By removing the profile entirely, this will no longer be the case.

[Silver-IT]

@corrideat, please review this PR, and focus on identityContractsMapToKeepSyncing variable. This is useful to ignore ideneity contracts to remove when they are using for another purposes, such as for direct messages or for another group contracts.

[corrideat]

I feel that we're talking past each other. identityContractsMapToKeepSyncing tries to keep identity contracts that are used in other groups. So, if you have G1 = {A, B, C} and G2 = {A, B, C}, and then B leaves G1, A and C will not remove B from their sync list because they still have a group in common, namely G2.

It's good to handle this, but this is not the situation I was mentioning. In the situation that I mentioned G2 doesn't exist (and there are no DMs either). What would happen then is that B will be removed by A and C, which is exactly the point of this PR. So far, everything is working as intended. Except that now, because B was removed, ourContactProfilesById / ourContactProfilesByUsername / globalProfile will not have the profile information for B. Because of this, the information that can be displayed about B is pretty limited, which is just their user ID.

[Silver-IT]

For the situation you mentioned, A and C should not remove the B's identity contract. Only B should remove the identity contracts of A and C. Because B is not needed those contracts. To remove identity contracts only happens for themselves who leave group, not the other group members.

[corrideat]

@Silver-IT Yes, you're right. I take it back, the scenario that I described isn't an issue, and because you're not checking whether the profile is active, other related scenarios that I thought of aren't an issue either (this was, create G1 = {A, B, C}, G2 = {A, B, C}, A, C leave G1, then B leaves G2).

So, overall, now I think that this could work, if you can wait on the queues to emptied in a safe manner.

[corrideat]

I'm not a fan of using setInterval in general, and in this case it seems like it might work but is unsafe. If you must do it this way, firstly I'd use setTimeout instead, which ensures that the callback isn't executed more than once at the same time. I'd also combine it with a /wait call to start it off, since you're expecting that those contracts to be removed, so most of the time there won't be any more events after the call to /wait completes.

A safer approach to doing this, without any setTimeout / setInterval would be to create a new sync barrier API. This barrier API would be something of a semaphore that'd also keep track in its internal state what's being waited, to attempt to break cycles in some way.

[taoeffect]

What exactly does this line do and why is it here?

I think this would work without it... and possibly be safer too.

[Silver-IT]

The pending variable would be filled with pending invocations. Since we should access the state of other contracts when we are leaving the group, we should make it clear that those contracts are all up to date. So just wait until this pending should be an empty list.

The reason why we could not use await sbp('chelonia/contract/wait', contractIDs) is that this has a potential deadlock. I need another solution that doesn't use setInterval (I knew I shouldn't use it, but could not find another solution), and I will find the better solution with the help of @corrideat.

[taoeffect]

Sorry, I wasn't very clear, I meant why is this checking for typeof q === 'strings'? I believe they should all be strings.

[taoeffect]

Nice work @Silver-IT!

This does seem like it would work.

While the ideal way to solve this would be to use reference counting (#1249), we are not anywhere close to having that implemented yet, and this solution should work well until then I think.

And actually, even with reference counting there is still the issue that @corrideat brought up, of display names and profile pictures being missing from chatrooms for identities that have left the group. This PR actually doesn't have that problem, because it only removes identity contracts when we are the ones that left the group.

I left a single comment/request.

[corrideat]

Instead of using setTimeout / setInterval, I'd refactor this as follows

const foo = () => {
  sbp('chelonia/contract/wait', contractIDs).then(() => {
    // You need to check the user because if the session changed this is no longer relevant
    // and you could be removing contracts that you shouldn't
    if (currentUserHasChanged) return; // alternatively, you could use an event listener
    if (contractIDsInQueue) { // the check that you're doing with
                              // const pending = Object.entries(sbp('okTurtles.eventQueue/queuedInvocations'))...
                              // IMPORTANT: The check needs to be updated to check the
                              // same queues that `chelonia/contract/wait` is using
                              // or else this could cause an infinite loop.
                              // (Even if you use the `setInterval` approach, you should
                              // still only check the relevant queues, because there could
                              // always be unrelated things running in the background)

      foo()
      return
    }
    // The rest of the logic follows here
  }).catch(/* ... */);
}

I think this would be safe (or safer) while avoiding unnecessary checking. It also checks for the user changing, which is needed because you don't want to remove contracts in that case. Maybe you can also give up after a certain number of attempts.

ETA: IMHO not needed, but you can also wrap the then callback in debounce and that will ensure that, even if something goes wrong, there will not be a blocking infinite loop. You could also replace foo with setTimeout(foo, 100) or something like that.

[Silver-IT]

@corrideat, I don't think we can simply remove setTimeout/setInterval by using the only chelonia/contract/wait to wait for invocation queues to be empty. That's because chelonia/contract/wait is also creating a new invocation in the queue.

Let's imagine that there is a pending invocation which is to publish a new event (as well as that we are running the above codes). This would always check if the invocation queue is empty at the beginning of publish. If the queue is not empty, it will add another invocation in the end and wait. So there will be inifinite adding invocations to the queue and waiting for the queues to be empty on both sides.

As a result (Also it was a starting point), the problem is that we should have a way to wait the invocation queue to be empty without adding new invocation to the queue. That was the reason why I used setInterval (setTimeout is also fine).

cc: @taoeffect

[corrideat]

Let's imagine that there is a pending invocation which is to publish a new event (as well as that we are running the above codes). This would always check if the invocation queue is empty at the beginning of publish. If the queue is not empty, it will add another invocation in the end and wait. So there will be inifinite adding invocations to the queue and waiting for the queues to be empty on both sides.

Greg and I discussed this last week, and that's the reason to make sure that the check for the queue state uses the exact same queues as wait does. If the queues match, there could be an infinite loop (you can prevent this by giving up after a while), but it'd be the same as if you were using set*. Crucially, the loop, infinite or not, will not block the application (this also applies to set*).

Let's go over that would happen:

1. You call wait with [A, B, C]. This pushes a new event at the end of each of those queues. Let's call it e1.
2. By the time the callback is executed (I corrected the example above, which was using the API incorrectly: it was using the callback as the third argument (which doesn't exist) instead of it being a .then() callback), e1 has completed. Now you check the queues [A, B, C] again.
3. If the queues are empty, you proceed. However, if there is some new event on either of those queues, you go back to step 1. This is what could cause an infinite loop. However, the important part is that, at step 2, e1 has completed, so the queues have a chance to be empty.

[Silver-IT]

Yeah, Correct. So we have no way to give up using set* in this situation. That's what I was saying, we can not simply remove setTimeout/setInterval by using the only chelonia/contract/wait.

If you and also Greg are fine with using setTimeout to temporarily prevent the infinite loop, I will do that way.

[taoeffect]

@Silver-IT I am fairly convinced by @corrideat's updated version of his code that it will not result in an infinite loop.

I don't think setTimeout is needed. Can you give his code a try and see if it results in an infinite loop? If it doesn't, I think it's a cleaner approach.

[Silver-IT]

Actually, I already tried that way. But it results in a infinite loop. That's why I added a comment above.

You could also replace foo with setTimeout(foo, 100) or something like that.

The one of the solutions is this. We need to use setTimeout.

[taoeffect]

Actually, I already tried that way.

@Silver-IT Please try again, using the latest code that @corrideat used. Note: he just added this code about an hour ago. (Did you use that latest version?)

[Silver-IT]

I have already tried that way this morning. It results in a infinite loop.

[corrideat]

@Silver-IT Are you able to push your changes? I can't see how an infinite loop would happen, but it'd be easier to see the issue with a working example.

[corrideat]

This is where the main issue lies. As explained in the comments, you must filter out the irrelevant keys. It's not enough to check whether there are any queued events, but you must check whether there are events only for the keys you're interested in.

[corrideat]

So, here you'd have:

const waitFor = [...chatRoomIDs, ...groupIDs]

sbp('chelonia/contract/wait', waitFor).then(() => {
  // ...
  const pending = Object.entries(sbp('okTurtles.eventQueue/queuedInvocations'))
    .filter(x => waitFor.includes(x))

  if (pending.length) {
    return removeIdentityContracts()
  }
  // ...
})

[corrideat]

The comment below about the outer scope mostly referred to this waitFor here. I.e., you could refresh the list as the list of groups or chatrooms could've changed between the time wait was called and pending is checked.

[taoeffect]

Looking great @Silver-IT! One forgotten debug logging statement that needs to be removed and after that I think it's GTM

[taoeffect]

So I added this logging to test this:

        const identityContractsToRemove = possiblyUselessContractIDs.filter(cID => !identityContractsMapToKeepSyncing[cID])
        console.error('going to remove', identityContractsToRemove.length, 'contracts:', identityContractsToRemove)

And then I did these steps:

1. [Tab1] u1 creates group and copies invite link
2. [Tab2] u2 joins using invite link
3. [Tab3] u3 joins using invite link
4. [Tab1] u1 leaves group

This resulted in the following error appearing in the console:

Note also that the error that I added that I expected to appear, does not appear in the console!

[Silver-IT]

The console won't be logged. Because in your situation, there is no chatroomIDs and groupIDs to consider. u1 doesn't have another groups and any direct messages. So it will be returned in line 916 of controller/actions/group.js.

I will check the console error you pointed, and get you updated.

[Silver-IT]

And about the error you got while testing, is that what we could ignore. I think that's related to the updated workflow. It happens in master branch either.

cc: @corrideat

[taoeffect]

Because in your situation, there is no chatroomIDs and groupIDs to consider. u1 doesn't have another groups and any direct messages. So it will be returned in line 916 of controller/actions/group.js.

I replied on Slack about this: https://okturtles.slack.com/archives/C0EH7P20Y/p1710933952734909

(You can reply there).

But I also want to share another error that I got with @corrideat about what happened when I rejoined u1 to that group (see the steps here), I got this error:

Is this one of the errors that's supposed to happen? If so, should it be a warning?

[taoeffect]

Approved after @Silver-IT's reply on Slack. I missed the behavior on line 913