fix: handle rejected analytics request in WalletProvider.trackInitialization - #1408
Open
shlee-lab wants to merge 1 commit into
Open
fix: handle rejected analytics request in WalletProvider.trackInitialization#1408shlee-lab wants to merge 1 commit into
shlee-lab wants to merge 1 commit into
Conversation
🟡 Heimdall Review Status
|
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.
Description
WalletProvider.trackInitialization()wrapssendAnalyticsEvent()in a try/catch, but the call isn't awaited.sendAnalyticsEventisasyncand throws on a non-ok response:A synchronous try/catch can't see that rejection, so it escapes to the top level. Node has terminated the process on unhandled rejections by default since v15, so a failed analytics request takes the host application down about a second after the wallet provider is constructed.
It only shows up when the analytics request actually fails, which is probably why it hasn't come up on the usual CDP/Base setups. I ran into it on a custom EVM chain, where
cca-lite.coinbase.comreturns 400 for the unrecognisednetwork_id. Anyone running offline or behind a firewall that blocks the endpoint should hit the same thing, sincefetchrejects there too.Observed on 0.10.4 in an MCP server built on AgentKit. The server finishes startup and answers
initialize, then exits with:Running the same binary with
NODE_OPTIONS=--unhandled-rejections=warnkeeps it alive, which confirms the rejection is what kills it.The fix attaches
.catch()so the existing warn-and-continue path actually runs. I kept the synchronous try/catch becausegetName(),getAddress()andgetNetwork()are still called synchronously and can throw.The Python implementation isn't affected.
send_analytics_eventis synchronous there, so itstry/exceptalready works.Tests
Added
should handle tracking rejections gracefullytowalletProvider.test.ts. It mockssendAnalyticsEventto reject, then checks that the warning is logged and that nounhandledRejectionfires.Reverting the source change makes the new test fail, so it covers the actual regression:
The file's suite passes 5/5.
eslint,tsc --noEmitandprettier --checkare clean on the touched files.