Skip to content

Conversation

@ppeble
Copy link

@ppeble ppeble commented Jun 24, 2025

Summary

Addresses #188 and #176.

Adds in default disconnect handler behavior to immediately close sessions on 'non-retryable' errors, which for now is just auth errors. This will result in the following output using the quickstart code from the example:

10:28:36 phil@phil-System ngrok-test → go run main.go
2025/06/23 22:28:38 failed to connect: Usage of ngrok requires a verified account and authtoken.

Sign up for an account: https://dashboard.ngrok.com/signup
Install your authtoken: https://dashboard.ngrok.com/get-started/your-authtoken

ERR_NGROK_4018
exit status 1

Other errors will continue to retry indefinitely, as they do today.

Details

I see that there was a large refactor pretty recently. Because of this I did not want to completely redo how the errors were done between the legacy and main package.

This PR adds the NonRetryable interface in the legacy internal package and then tests for that behavior in the syntactic sugar function Retryable(error) in the errors.go in the root directory.

This way we can add additional legacy errors as NonRetryable by adding the appropriate function. When the decision to refactor away the legacy package comes hopefully the Retryable function can be reused and nothing calling it will have to change.

Copy link
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

@ppeble, thanks for the contribution and fixes 🙂

@ppeble
Copy link
Author

ppeble commented Jun 30, 2025

@Alice-Lilith I thought I had run all of these tests correctly locally with a valid auth token but maybe I didn't set the right env vars to run everything. Let me see if I can reproduce the failure locally later this afternoon

@Alice-Lilith
Copy link
Member

@ppeble I think this was just a case where the way the our CI is setup, it doesn't have access to secrets for PRs coming from forks even when approved.

I was just talking to @inconshreveable about this one. He let me know that we actually have a recently created plan to address this issue by adding a new EndpointOption such as func WithRetryStrategy(func...) EndpointOption. This would let users supply their own handler for whether or not a given error should be retried. I've been informed there are users relying on retries for errAuthFailed, first because it can be an error with the initial connection other than just the auth token being missing/wrong, and second because even for auth token specific errors, some users still rely on being able to supply/fix the auth token remotely to restore connectivity to distributed/embeded devices.

Sorry for the runaround. If you're interested in re-implementing this PR that way then we'd still love to accept your contribution. If not, then no worries, one of us will get to this problem soon.

@ppeble
Copy link
Author

ppeble commented Jun 30, 2025

No, not at all! We can close this. As I mentioned I didn't want to do anything drastic because I saw the recent large refactor and figured new stuff would be coming anyways. I'll take a gander at the EndpointOption but don't wait on me if someone is ready to do it.

@ppeble ppeble closed this Jun 30, 2025
@inconshreveable
Copy link
Contributor

very much appreciate the contribution phil! we know it's a pain point and want to resolve it.

btw if you do take a stab at it, it'll be an AgentOption

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.

3 participants