-
Notifications
You must be signed in to change notification settings - Fork 12.7k
Handle partial remote installed plugin failures #25208
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -689,12 +689,49 @@ pub(crate) async fn fetch_remote_installed_plugins( | |
| Ok::<_, RemotePluginCatalogError>((scope, installed_plugins)) | ||
| }; | ||
|
|
||
| let (global, workspace) = tokio::try_join!(global, workspace)?; | ||
| let mut installed_plugins = [global, workspace] | ||
| .into_iter() | ||
| .flat_map(|(_scope, plugins)| plugins) | ||
| .map(|plugin| remote_installed_plugin_to_cache_entry(&plugin)) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| let (global, workspace) = tokio::join!(global, workspace); | ||
|
|
||
| let mut installed_plugins = Vec::new(); | ||
| let mut error_if_all_failed = None; | ||
| let mut any_scope_succeeded = false; | ||
| for (scope, result) in [ | ||
| (RemotePluginScope::Global, global), | ||
| (RemotePluginScope::Workspace, workspace), | ||
| ] { | ||
| match result { | ||
| Ok((_scope, plugins)) => { | ||
| any_scope_succeeded = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When only one remote installed scope is enabled for the client (for example Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is expected
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When one scope succeeds and the other scope returns an error, this marks the whole fetch as successful and returns only the plugins from the successful scope. Both Useful? React with 👍 / 👎. |
||
| for plugin in plugins { | ||
| match remote_installed_plugin_to_cache_entry(&plugin) { | ||
| Ok(plugin) => installed_plugins.push(plugin), | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| scope = scope.api_value(), | ||
| plugin_id = %plugin.plugin.id, | ||
| plugin_name = %plugin.plugin.name, | ||
| error = %err, | ||
| "skipping malformed remote installed plugin" | ||
| ); | ||
|
Comment on lines
+707
to
+714
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a successful scope contains one malformed installed-plugin entry, this branch drops only that entry and still returns Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| scope = scope.api_value(), | ||
| error = %err, | ||
| "failed to fetch remote installed plugins for scope" | ||
| ); | ||
| if error_if_all_failed.is_none() { | ||
| error_if_all_failed = Some(err); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if !any_scope_succeeded && let Some(err) = error_if_all_failed { | ||
| return Err(err); | ||
| } | ||
|
|
||
| installed_plugins.sort_by(|left, right| { | ||
| left.marketplace_name | ||
| .cmp(&right.marketplace_name) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from
try_join!tojoin!means a quick failure from one scope no longer completes the request while the other in-flight scope is still pending. The default remote-plugin reqwest client does not set an application timeout, so during a partial backend outage where (for example)GLOBALreturns 500 butWORKSPACEstalls,plugin/installedcan now block until the transport/OS gives up instead of returning the error promptly as it did before; add a per-scope timeout or other cancellation path before waiting for both results.Useful? React with 👍 / 👎.