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

[Bug]: Notifications - Some accounts are loading and get the error Failed to check accounts presence Error: Exceeds maximum number of requests waiting to be resolved, please try again. #25749

Closed
seaona opened this issue Jul 10, 2024 · 8 comments
Labels
regression-beta-12.0.0 Regression bug that was found in beta in release 12.0.0 regression-RC-12.0.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-notifications Notifications team type-bug

Comments

@seaona
Copy link
Contributor

seaona commented Jul 10, 2024

Describe the bug

Whenever I enable the notifications, when I go to the notifications settings, I see how some of the accounts remain with the loading spinner forever, and I can see this error in the background console Failed to check accounts presence Error: Exceeds maximum number of requests waiting to be resolved, please try again.

Expected behavior

No response

Screenshots/Recordings

Screenshot from 2024-07-10 18-12-01

fail-check-actts-presence.mp4

Steps to reproduce

  1. Create several accounts
  2. Enable notifications
  3. Go to notification settings
  4. See last accounts are forever loading and there is the background error

Error messages or log output

No response

Version

12.0.0

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response

@sleepytanya
Copy link
Contributor

sleepytanya commented Jul 10, 2024

Confirming the same behavior on v12.1.0 although it doesn't happen every time, sometimes accounts are loaded without any issues:

spinner12-1-0.mov
Screenshot 2024-07-10 at 18 15 55

@Prithpal-Sooriya
Copy link
Contributor

TY. Jira Ticket created here. Our team will investigate.

@Prithpal-Sooriya
Copy link
Contributor

This is interesting. Seems to come from queued requests to the snap controller. Our team will better manage/cache responses back from the pre-installed snap we are using (we don't need to realistically call this every time)

@gauthierpetetin gauthierpetetin added Sev2-normal Normal severity; minor loss of service or inconvenience. Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. and removed Sev2-normal Normal severity; minor loss of service or inconvenience. labels Jul 15, 2024
@sleepytanya sleepytanya added the regression-beta-12.0.0 Regression bug that was found in beta in release 12.0.0 label Jul 16, 2024
@sleepytanya
Copy link
Contributor

sleepytanya commented Jul 16, 2024

Present in v12.0.0 beta:

Screenshot 2024-07-15 at 21 40 49 Screenshot 2024-07-15 at 21 36 39

@sleepytanya
Copy link
Contributor

I think it's the same bug because I see the same console messages when Notifications list appears empty:

empty.mov
Screenshot 2024-07-16 at 23 52 56 Screenshot 2024-07-16 at 23 54 20

@Prithpal-Sooriya
Copy link
Contributor

For visibility, it seems that we make quite a few calls to the preinstalled snap we are using, leading to a large request queue (and errors being thrown).

A fix is made in the core library (we are in process of migrating extension to using the core library)
MetaMask/core#4532

I can also backport PR to the main extension

@danjm danjm added release-blocker This bug is blocking the next release and removed regression-RC-12.1.0 labels Jul 18, 2024
Prithpal-Sooriya added a commit that referenced this issue Jul 19, 2024
## **Description**

This is a series of fixes added to core libraries. Adding to extension,
and soon we will migrate extension to use shared libraries.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25946?quickstart=1)

## **Related issues**

MetaMask/core#4530
MetaMask/core#4531
MetaMask/core#4532
MetaMask/core#4533
MetaMask/core#4536

#25749

## **Manual testing steps**

1. Create multiple accounts
2. Go to notification settings page
3. Wait for settings to load, and try toggling notifications

Before: some settings would error or not load
After: you should be able to toggle accounts and not see errors.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

[<!-- [screenshots/recordings]
-->](#25749)

### **After**


https://www.loom.com/share/49b582e8c33b4199bdafc994a3f6087f?sid=9f94a885-351b-4fee-84d8-f72c97506e7d

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Prithpal-Sooriya added a commit that referenced this issue Jul 19, 2024
## **Description**

This is a series of fixes added to core libraries. Adding to extension,
and soon we will migrate extension to use shared libraries.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25946?quickstart=1)

## **Related issues**

MetaMask/core#4530
MetaMask/core#4531
MetaMask/core#4532
MetaMask/core#4533
MetaMask/core#4536

#25749

## **Manual testing steps**

1. Create multiple accounts
2. Go to notification settings page
3. Wait for settings to load, and try toggling notifications

Before: some settings would error or not load
After: you should be able to toggle accounts and not see errors.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

[<!-- [screenshots/recordings]
-->](#25749)

### **After**


https://www.loom.com/share/49b582e8c33b4199bdafc994a3f6087f?sid=9f94a885-351b-4fee-84d8-f72c97506e7d

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
chloeYue pushed a commit that referenced this issue Jul 19, 2024
)

## **Description**

This is a cherry pick for
#25946
(003ee98)

------

This is a series of fixes added to core libraries. Adding to extension,
and soon we will migrate extension to use shared libraries.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25946?quickstart=1)

## **Related issues**

MetaMask/core#4530
MetaMask/core#4531
MetaMask/core#4532
MetaMask/core#4533
MetaMask/core#4536

#25749

## **Manual testing steps**

1. Create multiple accounts
2. Go to notification settings page
3. Wait for settings to load, and try toggling notifications

Before: some settings would error or not load
After: you should be able to toggle accounts and not see errors.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

[<!-- [screenshots/recordings]
-->](#25749)

### **After**


https://www.loom.com/share/49b582e8c33b4199bdafc994a3f6087f?sid=9f94a885-351b-4fee-84d8-f72c97506e7d

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@danjm danjm closed this as completed Jul 22, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jul 22, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Jul 22, 2024
@benjisclowder
Copy link
Contributor

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

1. What PR fixed the issue?
2. Can you pinpoint the commit from which the issue originated?
3. Write a short explanation of the technical cause of the bug
4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?

4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @Prithpal-Sooriya

@Prithpal-Sooriya
Copy link
Contributor

@benjisclowder

  1. What PR fixed the issue?

#25946

  1. Can you pinpoint the commit from which the issue originated?

This is hard to pinpoint, as this was a new feature, not an existing feature.

  1. Write a short explanation of the technical cause of the bug

Commit for the fix: 5f76720

When a user has many accounts, we were making many API calls. Also we were making many Snap requests. The snap requests especially were being queued and then failing. Reducing this to only make 1 call at the page level and caching resolved this issue.

  1. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?

We should have tested with multiple accounts (10+ addresses). We never hit this snap request issue during development.

4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?

Unit tests were covered, but the Snap Environment was mocked.

Integration tests (using the snap environment), or E2E may have spotted this - however it was a flakey issue.

More manual testing (at least during development) may have raised this early too.

4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?

Not particularly, most of the knowledge base around notifications were known.
The only unknown was the request queue the Snap Controller contains.

4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-beta-12.0.0 Regression bug that was found in beta in release 12.0.0 regression-RC-12.0.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-notifications Notifications team type-bug
Projects
Archived in project
Development

No branches or pull requests

6 participants