Repository navigation
Keep the Agents window responsive when marking many sessions as read - #342253
Draft
Karthik Nadig (karthiknadig) wants to merge 1 commit into
Draft
Karthik Nadig (karthiknadig) wants to merge 1 commit into
Karthik Nadig (karthiknadig) wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused change preserves existing read-state behavior, adds targeted regression coverage, and has no unresolved findings.
No open issues 🎉
What changed in this PR
Reduces Agents window freezes during Mark All as Read by batching provider notifications instead of refreshing listeners once per session.
Changes:
- Adds optional provider-level batch read-state updates with a per-session fallback.
- Merges agent-host session change events while preserving existing read-state operations.
- Adds regression coverage for provider routing, current and legacy protocols, and immediate single-session updates.
No actionable findings remain. Tests were not rerun during this review.
| File | Description |
|---|---|
| src/vs/sessions/services/sessions/test/browser/sessionsManagementService.test.ts | Tests batch routing and fallback calls. |
| src/vs/sessions/services/sessions/common/sessionsProvider.ts | Defines the optional batch capability. |
| src/vs/sessions/services/sessions/browser/sessionsManagementService.ts | Groups read-state updates by provider. |
| src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts | Tests merged notifications and protocol compatibility. |
| src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts | Batches notifications using a pauseable emitter. |
Review effort: Balanced
Mark All as Read set each session's read state separately, and the agent host provider published one session change per session. The sessions list and other listeners rebuild on every change, so marking N sessions did O(N^2) work in one renderer task (about 3.4 s for 500 sessions). markAllRead now hands each provider its sessions in one call. The agent host provider applies them while its session change emitter is paused and publishes one merged change. Providers without the batch method still get one call per session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Karthik Nadig (karthiknadig)
force-pushed
the
karthiknadig/bug-swarm/c9-f1-mark-all-read-freeze
branch
from
October 18, 2026 05:11
7ffe52f to
47c9f4d
Compare
This branch has not been deployed
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.
Problem
In the Agents window, right-clicking a sessions section header and choosing Mark All as Read froze the whole window until every session had been processed. The freeze grows faster than the number of sessions. With 500 unread sessions in one section, the window stopped painting and ignored input for about 3.4 s (median of 3 runs). With 100 sessions, the freeze lasted about 0.4 s.
With this change, neither size produces a renderer task over 50 ms, and all visible rows show as read in about 0.2 s.
mainmainHow these numbers were measured:
mainat 1e81fc7 and from this PR, both withVSCODE_DEV_DISABLE_DISPOSABLE_TRACKING=1. Each build ran in an Agents window connected throughchat.remoteAgentHoststo a local synthetic agent host (the testScriptedMockAgent, script below).main, and no long task with this PR.longtaskentry recorded by aPerformanceObserverin the renderer. Recording ran from just before the click until 1.5 s after the rows turned read. The browser reports only tasks longer than 50 ms, so "no task over 50 ms" means it reported no long task at all.Root cause
SessionsManagementService.markAllReadcalledsetSessionReadStateonce for each session. For the agent host providers (local and remote), each call firedonDidChangeSessionsfor that one session: seesetSessionReadStateand the legacy-protocol path_setLegacySessionReadState.Each of those events reached every listener of the session change event, and several listeners do work proportional to the total number of sessions. The sessions list, for example, responds to every event with a synchronous
refresh(). That refresh reads all sessions again, rebuilds an autorun over every session, regroups the list, and callstree.setChildren. Marking N sessions therefore did N refreshes of size N, so the work grew with N² inside one task.A CPU profile of Mark All as Read with 500 sessions on
mainconfirmed this:markAllRead.The per-session event pattern dates from moving read state into the providers in e2d7a76 (#325982).
Fix
ISessionsProvider.setSessionsReadState(sessionIds, isRead). It matches the existingsetChatReadStatesbatch method.markAllReadnow groups sessions by provider and calls each provider once. A provider without the batch method, such as the Copilot cloud provider, still receives onesetSessionReadStatecall per session.BaseAgentHostSessionsProvider, which covers both the local and remote agent hosts, implements the batch method by running its existing per-sessionsetSessionReadStatewhile its session change emitter is paused. The emitter is now aPauseableEmitterthat merges queued events with the existingmergeSessionChangeEvents. Listeners therefore receive one change event that lists every affected session.I fixed this in the provider rather than debouncing the sessions list, for two reasons:
What stays the same:
SessionIsReadChangedorChatIsReadChanged. This includes the legacy-protocol path.Before / after
Mark All as Read on a section with 500 unread sessions. The caption at the top states the median result. The orange box is an on-screen timer driven by
requestAnimationFrame, so it stops updating while the renderer is blocked. When the run ends, the box shows how long the rows took to turn read and the longest renderer task in that run.Before
After
How to test
node mock-host.mjs 8995 500. Then add"chat.remoteAgentHosts": [{ "address": "ws://127.0.0.1:8995", "name": "BS Mock Host" }]to your settings.main, the window freezes for several seconds.Automated repro scenario
Save as
repro.cjs. Launch a Code OSS Agents window with remote debugging enabled, connected to the synthetic host below. Then runnode <bug-swarm skill>/scripts/cdp-scenario.mjs --cdp <port> --scenario repro.cjs --out <dir>. The scenario fails onmainand passes with this PR.Synthetic agent host used for the measurements (
mock-host.mjs)Validation
main@1e81fc7, 500 sessionscdp-scenario.mjs --scenario repro.cjs(1 warm-up + 3 runs)main: the longest task was 376–427 ms, under the 1000 ms limit but still a visible stall. This PR: no task over 50 ms, and rows turned read after 113–136 ms.mainsourcesscripts\test.bat --run <sessionsManagementService.test.ts> --run <localAgentHostSessionsProvider.test.ts> --grep "markAllRead hands each provider its sessions in one call|marking several sessions read publishes one session change"markAllReadmade 4 per-session calls instead of 1 batch call plus 2 fallback calls, and the provider had no batch method.scripts\test.bat --run src\vs\sessions\services\sessions\test\browser\sessionsManagementService.test.ts --run src\vs\sessions\contrib\providers\agentHost\test\browser\localAgentHostSessionsProvider.test.ts --run src\vs\sessions\contrib\sessions\test\browser\sessionsList.test.ts --run src\vs\sessions\contrib\sessions\test\browser\sessionsListContextMenu.test.tsSessionsList comparison groups renders synthesis and Judge before connected compact attemptsexpects 1px connector strokes and gets 0.8px on this 125% display-scale machine. It fails the same way onmain:scripts\test.bat --run src\vs\sessions\contrib\sessions\test\browser\sessionsList.test.ts --grep "renders synthesis and Judge before connected compact attempts".cdp-scenario.mjs --scenario control.cjsnpm run typecheck-clientnpm run eslint -- <5 changed files>Found and fixed with automated exploratory testing. Draft until reviewed by a human.