Skip to content

managed settings: keep policy through transient refresh failures - #337583

Merged
Ross Wollman (rwoll) merged 8 commits into
mainfrom
agents/otel-notification-investigation
Sep 24, 2026
Merged

managed settings: keep policy through transient refresh failures#337583
Ross Wollman (rwoll) merged 8 commits into
mainfrom
agents/otel-notification-investigation

Conversation

@rwoll

@rwoll Ross Wollman (rwoll) commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Copilot's OTel reload notification (reworked in #336701) exposed a managed-settings refresh regression from #330896. Enterprise users see "Copilot OTel endpoint will change to http://localhost:4318 after reload." every few hours, especially after idle, and reloading clears chat.

Cause

Since #330896, a cached server-managed-settings response expires one hour after the last successful fetch. If a refresh fails after that (no response, 401/403, 429, 5xx, malformed), the policy is dropped rather than kept. A policy-backed OTel endpoint then reverts to its default and the notification appears. The next successful fetch restores the policy, so it keeps switching back and forth. Every other server-managed control switches back and forth in the same way, so for the length of the gap their restrictions are silently lifted.

Change

Keep the policy unless the service answers affirmatively that it should change. A failed refresh without forceRemoteSettingsRefresh now keeps the last successful response, however old, with its original timestamp. The failure therefore doesn't count as a refresh, and the next poll or focus retries. A 401/403 is treated as a failure because it can be temporary, for example while a token refreshes.

This keeps #330896's fix that a failure never renews the cache timestamp, and reverts only its dropping of stale policy. Before #330896, failures also kept the previous policy, but they renewed the timestamp.

These still behave as before:

  • A 200 replaces the policy; a 404 or 466 clears it.
  • Sign-out, or an account or scope change, clears or stops reusing the policy.
  • forceRemoteSettingsRefresh still fails closed.

Follow-up

  • A thrown session lookup error still makes getDefaultAccountForAuthenticationProvider return null, so setDefaultAccount(null) clears the policy without applying this retention. This path predates managed settings: fix stale cache case when moving to unavailable #330896 and changes account availability, so it needs a separate PR.
  • managedSettingsActive (set when the server returns settings that no VS Code policy uses) is never cleared once set. This bug existed before this PR and will be fixed separately.

Validation

  • defaultAccount.test.ts: 49 passing. The updated test fails without this change.
  • Account and policy suites: 250 passing.

@rwoll
managed settings: keep policy through transient refresh failures
589a441 
Since #330896, a failed refresh after the one-hour cache boundary dropped server-managed settings, so policy-backed settings such as the Copilot OTel endpoint flapped back to defaults and triggered reload prompts. Retain the last successful response for up to 24 hours without renewing its timestamp, and carry managedSettingsFetchedAt forward when managed settings are not fetched.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 23, 2026 23:32
@rwoll Ross Wollman (rwoll) self-assigned this Sep 23, 2026
@rwoll
Approve reviewed PR state
fcfc661 
Reviewed-head: 589a441
Pull-request: #337583
Approved-by: rwoll via PR Sign-Off
Requested-by: copilot
@rwoll Ross Wollman (rwoll) added the bug Issue identified by VS Code Team member as probable bug label Sep 23, 2026
@rwoll Ross Wollman (rwoll) added this to the 1.140.0 milestone Sep 23, 2026
Copilot AI reviewed Sep 23, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Expiration does not clear managedSettingsActive, allowing governance state to persist indefinitely.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Preserves server-managed settings through transient refresh failures while maintaining bounded expiration.

Changes:

  • Retains failed refresh data for up to 24 hours without renewing timestamps.
  • Preserves timestamps across skipped managed-settings fetches.
  • Adds regression coverage.
File Description
defaultAccount.ts Implements retention and timestamp propagation.
defaultAccount.test.ts Tests retention, expiry, and entitlement outages.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/vs/workbench/services/accounts/browser/defaultAccount.ts Outdated
@rwoll
managed settings: clear unprojected server presence with its payload
63ce9e8 
managedSettingsActive was only ever set, so a server response containing only unprojected settings kept the channel marked active after a 404, an empty response, or retention expiry. Retain and clear it together with managedSettings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rwoll
Approve reviewed PR state
d341a97 
Reviewed-head: 63ce9e8
Pull-request: #337583
Approved-by: rwoll via PR Sign-Off
Requested-by: copilot
@rwoll
managed settings: retain policy until the service answers
1b0a35c 
Drop the 24-hour retention bound. Failed refreshes, including 401/403 responses that a token refresh or SSO sign-in may resolve, keep the last successful response without renewing its timestamp; only a service answer, sign-out, or account or scope change clears it. This matches the retention before #330896 while keeping its fix that a failure never renews the cache.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rwoll
Approve reviewed PR state
1e4bf12 
Reviewed-head: 1b0a35c
Pull-request: #337583
Approved-by: rwoll via PR Sign-Off
Requested-by: copilot
@rwoll
Ross Wollman (rwoll) enabled auto-merge (squash) September 24, 2026 00:01
@rwoll
Ross Wollman (rwoll) marked this pull request as draft September 24, 2026 00:06
auto-merge was automatically disabled September 24, 2026 00:06

Pull request was converted to draft

@rwoll
Ross Wollman (rwoll) marked this pull request as ready for review September 24, 2026 00:07
@rwoll
Ross Wollman (rwoll) enabled auto-merge (squash) September 24, 2026 00:07
roblourens
roblourens previously approved these changes Sep 24, 2026
View reviewed changes
@github-actions

github-actions Bot commented Sep 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Base: 68070681 Current: 08db08d5

No screenshot changes.

@rwoll
Ross Wollman (rwoll) marked this pull request as draft September 24, 2026 00:28
auto-merge was automatically disabled September 24, 2026 00:28

Pull request was converted to draft

@rwoll
managed settings: narrow to the minimal retention fix
ac2602b 
Keep server-managed policy on any failed refresh, including 401/403, with its original timestamp so the failure never counts as a refresh. Revert the unrelated managedSettingsActive change and the managedSettingsFetchedAt carry-forward, which are not needed for this fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rwoll
Approve reviewed PR state
775face 
Reviewed-head: ac2602b
Pull-request: #337583
Approved-by: rwoll via PR Sign-Off
Requested-by: copilot
@rwoll
Ross Wollman (rwoll) marked this pull request as ready for review September 24, 2026 00:49
@rwoll
Ross Wollman (rwoll) enabled auto-merge (squash) September 24, 2026 00:56
@rwoll

Ross Wollman (rwoll) commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

Joaquín Ruales (@jruales) - FYI, this only fixes one possible source of policy flapping. getDefaultAccountForAuthenticationProvider also can cause flaps.

@rwoll
Ross Wollman (rwoll) merged commit 29b6800 into main Sep 24, 2026
33 checks passed
@rwoll
Ross Wollman (rwoll) deleted the agents/otel-notification-investigation branch September 24, 2026 01:20
@rwoll Ross Wollman (rwoll) added the ~release-cherry-pick Trigger: cherry-pick this PR to the latest release branch label Sep 24, 2026
@vs-code-engineering vs-code-engineering Bot added release-cherry-pick Automated cherry-pick between release and main branches and removed ~release-cherry-pick Trigger: cherry-pick this PR to the latest release branch labels Sep 24, 2026
@vs-code-engineering vs-code-engineering Bot mentioned this pull request Sep 24, 2026
Merged
@jruales Joaquín Ruales (jruales) mentioned this pull request Sep 24, 2026
Closed
@jruales Joaquín Ruales (jruales) linked an issue Sep 24, 2026 that may be closed by this pull request
Managed settings are dropped after transient refresh failures #337804
Closed
@rwoll Ross Wollman (rwoll) mentioned this pull request Sep 24, 2026
Closed
1 of 13 tasks
@anthonykim1 Anthony Kim (anthonykim1) mentioned this pull request Oct 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jruales Joaquín Ruales (jruales) jruales approved these changes
@digitarald Harald Kirschner (digitarald) digitarald left review comments
@roblourens roblourens roblourens left review comments
@Yoyokrazy Michael Lively (Yoyokrazy) Yoyokrazy left review comments

Assignees

@rwoll Ross Wollman (rwoll)

Labels

bug Issue identified by VS Code Team member as probable bug release-cherry-pick Automated cherry-pick between release and main branches

Projects

None yet

Milestone

1.140.0

Development

Successfully merging this pull request may close these issues.

Managed settings are dropped after transient refresh failures

6 participants

@rwoll @digitarald @roblourens @jruales @Yoyokrazy