codespace: Handle HTTP request retry interruption

[azrsh]

ref: #7819

About

Fix the problem that the process of changing the error message depending on the cause of the HTTP request failure in ConnectToLiveshare did not work well.

Problem

The current ConnectToLiveshare implementation always shows a timeout error message even if the GitHub API returns a 5XX error. This confuses users.

Changes

The process of changing the error message depending on the cause of the HTTP request failure in ConnectToLiveshare was implemented by determining if the error was wrapped with backoff.PermanentError. However, backoff.Retry unwraps it and returns the unwrapped error. Therefore, it cannot be determined by this method.
https://github.com/cenkalti/backoff/blob/a04a6fe64ffb0e3fd0816460529d300be5f252df/retry.go#L93-L96

In order to be able to determine it, define a new error type TimeoutError that represents a timeout, and use this type to determine the cause of the HTTP request failure.

Behavior

With this change, the error message when a request in ConnectToCodespace function fails with a GitHub API's 5XX error will change as follows:

• before: failed to connect to Live Share: timed out while waiting for the codespace to start
• after: failed to connect to Live Share: error getting codespace: error making request: retry

[samcoe]

@azrsh Do you have a before and after screenshot of changed error message? From the code I don't see how this changes anything. Additionally, backoff.PermanentError has an Unwrap() method that likely would make it unnecessary to create the new dedicated error type.

[jkeech]

@samcoe The issue seems to be that the PermanentError is unwrapped inside the retry library before returning, so when the retry finishes, we don't have the context on whether it was a permanent error or not.

https://github.com/cenkalti/backoff/blob/v4/retry.go#L95

I wonder if we can solve this a different way without having to wrap the error in an additional layer though. For instance, instead of assuming everything is a timeout unless we know otherwise, can we invert that logic and look specifically for an error that indicates a timeout (DeadlineExceeded?) to give that nicer timed out while waiting for the codespace to start error message, and then for all other error messages just return the err directly? That way we don't need to check for any sort of PermanentError.

[azrsh]

@\samcoe The issue seems to be that the PermanentError is unwrapped inside the retry library before returning, so when the retry finishes, we don't have the context on whether it was a permanent error or not.

https://github.com/cenkalti/backoff/blob/v4/retry.go#L95

Thanks for the supplement. exactly.

I wonder if we can solve this a different way without having to wrap the error in an additional layer though. For instance, instead of assuming everything is a timeout unless we know otherwise, can we invert that logic and look specifically for an error that indicates a timeout (DeadlineExceeded?) to give that nicer timed out while waiting for the codespace to start error message, and then for all other error messages just return the err directly? That way we don't need to check for any sort of PermanentError.

I didn't come up with a way to do that. They certainly seem to be better.

I've scoured the codebase and it looks like there are existing similar examples. Let's define a new error type representing a timeout in the codespace package to match the name of the structure in this code.

cli/internal/keyring/keyring.go

Lines 10 to 31 in 4a57a81

	type TimeoutError struct {
	message string
	}
	func (e *TimeoutError) Error() string {
	return e.message
	}
	// Set secret in keyring for user.
	func Set(service, user, secret string) error {
	ch := make(chan error, 1)
	go func() {
	defer close(ch)
	ch <- keyring.Set(service, user, secret)
	}()
	select {
	case err := <-ch:
	return err
	case <-time.After(3 * time.Second):
	return &TimeoutError{"timeout while trying to set secret in keyring"}
	}
	}

(This time, I won't go as far as standardizing error types that represents timeouts, but if you want, let me know!)

[jkeech]

Thanks for making the suggested changes! The only other suggestion I would have is to consider updating https://github.com/cli/cli/blob/trunk/internal/codespaces/api/api.go#L1162 to include the HTTP status code that caused the internal retry, that way it gets bubbled up in the final error message that the user sees.

Instead of the user seeing failed to connect to Live Share: error getting codespace: error making request: retry, I think it would be nicer if they see something like failed to connect to Live Share: error getting codespace: error making request: received response with status code 500.

[azrsh]

Do you have a before and after screenshot of changed error message? From the code I don't see how this changes anything.

Added the change error message before and after to the description. Also updated the description with acd3566.

[azrsh]

Thanks for making the suggested changes! The only other suggestion I would have is to consider updating https://github.com/cli/cli/blob/trunk/internal/codespaces/api/api.go#L1162 to include the HTTP status code that caused the internal retry, that way it gets bubbled up in the final error message that the user sees.

Thanks, I think it's a good idea. I accept suggestions and create changes!

[jkeech]

Thanks! The change looks good to me. It would be good to get @samcoe's 👀 as well before merging.

[azrsh]

Thank you for your reviews, @jkeech !
@samcoe please review! I rebased the commit history and squashed the fixup commit, so if you approve, please merge as is!

[samcoe]

Looks great, @azrsh thanks for the contribution and @jkeech thanks for suggesting a better approach!