Automations: migrate to provider supplied automations client side - #329655
Automations: migrate to provider supplied automations client side#329655Ben Villalobos (benvillalobos) wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds provider-owned durable Automation storage while retaining and migrating the legacy ledger.
Changes:
- Extracts keyed
AutomationStorepersistence. - Adds provider-aware aggregation, routing, migration, and recovery.
- Adds Local Agent Host storage and focused tests.
Show a summary per file
| File | Description |
|---|---|
automationService.ts (workbench common) |
Extracts the store interface. |
SESSIONS.md |
Documents provider-owned storage. |
sessionsProvider.ts |
Adds the provider capability. |
localAgentHostSessionsProvider.ts |
Creates the Local Agent Host store. |
providerAutomationService.test.ts |
Tests routing, migration, and recovery. |
automationTools.test.ts |
Updates keyed-storage test doubles. |
automationTestUtils.ts |
Supports keyed test storage. |
automationService.test.ts |
Tests store isolation. |
electron-browser/automationStorageService.ts |
Passes storage keys through IPC. |
common/automationStorageService.ts |
Adds provider keys and keyed APIs. |
providerAutomationService.ts |
Aggregates stores and migrates legacy data. |
browser/automationStorageService.ts |
Uses keyed browser storage. |
browser/automationService.ts |
Extracts keyed store implementation. |
automations.contribution.ts |
Registers the provider-aware service. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/vs/sessions/contrib/automations/browser/providerAutomationService.ts:203
- Legacy migration has the same read-copy-delete race with live mutations. If a user update or run write commits to the legacy ledger after this migration snapshots the Automation/runs but before
removeAutomationForTransfer, the newer data is deleted while only the older snapshot remains in the provider store. Make source removal conditional on the migrated source revision/state and retry the import with the latest data on conflict (or otherwise serialize migration with all cross-window mutations).
try {
await providerStore.importAutomation(automation, this.legacyStore.runsFor(automation.id).get());
await this.legacyStore.removeAutomationForTransfer(automation.id);
} catch (error) {
src/vs/sessions/contrib/automations/browser/providerAutomationService.ts:91
- Reusing
mutationGuardfor the transfer can turn a cancelled guarded update into a committed change. The source update has already passed its guard and committed; if cancellation or feature disablement happens before the destination CAS, this second guard throws, andAutomationToolsreports “No changes were made” even though the source Automation (including its new target) was updated. Treat the first successful source CAS as the point of no return and finish/rollback the transfer without returning a cancellation result, or otherwise make the composite operation transactional.
const result = await source.updateAutomationIfUnchanged(id, patch, expected, mutationGuard);
if (result.kind === 'updated') {
await this.transferAutomationIfNeeded(source, result.automation, mutationGuard);
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ben Villalobos (benvillalobos)
changed the title
Ben Villalobos (benvillalobos)
changed the title
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Sandeep Somavarapu (@sandy081)Matched files:
Ladislau Szomoru (@lszomoru)Matched files:
|
Dmitriy Vasyura (dmitrivMS)
left a comment
Dmitriy Vasyura (dmitrivMS)
left a comment
There was a problem hiding this comment.
AI Review: Found three correctness issues in provider-owned automation migration and lifecycle handling.
| ).sort((a, b) => b.startedAt.localeCompare(a.startedAt)); | ||
| }); | ||
| this._register(sessionsProvidersService.onDidChangeProviders(event => { | ||
| if (event.added.some(provider => provider.automations)) { |
There was a problem hiding this comment.
AI Review: Providers registered after construction only queue legacy migration. Stale-run recovery is a separate one-shot operation, so a late provider can expose persisted running rows that are never marked failed and can continue blocking new run claims. Please run stale recovery as part of the added-provider lifecycle and cover late registration with a test.
| async importAutomationSnapshot(snapshot: IAutomationSnapshot): Promise<IAutomationSnapshotImportResult> { | ||
| const { automation, runs } = snapshot; | ||
| return this.mutateLedger(ledger => { | ||
| const hasAutomation = ledger.automations.some(candidate => candidate.id === automation.id); |
There was a problem hiding this comment.
AI Review: An existing automation ID is treated as equivalent without comparing the stored payload, and same-ID runs are also skipped without comparing their payloads. Migration then removes the unchanged legacy snapshot, so divergent provider state silently wins and legacy automation/run data is lost. Please report a conflict for divergent snapshots and remove the source only after destination equivalence is established.
| this.logService.warn(`[ProviderAutomationService] Automation '${snapshot.automation.id}' changed in the destination store during rollback; leaving both copies in place.`); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
AI Review: missing is treated as rollback success here. In the conflict path, if the destination is concurrently deleted before rollback, this returns true; the retry then upserts the newer source snapshot and resurrects the destination automation despite that deletion. Please distinguish missing from a completed rollback, stop the transfer while leaving the source intact, and add a destination-delete race test.
This change moves from a centrally owned automation service to a provider-first automation service. Providers on the client side can initialize an optional
automationsfield, which the newproviderAutomationServicewill discover by iterating over all providers to see which ones support automations. Automations and runs will be supplied by providers, stored in the storage service except keyed now on the provider id. Also includes auto migration of older automations that chose supported providers (agent host only for now)=== AI Generated Description ===
Moves durable Automation storage behind an optional Sessions provider capability.
AutomationStore.ProviderAutomationServicethat routes mutations to the owning provider.Validation
npm run typecheck-clientnpm run valid-layers-check