diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0f71e000f..e580ccffb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -262,7 +262,7 @@ jobs: retention-days: 7 e2e-local: - name: E2E (stdio MCP) + name: E2E (local MCP) # Skipped on pull_request: the local scenario boots a real `executor web` # plus a browser and is currently flaky on PRs. Still runs on push to main. if: github.event_name != 'pull_request' @@ -309,14 +309,16 @@ jobs: working-directory: e2e # The `local` project is excluded from the default `test` chain (each - # scenario boots its own `executor web`). Run just the stdio MCP scenario - # here: it is the auto-connect / env-as-secret regression guard, and - # running it alone avoids the boot-resource accumulation and the - # pre-existing browser flakiness of the rest of the local suite. Expanding - # to the full `local` project (bun run test:local) is a follow-up once - # those are stabilized. - - name: Run the stdio MCP scenario - run: bunx vitest run --project local local/stdio-mcp.test.ts + # scenario boots its own `executor web`). Run the two headless MCP + # scenarios here: stdio is the auto-connect / env-as-secret regression + # guard, and toolkits covers `/mcp/toolkits/` (which regressed to a + # blanket 500 unnoticed precisely because nothing ran it). Neither drives + # a browser, so running just these avoids the boot-resource accumulation + # and the pre-existing browser flakiness of the rest of the local suite. + # Expanding to the full `local` project (bun run test:local) is a + # follow-up once those are stabilized. + - name: Run the headless MCP scenarios + run: bunx vitest run --project local local/stdio-mcp.test.ts local/toolkits-mcp.test.ts working-directory: e2e desktop-smoke: diff --git a/apps/local/src/executor.test.ts b/apps/local/src/executor.test.ts index 8156bf677..49f5d1315 100644 --- a/apps/local/src/executor.test.ts +++ b/apps/local/src/executor.test.ts @@ -4,8 +4,9 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; -import { disposeExecutor, getExecutor, reloadExecutor } from "./executor"; +import { disposeExecutor, getExecutor, getExecutorBundle, reloadExecutor } from "./executor"; const withIsolatedExecutorDataDir = async (body: () => Promise): Promise => { const previousDataDir = process.env.EXECUTOR_DATA_DIR; @@ -52,3 +53,38 @@ describe("reloadExecutor", () => { }); }); }); + +describe("createScopedExecutor", () => { + it("derives a toolkit-scoped executor while the shared bundle holds the data dir", async () => { + await withIsolatedExecutorDataDir(async () => { + const bundle = await getExecutorBundle(); + + // The bundle holds the data dir's ownership lock (a `BEGIN EXCLUSIVE` on + // `data.db.owner-lock`, per-connection, `busy_timeout = 0`) for its whole + // lifetime. Building this executor by opening a second owned database + // would hit SQLITE_BUSY against that lock and reject, which is what made + // every `/mcp/toolkits/` request 500. + const scoped = await bundle.createScopedExecutor({ activeToolkitSlug: "scoped-slug" }); + + expect(scoped.executor).toBeDefined(); + await scoped.dispose(); + }); + }); + + it("leaves the shared database open when a scoped executor is disposed", async () => { + await withIsolatedExecutorDataDir(async () => { + const bundle = await getExecutorBundle(); + const scoped = await bundle.createScopedExecutor({ activeToolkitSlug: "scoped-slug" }); + + await scoped.dispose(); + + // A scoped executor borrows the bundle's open handle, so disposing one + // must close its own plugins and nothing else. If it ever closed the + // shared database (by being built over the owning `{ db, close }` wrapper + // rather than the handle), the daemon would lose `/mcp` and `/api` the + // moment any toolkit session ended. + const integrations = await Effect.runPromise(bundle.executor.integrations.list()); + expect(Array.isArray(integrations)).toBe(true); + }); + }); +}); diff --git a/apps/local/src/executor.ts b/apps/local/src/executor.ts index 5fa7c9a0f..bfaf9f618 100644 --- a/apps/local/src/executor.ts +++ b/apps/local/src/executor.ts @@ -88,9 +88,30 @@ const loadLocalPlugins = (options: LocalExecutorOptions = {}) => }; }); +/** + * An executor over an ALREADY-OPEN local database, differing from the bundle's + * own executor only in its plugin set (the `activeToolkitSlug` seam). Disposing + * one closes just that executor's plugins: the SQLite handle and the data-dir + * ownership lock belong to the bundle that derived it. + */ +export interface ScopedExecutorHandle { + readonly executor: Executor; + readonly plugins: LocalPlugins; + readonly dispose: () => Promise; +} + interface LocalExecutorBundle { readonly executor: Executor; readonly plugins: LocalPlugins; + /** + * Derive an executor with a different plugin scope over this bundle's open + * database. The bundle holds the data dir's ownership lock (a `BEGIN + * EXCLUSIVE` on `data.db.owner-lock`) for its whole lifetime, so anything + * needing a differently-scoped executor IN THIS PROCESS must come through + * here. Opening a second owned database instead deadlocks against that lock + * and can never succeed while the daemon runs. + */ + readonly createScopedExecutor: (options: LocalExecutorOptions) => Promise; } class LocalExecutorTag extends Context.Service()( @@ -137,6 +158,31 @@ const handleOrNull = (promise: ReturnType) => ), ); +const closeExecutorOnly = (executor: Executor) => (): Promise => + Effect.runPromise(Effect.ignore(executor.close())); + +/** + * Builds a bundle's `createScopedExecutor` from the seam that makes an executor + * over its already-open database. Lives at module scope so the deferred + * `Effect.runPromise` is not nested inside the layer's own Effect. + */ +const makeScopedExecutorFactory = + (makeExecutor: (plugins: LocalPlugins) => Effect.Effect, E>) => + (scopedOptions: LocalExecutorOptions): Promise => + Effect.runPromise( + Effect.gen(function* () { + const scoped = yield* loadLocalPlugins(scopedOptions); + const scopedExecutor = yield* makeExecutor(scoped.plugins); + return { + executor: scopedExecutor, + plugins: scoped.plugins, + // Closes this executor's plugins only. The database handle and the + // data-dir ownership lock belong to the bundle that derived this. + dispose: closeExecutorOnly(scopedExecutor), + }; + }), + ); + const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => { const storage = resolveStorage(); @@ -186,23 +232,36 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => { const webBaseUrl = process.env.EXECUTOR_WEB_BASE_URL ?? `http://localhost:${process.env.PORT ?? "4788"}`; - const executor = yield* createExecutor({ - tenant: Tenant.make(tenantId), - subject: Subject.make(LOCAL_SUBJECT), - db: sqlite.db, - plugins, - onElicitation: "accept-all", - oauthEndpointUrlPolicy: { allowHttp: true }, - // EXPLICIT OAuth callback — the daemon serves the v2 `/api/oauth/callback` - // route on the same origin as the web UI. Derived from `webBaseUrl` - // (loopback localhost is correct + intended for the local CLI, but it - // is wired explicitly here rather than relying on a hidden default). - redirectUri: new URL("/api/oauth/callback", webBaseUrl).toString(), - // Built-in agent-facing tools (integrations / connections / policies). - coreTools: { - webBaseUrl, - }, - }); + // Only `plugins` varies between the boot executor and a scoped one: the + // data dir, tenant, and subject are fixed for the process. `makeExecutor` + // is that seam, and it closes over the open `sqlite.db` so deriving an + // executor never re-opens (and so never re-locks) the data dir. + // + // `db` is the FumaDB handle, NOT the owning `sqlite` wrapper: an executor + // built over a `{ db, close }` wrapper would close the SHARED database on + // its own close(). Passing the handle keeps disposal to plugins alone. + const makeExecutor = (executorPlugins: LocalPlugins) => + createExecutor({ + tenant: Tenant.make(tenantId), + subject: Subject.make(LOCAL_SUBJECT), + db: sqlite.db, + plugins: executorPlugins, + onElicitation: "accept-all", + oauthEndpointUrlPolicy: { allowHttp: true }, + // EXPLICIT OAuth callback — the daemon serves the v2 `/api/oauth/callback` + // route on the same origin as the web UI. Derived from `webBaseUrl` + // (loopback localhost is correct + intended for the local CLI, but it + // is wired explicitly here rather than relying on a hidden default). + redirectUri: new URL("/api/oauth/callback", webBaseUrl).toString(), + // Built-in agent-facing tools (integrations / connections / policies). + coreTools: { + webBaseUrl, + }, + }); + + const executor = yield* makeExecutor(plugins); + + const createScopedExecutor = makeScopedExecutorFactory(makeExecutor); if (migration.migrated) { console.warn( @@ -233,7 +292,7 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => { ); } - return { executor, plugins }; + return { executor, plugins, createScopedExecutor }; }), ); }; @@ -246,6 +305,7 @@ export const createExecutorHandle = async (options: LocalExecutorOptions = {}) = return { executor: bundle.executor, plugins: bundle.plugins, + createScopedExecutor: bundle.createScopedExecutor, dispose: async () => { await Effect.runPromise(Effect.ignore(bundle.executor.close())); await ignorePromiseFailure("disposeRuntime", () => runtime.dispose()); diff --git a/apps/local/src/main.ts b/apps/local/src/main.ts index 750cd72f6..55edee01f 100644 --- a/apps/local/src/main.ts +++ b/apps/local/src/main.ts @@ -3,7 +3,7 @@ import { Context, Data, Effect, Layer, ManagedRuntime } from "effect"; import { createExecutionEngine } from "@executor-js/execution"; import { makeQuickJsExecutor } from "@executor-js/runtime-quickjs"; import { makeLocalApiHandler } from "./app"; -import { createExecutorHandle, disposeExecutor, getExecutorBundle } from "./executor"; +import { disposeExecutor, getExecutorBundle } from "./executor"; import { createMcpRequestHandler, type McpRequestHandler } from "./mcp"; // --------------------------------------------------------------------------- @@ -81,25 +81,29 @@ export const createServerHandlers = async (token: string): Promise { if (resource.kind === "default") return { config: { engine } }; - const handle = await createExecutorHandle({ + // A toolkit resource differs from the default only in its plugin scope, + // so it derives from the boot bundle's ALREADY-OPEN database. Opening a + // second owned database here would deadlock against the data-dir + // ownership lock this very process holds, failing every request. + const scoped = await bundle.createScopedExecutor({ activeToolkitSlug: resource.slug, }); const toolkitEngine = createExecutionEngine({ - executor: handle.executor, + executor: scoped.executor, codeExecutor: makeQuickJsExecutor(), }); return { config: { engine: toolkitEngine }, - close: handle.dispose, + close: scoped.dispose, }; }, });