Fix dropped_client_doesnt_create_duplicate_carbons flaky test #4423

gustawlippa · 2024-12-06T14:10:13Z

carboncopy_SUITE:dropped_client_doesnt_create_duplicate_carbons is one of the flaky tests (failed run on GH Actions: https://github.com/esl/MongooseIM/actions/runs/12253944118/job/34183783884, failed run on CircleCI: https://app.circleci.com/pipelines/github/esl/MongooseIM/13363/workflows/761201e1-fae8-4792-a5df-051914a41ae6/jobs/248193).

The test:

Disconnects one of two user's resources,
waits for presence unavailable on the remaining resource,
after receiving the presence, sends a message from the remaining resource,
checks to make sure that the disconnected resource didn't get a carboncopy of the message.
However, sometimes the disconnected resource would actually get the stanza, resulting in a flaky test.

I think the issue comes from a race condition between sending the unavailable stanza on behalf of the user, and calling the unset_presence hook. To illustrate the point, I've tested a commit almost ensuring that the issue will appear by making MIM slower in the problematic place: https://github.com/esl/MongooseIM/commits/deab685f495bf73cec5409fc873f98ba75d168d0. The tests have failed as was suspected: https://github.com/esl/MongooseIM/runs/34035992149. I think that the dropped_client_doesnt_create_duplicate_carbons test is correct, and while it is unlikely that a user would notice this behaviour, it's probably still better to fix the implementation instead of changing the testcase.

The fix required changing the order: First, removing the session, and later broadcasting the unavailable presence. I also changed the direct hook call to a call through ejabberd_sm, which additionally sets priority for the disconnected resource to undefined. The session would be removed anyway by mongoose_c2s, but now there is less chance of a similar issue from different modules which would listen to unset_presence hook. mod_presence:presence_update_to_unavailable/6 function also calls this hook through ejabberd_sm.
Perhaps it would be better to change the ejabberd_sm:(un)set_presence/5 functions to be handlers of the unset_presence hook, and make sure only mod_presence is responsible for calling it, but I decided this rework was out of scope of this PR.

I've run the GH Actions manually for this PR, and there are still flaky tests, but the carboncopy case is not one of them: https://github.com/esl/MongooseIM/actions/runs/12278680395. Of course it is tricky to test that the test is not flaky any more, since it may pass by chance.

mongoose-im · 2024-12-06T14:18:39Z

elasticsearch_and_cassandra_27 / elasticsearch_and_cassandra_mnesia / deab685
Reports root/ big
OK: 473 / Failed: 0 / User-skipped: 49 / Auto-skipped: 0

small_tests_26 / small_tests / deab685
Reports root / small

small_tests_27 / small_tests / deab685
Reports root / small

small_tests_27_arm64 / small_tests / deab685
Reports root / small

ldap_mnesia_26 / ldap_mnesia / deab685
Reports root/ big
OK: 2329 / Failed: 20 / User-skipped: 912 / Auto-skipped: 3

carboncopy_SUITE:one2one:dropped_client_doesnt_create_duplicate_carbons

{error,
 {test_case_failed,
  {has_stanzas_but_shouldnt,
   {client,
  <<"alice_dropped_client_doesnt_create_duplicate_carbons_471@localhost/res1">>,
  escalus_tcp,<0.10431.0>,
  [{event_manager,<0.10306.0>},
   {server,<<"localhost">>},
   {username,<<"alicE_dropped_client_doesnt_create_duplicate_carbons_471">>},
   {resource,<<"res1">>}],
  [{event_client,
    [{event_manager,<0.10306.0>},
     {server,<<"localhost">>},
     {username,
    <<"alicE_dropped_client_doesnt_create_duplicate_carbons_471">>},
     {resource,<<"res1">>}]},
   {resource,<<"res1">>},
   {username,<<"alice_dropped_client_doesnt_create_duplicate_carbons_471">>},
   {server,<<"localhost">>},
   {host,<<"localhost">>},
   {port,5222},
   {auth,{escalus_auth,auth_plain}},
   {wspath,undefined},
   {username,<<"alicE_dropped_client_doesnt_create_duplicate_carbons_471">>},
   {server,<<"localhost">>},
   {password,<<"matygrysa">>},
   {stream_id,<<"d5ee7818834704a3">>}]},
   [{xmlel,<<"message">>,
   [{<<"from">>,
     <<"alice_dropped_client_doesnt_create_duplicate_carbons_471@localhost">>},
    {<<"to">>,
     <<"alice_dropped_client_doesnt_create_duplicate_carbons_471@localhost/res2">>},
    {<<"xmlns">>,<<"jabber:client">>},
    {<<"type">>,<<"chat">>}],
   [{xmlel,<<"sent">>,
     [{<<"xmlns">>,<<"urn:xmpp:carbons:2">>}],
     [{xmlel,<<"forwarded">>,
     [{<<"xmlns">>,<<"urn:xmpp:forward:0">>}],
     [{xmlel,<<"message">>,
       [{<<"from">>,
       <<"alice_dropped_client_doesnt_create_duplicate_c...

Fix dropped_client_doesnt_create_duplicate_carbons flaky test #4423

Fix dropped_client_doesnt_create_duplicate_carbons flaky test #4423

Conversation

gustawlippa commented Dec 6, 2024 • edited Loading

mongoose-im commented Dec 6, 2024 • edited Loading

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

mongoose-im commented Dec 9, 2024 • edited Loading

mongoose-im commented Dec 11, 2024 • edited Loading

jacekwegr left a comment

Choose a reason for hiding this comment

gustawlippa commented Dec 6, 2024 •

edited

Loading

mongoose-im commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

mongoose-im commented Dec 9, 2024 •

edited

Loading

mongoose-im commented Dec 11, 2024 •

edited

Loading