Skip to content

Apply session _meta once per adapter update and keep it transactional - #327675

Merged
ulugbekna merged 2 commits into
mainfrom
ulugbekna/quick-chat-meta-followup
Jul 28, 2026
Merged

Apply session _meta once per adapter update and keep it transactional#327675
ulugbekna merged 2 commits into
mainfrom
ulugbekna/quick-chat-meta-followup

Conversation

@ulugbekna

@ulugbekna ulugbekna commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #327629 ("quick chats: fix: show within Chats section instead of random UUID dirs"), now merged. Rebased onto main; this PR is a single commit against main.

Scope, stated plainly: the behavioural fix for the quick-chat grouping bug shipped in #327629. This one is an optimization, its prerequisite, and two hygiene items. Sizing it that way up front so it can be triaged accordingly — or trimmed, see "If you want to trim" below.

#327629 made _meta arrive on every listSessions refresh. _refreshSessions() fires on each ChatTurnComplete and calls update() for every listed session (the cache caps at 100), so anything redundant in AgentHostSessionAdapter.update() is now per-session, per-turn work.


1. Apply _meta once per update — optimization

update() applied _meta twice: once inline, then again through the trailing setMeta. With promotion now living inside setMeta, the duplicate also re-ran _promoteToQuickChatIfWorkspaceless and _computeWorkspace() — the latter allocating a workspace object and doing a configurationService.getValue('git.branchProtection', { resource }) lookup — plus a second agentHostSessionWorkspaceKey() (an 11-field string join).

Applied once now, up front, where the promotion still precedes the workspace rebuild. Halves that work. Real but modest — not something you'd notice in a profile.

2. setMeta joins the caller's transaction — prerequisite + hardening, not a live bug fix

Being straight about this one: on main today there is no tearing. setMeta is the last write in both transactional callers (update() and _handleSessionSummaryChanged), so its nested transaction() flushes after everything else is applied.

It's needed because of change 1. transaction() never joins an enclosing one — it constructs a TransactionImpl and finish()es it — so collapsing to a single application at the top of update() is exactly what opens the window: observers of _meta / isQuickChat / workspace would be notified before isArchived, isRead, changes and activity land, handing them a torn snapshot.

setMeta now takes an optional ITransaction and uses subtransaction; update() and _handleSessionSummaryChanged thread theirs through. The two standalone callers (_applySessionMetaFromState, _handleSessionMetaChanged) pass nothing and keep their own transaction, so their behaviour is unchanged.

Beyond unblocking change 1, this removes an ordering landmine: today the invariant "setMeta must be the last write" is implicit and unenforced, so adding any field write after it silently reintroduces torn notifications.

3. Doc key correction + negative wire test — the part I'd argue actually matters

  • copilot.workspaceless appears nowhere in the code. The marker is persisted as agentHost.workspaceless (AH_META_WORKSPACELESS_DB_KEY), written centrally by AgentService; the agents only read it. The wrong key was named in AGENT_HOST_SESSIONS_PROVIDER.md and in .github/skills/sessions/SKILL.md — the latter is read by agents, so it keeps propagating.
  • Pins the absent-_meta case on the wire. The item is built field by field and satisfies SessionSummary catches a dropped optional in neither direction — which is exactly how this bug shipped twice.

If you want to trim

The honest minimum is section 3. Sections 1 and 2 travel together (2 exists to make 1 safe) and can be dropped without affecting anything user-visible.

Testing

  • a refresh publishes _meta and summary fields as one atomic update — asserts an autorun observing workspace + isArchived sees exactly two snapshots across a refresh that moves both. Re-verified after the rebase that it fails with a torn { feature, false } middle snapshot when subtransaction is reverted to transaction.
  • listSessions omits _meta when the agent provides none at the protocol layer.
  • npm run typecheck-client clean, eslint clean, 532 tests passing across the local/remote provider, protocol server/client, agent service and sessions-list suites.

Review provenance

#327629 and an independent branch of mine were produced by two separate sessions that reached the same root-cause diagnosis and the same wire fix, then diverged on how to heal already-mis-classified sessions. Rather than merge them, I kept #327629's approach and reduced mine to the deltas above.

  • Plan review (3 frontier models, on my original branch) unanimously rejected making the adapter's session-kind mutable — the concerns were an open session's workspace flipping to undefined mid-session and supportsMultipleChats collapsing chat tabs — and recommended dropping the persisted cache via a storage-key bump instead. Keep workspace-less quick chats out of bogus workspace groups #327629 shipped the mutable-kind approach anyway, with a one-way promotion guard, and it is the better call: it heals in place with no cold-start cost and covers the remote provider for free. My cache-key bump and _metaByRawId guard were dropped as redundant.
  • Implementation review (3 frontier models). One caught the nested-transaction hazard described in section 2 in my original branch, where it was a live regression I had introduced by reordering; that finding is what section 2 carries forward as hardening here. Another verified the update() rewrite is equivalent to the original in didChange propagation and notification timing, and that the preserved metadata is contained to _persistCache. A third flagged the remote provider's cache key and withdrew after the offline-list tradeoff was explained.

Copilot AI review requested due to automatic review settings July 27, 2026 18:18
@ulugbekna
ulugbekna changed the base branch from main to ulugbekna/automation-session-grouping July 27, 2026 18:18

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.

Pull request overview

Makes agent-host session metadata updates transactional and avoids duplicate _meta application.

Changes:

  • Threads transactions through setMeta.
  • Adds atomic refresh and protocol metadata tests.
  • Corrects workspace-less persistence key documentation.
Show a summary per file
File Description
baseAgentHostSessionsProvider.ts Updates transactional metadata handling.
localAgentHostSessionsProvider.test.ts Tests atomic refresh publication.
protocolServerHandler.test.ts Tests absent _meta serialization.
AGENT_HOST_SESSIONS_PROVIDER.md Corrects the persistence key.
.github/skills/sessions/SKILL.md Corrects session guidance.

Review details

  • Files reviewed: 5/5 changed files
  • Comments generated: 2
  • Review effort level: Medium

roblourens
roblourens previously approved these changes Jul 27, 2026
…sactional

Follow-up to "agents window: keep workspace-less quick chats out of bogus
workspace groups". That change made `_meta` arrive on every `listSessions`
refresh, which turns two pre-existing quirks in `AgentHostSessionAdapter.update`
into per-session, per-turn-completion work:

- `_meta` was applied twice: once inline and again through the trailing
  `setMeta`. With the promotion logic now living in `setMeta`, that also ran
  `_promoteToQuickChatIfWorkspaceless` and `_computeWorkspace` twice. Apply it
  once, up front, where the promotion still precedes the workspace rebuild.
- `setMeta` opened its own `transaction()`. `transaction()` never joins an
  enclosing one - it constructs a `TransactionImpl` and `finish()`es it - so
  moving the single application to the top of `update` would notify observers
  of `_meta` / `isQuickChat` / `workspace` before `isArchived`, `isRead`,
  `changes` and `activity` were applied, handing them a torn snapshot. Take an
  optional `ITransaction` and use `subtransaction`, and thread the caller's
  transaction from `update` and `_handleSessionSummaryChanged`. The two
  standalone callers pass nothing and keep their own transaction.

Also pins the absent-`_meta` case on the wire (the item is built field by field
and `satisfies SessionSummary` cannot catch a dropped optional in either
direction), and corrects the session-database key named in the docs: the marker
is persisted as `agentHost.workspaceless` (`AH_META_WORKSPACELESS_DB_KEY`,
written by `AgentService`), never `copilot.workspaceless`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f4788b1-3307-4b88-b3fc-23565548de06
@ulugbekna
ulugbekna force-pushed the ulugbekna/quick-chat-meta-followup branch from 5bb20ed to ce3e04a Compare July 28, 2026 09:23
@ulugbekna
ulugbekna changed the base branch from ulugbekna/automation-session-grouping to main July 28, 2026 09:23
@ulugbekna
ulugbekna dismissed roblourens’s stale review July 28, 2026 09:23

The base branch was changed.

…ansaction threading

Addresses review feedback on this PR.

- `update()` still recomputed the workspace unconditionally after `setMeta`
  had already rebuilt it from the same project / working directories and the
  same `_meta`, so a `_meta`-bearing refresh paid for `_computeWorkspace()`
  twice - exactly the duplicate per-turn work this PR set out to remove. The
  fallback rebuild now only runs when the snapshot carries no `_meta`.

- `setChangesSummary` and `setActivity` wrote their observables with no
  transaction, and `observableValue.set(v, undefined)` builds and finishes one
  of its own, so they notified mid-way through the enclosing update. Both now
  take an optional `ITransaction`, and `update()` /
  `_handleSessionSummaryChanged` thread theirs through. Without this, threading
  the transaction into `setMeta` alone left `SessionSummaryChanged` still torn:
  `changes` is applied before `_meta`, so an observer of both ran once on the
  new chip with the stale workspace, then again at the outer finish.

The new test covers that notification path and fails with the intermediate
`{ branch: undefined, files: 2 }` snapshot when the transaction is dropped.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f4788b1-3307-4b88-b3fc-23565548de06
@ulugbekna
ulugbekna enabled auto-merge (squash) July 28, 2026 10:15
@ulugbekna
ulugbekna merged commit e269291 into main Jul 28, 2026
29 checks passed
@ulugbekna
ulugbekna deleted the ulugbekna/quick-chat-meta-followup branch July 28, 2026 18:11
@vs-code-engineering vs-code-engineering Bot added this to the 1.132.0 milestone Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants