glitch-soc / mastodon

A glitchy but lovable microblogging server
https://glitch-soc.github.io/docs/
GNU Affero General Public License v3.0
706 stars 182 forks source link

[Vanilla?] Grouped notifications of type 'status' for deleted statuses fail #2802

Open kescherCode opened 3 months ago

kescherCode commented 3 months ago

Steps to reproduce the problem

  1. Have a notification for a status that is deleted
  2. Have "New posts" on in notification filters (vanilla or glitch, doesn't matter)
  3. JavaScript error will occur

Expected behaviour

Notifications load without any issue

Actual behaviour

Oops! An unexpected error occurred.

Detailed description

No response

Mastodon instance

catcatnya.com, cts.kescher.at, local dev container with artificial data

Mastodon version

commit 32717657ceb9898e9d502a87043b5716e86bd67c, but probably occurs since initial implementation

Browser name and version

Recent versions of both Chromium and Firefox

Operating system

Linux, Android

Technical details

Error is:

TypeError: Cannot read properties of null (reading 'id')
    at e (index.js:64:95)
    at Array.forEach (<anonymous>)
    at index.js:84:14
    at redux-thunk.legacy-esm.js:5:14
    at dispatch (redux.legacy-esm.js:371:38)
    at f (notification_groups.ts:62:5)
    at notification_groups.ts:79:5
    at typed_functions.ts:197:28
    at async typed_functions.ts:66:16
    at async createAsyncThunk.ts:336:13

Leading me to this line: https://github.com/glitch-soc/mastodon/blob/c72b6e03ec6b4042dbf958a979b02cc6870128a9/app/javascript/flavours/glitch/actions/importer/index.js#L64

Here is a redacted JSON object that's part of the response of /api/v2_alpha/notifications?exclude_types[]=follow_request&exclude_types[]=favourite&exclude_types[]=update&exclude_types[]=reaction&exclude_types[]=mention&exclude_types[]=follow&exclude_types[]=admin.report&exclude_types[]=reblog&exclude_types[]=admin.sign_up&exclude_types[]=poll:

{
  "group_key": "ungrouped-1177432",
  "notifications_count": 1,
  "type": "status",
  "most_recent_notification_id": 1177432,
  "page_min_id": "1177432",
  "page_max_id": "1177432",
  "latest_page_notification_at": "2024-07-31T10:51:42.901Z",
  "sample_accounts": [
    {
      "id": "1337",
      "username": "redacted",
      "acct": "redacted",
      "display_name": "redacted",
      "locked": true,
      "bot": false,
      "discoverable": false,
      "indexable": false,
      "group": false,
      "created_at": "1970-01-01T00:00:00.000Z",
      "note": "redacted",
      "url": "redacted",
      "uri": "redacted",
      "avatar": "redacted",
      "avatar_static": "redacted",
      "header": "redacted",
      "header_static": "redacted",
      "followers_count": -1,
      "following_count": -1,
      "statuses_count": -1,
      "last_status_at": "1970-01-01",
      "hide_collections": true,
      "noindex": true,
      "emojis": [],
      "roles": [],
      "fields": [
        {
          "name": "redacted",
          "value": "redacted",
          "verified_at": null
      ]
    }
  ],
  "status": null
}

Note that usually, status is populated.

kescherCode commented 3 months ago

Now, the question is whether status being null is the bug here, or such null statuses not being filtered from notifications

kescherCode commented 3 months ago

Given null checks are present for other notification types, I'd say it's the latter.

kescherCode commented 3 months ago

Manually deleting all "status" type notifications where no status exists is a working workaround, too. There were over 5000 such notifications in the database since catcatnya.com has started to exist.

ClearlyClaire commented 3 months ago

Can you retry with the last update? I suspect it behaves better. But it sounds like there's something weird going on. We technically expect some orphaned notifications for various reasons (basically polymorphism and race conditions), so it is good to guard against null statuses in the clients, but over 5k seems a bit much.

kescherCode commented 3 months ago

but over 5k seems a bit much.

5k notifications since 2022, for the entire userbase of 150+ monthly active users. I think that's not that bad. There are worse issues in Mastodon.

Can you retry with the last update?

As production yields no new such notifications since my report (since no one rapid fire posted-and-deleted), I've gone ahead and injected a fake entry to bring back the issue (and then, just as I did that, someone managed to actually do it again, causing two entries for two users, one of those being me), then upgraded. Result: Notifications no longer result in an error, so it no longer breaks. However, there is an empty notification whenever the status can't be found:

An empty status in-between two "just posted" notifications

Btw, here's a quick query to run to get any such notifications:

select * from notifications n where n.type='status' and not exists (select s.id from statuses s where n.activity_id = s.id);
kescherCode commented 3 months ago

If you think the empty notification is not a big deal, feel free to close the bug.

ClearlyClaire commented 3 months ago

It's not a very big deal but it's still a bug that needs to be investigated, at the very least on the display side.

ClearlyClaire commented 3 months ago

I think it has been addressed with recent changes, although the underlying cause still needs to be investigated