fix!: Always consume latest registry in updateRegistry#3948
fix!: Always consume latest registry in updateRegistry#3948FrederikBolding merged 5 commits intomainfrom
updateRegistry#3948Conversation
packages/snaps-controllers/src/snaps/registry/SnapRegistryController.ts
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3948 +/- ##
=======================================
Coverage 98.56% 98.56%
=======================================
Files 427 427
Lines 12343 12345 +2
Branches 1939 1939
=======================================
+ Hits 12166 12168 +2
Misses 177 177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mrtenz
left a comment
There was a problem hiding this comment.
One nit, looks good to me otherwise
updateRegistryupdateRegistry
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 621a01a. Configure here.
| this.update((state) => { | ||
| state.databaseUnavailable = true; | ||
| }); | ||
| this.messenger.publish('SnapRegistryController:registryUpdated', false); |
There was a problem hiding this comment.
Infinite loop when registry fetch fails with null database
High Severity
Publishing registryUpdated in the #update() error path creates an infinite loop when the database is null and fetches keep failing. #handleRegistryUpdate() fires, calls SnapRegistryController:get, which calls #getDatabase(), which calls requestUpdate() (because database is still null), which calls #update(), which fails again and publishes registryUpdated again — restarting the cycle. The old stateChange subscription with the ({ database }) => database selector prevented this because a failed fetch doesn't change database. The new unfiltered registryUpdated subscription removes that protection. Additionally, lastUpdated is never set in the error path, so #wasRecentlyFetched() never short-circuits the loop.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 621a01a. Configure here.
BugBot indicated that my previous PR had a infinite loop in case of the registry failure state: #3948 (comment) We do not need to emit `:registryUpdated` on failure, which should fix this problem. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: a small behavioral change to event emission that only affects failure paths, reducing the chance of update-trigger loops when the registry is unavailable. > > **Overview** > Prevents `SnapRegistryController` from publishing the `SnapRegistryController:registryUpdated` event when `#update()` fails (e.g., fetch/verify errors), while still marking `databaseUnavailable` in state. > > Successful updates and no-op updates (recent fetch or unchanged signature) continue to emit `:registryUpdated` as before. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8af7c86. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->


Always consume latest registry in
updateRegistryin an effort to provide a way to retry failed OTA updates without requiring a registry update.Note
Medium Risk
Changes the event contract between
SnapRegistryControllerandSnapController, which affects when preinstalled Snap auto-updates and block/unblock checks run. Risk is moderate because it can alter update frequency/behavior even when the registry database is unchanged or fetch is skipped.Overview
SnapControllernow reacts to a newSnapRegistryController:registryUpdatedevent instead ofstateChange-filtering ondatabase, ensuring#handleRegistryUpdateruns even when the registry fetch is skipped (recently fetched), the signature is unchanged, or an update fails.SnapRegistryControllerpublishesregistryUpdatedwith adatabaseUpdatedboolean on all update paths, and tests/tooling are updated accordingly, including a new test that verifies preinstalled Snap OTA updates can be retried when the registry database itself did not update.Reviewed by Cursor Bugbot for commit 621a01a. Bugbot is set up for automated code reviews on this repo. Configure here.