v0.28.10 fix: lightweight /health endpoint — SELECT 1 instead of getStats() (#701)
* fix: lightweight /health endpoint — SELECT 1 instead of getStats() On large brains (96K+ pages), getStats() runs 6× count(*) queries that routinely exceed the 3s HEALTH_TIMEOUT_MS through PgBouncer. This produces false 503s that cause external health monitors (cron, Fly.io, k8s) to restart otherwise-healthy servers — which in turn creates advisory lock pile-ups when multiple serve instances compete for the migration lock. Changes: - /health now runs `SELECT 1` for liveness (sub-millisecond) - ?full=true opt-in preserves the old getStats() behavior - /admin/api/health-indicators still returns full stats - probeHealth() retained for callers that need it * refactor(health): extract probeLiveness, move full stats to /admin/api/full-stats Addresses outside-voice review of PR #701. The original ?full=true query-param escape hatch was withdrawn because the loopback IP gate's correctness depended on app.set('trust proxy', 'loopback') semantics holding under proxy/XFF misconfiguration, and the PR's own comment misidentified /admin/api/health-indicators as a full-stats endpoint when it actually returns only {expiring_soon, error_rate}. Changes: - src/commands/serve-http.ts: new probeLiveness(sql, engineName, version, timeoutMs) helper next to probeHealth. Same shape, same return type, same finally-block clearTimeout discipline. /health is now a 2-line dispatch through probeLiveness. Removes ?full=true entirely. Adds new admin route /admin/api/full-stats behind the existing requireAdmin middleware that returns probeHealth(engine, ...) — same body shape /health used to expose (status, version, engine, page_count, chunk_count, embedded_count, link_count, tag_count, timeline_entry_count). - test/serve-http-health.test.ts: 4 new probeLiveness cases (success-shape regression with exact-keys assertion, timeout, db-error, timer-cleanup under 100 concurrent probes). - test/e2e/serve-http-oauth.test.ts: existing /health body-shape assertion rewritten to the liveness-only contract (page_count must NOT be present); 2 new admin-stats cases (401 without cookie, 200 with magic-link-derived admin cookie returns getStats() body). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: bump version and changelog (v0.28.10) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: update CLAUDE.md serve-http.ts annotation for v0.28.10 split Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(claude): explicit "run E2E without asking" + schema-bootstrap step The previous wording ("Always run E2E tests when they exist") was easy to read as a soft preference; in practice agents kept proposing the run instead of just doing it. Make the policy unmistakable: if there's a relevant E2E and you want to verify behavior, just spin up the DB and run. Also documents the schema-bootstrap step that bit a fresh container today — `oauth_clients` doesn't exist on a virgin pgvector image until `gbrain doctor` (or any engine-connecting command) triggers `initSchema()`. `apply-migrations` alone runs ALTER-style migrations on top of an already-bootstrapped schema; it does not seed base tables. Tests that bypass the engine via execSync against `gbrain auth register-client` hit the DB directly and need bootstrap first. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(serve-http): persist mcp_request_log on every JSON-RPC method + admin-scope F7 tests Closes the 4 pre-existing E2E failures in test/e2e/serve-http-oauth.test.ts that surfaced when DATABASE_URL was set on the v0.28.10 branch. The branch isn't the cause — these were broken on master too (verified by checking out origin/master's serve-http.ts + test file: 0/4 pass). Owning them here as a bisectable commit. Two root causes, both in serve-http.ts's /mcp logging + scope discipline. 1. mcp_request_log was only INSERTed inside the tools/call success/error paths. tools/list, the unknown-op early-return, and the insufficient-scope early-return all returned without logging. The v0.26.3 persistence regression test calls tools/list + tools/call non-existent and expects >= 2 rows; on the prior implementation it got 0. The agent_name resolution test (single tools/list, expects the row) had the same shape. Fix: log every JSON-RPC method exit point. tools/list logs operation = 'tools/list' with status='success' (lists never fail). Unknown-op logs operation = the attempted name with error_message starting 'unknown_operation:'. Insufficient-scope logs operation = the attempted name with error_message 'insufficient_scope: requires <scope>'. Admin agents auditing /admin/api/requests now see the full attempt log, not just successful valid-op calls. 2. The F7 RCE-regression tests minted 'read write' tokens to assert submit_job for protected names ('shell', 'subagent') gets rejected. But submit_job's required scope is 'admin' (set by hasScope-aware v0.28 enforcement), so a 'read write' token gets rejected with insufficient_scope BEFORE reaching the F7 protected-name guard at operations.ts:1527. The test's assertion checked for 'permission_denied' / 'cannot be submitted over MCP' — neither appears in an insufficient_scope response — so 'rejected' computed to false even though the call was actually rejected. Worse, if someone removed the F7 guard, the test would still pass because scope check would catch it: regression-test integrity failure. Fix: register the e2e-oauth-test client with admin in its allowed scopes (was 'read write', now 'read write admin'), and have F7 tests mint admin-scoped tokens explicitly. Adding admin to the client's allowed ceiling does not auto-grant it to subset-mint calls — other tests minting 'read' / 'read write' still get the subset they ask for. The persistence test's assertion 'rows.find(r => r.operation === "tools/call")' was also updated to match the actual logging convention (operation = inner tool name on call paths, JSON-RPC method on list/scope/unknown paths). E2E result: 29/29 pass on a fresh pgvector container (fixed 4, kept the 25 that were passing). Unit suite: 4191 pass, 0 fail, unchanged. Typecheck: clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: regenerate llms-full.txt after CLAUDE.md update Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: garrytan-agents <garrytan-agents@users.noreply.github.com> Co-authored-by: Garry Tan <garrytan@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
garrytan-agents
@@ -43,8 +43,17 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
// via process.env mutation, which is invisible to subprocesses unless we
|
||||
// explicitly re-pass process.env. Same pattern applies to every execSync
|
||||
// in this file.
|
||||
// v0.28.10: register with admin scope so the F7 protected-name guard
|
||||
// tests can mint admin-scoped tokens that actually exercise the guard
|
||||
// at operations.ts:1527. Without admin in the client's allowed scopes,
|
||||
// submit_job for a protected name (`shell`, `subagent`) gets rejected
|
||||
// by hasScope() in serve-http.ts BEFORE reaching the F7 guard, so the
|
||||
// test was validating scope enforcement instead of the RCE protection.
|
||||
// Other tests that mint specific subsets ('read', 'read write') still
|
||||
// get the subset they ask for — adding admin to the client's allowed
|
||||
// ceiling does not auto-grant it to every minted token.
|
||||
const regOutput = execSync(
|
||||
'bun run src/cli.ts auth register-client e2e-oauth-test --grant-types client_credentials --scopes "read write"',
|
||||
'bun run src/cli.ts auth register-client e2e-oauth-test --grant-types client_credentials --scopes "read write admin"',
|
||||
{ cwd: process.cwd(), encoding: 'utf8', env: { ...process.env } }
|
||||
);
|
||||
const idMatch = regOutput.match(/Client ID:\s+(gbrain_cl_\S+)/);
|
||||
@@ -286,22 +295,78 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
}, 15_000);
|
||||
|
||||
// =========================================================================
|
||||
// Health endpoint (no auth required)
|
||||
// Health endpoint (no auth required) — v0.28.10 made /health liveness-only;
|
||||
// engine stats moved to /admin/api/full-stats behind requireAdmin so a
|
||||
// saturated pool can't pin /health and trigger orchestrator restart cascades.
|
||||
// =========================================================================
|
||||
|
||||
test('health endpoint returns OK without auth', async () => {
|
||||
test('v0.28.10: /health returns liveness-only body (no engine stats)', async () => {
|
||||
const res = await fetch(`${BASE}/health`);
|
||||
expect(res.ok).toBe(true);
|
||||
const data = await res.json() as any;
|
||||
expect(data.status).toBe('ok');
|
||||
expect(data.version).toBeDefined();
|
||||
// page_count: the endpoint must return a non-negative integer. The exact
|
||||
// value depends on the deployment's brain state and is not what this test
|
||||
// is checking — pre-v0.26.2 this asserted `> 0` and broke on fresh schemas.
|
||||
expect(typeof data.page_count).toBe('number');
|
||||
expect(data.page_count).toBeGreaterThanOrEqual(0);
|
||||
expect(data.engine).toBeDefined();
|
||||
// Regression: pre-v0.28.10 /health spread getStats() (page_count,
|
||||
// chunk_count, etc.) into the body. The whole point of the v0.28.10
|
||||
// split is that /health stops touching those tables. If page_count
|
||||
// ever reappears here, the heavy probe leaked back into the public
|
||||
// route and the original DoS surface is back.
|
||||
expect(data.page_count).toBeUndefined();
|
||||
expect(data.chunk_count).toBeUndefined();
|
||||
expect(data.embedded_count).toBeUndefined();
|
||||
// Body shape is exactly {status, version, engine}.
|
||||
expect(Object.keys(data).sort()).toEqual(['engine', 'status', 'version']);
|
||||
});
|
||||
|
||||
test('v0.28.10: /admin/api/full-stats without admin cookie returns 401', async () => {
|
||||
const res = await fetch(`${BASE}/admin/api/full-stats`);
|
||||
expect(res.status).toBe(401);
|
||||
const data = await res.json() as any;
|
||||
expect(data.error).toBe('Admin authentication required');
|
||||
});
|
||||
|
||||
test('v0.28.10: /admin/api/full-stats with valid admin cookie returns getStats() body', async () => {
|
||||
// Same magic-link cookie dance the existing single-use test uses.
|
||||
// Skip gracefully if the bootstrap token isn't extractable — the 401
|
||||
// case above pins the auth gate; this test pins the happy path.
|
||||
const stderrBuf = (serverProcess as any)?._stderrBuffer || '';
|
||||
const tokenMatch = String(stderrBuf).match(/Admin Token[\s\S]*?([a-f0-9]{32,64})/);
|
||||
if (!tokenMatch) {
|
||||
console.warn('[e2e] skipped /admin/api/full-stats happy path: could not extract bootstrap token');
|
||||
return;
|
||||
}
|
||||
const bootstrapToken = tokenMatch[1];
|
||||
|
||||
const issueRes = await fetch(`${BASE}/admin/api/issue-magic-link`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${bootstrapToken}` },
|
||||
body: '{}',
|
||||
});
|
||||
expect(issueRes.ok).toBe(true);
|
||||
const { url } = await issueRes.json() as any;
|
||||
|
||||
const click = await fetch(url, { redirect: 'manual' });
|
||||
expect(click.status).toBe(302);
|
||||
const setCookie = click.headers.get('set-cookie') || '';
|
||||
const cookieMatch = setCookie.match(/gbrain_admin=([^;]+)/);
|
||||
expect(cookieMatch).toBeTruthy();
|
||||
const cookieValue = cookieMatch![1];
|
||||
|
||||
const statsRes = await fetch(`${BASE}/admin/api/full-stats`, {
|
||||
headers: { Cookie: `gbrain_admin=${cookieValue}` },
|
||||
});
|
||||
expect(statsRes.ok).toBe(true);
|
||||
const stats = await statsRes.json() as any;
|
||||
expect(stats.status).toBe('ok');
|
||||
expect(stats.version).toBeDefined();
|
||||
expect(stats.engine).toBeDefined();
|
||||
// The full-stats body is probeHealth's spread of getStats() — page_count
|
||||
// is the canonical signal that we're hitting the heavy path here.
|
||||
expect(typeof stats.page_count).toBe('number');
|
||||
expect(stats.page_count).toBeGreaterThanOrEqual(0);
|
||||
}, 15_000);
|
||||
|
||||
// =========================================================================
|
||||
// Token lifecycle
|
||||
// =========================================================================
|
||||
@@ -476,8 +541,9 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
expect(okRes.status).not.toBe(401);
|
||||
|
||||
// Trigger an error path so the error_message column gets a value too.
|
||||
// Request a tool that doesn't exist — server returns an MCP error in
|
||||
// the body but the underlying handler logs status='error' to mcp_request_log.
|
||||
// Request a tool that doesn't exist — v0.28.10 logs unknown-op attempts
|
||||
// with operation = the attempted name and error_message starting with
|
||||
// 'unknown_operation:'.
|
||||
await mcpCall(access_token, 'tools/call', { name: 'this_tool_does_not_exist', arguments: {} });
|
||||
|
||||
// Allow async best-effort INSERT to flush.
|
||||
@@ -498,18 +564,26 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
expect(row.agent_name).toBe('e2e-oauth-test');
|
||||
}
|
||||
|
||||
// params persisted as JSONB (postgres-js returns object form).
|
||||
// The params field is non-null on tools/call (carries the call args)
|
||||
// and on tools/list (carries an empty {} or undefined depending on payload).
|
||||
const callRow = rows.find(r => r.operation === 'tools/call');
|
||||
// v0.28.10: tools/list logs as operation='tools/list' (the JSON-RPC
|
||||
// method name). tools/call success/error logs as operation=<inner
|
||||
// tool name> (the convention preserved from pre-v0.28.10 dispatch
|
||||
// logging — agents querying mcp_request_log filter by tool name, not
|
||||
// by JSON-RPC method).
|
||||
const listRow = rows.find(r => r.operation === 'tools/list');
|
||||
expect(listRow).toBeDefined();
|
||||
expect(listRow!.status).toBe('success');
|
||||
|
||||
// The unknown-op call shows up with operation = the attempted name.
|
||||
const callRow = rows.find(r => r.operation === 'this_tool_does_not_exist');
|
||||
expect(callRow).toBeDefined();
|
||||
expect(callRow!.params).toBeDefined();
|
||||
expect(callRow!.status).toBe('error');
|
||||
|
||||
// error_message populated on the failed call.
|
||||
const errorRow = rows.find(r => r.status === 'error');
|
||||
expect(errorRow).toBeDefined();
|
||||
expect(errorRow!.error_message).toBeTruthy();
|
||||
expect(typeof errorRow!.error_message).toBe('string');
|
||||
expect(errorRow!.error_message as string).toContain('unknown_operation');
|
||||
} finally {
|
||||
await sql.end();
|
||||
}
|
||||
@@ -758,7 +832,12 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
// Together they close the path even if either layer regresses alone.
|
||||
|
||||
test('F7: HTTP MCP cannot submit shell jobs (RCE regression)', async () => {
|
||||
const { access_token } = await mintToken('read write');
|
||||
// v0.28.10: must mint admin scope. submit_job's required scope is
|
||||
// 'admin'; without it, hasScope() rejects with insufficient_scope BEFORE
|
||||
// the F7 protected-name guard at operations.ts:1527 fires. To validate
|
||||
// the actual RCE protection (the protected-name guard), the token has
|
||||
// to clear the scope check first.
|
||||
const { access_token } = await mintToken('admin');
|
||||
const res = await mcpCall(access_token, 'tools/call', {
|
||||
name: 'submit_job',
|
||||
arguments: { name: 'shell', data: { cmd: 'id' } },
|
||||
@@ -782,7 +861,8 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => {
|
||||
}, 15_000);
|
||||
|
||||
test('F7: HTTP MCP cannot submit subagent jobs (protected name)', async () => {
|
||||
const { access_token } = await mintToken('read write');
|
||||
// Same admin-scope requirement as the shell-job sibling test above.
|
||||
const { access_token } = await mintToken('admin');
|
||||
const res = await mcpCall(access_token, 'tools/call', {
|
||||
name: 'submit_job',
|
||||
arguments: { name: 'subagent', data: { prompt: 'noop' } },
|
||||
|
||||
Reference in New Issue
Block a user