Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FS-1588] No proposals after deleting a subconversation #3123

Merged
merged 6 commits into from
Mar 9, 2023
Merged

[FS-1588] No proposals after deleting a subconversation #3123

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21f1797
Test for no leftover proposals
mdimjasevic Mar 3, 2023
523834d
Delete all proposals upon deleting a subconv
mdimjasevic Mar 3, 2023
2f2d0fa
Add a changelog
mdimjasevic Mar 7, 2023
e110f51
Hi CI
mdimjasevic Mar 9, 2023
d59b7f5
Revert "Delete all proposals upon deleting a subconv"
mdimjasevic Mar 9, 2023
122b6a8
fixup! Test for no leftover proposals
mdimjasevic Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.d/1-api-changes/delete-subconversation
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Introduce an endpoint for deleting a subconversation (#2956, #3119)
Introduce an endpoint for deleting a subconversation (#2956, #3119, #3123)
1 change: 1 addition & 0 deletions services/galley/src/Galley/API/Federation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,7 @@ deleteSubConversationForRemoteUser ::
Input (Local ()),
Input Env,
MemberStore,
ProposalStore,
Resource,
SubConversationStore,
SubConversationSupply
Expand Down
5 changes: 5 additions & 0 deletions services/galley/src/Galley/API/MLS/SubConversation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import Galley.Effects
import Galley.Effects.FederatorAccess
import qualified Galley.Effects.FederatorAccess as Eff
import qualified Galley.Effects.MemberStore as Eff
import qualified Galley.Effects.ProposalStore as Eff
import qualified Galley.Effects.SubConversationStore as Eff
import Galley.Effects.SubConversationSupply (SubConversationSupply)
import qualified Galley.Effects.SubConversationSupply as Eff
Expand Down Expand Up @@ -236,6 +237,7 @@ deleteSubConversation ::
FederatorAccess,
Input Env,
MemberStore,
ProposalStore,
Resource,
SubConversationStore,
SubConversationSupply
Expand Down Expand Up @@ -264,6 +266,7 @@ deleteLocalSubConversation ::
FederatorAccess,
Input Env,
MemberStore,
ProposalStore,
Resource,
SubConversationStore,
SubConversationSupply,
Expand All @@ -288,7 +291,9 @@ deleteLocalSubConversation qusr lcnvId scnvId dsc = do
let (gid, epoch) = (cnvmlsGroupId &&& cnvmlsEpoch) (scMLSData sconv)
unless (dscGroupId dsc == gid) $ throwS @'ConvNotFound
unless (dscEpoch dsc == epoch) $ throwS @'MLSStaleMessage

Eff.removeAllMLSClients gid
Eff.deleteAllProposals gid

newGid <- Eff.makeFreshGroupId

Expand Down
2 changes: 1 addition & 1 deletion services/galley/src/Galley/App.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,10 @@ evalGalley e =
. interpretLegalHoldStoreToCassandra lh
. interpretCustomBackendStoreToCassandra
. randomToIO
. interpretProposalStoreToCassandra
. interpretSubConversationSupplyToRandom
. interpretSubConversationStoreToCassandra
. interpretConversationStoreToCassandra
. interpretProposalStoreToCassandra
. interpretCodeStoreToCassandra
. interpretClientStoreToCassandra
. interpretFireAndForget
Expand Down
11 changes: 9 additions & 2 deletions services/galley/src/Galley/Cassandra/SubConversation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import Galley.API.MLS.Types (SubConversation (..))
import Galley.Cassandra.Conversation.MLS (lookupMLSClients)
import qualified Galley.Cassandra.Queries as Cql
import Galley.Cassandra.Store (embedClient)
import Galley.Effects.ProposalStore
import Galley.Effects.SubConversationStore (SubConversationStore (..))
import Imports
import Polysemy
Expand Down Expand Up @@ -103,7 +104,10 @@ listSubConversations cid = do
)

interpretSubConversationStoreToCassandra ::
Members '[Embed IO, Input ClientState] r =>
( Member (Embed IO) r,
Member (Input ClientState) r,
Member ProposalStore r
) =>
Sem (SubConversationStore ': r) a ->
Sem r a
interpretSubConversationStoreToCassandra = interpret $ \case
Expand All @@ -115,7 +119,10 @@ interpretSubConversationStoreToCassandra = interpret $ \case
SetSubConversationEpoch cid sconv epoch -> embedClient $ setEpochForSubConversation cid sconv epoch
DeleteGroupIdForSubConversation groupId -> embedClient $ deleteGroupId groupId
ListSubConversations cid -> embedClient $ listSubConversations cid
DeleteSubConversation convId subConvId -> embedClient $ deleteSubConversation convId subConvId
DeleteSubConversation convId subConvId -> do
msub <- embedClient (selectSubConversation convId subConvId)
for_ msub $ deleteAllProposals . cnvmlsGroupId . scMLSData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why explicitly deleting the pending proposals? Essentially, this is buying Cassandra space and paying with users' time. I'd argue that users' time is more valuable. Also, the Cassandra records have a TTL and will be deleted eventually and they will not appear in a recreated subconversation, since the new subconversation's groupID is a different one than the one of the subconversation before. This is also demonstrated by your test which passes even without changes in business logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not seeing and commenting this when I checked this PR previously!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your reasoning is perfectly fine, but if we're to accept this argument, I'd prefer to hear arguments for the deleting a subconversation operation to be on a critical path, ideally backed by numbers showing this DB clean-up noticeably slows down the response time. We could let the proposals linger until they're garbage-collected, but to me this seems not so great.

I expect this delete DB statement to be deleting one or two records on average, and in rare case a handful of them. When this is put next to so many DB operations we do anyway in this endpoint handler, I'd argue the introduced time overhead is negligible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you raise this in the chat? I would argue that removing the records was not forgotten, but deliberately skipped. As to the critical path, ending a call should be done as quick as possible. What would you do if you hang up and nothing happens for a moment? We need a second opinion/review if you insist on cleaning up the non-user-facing database in exchange for user-facing latency.

Copy link
Contributor

@pcapriotti pcapriotti Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since proposals have a TTL, there isn't much point to delete them, unless you really want them gone for semantic reasons. Note that deleting doesn't actually reclaim any space in cassandra, AFAIU.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll revert the change in the application code. The test can stay as it doesn't hurt to assert there are no leftovers.

embedClient $ deleteSubConversation convId subConvId

--------------------------------------------------------------------------------
-- Utilities
Expand Down
2 changes: 1 addition & 1 deletion services/galley/src/Galley/Effects.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ type GalleyEffects1 =
FireAndForget,
ClientStore,
CodeStore,
ProposalStore,
ConversationStore,
SubConversationStore,
SubConversationSupply,
ProposalStore,
Random,
CustomBackendStore,
LegalHoldStore,
Expand Down
44 changes: 44 additions & 0 deletions services/galley/test/integration/API/MLS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ tests s =
test s "reset a subconversation as a creator" (testDeleteSubConv SubConvMember),
test s "reset a subconversation as a conversation member" (testDeleteSubConv ConvMember),
test s "reset a subconversation as a random user" (testDeleteSubConv RandomUser),
test s "reset a subconversation and assert no leftover proposals" testJoinDeletedSubConvWithRemoval,
test s "fail to reset a subconversation with wrong epoch" testDeleteSubConvStale,
test s "leave a subconversation as a creator" (testLeaveSubConv True),
test s "leave a subconversation as a non-creator" (testLeaveSubConv False),
Expand Down Expand Up @@ -2793,6 +2794,49 @@ testDeleteSubConv deleterType = do
"Old and new subconversation are not equal"
(sub == newSub)

-- In this test case, Alice creates a subconversation, Bob joins and Alice
-- leaves. The leaving causes the backend to generate an external remove
-- proposal for the client by Alice. Next, Bob does not commit (simulating his
-- client crashing), and then deleting the subconversation after coming back up.
-- This should make the state of the subconversation completely clean, including
-- any leftover proposals removed. Then Bob creates a subconversation with the
-- same subconversation ID and the test asserts that both Alice and Bob get no
-- events, which means the backend does not resubmit the pending remove proposal
-- for Alice's client.
testJoinDeletedSubConvWithRemoval :: TestM ()
testJoinDeletedSubConvWithRemoval = do
[alice, bob] <- createAndConnectUsers [Nothing, Nothing]
runMLSTest $ do
[alice1, bob1] <- traverse createMLSClient [alice, bob]
void $ uploadNewKeyPackage bob1
(_, qcnv) <- setupMLSGroup alice1
void $ createAddCommit alice1 [bob] >>= sendAndConsumeCommitBundle
let subConvId = SubConvId "conference"
qsconvId <- createSubConv qcnv alice1 subConvId
void $
createExternalCommit bob1 Nothing qsconvId
>>= sendAndConsumeCommitBundle
liftTest $
leaveSubConv (ciUser alice1) (ciClient alice1) qcnv subConvId
!!! const 200 === statusCode
-- no committing by Bob of the backend-generated remove proposal for alice1
-- (simulating his client crashing)

do
sub <-
liftTest $
responseJsonError
=<< getSubConv (qUnqualified bob) qcnv subConvId
<!! const 200 === statusCode
let dsc = DeleteSubConversationRequest (pscGroupId sub) (pscEpoch sub)
liftTest $
deleteSubConv (qUnqualified bob) qcnv subConvId dsc
!!! const 200 === statusCode

mlsBracket [alice1, bob1] $ \wss -> do
void $ createSubConv qcnv bob1 subConvId
void . liftIO $ WS.assertNoEvent (3 # WS.Second) wss

testDeleteSubConvStale :: TestM ()
testDeleteSubConvStale = do
alice <- randomQualifiedUser
Expand Down