Skip to content

Commit 80f1dc3

Browse files
fix(diff-view): release a placeholder path another writer filled
The abandoned-stream cleanup unlinked placeholderPath on the field alone. The field only names the path open() created empty; it is not proof that this edit still owns the file. Another provider working the same path can write and save approved content into it while this session still carries the field, and the unlink then deleted that approved write outright. The placeholder open() created is zero bytes, so the sweep now removes the file only while it is still zero bytes. A file with content in it, and a file that is already gone, release the ownership and leave the bytes alone, so a later writer's approved write survives the cleanup of an abandoned session.
1 parent e1d6b6c commit 80f1dc3

2 files changed

Lines changed: 38 additions & 4 deletions

File tree

‎src/integrations/editor/DiffViewProvider.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -757,12 +757,17 @@ export class DiffViewProvider {
757757
// Only a placeholder THIS edit created may be removed. relPath survives reset(), and
758758
// saveDirectly() sets it for a file that was written with approval, so unlinking
759759
// absolutePath unconditionally can delete content the user approved.
760-
if (placeholderPath) {
761-
// The helpers carry the ENOENT tolerance: open() creates the placeholder,
762-
// and if it is already gone there is nothing left to remove.
760+
// The field names the path open() created empty; it is not proof that this edit
761+
// still owns the file. Another provider working the same path can write and save
762+
// approved content there while this abandoned session still carries the field, and
763+
// unlinking on the field alone deletes that write. The placeholder open() created is
764+
// zero bytes, so only a file that is still zero bytes is this edit's to remove - the
765+
// ENOENT case and any file with content in it release the ownership and leave the
766+
// bytes alone, which is what a later writer's approved write needs.
767+
if (placeholderPath && (await fs.stat(placeholderPath).catch(() => null))?.size === 0) {
763768
await this.removeCreatedFile(placeholderPath)
764-
this.placeholderPath = undefined
765769
}
770+
this.placeholderPath = undefined
766771

767772
// Remove only the directories this edit created, in reverse order.
768773
for (let i = createdDirs.length - 1; i >= 0; i--) {

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ vi.mock("fs/promises", () => ({
2323
// directories the edit created.
2424
unlink: vi.fn().mockResolvedValue(undefined),
2525
rmdir: vi.fn().mockResolvedValue(undefined),
26+
// The abandoned-stream cleanup stats the placeholder before unlinking it: only the
27+
// file open() created empty is its own to delete. The default double reports exactly
28+
// that zero-byte placeholder; rows that mean a filled file override it once.
29+
stat: vi.fn(async () => ({ size: 0 })),
2630
}))
2731

2832
// Mock utils
@@ -2576,6 +2580,31 @@ describe("DiffViewProvider", () => {
25762580
expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts"))
25772581
})
25782582

2583+
it("releases the placeholder instead of unlinking a path another writer filled", async () => {
2584+
// The field only names the path open() created empty - it does not prove this edit still
2585+
// owns the file. A second provider working the same path can write and save approved
2586+
// content into that file while this abandoned session still carries the field, and the
2587+
// unlink the cleanup used to run unconditionally deletes that approved write. The bytes
2588+
// are somebody else's now, so only the ownership is released here.
2589+
const callOrder: string[] = []
2590+
const document = makeAbandonedDocument(callOrder)
2591+
openAbandonedView(document, [], callOrder)
2592+
// 42 bytes on disk: not the empty placeholder this edit created.
2593+
vi.mocked(fs.stat).mockResolvedValueOnce({ size: 42 } as unknown as fs.Stats)
2594+
vi.mocked(vscode.workspace.applyEdit).mockResolvedValueOnce(true)
2595+
2596+
await diffViewProvider.discardUnapprovedStream()
2597+
2598+
// The buffer work still happened (the restore is mocked to resolve, so it is not
2599+
// recorded in the order list):
2600+
expect(callOrder).toEqual(["save", "closeDiffViews", "closeFileTab"])
2601+
// ...but the file with content in it is not unlinked, and the ownership is released so
2602+
// the caller can reset the session without a later cleanup deleting the write again.
2603+
expect(fs.unlink).not.toHaveBeenCalled()
2604+
expect(diffViewProvider["placeholderPath"]).toBeUndefined()
2605+
expect(diffViewProvider["ownsPlaceholder"]).toBe(false)
2606+
})
2607+
25792608
it("restores a modify buffer in memory and never saves the target file", async () => {
25802609
// revertChanges() restores a modify by applyEdit + document.save(). For a stream the
25812610
// user never approved that save is a write they never asked for, and for a

0 commit comments

Comments
 (0)