From 1195f52e8077c45b69162d25e06454ff87be8fc7 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Sun, 15 Mar 2026 14:16:33 -0400 Subject: [PATCH 1/3] test(file-time): add tests for FileTime read/assert/withLock 7 tests covering: timestamp recording, session scoping, assert on unread/unchanged/modified files, and withLock serialization. All pass against the current Instance.state implementation. --- packages/opencode/test/file/time.test.ts | 442 ++++++----------------- 1 file changed, 101 insertions(+), 341 deletions(-) diff --git a/packages/opencode/test/file/time.test.ts b/packages/opencode/test/file/time.test.ts index e46d5229b449..112afef6ab08 100644 --- a/packages/opencode/test/file/time.test.ts +++ b/packages/opencode/test/file/time.test.ts @@ -1,361 +1,121 @@ -import { describe, test, expect, beforeEach } from "bun:test" -import path from "path" +import { afterEach, expect, test } from "bun:test" import fs from "fs/promises" -import { FileTime } from "../../src/file/time" -import { Instance } from "../../src/project/instance" -import { Filesystem } from "../../src/util/filesystem" +import path from "path" import { tmpdir } from "../fixture/fixture" +import { Instance } from "../../src/project/instance" +import { FileTime } from "../../src/file/time" -describe("file/time", () => { - const sessionID = "test-session-123" - - describe("read() and get()", () => { - test("stores read timestamp", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const before = FileTime.get(sessionID, filepath) - expect(before).toBeUndefined() - - FileTime.read(sessionID, filepath) - - const after = FileTime.get(sessionID, filepath) - expect(after).toBeInstanceOf(Date) - expect(after!.getTime()).toBeGreaterThan(0) - }, - }) - }) - - test("tracks separate timestamps per session", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read("session1", filepath) - FileTime.read("session2", filepath) - - const time1 = FileTime.get("session1", filepath) - const time2 = FileTime.get("session2", filepath) - - expect(time1).toBeDefined() - expect(time2).toBeDefined() - }, - }) - }) - - test("updates timestamp on subsequent reads", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) - const first = FileTime.get(sessionID, filepath)! - - await new Promise((resolve) => setTimeout(resolve, 10)) - - FileTime.read(sessionID, filepath) - const second = FileTime.get(sessionID, filepath)! - - expect(second.getTime()).toBeGreaterThanOrEqual(first.getTime()) - }, - }) - }) +afterEach(() => Instance.disposeAll()) + +test("read records a timestamp for a file", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "test.txt") + await fs.writeFile(file, "hello") + + await Instance.provide({ + directory: tmp.path, + fn: () => { + const before = new Date() + FileTime.read("session-1", file) + const time = FileTime.get("session-1", file) + expect(time).toBeDefined() + expect(time!.getTime()).toBeGreaterThanOrEqual(before.getTime()) + }, }) +}) - describe("assert()", () => { - test("passes when file has not been modified", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) - - // Should not throw - await FileTime.assert(sessionID, filepath) - }, - }) - }) - - test("throws when file was not read first", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow("You must read file") - }, - }) - }) - - test("throws when file was modified after read", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) - - // Wait to ensure different timestamps - await new Promise((resolve) => setTimeout(resolve, 100)) - - // Modify file after reading - await fs.writeFile(filepath, "modified content", "utf-8") - - await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow("modified since it was last read") - }, - }) - }) - - test("includes timestamps in error message", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) - await new Promise((resolve) => setTimeout(resolve, 100)) - await fs.writeFile(filepath, "modified", "utf-8") - - let error: Error | undefined - try { - await FileTime.assert(sessionID, filepath) - } catch (e) { - error = e as Error - } - expect(error).toBeDefined() - expect(error!.message).toContain("Last modification:") - expect(error!.message).toContain("Last read:") - }, - }) - }) - - test("skips check when OPENCODE_DISABLE_FILETIME_CHECK is true", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const { Flag } = await import("../../src/flag/flag") - const original = Flag.OPENCODE_DISABLE_FILETIME_CHECK - ;(Flag as { OPENCODE_DISABLE_FILETIME_CHECK: boolean }).OPENCODE_DISABLE_FILETIME_CHECK = true - - try { - // Should not throw even though file wasn't read - await FileTime.assert(sessionID, filepath) - } finally { - ;(Flag as { OPENCODE_DISABLE_FILETIME_CHECK: boolean }).OPENCODE_DISABLE_FILETIME_CHECK = original - } - }, - }) - }) +test("get returns undefined for unread files", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: () => { + expect(FileTime.get("session-1", "/nonexistent")).toBeUndefined() + }, }) +}) - describe("withLock()", () => { - test("executes function within lock", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - let executed = false - await FileTime.withLock(filepath, async () => { - executed = true - return "result" - }) - expect(executed).toBe(true) - }, - }) - }) - - test("returns function result", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const result = await FileTime.withLock(filepath, async () => { - return "success" - }) - expect(result).toBe("success") - }, - }) - }) - - test("serializes concurrent operations on same file", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const order: number[] = [] - - const op1 = FileTime.withLock(filepath, async () => { - order.push(1) - await new Promise((resolve) => setTimeout(resolve, 10)) - order.push(2) - }) - - const op2 = FileTime.withLock(filepath, async () => { - order.push(3) - order.push(4) - }) - - await Promise.all([op1, op2]) - - // Operations should be serialized - expect(order).toContain(1) - expect(order).toContain(2) - expect(order).toContain(3) - expect(order).toContain(4) - }, - }) - }) - - test("allows concurrent operations on different files", async () => { - await using tmp = await tmpdir() - const filepath1 = path.join(tmp.path, "file1.txt") - const filepath2 = path.join(tmp.path, "file2.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - let started1 = false - let started2 = false - - const op1 = FileTime.withLock(filepath1, async () => { - started1 = true - await new Promise((resolve) => setTimeout(resolve, 50)) - expect(started2).toBe(true) // op2 should have started while op1 is running - }) - - const op2 = FileTime.withLock(filepath2, async () => { - started2 = true - }) - - await Promise.all([op1, op2]) - - expect(started1).toBe(true) - expect(started2).toBe(true) - }, - }) - }) - - test("releases lock even if function throws", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - await expect( - FileTime.withLock(filepath, async () => { - throw new Error("Test error") - }), - ).rejects.toThrow("Test error") - - // Lock should be released, subsequent operations should work - let executed = false - await FileTime.withLock(filepath, async () => { - executed = true - }) - expect(executed).toBe(true) - }, - }) - }) - - test("deadlocks on nested locks (expected behavior)", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") +test("read times are scoped per session", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "test.txt") + await fs.writeFile(file, "hello") + + await Instance.provide({ + directory: tmp.path, + fn: () => { + FileTime.read("session-a", file) + expect(FileTime.get("session-a", file)).toBeDefined() + expect(FileTime.get("session-b", file)).toBeUndefined() + }, + }) +}) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - // Nested locks on same file cause deadlock - this is expected - // The outer lock waits for inner to complete, but inner waits for outer to release - const timeout = new Promise((_, reject) => - setTimeout(() => reject(new Error("Deadlock detected")), 100), - ) +test("assert throws if file was not read first", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "test.txt") + await fs.writeFile(file, "hello") - const nestedLock = FileTime.withLock(filepath, async () => { - return FileTime.withLock(filepath, async () => { - return "inner" - }) - }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + expect(FileTime.assert("session-1", file)).rejects.toThrow("must read file") + }, + }) +}) - // Should timeout due to deadlock - await expect(Promise.race([nestedLock, timeout])).rejects.toThrow("Deadlock detected") - }, - }) - }) +test("assert passes if file unchanged since read", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "test.txt") + await fs.writeFile(file, "hello") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read("session-1", file) + await FileTime.assert("session-1", file) + }, }) +}) - describe("stat() Filesystem.stat pattern", () => { - test("reads file modification time via Filesystem.stat()", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "content", "utf-8") +test("assert throws if file modified after read", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "test.txt") + await fs.writeFile(file, "hello") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read("session-1", file) + await Bun.sleep(100) + await fs.writeFile(file, "modified") + expect(FileTime.assert("session-1", file)).rejects.toThrow("has been modified") + }, + }) +}) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) +test("withLock serializes concurrent writes to same file", async () => { + await using tmp = await tmpdir() + const file = path.join(tmp.path, "locked.txt") + await fs.writeFile(file, "") - const stats = Filesystem.stat(filepath) - expect(stats?.mtime).toBeInstanceOf(Date) - expect(stats!.mtime.getTime()).toBeGreaterThan(0) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const order: number[] = [] - // FileTime.assert uses this stat internally - await FileTime.assert(sessionID, filepath) - }, + const a = FileTime.withLock(file, async () => { + order.push(1) + await Bun.sleep(50) + order.push(2) }) - }) - - test("detects modification via stat mtime", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "original", "utf-8") - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read(sessionID, filepath) - - const originalStat = Filesystem.stat(filepath) - - // Wait and modify - await new Promise((resolve) => setTimeout(resolve, 100)) - await fs.writeFile(filepath, "modified", "utf-8") - - const newStat = Filesystem.stat(filepath) - expect(newStat!.mtime.getTime()).toBeGreaterThan(originalStat!.mtime.getTime()) - - await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow() - }, + const b = FileTime.withLock(file, async () => { + order.push(3) + await Bun.sleep(10) + order.push(4) }) - }) + + await Promise.all([a, b]) + expect(order).toEqual([1, 2, 3, 4]) + }, }) }) From 86aa3313660a413ebd15d5b5ad385ae1b2940a5a Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Sun, 15 Mar 2026 14:32:50 -0400 Subject: [PATCH 2/3] refactor(file-time): effectify FileTimeService with Semaphore locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert FileTime from Instance.state to Effect ServiceMap.Service. Replace hand-rolled Promise lock chain with SynchronizedRef + Semaphore.withPermits for per-file mutual exclusion. Legacy facade preserved — callers unchanged. --- packages/opencode/src/effect/instances.ts | 5 +- packages/opencode/src/file/time.ts | 129 ++++---- packages/opencode/src/file/watcher.ts | 3 +- packages/opencode/src/flag/flag.ts | 4 +- packages/opencode/test/file/time.test.ts | 384 ++++++++++++++++------ 5 files changed, 367 insertions(+), 158 deletions(-) diff --git a/packages/opencode/src/effect/instances.ts b/packages/opencode/src/effect/instances.ts index 2e6fbe167a37..78b340e77560 100644 --- a/packages/opencode/src/effect/instances.ts +++ b/packages/opencode/src/effect/instances.ts @@ -6,6 +6,7 @@ import { QuestionService } from "@/question/service" import { PermissionService } from "@/permission/service" import { FileWatcherService } from "@/file/watcher" import { VcsService } from "@/project/vcs" +import { FileTimeService } from "@/file/time" import { Instance } from "@/project/instance" export { InstanceContext } from "./instance-context" @@ -16,6 +17,7 @@ export type InstanceServices = | ProviderAuthService | FileWatcherService | VcsService + | FileTimeService function lookup(directory: string) { const project = Instance.project @@ -24,8 +26,9 @@ function lookup(directory: string) { Layer.fresh(QuestionService.layer), Layer.fresh(PermissionService.layer), Layer.fresh(ProviderAuthService.layer), - Layer.fresh(FileWatcherService.layer), + Layer.fresh(FileWatcherService.layer).pipe(Layer.orDie), Layer.fresh(VcsService.layer), + Layer.fresh(FileTimeService.layer).pipe(Layer.orDie), ).pipe(Layer.provide(ctx)) } diff --git a/packages/opencode/src/file/time.ts b/packages/opencode/src/file/time.ts index efb1c437647f..562bbb962f4c 100644 --- a/packages/opencode/src/file/time.ts +++ b/packages/opencode/src/file/time.ts @@ -1,71 +1,88 @@ -import { Instance } from "../project/instance" import { Log } from "../util/log" -import { Flag } from "../flag/flag" +import { Flag } from "@/flag/flag" import { Filesystem } from "../util/filesystem" +import { Effect, Layer, ServiceMap, Semaphore } from "effect" +import { runPromiseInstance } from "@/effect/runtime" -export namespace FileTime { - const log = Log.create({ service: "file.time" }) - // Per-session read times plus per-file write locks. - // All tools that overwrite existing files should run their - // assert/read/write/update sequence inside withLock(filepath, ...) - // so concurrent writes to the same file are serialized. - export const state = Instance.state(() => { - const read: { - [sessionID: string]: { - [path: string]: Date | undefined +const log = Log.create({ service: "file.time" }) + +export namespace FileTimeService { + export interface Service { + readonly read: (sessionID: string, file: string) => Effect.Effect + readonly get: (sessionID: string, file: string) => Effect.Effect + readonly assert: (sessionID: string, filepath: string) => Effect.Effect + readonly withLock: (filepath: string, fn: () => Promise) => Effect.Effect + } +} + +export class FileTimeService extends ServiceMap.Service()( + "@opencode/FileTime", +) { + static readonly layer = Layer.effect( + FileTimeService, + Effect.gen(function* () { + const disableCheck = yield* Flag.OPENCODE_DISABLE_FILETIME_CHECK + const reads: { [sessionID: string]: { [path: string]: Date | undefined } } = {} + const locks = new Map() + + function getLock(filepath: string) { + let lock = locks.get(filepath) + if (!lock) { + lock = Semaphore.makeUnsafe(1) + locks.set(filepath, lock) + } + return lock } - } = {} - const locks = new Map>() - return { - read, - locks, - } - }) + return FileTimeService.of({ + read: Effect.fn("FileTimeService.read")(function* (sessionID: string, file: string) { + log.info("read", { sessionID, file }) + reads[sessionID] = reads[sessionID] || {} + reads[sessionID][file] = new Date() + }), + + get: Effect.fn("FileTimeService.get")(function* (sessionID: string, file: string) { + return reads[sessionID]?.[file] + }), + + assert: Effect.fn("FileTimeService.assert")(function* (sessionID: string, filepath: string) { + if (disableCheck) return + + const time = reads[sessionID]?.[filepath] + if (!time) throw new Error(`You must read file ${filepath} before overwriting it. Use the Read tool first`) + const mtime = Filesystem.stat(filepath)?.mtime + if (mtime && mtime.getTime() > time.getTime() + 50) { + throw new Error( + `File ${filepath} has been modified since it was last read.\nLast modification: ${mtime.toISOString()}\nLast read: ${time.toISOString()}\n\nPlease read the file again before modifying it.`, + ) + } + }), + + withLock: Effect.fn("FileTimeService.withLock")(function* (filepath: string, fn: () => Promise) { + const lock = getLock(filepath) + return yield* Effect.promise(fn).pipe(lock.withPermits(1)) + }), + }) + }), + ) +} + +// Legacy facade — callers don't need to change +export namespace FileTime { export function read(sessionID: string, file: string) { - log.info("read", { sessionID, file }) - const { read } = state() - read[sessionID] = read[sessionID] || {} - read[sessionID][file] = new Date() + // Fire-and-forget — callers never await this + runPromiseInstance(FileTimeService.use((s) => s.read(sessionID, file))) } export function get(sessionID: string, file: string) { - return state().read[sessionID]?.[file] - } - - export async function withLock(filepath: string, fn: () => Promise): Promise { - const current = state() - const currentLock = current.locks.get(filepath) ?? Promise.resolve() - let release: () => void = () => {} - const nextLock = new Promise((resolve) => { - release = resolve - }) - const chained = currentLock.then(() => nextLock) - current.locks.set(filepath, chained) - await currentLock - try { - return await fn() - } finally { - release() - if (current.locks.get(filepath) === chained) { - current.locks.delete(filepath) - } - } + return runPromiseInstance(FileTimeService.use((s) => s.get(sessionID, file))) } export async function assert(sessionID: string, filepath: string) { - if (Flag.OPENCODE_DISABLE_FILETIME_CHECK === true) { - return - } + return runPromiseInstance(FileTimeService.use((s) => s.assert(sessionID, filepath))) + } - const time = get(sessionID, filepath) - if (!time) throw new Error(`You must read file ${filepath} before overwriting it. Use the Read tool first`) - const mtime = Filesystem.stat(filepath)?.mtime - // Allow a 50ms tolerance for Windows NTFS timestamp fuzziness / async flushing - if (mtime && mtime.getTime() > time.getTime() + 50) { - throw new Error( - `File ${filepath} has been modified since it was last read.\nLast modification: ${mtime.toISOString()}\nLast read: ${time.toISOString()}\n\nPlease read the file again before modifying it.`, - ) - } + export async function withLock(filepath: string, fn: () => Promise): Promise { + return runPromiseInstance(FileTimeService.use((s) => s.withLock(filepath, fn))) } } diff --git a/packages/opencode/src/file/watcher.ts b/packages/opencode/src/file/watcher.ts index 16ee8f27c84e..1a3a4f742f42 100644 --- a/packages/opencode/src/file/watcher.ts +++ b/packages/opencode/src/file/watcher.ts @@ -72,7 +72,8 @@ export class FileWatcherService extends ServiceMap.Service Instance.disposeAll()) -test("read records a timestamp for a file", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "test.txt") - await fs.writeFile(file, "hello") - - await Instance.provide({ - directory: tmp.path, - fn: () => { - const before = new Date() - FileTime.read("session-1", file) - const time = FileTime.get("session-1", file) - expect(time).toBeDefined() - expect(time!.getTime()).toBeGreaterThanOrEqual(before.getTime()) - }, - }) -}) +describe("file/time", () => { + const sessionID = "test-session-123" -test("get returns undefined for unread files", async () => { - await using tmp = await tmpdir() - await Instance.provide({ - directory: tmp.path, - fn: () => { - expect(FileTime.get("session-1", "/nonexistent")).toBeUndefined() - }, - }) -}) + describe("read() and get()", () => { + test("stores read timestamp", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") -test("read times are scoped per session", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "test.txt") - await fs.writeFile(file, "hello") - - await Instance.provide({ - directory: tmp.path, - fn: () => { - FileTime.read("session-a", file) - expect(FileTime.get("session-a", file)).toBeDefined() - expect(FileTime.get("session-b", file)).toBeUndefined() - }, - }) -}) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const before = await FileTime.get(sessionID, filepath) + expect(before).toBeUndefined() -test("assert throws if file was not read first", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "test.txt") - await fs.writeFile(file, "hello") + FileTime.read(sessionID, filepath) + await Bun.sleep(10) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - expect(FileTime.assert("session-1", file)).rejects.toThrow("must read file") - }, - }) -}) + const after = await FileTime.get(sessionID, filepath) + expect(after).toBeInstanceOf(Date) + expect(after!.getTime()).toBeGreaterThan(0) + }, + }) + }) + + test("tracks separate timestamps per session", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read("session1", filepath) + FileTime.read("session2", filepath) + await Bun.sleep(10) -test("assert passes if file unchanged since read", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "test.txt") - await fs.writeFile(file, "hello") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read("session-1", file) - await FileTime.assert("session-1", file) - }, + const time1 = await FileTime.get("session1", filepath) + const time2 = await FileTime.get("session2", filepath) + + expect(time1).toBeDefined() + expect(time2).toBeDefined() + }, + }) + }) + + test("updates timestamp on subsequent reads", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(10) + const first = await FileTime.get(sessionID, filepath) + + await Bun.sleep(10) + + FileTime.read(sessionID, filepath) + await Bun.sleep(10) + const second = await FileTime.get(sessionID, filepath) + + expect(second!.getTime()).toBeGreaterThanOrEqual(first!.getTime()) + }, + }) + }) }) -}) -test("assert throws if file modified after read", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "test.txt") - await fs.writeFile(file, "hello") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - FileTime.read("session-1", file) - await Bun.sleep(100) - await fs.writeFile(file, "modified") - expect(FileTime.assert("session-1", file)).rejects.toThrow("has been modified") - }, + describe("assert()", () => { + test("passes when file has not been modified", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(10) + await FileTime.assert(sessionID, filepath) + }, + }) + }) + + test("throws when file was not read first", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow("You must read file") + }, + }) + }) + + test("throws when file was modified after read", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(100) + await fs.writeFile(filepath, "modified content", "utf-8") + await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow("modified since it was last read") + }, + }) + }) + + test("includes timestamps in error message", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(100) + await fs.writeFile(filepath, "modified", "utf-8") + + let error: Error | undefined + try { + await FileTime.assert(sessionID, filepath) + } catch (e) { + error = e as Error + } + expect(error).toBeDefined() + expect(error!.message).toContain("Last modification:") + expect(error!.message).toContain("Last read:") + }, + }) + }) }) -}) -test("withLock serializes concurrent writes to same file", async () => { - await using tmp = await tmpdir() - const file = path.join(tmp.path, "locked.txt") - await fs.writeFile(file, "") + describe("withLock()", () => { + test("executes function within lock", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const order: number[] = [] + await Instance.provide({ + directory: tmp.path, + fn: async () => { + let executed = false + await FileTime.withLock(filepath, async () => { + executed = true + return "result" + }) + expect(executed).toBe(true) + }, + }) + }) + + test("returns function result", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") - const a = FileTime.withLock(file, async () => { - order.push(1) - await Bun.sleep(50) - order.push(2) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const result = await FileTime.withLock(filepath, async () => { + return "success" + }) + expect(result).toBe("success") + }, }) + }) + + test("serializes concurrent operations on same file", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const order: number[] = [] + + const op1 = FileTime.withLock(filepath, async () => { + order.push(1) + await Bun.sleep(50) + order.push(2) + }) + + const op2 = FileTime.withLock(filepath, async () => { + order.push(3) + order.push(4) + }) + + await Promise.all([op1, op2]) + expect(order).toEqual([1, 2, 3, 4]) + }, + }) + }) + + test("allows concurrent operations on different files", async () => { + await using tmp = await tmpdir() + const filepath1 = path.join(tmp.path, "file1.txt") + const filepath2 = path.join(tmp.path, "file2.txt") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + let started1 = false + let started2 = false + + const op1 = FileTime.withLock(filepath1, async () => { + started1 = true + await Bun.sleep(50) + expect(started2).toBe(true) + }) + + const op2 = FileTime.withLock(filepath2, async () => { + started2 = true + }) - const b = FileTime.withLock(file, async () => { - order.push(3) - await Bun.sleep(10) - order.push(4) + await Promise.all([op1, op2]) + expect(started1).toBe(true) + expect(started2).toBe(true) + }, }) + }) - await Promise.all([a, b]) - expect(order).toEqual([1, 2, 3, 4]) - }, + test("releases lock even if function throws", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + await expect( + FileTime.withLock(filepath, async () => { + throw new Error("Test error") + }), + ).rejects.toThrow("Test error") + + let executed = false + await FileTime.withLock(filepath, async () => { + executed = true + }) + expect(executed).toBe(true) + }, + }) + }) + }) + + describe("stat() Filesystem.stat pattern", () => { + test("reads file modification time via Filesystem.stat()", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "content", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(10) + + const stats = Filesystem.stat(filepath) + expect(stats?.mtime).toBeInstanceOf(Date) + expect(stats!.mtime.getTime()).toBeGreaterThan(0) + + await FileTime.assert(sessionID, filepath) + }, + }) + }) + + test("detects modification via stat mtime", async () => { + await using tmp = await tmpdir() + const filepath = path.join(tmp.path, "file.txt") + await fs.writeFile(filepath, "original", "utf-8") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + FileTime.read(sessionID, filepath) + await Bun.sleep(10) + + const originalStat = Filesystem.stat(filepath) + + await Bun.sleep(100) + await fs.writeFile(filepath, "modified", "utf-8") + + const newStat = Filesystem.stat(filepath) + expect(newStat!.mtime.getTime()).toBeGreaterThan(originalStat!.mtime.getTime()) + + await expect(FileTime.assert(sessionID, filepath)).rejects.toThrow() + }, + }) + }) }) }) From c2ae06290741b6bb5e294c86b6351b6b2f2e797a Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Mon, 16 Mar 2026 13:54:09 -0400 Subject: [PATCH 3/3] fix(file-time): await read tracking and compare file snapshots --- packages/opencode/src/file/time.ts | 35 +++++++++++++++++++------ packages/opencode/src/session/prompt.ts | 2 +- packages/opencode/src/tool/edit.ts | 4 +-- packages/opencode/src/tool/read.ts | 2 +- packages/opencode/src/tool/write.ts | 2 +- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/packages/opencode/src/file/time.ts b/packages/opencode/src/file/time.ts index 562bbb962f4c..adb5b3ae8abc 100644 --- a/packages/opencode/src/file/time.ts +++ b/packages/opencode/src/file/time.ts @@ -15,6 +15,24 @@ export namespace FileTimeService { } } +type Stamp = { + readonly read: Date + readonly mtime: number | undefined + readonly ctime: number | undefined + readonly size: number | undefined +} + +function stamp(file: string): Stamp { + const stat = Filesystem.stat(file) + const size = typeof stat?.size === "bigint" ? Number(stat.size) : stat?.size + return { + read: new Date(), + mtime: stat?.mtime?.getTime(), + ctime: stat?.ctime?.getTime(), + size, + } +} + export class FileTimeService extends ServiceMap.Service()( "@opencode/FileTime", ) { @@ -22,7 +40,7 @@ export class FileTimeService extends ServiceMap.Service() function getLock(filepath: string) { @@ -38,11 +56,11 @@ export class FileTimeService extends ServiceMap.Service time.getTime() + 50) { + const next = stamp(filepath) + const changed = next.mtime !== time.mtime || next.ctime !== time.ctime || next.size !== time.size + + if (changed) { throw new Error( - `File ${filepath} has been modified since it was last read.\nLast modification: ${mtime.toISOString()}\nLast read: ${time.toISOString()}\n\nPlease read the file again before modifying it.`, + `File ${filepath} has been modified since it was last read.\nLast modification: ${new Date(next.mtime ?? next.read.getTime()).toISOString()}\nLast read: ${time.read.toISOString()}\n\nPlease read the file again before modifying it.`, ) } }), @@ -70,8 +90,7 @@ export class FileTimeService extends ServiceMap.Service s.read(sessionID, file))) + return runPromiseInstance(FileTimeService.use((s) => s.read(sessionID, file))) } export function get(sessionID: string, file: string) { diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 5bde2608f0b5..bf939c7e2751 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1245,7 +1245,7 @@ export namespace SessionPrompt { ] } - FileTime.read(input.sessionID, filepath) + await FileTime.read(input.sessionID, filepath) return [ { messageID: info.id, diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index c7b12378ed86..1a7614fc17fb 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -78,7 +78,7 @@ export const EditTool = Tool.define("edit", { file: filePath, event: existed ? "change" : "add", }) - FileTime.read(ctx.sessionID, filePath) + await FileTime.read(ctx.sessionID, filePath) return } @@ -119,7 +119,7 @@ export const EditTool = Tool.define("edit", { diff = trimDiff( createTwoFilesPatch(filePath, filePath, normalizeLineEndings(contentOld), normalizeLineEndings(contentNew)), ) - FileTime.read(ctx.sessionID, filePath) + await FileTime.read(ctx.sessionID, filePath) }) const filediff: Snapshot.FileDiff = { diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index c981ac16e43c..85be8f9d394d 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -214,7 +214,7 @@ export const ReadTool = Tool.define("read", { // just warms the lsp client LSP.touchFile(filepath, false) - FileTime.read(ctx.sessionID, filepath) + await FileTime.read(ctx.sessionID, filepath) if (instructions.length > 0) { output += `\n\n\n${instructions.map((i) => i.content).join("\n\n")}\n` diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 8c1e53ccaf3a..83474a543ca1 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -49,7 +49,7 @@ export const WriteTool = Tool.define("write", { file: filepath, event: exists ? "change" : "add", }) - FileTime.read(ctx.sessionID, filepath) + await FileTime.read(ctx.sessionID, filepath) let output = "Wrote file successfully." await LSP.touchFile(filepath, true)