Skip to content

Commit a591a1e

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(webview): compensate a profile deletion that the mutation timeout cancels
Chaining the provider profile mutation queue to the running mutation removed the assumption the deletion path was written against. Its catch block skipped compensation whenever the abort signal had fired, because under the old contract a later queued mutation could already have written those stores and a rollback would have overwritten it. Nothing can run in between any more, so skipping the rollback no longer protects anyone - it leaves a half-applied deletion behind: settings deleted while the profile list and the shared selection still name the profile, or the list pruned while the selection and the view pins keep pointing at it. Compensation now always runs, and the three comments that described the superseded contract are corrected. The test that pinned the old behaviour asserted the rollback never replayed; under the new contract it asserts the opposite, and a new red-first test covers the earlier cancellation point, between the settings commit and the profile-list write.
1 parent 999c651 commit a591a1e

2 files changed

Lines changed: 76 additions & 20 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2654,10 +2654,10 @@ export class ClineProvider
26542654
)
26552655
}
26562656

2657-
// The queue advances on the timeout-bounded caller result, so a cancelled deletion
2658-
// must stop touching durable state here. Everything below rewrites the shared
2659-
// selection, the shared provider settings, and other views' pins - any of those
2660-
// landing after the next queued mutation has run would overwrite that mutation.
2657+
// Stopping here is still deliberate, but for a different reason than before the queue
2658+
// was chained: throwing routes through the compensation below, which now always runs,
2659+
// so the deletion is rolled back rather than left half-applied. Continuing into the
2660+
// rewrites below would spend writes on a caller that has already been released.
26612661
if (signal.aborted) {
26622662
this.log(
26632663
`deleteProviderProfile: cancelled before the selection/settings rewrite; the deletion stopped with the profile list already updated.`,
@@ -2709,16 +2709,11 @@ export class ClineProvider
27092709
// deleted profile's name and configuration.
27102710
await this.rePinViewLocalStateForDeletedProfile(profileToDelete.name, profileToActivate, survivingSettings)
27112711
} catch (error: unknown) {
2712-
// A cancelled (timed-out) deletion has already yielded its place in the queue: the next
2713-
// mutation may have written these stores since, so replaying this operation's rollback
2714-
// would overwrite IT. Stop at the cancellation point and report the partial state
2715-
// instead of compensating.
2716-
if (signal.aborted) {
2717-
this.log(
2718-
`deleteProviderProfile: cancelled by the mutation timeout (${error instanceof Error ? error.message : String(error)}); skipping compensation so it cannot overlap the next queued mutation. Durable state reflects the steps that had already landed.`,
2719-
)
2720-
throw error
2721-
}
2712+
// A cancelled (timed-out) deletion still compensates. The queue is chained to this run,
2713+
// so no later mutation can have written these stores in the meantime: holding the queue
2714+
// until the run settles is exactly what makes the rollback safe again. Skipping it would
2715+
// leave a half-applied deletion behind - settings gone while the profile list or the
2716+
// selection still name the profile, or the list pruned while pins keep pointing at it.
27222717

27232718
// Compensate every store that already landed, each awaited on its own so one
27242719
// failing restore cannot skip the next one. The deletion simply did not happen:

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

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4080,7 +4080,7 @@ describe("ClineProvider", () => {
40804080
await failingSibling.dispose()
40814081
})
40824082

4083-
it("stops writing durable state once the deletion's abort signal fires", async () => {
4083+
it("rolls back a deletion that the abort signal cancels after the profile-list write", async () => {
40844084
const provider = new ClineProvider(
40854085
mockContext,
40864086
mockOutputChannel,
@@ -4110,8 +4110,8 @@ describe("ClineProvider", () => {
41104110
saveConfig,
41114111
getProfile: vi.fn().mockImplementation(async ({ name }) => {
41124112
// The cancellation lands on the survivor lookup, i.e. after the settings commit
4113-
// and the profile-list write - the point where the queue has already handed
4114-
// the turn to the next mutation.
4113+
// and the profile-list write. The queue is still held by this run, so the rollback
4114+
// below cannot race a later mutation.
41154115
if (name === "keeper-profile") {
41164116
controller.abort()
41174117
}
@@ -4135,14 +4135,21 @@ describe("ClineProvider", () => {
41354135
"Profile deletion was cancelled before the selection and settings rewrite",
41364136
)
41374137

4138-
// The list write landed before the cancellation point...
4139-
expect(provider.contextProxy.getValue("listApiConfigMeta")).toEqual([keeperProfile])
4138+
// The list write had landed before the cancellation point, and the compensation replays
4139+
// the snapshot over it: the deletion did not happen, so the list names both profiles again.
4140+
expect(provider.contextProxy.getValue("listApiConfigMeta")).toEqual([doomedProfile, keeperProfile])
4141+
// The forward rewrites still stop at the cancellation point: the shared selection and the
4142+
// shared provider settings were never touched, so there is nothing to undo for them, and
4143+
// no write is spent on a caller that has already been released.
41404144
// ...but the shared selection, the shared provider settings and any sibling view's pin
41414145
// must not be rewritten afterwards, and the rollback must not replay either: the next
41424146
// queued mutation owns those stores now.
41434147
expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("doomed-profile")
41444148
expect(provider.contextProxy.getValue("apiProvider")).not.toBe(providerIdentifiers.anthropic)
4145-
expect(saveConfig).not.toHaveBeenCalled()
4149+
expect(saveConfig).toHaveBeenCalledWith(
4150+
"doomed-profile",
4151+
expect.objectContaining({ openRouterApiKey: "doomed-secret" }),
4152+
)
41464153
await provider.dispose()
41474154
})
41484155

@@ -4272,6 +4279,60 @@ describe("ClineProvider", () => {
42724279
await provider.dispose()
42734280
})
42744281
})
4282+
it("compensates a deletion that the mutation timeout cancelled after the settings commit", async () => {
4283+
const provider = new ClineProvider(
4284+
mockContext,
4285+
mockOutputChannel,
4286+
"sidebar",
4287+
new ContextProxy(mockContext),
4288+
new WebviewFocusTracker(),
4289+
)
4290+
const doomedProfile: ProviderSettingsEntry = {
4291+
name: "doomed-profile",
4292+
id: "doomed-id",
4293+
apiProvider: providerIdentifiers.openrouter,
4294+
}
4295+
const keeperProfile: ProviderSettingsEntry = {
4296+
name: "keeper-profile",
4297+
id: "keeper-id",
4298+
apiProvider: providerIdentifiers.anthropic,
4299+
}
4300+
const doomedSettings = { apiProvider: providerIdentifiers.openrouter, apiKey: "doomed-key" }
4301+
const keeperSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "keeper-key" }
4302+
// The timeout fires while the deletion is between its two durable writes: the abort lands
4303+
// right after the settings commit, which is the point where a half-applied deletion would
4304+
// otherwise be left behind in storage.
4305+
const controller = new AbortController()
4306+
const saveConfig = vi.fn().mockResolvedValue(undefined)
4307+
const deleteConfig = vi.fn().mockImplementation(async () => {
4308+
controller.abort()
4309+
})
4310+
// @ts-ignore - Replace providerSettingsManager with a test double.
4311+
provider.providerSettingsManager = {
4312+
deleteConfig,
4313+
saveConfig,
4314+
getProfile: vi
4315+
.fn()
4316+
.mockResolvedValueOnce({ name: "doomed-profile", ...doomedSettings })
4317+
.mockResolvedValueOnce({ name: "keeper-profile", ...keeperSettings }),
4318+
}
4319+
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
4320+
// The compensation path logs its outcome; silence it so the worker does not race a console
4321+
// flush against environment teardown.
4322+
vi.spyOn(provider, "log").mockImplementation(() => {})
4323+
await provider.contextProxy.setValue("listApiConfigMeta", [doomedProfile, keeperProfile])
4324+
await provider.contextProxy.setValue("currentApiConfigName", "doomed-profile")
4325+
4326+
await expect(provider["deleteProviderProfileUnlocked"](doomedProfile, controller.signal)).rejects.toThrow(
4327+
"Profile deletion was cancelled before the profile-list write",
4328+
)
4329+
4330+
// The queue holds every later mutation until this run settles, so a cancelled deletion is
4331+
// rolled back like a failed one: the settings and the profile list must agree again.
4332+
expect(saveConfig).toHaveBeenCalledWith("doomed-profile", expect.objectContaining({ apiKey: "doomed-key" }))
4333+
expect(provider.contextProxy.getValue("listApiConfigMeta")).toEqual([doomedProfile, keeperProfile])
4334+
await provider.dispose()
4335+
})
42754336

42764337
describe("local state isolation", () => {
42774338
it("should isolate mode state between instances", async () => {

0 commit comments

Comments
 (0)