From 68ff83a98ddacd992b0f026cd0dd00118eda9d90 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Sat, 2 May 2026 23:23:03 -0400 Subject: [PATCH] refactor(opencode): simplify database effect reset lifecycle --- packages/opencode/src/effect/app-runtime.ts | 4 +- packages/opencode/src/storage/db-effect.ts | 18 +++-- packages/opencode/src/storage/db.ts | 12 +-- .../test/effect/managed-runtime.test.ts | 78 ------------------- packages/opencode/test/fixture/db.ts | 6 -- .../opencode/test/storage/db-effect.test.ts | 76 ++++-------------- 6 files changed, 33 insertions(+), 161 deletions(-) delete mode 100644 packages/opencode/test/effect/managed-runtime.test.ts diff --git a/packages/opencode/src/effect/app-runtime.ts b/packages/opencode/src/effect/app-runtime.ts index 50e3ac5ed9..e8c8025ea3 100644 --- a/packages/opencode/src/effect/app-runtime.ts +++ b/packages/opencode/src/effect/app-runtime.ts @@ -1,6 +1,5 @@ -import { Layer } from "effect" +import { Layer, ManagedRuntime } from "effect" import { attach } from "./run-service" -import { makeManagedRuntime } from "./managed-runtime" import * as Observability from "@opencode-ai/core/effect/observability" import { AppFileSystem } from "@opencode-ai/core/filesystem" @@ -52,6 +51,7 @@ import { ShareNext } from "@/share/share-next" import { SessionShare } from "@/share/session" import { SyncEvent } from "@/sync" import { Npm } from "@opencode-ai/core/npm" +import { memoMap } from "@opencode-ai/core/effect/memo-map" export const AppLayer = Layer.mergeAll( Npm.defaultLayer, diff --git a/packages/opencode/src/storage/db-effect.ts b/packages/opencode/src/storage/db-effect.ts index 97afcd3b5d..cb76beeefd 100644 --- a/packages/opencode/src/storage/db-effect.ts +++ b/packages/opencode/src/storage/db-effect.ts @@ -1,18 +1,20 @@ import { Database } from "@/storage/db" -import { Context, Effect, Layer } from "effect" +import { Context, Layer } from "effect" import type { EffectSQLiteDatabase } from "@opencode-ai/effect-drizzle-sqlite" import * as StorageSchema from "@/storage/schema" -// Thin Effect Service over the module-global `Database.Client` lazy. The DB -// lifecycle is owned by `Database.open` / `Database.close`, not by this -// layer. Any runtime (see `effect/managed-runtime.ts`) that consumes this -// layer through the shared layer memoMap must be disposed before -// `Database.close()` so its memoized Service value does not outlive the -// underlying SQLite handle. See `test/fixture/db.ts:resetDatabase`. export class Service extends Context.Service>()( "@opencode/DatabaseEffect", ) {} -export const layer = Layer.effect(Service, Effect.sync(Database.Client)) +const client = new Proxy({} as EffectSQLiteDatabase, { + get(_target, property) { + const db = Database.Client() + const value = Reflect.get(db, property) + return typeof value === "function" ? value.bind(db) : value + }, +}) + +export const layer = Layer.succeed(Service, client) export * as DatabaseEffect from "./db-effect" diff --git a/packages/opencode/src/storage/db.ts b/packages/opencode/src/storage/db.ts index e06b1955ce..ca2b42a78e 100644 --- a/packages/opencode/src/storage/db.ts +++ b/packages/opencode/src/storage/db.ts @@ -48,9 +48,9 @@ export type Transaction = Parameters[0]>[0] type Journal = { sql: string; timestamp: number; name: string }[] // Drizzle's migrate overloads trigger expensive variance checks here; narrow to the journal overload we actually use. -const migrateFromJournal = migrate as unknown as (db: SQLiteBunDatabase, entries: Journal) => void +const migrateFromJournal = migrate as unknown as (db: Client, entries: Journal) => void -function applyMigrations(db: SQLiteBunDatabase, entries: Journal) { +function applyMigrations(db: Client, entries: Journal) { migrateFromJournal(db, entries) } @@ -122,10 +122,10 @@ export function open() { export const Client = lazy(open) -export function close(client = Client.peek()) { - if (!client) return - client.$client.close() - Client.resetIf(client) +export function close() { + if (!Client.loaded()) return + Client().$client.close() + Client.reset() } export type TxOrDb = Transaction | Client diff --git a/packages/opencode/test/effect/managed-runtime.test.ts b/packages/opencode/test/effect/managed-runtime.test.ts deleted file mode 100644 index 35ca382cc1..0000000000 --- a/packages/opencode/test/effect/managed-runtime.test.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { describe, expect, test } from "bun:test" -import { Context, Effect, Layer } from "effect" -import { makeManagedRuntime } from "@/effect/managed-runtime" -import { lazy } from "@/util/lazy" - -class Counter extends Context.Service()("@test/Counter") {} - -const layerWith = (value: number) => Layer.succeed(Counter, { value }) - -describe("makeManagedRuntime", () => { - test("disposing an unbuilt runtime is a no-op", async () => { - const rt = makeManagedRuntime(layerWith(0)) - expect(rt.peek()).toBeUndefined() - await rt.dispose() - expect(rt.peek()).toBeUndefined() - }) - - test("disposing rebuilds on next access", async () => { - const rt = makeManagedRuntime(layerWith(7)) - const first = rt() - const value = await first.runPromise( - Effect.gen(function* () { - return (yield* Counter).value - }), - ) - expect(value).toBe(7) - - await rt.dispose() - expect(rt.peek()).toBeUndefined() - - const second = rt() - expect(second).not.toBe(first) - expect(rt.peek()).toBe(second) - await rt.dispose() - }) - - test("dispose() does not clobber a runtime that was rebuilt mid-dispose", async () => { - // Simulates a race where dispose() ran on instance A, then someone - // invoked the lazy and got a fresh instance B before dispose() returned. - // The resetIf guard must leave instance B intact. - const rt = makeManagedRuntime(layerWith(1)) - const first = rt() - rt.reset() // force-eject the lazy - const second = rt() // build a new instance, distinct from first - expect(second).not.toBe(first) - - // Calling dispose() now should tear down `second` (the current value), - // not the orphaned `first`. - await rt.dispose() - expect(rt.peek()).toBeUndefined() - }) -}) - -describe("lazy.resetIf", () => { - test("resets when the value matches", () => { - const factory = lazy(() => ({})) - const value = factory() - expect(factory.peek()).toBe(value) - factory.resetIf(value) - expect(factory.peek()).toBeUndefined() - }) - - test("leaves the lazy intact when the value does not match", () => { - const factory = lazy(() => ({})) - const captured = factory() - factory.reset() - const fresh = factory() - expect(fresh).not.toBe(captured) - factory.resetIf(captured) - expect(factory.peek()).toBe(fresh) - }) - - test("is a no-op on an unloaded lazy", () => { - const factory = lazy(() => ({})) - factory.resetIf({} as never) - expect(factory.peek()).toBeUndefined() - }) -}) diff --git a/packages/opencode/test/fixture/db.ts b/packages/opencode/test/fixture/db.ts index c3415f913b..07b42d9946 100644 --- a/packages/opencode/test/fixture/db.ts +++ b/packages/opencode/test/fixture/db.ts @@ -2,12 +2,6 @@ import { rm } from "fs/promises" import { Database } from "@/storage/db" import { disposeAllInstances } from "./fixture" -// Order matters and must stay serial: every runtime that transitively consumes -// `DatabaseEffect.layer` shares the global layer memoMap with the others, so -// each one's memoized Service value still references the live SQLite handle. -// We dispose every runtime/handler first, then close the DB. If a future -// module-scoped runtime is added that depends on the DB, register its -// dispose() here. export async function resetDatabase() { await disposeAllInstances().catch(() => undefined) Database.close() diff --git a/packages/opencode/test/storage/db-effect.test.ts b/packages/opencode/test/storage/db-effect.test.ts index 648eaaae9a..015786d5ce 100644 --- a/packages/opencode/test/storage/db-effect.test.ts +++ b/packages/opencode/test/storage/db-effect.test.ts @@ -1,6 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test" import { Effect, ManagedRuntime } from "effect" -import { memoMap } from "@opencode-ai/core/effect/memo-map" import { Database } from "@/storage/db" import { DatabaseEffect } from "@/storage/db-effect" import { resetDatabase } from "../fixture/db" @@ -25,17 +24,15 @@ describe("DatabaseEffect.layer", () => { } }) - test("rebuilds a fresh handle after Database.close + runtime dispose", async () => { - const rt1 = ManagedRuntime.make(DatabaseEffect.layer) - const first = await rt1.runPromise(Effect.sync(() => Database.Client().$client)) + test("service resolves a fresh handle after Database.close", async () => { + const rt = ManagedRuntime.make(DatabaseEffect.layer) + const first = await rt.runPromise(Effect.sync(() => Database.Client().$client)) expect(first.prepare("SELECT 1 as n").get()).toEqual({ n: 1 }) - await rt1.dispose() Database.close() - const rt2 = ManagedRuntime.make(DatabaseEffect.layer) try { - const second = await rt2.runPromise( + const second = await rt.runPromise( Effect.gen(function* () { const db = yield* DatabaseEffect.Service return db.$client @@ -44,34 +41,24 @@ describe("DatabaseEffect.layer", () => { expect(second).not.toBe(first) expect(second.prepare("SELECT 1 as n").get()).toEqual({ n: 1 }) } finally { - await rt2.dispose() + await rt.dispose() } }) -}) -// Regression for the memoMap lifecycle bug. The shared layer memoMap caches -// every `DatabaseEffect.layer` build across every runtime built with -// `makeManagedRuntime`. If a runtime that consumed the layer is NOT disposed -// before `Database.close()`, the cached Service value (a Drizzle wrapper -// over a now-closed `bun:sqlite` handle) persists in the memoMap and any -// subsequent runtime that consumes the layer reuses it and operates on a -// closed handle. -// -// `test/fixture/db.ts:resetDatabase` disposes every module-scoped runtime -// before closing the DB to release the memoMap entries. The two tests below -// pin both halves of the invariant. -describe("DatabaseEffect.layer + shared memoMap lifecycle", () => { - test("disposing a runtime releases its memoMap entry so the next build sees a fresh DB handle", async () => { - const rt1 = ManagedRuntime.make(DatabaseEffect.layer, { memoMap }) - const captured = await rt1.runPromise(Effect.sync(() => Database.Client().$client)) + test("a runtime kept alive over Database.close uses the refreshed handle", async () => { + const rt = ManagedRuntime.make(DatabaseEffect.layer) + const captured = await rt.runPromise( + Effect.gen(function* () { + const db = yield* DatabaseEffect.Service + return db.$client + }), + ) expect(captured.prepare("SELECT 1 as n").get()).toEqual({ n: 1 }) - await rt1.dispose() Database.close() - const rt2 = ManagedRuntime.make(DatabaseEffect.layer, { memoMap }) try { - const fresh = await rt2.runPromise( + const fresh = await rt.runPromise( Effect.gen(function* () { const db = yield* DatabaseEffect.Service return db.$client @@ -80,40 +67,7 @@ describe("DatabaseEffect.layer + shared memoMap lifecycle", () => { expect(fresh).not.toBe(captured) expect(fresh.prepare("SELECT 1 as n").get()).toEqual({ n: 1 }) } finally { - await rt2.dispose() - } - }) - - test("a stale runtime kept alive over Database.close poisons later memoMap consumers", async () => { - const stale = ManagedRuntime.make(DatabaseEffect.layer, { memoMap }) - const captured = await stale.runPromise( - Effect.gen(function* () { - const db = yield* DatabaseEffect.Service - return db.$client - }), - ) - expect(captured.prepare("SELECT 1 as n").get()).toEqual({ n: 1 }) - - // Intentionally do NOT dispose `stale` before closing the DB. This is - // the shape of the bug `resetDatabase` guards against. - Database.close() - - const next = ManagedRuntime.make(DatabaseEffect.layer, { memoMap }) - try { - const seen = await next.runPromise( - Effect.gen(function* () { - const db = yield* DatabaseEffect.Service - return db.$client - }), - ) - // The memoMap returned the same stale handle because `stale` was - // never disposed. The underlying connection is closed, so any query - // on the handle throws. - expect(seen).toBe(captured) - expect(() => seen.prepare("SELECT 1 as n").get()).toThrow() - } finally { - await next.dispose() - await stale.dispose() + await rt.dispose() } }) })