chat: fix per-marketplace plugin auto-update never running - #333289
Draft
joshspicer wants to merge 2 commits into
Draft
chat: fix per-marketplace plugin auto-update never running#333289joshspicer wants to merge 2 commits into
joshspicer wants to merge 2 commits into
Conversation
Installed plugin metadata is hydrated asynchronously from `installed.json`, so the periodic update check scheduled at startup idle almost always observed an empty installed list. It then bailed out and its `finally` block re-armed with a hard `PLUGIN_UPDATE_CHECK_INTERVAL_MS` (24h) delay, so no marketplace was ever fetched and no plugin was ever auto-updated -- regardless of `extensions.autoUpdate` or a managed marketplace's `autoUpdate` override. Track whether the last check bailed out for that reason and re-arm as soon as installed entries become observable, both from an autorun on `installedPlugins` and from the check's own backoff decision. Enforcement stays strictly per-marketplace and `strictKnownMarketplaces` continues to gate every refresh path. Adds the auto-update policy matrix from the feature spec: `true`, `false`, `undefined`, mixed marketplaces, global auto-update disabled, strict-policy gating, plus a regression test for the hydration race. Fixes #330090 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts — Initializing this flag to true treats “no check has run yet” as if an update check observed no… |
 |
src/vs/workbench/contrib/chat/test/common/plugins/pluginMarketplaceService.test.ts — This test comment exceeds the one-line inline-comment convention and only describes the setup… |
|
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts — This private-field JSDoc includes startup control flow and issue history that are also described at… |
What changed in this PR
Fixes plugin auto-update checks missed during asynchronous metadata hydration.
Changes:
- Re-schedules checks when installed plugins become observable.
- Adds per-marketplace policy and hydration regression tests.
| File | Description |
|---|---|
pluginMarketplaceService.ts |
Adds hydration-aware update scheduling. |
pluginMarketplaceService.test.ts |
Tests policy combinations and delayed hydration. |
Suppressed comments (2)
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts:426
- This multi-line inline comment narrates the immediately following autorun and duplicates the field documentation. Keep the non-obvious timing constraint in a single concise line.
// Installed plugin metadata is hydrated asynchronously, so the
// check above typically runs against an empty list. Re-arm as soon
// as installed entries become observable instead of waiting a full
// check interval.
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts:890
- This two-line inline comment restates the condition below it. Condense it to a single line so the retry invariant remains clear without narrating the expression.
// A check that found nothing installed did not really run;
// retry immediately if entries have appeared since.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
Base:
|
- Start `_lastCheckFoundNoInstalledPlugins` as `false` so a fresh session no longer treats "no check has run yet" as an empty check. Previously, when metadata happened to be hydrated before startup idle, the autorun forced `_scheduleUpdateCheck(0)` and overrode the deadline derived from `PLUGIN_UPDATE_LAST_CHECK_STORAGE_KEY`, refetching every eligible marketplace on each restart. `_doRunUpdateCheck` now sets the flag only after an actual empty check. - Condense the field JSDoc and the two scheduling-site comments to one line each, keeping the rationale at the scheduling site only. Adds a regression test asserting that a recent successful check is not re-run when metadata is already hydrated at startup; it fails with the previous `true` initializer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts — This private-field JSDoc includes startup control flow and issue history that are also described at… View resolved comment |
|
src/vs/workbench/contrib/chat/test/common/plugins/pluginMarketplaceService.test.ts — This test comment exceeds the one-line inline-comment convention and only describes the setup… View resolved comment |
|
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts — Initializing this flag to true treats “no check has run yet” as if an update check observed no… View resolved comment |
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.
Fixes #330090
Root cause — a real product bug, not tester environment
The per-marketplace
autoUpdateplumbing from microsoft/vscode-internalbacklog#8462 is correct end to end (managed settings →copilotManagedSettingsnormalization →chat.plugins.extraMarketplaces→marketplaceReferenceparsing →isMarketplaceAutoUpdateEnabled). Six of the seven new tests pass onmainunchanged, which is the evidence that the policy layer itself was never broken.The defect is one layer down, in the periodic update check that the policy gates:
_hydratePluginMetadatare-reads each marketplace to rebuild descriptors frominstalled.json)._scheduleUpdateCheck()is armed fromrunWhenGlobalIdle, which fires long before that completes._doRunUpdateCheck()therefore seesinstalledPlugins.get().length === 0and returns early.finallyblock unconditionally re-armed with_scheduleUpdateCheck(PLUGIN_UPDATE_CHECK_INTERVAL_MS)— a hard 24 h delay — and nothing observedinstalledPluginsto re-arm sooner.So in every real session the one scheduled check lost the race against hydration, then went quiet for a day. No marketplace was fetched,
marketplacesWithUpdatesstayed empty,PluginAutoUpdatewas never signalled, and no plugin ever auto-updated — which is why Eleanor Boyd (@eleanorjboyd) saw no update withextensions.autoUpdatedisabled and enabled. The early return also never wrotePLUGIN_UPDATE_LAST_CHECK_STORAGE_KEY, so restarting just repeated the lost race.This is independent of the
autoUpdateoverride, which is why the feature looked correct in review but shipped non-functional. microsoft/vscode-internalbacklog#8462 should be reopened.Fix
Track whether the last check bailed out purely because metadata had not hydrated, and re-arm as soon as installed entries appear:
_lastCheckFoundNoInstalledPluginsrecords that state, set only by an actual empty check.autorunoninstalledPluginsre-schedules with delay0the first time entries appear, guarded on_updateCheckRunningso it cannot overlap an in-flight check.0instead of a full interval when it previously found nothing and entries have since appeared — covering hydration landing while a check runs.The flag returns to
falseas soon as a check observes installed plugins, so this cannot loop; normal 24 h backoff resumes. With no plugins installed, both paths short-circuit to exactly the previous behaviour.Enforcement stays strictly per-marketplace:
_doRunUpdateCheckstill filters every marketplace throughisMarketplaceAutoUpdateEnabled(ref)and_isMarketplaceAllowedByStrictPolicy(ref)before fetching, and reports only canonical IDs with real upstream commits, soPluginAutoUpdatekeeps callingupdateAllPlugins({ marketplaceIds })with a targeted set rather than acting on a global "something has updates" signal.strictKnownMarketplacesgating is unchanged on every refresh and update path.Tests
New suite
PluginMarketplaceService - per-marketplace auto-update enforcement, covering the matrix from #8462:autoUpdate: true, globaloffautoUpdate: false, globalonundefined, globalonundefined, globaloffstrictKnownMarketplaces+autoUpdate: trueBoth regression tests were verified to fail without their corresponding fix and pass with it.
Validation
npm run transpile-client,npm run typecheck-client— no errors in touched filesnpm run valid-layers-check— clean./scripts/test.sh --grep "PluginMarketplaceService|PluginAutoUpdate|PluginInstallService"— 124 passingScope is deliberately minimal: 2 files, 21 production lines changed.