From 8bc4f91fd9952612bacf1297ca84424937ce9399 Mon Sep 17 00:00:00 2001 From: Aiden Cline <63023139+rekram1-node@users.noreply.github.com> Date: Mon, 20 Apr 2026 00:14:21 -0500 Subject: [PATCH] fix: parallel edits sometimes would override each other (#23483) --- packages/opencode/src/tool/edit.ts | 128 +++++++++++++---------- packages/opencode/test/tool/edit.test.ts | 42 +++++--- 2 files changed, 97 insertions(+), 73 deletions(-) diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index f535183d4c..2f53cd1949 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -5,7 +5,7 @@ import z from "zod" import * as path from "path" -import { Effect } from "effect" +import { Effect, Semaphore } from "effect" import * as Tool from "./tool" import { LSP } from "../lsp" import { createTwoFilesPatch, diffLines } from "diff" @@ -32,6 +32,18 @@ function convertToLineEnding(text: string, ending: "\n" | "\r\n"): string { return text.replaceAll("\n", "\r\n") } +const locks = new Map() + +function lock(filePath: string) { + const resolvedFilePath = AppFileSystem.resolve(filePath) + const hit = locks.get(resolvedFilePath) + if (hit) return hit + + const next = Semaphore.makeUnsafe(1) + locks.set(resolvedFilePath, next) + return next +} + const Parameters = z.object({ filePath: z.string().describe("The absolute path to the file to modify"), oldString: z.string().describe("The text to replace"), @@ -68,11 +80,50 @@ export const EditTool = Tool.define( let diff = "" let contentOld = "" let contentNew = "" - yield* Effect.gen(function* () { - if (params.oldString === "") { - const existed = yield* afs.existsSafe(filePath) - contentNew = params.newString - diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew)) + yield* lock(filePath).withPermits(1)( + Effect.gen(function* () { + if (params.oldString === "") { + const existed = yield* afs.existsSafe(filePath) + contentNew = params.newString + diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew)) + yield* ctx.ask({ + permission: "edit", + patterns: [path.relative(Instance.worktree, filePath)], + always: ["*"], + metadata: { + filepath: filePath, + diff, + }, + }) + yield* afs.writeWithDirs(filePath, params.newString) + yield* format.file(filePath) + yield* bus.publish(File.Event.Edited, { file: filePath }) + yield* bus.publish(FileWatcher.Event.Updated, { + file: filePath, + event: existed ? "change" : "add", + }) + return + } + + const info = yield* afs.stat(filePath).pipe(Effect.catch(() => Effect.succeed(undefined))) + if (!info) throw new Error(`File ${filePath} not found`) + if (info.type === "Directory") throw new Error(`Path is a directory, not a file: ${filePath}`) + contentOld = yield* afs.readFileString(filePath) + + const ending = detectLineEnding(contentOld) + const old = convertToLineEnding(normalizeLineEndings(params.oldString), ending) + const next = convertToLineEnding(normalizeLineEndings(params.newString), ending) + + contentNew = replace(contentOld, old, next, params.replaceAll) + + diff = trimDiff( + createTwoFilesPatch( + filePath, + filePath, + normalizeLineEndings(contentOld), + normalizeLineEndings(contentNew), + ), + ) yield* ctx.ask({ permission: "edit", patterns: [path.relative(Instance.worktree, filePath)], @@ -82,62 +133,25 @@ export const EditTool = Tool.define( diff, }, }) - yield* afs.writeWithDirs(filePath, params.newString) + + yield* afs.writeWithDirs(filePath, contentNew) yield* format.file(filePath) yield* bus.publish(File.Event.Edited, { file: filePath }) yield* bus.publish(FileWatcher.Event.Updated, { file: filePath, - event: existed ? "change" : "add", + event: "change", }) - return - } - - const info = yield* afs.stat(filePath).pipe(Effect.catch(() => Effect.succeed(undefined))) - if (!info) throw new Error(`File ${filePath} not found`) - if (info.type === "Directory") throw new Error(`Path is a directory, not a file: ${filePath}`) - contentOld = yield* afs.readFileString(filePath) - - const ending = detectLineEnding(contentOld) - const old = convertToLineEnding(normalizeLineEndings(params.oldString), ending) - const next = convertToLineEnding(normalizeLineEndings(params.newString), ending) - - contentNew = replace(contentOld, old, next, params.replaceAll) - - diff = trimDiff( - createTwoFilesPatch( - filePath, - filePath, - normalizeLineEndings(contentOld), - normalizeLineEndings(contentNew), - ), - ) - yield* ctx.ask({ - permission: "edit", - patterns: [path.relative(Instance.worktree, filePath)], - always: ["*"], - metadata: { - filepath: filePath, - diff, - }, - }) - - yield* afs.writeWithDirs(filePath, contentNew) - yield* format.file(filePath) - yield* bus.publish(File.Event.Edited, { file: filePath }) - yield* bus.publish(FileWatcher.Event.Updated, { - file: filePath, - event: "change", - }) - contentNew = yield* afs.readFileString(filePath) - diff = trimDiff( - createTwoFilesPatch( - filePath, - filePath, - normalizeLineEndings(contentOld), - normalizeLineEndings(contentNew), - ), - ) - }).pipe(Effect.orDie) + contentNew = yield* afs.readFileString(filePath) + diff = trimDiff( + createTwoFilesPatch( + filePath, + filePath, + normalizeLineEndings(contentOld), + normalizeLineEndings(contentNew), + ), + ) + }).pipe(Effect.orDie), + ) const filediff: Snapshot.FileDiff = { file: filePath, diff --git a/packages/opencode/test/tool/edit.test.ts b/packages/opencode/test/tool/edit.test.ts index 4759b8be36..8756d65d24 100644 --- a/packages/opencode/test/tool/edit.test.ts +++ b/packages/opencode/test/tool/edit.test.ts @@ -29,11 +29,6 @@ afterEach(async () => { await Instance.disposeAll() }) -async function touch(file: string, time: number) { - const date = new Date(time) - await fs.utimes(file, date, date) -} - const runtime = ManagedRuntime.make( Layer.mergeAll( LSP.defaultLayer, @@ -639,44 +634,59 @@ describe("tool.edit", () => { }) describe("concurrent editing", () => { - test("serializes concurrent edits to same file", async () => { + test("preserves concurrent edits to different sections of the same file", async () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "0", "utf-8") + await fs.writeFile(filepath, "top = 0\nmiddle = keep\nbottom = 0\n", "utf-8") await Instance.provide({ directory: tmp.path, fn: async () => { const edit = await resolve() + let asks = 0 + const firstAsk = Promise.withResolvers() + const delayedCtx = { + ...ctx, + ask: () => + Effect.gen(function* () { + asks++ + if (asks !== 1) return + firstAsk.resolve() + yield* Effect.promise(() => Bun.sleep(50)) + }), + } - // Two concurrent edits const promise1 = Effect.runPromise( edit.execute( { filePath: filepath, - oldString: "0", - newString: "1", + oldString: "top = 0", + newString: "top = 1", }, - ctx, + delayedCtx, ), ) + await firstAsk.promise + const promise2 = Effect.runPromise( edit.execute( { filePath: filepath, - oldString: "0", - newString: "2", + oldString: "bottom = 0", + newString: "bottom = 2", }, - ctx, + delayedCtx, ), ) - // Both should complete without error (though one might fail due to content mismatch) const results = await Promise.allSettled([promise1, promise2]) - expect(results.some((r) => r.status === "fulfilled")).toBe(true) + expect(results[0]?.status).toBe("fulfilled") + expect(results[1]?.status).toBe("fulfilled") + expect(await fs.readFile(filepath, "utf-8")).toBe("top = 1\nmiddle = keep\nbottom = 2\n") }, }) }) + }) })