Managed settings: reuse cached policy more often - #333697
Merged
Merged
Conversation
joshspicer
changed the title
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/services/accounts/browser/defaultAccount.ts — The cache-hit path no longer verifies managedSettingsScope. getScopedServerManagedSettings… |
 |
src/vs/workbench/services/accounts/browser/defaultAccount.ts — This four-line inline comment exceeds the repository's comment convention and narrates the adjacent… |
What changed in this PR
Reuses fresh managed-settings cache entries to avoid redundant startup requests.
Changes:
- Removes the per-process fetch gate.
- Updates cache behavior and related unit tests.
| File | Description |
|---|---|
defaultAccount.ts |
Reuses fresh cached managed settings. |
defaultAccount.test.ts |
Updates cache and forced-refresh expectations. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
joshspicer
force-pushed
the
joshspicer/cache-managed-settings-reload
branch
from
September 1, 2026 01:07
192191e to
e042b4e
Compare
When forceRemoteSettingsRefresh is not effective, a fresh cached managed-settings response now satisfies startup without a network round-trip, matching the sibling entitlements/token/MCP fetches which only refetch once the cache goes stale. Previously a per-process gate (hasFetchedThisProcess) forced one /managed_settings request on every reload even with a fresh cache and no policy requirement, which for a user without an enterprise policy fanned out into one 404 per GitHub session. Cache reuse is scoped: a cached response is only reused when it was captured for the current account, authentication provider, and endpoint (a legacy cache with no recorded scope is trusted), so a previous GitHub Enterprise host's policy is not applied after switching scopes. An outstanding client_update_required compatibility error is never served from cache: it is revalidated on the next refresh so the fail-closed block clears once the client or server recovers, instead of lingering for the full cache lifetime. The fail-closed path is otherwise unchanged: when the control is effective, only a fresh successful response for the current account, provider, and endpoint clears the requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
force-pushed
the
joshspicer/cache-managed-settings-reload
branch
from
September 1, 2026 01:27
e042b4e to
8a219be
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/vs/workbench/services/accounts/browser/defaultAccount.ts — This four-line inline comment exceeds the repository's comment convention and narrates the adjacent… View resolved comment |
|
src/vs/workbench/services/accounts/browser/defaultAccount.ts — The cache-hit path no longer verifies managedSettingsScope. getScopedServerManagedSettings… View resolved comment |
joshspicer
marked this pull request as ready for review
September 1, 2026 01:35
joshspicer
enabled auto-merge (squash)
September 1, 2026 01:40
joshspicer
added
the
~release-cherry-pick
Contributor
|
This PR will be automatically cherry-picked to |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
force-pushed
the
joshspicer/cache-managed-settings-reload
branch
from
September 1, 2026 01:56
6cbafc3 to
10f67ff
Compare
joshspicer
removed
the
~release-cherry-pick
joshspicer
disabled auto-merge
September 1, 2026 02:47
Do not retain or relabel managed settings cached for a different account provider or endpoint when refresh fails or automatic retries are blocked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
enabled auto-merge (squash)
September 1, 2026 03:08
Vritant Bhardwaj (vritant24)
approved these changes
Sep 1, 2026
Vritant Bhardwaj (vritant24)
approved these changes
Sep 1, 2026
Bhavya U (bhavyaus)
pushed a commit
that referenced
this pull request
Sep 1, 2026
* managed settings: reuse cached policy on reload instead of refetching When forceRemoteSettingsRefresh is not effective, a fresh cached managed-settings response now satisfies startup without a network round-trip, matching the sibling entitlements/token/MCP fetches which only refetch once the cache goes stale. Previously a per-process gate (hasFetchedThisProcess) forced one /managed_settings request on every reload even with a fresh cache and no policy requirement, which for a user without an enterprise policy fanned out into one 404 per GitHub session. Cache reuse is scoped: a cached response is only reused when it was captured for the current account, authentication provider, and endpoint (a legacy cache with no recorded scope is trusted), so a previous GitHub Enterprise host's policy is not applied after switching scopes. An outstanding client_update_required compatibility error is never served from cache: it is revalidated on the next refresh so the fail-closed block clears once the client or server recovers, instead of lingering for the full cache lifetime. The fail-closed path is otherwise unchanged: when the control is effective, only a fresh successful response for the current account, provider, and endpoint clears the requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Trigger signed commit verification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * managed settings: scope failure fallbacks Do not retain or relabel managed settings cached for a different account provider or endpoint when refresh fails or automatic retries are blocked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
On reload,
DefaultAccountProviderrestores cached policy from storage but still re-fetched/managed_settingseven when we had a fresh (<1h) cache. This change continues to allow the 'ungoverned' (standard) path reuse a fresh cached response with no network round-trip (matching the sibling entitlements/token/MCP fetches, which only refetch once the 1-hour cache goes stale), so reloads within the window now make zero/managed_settingsrequests. The fail-closedforceRemoteSettingsRefreshpath is unchanged: only a fresh successful response for the current account, provider, and endpoint clears the requirement.