diff --git a/packages/core/src/file-mutation.ts b/packages/core/src/file-mutation.ts index 52ac682cbbc..fd443b1ea9f 100644 --- a/packages/core/src/file-mutation.ts +++ b/packages/core/src/file-mutation.ts @@ -29,6 +29,10 @@ export interface WriteResult { } export interface Interface { + /** Serialize a complete read/prepare/write mutation transaction by resolved path. */ + readonly withLock: ( + targets: ReadonlyArray, + ) => (effect: Effect.Effect) => Effect.Effect readonly write: (input: WriteInput) => Effect.Effect /** Write text while retaining an existing UTF-8 BOM and emitting at most one BOM. */ readonly writeTextPreservingBom: (input: TextWriteInput) => Effect.Effect @@ -36,6 +40,9 @@ export interface Interface { export class Service extends Context.Service()("@opencode/FileMutation") {} +/** Share transaction locks across Location graphs that address the same file. */ +const transactionLocks = KeyedMutex.makeUnsafe() + /** * Serialize file changes by absolute target. Conditional writes compare and * write under the same process-local lock so cooperating OpenCode mutations do @@ -46,6 +53,10 @@ const layer = Layer.effect( Effect.gen(function* () { const fs = yield* FSUtil.Service const locks = KeyedMutex.makeUnsafe() + const withLock: Interface["withLock"] = (targets) => (effect) => + [...new Set(targets.map(FSUtil.resolve))] + .sort() + .reduceRight((result, target) => transactionLocks.withLock(target)(result), effect) const withTargetLock = (target: Target) => (effect: Effect.Effect) => @@ -84,7 +95,7 @@ const layer = Layer.effect( ), ) - return Service.of({ write, writeTextPreservingBom }) + return Service.of({ withLock, write, writeTextPreservingBom }) }), ) diff --git a/packages/core/src/tool/plugin/edit.ts b/packages/core/src/tool/plugin/edit.ts index 60cc3c95016..6f50ab772a8 100644 --- a/packages/core/src/tool/plugin/edit.ts +++ b/packages/core/src/tool/plugin/edit.ts @@ -11,9 +11,11 @@ import { ToolFailure } from "@opencode-ai/ai" import { FileDiff } from "@opencode-ai/schema/file-diff" import { Bom } from "@opencode-ai/util/bom" import { Effect, Schema } from "effect" +import path from "path" import { FileMutation } from "../../file-mutation" import { Formatter } from "../../formatter" import { FSUtil } from "@opencode-ai/util/fs-util" +import { Location } from "../../location" import { LocationMutation } from "../../location-mutation" import { Permission } from "../../permission" import { fileDiff } from "./file-diff" @@ -112,6 +114,7 @@ export const Plugin = { const files = yield* FileMutation.Service const formatter = yield* Formatter.Service const fs = yield* FSUtil.Service + const location = yield* Location.Service const permission = yield* Permission.Service yield* ctx.tool @@ -217,6 +220,7 @@ export const Plugin = { replacements, } satisfies Output }).pipe( + files.withLock([path.resolve(location.directory, input.path)]), Effect.map((output) => ({ output, content: `Edited ${output.files[0]?.file} (${output.replacements} replacement${output.replacements === 1 ? "" : "s"})`, diff --git a/packages/core/src/tool/plugin/patch.ts b/packages/core/src/tool/plugin/patch.ts index 7f9bd01508d..a69c92918fb 100644 --- a/packages/core/src/tool/plugin/patch.ts +++ b/packages/core/src/tool/plugin/patch.ts @@ -4,12 +4,13 @@ import type { Context as PluginContext } from "@opencode-ai/plugin/effect/plugin import { ToolFailure } from "@opencode-ai/ai" import { FileDiff } from "@opencode-ai/schema/file-diff" import { createTwoFilesPatch, diffLines } from "diff" -import { Effect, Schema } from "effect" +import { Effect, Result, Schema } from "effect" import { PlatformError } from "effect/PlatformError" import path from "path" import { Bom } from "@opencode-ai/util/bom" import { FSUtil } from "@opencode-ai/util/fs-util" import { Formatter } from "../../formatter" +import { FileMutation } from "../../file-mutation" import { Location } from "../../location" import { Patch } from "@opencode-ai/util/patch" import { Permission } from "../../permission" @@ -70,6 +71,7 @@ export const Plugin = { id: "opencode.tool.patch", effect: Effect.fn("PatchTool.Plugin")(function* (ctx: PluginContext) { const fs = yield* FSUtil.Service + const mutation = yield* FileMutation.Service const formatter = yield* Formatter.Service const location = yield* Location.Service const permission = yield* Permission.Service @@ -84,6 +86,15 @@ export const Plugin = { output: Output, execute: (input, context) => { const applied: Array = [] + const parsed = Patch.parse(input.patchText) + const lockTargets = Result.isSuccess(parsed) + ? parsed.success.flatMap((hunk) => [ + path.resolve(location.directory, hunk.path), + ...(hunk.type === "update" && hunk.movePath + ? [path.resolve(location.directory, hunk.movePath)] + : []), + ]) + : [] const fail = (operation: string, error: unknown) => { const completed = applied.map((item) => item.resource).join(", ") return new ToolFailure({ @@ -315,6 +326,7 @@ export const Plugin = { }) return { applied, files } }).pipe( + mutation.withLock(lockTargets), Effect.map((output) => ({ output, content: toModelOutput(output), diff --git a/packages/core/test/file-mutation.test.ts b/packages/core/test/file-mutation.test.ts index 6b64d66d9de..48f2c3a371c 100644 --- a/packages/core/test/file-mutation.test.ts +++ b/packages/core/test/file-mutation.test.ts @@ -152,6 +152,57 @@ describe("FileMutation", () => { ), ) + it.live("shares transaction locks across Location service instances", () => + withTmp((directory) => + Effect.gen(function* () { + const firstStarted = yield* Deferred.make() + const releaseFirst = yield* Deferred.make() + const secondStarted = yield* Deferred.make() + const target = path.join(directory, "shared.txt") + const first = yield* Effect.gen(function* () { + const files = yield* FileMutation.Service + yield* files.withLock([target])( + Deferred.succeed(firstStarted, undefined).pipe(Effect.andThen(Deferred.await(releaseFirst))), + ) + }).pipe(provide(directory), Effect.forkChild) + yield* Deferred.await(firstStarted) + const second = yield* Effect.gen(function* () { + const files = yield* FileMutation.Service + yield* files.withLock([target])(Deferred.succeed(secondStarted, undefined)) + }).pipe(provide(directory), Effect.forkChild) + yield* Effect.yieldNow + expect(yield* Deferred.isDone(secondStarted)).toBe(false) + + yield* Deferred.succeed(releaseFirst, undefined) + yield* Deferred.await(secondStarted) + yield* Fiber.join(first) + yield* Fiber.join(second) + }), + ), + ) + + it.live("allows transaction locks for distinct resolved paths to proceed independently", () => + withTmp((directory) => + Effect.gen(function* () { + const firstStarted = yield* Deferred.make() + const releaseFirst = yield* Deferred.make() + const secondFinished = yield* Deferred.make() + const files = yield* FileMutation.Service + const first = yield* files + .withLock([path.join(directory, "first.txt")])( + Deferred.succeed(firstStarted, undefined).pipe(Effect.andThen(Deferred.await(releaseFirst))), + ) + .pipe(Effect.forkChild) + yield* Deferred.await(firstStarted) + yield* files.withLock([path.join(directory, "second.txt")])(Deferred.succeed(secondFinished, undefined)) + expect(yield* Deferred.isDone(secondFinished)).toBe(true) + + yield* Deferred.succeed(releaseFirst, undefined) + yield* Fiber.join(first) + }).pipe(provide(directory)), + ), + ) + it.live("allows distinct absolute targets to proceed independently", () => withTmp((directory) => Effect.gen(function* () { diff --git a/packages/core/test/tool-edit.test.ts b/packages/core/test/tool-edit.test.ts index 35269e37bf8..513ca375a85 100644 --- a/packages/core/test/tool-edit.test.ts +++ b/packages/core/test/tool-edit.test.ts @@ -23,7 +23,7 @@ import { toolIdentity, executeTool, registerToolPlugin, toolDefinitions } from " const editToolNode = makeLocationNode({ name: "test/edit-tool-plugin", layer: Layer.effectDiscard(registerToolPlugin(EditTool.Plugin)), - deps: [Tool.node, LocationMutation.node, FileMutation.node, Formatter.node, FSUtil.node, Permission.node], + deps: [Tool.node, LocationMutation.node, FileMutation.node, Formatter.node, FSUtil.node, Location.node, Permission.node], }) const sessionID = Session.ID.make("ses_edit_tool_test") @@ -645,6 +645,43 @@ describe("EditTool", () => { ), ) + it.live("serializes concurrent edit transactions", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => { + reset() + const target = path.join(tmp.path, "concurrent.txt") + afterRead = () => (reads === 1 ? Effect.sleep("50 millis") : Effect.void) + return Effect.promise(() => fs.writeFile(target, "one\ntwo\n")).pipe( + Effect.andThen( + withTool(tmp.path, (registry) => + Effect.all( + [ + executeTool( + registry, + call({ path: "concurrent.txt", oldString: "one", newString: "ONE" }, "call-edit-one"), + ), + executeTool( + registry, + call({ path: "concurrent.txt", oldString: "two", newString: "TWO" }, "call-edit-two"), + ), + ], + { concurrency: "unbounded" }, + ), + ), + ), + Effect.andThen((results) => + Effect.gen(function* () { + expect(results.map((result) => result.status)).toEqual(["completed", "completed"]) + expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n") + }), + ), + ) + }, + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + it.live("applies the edit when content changes after matching", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index 4a1a2e095f8..50d7f3a4277 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -7,6 +7,7 @@ import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder" import { LayerNode } from "@opencode-ai/util/effect/layer-node" import { FSUtil } from "@opencode-ai/util/fs-util" import { Formatter } from "@opencode-ai/core/formatter" +import { FileMutation } from "@opencode-ai/core/file-mutation" import { Location } from "@opencode-ai/core/location" import { Permission } from "@opencode-ai/core/permission" import { AbsolutePath } from "@opencode-ai/core/schema" @@ -22,7 +23,7 @@ import { toolIdentity, executeTool, registerToolPlugin, toolDefinitions } from " const patchToolNode = makeLocationNode({ name: "test/patch-tool-plugin", layer: Layer.effectDiscard(registerToolPlugin(PatchTool.Plugin)), - deps: [Tool.node, Formatter.node, FSUtil.node, Location.node, Permission.node], + deps: [Tool.node, FileMutation.node, Formatter.node, FSUtil.node, Location.node, Permission.node], }) const sessionID = Session.ID.make("ses_patch_tool_test") @@ -139,7 +140,7 @@ const withTool = ( return yield* body(yield* Tool.Service) }).pipe( Effect.provide( - AppNodeBuilder.build(LayerNode.group([Tool.node, patchToolNode]), [ + AppNodeBuilder.build(LayerNode.group([Tool.node, FileMutation.node, patchToolNode]), [ [FSUtil.node, filesystem], [Location.node, activeLocation], [Formatter.node, formatter], @@ -262,6 +263,45 @@ describe("PatchTool", () => { ), ) + it.live("serializes concurrent patch transactions", () => + withTempTool((directory, registry) => { + const target = path.join(directory, "concurrent.txt") + afterEditApproval = () => + assertions.filter((input) => input.action === "edit").length === 1 + ? Effect.sleep("50 millis") + : Effect.void + return Effect.promise(() => fs.writeFile(target, "one\ntwo\n")).pipe( + Effect.andThen( + Effect.all( + [ + executeTool( + registry, + call( + "*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-one\n+ONE\n*** End Patch", + "call-patch-one", + ), + ), + executeTool( + registry, + call( + "*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-two\n+TWO\n*** End Patch", + "call-patch-two", + ), + ), + ], + { concurrency: "unbounded" }, + ), + ), + Effect.andThen((results) => + Effect.gen(function* () { + expect(results.map((result) => result.status)).toEqual(["completed", "completed"]) + expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n") + }), + ), + ) + }), + ) + it.live("returns file diffs for final formatted content", () => withTempTool((directory, registry) => { const target = path.join(directory, "formatted.txt")