Skip to content

Commit 8fb1532

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix: roll back task-side mode writes when the view mode pin write fails,
rebuild re-pinned siblings' tasks, keep headless task creation working Review-round fixes on the shared code this unit carries: (1) when the view's durable mode pin write rejected inside handleModeSwitchUnlocked, the task-history write and the in-memory task mode had already landed and the caller delivered the submitted message with the new task mode against the view's unchanged mode and mode-linked API configuration - the failure now restores the pre-switch task mode and history entry before rethrowing. (2) rePinViewLocalStateForDeletedProfile updated each sibling's buffer but not its running task: the posted state prefers the task's own apiConfiguration, so a conversation in a view pinned to the deleted profile kept sending the deleted profile's settings - the re-pin now rebuilds the sibling's API handler with the replacement's settings and re-points the task's sticky profile name. (3) The view-state readiness bound rejected every startNewTask whose webview never launched, breaking headless API callers - the bound now only rejects when the webview has launched, and the IPC StartNewTask listener catches rejections since IpcServer does not await async listeners. (4) The mode-ordering test only checked the end state - it now asserts the switch has not been called before the pending initialization state is released. Proofs: ClineProvider.spec 294/294, sticky-mode 34/34 (new pin-write- failure rollback test), parallel-mode 40/40 (sibling re-pin test asserts the task handler rebuild and sticky-name re-point), Task.spec 187/187, api-task-control 25/25 (headless-fallback and launched-stuck tests); negative controls: dropping the rollback call reddens exactly the rollback test, dropping the rebuild reddens exactly the re-pin test, dropping the waitForModeInitialization await reddens exactly the ordering test's in-flight assertion, making the readiness timeout unconditional reddens exactly the headless-fallback test, restores byte-exact (hash match); eslint --max-warnings=0 clean; prettier clean.
1 parent 51e59da commit 8fb1532

6 files changed

Lines changed: 204 additions & 14 deletions

File tree

‎src/core/task/__tests__/Task.spec.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3557,6 +3557,14 @@ describe("Cline", () => {
35573557

35583558
const submitted = task.submitUserMessage("select while initializing", undefined, "architect")
35593559

3560+
// Flush microtasks: the switch must still wait for the pending
3561+
// initialization. Without the await, the mocked switch would already have
3562+
// run here and the end-state assertion could not tell the orders apart.
3563+
for (let i = 0; i < 5; i++) {
3564+
await Promise.resolve()
3565+
}
3566+
expect(mockProvider.handleModeSwitch).not.toHaveBeenCalled()
3567+
35603568
// Resolve the pending provider state only after the submission has started,
35613569
// carrying the provider's pre-switch mode.
35623570
releaseState(Object.assign({} as ProviderState, { mode: "ask" }))

‎src/core/webview/ClineProvider.ts‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2266,6 +2266,10 @@ export class ClineProvider
22662266
): Promise<void> {
22672267
const task = targetTask
22682268

2269+
// Captured for the pin-write rollback below: the mode this task held before
2270+
// the switch (bracket access: _taskMode is private to Task).
2271+
const previousTaskMode = task ? task["_taskMode"] : undefined
2272+
let rollbackTaskWrites: (() => Promise<void>) | undefined
22692273
// A cancelled or timed-out switch must not be partially applied: bail out
22702274
// before the task history / _taskMode writes as well as the durable mode
22712275
// write below. Aborts that land while the history write is in flight are
@@ -2311,6 +2315,21 @@ export class ClineProvider
23112315

23122316
// Only update the task's mode after successful persistence.
23132317
;(task as any)._taskMode = newMode
2318+
2319+
// If the view's durable mode pin below fails after these task-side writes
2320+
// landed, restore them: the caller delivers the message on the caught
2321+
// error, and the new task mode must not run against the view's unchanged
2322+
// mode and mode-linked API configuration.
2323+
const historyItemBefore = taskHistoryItem
2324+
rollbackTaskWrites = async () => {
2325+
task["_taskMode"] = previousTaskMode
2326+
if (historyItemBefore) {
2327+
// Restore only the field this switch changed: re-read the item so
2328+
// fields the running task persisted during the write survive.
2329+
const latest = this.getTaskHistoryItem(task.taskId) ?? historyItemBefore
2330+
await this.updateTaskHistory({ ...latest, mode: historyItemBefore.mode })
2331+
}
2332+
}
23142333
} catch (error) {
23152334
// If persistence fails, log the error but don't update the in-memory state.
23162335
this.log(
@@ -2337,7 +2356,26 @@ export class ClineProvider
23372356
if (viewScopedSwitch) {
23382357
// Pin the view's durable per-view mode first: the pin is this view's own
23392358
// selection and must not be undone if the shared write later fails.
2340-
await this.saveViewState("mode", newMode)
2359+
// If the pin write itself fails, the task-side writes above are already
2360+
// applied: roll them back before rethrowing so the caller cannot deliver
2361+
// the message with the new task mode while the view's mode and mode-linked
2362+
// API configuration stayed old.
2363+
try {
2364+
await this.saveViewState("mode", newMode)
2365+
} catch (error) {
2366+
if (rollbackTaskWrites) {
2367+
try {
2368+
await rollbackTaskWrites()
2369+
} catch (rollbackError) {
2370+
this.log(
2371+
`[handleModeSwitch] Failed to roll back task mode writes after the view mode pin write failed: ${
2372+
rollbackError instanceof Error ? rollbackError.message : String(rollbackError)
2373+
}`,
2374+
)
2375+
}
2376+
}
2377+
throw error
2378+
}
23412379

23422380
// setValue (not the deprecated updateGlobalState) so the in-memory viewLocalState
23432381
// buffer stays in sync with the durable global write: getValues() merges
@@ -2959,6 +2997,16 @@ export class ClineProvider
29592997

29602998
// Direct private access: compile-time safe across sibling instances.
29612999
await instance._saveViewLocalStateFromMutation(values)
3000+
// The active task of a view pinned to the deleted profile holds its own API
3001+
// handler and sticky profile name built from the deleted profile: rebuild
3002+
// the handler with the replacement's settings and re-point the sticky name,
3003+
// or the running conversation keeps sending the deleted profile's
3004+
// configuration even though the view's buffer was re-pointed (the posted
3005+
// state prefers the task's own apiConfiguration).
3006+
if (replacementSettings) {
3007+
instance.updateTaskApiHandlerIfNeeded(replacementSettings, { forceRebuild: true })
3008+
}
3009+
await instance.persistStickyProviderProfileToCurrentTask(replacementName)
29623010
await instance.postStateToWebview()
29633011
}),
29643012
)

‎src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1380,7 +1380,19 @@ describe("ClineProvider - Parallel Mode Support", () => {
13801380
vi.spyOn(providerA.providerSettingsManager, "listConfig").mockResolvedValue([
13811381
{ id: "replacement-id", name: "replacement-profile", apiProvider: providerIdentifiers.openrouter },
13821382
])
1383-
// The pre-return re-pin resolves the replacement's settings through the manager.
1383+
// B has an active task built from the doomed profile's settings: the posted
1384+
// state prefers the task's own apiConfiguration, so the re-pin must rebuild
1385+
// the task's handler and re-point its sticky profile name too.
1386+
const siblingTask = {
1387+
apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "doomed-key" },
1388+
updateApiConfiguration: vi.fn(),
1389+
setTaskApiConfigName: vi.fn(),
1390+
}
1391+
vi.spyOn(providerB, "getCurrentTask").mockReturnValue(
1392+
siblingTask as unknown as ReturnType<typeof providerB.getCurrentTask>,
1393+
)
1394+
1395+
// The pre-return re-pin // The pre-return re-pin resolves the replacement's settings through the manager.
13841396
vi.spyOn(providerA.providerSettingsManager, "getProfile").mockImplementation(
13851397
async (_args: { name?: string; id?: string }) =>
13861398
({
@@ -1405,6 +1417,18 @@ describe("ClineProvider - Parallel Mode Support", () => {
14051417
openRouterBaseUrl: "repl-url",
14061418
})
14071419
expect((await providerB.getState()).currentApiConfigName).toBe("replacement-profile")
1420+
// The active task's handler is rebuilt with the replacement's settings and
1421+
// its sticky profile name re-points to the replacement: without this the
1422+
// running conversation keeps sending the deleted profile's configuration.
1423+
// objectContaining: the replacement's settings carry the profile id alongside
1424+
// the provider fields (the name is stripped, the id is not).
1425+
expect(siblingTask.updateApiConfiguration).toHaveBeenCalledWith(
1426+
expect.objectContaining({
1427+
apiProvider: providerIdentifiers.openrouter,
1428+
openRouterBaseUrl: "repl-url",
1429+
}),
1430+
)
1431+
expect(siblingTask.setTaskApiConfigName).toHaveBeenCalledWith("replacement-profile")
14081432

14091433
await providerA.dispose()
14101434
await providerB.dispose()

‎src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,55 @@ describe("ClineProvider - Sticky Mode", () => {
627627
expect(mockContext.globalState.update).not.toHaveBeenCalledWith("mode", "architect")
628628
})
629629

630+
it("rolls back the task mode and history when the view mode pin write fails", async () => {
631+
// Same typed double as the abort rollback test: the private members stay
632+
// out of the Object.assign intersection type.
633+
const mockTask = Object.assign(
634+
{} as Task,
635+
{
636+
taskId: "test-task-id",
637+
taskMode: "code",
638+
_taskMode: "code",
639+
emit: vi.fn(),
640+
saveClineMessages: vi.fn(),
641+
clineMessages: [],
642+
apiConversationHistory: [],
643+
updateApiConfiguration: vi.fn(),
644+
} as Partial<Task>,
645+
)
646+
647+
const historyItem: HistoryItem = {
648+
id: "test-task-id",
649+
ts: Date.now(),
650+
task: "Test task",
651+
mode: "code",
652+
number: 1,
653+
tokensIn: 0,
654+
tokensOut: 0,
655+
cacheWrites: 0,
656+
cacheReads: 0,
657+
totalCost: 0,
658+
}
659+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
660+
661+
// The task-history write lands; the view's durable mode pin write then
662+
// rejects. The caller (submitUserMessage) logs the failure and delivers the
663+
// message, so the task-side writes must not stand.
664+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
665+
vi.spyOn(provider, "saveViewState").mockRejectedValueOnce(new Error("pin write failed"))
666+
await provider.addClineToStack(mockTask)
667+
668+
await expect(provider["handleModeSwitchUnlocked"]("architect", mockTask)).rejects.toThrow(
669+
"pin write failed",
670+
)
671+
672+
// No partial application: the task keeps its previous mode and the history
673+
// entry is restored, so the delivered message cannot run the new task mode
674+
// against the view's unchanged mode and mode-linked API configuration.
675+
expect(mockTask["_taskMode"]).toBe("code")
676+
expect(updateTaskHistorySpy).toHaveBeenLastCalledWith(historyItem)
677+
})
678+
630679
it("keeps the cancellation result when the abort rollback write itself fails", async () => {
631680
const mockTask = Object.assign(
632681
{} as Task,

‎src/extension/__tests__/api-task-control.spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,44 @@ describe("API task controls", () => {
229229
}
230230
})
231231

232+
it("continues with the shared-state fallback when the webview never launches", async () => {
233+
// Headless API callers create tasks without ever launching a webview: the
234+
// readiness bound expiring must not reject them - the pre-existing
235+
// shared-state fallback still applies.
236+
vi.useFakeTimers()
237+
sidebarProvider.viewLaunched = false
238+
sidebarProvider.viewStateReadiness = new Promise<void>(() => {})
239+
240+
const started = api.startNewTask({ configuration, text: "headless task" })
241+
await vi.advanceTimersByTimeAsync(3000)
242+
243+
try {
244+
await expect(started).resolves.toBe("sidebar-task")
245+
} finally {
246+
vi.useRealTimers()
247+
}
248+
})
249+
250+
it("still rejects when the webview launched but the readiness bound expires", async () => {
251+
// A launched view whose readiness never settles is a real failure: the
252+
// task must not consume shared defaults behind a live view.
253+
vi.useFakeTimers()
254+
sidebarProvider.viewStateReadiness = new Promise<void>(() => {})
255+
256+
const started = api.startNewTask({ configuration, text: "stuck view task" })
257+
// Attach the rejection handler before advancing: with fake timers the
258+
// bound fires during the advance, and a handler attached afterwards would
259+
// see the rejection briefly unhandled.
260+
const settled = expect(started).rejects.toThrow("Timed out waiting for the view state to become ready")
261+
await vi.advanceTimersByTimeAsync(3000)
262+
263+
try {
264+
await settled
265+
} finally {
266+
vi.useRealTimers()
267+
}
268+
})
269+
232270
it("fails task creation when the provider is disposed while the readiness wait is pending", async () => {
233271
// Disposal resolves viewStateReadiness to release waiters (see ClineProvider.dispose)
234272
// and marks the provider disposed first: the wait must then fail instead of

‎src/extension/api.ts‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,15 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI, RooC
106106
this.log(
107107
`[API] StartNewTask -> ${command.data.text}, ${JSON.stringify(command.data.configuration)}`,
108108
)
109-
await this.startNewTask(command.data)
109+
try {
110+
await this.startNewTask(command.data)
111+
} catch (error) {
112+
// IpcServer does not await async listeners: an uncaught rejection here
113+
// would surface as an unhandled rejection in the extension host.
114+
this.log(
115+
`[API] StartNewTask failed: ${error instanceof Error ? error.message : String(error)}`,
116+
)
117+
}
110118
break
111119
case TaskCommandName.CancelTask:
112120
this.log(`[API] CancelTask`)
@@ -225,17 +233,32 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI, RooC
225233
// hang task creation; if the bound expires, fail task creation rather than start
226234
// the task against stale shared defaults.
227235
let readinessTimer: ReturnType<typeof setTimeout> | undefined
228-
await Promise.race([
229-
provider.viewStateReadiness,
230-
new Promise<never>((_, reject) => {
231-
readinessTimer = setTimeout(
232-
() => reject(new Error("Timed out waiting for the view state to become ready")),
233-
3000,
234-
)
235-
}),
236-
]).finally(() => {
237-
clearTimeout(readinessTimer)
238-
})
236+
try {
237+
await Promise.race([
238+
provider.viewStateReadiness,
239+
new Promise<never>((_, reject) => {
240+
readinessTimer = setTimeout(
241+
() => reject(new Error("Timed out waiting for the view state to become ready")),
242+
3000,
243+
)
244+
}),
245+
]).finally(() => {
246+
clearTimeout(readinessTimer)
247+
})
248+
} catch (error) {
249+
if (
250+
provider.viewLaunched ||
251+
!(error instanceof Error) ||
252+
error.message !== "Timed out waiting for the view state to become ready"
253+
) {
254+
throw error
255+
}
256+
// Headless API callers create tasks without ever launching a webview: the
257+
// readiness bound expiring keeps the pre-existing shared-state fallback.
258+
// A launched view whose readiness failed is a real failure and rethrows.
259+
this.log("[API#startNewTask] webview not launched; continuing in headless mode")
260+
}
261+
239262
// Disposal resolves viewStateReadiness to release waiters: a provider torn down while
240263
// the wait was pending must fail task creation rather than create a task against the
241264
// disposed view (its posts and task registration would be orphaned).

0 commit comments

Comments
 (0)