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.

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

API to subscribe to push notification does not seem to delete previous subscription? #30103

Closed
vincentneo opened this issue Apr 28, 2024 · 10 comments · Fixed by #30166
Closed
Labels
api REST API, Streaming API, Web Push API bug Something isn't working status/identified This bug has been identified

Comments

@vincentneo
Copy link

Steps to reproduce the problem

  1. POST /api/v1/push/subscription many times. (Not sure if many times in a short time matters though, just stating based on what I (accidentally) did)
  2. Send a post via another account, tagging the logged in user.
  3. Wait for notification to arrive
  4. Realised that tons of notifications came flooding in, only one decoded correctly (non unexpected, since my app only saved the latest keys)

Expected behaviour

1 notification with correct contents.

Actual behaviour

hundreds? of notifications.

Detailed description

Now, I am not entirely sure if the fault is on my end, but I am somewhat confident that it is the API's fault. Please advise if I'm doing something terribly wrong without noticing 😅

I am referring to the POST /api/v1/push/subscription endpoint.
So the story here is that, new (unreleased) app here, where a bug occurred, which resulted in an endless loop, causing the app to accidentally send out many requests.

I realised this probably after like 2 seconds, shut the app down, fixed the issue, and relaunched the app.

I sent a post out using another account, tagging my app's logged in account, the rest is explained in the steps to reproduce section.

The documentation for that, states:

If you create a new subscription, the old subscription is deleted.

In this case, it didn't seem to be true? Eventually, I also tried calling DELETE /api/v1/push/subscription, but symptoms remains.

My setup is for a watchOS app. Notifications are relayed using metatext-apns, which by the looks of it is just a simple relay so that shouldn't be the cause, right?

I am very new to this Mastodon/Web Push notifications stuff, so please let me know if you believe it's an issue from my end. Thank you!

Mastodon instance

mastodon.social

Mastodon version

v4.3.0-nightly.2024-04-18

Technical details

  • watchOS 10.2
  • Both accounts are on mastodon.social
@vincentneo vincentneo added bug Something isn't working status/to triage This issue needs to be triaged labels Apr 28, 2024
@vincentneo
Copy link
Author

I've realise today that if I were to call DELETE followed by GET, the GET response will always contain a different id (though not sequential).
If I were to stop calling DELETE, and only call GET, the id stays the same.

My guess is that:

  1. DELETE deletes only the (latest?) push notification subscription, and in my case of having erroneous multiple subscriptions meant that a way to solve it could be to call DELETE multiple times?
  2. It seems to disprove the statement "If you create a new subscription, the old subscription is deleted." since it the above behaviour with the different ids, seems to show that older subscriptions appears to have persisted.
  3. There can be more than one push subscription associated with an access token, of which all subscriptions will be notified whenever an action that will result in a notification occurs.

Of course, typically no one would call the subscribe endpoint in the same way that I accidentally did, though I think this makes for a point that something somewhere is broken.

Also, I did not test this against if I only call POST twice, or of a lesser extent, so I am unsure if the amount of calls in a short period matters in this case or not.

@trwnh
Copy link
Member

trwnh commented Apr 29, 2024

the line in the docs came from this:

before_action :set_push_subscription

and

def set_push_subscription
@push_subscription = Web::PushSubscription.find_by(access_token_id: doorkeeper_token.id)
end

and

def create
@push_subscription&.destroy!

so maybe it's not finding the old web push subscription based on the access token id?

@vincentneo
Copy link
Author

so maybe it's not finding the old web push subscription based on the access token id?

What's odd is if thats the case, the behaviour with DELETE shouldn't be observed at all

@trwnh trwnh added api REST API, Streaming API, Web Push API and removed status/to triage This issue needs to be triaged labels Apr 29, 2024
@ClearlyClaire
Copy link
Contributor

As @trwnh pointed out, there should only be one push subscription per access token, and creating an access token should remove the previous one.

I've realise today that if I were to call DELETE followed by GET, the GET response will always contain a different id (though not sequential).
If I were to stop calling DELETE, and only call GET, the id stays the same.

GET does not generate a new push subscription, POST does, so calling GET multiple times would not change the ID.

At this point, I don't have an explanation for what you're observing. Since you are using mastodon.social, do you mind sharing the account with which you are experiencing the issue, and give us your permission to look into the database to see what push subscriptions are currently registered?

@vincentneo
Copy link
Author

vincentneo commented May 2, 2024

GET does not generate a new push subscription, POST does, so calling GET multiple times would not change the ID.

I guess I could have explained it better. I am not trying to say that GET isn't generating a subscription (I well understand its not suppose to and isn't doing so), rather the current behaviour seems to show DELETE works (for deletion of a random subscription), but there are indeed more than one subscriptions, evident by what I am seeing from the DELETE, GET and returned ids.

My account is @vincentneo@mastodon.social, and I am ok with y'all checking in the DB for this, thank you!

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented May 2, 2024

We're seeing a total of 74 push subscriptions with your account, with 65 of them associated to your application… and among those, only 3 different access tokens… so something is indeed up, with multiple subscriptions being created for the same token, though I don't understand how this is possible… we even have a test for this behavior

@ClearlyClaire
Copy link
Contributor

Oh, I missed the fact that they all seem to have been created approximately at the same time. So we're having a race condition where multiple calls created the subscription at the same time!

@ClearlyClaire ClearlyClaire added the status/identified This bug has been identified label May 2, 2024
@vincentneo
Copy link
Author

So we're having a race condition where multiple calls

Yeah, that is very likely the case here.

By the way, can you check if in the db if there's records on any access token having only like 2/3 push subscriptions, rather than the extreme large amount of 65? I recall a while back seeing duplicated notifications of a smaller scale (2/3 duplicates, rather than the large number), and check if it created at around the same time? I may have revoked the token though, so unsure if it's still there.

@ClearlyClaire
Copy link
Contributor

There was just a token with 63 registrations afaik. We'll fix this, in the meantime you should be able to just call DELETE repeatedly with the same access token.

@vincentneo
Copy link
Author

Alright thanks @ClearlyClaire!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api REST API, Streaming API, Web Push API bug Something isn't working status/identified This bug has been identified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants