fix(web-wallet): de-flake E2E cold-start connect (#177 item 2b) - #181
Open
raul-oliveira wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The dominant flake in the real-MetaMask E2E suite is the cold-start of MetaMask's MV3 service
worker plus the Snap's first heavy op. On its first RPC the Snap starts a read-only Hathor wallet
(a wallet-service round-trip). The dApp's own connect path races that with a
NETWORK_CHECK(
htr_getConnectedNetwork, capped bySNAP_TIMEOUT_MS); if the check fires while the cold-start isstill in flight, connect throws "Snap is not responding" and the journey never reaches the
connected home (empirically ~2 pass / 3 fail on a slow headless box — issue #177 item 2).
helpers/journeys.tsalready warmed the Snap out-of-band (warmSnap) before the timed connect, butthe warm-up was best-effort: the provider/worker waits used fixed try-counts and the final
read-only invoke (
htr_getConnectedNetwork) was fire-and-forget (.catch(() => undefined)). SowarmSnapcould return while the read-only wallet was still starting — leaving the very race thedApp's timed check then loses.
This PR (decision D1 in the #177 design) converts
warmSnapinto a success-gated readinessprobe: it does not return until every stage has actually succeeded, each bounded by a real
deadline (not a fixed try-count) — (a) provider injected, (b)
wallet_getSnapsresolves (MV3 workerawake), (c)
wallet_requestSnapsresolves (Snap installed, retried on a cold-worker reject), (d) areal
htr_getConnectedNetworkinvoke resolves (read-only wallet finished starting). Stage (d) — theexact call the dApp's timed check would otherwise race — is now awaited instead of fire-and-forget.
New budgets live in the centralized
TIMEOUTS.probe(from PR 1), soE2E_TIMEOUT_SCALEstill scalesthem.
This is PR 2 of the #177 follow-up sequence and closes item 2b. It stacks on PR 1
(#178, base branch
raul-oliveira/feat/web-wallet-e2e-followups-pr1), which relocated the sharedtimeouts module; GitHub auto-retargets this PR to
masteronce PR 1 merges. Item 2a shipped in PR 1;the nightly CI (item 2c) is PR 3. No production runtime code changes — only the E2E harness.
Acceptance Criteria
warmSnapis success-gated: it returns only after stages (a)–(d) each resolve within theirdeadline; the previously fire-and-forget
htr_getConnectedNetwork(stage d) is now awaited.TIMEOUTS.probe(wrapped inms(), plain numbers so theysurvive
page.evaluate), scaling withE2E_TIMEOUT_SCALE;SNAP_ID,connect,switchNetwork,and the
provision*helpers are unchanged.yarn workspace @hathor/web-wallet lintis clean.yarn e2e --project=onboarding, the webServer'sSNAP_TIMEOUT_MS=60000cap) passes:9/9 with the probe applied.
--retries=0, cold every run, first-attempt pass rate).To make the cold-start race observable, the dApp was driven with a production-like
SNAP_TIMEOUT_MScap far below the webServer's 60000, servers booted manually and reused by Playwright. Each run uses
a fresh MetaMask seed → fresh xpub → the wallet-service is cold per run, so warming is per-run and
the comparison is clean:
SNAP_TIMEOUT_MS=5000— baseline (pristine) 8/8 vs after (probe) 6/6. No regression at aviable cap.
SNAP_TIMEOUT_MS=6000baseline 8/8;SNAP_TIMEOUT_MS=8000baseline 8/8 — this box is fastenough that the best-effort background warm-up already completes before connect, so the flake does
not reproduce at these caps.
SNAP_TIMEOUT_MS=4000— baseline flakes 4/8, but this cap sits below the Snap's warmread-only-wallet floor:
getAndStartReadOnlyHathorWallet(packages/snapsrc/utils/wallet.ts)constructs a fresh wallet and calls
startReadOnly()on every RPC, so even a fully warmedwallet-service still costs a fixed round-trip that exceeds 4000ms. Neither best-effort nor the
probe can win there (probe 0/5), so 4000 is not a valid comparison point.
baseline flakes (4000) it is below the warm floor, and where the probe could help (≥5000) the box
already passes 8/8. The probe is still correct: it removes the theoretical race by
success-gating stage (d) (fire-and-forget → awaited), which strictly dominates on a genuinely slow
box (the issue's headless 2-pass/3-fail environment) where the cold-start greatly exceeds the warm
floor. Traced failures confirmed the exact mechanism: "Network check timeout" → "Snap is not
responding" at
useWalletConnection.ts(thehtr_getConnectedNetworkNETWORK_CHECK).Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged