diff --git a/packages/core/src/git.ts b/packages/core/src/git.ts index db4fa9d12..8c60257bc 100644 --- a/packages/core/src/git.ts +++ b/packages/core/src/git.ts @@ -459,15 +459,39 @@ const layer = Layer.effect( ) const candidates = Array.from(new Set([...tracked, ...untracked])) if (!candidates.length) return { skipped: [] } - const ignored = input.ignores - ? new Set( - (yield* repositoryOperation("refresh", input.ignores, ["check-ignore", "--no-index", "--stdin", "-z"], { - stdin: candidates.join("\0") + "\0", - }).pipe(Effect.catch(() => Effect.succeed({ text: "", stderr: "" })))).text - .split("\0") - .filter(Boolean), + const ignored = new Set() + if (input.ignores) { + // Git check-ignore exits 1 for "nothing ignored"; any other failure + // must abort the capture, rather than silently treating ignored files + // as safe to stage into a snapshot. + const check = yield* proc + .run( + ChildProcess.make( + "git", + repositoryArgs(input.ignores, ["check-ignore", "--no-index", "--stdin", "-z"]), + { cwd: input.ignores.worktree, extendEnv: true }, + ), + { stdin: candidates.join("\0") + "\0" }, + ) + .pipe( + Effect.mapError( + (cause) => + new OperationError({ + operation: "refresh", + directory: input.ignores!.worktree, + message: cause.message, + cause, + }), + ), ) - : new Set() + if (check.exitCode !== 0 && check.exitCode !== 1) + return yield* new OperationError({ + operation: "refresh", + directory: input.ignores.worktree, + message: check.stderr.toString("utf8").trim() || "Failed to check ignored paths", + }) + for (const file of check.stdout.toString("utf8").split("\0").filter(Boolean)) ignored.add(file) + } const allowed = candidates.filter((item) => !ignored.has(item)) const maximum = input.maximumUntrackedFileBytes const skipped = maximum @@ -573,7 +597,16 @@ const layer = Layer.effect( }) const writeTree = Effect.fn("Git.tree.write")(function* (repository: Repository) { - return TreeID.make((yield* repositoryOperation("write_tree", repository, ["write-tree"])).text.trim()) + const result = yield* repositoryOperation("write_tree", repository, ["write-tree"]) + const oid = result.text.trim() + if (!/^(?:[a-f0-9]{40}|[a-f0-9]{64})$/i.test(oid)) { + return yield* new OperationError({ + operation: "write_tree", + directory: repository.worktree, + message: "Git write-tree did not return a valid tree object ID", + }) + } + return TreeID.make(oid) }) type CaptureInput = { diff --git a/packages/core/src/snapshot-index-lock.ts b/packages/core/src/snapshot-index-lock.ts new file mode 100644 index 000000000..ba3ed896c --- /dev/null +++ b/packages/core/src/snapshot-index-lock.ts @@ -0,0 +1,37 @@ +import path from "path" +import { Effect } from "effect" +import { Flock } from "./util/flock" + +/** + * Shared writer lease for the V1 and core/V2 snapshot implementations. + * + * Both services intentionally use data/snapshot// as their + * shadow gitdir. Their separate in-process semaphores cannot protect that + * Git index from each other or from another OpenFork server process. + * + * Use a different lock directory from Git's index.lock: Flock provides atomic + * mkdir acquisition, heartbeat, owner token validation and stale lease recovery. + * Keep this lock across the entire index transaction, not merely git add. + */ +export namespace SnapshotIndexLock { + export const key = (gitDirectory: string) => `snapshot-index:${path.normalize(gitDirectory)}` + export const root = (dataDirectory: string) => path.join(dataDirectory, "snapshot-locks") + + export const withLock = ( + gitDirectory: string, + dataDirectory: string, + body: Effect.Effect, + ) => + Effect.scoped( + Effect.gen(function* () { + yield* Flock.effect(key(gitDirectory), { + dir: root(dataDirectory), + staleMs: 180_000, + timeoutMs: 45_000, + baseDelayMs: 60, + maxDelayMs: 750, + }) + return yield* body + }), + ) +} diff --git a/packages/core/src/snapshot.ts b/packages/core/src/snapshot.ts index aff038622..155b8e45b 100644 --- a/packages/core/src/snapshot.ts +++ b/packages/core/src/snapshot.ts @@ -13,6 +13,7 @@ import { Location } from "./location" import { AppProcess } from "./process" import { AbsolutePath, RelativePath } from "./schema" import { Hash } from "./util/hash" +import { SnapshotIndexLock } from "./snapshot-index-lock" import { GitRuntime } from "./git-runtime" export const ID = Schema.String.pipe(Schema.brand("Snapshot.ID")) @@ -153,6 +154,13 @@ const layer = Layer.effect( : location.project.directory const gitDirectory = AbsolutePath.make(path.join(global.data, "snapshot", location.project.id, Hash.fast(worktree))) + // V1 Snapshot and this V2/core service use the *same* shadow gitdir. + // Git.tree has an in-process lock, but that cannot coordinate V1 or + // independent OpenFork server processes. Reuse the exact V1 filesystem + // lease key and hold it across initialization, refresh and write-tree. + const withShadowIndex = (body: Effect.Effect) => + SnapshotIndexLock.withLock(gitDirectory, global.data, body) + const scope = Effect.fnUntraced(function* () { const relative = path.relative(worktree, location.directory) if (relative.startsWith("..") || path.isAbsolute(relative)) @@ -218,19 +226,23 @@ const layer = Layer.effect( const capture = Effect.fn("Snapshot.capture")(function* () { if (!(yield* enabled())) return undefined - return yield* Effect.gen(function* () { - const repo = yield* repository() - yield* healIndex(repo) - return ID.make( - yield* git.tree.capture({ - repository: repo, - scopes: [yield* scope()], - ignores: source, - maximumUntrackedFileBytes: 2 * 1024 * 1024, - }), - ) - }).pipe( - Effect.catch((cause) => Effect.logWarning("failed to capture snapshot", { cause }).pipe(Effect.as(undefined))), + return yield* withShadowIndex( + Effect.gen(function* () { + const repo = yield* repository() + yield* healIndex(repo) + return ID.make( + yield* git.tree.capture({ + repository: repo, + scopes: [yield* scope()], + ignores: source, + maximumUntrackedFileBytes: 2 * 1024 * 1024, + }), + ) + }), + ).pipe( + Effect.catchCause((cause) => + Effect.logWarning("failed to capture snapshot; no tree published", { cause }).pipe(Effect.as(undefined)), + ), ) }) @@ -279,40 +291,52 @@ const layer = Layer.effect( const preview = Effect.fn("Snapshot.preview")(function* (input: PreviewInput) { if (!(yield* enabled())) return yield* new Error({ operation: "preview", message: "Snapshots are disabled" }) - const repo = yield* repository().pipe(Effect.mapError((cause) => failure("preview", cause))) - yield* healIndex(repo).pipe(Effect.catch(() => Effect.void)) - const files = yield* plan("preview", input) - const current = yield* git.tree - .capture({ - repository: repo, - scopes: Array.from(files.keys()), - ignores: source, - maximumUntrackedFileBytes: 2 * 1024 * 1024, - }) - .pipe(Effect.mapError((cause) => failure("preview", cause))) - return yield* git.tree - .preview({ - repository: repo, - current, - files, - context: input.context, - }) - .pipe(Effect.mapError((cause) => failure("preview", cause))) + return yield* withShadowIndex( + Effect.gen(function* () { + const repo = yield* repository().pipe(Effect.mapError((cause) => failure("preview", cause))) + yield* healIndex(repo) + const files = yield* plan("preview", input) + const current = yield* git.tree + .capture({ + repository: repo, + scopes: Array.from(files.keys()), + ignores: source, + maximumUntrackedFileBytes: 2 * 1024 * 1024, + }) + .pipe(Effect.mapError((cause) => failure("preview", cause))) + return yield* git.tree + .preview({ + repository: repo, + current, + files, + context: input.context, + }) + .pipe(Effect.mapError((cause) => failure("preview", cause))) + }), + ) }) const restore = Effect.fn("Snapshot.restore")(function* (input: RestoreInput) { if (!(yield* enabled())) return yield* new Error({ operation: "restore", message: "Snapshots are disabled" }) - const repo = yield* repository().pipe(Effect.mapError((cause) => failure("restore", cause))) - yield* git.tree - .restore({ repository: repo, files: yield* plan("restore", input) }) - .pipe(Effect.mapError((cause) => failure("restore", cause))) + return yield* withShadowIndex( + Effect.gen(function* () { + const repo = yield* repository().pipe(Effect.mapError((cause) => failure("restore", cause))) + yield* git.tree + .restore({ repository: repo, files: yield* plan("restore", input) }) + .pipe(Effect.mapError((cause) => failure("restore", cause))) + }), + ) }) const checkout = Effect.fn("Snapshot.checkout")(function* (snapshot: ID) { - const repo = yield* repository().pipe(Effect.mapError((cause) => failure("restore", cause))) - yield* git.tree - .checkout({ repository: repo, tree: Git.TreeID.make(snapshot) }) - .pipe(Effect.mapError((cause) => failure("restore", cause))) + return yield* withShadowIndex( + Effect.gen(function* () { + const repo = yield* repository().pipe(Effect.mapError((cause) => failure("restore", cause))) + yield* git.tree + .checkout({ repository: repo, tree: Git.TreeID.make(snapshot) }) + .pipe(Effect.mapError((cause) => failure("restore", cause))) + }), + ) }) const epoch = Effect.fnUntraced(function* () { diff --git a/packages/opencode/src/snapshot/index.ts b/packages/opencode/src/snapshot/index.ts index 2dbc26c12..1401f0440 100644 --- a/packages/opencode/src/snapshot/index.ts +++ b/packages/opencode/src/snapshot/index.ts @@ -19,6 +19,7 @@ import { GitRuntime } from "@opencode-ai/core/git-runtime" import { InstanceState } from "@/effect/instance-state" import { FSUtil } from "@opencode-ai/core/fs-util" import { Hash } from "@opencode-ai/core/util/hash" +import { SnapshotIndexLock } from "@opencode-ai/core/snapshot-index-lock" import { Config } from "@/config/config" import { Global } from "@opencode-ai/core/global" import { Info } from "@opencode-ai/schema/file-diff" @@ -56,6 +57,7 @@ const core = ["-c", "core.longpaths=true", "-c", "core.symlinks=true"] const cfg = [...GitRuntime.args([]), ...core] const quote = [...cfg, "-c", "core.quotepath=false"] const CAPTURE_TIMEOUT = Duration.minutes(5) +const TREE_OID = /^(?:[a-f0-9]{40}|[a-f0-9]{64})$/i interface GitResult { readonly code: ChildProcessSpawner.ExitCode readonly text: string @@ -121,8 +123,9 @@ export class Service extends Context.Service()("@opencode/Sn // Process-wide shadow-repo lock. Every Snapshot layer instance in this process // must serialize mutations to one shadow gitdir: the earlier per-layer map let // two instances write a single index concurrently, which is how a non-atomic -// seed copy could leave a zeroed index. Cross-process writers (separate server -// runs) are additionally protected by Git's own `index.lock`. +// seed copy could leave a zeroed index. The filesystem Flock below serializes +// the entire shadow-index transaction across independent server processes. +// Git's index.lock remains the last defense against legacy/noncooperating writers. const shadowLocks = new Map() const shadowLock = (key: string) => { const hit = shadowLocks.get(key) @@ -302,7 +305,7 @@ const layer: Layer.Layer< stdin: encodeNulTerminatedPaths(checkIgnorePaths), }, ) - if (check.code !== 0 && check.code !== 1) return new Set() + if (check.code !== 0 && check.code !== 1) return yield* failGit("check-ignore", check) return new Set( check.text .split("\0") @@ -311,9 +314,12 @@ const layer: Layer.Layer< ) }) + const failGit = (command: string, result: GitResult) => + Effect.die(new Error(`snapshot ${command} failed (exit ${result.code}): ${result.stderr.trim() || "no stderr"}`)) + const drop = Effect.fnUntraced(function* (files: string[]) { if (!files.length) return - yield* git( + const result = yield* git( [ ...cfg, ...args(["rm", "--cached", "-f", "--ignore-unmatch", "--pathspec-from-file=-", "--pathspec-file-nul"]), @@ -323,28 +329,44 @@ const layer: Layer.Layer< stdin: encodeTopLevelLiteralPathspecs(files), }, ) + if (result.code !== 0) return yield* failGit("rm --cached", result) }) const stage = Effect.fnUntraced(function* (files: string[]) { if (!files.length) return - const result = yield* git( - [...cfg, ...args(["add", "--all", "--sparse", "--pathspec-from-file=-", "--pathspec-file-nul"])], - { - cwd: state.worktree, - stdin: encodeTopLevelLiteralPathspecs(files), - }, - ) - if (result.code === 0) return - yield* Effect.logWarning("failed to add snapshot files", { - exitCode: result.code, - stderr: result.stderr, - }) + // A legacy server that predates our cross-process lease may still + // hold Git's index.lock. Retry only that transient contention. + // Never write-tree after an unsuccessful stage. + for (let attempt = 0; attempt < 4; attempt++) { + const result = yield* git( + [...cfg, ...args(["add", "--all", "--sparse", "--pathspec-from-file=-", "--pathspec-file-nul"])], + { + cwd: state.worktree, + stdin: encodeTopLevelLiteralPathspecs(files), + }, + ) + if (result.code === 0) return + if (attempt < 3 && result.stderr.includes("index.lock") && /file exists/i.test(result.stderr)) { + yield* Effect.sleep(Duration.millis(100 * (attempt + 1))) + continue + } + yield* Effect.logWarning("failed to add snapshot files", { + exitCode: result.code, + stderr: result.stderr, + attempt: attempt + 1, + }) + return yield* failGit("add", result) + } }) const exists = (file: string) => fs.exists(file).pipe(Effect.orDie) const read = (file: string) => fs.readFileString(file).pipe(Effect.catch(() => Effect.succeed(""))) const remove = (file: string) => fs.remove(file).pipe(Effect.catch(() => Effect.void)) - const locked = (fx: Effect.Effect) => shadowLock(state.gitdir).withPermits(1)(fx) + const locked = (fx: Effect.Effect) => + shadowLock(state.gitdir).withPermits(1)( + // Exclusive across V1, core/V2 and any number of server processes. + SnapshotIndexLock.withLock(state.gitdir, Global.Path.data, fx), + ) const enabled = Effect.fnUntraced(function* () { if (state.vcs !== "git") return false @@ -465,6 +487,7 @@ const layer: Layer.Layer< exitCode: result.code, stderr: result.stderr, }) + return yield* failGit("rebuild index", result) } }) @@ -510,7 +533,7 @@ const layer: Layer.Layer< otherCode: other.code, otherStderr: other.stderr, }) - return + return yield* failGit("list files", diff.code !== 0 ? diff : other) } const tracked = diff.text.split("\0").filter(Boolean) @@ -600,6 +623,16 @@ const layer: Layer.Layer< yield* add() const result = yield* git(args(["write-tree"]), { cwd: state.directory }) const hash = result.text.trim() + // An empty or malformed hash is never a snapshot. In particular, + // a failed git add must not silently reuse the previous index tree. + if (result.code !== 0 || !TREE_OID.test(hash)) { + yield* Effect.logWarning("snapshot write-tree failed", { + exitCode: result.code, + stderr: result.stderr, + hash, + }) + return yield* failGit("write-tree", result) + } yield* Effect.logInfo("tracking", { hash, cwd: state.directory, git: state.gitdir }) return hash }) @@ -648,6 +681,19 @@ const layer: Layer.Layer< }), ), ).pipe( + Effect.catchCause((cause) => + Effect.gen(function* () { + yield* Effect.logWarning("snapshot capture failed; no tree published", { + directory: state.directory, + git: state.gitdir, + cause: Cause.pretty(cause), + }) + yield* SynchronizedRef.update(materialization, (snapshot) => + snapshot.cached?.token === token ? { ...snapshot, cached: undefined } : snapshot, + ) + return undefined as string | undefined + }), + ), Effect.timeoutOrElse({ duration: CAPTURE_TIMEOUT, orElse: () => @@ -748,23 +794,11 @@ const layer: Layer.Layer< Effect.gen(function* () { yield* Effect.logInfo("restore", { commit: snapshot }) const result = yield* git([...core, ...args(["read-tree", snapshot])], { cwd: state.worktree }) - if (result.code === 0) { - const checkout = yield* git([...core, ...args(["checkout-index", "-a", "-f"])], { - cwd: state.worktree, - }) - if (checkout.code === 0) return - yield* Effect.logError("failed to restore snapshot", { - snapshot, - exitCode: checkout.code, - stderr: checkout.stderr, - }) - return - } - yield* Effect.logError("failed to restore snapshot", { - snapshot, - exitCode: result.code, - stderr: result.stderr, + if (result.code !== 0) return yield* failGit("read-tree (restore)", result) + const checkout = yield* git([...core, ...args(["checkout-index", "-a", "-f"])], { + cwd: state.worktree, }) + if (checkout.code !== 0) return yield* failGit("checkout-index (restore)", checkout) }), ), "snapshot.restore") }) @@ -775,13 +809,22 @@ const layer: Layer.Layer< const ops: { hash: string; file: string; rel: string }[] = [] const seen = new Set() for (const item of patches) { + // Reject invalid source trees *before* touching any worktree file. + // A failed lookup must never be interpreted as an absent file. + if (item.files.length && !TREE_OID.test(item.hash)) { + return yield* Effect.die(new Error(`invalid snapshot revert tree: ${item.hash}`)) + } for (const file of item.files) { if (seen.has(file)) continue + const relative = path.relative(state.worktree, file) + if (!relative || relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { + return yield* Effect.die(new Error(`snapshot revert path escapes worktree: ${file}`)) + } seen.add(file) ops.push({ hash: item.hash, file, - rel: path.relative(state.worktree, file).replaceAll("\\", "/"), + rel: relative.replaceAll("\\", "/"), }) } } @@ -795,13 +838,8 @@ const layer: Layer.Layer< const tree = yield* git([...core, ...args(["ls-tree", op.hash, "--", op.rel])], { cwd: state.worktree, }) - if (tree.code === 0 && tree.text.trim()) { - yield* Effect.logInfo("file existed in snapshot but checkout failed, keeping", { - file: op.file, - hash: op.hash, - }) - return - } + if (tree.code !== 0) return yield* failGit("ls-tree (revert)", tree) + if (tree.text.trim()) return yield* failGit("checkout (revert)", result) yield* Effect.logInfo("file did not exist in snapshot, deleting", { file: op.file, hash: op.hash }) yield* remove(op.file) }) diff --git a/packages/opencode/test/snapshot/snapshot.test.ts b/packages/opencode/test/snapshot/snapshot.test.ts index f907b494f..10b89d6a3 100644 --- a/packages/opencode/test/snapshot/snapshot.test.ts +++ b/packages/opencode/test/snapshot/snapshot.test.ts @@ -7,7 +7,9 @@ import { Global } from "@opencode-ai/core/global" import { Hash } from "@opencode-ai/core/util/hash" import fs from "fs/promises" import path from "path" -import { Deferred, Effect, Fiber, Layer } from "effect" +import { Deferred, Effect, Exit, Fiber, Layer } from "effect" +import { Flock } from "@opencode-ai/core/util/flock" +import { SnapshotIndexLock } from "@opencode-ai/core/snapshot-index-lock" import { Snapshot } from "../../src/snapshot" import { InstanceState } from "@/effect/instance-state" import { @@ -934,6 +936,102 @@ it.instance( ) +it.instance( + "does not publish an old tree or an empty hash when git add encounters index.lock", + Effect.gen(function* () { + const tmp = yield* bootstrap() + const snapshot = yield* Snapshot.Service + const before = yield* snapshot.track() + expect(before).toMatch(/^[a-f0-9]{40,64}$/) + + const ctx = yield* InstanceState.context + const gitdir = path.join(Global.Path.data, "snapshot", ctx.project.id, Hash.fast(ctx.worktree)) + const lock = path.join(gitdir, "index.lock") + yield* write(path.join(tmp.path, "new.txt"), "new snapshot content") + yield* write(lock, "simulated legacy git index writer") + + // Staging is required here, so an existing Git lock must make the whole + // capture fail closed. The invalid capture must not poison single-flight. + const failed = yield* snapshot.track() + expect(failed).toBeUndefined() + expect(yield* exists(lock)).toBe(true) + yield* rm(lock) + + const after = yield* snapshot.track() + expect(after).toMatch(/^[a-f0-9]{40,64}$/) + expect(after).not.toBe(before) + expect((yield* snapshot.patch(before!, after!)).files).toContain(fwd(tmp.path, "new.txt")) + }), + { git: true }, + { timeout: 30_000 }, +) + +it.instance( + "holds the filesystem lease across a complete snapshot capture", + Effect.gen(function* () { + const tmp = yield* bootstrap() + const snapshot = yield* Snapshot.Service + expect(yield* snapshot.track()).toBeTruthy() + const ctx = yield* InstanceState.context + const gitdir = path.join(Global.Path.data, "snapshot", ctx.project.id, Hash.fast(ctx.worktree)) + const options = { + dir: SnapshotIndexLock.root(Global.Path.data), + timeoutMs: 1_000, + } + yield* write(path.join(tmp.path, "new.txt"), "new snapshot content") + const fiber = yield* Effect.acquireUseRelease( + Effect.promise(() => Flock.acquire(SnapshotIndexLock.key(gitdir), options)), + () => + Effect.gen(function* () { + const pending = yield* snapshot.track().pipe(Effect.forkScoped) + yield* Effect.sleep("200 millis") + // The competing capture cannot advance until the external owner + // releases its lease, even though the Git index is not itself locked. + expect(pending.pollUnsafe()).toBeUndefined() + return pending + }), + (lease) => Effect.promise(() => lease.release()), + ) + expect(yield* Fiber.join(fiber)).toMatch(/^[a-f0-9]{40,64}$/) + }), + { git: true }, + { timeout: 30_000 }, +) + +it.instance( + "revert rejects paths outside the captured worktree", + Effect.gen(function* () { + const tmp = yield* bootstrap() + const snapshot = yield* Snapshot.Service + const tree = yield* snapshot.track() + expect(tree).toBeTruthy() + const outside = path.join(path.dirname(tmp.path), `${path.basename(tmp.path)}-outside.txt`) + yield* Effect.addFinalizer(() => rm(outside)) + yield* write(outside, "preserve outside file") + const result = yield* Effect.exit(snapshot.revert([{ hash: tree!, files: [outside] }])) + expect(Exit.isFailure(result)).toBe(true) + expect(yield* readText(outside)).toBe("preserve outside file") + }), + { git: true }, + { timeout: 30_000 }, +) + +it.instance( + "invalid revert refs never cause worktree files to be deleted", + Effect.gen(function* () { + const tmp = yield* bootstrap() + const snapshot = yield* Snapshot.Service + expect(yield* snapshot.track()).toBeTruthy() + const target = path.join(tmp.path, "precious.txt") + yield* write(target, "retain me") + const result = yield* Effect.exit(snapshot.revert([{ hash: "invalid-tree", files: [target] }])) + expect(Exit.isFailure(result)).toBe(true) + expect(yield* readText(target)).toBe("retain me") + }), + { git: true }, + { timeout: 30_000 }, +) + it.instance( "reconstructs a corrupt shadow index instead of failing every capture", () =>