Handle plugin marketplace pull recovery#318270
Open
aaronpowell wants to merge 5 commits into
Open
Conversation
Use state-based pull failover for plugin marketplace repos: retry ff-only pull after fetch, then hard-reset only for clean, truly diverged repos. Also add a user-facing purge-and-reclone recovery action when update still fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves resilience of plugin marketplace updates by adding recovery options in the UI and making git pulls more robust (retrying after fetch and optionally recovering from diverged histories).
Changes:
- Add a notification action to purge the marketplace cache and reclone when plugin update fails.
- Enhance
LocalGitService.pull()with a fetch + retry strategy and optional hard-reset recovery when history is diverged and the working tree is clean. - Add targeted unit tests covering the new
pull()recovery behaviors.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/agentPluginRepositoryService.ts | Adds “Purge Marketplace Cache and Reclone” action and implements purge+reclone workflow with progress + notifications. |
| src/vs/platform/git/node/localGitService.ts | Implements pull retry after fetch and (in specific cases) hard reset to upstream when repo is diverged. |
| src/vs/platform/git/test/node/localGitService.test.ts | Adds test coverage for pull retry and diverged-history recovery scenarios. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
Comment on lines
+53
to
+70
| try { | ||
| await this._exec(operationId, ['pull', '--ff-only'], repoPath); | ||
| } catch (err) { | ||
| this._logService.warn(`[LocalGitService] Fast-forward pull failed for ${repoPath}. Retrying after fetch.`); | ||
| await this._exec(operationId, ['fetch', '--prune'], repoPath); | ||
|
|
||
| try { | ||
| await this._exec(operationId, ['pull', '--ff-only'], repoPath); | ||
| } catch (retryErr) { | ||
| const upstream = await this._getSafeHardResetTarget(operationId, repoPath); | ||
| if (!upstream) { | ||
| throw retryErr; | ||
| } | ||
|
|
||
| this._logService.warn(`[LocalGitService] Pull retries exhausted for ${repoPath}. Performing hard reset to ${upstream}.`); | ||
| await this._exec(operationId, ['reset', '--hard', upstream], repoPath); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I think this will be fine as this code path isn't intended for use by the user.
|
|
||
| private async _revListCount(operationId: string, repoPath: string, fromRef: string, toRef: string): Promise<number> { | ||
| const result = await this._exec(operationId, ['rev-list', '--count', `${fromRef}..${toRef}`], repoPath); | ||
| return Number(result.trim()) || 0; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+50
to
+70
| async pull(operationId: string, repoPath: string): Promise<boolean> { | ||
| const before = (await this._exec(operationId, ['rev-parse', 'HEAD'], repoPath)).trim(); | ||
| await this._exec(operationId, ['pull', '--ff-only'], repoPath); | ||
|
|
||
| try { | ||
| await this._exec(operationId, ['pull', '--ff-only'], repoPath); | ||
| } catch (err) { | ||
| this._logService.warn(`[LocalGitService] Fast-forward pull failed for ${repoPath}. Retrying after fetch.`); | ||
| await this._exec(operationId, ['fetch', '--prune'], repoPath); | ||
|
|
||
| try { | ||
| await this._exec(operationId, ['pull', '--ff-only'], repoPath); | ||
| } catch (retryErr) { | ||
| const upstream = await this._getSafeHardResetTarget(operationId, repoPath); | ||
| if (!upstream) { | ||
| throw retryErr; | ||
| } | ||
|
|
||
| this._logService.warn(`[LocalGitService] Pull retries exhausted for ${repoPath}. Performing hard reset to ${upstream}.`); | ||
| await this._exec(operationId, ['reset', '--hard', upstream], repoPath); | ||
| } | ||
| } |
Comment on lines
+55
to
+57
| } catch (err) { | ||
| this._logService.warn(`[LocalGitService] Fast-forward pull failed for ${repoPath}. Retrying after fetch.`); | ||
| await this._exec(operationId, ['fetch', '--prune'], repoPath); |
Comment on lines
+76
to
+87
| private async _getSafeHardResetTarget(operationId: string, repoPath: string): Promise<string | undefined> { | ||
| const status = (await this._exec(operationId, ['status', '--porcelain'], repoPath)).trim(); | ||
| if (status.length > 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| let upstream: string; | ||
| try { | ||
| upstream = (await this._exec(operationId, ['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'], repoPath)).trim(); | ||
| } catch { | ||
| return undefined; | ||
| } |
Comment on lines
+178
to
+182
| const primaryActions = [new Action('showGitOutput', localize('showGitOutput', "Show Git Output"), undefined, true, () => this._commandService.executeCommand('git.showOutput'))]; | ||
|
|
||
| if (marketplace.kind !== MarketplaceReferenceKind.LocalFileUri) { | ||
| primaryActions.push(new Action('purgeAndRecloneMarketplace', localize('purgeAndRecloneMarketplace', "Purge Marketplace Cache and Reclone"), undefined, true, () => this._purgeAndRecloneMarketplace(marketplace, options?.marketplaceType, updateLabel))); | ||
| } |
| async () => { | ||
| const exists = await this._fileService.exists(repoDir); | ||
| if (exists) { | ||
| await this._fileService.del(repoDir, { recursive: true }); |
Make hard-reset recovery opt-in for LocalGitService.pull and enable it only for plugin marketplace sync. Also return promises from notification actions, align failure labels, avoid trash during cache purge, tighten rev-list parsing, and add coverage for no-upstream and opt-in behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inject execFile into LocalGitService so unit tests can provide a fake implementation instead of stubbing child_process.execFile. This avoids the ESM stub failure in Linux Electron CI and keeps the recovery coverage intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Defer the fake execFile callback with queueMicrotask so LocalGitService sees the child process registration before the callback fires. This matches the real async contract and avoids the Canceled path in Electron unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
I would actually be okay if we just wanted to replace the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Force-pushed marketplace branches can leave cached plugin repos in a diverged state, causing
Chat: Update Pluginsto fail repeatedly. This change makes plugin marketplace sync more resilient without relying on brittle git error-message matching.What changed
LocalGitService.pullto use state-based failover:git pull --ff-onlygit fetch --pruneand retrygit pull --ff-only@{u})Notes