[HOLD for payment 2022-03-02] Conversation - A single message was sent 100s of times #7666
Assignees
Labels
Awaiting Payment
Auto-added when associated PR is deployed to production
Daily
KSv2
Engineering
External
Added to denote the issue can be worked on by a contributor
Improvement
Item broken or needs improvement.
Needs Reproduction
Reproducible steps needed
Comments
isagoico
added
Engineering
Daily
|
Triggered auto assignment to @pecanoro ( |
|
This hit me too, and it was so weird |
|
The same issue happened with @conorpendergrast this morning:
Also happened with Conor's 10ams so it seems to be an issue on his end rather than mine: |
|
Here's logs when it happened to me, also, my steps, since it happened right after I reopened my computer. BUG: A message sent while my laptop was closed kept sending a couple dozen times and I got a notification ping for each.
|
|
Also happened to @twisterdotcom and I at 12:25 UTC (i kept getting notification after notification for the same line)
Can't make it stop by closing the app and restarting unfortunately. |
|
I first sent that message to Stef at |
|
Happened when I next sent a message to @muttmuure and then my test account [email protected] Desktop, endless errors but no repeating message: Basically endless: Mobile vid: Logs from the recipient: One such log: 6db5746a0dfa76f9-SAN From the sender: One example: 6db560f1683b773b-LHR |
|
Here were the logs from when it happened to me yesterday. |
This comment was marked as outdated.
This comment was marked as outdated.
pecanoro
added
Improvement
|
Triggered auto assignment to @NicMendonca ( |
|
Job posting: https://www.upwork.com/jobs/~01fb06ef1c153b1cab |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane ( |
MelvinBot
added
the
Help Wanted
|
Triggered auto assignment to @Beamanator ( |
|
I haven't seen it happen since I deleted and redownloaded the staging.dmg. I don't think that should stop us merging logging though because, my guess is it will crop up again for somebody. |
|
Yeah, just to be clear it's merged, but not deployed to staging/prod yet |
|
Any updates here? Are there logs suggesting it might be happening?
It would probably help if we see the history leading to these logs |
|
I haven't ever seen this happen again. Here are the occurrences of those logs in the last 7 days:
Looks like that exploded around The second log correlates with that time period as well: Zoomed in: It looks like most of those are for the email |
|
Thanks for the info @tgolen
I don't know who uses |
|
I have a theory that the Desktop app caused the 100s of message See the code here: Lines 161 to 190 in 853db57 Particularly lines that set details.responseHeaders['access-control-allow-origin'] = ['app://-']; The code was merged on 10th February - last date anyone reported the issue happening to them I would do a test to confirm |
|
OK, thanks. That desktop code is pretty familiar to @roryabraham so maybe he can also help.
What are you thinking of for a test? Like, reverting that code and see if it happens again? Or something else? |
Looks like that's @thesahindia from this slack message. |
Yes, reverting the code Yet the Desktop app worked for a long time with no CORS problems and without the mentioned changes Something seems to have resolved the issue, but we don't know what |
Yes I use that email. |
|
I think there might be something to @kidroca's theory, but I'm not really sure. The code @kidroca referenced above fixed a CORS issue on desktop – maybe before that CORS issue was fixed, we were blasting FWIW, there were no back-end or API changes related to this PR, which introduced the CORS errors in the first place. |
|
Thanks @roryabraham, @tgolen, I've managed to recreated it with the Desktop app Screen.Recording.2022-02-23.at.21.53.09.movPeople that happened to have the a fresh token an updated to the version in #7567 (1.1.38-0) would have had the issue Some takeaways:
Usually when cross origin requests are made there's OPTIONS preflight request verifying current origin is allowed to perform the requested operation and then the actual request follow. The linked SO thread explains why preflight request can sometimes be skipped Bottom line: Multiple domains can be whitelisted by implementing something like this: Access-Control-Allow-Origin Multiple Origin Domains? |
|
Just to be clear: there are no CORS issues at the moment This would allow us to whitelist the following domains and get rid of the proxy server:
|
botify
added
Weekly
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.39-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-03-02. 🎊 |
botify
changed the title
|
Cool, thanks for that info! This is the backend code for the CORS header:
So, maybe we should just ad |
The current code adds this Line 176 in 853db57 It shouldn't be prefixed with Buuut it also changes the origin sent by the request (I geuss Line 167 in 853db57 Also maybe there's some way to change |
|
@NicMendonca, @tgolen I've tracked any work around this ticket (including recreating the problem in Desktop today) in my hourly contract referencing this issue |
|
@NicMendonca Can you close the upwork job, please? Maybe to fix the host/origin problem, the PHP code can also look at HTTP_REFERRER to see if it contains if (in_array($host, ['comp.expensify.com', 'new.expensify.com', 'staging.new.expensify.com']) ) {
header('Access-Control-Allow-Origin: https://'.$host);
header('Access-Control-Allow-Credentials: true');
} elseif (str_starts_with($_SERVER['HTTP_REFERER'], 'app://')) {
header('Access-Control-Allow-Origin: app://-');
header('Access-Control-Allow-Credentials: true');
} elseif (DEBUG) {
header('Access-Control-Allow-Origin: *');
header('Access-Control-Allow-Credentials: true');
}We would have to change this too: |
|
Well just so you know both the referrer and origin are overridden here, so if you do decide to take this to the backend we should unset them on Desktop (which can happen after the server is updated) Lines 167 to 169 in 853db57 |
|
@kidroca is there still work being done here before filling out the Root Cause Analysis (RCA) doc? |
Not that I know of. |
|
@mallenexpensify This ticket can be closed: #7666 (comment) |
|
Thanks @kidroca , closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
A single message should be seen in the conversation history.
Actual Result:
100s of repeated messages were sent to the other user.
Workaround:
No need.
Platform:
Where is this issue occurring?
Version Number: v1.1.38-1
Reproducible in staging?: Unknown
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Expensify/Expensify Issue URL:
Issue reported by: @tgolen
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644440925115879
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: