History

Fix the local e2e suite: toolkit MCP DB lock, Bun-only spawn, stale auth selector

Rhys Sullivan committed 3 months ago

Commit 41e3ecd1 parent f674fb84 files · +90 −65Browse files

Unified · Split

4 files
  1. apps/local/src/executor.ts +37 −18
  2. apps/local/src/main.ts +5 −0
  3. e2e/local/auth.test.ts +5 −3
  4. e2e/local/cli-mcp-daemon-attach-stress.test.ts +43 −44
apps/local/src/executor.ts+37−18
@@ -19,7 +19,7 @@ import type { McpPluginExtension } from "@executor-js/plugin-mcp"; · 18 unmodified lines
1919 import executorConfig from "../executor.config";
2020 import { localAnalytics } from "./analytics";
2121 import { localDataMigrations } from "./db/data-migrations";
22−import { openOwnedLocalDatabase } from "./db/owned-database";
22+import { openOwnedLocalDatabase, type OwnedLocalDatabase } from "./db/owned-database";
2323
2424 interface ResolvedStorage {
2525 readonly dataDir: string;
@@ -56,6 +56,16 @@ type LocalPlugins = readonly AnyPlugin[]; · 30 unmodified lines
5656
5757 export interface LocalExecutorOptions {
5858 readonly activeToolkitSlug?: string;
59+ /**
60+ * Reuse an already-open owned database instead of opening (and locking) the
61+ * data dir again. A toolkit-scoped MCP session differs from the default one
62+ * only in its plugin set, so it must ride the running server's DB handle:
63+ * `openOwnedLocalDatabase` takes an EXCLUSIVE lock, and a second open from
64+ * inside the same process contends with the lock this process already holds.
65+ * The borrowed handle is NOT closed when the derived executor disposes —
66+ * whoever opened it still owns its lifetime.
67+ */
68+ readonly borrowedDb?: OwnedLocalDatabase;
5969 }
6070
6171 const loadLocalPlugins = (options: LocalExecutorOptions = {}) =>
@@ -92,6 +102,10 @@ const loadLocalPlugins = (options: LocalExecutorOptions = {}) => · 30 unmodified lines
92102 interface LocalExecutorBundle {
93103 readonly executor: Executor<LocalPlugins>;
94104 readonly plugins: LocalPlugins;
105+ /** The owned DB this bundle opened (or borrowed). Surfaced so a
106+ * toolkit-scoped executor can ride the SAME handle instead of contending
107+ * with this process's own exclusive data-dir lock. */
108+ readonly db: OwnedLocalDatabase;
95109 /** Where this daemon's web UI is reachable, resolved once at boot. Surfaced
96110 * so callers building user-facing links (MCP artifact deep links) use the
97111 * same origin the executor itself was configured with. */
@@ -151,23 +165,27 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => { · 53 unmodified lines
151165 const tenantId = makeTenantId(cwd);
152166 const tables = collectTables();
153167
154− const owned = yield* Effect.acquireRelease(
155− Effect.tryPromise({
156− try: () =>
157− openOwnedLocalDatabase({
158− dataDir: storage.dataDir,
159− tables,
160− namespace: localNamespace,
161− tenantId,
168+ // A borrowed handle is owned by its opener, so it is used as-is and left
169+ // open on release; only a handle opened here is closed here.
170+ const owned = options.borrowedDb
171+ ? options.borrowedDb
172+ : yield* Effect.acquireRelease(
173+ Effect.tryPromise({
174+ try: () =>
175+ openOwnedLocalDatabase({
176+ dataDir: storage.dataDir,
177+ tables,
178+ namespace: localNamespace,
179+ tenantId,
180+ }),
181+ catch: (cause) =>
182+ new LocalExecutorCreateError({
183+ message: CREATE_SQLITE_ERROR_MESSAGE,
184+ cause,
185+ }),
162186 }),
163− catch: (cause) =>
164− new LocalExecutorCreateError({
165− message: CREATE_SQLITE_ERROR_MESSAGE,
166− cause,
167− }),
168− }),
169− (database) => Effect.promise(() => database.close()).pipe(Effect.ignore),
170− );
187+ (database) => Effect.promise(() => database.close()).pipe(Effect.ignore),
188+ );
171189 const sqlite = owned.db;
172190 const migration = owned.migration;
173191
@@ -243,7 +261,7 @@ const createLocalExecutorLayer = (options: LocalExecutorOptions = {}) => { · 69 unmodified lines
243261 );
244262 }
245263
246− return { executor, plugins, webBaseUrl };
264+ return { executor, plugins, webBaseUrl, db: owned };
247265 }),
248266 );
249267 };
@@ -257,6 +275,7 @@ export const createExecutorHandle = async (options: LocalExecutorOptions = {}) = · 7 unmodified lines
257275 executor: bundle.executor,
258276 plugins: bundle.plugins,
259277 webBaseUrl: bundle.webBaseUrl,
278+ db: bundle.db,
260279 dispose: async () => {
261280 await Effect.runPromise(Effect.ignore(bundle.executor.close()));
262281 await ignorePromiseFailure("disposeRuntime", () => runtime.dispose());
apps/local/src/main.ts+5−0
@@ -136,8 +136,13 @@ export const createServerHandlers = async (token: string): Promise<ServerHandler · 135 unmodified lines
136136 },
137137 };
138138 }
139+ // Borrow the running server's DB handle: this process already holds the
140+ // data dir's exclusive ownership lock, so opening it a second time here
141+ // fails against ourselves. The toolkit executor differs only in its
142+ // plugin set, and the borrowed handle stays open when it disposes.
139143 const handle = await createExecutorHandle({
140144 activeToolkitSlug: resource.slug,
145+ borrowedDb: (await getExecutorBundle()).db,
141146 });
142147 const toolkitEngine = withExecutionAnalytics(
143148 createExecutionEngine({
e2e/local/auth.test.ts+5−3
@@ -36,8 +36,10 @@ scenario( · 35 unmodified lines
3636 await page.goto(url, { waitUntil: "domcontentloaded" });
3737 await page.getByRole("link", { name: "Secrets" }).first().waitFor({ timeout: 30_000 });
3838 // Integrations actually LOAD (the built-in Executor integration) — proves
39− // auth + data, not just the static shell.
40− await page.getByText("built-in").first().waitFor({ timeout: 30_000 });
39+ // auth + data, not just the static shell. Matched on the row's stable
40+ // testid: the list renders each integration's name + slug, never the
41+ // literal "built-in" (that string is only an internal `kind`).
42+ await page.getByTestId("integration-entry-executor").first().waitFor({ timeout: 30_000 });
4143 // The token is moved out of the URL and persisted to localStorage.
4244 expect(new URL(page.url()).searchParams.has("_token")).toBe(false);
4345 const stored = await page.evaluate(() => localStorage.getItem("executor.authToken"));
@@ -70,7 +72,7 @@ scenario( · 26 unmodified lines
7072 await page.getByRole("button", { name: "Connect" }).click();
7173 await page.getByRole("link", { name: "Secrets" }).first().waitFor({ timeout: 30_000 });
7274 // The reconnect fully restores — integrations LOAD, not a stale 401.
73− await page.getByText("built-in").first().waitFor({ timeout: 30_000 });
75+ await page.getByTestId("integration-entry-executor").first().waitFor({ timeout: 30_000 });
7476 });
7577 }),
7678 );
e2e/local/cli-mcp-daemon-attach-stress.test.ts+43−44
@@ -21,11 +21,11 @@ import { expect } from "@effect/vitest"; · 20 unmodified lines
2121 import { Client } from "@modelcontextprotocol/sdk/client/index.js";
2222 import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js";
2323 import { Effect } from "effect";
24+import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process";
2425 import { mkdtempSync, readdirSync, rmSync } from "node:fs";
2526 import { tmpdir } from "node:os";
2627 import { join } from "node:path";
2728 import { fileURLToPath } from "node:url";
28−import type { Subprocess } from "bun";
2929
3030 import { scenario } from "../src/scenario";
3131
@@ -34,7 +34,10 @@ const testScope = join(repoRoot, "apps/local"); · 2 unmodified lines
3434 // Generous: a dev-mode daemon boots a Vite dev server, slow under machine load.
3535 const readyTimeoutMs = 150_000;
3636
37−type DaemonProc = Subprocess<"ignore", "pipe", "pipe">;
37+// vitest runs this suite under NODE, not bun, so the daemon is spawned with
38+// node:child_process (the rest of the e2e harness does the same). `Bun.spawn`
39+// here threw `ReferenceError: Bun is not defined` on every run.
40+type DaemonProc = ChildProcessWithoutNullStreams;
3841
3942 const waitForDaemonReady = (
4043 proc: DaemonProc,
@@ -44,50 +47,38 @@ const waitForDaemonReady = ( · 3 unmodified lines
4447 let stdoutBuffer = "";
4548 let stderrBuffer = "";
4649 let settled = false;
47− const decoder = new TextDecoder();
48− const stdout = proc.stdout.getReader();
49− const stderr = proc.stderr.getReader();
5050 const deadline = setTimeout(() => {
5151 if (settled) return;
5252 settled = true;
5353 // oxlint-disable-next-line executor/no-promise-reject, executor/no-error-constructor -- boundary: captured daemon stderr
5454 rejectReady(new Error(`daemon did not announce ready: ${stderrBuffer}`));
5555 }, readyTimeoutMs);
56− void (async () => {
57− while (true) {
58− const { value, done } = await stderr.read();
59− if (done) return;
60− stderrBuffer += decoder.decode(value);
61− }
62− })();
63− void (async () => {
64− while (true) {
65− const { value, done } = await stdout.read();
66− if (done) {
67− if (!settled) {
68− settled = true;
69− clearTimeout(deadline);
70− // oxlint-disable-next-line executor/no-promise-reject, executor/no-error-constructor -- boundary: captured daemon stderr
71− rejectReady(new Error(`daemon stdout closed before ready: ${stderrBuffer}`));
72− }
73− return;
74− }
75− stdoutBuffer += decoder.decode(value);
76− const match = /Daemon ready on http:\/\/(?:\[[^\]]+\]|[^:\s]+):(\d+)/.exec(stdoutBuffer);
77− if (match) {
78− settled = true;
79− clearTimeout(deadline);
80− resolveReady({ port: Number(match[1]), stderr: () => stderrBuffer });
81− return;
82− }
56+ proc.stderr.on("data", (chunk: Buffer) => {
57+ stderrBuffer += chunk.toString();
58+ });
59+ proc.stdout.on("data", (chunk: Buffer) => {
60+ if (settled) return;
61+ stdoutBuffer += chunk.toString();
62+ const match = /Daemon ready on http:\/\/(?:\[[^\]]+\]|[^:\s]+):(\d+)/.exec(stdoutBuffer);
63+ if (match) {
64+ settled = true;
65+ clearTimeout(deadline);
66+ resolveReady({ port: Number(match[1]), stderr: () => stderrBuffer });
8367 }
84− })();
68+ });
69+ proc.stdout.on("close", () => {
70+ if (settled) return;
71+ settled = true;
72+ clearTimeout(deadline);
73+ // oxlint-disable-next-line executor/no-promise-reject, executor/no-error-constructor -- boundary: captured daemon stderr
74+ rejectReady(new Error(`daemon stdout closed before ready: ${stderrBuffer}`));
75+ });
8576 });
8677
8778 const spawnDaemon = (dataDir: string): DaemonProc =>
88− Bun.spawn(
79+ spawn(
80+ "bun",
8981 [
90− "bun",
9182 "run",
9283 "dev:cli",
9384 "daemon",
@@ -103,17 +94,20 @@ const spawnDaemon = (dataDir: string): DaemonProc => · 9 unmodified lines
10394 {
10495 cwd: repoRoot,
10596 env: { ...process.env, EXECUTOR_DATA_DIR: dataDir },
106− stdin: "ignore",
107− stdout: "pipe",
108− stderr: "pipe",
97+ stdio: ["ignore", "pipe", "pipe"],
10998 },
110− );
99+ ) as DaemonProc;
100+
101+const exited = (proc: DaemonProc): Promise<void> =>
102+ proc.exitCode !== null || proc.signalCode !== null
103+ ? Promise.resolve()
104+ : new Promise((resolve) => proc.once("exit", () => resolve()));
111105
112106 const stopProc = async (proc: DaemonProc): Promise<void> => {
113− if (proc.exitCode !== null) return;
107+ if (proc.exitCode !== null || proc.signalCode !== null) return;
114108 proc.kill("SIGTERM");
115− await Promise.race([proc.exited, Bun.sleep(3000)]);
116− if (proc.exitCode === null) proc.kill("SIGKILL");
109+ await Promise.race([exited(proc), new Promise<void>((resolve) => setTimeout(resolve, 3000))]);
110+ if (proc.exitCode === null && proc.signalCode === null) proc.kill("SIGKILL");
117111 };
118112
119113 const startForegroundDaemon = (dataDir: string) =>
@@ -322,7 +316,12 @@ scenario( · 202 unmodified lines
322316 );
323317
324318 daemon.proc.kill("SIGKILL");
325− yield* Effect.promise(() => Promise.race([daemon.proc.exited, Bun.sleep(3000)]));
319+ yield* Effect.promise(() =>
320+ Promise.race([
321+ exited(daemon.proc),
322+ new Promise<void>((resolve) => setTimeout(resolve, 3000)),
323+ ]),
324+ );
326325
327326 // The next call must settle (reject) quickly — a 10s bound well under the
328327 // scenario timeout catches a hang.
@@ -332,7 +331,7 @@ scenario( · 3 unmodified lines
332331 .callTool({ name: "execute", arguments: { code: "return 3" } })
333332 .then(() => "resolved" as const)
334333 .catch(() => "rejected" as const),
335− Bun.sleep(10_000).then(() => "timeout" as const),
334+ new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 10_000)),
336335 ]),
337336 );
338337 // eslint-disable-next-line no-console

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t