Skip to content

Commit 7752664

Browse files
author
easonLiangWorldedtech
committed
fix(diff-view): roll back the created file and dirs when open() fails early
DiffViewProvider.open() creates the parent directories and writes an empty placeholder BEFORE it awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and revertChanges() bailed out on `!this.activeDiffEditor` - leaving the placeholder and the new directories on disk. The next execute() then saw an empty file and treated the requested new file as an existing one, so a denial preserved the debris instead of removing it. The streaming-failure cleanup in handlePartial() calls exactly this rollback, so the debris was reachable from the new path in this unit. The filesystem rollback now runs regardless of the editor: only the document work (save the dirty buffer, close the diff views and the tab) needs one. removeCreatedFile/removeCreatedDir tolerate ENOENT, since the failed open may never have written the placeholder - that must not abort the rest of the cleanup. Tests: 'revertChanges() removes the placeholder and created dirs when open() failed before the editor existed' (unlink for the relPath, rmdir in reverse order) and 'revertChanges() tolerates a placeholder that was never written' (ENOENT rejection does not propagate, delete still attempted). Pin: restoring the old `!this.activeDiffEditor` early return fails both. Local: integrations/editor + writeToFileTool.spec + core/task + assistant-message = 786 passed (the single remaining failure, saveChanges default delay, reproduces without these changes); tsc 0; eslint 0 err / 0 warn on both files.
1 parent e0b0e0e commit 7752664

2 files changed

Lines changed: 100 additions & 10 deletions

File tree

‎src/integrations/editor/DiffViewProvider.ts‎

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ import { Task } from "../../core/task/Task"
2121

2222
import { DecorationController } from "./DecorationController"
2323

24+
/** Narrow ENOENT test so a rollback can tolerate a file or dir that never landed. */
25+
function isEnoent(error: unknown): boolean {
26+
return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT"
27+
}
28+
2429
export const DIFF_VIEW_URI_SCHEME = "cline-diff"
2530
export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes"
2631

@@ -513,13 +518,38 @@ export class DiffViewProvider {
513518
return JSON.stringify(result)
514519
}
515520

521+
/**
522+
* Remove a file this edit created. Tolerates ENOENT: when open() failed before the
523+
* placeholder was written (or the write itself failed) there is nothing to delete, and
524+
* that must not abort the rollback.
525+
*/
526+
private async removeCreatedFile(absolutePath: string): Promise<void> {
527+
try {
528+
await fs.unlink(absolutePath)
529+
} catch (error: unknown) {
530+
if (!isEnoent(error)) {
531+
throw error
532+
}
533+
}
534+
}
535+
536+
/** Same tolerance for a directory this edit created that never made it to disk. */
537+
private async removeCreatedDir(dirPath: string): Promise<void> {
538+
try {
539+
await fs.rmdir(dirPath)
540+
} catch (error: unknown) {
541+
if (!isEnoent(error)) {
542+
throw error
543+
}
544+
}
545+
}
546+
516547
async revertChanges(): Promise<void> {
517-
if (!this.relPath || !this.activeDiffEditor) {
548+
if (!this.relPath) {
518549
return
519550
}
520551

521552
const fileExists = this.editType === "modify"
522-
const updatedDocument = this.activeDiffEditor.document
523553
const absolutePath = path.resolve(this.cwd, this.relPath)
524554

525555
// Stop tracking touches and cancel any pending scroll-to-diff before any
@@ -528,21 +558,38 @@ export class DiffViewProvider {
528558
this.cancelDeferredScroll()
529559

530560
if (!fileExists) {
531-
if (updatedDocument.isDirty) {
532-
await updatedDocument.save()
561+
// open() creates the parent directories and an empty placeholder file BEFORE it
562+
// awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and
563+
// the previous early return here left the placeholder and the new directories on
564+
// disk: the next execute() then saw an empty file and treated the requested new file
565+
// as an existing one, so a denial preserved the debris. The filesystem rollback runs
566+
// either way; only the document work needs an editor.
567+
if (this.activeDiffEditor) {
568+
const updatedDocument = this.activeDiffEditor.document
569+
if (updatedDocument.isDirty) {
570+
await updatedDocument.save()
571+
}
572+
573+
await this.closeAllDiffViews()
574+
// The file was newly created for this edit; close its transiently
575+
// opened tab before deleting it from disk.
576+
await this.closeFileTab(absolutePath)
533577
}
534578

535-
await this.closeAllDiffViews()
536-
// The file was newly created for this edit; close its transiently
537-
// opened tab before deleting it from disk.
538-
await this.closeFileTab(absolutePath)
539-
await fs.unlink(absolutePath)
579+
await this.removeCreatedFile(absolutePath)
540580

541581
// Remove only the directories we created, in reverse order.
542582
for (let i = this.createdDirs.length - 1; i >= 0; i--) {
543-
await fs.rmdir(this.createdDirs[i])
583+
await this.removeCreatedDir(this.createdDirs[i])
544584
}
585+
545586
} else {
587+
// Only reachable after a successful open(), so the editor exists.
588+
const updatedDocument = this.activeDiffEditor?.document
589+
if (!updatedDocument) {
590+
return
591+
}
592+
546593
// Revert document.
547594
const edit = new vscode.WorkspaceEdit()
548595

‎src/integrations/editor/__tests__/DiffViewProvider.spec.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import * as fs from "fs/promises"
12
import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider"
23
import * as vscode from "vscode"
34
import * as path from "path"
@@ -15,6 +16,10 @@ vi.mock("fs/promises", () => ({
1516
readFile: vi.fn().mockResolvedValue("file content"),
1617
writeFile: vi.fn().mockResolvedValue(undefined),
1718
access: vi.fn().mockResolvedValue(undefined),
19+
// revertChanges() rolls a new-file edit back by deleting the placeholder and the
20+
// directories the edit created.
21+
unlink: vi.fn().mockResolvedValue(undefined),
22+
rmdir: vi.fn().mockResolvedValue(undefined),
1823
}))
1924

2025
// Mock utils
@@ -1165,6 +1170,44 @@ describe("DiffViewProvider", () => {
11651170
expect(vscode.window.showTextDocument).toHaveBeenCalled()
11661171
})
11671172

1173+
it("revertChanges() removes the placeholder and created dirs when open() failed before the editor existed", async () => {
1174+
// open() creates the parent dirs and an empty placeholder BEFORE it awaits
1175+
// openDiffEditor(). If that await rejects there is no activeDiffEditor, and the
1176+
// rollback used to bail out - leaving an empty file that the next execute() mistook
1177+
// for an existing file, which a denial then preserved.
1178+
const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`]
1179+
Object.assign(diffViewProvider, {
1180+
relPath: "mock-target-file.ts",
1181+
activeDiffEditor: undefined,
1182+
editType: "create",
1183+
createdDirs,
1184+
})
1185+
1186+
await diffViewProvider.revertChanges()
1187+
1188+
expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`)
1189+
expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1])
1190+
expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0])
1191+
})
1192+
1193+
it("revertChanges() tolerates a placeholder that was never written", async () => {
1194+
// The failed open may have died before fs.writeFile ran; ENOENT during the
1195+
// rollback is success, not a new failure that would abort the cleanup.
1196+
Object.assign(diffViewProvider, {
1197+
relPath: "mock-target-file.ts",
1198+
activeDiffEditor: undefined,
1199+
editType: "create",
1200+
createdDirs: [],
1201+
})
1202+
vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
1203+
1204+
await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined()
1205+
1206+
// The rollback still attempted the delete - the tolerance is about not
1207+
// aborting the rest of the cleanup, not about skipping it.
1208+
expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`)
1209+
})
1210+
11681211
it("revertChanges() closes the file tab when the file was not open and untouched", async () => {
11691212
const closeFileTab = vi.fn().mockResolvedValue(undefined)
11701213
vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true)

0 commit comments

Comments
 (0)