Skip to content

Commit a71cf5b

Browse files
easonliang28easonliang28
committed
fix(checkpoints): recover later writes after a transient checkpoint failure
Port of the row-12 fixes for the shared checkpointSave block: a failure-driven disable now sets checkpointsDisabledByFailure, and the next write-metadata save attempts one recovery (reuse a live service handle, else retry init) instead of failing every later write in the task. Enabling checkpoints mid-task no longer retroactively fails writes: the throw is keyed on the failure flag, not live settings. WriteToFileTool skips the per-write checkpoint when an existing file's approval diff is empty (nothing to stage, no commit, no false failure).
1 parent 548231c commit a71cf5b

6 files changed

Lines changed: 116 additions & 48 deletions

File tree

‎src/core/checkpoints/__tests__/checkpointJournal.test.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,11 +268,9 @@ describe("checkpointSave change-journal wiring (B2)", () => {
268268
// Documented contract: user-disabled checkpoints means the absence of
269269
// rollback metadata is the user's choice, so the save must not fail the
270270
// write tool (no journal entry, no change card, no throw). Genuine
271-
// init-failure coverage lives in checkpointSave.spec.ts. The
272-
// persisted setting agrees the user disabled checkpoints (an unset
273-
// setting would mean an earlier failure flipped the task flag).
271+
// init-failure coverage lives in checkpointSave.spec.ts. No
272+
// checkpointsDisabledByFailure flag is set: the flip is a user choice.
274273
mockTask.enableCheckpoints = false
275-
mockProvider.getState = vi.fn().mockResolvedValue({ enableCheckpoints: false })
276274

277275
await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toBeUndefined()
278276
expect(saveCheckpointSpy).not.toHaveBeenCalled()

‎src/core/checkpoints/__tests__/checkpointSave.spec.ts‎

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import fs from "fs/promises"
22
import os from "os"
33
import path from "path"
44

5-
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
5+
import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from "vitest"
66

77
import type { Task } from "../../task/Task"
88
import { journalPath } from "../changeJournal"
@@ -251,12 +251,37 @@ describe("checkpointSave change-card emission (B3a)", () => {
251251
expect(say).not.toHaveBeenCalled()
252252
})
253253

254-
it("fails a write-metadata save when an earlier failure flipped the task flag", async () => {
255-
// getCheckpointService and the save-failure handlers flip
256-
// task.enableCheckpoints off; the persisted setting still says the user
257-
// wants checkpoints, so a later write-metadata save must throw too
258-
// instead of silently proceeding with no rollback metadata.
259-
const { task, say } = makeTask({ enableCheckpoints: false, state: { enableCheckpoints: true } })
254+
it("recovers the next write-metadata save after a transient save failure", async () => {
255+
// A failed save flips the task flag (checkpointsDisabledByFailure).
256+
// The next write-metadata save must attempt recovery - reusing the
257+
// live service handle - instead of failing every later write in the
258+
// task with a misleading init-failure error.
259+
const { task } = makeTask({})
260+
const svc = task.checkpointService as unknown as { saveCheckpoint: Mock }
261+
svc.saveCheckpoint = vi
262+
.fn()
263+
.mockRejectedValueOnce(new Error("index.lock contention"))
264+
.mockResolvedValue({ commit: "sha-recovered-1" })
265+
266+
await expect(
267+
checkpointSave(task, false, true, { path: "src/a.ts", operation: "create", diff: "+x" }),
268+
).rejects.toThrow(/Checkpoint save failed after a file write/)
269+
expect(task.checkpointsDisabledByFailure).toBe(true)
270+
271+
await expect(
272+
checkpointSave(task, false, true, { path: "src/b.ts", operation: "create", diff: "+y" }),
273+
).resolves.toBeDefined()
274+
expect(task.enableCheckpoints).toBe(true)
275+
const journalRaw = await fs.readFile(journalPath(globalStorageDir, "task-card"), "utf8")
276+
expect(journalRaw).toContain("src/b.ts")
277+
})
278+
279+
it("fails a write-metadata save when recovery after an earlier failure cannot re-initialize", async () => {
280+
// No live handle to reuse and initialization fails again: the write
281+
// has no rollback metadata and must surface the failure.
282+
const { task, say } = makeTask({ storageDir: null })
283+
task.enableCheckpoints = false
284+
task.checkpointsDisabledByFailure = true
260285

261286
await expect(
262287
checkpointSave(task, false, true, { path: "src/a.ts", operation: "create", diff: "+x" }),
@@ -265,6 +290,24 @@ describe("checkpointSave change-card emission (B3a)", () => {
265290
expect(say).not.toHaveBeenCalled()
266291
})
267292

293+
it("resolves silently when checkpoints were off for the task and no failure occurred", async () => {
294+
// Enabling checkpoints in settings mid-task does not retroactively
295+
// fail writes: the task was constructed without checkpoints and no
296+
// failure flipped the flag, so the absence of rollback metadata is
297+
// the construction-time user choice.
298+
const { task, say, saveCheckpoint } = makeTask({
299+
enableCheckpoints: false,
300+
state: { enableCheckpoints: true },
301+
})
302+
303+
await expect(
304+
checkpointSave(task, false, true, { path: "src/a.ts", operation: "create", diff: "+x" }),
305+
).resolves.toBeUndefined()
306+
307+
expect(saveCheckpoint).not.toHaveBeenCalled()
308+
expect(say).not.toHaveBeenCalled()
309+
})
310+
268311
it("keeps the journal append when a card emission failure occurs", async () => {
269312
const { task, say } = makeTask({})
270313
say.mockImplementation(async (type: string) => {

‎src/core/checkpoints/index.ts‎

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,16 @@ function sendCheckpointInitWarn(task: Task, type?: "WAIT_TIMEOUT" | "INIT_TIMEOU
2828
})
2929
}
3030

31+
// Every checkpoint disable in this module is failure-driven (init, save,
32+
// restore, or diff). Route the flip through this helper so checkpointSave
33+
// can tell an earlier failure apart from a user who never enabled
34+
// checkpoints: the persisted setting cannot make that distinction (it can
35+
// change after the task started).
36+
function disableCheckpointsAfterFailure(task: Task): void {
37+
task.enableCheckpoints = false
38+
task.checkpointsDisabledByFailure = true
39+
}
40+
3141
export async function getCheckpointService(task: Task, { interval = 250 }: { interval?: number } = {}) {
3242
if (!task.enableCheckpoints) {
3343
return undefined
@@ -59,15 +69,15 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int
5969

6070
if (!workspaceDir) {
6171
log("[Task#getCheckpointService] workspace folder not found, disabling checkpoints")
62-
task.enableCheckpoints = false
72+
disableCheckpointsAfterFailure(task)
6373
return undefined
6474
}
6575

6676
const globalStorageDir = provider?.context.globalStorageUri.fsPath
6777

6878
if (!globalStorageDir) {
6979
log("[Task#getCheckpointService] globalStorageDir not found, disabling checkpoints")
70-
task.enableCheckpoints = false
80+
disableCheckpointsAfterFailure(task)
7181
return undefined
7282
}
7383

@@ -101,7 +111,7 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int
101111
)
102112
if (!task?.checkpointService) {
103113
sendCheckpointInitWarn(task, "INIT_TIMEOUT", task.checkpointTimeout)
104-
task.enableCheckpoints = false
114+
disableCheckpointsAfterFailure(task)
105115
return undefined
106116
} else {
107117
sendCheckpointInitWarn(task)
@@ -126,7 +136,7 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int
126136
sendCheckpointInitWarn(task, "INIT_TIMEOUT", task.checkpointTimeout)
127137
}
128138
log(`[Task#getCheckpointService] ${err.message}`)
129-
task.enableCheckpoints = false
139+
disableCheckpointsAfterFailure(task)
130140
task.checkpointServiceInitializing = false
131141
return undefined
132142
}
@@ -143,7 +153,7 @@ async function checkGitInstallation(
143153

144154
if (!gitInstalled) {
145155
log("[Task#getCheckpointService] Git is not installed, disabling checkpoints")
146-
task.enableCheckpoints = false
156+
disableCheckpointsAfterFailure(task)
147157
task.checkpointServiceInitializing = false
148158

149159
// Show user-friendly notification
@@ -192,7 +202,7 @@ async function checkGitInstallation(
192202
} catch (err) {
193203
log("[Task#getCheckpointService] caught unexpected error in on('checkpoint'), disabling checkpoints")
194204
console.error(err)
195-
task.enableCheckpoints = false
205+
disableCheckpointsAfterFailure(task)
196206
}
197207
})
198208

@@ -202,12 +212,12 @@ async function checkGitInstallation(
202212
await service.initShadowGit()
203213
} catch (err) {
204214
log(`[Task#getCheckpointService] initShadowGit -> ${err.message}`)
205-
task.enableCheckpoints = false
215+
disableCheckpointsAfterFailure(task)
206216
}
207217
} catch (err) {
208218
log(`[Task#getCheckpointService] Unexpected error during Git check: ${err.message}`)
209219
console.error("Git check error:", err)
210-
task.enableCheckpoints = false
220+
disableCheckpointsAfterFailure(task)
211221
task.checkpointServiceInitializing = false
212222
}
213223
}
@@ -276,27 +286,31 @@ export async function checkpointSave(
276286
// the user expects checkpoints" (throw).
277287
const checkpointsRequested = task.enableCheckpoints
278288

279-
const service = await getCheckpointService(task)
289+
let service = await getCheckpointService(task)
280290

281291
if (!service) {
282-
let requested = checkpointsRequested
283-
if (hasWriteMetadata && !requested) {
284-
// The task flag also flips off when an EARLIER failure disabled
285-
// checkpoints (a failed service init, or the save-failure handlers
286-
// below). The persisted setting still carries the user's intent
287-
// (unset defaults to enabled, matching Task construction), so a
288-
// write-metadata save after such a failure must throw too instead
289-
// of silently proceeding with no rollback metadata. Only a user who
290-
// disabled checkpoints in settings resolves silently.
291-
const state = await task.providerRef.deref()?.getState()
292-
requested = state?.enableCheckpoints ?? true
292+
if (hasWriteMetadata && !checkpointsRequested && task.checkpointsDisabledByFailure) {
293+
// An earlier failure (init or save) disabled checkpoints while the
294+
// user never turned them off. Attempt one recovery so a single
295+
// transient failure (e.g. index.lock contention) does not
296+
// permanently fail every later write in the task: reuse a live
297+
// service handle when one exists (a save failure does not
298+
// invalidate the handle), otherwise retry initialization once. The
299+
// persisted setting cannot make this decision - it can change
300+
// after the task started, and enabling checkpoints mid-task must
301+
// not retroactively fail writes that were never promised a
302+
// checkpoint.
303+
task.enableCheckpoints = true
304+
service = task.checkpointService ?? (await getCheckpointService(task))
293305
}
294-
if (hasWriteMetadata && requested) {
295-
throw new Error(
296-
"Checkpoint save with write metadata could not initialize the checkpoint service; the write has no rollback metadata.",
297-
)
306+
if (!service) {
307+
if (hasWriteMetadata && (checkpointsRequested || task.checkpointsDisabledByFailure)) {
308+
throw new Error(
309+
"Checkpoint save with write metadata could not initialize the checkpoint service; the write has no rollback metadata.",
310+
)
311+
}
312+
return
298313
}
299-
return
300314
}
301315

302316
// A save that carries write metadata needs a stable journal location for
@@ -334,7 +348,7 @@ export async function checkpointSave(
334348
result = await savePromise
335349
} catch (err) {
336350
console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err)
337-
task.enableCheckpoints = false
351+
disableCheckpointsAfterFailure(task)
338352
throw new Error(
339353
`Checkpoint save failed after a file write; the write has no rollback metadata: ${err instanceof Error ? err.message : String(err)}`,
340354
{ cause: err },
@@ -343,7 +357,7 @@ export async function checkpointSave(
343357
} else {
344358
result = await savePromise.catch((err) => {
345359
console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err)
346-
task.enableCheckpoints = false
360+
disableCheckpointsAfterFailure(task)
347361
})
348362
}
349363

@@ -518,7 +532,7 @@ export async function checkpointRestore(
518532
provider?.cancelTask()
519533
} catch {
520534
provider?.log("[checkpointRestore] disabling checkpoints for this task")
521-
task.enableCheckpoints = false
535+
disableCheckpointsAfterFailure(task)
522536
}
523537
}
524538

@@ -608,6 +622,6 @@ export async function checkpointDiff(task: Task, { commitHash, mode }: Checkpoin
608622
} catch {
609623
const provider = task.providerRef.deref()
610624
provider?.log("[checkpointDiff] disabling checkpoints for this task")
611-
task.enableCheckpoints = false
625+
disableCheckpointsAfterFailure(task)
612626
}
613627
}

‎src/core/task/Task.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
511511
checkpointTimeout: number
512512
checkpointService?: RepoPerTaskCheckpointService
513513
checkpointServiceInitializing = false
514+
// Set when checkpoints were disabled by a failure (init, save, restore,
515+
// or diff), so checkpointSave can distinguish an earlier failure from a
516+
// user who never enabled checkpoints, without inferring it from live
517+
// settings (which can change after the task started).
518+
checkpointsDisabledByFailure = false
514519

515520
// Message Queue Service
516521
public readonly messageQueueService: MessageQueueService

‎src/core/tools/WriteToFileTool.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
191191
task.didEditFile = true
192192
didWrite = true
193193

194-
if (perWriteCheckpoints) {
194+
// A write whose approval diff is empty leaves the shadow repository
195+
// nothing to stage: the per-write checkpoint would resolve without
196+
// a commit and the write-metadata rejection would report failure
197+
// for a write that succeeded. Skip the checkpoint for that no-op
198+
// (the write result is still pushed), mirroring EditFileTool's
199+
// unchanged-content early return.
200+
const contentUnchanged = fileExists && approvalDiff === ""
201+
202+
if (perWriteCheckpoints && !contentUnchanged) {
195203
// B2: the change-journal entry for this write is appended inside
196204
// checkpointSave (the hook stays a single call site), keyed by the
197205
// checkpoint commit that call produces. Await so the checkpoint

‎src/core/tools/__tests__/writeToFileTool.spec.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -619,18 +619,18 @@ describe("writeToFileTool", () => {
619619
})
620620
})
621621

622-
it("omits diff stats from the checkpoint write when the approval diff is empty", async () => {
622+
it("skips the checkpoint when an existing file's content is unchanged", async () => {
623623
// Writing identical content to an existing file produces an empty
624-
// approval diff, so the checkpoint write carries no diffStats.
624+
// approval diff: the shadow repository has nothing to stage, so a
625+
// checkpoint would resolve without a commit and the write-metadata
626+
// rejection would report failure for a write that succeeded. The
627+
// write result is still pushed.
625628
vi.mocked(formatResponse.createPrettyPatch).mockReturnValueOnce("")
626629

627630
await executeWriteFileTool({}, { fileExists: true })
628631

629-
expect(mockedCheckpointSave).toHaveBeenCalledOnce()
630-
expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, {
631-
path: testFilePath,
632-
operation: "update",
633-
})
632+
expect(mockedCheckpointSave).not.toHaveBeenCalled()
633+
expect(mockPushToolResult).toHaveBeenCalled()
634634
})
635635

636636
it("threads autoApproved into the checkpoint write for auto-approved steps", async () => {

0 commit comments

Comments
 (0)