Close #8846: Re-subscribe FxA push subscription on subscriptionExpired #8883
Conversation
|
This pull request has conflicts when rebasing. Could you fix it @jonalmeida? 🙏 |
|
jonalmeida
force-pushed
the
issue-8846
branch
from
0482d83 to
95e2124
Compare
|
This pull request has conflicts when rebasing. Could you fix it @jonalmeida? 🙏 |
| logger.info("Subscribing to FxA push failed at login.", e) | ||
| }, | ||
| onSubscribe = { subscription -> | ||
| logger.info("Created a new subscription: $subscription") |
There was a problem hiding this comment.
Could be a useful breadcrumb to have as well
There was a problem hiding this comment.
I don't think we would ever get this breadcrumb though. This is part of the success path so we would not get an exception in the crash reporter.
If we do one day get into the failure path, this breadcrumb would still not be helpful because our subscription.endpoint == oldEndpoint check would give us the old and new state already.
...ccounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| CoroutineScope(Dispatchers.Main).launch { | ||
| account.deviceConstellation().setDevicePushSubscription(subscription.into()) |
There was a problem hiding this comment.
Maybe leave a unique breadcrumb here as well?
This could fail; if it fails, our re-subscription didn't fully work - we have a new endpoint, but it's not recorded on the device record.
If this succeeds, I'm assuming this method will clear the "needs resubscription" flag?
If this fails, what happens? How do we recover?
Next time we fetch our device record, see the bad flag, and attempt again? This sounds like a potential loop to me, without a circuit breaker in case something goes wrong (e.g. maybe there's a sever bug where they don't clear the "needs resub.." flag, and we end up ddossing both autopush and fxa).
It'll be nice to treat resubscription as an atomic operation spanning several services, all of which must succeed in order for us to successfully complete resubscription. We can also take one more step back, and declare resubscription configured to be successful once we receive an fxa push message!
That's definition out of scope here, but I'd suggest thinking about how you'd prevent this bad loop from happening, at least.
This is also a pattern we have in at least one other place (fxa auth loop), which did actually bite us due to a server-side bug (we'd think we've successfully recovered from an auth problem, only to be told by sync that it hit a 401, we'd then recover from an auth problem, only to be told by sync that it hit a 401, etc etc etc...). In that case, we've added circuit breaker logic.
If you think a loop is indeed possible here, can you file a follow-up to consider our options here?
There was a problem hiding this comment.
If this succeeds, I'm assuming this method will clear the "needs resubscription" flag?
Yes, it should do that on the server end when the request goes through.
Next time we fetch our device record, see the bad flag, and attempt again? This sounds like a potential loop to me, without a circuit breaker in case something goes wrong (e.g. maybe there's a sever bug where they don't clear the "needs resub.." flag, and we end up ddossing both autopush and fxa).
It'll be nice to treat resubscription as an atomic operation spanning several services, all of which must succeed in order for us to successfully complete resubscription. We can also take one more step back, and declare resubscription configured to be successful once we receive an fxa push message!
That's definition out of scope here, but I'd suggest thinking about how you'd prevent this bad loop from happening, at least.
This is also a pattern we have in at least one other place (fxa auth loop), which did actually bite us due to a server-side bug (we'd think we've successfully recovered from an auth problem, only to be told by sync that it hit a 401, we'd then recover from an auth problem, only to be told by sync that it hit a 401, etc etc etc...). In that case, we've added circuit breaker logic.
If you think a loop is indeed possible here, can you file a follow-up to consider our options here?
To prevent us from going through this loop and DDOS-ing our own servers, we have the VerificationDelegate to rate-limit ourselves. App services have also re-implemented this internally so we can remove our rate-limiting code in the future as well when it covers this endpoint too.
jonalmeida
force-pushed
the
issue-8846
branch
from
95e2124 to
d121496
Compare
…criptionExpired This is a workaround fix to re-enable Send Tab when we are notified about the subscriptionExpired flag. Until mozilla-mobile#7143 is fixed, we need to avoid calling `registrationRenewal` to avoid going into an unfixable state. In the context of the FxaPushSupportFeature, we know that our end goal is to call `push.subscribe(fxaPushScope)`, so we can by pass the `verifyConnection` call **assuming** that our native push client is always has the latest push token and knows how to invalidate the endpoint with the Autopush server.
jonalmeida
force-pushed
the
issue-8846
branch
from
d121496 to
f323cf6
Compare
jonalmeida
added
🛬 needs landing
This is a workaround fix to re-enable Send Tab when we are notified
about the subscriptionExpired flag. Until #7143 is fixed, we need to
avoid calling
registrationRenewalto avoid going into an unfixablestate.
In the context of the FxaPushSupportFeature, we know that our end goal
is to call
push.subscribe(fxaPushScope), so we can by pass theverifyConnectioncall assuming that our native push client isalways has the latest push token and knows how to invalidate the
endpoint with the Autopush server.
Closes #8846
Pull Request checklist
After merge