Fix WorktreeCreatedTaskDispatcher overfiring (#318241)#318880
Merged
connor4312 merged 2 commits intoMay 29, 2026
Conversation
…18241) Introduce onDidStartSession event to distinguish sessions we just started from sessions that appear in the catalog (via cloud sync, refresh, etc). Problem: WorktreeCreatedTaskDispatcher listened to onDidChangeSessions and tried to filter on status=Untitled to identify newly started sessions. This was wrong because: - Agent-host sessions are never Untitled when arriving via change events (skeleton gets setStatus(InProgress) before firing) - onDidChangeSessions fires for any catalog change including cloud-synced/restored sessions from other devices - This caused tasks to run for existing sessions on every sync, not just newly started ones Solution: - Add onDidStartSession: Event<ISession> to ISessionsManagementService - Fire it from sendNewChatRequest after provider.sendRequest() resolves with committed session - Rewrite WorktreeCreatedTaskDispatcher to listen ONLY to onDidStartSession instead of added/changed events - This correctly identifies 'sessions we just started locally' vs 'sessions in the catalog' Changes: - ISessionsManagementService: add onDidStartSession event - SessionsManagementService: implement emitter + fire in sendNewChatRequest - WorktreeCreatedTaskDispatcher: rewrite to use onDidStartSession exclusively - Tests: comprehensive rewrite with regression test for #318241 - Docs: update SESSIONS.md spec Fixes #318241 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @sandy081Matched files:
@lszomoruMatched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes overfiring of worktreeCreated session tasks by distinguishing locally started sessions from restored/synced catalog updates in the Agents window session flow.
Changes:
- Adds
ISessionsManagementService.onDidStartSessionand fires it aftersendNewChatRequestcommits a session. - Reworks
WorktreeCreatedTaskDispatcherto track only newly started sessions and use catalog changes only for removal cleanup. - Updates related tests, configuration default, and session documentation.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/SESSIONS.md |
Documents the new onDidStartSession-based worktree task dispatch flow. |
src/vs/sessions/services/sessions/common/sessionsManagement.ts |
Adds the new service event contract. |
src/vs/sessions/services/sessions/browser/sessionsManagementService.ts |
Implements and fires onDidStartSession after session commit. |
src/vs/sessions/services/sessions/test/browser/sessionNavigation.test.ts |
Updates the session management service test mock. |
src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts |
Rewrites dispatch tracking to listen to started sessions instead of catalog additions/changes. |
src/vs/sessions/contrib/chat/browser/chat.contribution.ts |
Changes the agent-host worktree-created task setting default to enabled. |
src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts |
Reworks dispatcher tests around the new event and adds a restored-session regression case. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 4
…318880) Fix 4 issues identified in review: 1. **Autorun synchronous-dispatch leak** (line 79-91): - autorun was running synchronously before store.add() completed - when conditions were met immediately, it would dispose the store before the autorun was registered - Switch to registerAutorunSelfDisposable to ensure the autorun is registered before it can dispose itself 2. **Config default documentation mismatch** (line 18-21): - JSDoc claimed setting defaults to false, but chat.contribution.ts sets true - Update JSDoc to say 'Defaults to `true`' 3. **Test doesn't reflect new default** (line 258-268): - Test 'skips agent host sessions when the setting is disabled (default)' relied on default - Now that default is true, explicitly set config to false - Remove '(default)' from test name 4. **Stale comment** (line 74-77): - Comment still referred to removed 'dispatched' flag - Update to describe actual guard (disposing store removes subscriptions)
TylerLeonhardt
approved these changes
May 29, 2026
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
WorktreeCreatedTaskDispatcher fires 'Install & Watch' worktree tasks excessively for restored/synced sessions instead of only for newly-started ones.
Root Cause
The dispatcher listened to
onDidChangeSessionsand tried to filter onstatus === Untitledto identify newly started sessions. This was fundamentally wrong:Agent-host sessions are never Untitled when arriving via change events:
setStatus(InProgress)before the change event fires (baseAgentHostSessionsProvider.ts:1854-1859)_refreshSessionsare constructed with server metadata status (baseAgentHostSessionsProvider.ts:2106-2147)onDidChangeSessionsis a catalog-change event, not a 'we started this' event:Solution
Introduce an explicit
onDidStartSessionevent that fires only fromsendNewChatRequestafterprovider.sendRequest()resolves with the committed session.Changes
readonly onDidStartSession: Event<ISession>declarationsendNewChatRequest(after session is committed)onDidStartSessioninstead ofadded/changedeventsWhy This is Correct
Sessions that flow through
sendNewChatRequestare exactly the ones 'we just started locally'. Cloud-synced / restored / refreshed sessions appear viaprovider._onDidChangeSessionsfrom_refreshSessions, which the dispatcher no longer listens to.This correctly distinguishes:
Fixes #318241