Skip to content

Commit 5cbdcf1

Browse files
easonliang28easonliang28
committed
fix(webview): close the profile deletion write group and make its ordering observable
- deleteProviderProfile routes the whole deletion through the mutation queue, so a concurrent upsert cannot land between the deletion write and its rollback; the test orders the upsert save against the deletion own restore write, filtered by profile name. - the acting view strict sticky write happens before the task handler rebuild, and the compensation restores this view pin and overlay, then the acting task name and history, each guarded on its own so one failed restore cannot hide the other. - a racing activation that starts mid-deletion now runs after the deletion settles: the new test pins that ordering instead of asserting the wrapper exists. - a mode switch that fails only on the profile tail keeps the mode pin it wrote; a TaskModeSwitched listener failure keeps the history entry on the running mode, and the TaskModeSwitched announcement is emitted after the durable write settles, outside the persistence try, so a listener failure cannot undo a mode the switch kept. - deleteApiConfiguration falls back to the UI-facing listApiConfigMeta entry when the stored record is already gone, so a stale selector entry is prunable through the idempotent prune; a name in neither list is still a no-op rather than a delete. - ContextProxy: a write that rejects after resetAllState no longer resurrects the pre-reset value, which is the per-key owner token that landed from main. - re-adopt the diagnostics-report test from main that our own merge resolution dropped, and control-check it against the command payload. Resolved threads: #1921 6z5Q_Q 6z5Q_M 6z5Q_D 6z5Q_H 6z5Q_Y 6z5Q-5 6z5Q-9 6z5Q_B 6z5Q-h 6z5Q-y
1 parent 4819893 commit 5cbdcf1

8 files changed

Lines changed: 747 additions & 53 deletions

File tree

‎src/activate/__tests__/registerCommands.spec.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,9 @@ vi.mock("../../i18n", () => ({
124124
t: (key: string) => key,
125125
}))
126126

127+
vi.mock("../../services/diagnostics", () => ({
128+
createDiagnosticsReport: vi.fn(),
129+
}))
127130
vi.mock("../../services/ripgrep/diagnostic", () => ({
128131
registerRipgrepDiagnosticCommand: vi.fn().mockReturnValue({ dispose: vi.fn() }),
129132
}))
@@ -252,6 +255,24 @@ describe("registerCommands handlers", () => {
252255
expect(mockContext.subscriptions).toContain(disposable)
253256
})
254257

258+
it("creates the diagnostics report from every provider instance, visible or not", async () => {
259+
// Re-adopted from main's copy of this file: the command was added with the privacy-safe
260+
// diagnostics report, and the click-origin rework of the title-bar handlers left it alone,
261+
// so it still feeds every instance to the service instead of only the visible one.
262+
const { createDiagnosticsReport } = await import("../../services/diagnostics")
263+
const providers = [mockProvider as unknown as ClineProvider, mockVisibleProviderInstance]
264+
vi.mocked(ClineProvider.getAllInstances).mockReturnValue(providers)
265+
vi.mocked(ClineProvider.getVisibleInstance).mockReturnValue(undefined)
266+
267+
handlers["zoo-code.createDiagnosticsReport"]()
268+
269+
expect(vi.mocked(createDiagnosticsReport)).toHaveBeenCalledWith({
270+
context: mockContext,
271+
outputChannel: mockOutputChannel,
272+
providers,
273+
})
274+
})
275+
255276
it("passes the shared focus tracker to the newTask handler", async () => {
256277
const { handleNewTask } = await import("../handleTask")
257278
const params = { prompt: "new task" }

‎src/activate/registerCommands.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,11 @@ async function disposeFailedTabCreation(
439439
}
440440
}
441441

442-
// The unserialized tab-creation body. Only openClineInNewTab may call it,
443-
// after it has stored the shared in-flight promise.
442+
// The unserialized tab-creation body. Its callers each serialize through their own in-flight
443+
// promise and start the body only after storing it: openClineInNewTab through
444+
// pendingTabPanelCreation (which also carries the forced-call and reuse rules above), and
445+
// openClineInNewTabForApi through apiTabPanelCreation. The two queues are not exclusive of
446+
// each other, so pendingTabPanelCreation does not cover API creations.
444447
const createTabPanelUnlocked = async ({
445448
context,
446449
outputChannel,

‎src/core/config/__tests__/ContextProxy.spec.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,49 @@ describe("ContextProxy", () => {
158158
expect(proxy.getGlobalState("apiProvider")).toBe("openrouter")
159159
})
160160

161+
it("does not resurrect a pre-reset value when a write rejects after resetAllState", async () => {
162+
// The owner-token design (ContextProxy.ts:401-432) corrects a rejected write by re-reading the
163+
// Memento while the writer is still the owner of the key. resetAllState writes undefined for
164+
// every global key before it re-initializes, so a write that is still in flight when the reset
165+
// runs reads back undefined on its rejection instead of putting the pre-reset value back.
166+
// The fixture models a Memento whose get and update agree, so the read-back sees what the
167+
// reset actually stored rather than a value the mock kept on its own.
168+
const store = new Map<string, unknown>()
169+
let holdNextWrite = false
170+
let heldWrite: { reject?: (error: Error) => void } | null = null
171+
mockGlobalState.get = vi.fn((key: string) => store.get(key))
172+
mockGlobalState.update = vi.fn((key: string, value: unknown) => {
173+
if (holdNextWrite) {
174+
holdNextWrite = false
175+
const slot: { reject?: (error: Error) => void } = {}
176+
heldWrite = slot
177+
return new Promise<void>((resolve, reject) => {
178+
slot.reject = reject
179+
})
180+
}
181+
store.set(key, value)
182+
return Promise.resolve(undefined)
183+
})
184+
await proxy.initialize()
185+
await proxy.updateGlobalState("apiProvider", "anthropic")
186+
expect(proxy.getGlobalState("apiProvider")).toBe("anthropic")
187+
188+
holdNextWrite = true
189+
const pendingWrite = proxy.updateGlobalState("apiProvider", "openrouter")
190+
191+
// The reset runs while that write is still in flight.
192+
await proxy.resetAllState()
193+
expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", undefined)
194+
expect(proxy.getGlobalState("apiProvider")).toBeUndefined()
195+
196+
heldWrite!.reject!(new Error("write failed after reset"))
197+
await expect(pendingWrite).rejects.toThrow("write failed after reset")
198+
199+
// The rejection handler is still the owner of the key, so it writes back what the Memento now
200+
// holds - undefined - rather than the value the cache carried before the reset.
201+
expect(proxy.getGlobalState("apiProvider")).toBeUndefined()
202+
})
203+
161204
it("should bypass cache for pass-through state keys", async () => {
162205
// Setup mock return value
163206
mockGlobalState.get.mockReturnValue("pass-through-value")

‎src/core/webview/ClineProvider.ts‎

Lines changed: 132 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,8 +2833,6 @@ export class ClineProvider
28332833
await this.updateTaskHistory({ ...taskHistoryItem, mode: newMode })
28342834
}
28352835

2836-
task.emit(RooCodeEventName.TaskModeSwitched, task.taskId, newMode)
2837-
28382836
// Only update the task's mode after successful persistence.
28392837
;(task as any)._taskMode = newMode
28402838
} catch (error) {
@@ -2847,8 +2845,17 @@ export class ClineProvider
28472845
// This ensures the in-memory state remains consistent with persisted state.
28482846
throw error
28492847
}
2850-
}
28512848

2849+
// Emit after the assignment and outside the persistence try. EventEmitter.emit rethrows a
2850+
// listener's error synchronously, and the extension API registers a relay listener for this
2851+
// event (src/extension/api.ts), so a consumer that throws used to fail the switch *after* the
2852+
// history entry had already moved: the entry named the mode the caller was told the switch
2853+
// rejected while the conversation, the shared mode and the view's pin kept the old one, and a
2854+
// later resume from history restored a mode nobody chose. Emitting below the assignment keeps
2855+
// the entry and the conversation on the same mode whatever a listener does; the reported
2856+
// failure and the shared-store behaviour are unchanged.
2857+
task.emit(RooCodeEventName.TaskModeSwitched, task.taskId, newMode)
2858+
}
28522859
// setValue (not the deprecated updateGlobalState) so the in-memory viewLocalState
28532860
// buffer stays in sync with the durable global write: getValues() merges
28542861
// viewLocalState on top of the ContextProxy values, so an unsynced stale
@@ -3574,6 +3581,16 @@ export class ClineProvider
35743581
}
35753582

35763583
async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) {
3584+
// The same queue that serialises activation, mode switch, save and rename. This deletion
3585+
// snapshots every store it rewrites and compensates them as a group, so a mutation that
3586+
// interleaves with it gets undone by the other's rollback: an activation that started
3587+
// mid-deletion would have its selection rolled back to the profile being removed, and a
3588+
// deletion that started mid-activation would compensate a pin for the profile the user had
3589+
// just left. Nothing below enqueues again, so holding the queue here cannot deadlock.
3590+
return this.enqueueProviderProfileMutation(() => this.deleteProviderProfileUnlocked(profileToDelete))
3591+
}
3592+
3593+
private async deleteProviderProfileUnlocked(profileToDelete: ProviderSettingsEntry) {
35773594
const globalSettings = this.contextProxy.getValues()
35783595
let profileToActivate: string | undefined = globalSettings.currentApiConfigName
35793596

@@ -3639,6 +3656,18 @@ export class ClineProvider
36393656
let deletedWasGlobal = false
36403657
let viewWasPinnedToDeleted = false
36413658

3659+
// Captured outside the write group. When this view was pinned to the deleted profile the write group
3660+
// re-points this view's own pin and overlay onto the survivor, and the conversation's sticky profile
3661+
// name and history entry move with them; none of that is part of restoring the shared stores, so it
3662+
// is snapshotted here and put back by the compensation below when the deletion is rolled back.
3663+
const actingViewPinBefore = this.viewLocalState.currentApiConfigName
3664+
const actingViewOverlayBefore = this.viewLocalState.apiConfiguration
3665+
const actingTaskAtEntry = this.getCurrentTask()
3666+
const actingTaskPreviousName = actingTaskAtEntry?.taskApiConfigName
3667+
const actingTaskPreviousHistoryItem = actingTaskAtEntry
3668+
? this.getTaskHistoryItem(actingTaskAtEntry.taskId)
3669+
: undefined
3670+
36423671
try {
36433672
const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name)
36443673

@@ -3744,12 +3773,14 @@ export class ClineProvider
37443773
// the sticky name moves to the survivor as well, or a later reload re-resolves
37453774
// a profile that no longer exists.
37463775
if (this.getCurrentTask()?.taskApiConfigName === profileToDelete.name) {
3747-
if (survivingSettings) {
3748-
this.updateTaskApiHandlerIfNeeded(survivingSettings, { forceRebuild: true })
3749-
}
37503776
// Strict: a task left naming the deleted profile must fail the deletion so the write
37513777
// group above is rolled back instead of reporting a successful deletion.
3778+
// It runs before the rebuild below on purpose: a rebuilt API handler is not part of that
3779+
// rollback, so a rejected sticky write must not leave this conversation on the survivor's settings.
37523780
await this.persistStickyProviderProfileToCurrentTask(profileToActivate, { strictPersistence: true })
3781+
if (survivingSettings) {
3782+
this.updateTaskApiHandlerIfNeeded(survivingSettings, { forceRebuild: true })
3783+
}
37533784
}
37543785
// Re-pin other live views still buffered on the deleted profile: their
37553786
// buffer and durable viewStates entry would otherwise keep serving the
@@ -3836,6 +3867,46 @@ export class ClineProvider
38363867
`[deleteProviderProfile] failed to restore the shared provider settings: ${restoreError instanceof Error ? restoreError.message : String(restoreError)}`,
38373868
)
38383869
}
3870+
if (recordRestored && viewWasPinnedToDeleted) {
3871+
// The shared stores are back, so this view's own re-point has to go back with them: the caller
3872+
// was told the deletion failed and the deleted profile exists again, so this view must not stay
3873+
// switched to the survivor - live and durable, because a reload reads the durable entry. Only
3874+
// undo a pin this deletion actually moved: if the user or a later activation moved this view
3875+
// onto a different profile during the failure window, that choice is newer than this snapshot.
3876+
// Same three-part rule as the sibling loop above: the record came back, this view was the one
3877+
// pinned to the deleted profile, and the pin still names what this operation wrote.
3878+
try {
3879+
if (this.viewLocalState.currentApiConfigName === profileToActivate) {
3880+
await this._saveViewLocalStateFromMutation({
3881+
currentApiConfigName: actingViewPinBefore,
3882+
apiConfiguration: actingViewOverlayBefore,
3883+
})
3884+
}
3885+
} catch (restoreError) {
3886+
this.log(
3887+
`[deleteProviderProfile] failed to restore this view's own pin: ${
3888+
restoreError instanceof Error ? restoreError.message : String(restoreError)
3889+
}`,
3890+
)
3891+
}
3892+
// The conversation's sticky profile name and its history entry moved with the pin. The strict
3893+
// write puts both back when it is the write that fails, so this only covers a later write in
3894+
// the same group failing after the sticky write had already landed.
3895+
try {
3896+
if (actingTaskAtEntry && actingTaskAtEntry.taskApiConfigName !== actingTaskPreviousName) {
3897+
actingTaskAtEntry.setTaskApiConfigName(actingTaskPreviousName)
3898+
if (actingTaskPreviousHistoryItem) {
3899+
await this.updateTaskHistory(actingTaskPreviousHistoryItem)
3900+
}
3901+
}
3902+
} catch (restoreError) {
3903+
this.log(
3904+
`[deleteProviderProfile] failed to restore the acting task's sticky profile: ${
3905+
restoreError instanceof Error ? restoreError.message : String(restoreError)
3906+
}`,
3907+
)
3908+
}
3909+
}
38393910
try {
38403911
const restore = ClineProvider.persistedViewStateWriteQueue.then(async () => {
38413912
const current = this.getPersistedViewStates({ fresh: true })
@@ -4337,11 +4408,22 @@ export class ClineProvider
43374408
// Snapshot every affected view BEFORE any write starts: a failure in one sibling
43384409
// has to roll back the siblings that already landed too, otherwise the deletion
43394410
// leaves some views pinned to a profile that no longer exists.
4340-
const snapshots = affected.map((instance) => ({
4341-
instance,
4342-
previousPin: instance.viewLocalState.currentApiConfigName,
4343-
previousOverlay: instance.viewLocalState.apiConfiguration,
4344-
}))
4411+
const snapshots = affected.map((instance) => {
4412+
// Decided before any write starts. The strict sticky persistence inside the settled loop moves
4413+
// this conversation's own profile name and its history entry, and neither is part of restoring
4414+
// the view's pin, so all of them are captured here. The eligibility flag is read before the
4415+
// write as well, because that same write re-points taskApiConfigName.
4416+
const task = instance.getCurrentTask()
4417+
return {
4418+
instance,
4419+
previousPin: instance.viewLocalState.currentApiConfigName,
4420+
previousOverlay: instance.viewLocalState.apiConfiguration,
4421+
taskAtSnapshot: task,
4422+
previousTaskApiConfigName: task?.taskApiConfigName,
4423+
previousTaskHistoryItem: task ? instance.getTaskHistoryItem(task.taskId) : undefined,
4424+
taskWasUnderDeletedProfile: task?.taskApiConfigName === deletedProfileName,
4425+
}
4426+
})
43454427

43464428
const results = await Promise.allSettled(
43474429
snapshots.map(async (snapshot) => {
@@ -4380,13 +4462,13 @@ export class ClineProvider
43804462
// hold a task that follows a different sticky profile, and forcing the
43814463
// replacement would silently switch that conversation and rewrite its
43824464
// persisted profile name without the user asking.
4383-
if (snapshot.instance.getCurrentTask()?.taskApiConfigName === deletedProfileName) {
4384-
if (replacementSettings) {
4385-
snapshot.instance.updateTaskApiHandlerIfNeeded(replacementSettings, { forceRebuild: true })
4386-
}
4465+
if (snapshot.taskWasUnderDeletedProfile) {
43874466
// Strict: the allSettled below turns a rejected task-history write into a rejected sibling,
43884467
// which rolls every affected view back and rejects the deletion, instead of leaving that
43894468
// sibling's conversation on the deleted profile while its view was already re-pointed.
4469+
// The matching handler rebuild is deliberately NOT here: a rebuilt API handler is not part of the
4470+
// rollback below, so rebuilding inside the settled loop leaves a sibling whose own write landed
4471+
// holding the replacement's handler while another sibling's rejected write rolls its pin back.
43904472
await snapshot.instance.persistStickyProviderProfileToCurrentTask(replacementName, {
43914473
strictPersistence: true,
43924474
})
@@ -4402,7 +4484,7 @@ export class ClineProvider
44024484
// leaves the view re-pointed. Each restore is awaited on its own so one failing restore
44034485
// cannot skip the next one.
44044486
const restoreFailures: string[] = []
4405-
for (const snapshot of snapshots) {
4487+
for (const [index, snapshot] of snapshots.entries()) {
44064488
// A provider that has begun disposal owns an orphaned viewStates key: viewId comes from
44074489
// the monotonic nextViewId counter, so no future view ever reads that entry again and
44084490
// prunePersistedViewStates() bounds it. Re-checked here too: the sibling can start
@@ -4425,6 +4507,31 @@ export class ClineProvider
44254507
// caller, or a sibling left pinned to the deleted profile is invisible.
44264508
restoreFailures.push(`view ${snapshot.instance.viewId}: ${message}`)
44274509
}
4510+
// The strict sticky persistence inside the settled loop also moved this conversation's own profile
4511+
// name and its history entry. Only the sibling whose own write FAILED is self-restored by that call,
4512+
// so a sibling whose write landed would keep naming the replacement while its pin goes back to the
4513+
// deleted profile - the same split state the pin restore above exists to prevent. Skipped when
4514+
// nothing actually moved: that covers the rejected sibling, which already put both back itself,
4515+
// and a sibling that a disposal guard returned early.
4516+
const task = snapshot.taskAtSnapshot
4517+
if (
4518+
results[index]?.status === "fulfilled" &&
4519+
task &&
4520+
task.taskApiConfigName !== snapshot.previousTaskApiConfigName
4521+
) {
4522+
try {
4523+
task.setTaskApiConfigName(snapshot.previousTaskApiConfigName)
4524+
if (snapshot.previousTaskHistoryItem) {
4525+
await snapshot.instance.updateTaskHistory(snapshot.previousTaskHistoryItem)
4526+
}
4527+
} catch (rollbackError: unknown) {
4528+
const message = rollbackError instanceof Error ? rollbackError.message : String(rollbackError)
4529+
snapshot.instance.log(
4530+
`[rePinViewLocalStateForDeletedProfile] Could not restore the task profile for view ${snapshot.instance.viewId} after a failed re-pin: ${message}`,
4531+
)
4532+
restoreFailures.push(`view ${snapshot.instance.viewId} task profile: ${message}`)
4533+
}
4534+
}
44284535
}
44294536
const original = rejected[0].reason
44304537
if (restoreFailures.length > 0) {
@@ -4445,16 +4552,15 @@ export class ClineProvider
44454552
// conversation keeps sending the deleted profile's configuration even though the view's
44464553
// buffer was re-pointed (the posted state prefers the task's own apiConfiguration).
44474554
for (const snapshot of snapshots) {
4448-
// Only re-point when the task actually runs under the deleted profile:
4449-
// a view pinned to it can hold a task that follows a different sticky
4450-
// profile, and forcing the replacement would silently switch that
4451-
// conversation and rewrite its persisted profile name without the user
4452-
// asking.
4453-
if (snapshot.instance.getCurrentTask()?.taskApiConfigName === deletedProfileName) {
4454-
if (replacementSettings) {
4455-
snapshot.instance.updateTaskApiHandlerIfNeeded(replacementSettings, { forceRebuild: true })
4456-
}
4457-
await snapshot.instance.persistStickyProviderProfileToCurrentTask(replacementName)
4555+
// The only place a task handler moves: a rebuilt handler cannot be compensated, so it waits for
4556+
// every sibling to land. The eligibility flag comes from before the writes, because the strict
4557+
// sticky persistence in the settled loop has already re-pointed taskApiConfigName for exactly
4558+
// this set - reading it here again would skip every conversation that needed the swap. Only
4559+
// conversations that actually ran under the deleted profile are re-pointed: a view pinned to it
4560+
// can hold a task that follows a different sticky profile, and forcing the replacement would
4561+
// silently switch that conversation and rewrite its persisted profile name without the user asking.
4562+
if (snapshot.taskWasUnderDeletedProfile && replacementSettings) {
4563+
snapshot.instance.updateTaskApiHandlerIfNeeded(replacementSettings, { forceRebuild: true })
44584564
}
44594565
await snapshot.instance.postStateToWebview()
44604566
}

0 commit comments

Comments
 (0)