Skip to content

[registry facade] Don't retry requests with error messages that were successful #20889

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 1 commit into from
Jun 9, 2025

Conversation

geropl
Copy link
Member

@geropl geropl commented Jun 9, 2025

Description

Follow up to #20880: That PR re-tried all requests, even if it was a 404.

This PR introduces a retryableError function that excludes non-retryable errors from being retried, fixing this test.

Related Issue(s)

Fixes CLC-1419

How to test

  • test are green ✔️

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft preemptible
    Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

Copy link
Member

@Siddhant-K-code Siddhant-K-code left a comment

Choose a reason for hiding this comment

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

Approval to unblock

Comment on lines +607 to +612
if strings.Contains(err.Error(), "not found") ||
strings.Contains(err.Error(), "invalid argument") ||
strings.Contains(err.Error(), "not implemented") ||
strings.Contains(err.Error(), "unsupported media type") {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, non-blocking:
@geropl how did you come up with this list? I ask because depending on the answer, we might want to include others like to cover scenarios like 401 (Unauthorized) and 403 (Forbidden).

Double checking workspace start prior to adding approval.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kylos101 Yeah, good call. After sleeping over it, I think it should have been the reverse: default to false, and only retry the exact errors we saw as 500 in the logs. Might even be worth swapping that now... 🤔

@roboquat roboquat merged commit 3b1f988 into main Jun 9, 2025
50 checks passed
@roboquat roboquat deleted the gpl/CLC-1419-build branch June 9, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants