mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-28 14:59:47 +00:00
* fix(serve): clean up stdio MCP server on client disconnect
The PGLite write lock leaked indefinitely when the parent of `gbrain serve`
disconnected. Three root causes: serve.ts never called engine.disconnect()
after startMcpServer() resolved; cli.ts short-circuited with a "serve doesn't
disconnect" comment; and the MCP SDK's StdioServerTransport only listens for
'data'/'error' on stdin, never 'end'/'close', so even a clean stdin EOF never
reached the SDK.
Net effect: the next `gbrain serve` waited for the in-process 5-minute stale-
lock check or hung indefinitely.
stdio path now installs a unified lifecycle:
- SIGTERM/SIGINT/SIGHUP all funnel into one idempotent shutdown path
(SIGHUP coverage matters for Claude Desktop on macOS / MCP gateway
restarts; SIGINT for Ctrl-C; SIGTERM for daemon shutdown).
- stdin 'end' (clean EOF) and 'close' (parent SIGKILL with pipe still
open) both trigger the same graceful path. TTY stdin skips the watchers
so interactive `gbrain serve` is unaffected.
- Parent-process watchdog polls the live kernel parent PID via spawnSync
('ps','-o','ppid=','-p',PID) every 5s. process.ppid is cached at process
creation by Bun (and Node) and never refreshes on re-parent — empirical
evidence on macOS shows ps reports the new parent within one tick while
process.ppid stays at the original PID indefinitely (oven-sh/bun#30305).
- Watchdog fires on `getParentPid() !== initialParentPid` (any reparent),
not just `=== 1`. Catches launchd / systemd / tmux / parent-shell-with-
PR_SET_CHILD_SUBREAPER cases where the kernel re-anchors us to a non-1
subreaper PID. Codex review caught the original `=== 1` was incomplete.
- One-shot startup probe verifies `spawnSync('ps')` actually works on this
host. If the probe fails (stripped containers / busybox without procps),
we skip installing the watchdog interval entirely AND emit a loud stderr
line — the operator sees "watchdog disabled" instead of an installed-
but-never-fires phantom that silently falls back to cached process.ppid.
- 5-second cleanup deadline: if engine.disconnect() wedges (PGLite WASM
stall, etc.), the process still calls process.exit(0). The abandoned
lock dir is reclaimed on the next start by the existing stale-lock
check in pglite-lock.ts.
- Optional `--stdio-idle-timeout <sec>`: default OFF safety net for
parents that leak the pipe but never close it. Strict parsing rejects
`abc` / `30junk` / `-1` / `1.5` / blank values explicitly so a typo
doesn't silently disable the safety net (closes #446).
Test seam: ServeOptions { stdin, signals, exit, log, startMcpServer,
getParentPid, setInterval, clearInterval, probeWatchdog } lets the
lifecycle be unit-tested deterministically without spawning a real Bun
child or booting the MCP SDK.
22 test cases covering signals, stdin EOF, TTY skip, watchdog reparent
(both PID-1 and subreaper-PID-N cases), ps-unavailable degraded mode,
idle timeout, idempotent shutdown, and cleanup-deadline behavior.
Closes #413, #446. Supersedes #591.
Co-Authored-By: Aragorn2046 <noreply@github.com>
Co-Authored-By: seungsu-kr <noreply@github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): route HTTP auth/admin SQL through active engine
`gbrain auth` and `gbrain serve --http` previously routed every SQL
through the postgres.js singleton in src/core/db.ts, which silently fell
back to a file-backed PGLite when DATABASE_URL was set but the config
file disagreed. The HTTP transport's verbatim use of the singleton also
made `gbrain serve --http` Postgres-only, even though the
`access_tokens` and `mcp_request_log` tables exist in both engine
schemas.
Auth, OAuth, admin, file uploads, and HTTP-transport SQL now run through
`engine.executeRaw` via a deliberately narrow tagged-template adapter
(`src/core/sql-query.ts`). The contract is scalar-binds-only — adding
JSONB or fragment composition would invite the adapter to drift into a
partial postgres.js clone. JSONB writes use a separate
`executeRawJsonb(engine, sql, scalarParams, jsonbParams)` helper that
composes positional `$N::jsonb` casts and passes objects through
`engine.executeRaw`. The CI guard at `scripts/check-jsonb-pattern.sh`
doesn't fire because the helper is a method call, not the banned
`${JSON.stringify(x)}::jsonb` template-literal interpolation, and the
v0.12.0 double-encode bug class doesn't apply to positional binding via
`postgres.js`'s `unsafe()` (verified by
`test/e2e/auth-permissions.test.ts:67` on Postgres and the new
`test/sql-query.test.ts` on PGLite).
Migrated call sites:
- src/commands/auth.ts: takes-holders writes (lines 52, 86) →
executeRawJsonb. List, revoke, register-client, revoke-client →
SqlQuery via withConfiguredSql() helper that opens an engine, runs
the callback, disconnects.
- src/commands/serve-http.ts: ~25 call sites including the four
mcp_request_log.params INSERTs (now write real JSONB objects, not
JSON-encoded strings — the read side `params->>'op'` returns the
operation name, closing CLAUDE.md's outstanding "JSON-string-into-
JSONB" note as a side effect). The /admin/api/requests dynamic
filter pattern (postgres.js fragment composition) is rewritten as
parametrized SQL string + params array.
- src/mcp/http-transport.ts: legacy bearer-auth path. The
Postgres-only fail-fast at startup is removed because both schemas
now carry access_tokens + mcp_request_log.
- src/core/oauth-provider.ts: SqlQuery / SqlValue types relocated
from here to sql-query.ts as the canonical home (Codex finding #8).
- src/commands/files.ts: all 5 db.getConnection() sites (lines 104,
139, 252, 326, 355). The line-256 INSERT into files.metadata uses
executeRawJsonb; the other four are scalar-only SqlQuery (Codex
finding #6 — scope was bigger than the plan's "lone INSERT" framing).
- src/core/config.ts: env-var DATABASE_URL inference. When dbUrl is
set, infer Postgres engine and clear the stale database_path.
Engine-internal sql.json() sites in src/core/postgres-engine.ts (5
sites: lines 520, 1689, 1728, 1790, 2313) STAY UNCHANGED. They live
inside PostgresEngine itself, where the postgres.js template-tag
sql.json() pattern is correct — those methods are only loaded when
Postgres is the active engine, so there's no PGLite-routing concern.
Migration v45 (mcp_request_log_params_jsonb_normalize): one-shot UPDATE
that lifts pre-v0.31 string-shaped JSONB rows to objects so the
/admin/api/requests endpoint at serve-http.ts:605 returns one
consistent shape to the admin SPA. Idempotent (subsequent runs find no
rows where jsonb_typeof = 'string'). Closes the mixed-shape window
that would otherwise have made post-deploy admin reads break.
Tests:
- test/sql-query.test.ts: 7 cases covering scalar binds, the
.json() rejection (defense in depth — SqlQuery is scalar-only),
JSONB round-trip with `jsonb_typeof = 'object'` and `->>`
semantics, the v0.12.0 double-encode regression guard, null
JSONB handling, and the scalars-then-jsonb call shape.
- test/config-env.test.ts: migrated from PR's manual `restoreEnv()`
in afterEach to the canonical `withEnv()` helper at
test/helpers/with-env.ts (CLAUDE.md R1 / codex finding D3).
Five cases covering DATABASE_URL precedence, GBRAIN_DATABASE_URL
operator override, file-only config, env-only config, and the
no-config null path.
- test/e2e/auth-takes-holders-pglite.test.ts: 6 cases against
in-memory PGLite (no DATABASE_URL gate). Covers create / update /
read of access_tokens.permissions, mcp_request_log.params object
+ null writes, and the migration v45 normalizer (seed
string-shaped row, run UPDATE, assert object shape; second-run
no-op for idempotency).
- test/http-transport.test.ts: mock updated to intercept
engine.executeRaw (the new code path) instead of the postgres.js
template tag. 24 cases pass.
Plan reference: ~/.claude/plans/system-instruction-you-are-working-peppy-moore.md.
Codex outside-voice review applied: D-codex-1, D-codex-2, D-codex-5,
D-codex-8, D-codex-9, D-codex-10 (and D1, D5 reversed by codex).
Closes the architectural intent of #681. Supersedes its branch.
Co-Authored-By: codex-bot <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: update CLAUDE.md key files for v0.31.3
Annotate the v0.31.3 changes in the canonical Key Files section:
new src/core/sql-query.ts adapter (#681), src/commands/serve.ts stdio
cleanup (#676), v0.31.3 amendments to auth.ts / serve-http.ts /
oauth-provider.ts surfaces, and migration v46 normalizer in migrate.ts.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* chore: regenerate llms-full.txt for v0.31.3 docs sync
CI's build-llms test asserts the committed llms.txt + llms-full.txt
match what scripts/build-llms.ts produces from current source state.
CLAUDE.md was amended by /document-release post-merge (new entries for
src/core/sql-query.ts and src/commands/serve.ts; amended notes on
auth.ts / serve-http.ts / migrate.ts), so the inlined-bundle fell out
of sync. Regenerated via `bun run build:llms`.
llms.txt unchanged (curated index — no new web URLs added).
llms-full.txt updated to inline the new CLAUDE.md content.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Aragorn2046 <noreply@github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
176 lines
7.2 KiB
TypeScript
176 lines
7.2 KiB
TypeScript
import { afterAll, beforeAll, describe, expect, test } from 'bun:test';
|
|
import { PGLiteEngine } from '../src/core/pglite-engine.ts';
|
|
import { sqlQueryForEngine, executeRawJsonb } from '../src/core/sql-query.ts';
|
|
|
|
let engine: PGLiteEngine;
|
|
|
|
beforeAll(async () => {
|
|
engine = new PGLiteEngine();
|
|
await engine.connect({});
|
|
}, 30_000);
|
|
|
|
afterAll(async () => {
|
|
if (engine) await engine.disconnect();
|
|
});
|
|
|
|
describe('sqlQueryForEngine', () => {
|
|
test('runs parameterized tagged-template SQL against PGLite', async () => {
|
|
const sql = sqlQueryForEngine(engine);
|
|
const rows = await sql`SELECT ${'pglite'}::text AS engine, ${3}::int AS count`;
|
|
expect(rows).toEqual([{ engine: 'pglite', count: 3 }]);
|
|
});
|
|
|
|
test('rejects postgres.js-style fragment / object values explicitly', async () => {
|
|
const sql = sqlQueryForEngine(engine);
|
|
await expect(
|
|
sql`SELECT ${(Promise.resolve([]) as any)}::text AS bad`
|
|
).rejects.toThrow(/only supports scalar bind values/);
|
|
await expect(
|
|
sql`SELECT ${(['read', 'write'] as any)}::text[] AS bad`
|
|
).rejects.toThrow(/only supports scalar bind values/);
|
|
await expect(
|
|
sql`SELECT ${({ takes_holders: ['world'] } as any)}::jsonb AS bad`
|
|
).rejects.toThrow(/only supports scalar bind values/);
|
|
});
|
|
});
|
|
|
|
describe('executeRawJsonb (D1 wave / v0.31)', () => {
|
|
test('round-trips an object as JSONB on PGLite (jsonb_typeof = object, ->> reads value)', async () => {
|
|
// Verifies the cross-engine JSONB write helper produces a real Postgres
|
|
// JSONB object — not a quoted JSON string. Codex's plan-review #9 said
|
|
// "use the actual JSONB contract, not string-grep for backslash-quote",
|
|
// and that's what this asserts: jsonb_typeof + ->>.
|
|
const tableName = `t_jsonb_${Math.random().toString(36).slice(2, 10)}`;
|
|
await engine.executeRaw(`CREATE TEMP TABLE ${tableName} (j jsonb)`);
|
|
try {
|
|
await executeRawJsonb(
|
|
engine,
|
|
`INSERT INTO ${tableName} (j) VALUES ($1::jsonb)`,
|
|
[],
|
|
[{ k: 'v', n: 42 }],
|
|
);
|
|
const rows = await engine.executeRaw<{ kind: string; k: string; n: number }>(
|
|
`SELECT jsonb_typeof(j) AS kind, j->>'k' AS k, (j->>'n')::int AS n FROM ${tableName}`,
|
|
);
|
|
expect(rows).toHaveLength(1);
|
|
expect(rows[0].kind).toBe('object');
|
|
expect(rows[0].k).toBe('v');
|
|
expect(rows[0].n).toBe(42);
|
|
} finally {
|
|
await engine.executeRaw(`DROP TABLE ${tableName}`);
|
|
}
|
|
});
|
|
|
|
test('takes-holders shape: object preserved, ->> returns the encoded array, NOT a double-encoded string (v0.12.0 regression guard)', async () => {
|
|
// The v0.12.0 silent-data-loss bug stored `${JSON.stringify(perms)}::jsonb`
|
|
// as a JSON string-of-an-object, so `permissions->>'takes_holders'`
|
|
// would return a string with backslashes instead of the array.
|
|
// Post-fix: real JSONB object, ->> on the array key returns the
|
|
// pretty-printed JSON of the array (because jsonb -> array -> ->> is
|
|
// the array as a text), and jsonb_typeof on the parent stays 'object'.
|
|
const tableName = `t_perms_${Math.random().toString(36).slice(2, 10)}`;
|
|
await engine.executeRaw(
|
|
`CREATE TEMP TABLE ${tableName} (id serial PRIMARY KEY, permissions jsonb)`,
|
|
);
|
|
try {
|
|
const perms = { takes_holders: ['world', 'garry'] };
|
|
await executeRawJsonb(
|
|
engine,
|
|
`INSERT INTO ${tableName} (permissions) VALUES ($1::jsonb)`,
|
|
[],
|
|
[perms],
|
|
);
|
|
const rows = await engine.executeRaw<{
|
|
outer_kind: string;
|
|
holders_kind: string;
|
|
first_holder: string;
|
|
text_form: string;
|
|
}>(
|
|
`SELECT
|
|
jsonb_typeof(permissions) AS outer_kind,
|
|
jsonb_typeof(permissions->'takes_holders') AS holders_kind,
|
|
permissions->'takes_holders'->>0 AS first_holder,
|
|
permissions::text AS text_form
|
|
FROM ${tableName}`,
|
|
);
|
|
expect(rows).toHaveLength(1);
|
|
// The parent JSONB is an object; the takes_holders child is an array.
|
|
// Pre-fix this would be 'string' / 'string' (string-of-object).
|
|
expect(rows[0].outer_kind).toBe('object');
|
|
expect(rows[0].holders_kind).toBe('array');
|
|
expect(rows[0].first_holder).toBe('world');
|
|
// Defense in depth: the text representation must NOT contain
|
|
// backslash-quote sequences, which is what double-encoded JSONB
|
|
// looked like in the v0.12.0 incident (e.g. `"{\"takes_holders\":...}"`).
|
|
expect(rows[0].text_form).not.toContain('\\"');
|
|
// And the text representation should look like a normal JSON object
|
|
// — starts with `{`, not `"{`.
|
|
expect(rows[0].text_form.startsWith('{')).toBe(true);
|
|
} finally {
|
|
await engine.executeRaw(`DROP TABLE ${tableName}`);
|
|
}
|
|
});
|
|
|
|
test('null jsonb value stores NULL, not the string "null"', async () => {
|
|
// serve-http.ts sometimes inserts NULL params (e.g. tools/list, scope-
|
|
// rejected paths). The helper must accept null without trying to
|
|
// encode it as the string "null" or rejecting it.
|
|
const tableName = `t_jnull_${Math.random().toString(36).slice(2, 10)}`;
|
|
await engine.executeRaw(`CREATE TEMP TABLE ${tableName} (j jsonb)`);
|
|
try {
|
|
await executeRawJsonb(
|
|
engine,
|
|
`INSERT INTO ${tableName} (j) VALUES ($1::jsonb)`,
|
|
[],
|
|
[null],
|
|
);
|
|
const rows = await engine.executeRaw<{ kind: string | null; is_null: boolean }>(
|
|
`SELECT jsonb_typeof(j) AS kind, (j IS NULL) AS is_null FROM ${tableName}`,
|
|
);
|
|
expect(rows[0].is_null).toBe(true);
|
|
// jsonb_typeof on SQL NULL returns NULL.
|
|
expect(rows[0].kind).toBeNull();
|
|
} finally {
|
|
await engine.executeRaw(`DROP TABLE ${tableName}`);
|
|
}
|
|
});
|
|
|
|
test('mixes scalar params and jsonb params in positional order', async () => {
|
|
// Real call shape: scalars first ($1..$N), JSONB params next
|
|
// ($N+1..$N+M). Mirrors the auth.ts `INSERT INTO access_tokens
|
|
// (name, token_hash, permissions) VALUES ($1, $2, $3::jsonb)` pattern.
|
|
const tableName = `t_mix_${Math.random().toString(36).slice(2, 10)}`;
|
|
await engine.executeRaw(
|
|
`CREATE TEMP TABLE ${tableName} (name text, weight int, payload jsonb)`,
|
|
);
|
|
try {
|
|
await executeRawJsonb(
|
|
engine,
|
|
`INSERT INTO ${tableName} (name, weight, payload) VALUES ($1, $2, $3::jsonb)`,
|
|
['alice', 7],
|
|
[{ tags: ['a', 'b'] }],
|
|
);
|
|
const rows = await engine.executeRaw<{ name: string; weight: number; first_tag: string }>(
|
|
`SELECT name, weight, payload->'tags'->>0 AS first_tag FROM ${tableName}`,
|
|
);
|
|
expect(rows).toEqual([{ name: 'alice', weight: 7, first_tag: 'a' }]);
|
|
} finally {
|
|
await engine.executeRaw(`DROP TABLE ${tableName}`);
|
|
}
|
|
});
|
|
|
|
test('rejects non-scalar values in scalarParams (defense in depth)', async () => {
|
|
// The scalar position validator should fire even when a misuse passes
|
|
// an object via scalarParams instead of jsonbParams. Catches the
|
|
// cross-up-the-positions footgun loud at the helper boundary.
|
|
await expect(
|
|
executeRawJsonb(
|
|
engine,
|
|
`SELECT $1::text AS bad`,
|
|
[{ object: 'in scalar position' } as any],
|
|
[],
|
|
),
|
|
).rejects.toThrow(/only supports scalar bind values/);
|
|
});
|
|
});
|