Skip to content

fix(multichain-account-service): guard fire-and-forget alignment with ensureReady - #9812

Merged
ccharly merged 8 commits into
mainfrom
cc/fix/ensure-ready-before-locking-wallet-mutex
Aug 10, 2026
Merged

fix(multichain-account-service): guard fire-and-forget alignment with ensureReady#9812
ccharly merged 8 commits into
mainfrom
cc/fix/ensure-ready-before-locking-wallet-mutex

Conversation

@ccharly

@ccharly ccharly commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Explanation

If we try to create a multichain account wallet during onboarding, the post-alignment part (e.g. non-EVM account creations) is locking this wallet's lock until onboarding is completed.

This prevents any other kind of operations on that wallet that requires the wallet's mutex to be locked again.

This is what we do now for the QR sync wallet during onboarding, like:

  • 1 - QR sync payload is received
  • 2 - The primary SRP gets created (onboarding still pending)
    • Post-alignment is scheduled and LOCKS the wallet's mutex (non-EVM creation part, since waitForAllProvidersToFinishCreatingAccounts=false)
  • 3 - Password is set (onboarding still pending)
    • Vault got created now, we proceed with the rest of QR sync flow (remaining wallets + importing metadata)
  • 4 - We try to import wallet's metadata for the primary wallet
    • 💥 Wallet's mutex is already LOCKED (during step 2), the onboarding cannot proceed -> deadlock 💥

To prevent this from happening, we now guard every post-alignment with a call to ensureReady on each providers before proceeding. This makes sure we only schedule wallet's operation once the providers are ready to proceed anything.

In the case of the SnapAccountProvider, this means they will wait for the Snap platform to boot up before starting to lock the wallet's mutex, solving the initial issue "naturally".

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes wallet locking order during group creation—a sensitive concurrency path—but scope is limited to deferred post-alignment and includes tests for lock timing and partial provider failure.

Overview
Fixes a deadlock during onboarding when non-EVM post-alignment ran under the wallet mutex while Snap providers were still waiting for the platform—blocking later steps such as QR sync metadata import.

Fire-and-forget post-alignment (when waitForAllProvidersToFinishCreatingAccounts is false) now calls ensureReady on all non-EVM providers before acquiring the alignment lock. Only providers that become ready are aligned; failures are logged and skipped so a later explicit alignment can recover them.

The ensureReady hook is added to the BIP-44 provider contract (no-op on base/EVM, delegated through AccountProviderWrapper, Snap waits via SnapAccountService:ensureReady).

Reviewed by Cursor Bugbot for commit 954eab5. Bugbot is set up for automated code reviews on this repo. Configure here.

@ccharly
ccharly marked this pull request as ready for review August 10, 2026 10:01
@ccharly
ccharly requested review from a team as code owners August 10, 2026 10:01
Comment on lines +750 to +756
// Ensure the Snap platform is ready for every non-EVM provider BEFORE
// acquiring the wallet lock. Without this guard the lock would be held
// while waiting for onboarding to complete, blocking all subsequent
// wallet operations that also need the lock.
await Promise.all(
otherProviders.map((provider) => provider.ensureReady()),
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we scope this fix only for this code-path.

We could use ensureReady on other code-paths wherever we need a provider, but most calls are already doing that. This one is a bit more different than the other since it's a fire-and-forget and might hold the lock asynchronously (which can be held forever during onboarding, see the PR's description for the flow).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use promise.allsettled? If one provider fails, it would fail for the alignment of the other providers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question 🤔 I guess we can turn that into a "best-effort" operation in that case. We do have explicit alignments to also address this kind of failure and be resilient anyway.

That makes sense IMO, let me try to do that!

Thanks 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

108a7cb

In the normal case, this will never fail. But at least, now it's even more resilient, thanks!

@ccharly
ccharly enabled auto-merge August 10, 2026 10:44
@ccharly
ccharly added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit 5351c9f Aug 10, 2026
148 checks passed
@ccharly
ccharly deleted the cc/fix/ensure-ready-before-locking-wallet-mutex branch August 10, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants