bettersg / checkMate

GNU General Public License v3.0
5 stars 1 forks source link

[Bug] Image messages being received more than once #152

Closed sarge1989 closed 1 year ago

sarge1989 commented 1 year ago

Occasionally, but seemingly not always, image messages are being received multiple times by the system.

Meaning multiple instances and sometimes messages get created, all from just 1 image message being sent in.

Users then get back multiple replies to the same message.

Managed to replicate in dev but does not always happen.

Still unsure why this is the case. Current guess is due to time taken for OCR - perhaps WhatsApp webhook fires retries instead of waiting ~15seconds ++ (expected time for image instance handling) for a HTTP 200? Need to look into this further.

WhatsApp Retries documentation: https://developers.facebook.com/docs/whatsapp/cloud-api/guides/set-up-webhooks

Webhook Delivery Failure If we send a webhook request to your endpoint and your server responds with an HTTP status code other than 200, or if we are unable to deliver the webhook for another reason, we will keep trying with decreasing frequency until the request succeeds, for up to 7 days. Note that retries will be sent to all apps that have subscribed to webhooks (and their appropriate fields) for the WhatsApp Business Account. This can result in duplicate webhook notifications.

madanalogy commented 1 year ago

Still unsure why this is the case. Current guess is due to time taken for OCR - perhaps WhatsApp webhook fires retries instead of waiting ~15seconds ++ (expected time for image instance handling) for a HTTP 200? Need to look into this further.

Pretty solid guess imo. Does the ocr operation need to be synchronous? Is there a need to wait for it to complete before sending the response to the webhook request? From what I can see, messages to the user are sent separately from the webhook request

stonefruit commented 1 year ago

I think we should be able to send a response immediately to whatsapp to acknowledge the webhook request.

In the long run, we might want to store these webhook requests temporarily and remove them when the related job ends, so that we can rerun them if something fails in our functions, and to debug any strange behaviours.

madanalogy commented 1 year ago

In the long run, we might want to store these webhook requests temporarily and remove them when the related job ends, so that we can rerun them if something fails in our functions, and to debug any strange behaviours.

I think a pub sub model really fits this use case. Once the webhook receives the request from whatsapp, all it does is publish to the topic. This allows other downstream services to independently perform followup actions (e.g. logging / storing the request or performing ocr etc) https://firebase.google.com/docs/functions/pubsub-events?gen=2nd

This could help with the system modularity as well, allowing any new features developed in future to be strongly decoupled from the webhook logic.

stonefruit commented 1 year ago

Nice. We should consider it later on since firebase supports it.

sarge1989 commented 1 year ago

Thanks @madanalogy and @stonefruit

In the long run, we might want to store these webhook requests temporarily and remove them when the related job ends, so that we can rerun them if something fails in our functions, and to debug any strange behaviours.

I think a pub sub model really fits this use case. Once the webhook receives the request from whatsapp, all it does is publish to the topic. This allows other downstream services to independently perform followup actions (e.g. logging / storing the request or performing ocr etc) https://firebase.google.com/docs/functions/pubsub-events?gen=2nd

This could help with the system modularity as well, allowing any new features developed in future to be strongly decoupled from the webhook logic.

I also lean towards pub/sub, since it's natively supported by firebase. I think moving the app in a more decoupled direction should be our goal. Also, a message queue may be a way to solve one perennial pain point, which is that sometimes a user can forward in multiple messages of a convo, and each reach us (and are handled) individually when ideally we want to amalgamate them.

sarge1989 commented 1 year ago

Probably will try and implement pub sub then. Just nice I wanna migrate to functions V2 haha so might as well. Meanwhile, since it's a relatively majorish change that might take some time, I've put up a PR to disable the OCR for now, so users dont get spammed.