Skip to content

Commit 624805a

Browse files
committed
Derive toolkit MCP executors over the daemon's open database
The daemon's boot bundle holds the data dir's ownership lock (a BEGIN EXCLUSIVE held open on `data.db.owner-lock`, per-connection, with `busy_timeout = 0`) for its whole lifetime. The toolkit MCP resource built its executor with `createExecutorHandle({ activeToolkitSlug })`, which opens a second owned database and so deadlocks against the lock this very process holds. Every `/mcp/toolkits/<slug>` request failed with SQLITE_BUSY, surfaced as a 500 (-32603), while the default `/mcp` resource kept working. `activeToolkitSlug` only varies the plugin set: it adds a tool policy provider to the toolkits plugin, while tenant, subject, and data dir are fixed for the process. The second open was never needed. Extract the `makeExecutor(plugins)` seam over the bundle's already-open handle and derive toolkit executors from it, mirroring how self-host pairs a long-lived database with per-request plugins. The executor is built over the inner FumaDB handle rather than the owning `{ db, close }` wrapper, so a scoped executor's `close()` tears down its own plugins and never the shared database. Cover both invariants in `apps/local` unit tests, which run on every PR, and add the toolkits scenario to the e2e job that previously ran only the stdio one. That gap is why this shipped unnoticed.
1 parent 0a50c79 commit 624805a

4 files changed

Lines changed: 136 additions & 34 deletions

File tree

.github/workflows/ci.yml

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ jobs:
262262
retention-days: 7
263263

264264
e2e-local:
265-
name: E2E (stdio MCP)
265+
name: E2E (local MCP)
266266
# Skipped on pull_request: the local scenario boots a real `executor web`
267267
# plus a browser and is currently flaky on PRs. Still runs on push to main.
268268
if: github.event_name != 'pull_request'
@@ -309,14 +309,16 @@ jobs:
309309
working-directory: e2e
310310

311311
# The `local` project is excluded from the default `test` chain (each
312-
# scenario boots its own `executor web`). Run just the stdio MCP scenario
313-
# here: it is the auto-connect / env-as-secret regression guard, and
314-
# running it alone avoids the boot-resource accumulation and the
315-
# pre-existing browser flakiness of the rest of the local suite. Expanding
316-
# to the full `local` project (bun run test:local) is a follow-up once
317-
# those are stabilized.
318-
- name: Run the stdio MCP scenario
319-
run: bunx vitest run --project local local/stdio-mcp.test.ts
312+
# scenario boots its own `executor web`). Run the two headless MCP
313+
# scenarios here: stdio is the auto-connect / env-as-secret regression
314+
# guard, and toolkits covers `/mcp/toolkits/<slug>` (which regressed to a
315+
# blanket 500 unnoticed precisely because nothing ran it). Neither drives
316+
# a browser, so running just these avoids the boot-resource accumulation
317+
# and the pre-existing browser flakiness of the rest of the local suite.
318+
# Expanding to the full `local` project (bun run test:local) is a
319+
# follow-up once those are stabilized.
320+
- name: Run the headless MCP scenarios
321+
run: bunx vitest run --project local local/stdio-mcp.test.ts local/toolkits-mcp.test.ts
320322
working-directory: e2e
321323

322324
desktop-smoke:

apps/local/src/executor.test.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ import { tmpdir } from "node:os";
44
import { join } from "node:path";
55

66
import { describe, expect, it } from "@effect/vitest";
7+
import { Effect } from "effect";
78

8-
import { disposeExecutor, getExecutor, reloadExecutor } from "./executor";
9+
import { disposeExecutor, getExecutor, getExecutorBundle, reloadExecutor } from "./executor";
910

1011
const withIsolatedExecutorDataDir = async (body: () => Promise<void>): Promise<void> => {
1112
const previousDataDir = process.env.EXECUTOR_DATA_DIR;
@@ -52,3 +53,38 @@ describe("reloadExecutor", () => {
5253
});
5354
});
5455
});
56+
57+
describe("createScopedExecutor", () => {
58+
it("derives a toolkit-scoped executor while the shared bundle holds the data dir", async () => {
59+
await withIsolatedExecutorDataDir(async () => {
60+
const bundle = await getExecutorBundle();
61+
62+
// The bundle holds the data dir's ownership lock (a `BEGIN EXCLUSIVE` on
63+
// `data.db.owner-lock`, per-connection, `busy_timeout = 0`) for its whole
64+
// lifetime. Building this executor by opening a second owned database
65+
// would hit SQLITE_BUSY against that lock and reject, which is what made
66+
// every `/mcp/toolkits/<slug>` request 500.
67+
const scoped = await bundle.createScopedExecutor({ activeToolkitSlug: "scoped-slug" });
68+
69+
expect(scoped.executor).toBeDefined();
70+
await scoped.dispose();
71+
});
72+
});
73+
74+
it("leaves the shared database open when a scoped executor is disposed", async () => {
75+
await withIsolatedExecutorDataDir(async () => {
76+
const bundle = await getExecutorBundle();
77+
const scoped = await bundle.createScopedExecutor({ activeToolkitSlug: "scoped-slug" });
78+
79+
await scoped.dispose();
80+
81+
// A scoped executor borrows the bundle's open handle, so disposing one
82+
// must close its own plugins and nothing else. If it ever closed the
83+
// shared database (by being built over the owning `{ db, close }` wrapper
84+
// rather than the handle), the daemon would lose `/mcp` and `/api` the
85+
// moment any toolkit session ended.
86+
const integrations = await Effect.runPromise(bundle.executor.integrations.list());
87+
expect(Array.isArray(integrations)).toBe(true);
88+
});
89+
});
90+
});

apps/local/src/executor.ts

Lines changed: 78 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,30 @@ const loadLocalPlugins = (options: LocalExecutorOptions = {}) =>
8888
};
8989
});
9090

91+
/**
92+
* An executor over an ALREADY-OPEN local database, differing from the bundle's
93+
* own executor only in its plugin set (the `activeToolkitSlug` seam). Disposing
94+
* one closes just that executor's plugins: the SQLite handle and the data-dir
95+
* ownership lock belong to the bundle that derived it.
96+
*/
97+
export interface ScopedExecutorHandle {
98+
readonly executor: Executor<LocalPlugins>;
99+
readonly plugins: LocalPlugins;
100+
readonly dispose: () => Promise<void>;
101+
}
102+
91103
interface LocalExecutorBundle {
92104
readonly executor: Executor<LocalPlugins>;
93105
readonly plugins: LocalPlugins;
106+
/**
107+
* Derive an executor with a different plugin scope over this bundle's open
108+
* database. The bundle holds the data dir's ownership lock (a `BEGIN
109+
* EXCLUSIVE` on `data.db.owner-lock`) for its whole lifetime, so anything
110+
* needing a differently-scoped executor IN THIS PROCESS must come through
111+
* here. Opening a second owned database instead deadlocks against that lock
112+
* and can never succeed while the daemon runs.
113+
*/
114+
readonly createScopedExecutor: (options: LocalExecutorOptions) => Promise<ScopedExecutorHandle>;
94115
}
95116

96117
class LocalExecutorTag extends Context.Service<LocalExecutorTag, LocalExecutorBundle>()(
@@ -137,6 +158,31 @@ const handleOrNull = (promise: ReturnType<typeof createExecutorHandle>) =>
137158
),
138159
);
139160

161+
const closeExecutorOnly = (executor: Executor<LocalPlugins>) => (): Promise<void> =>
162+
Effect.runPromise(Effect.ignore(executor.close()));
163+
164+
/**
165+
* Builds a bundle's `createScopedExecutor` from the seam that makes an executor
166+
* over its already-open database. Lives at module scope so the deferred
167+
* `Effect.runPromise` is not nested inside the layer's own Effect.
168+
*/
169+
const makeScopedExecutorFactory =
170+
<E>(makeExecutor: (plugins: LocalPlugins) => Effect.Effect<Executor<LocalPlugins>, E>) =>
171+
(scopedOptions: LocalExecutorOptions): Promise<ScopedExecutorHandle> =>
172+
Effect.runPromise(
173+
Effect.gen(function* () {
174+
const scoped = yield* loadLocalPlugins(scopedOptions);
175+
const scopedExecutor = yield* makeExecutor(scoped.plugins);
176+
return {
177+
executor: scopedExecutor,
178+
plugins: scoped.plugins,
179+
// Closes this executor's plugins only. The database handle and the
180+
// data-dir ownership lock belong to the bundle that derived this.
181+
dispose: closeExecutorOnly(scopedExecutor),
182+
};
183+
}),
184+
);
185+
140186
const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => {
141187
const storage = resolveStorage();
142188

@@ -186,23 +232,36 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => {
186232
const webBaseUrl =
187233
process.env.EXECUTOR_WEB_BASE_URL ?? `http://localhost:${process.env.PORT ?? "4788"}`;
188234

189-
const executor = yield* createExecutor({
190-
tenant: Tenant.make(tenantId),
191-
subject: Subject.make(LOCAL_SUBJECT),
192-
db: sqlite.db,
193-
plugins,
194-
onElicitation: "accept-all",
195-
oauthEndpointUrlPolicy: { allowHttp: true },
196-
// EXPLICIT OAuth callback — the daemon serves the v2 `/api/oauth/callback`
197-
// route on the same origin as the web UI. Derived from `webBaseUrl`
198-
// (loopback localhost is correct + intended for the local CLI, but it
199-
// is wired explicitly here rather than relying on a hidden default).
200-
redirectUri: new URL("/api/oauth/callback", webBaseUrl).toString(),
201-
// Built-in agent-facing tools (integrations / connections / policies).
202-
coreTools: {
203-
webBaseUrl,
204-
},
205-
});
235+
// Only `plugins` varies between the boot executor and a scoped one: the
236+
// data dir, tenant, and subject are fixed for the process. `makeExecutor`
237+
// is that seam, and it closes over the open `sqlite.db` so deriving an
238+
// executor never re-opens (and so never re-locks) the data dir.
239+
//
240+
// `db` is the FumaDB handle, NOT the owning `sqlite` wrapper: an executor
241+
// built over a `{ db, close }` wrapper would close the SHARED database on
242+
// its own close(). Passing the handle keeps disposal to plugins alone.
243+
const makeExecutor = (executorPlugins: LocalPlugins) =>
244+
createExecutor({
245+
tenant: Tenant.make(tenantId),
246+
subject: Subject.make(LOCAL_SUBJECT),
247+
db: sqlite.db,
248+
plugins: executorPlugins,
249+
onElicitation: "accept-all",
250+
oauthEndpointUrlPolicy: { allowHttp: true },
251+
// EXPLICIT OAuth callback — the daemon serves the v2 `/api/oauth/callback`
252+
// route on the same origin as the web UI. Derived from `webBaseUrl`
253+
// (loopback localhost is correct + intended for the local CLI, but it
254+
// is wired explicitly here rather than relying on a hidden default).
255+
redirectUri: new URL("/api/oauth/callback", webBaseUrl).toString(),
256+
// Built-in agent-facing tools (integrations / connections / policies).
257+
coreTools: {
258+
webBaseUrl,
259+
},
260+
});
261+
262+
const executor = yield* makeExecutor(plugins);
263+
264+
const createScopedExecutor = makeScopedExecutorFactory(makeExecutor);
206265

207266
if (migration.migrated) {
208267
console.warn(
@@ -233,7 +292,7 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => {
233292
);
234293
}
235294

236-
return { executor, plugins };
295+
return { executor, plugins, createScopedExecutor };
237296
}),
238297
);
239298
};
@@ -246,6 +305,7 @@ export const createExecutorHandle = async (options: LocalExecutorOptions = {}) =
246305
return {
247306
executor: bundle.executor,
248307
plugins: bundle.plugins,
308+
createScopedExecutor: bundle.createScopedExecutor,
249309
dispose: async () => {
250310
await Effect.runPromise(Effect.ignore(bundle.executor.close()));
251311
await ignorePromiseFailure("disposeRuntime", () => runtime.dispose());

apps/local/src/main.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { Context, Data, Effect, Layer, ManagedRuntime } from "effect";
33
import { createExecutionEngine } from "@executor-js/execution";
44
import { makeQuickJsExecutor } from "@executor-js/runtime-quickjs";
55
import { makeLocalApiHandler } from "./app";
6-
import { createExecutorHandle, disposeExecutor, getExecutorBundle } from "./executor";
6+
import { disposeExecutor, getExecutorBundle } from "./executor";
77
import { createMcpRequestHandler, type McpRequestHandler } from "./mcp";
88

99
// ---------------------------------------------------------------------------
@@ -81,25 +81,29 @@ export const createServerHandlers = async (token: string): Promise<ServerHandler
8181
// engine instance (the browser-approval + stdio surface is local-only and not
8282
// part of the shared API). Reuse the shared boot bundle so the MCP executor is
8383
// byte-identical to the one the API serves.
84-
const { executor } = await getExecutorBundle();
84+
const bundle = await getExecutorBundle();
8585
const engine = createExecutionEngine({
86-
executor,
86+
executor: bundle.executor,
8787
codeExecutor: makeQuickJsExecutor(),
8888
});
8989
mcp = createMcpRequestHandler({
9090
defaultConfig: { engine },
9191
createConfigForResource: async (resource) => {
9292
if (resource.kind === "default") return { config: { engine } };
93-
const handle = await createExecutorHandle({
93+
// A toolkit resource differs from the default only in its plugin scope,
94+
// so it derives from the boot bundle's ALREADY-OPEN database. Opening a
95+
// second owned database here would deadlock against the data-dir
96+
// ownership lock this very process holds, failing every request.
97+
const scoped = await bundle.createScopedExecutor({
9498
activeToolkitSlug: resource.slug,
9599
});
96100
const toolkitEngine = createExecutionEngine({
97-
executor: handle.executor,
101+
executor: scoped.executor,
98102
codeExecutor: makeQuickJsExecutor(),
99103
});
100104
return {
101105
config: { engine: toolkitEngine },
102-
close: handle.dispose,
106+
close: scoped.dispose,
103107
};
104108
},
105109
});

0 commit comments

Comments
 (0)