fix: stalled connection in CI #1758
Merged
Conversation
This file contains 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
# Conflicts: # package-lock.json # packages/core/package.json # packages/sign-client/package.json
xzilja
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
bkrem
reviewed
Dec 14, 2022
arein
approved these changes
Dec 14, 2022
| await publish; | ||
| } catch (err) { | ||
| this.logger.debug(`Publishing Payload stalled`); | ||
| this.relayer.events.emit(RELAYER_EVENTS.connection_stalled); |
There was a problem hiding this comment.
do you need to inspect the error here to tell for sure it's the connection_stalled one?
There was a problem hiding this comment.
This event is emitted when the publish request fails to resolve within the expiry (currently 10s). In normal circumstances, the expiry should be more than enough for a request to complete. If it doesn't, it is safe to assume the connection stalled. Also, the error here comes from the expiry rejecting the promise, not an actual network error
| @@ -12,7 +12,7 @@ import { describe, it, expect, afterEach } from "vitest"; | |||
|
|
|||
| const environment = process.env.ENVIRONMENT || "dev"; | |||
|
|
|||
| const timeout = environment === "prod" ? 610_000 : 70_000; | |||
| const timeout = environment === "prod" ? 610_000 : 30_000; | |||
There was a problem hiding this comment.
feel free to link this against the server config https://github.com/WalletConnect/rs-relay/blob/master/terraform/vars/staging.tfvars#L9-L11
…sible race conditions
bkrem
approved these changes
Dec 16, 2022
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
Running
sign-clienttests in GHA has been problematic for a while due to socket connections issues such assocket hang up<- inability to capture the error causing the CI run to failstalled connections<- A connection that is seeminglyactivein the SDK but on the relay, it has been closed with104 connection reset by peerStalled connections/104 errors are happening on the client machine and are raised by the OS when it doesn't recognize an incoming packet. This can happen by any firewall/router etc and since we have no control over GHA runners a solutions is needed to handle such scenarios
Worth noting that the issues are so far only reproducible during CI runs and not during normal SDK usage or local runs
Steps to increase tests stability
1. Reducing overhead
Added
/integrationfolder where full e2e test coverage is available without unnecessary duplicate tests and/or redundant pairings. We have 5+pingtests that are testing the same thing. Moved the validation tests out of the integration pipeline as they don't test the relay, just create overhead connections2. Better catching connection errors
Updated the
@walletconnect/jsonrpc-ws-connectionpackage to handle connection errors, also implemented reconnection onsocket hang upso we can continue processing instead of failing the run3. Proper disposal of connections after a test is done
Proper closing of connections and removing event listeners/heartbeats after a test is done
4. Handling stalled connections
Added expiry timeout on subscrptions/payloads requests where if the request expires a reconnection is attempted. From the test runs, I'm seeing that a reconnection might attempted several times before it is restored and active again.
The mechanism for stalled requests works by utilizing the
queuewhere the request is removed from the queue only after successful deliveryHow Has This Been Tested?
npm run test
tons of CI runs
canary has been running this code for ~2 weeks
Due Dilligence