fix: parallel edits sometimes would override each other (#23483)

Author: rekram1-node

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<string, Semaphore.Semaphore>()
+
+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,

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<void>()
+          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")
         },
       })
     })
+
   })
 })