Skip to content

feat: allow backend errors to be handled seperately #1167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 27, 2025

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Apr 27, 2025

Problem

Backend error messages are expected to be shown directly to the customer in chat. However, there are other error messages we don't want to show, and should show a generic message.

Solution

  • Implement an error wrapper with a code field, that allows each type of error to have its own handling.
  • Expand reach of try block such that any internal errors surfaced are caught and returned as ResponseError. This also makes this easier to test.

Testing and Verification

  • updated a unit test, and added another for generic behavior.
  • Backend error (replacing backend call with an error)
backendErrorDemo.mov
  • generic error (throwing an error elsewhere in the agentic loop)
unknownError.mov

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock Hweinstock marked this pull request as ready for review April 27, 2025 21:56
@Hweinstock Hweinstock requested a review from a team as a code owner April 27, 2025 21:56
Copy link

@velatha velatha left a comment

Choose a reason for hiding this comment

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

Is it possible to add a bit more context than just "'This is an error from the backend'"

@Hweinstock
Copy link
Contributor Author

That was a demo message I threw in there to demonstrate, an actual error from the backend would display its message there. I can try to produce an ISE to show what this would look like.

// Show backend error messages to the customer.
if (err.code === 'QModelResponse') {
this.#features.logging.error(`QModelResponse Error: ${JSON.stringify(err)}`)
return new ResponseError<ChatResult>(LSPErrorCodes.RequestFailed, err.message, {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a special check here to show this message when statusCode is 500?
https://github.com/aws/aws-toolkit-vscode/pull/6984/files

Copy link
Contributor Author

@Hweinstock Hweinstock Apr 27, 2025

Choose a reason for hiding this comment

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

Is it safe to assume 500 is throttling? Isn't this also ISE?

Edit: I was able to get a 500 below unreleted to throttling when I passed {} as the args.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right, right now this is a gap in our system where we couldn't differentiate bedrock throttling and real ISE. Given the fact that we just got a lot of capacity and we fallback to 3.5, the chance of real throttling happening is low so maybe it's fine to keep this as it is for now

@Hweinstock
Copy link
Contributor Author

@velatha Internal failure error demo:

ISE-Error.mov

@Hweinstock Hweinstock requested a review from velatha April 27, 2025 22:26
@Hweinstock Hweinstock merged commit 4c828d4 into aws:main Apr 27, 2025
6 checks passed
@Hweinstock Hweinstock deleted the errorRefactor branch April 27, 2025 22:38
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.

4 participants