fix(rpc): close the service-collision fail-open and the fix-wave gaps
Critical: - router: fail loudly (WRN-SERVICE-COLLISION) when two app/services files scan to the same service name, instead of silently letting directory-walk order pick a winner. Important: - server.ts: wrap a throwing input schema so its raw message cannot escape invoke(); returns RPC_INVALID and logs server-side instead. - client.ts: race timeoutMs against transport.call so a stalled transport cannot hang the caller; rejects with a ServiceError(RPC_TRANSPORT). - client.ts: the proxy returns undefined for undeclared properties (incl. then/catch/finally) instead of a function that throws, closing the await-client thenable trap. - gateway.ts / rpc-dispatch.ts: import RPC_PATH_PREFIX / RPC_INTERNAL_HEADER from @wrnexus/rpc instead of hardcoding local copies. - gateway.test.ts: cover the RPC-prefix edge block and internal-header stripping across casing variants. - http.test.ts / client.test.ts: cover anonymous-call header omission, the internal marker, the retryable-status sweep, network/malformed/HTML failures, AbortSignal propagation, the timeout path, and timer cleanup. Minor: - transport.ts: Object.hasOwn for handler lookup; note the entry-only abort check. - client.ts: wrap a missing/invalid WRNEXUS_RPC_SECRET as a ServiceError (RPC_IDENTITY) instead of a bare Error. - rpc/package.json: drop the unused @wrnexus/authz dependency. - server.ts: implement() now throws at construction time if a declared procedure has no own handler. Verified: reverting the service-collision check and the client timeout race each make their new test fail, then restore green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -19,7 +19,6 @@
|
||||
"check": "bun run typecheck && bun run test"
|
||||
},
|
||||
"dependencies": {
|
||||
"@wrnexus/authz": "workspace:*",
|
||||
"@wrnexus/core": "workspace:*",
|
||||
"@wrnexus/jwt": "workspace:*",
|
||||
"@wrnexus/helpers": "workspace:*",
|
||||
|
||||
@@ -32,24 +32,53 @@ export function serviceClient<Procedures extends AnyProcedures>(
|
||||
const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS;
|
||||
return new Proxy({} as ServiceClient<Procedures>, {
|
||||
get(_target, property) {
|
||||
if (typeof property !== "string") return undefined;
|
||||
// Every declared procedure is a string key; anything else (including
|
||||
// `then`/`catch`/`finally`) is not one of ours. Returning a function for
|
||||
// those makes `await client` or `return client` from an async function
|
||||
// read the proxy as thenable — the runtime then calls `then(resolve,
|
||||
// reject)`, which throws "Unknown procedure". Returning undefined lets
|
||||
// the caller be treated as a plain (non-thenable) object instead.
|
||||
if (typeof property !== "string" || !Object.hasOwn(contract.procedures, property)) {
|
||||
return undefined;
|
||||
}
|
||||
return async (input: unknown) => {
|
||||
if (!Object.hasOwn(contract.procedures, property)) {
|
||||
throw new ServiceError(RPC_ERROR_CODES.unknown, "Unknown procedure");
|
||||
let identity: string | undefined;
|
||||
if (options.as) {
|
||||
try {
|
||||
identity = await exportSubjectContext(options.as, app);
|
||||
} catch (error) {
|
||||
// A missing/misconfigured WRNEXUS_RPC_SECRET otherwise rejects with
|
||||
// a bare Error, so a caller matching on ServiceError treats
|
||||
// misconfiguration as a crash instead of a handled RPC failure.
|
||||
// The operator-facing message names no secret value, so it is safe
|
||||
// to preserve.
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
throw new ServiceError(RPC_ERROR_CODES.identity, message);
|
||||
}
|
||||
}
|
||||
const identity = options.as ? await exportSubjectContext(options.as, app) : undefined;
|
||||
const controller = new AbortController();
|
||||
const timer = setTimeout(() => controller.abort(), timeoutMs);
|
||||
const timeout = new Promise<never>((_, reject) => {
|
||||
controller.signal.addEventListener("abort", () => {
|
||||
reject(new ServiceError(RPC_ERROR_CODES.transport, "Call timed out"));
|
||||
});
|
||||
});
|
||||
const callPromise = options.transport.call(
|
||||
{ app, service: contract.name, procedure: property },
|
||||
input,
|
||||
{ signal: controller.signal, ...(identity ? { identity } : {}) },
|
||||
);
|
||||
try {
|
||||
const result = await options.transport.call(
|
||||
{ app, service: contract.name, procedure: property },
|
||||
input,
|
||||
{ signal: controller.signal, ...(identity ? { identity } : {}) },
|
||||
);
|
||||
const result = await Promise.race([callPromise, timeout]);
|
||||
if (result.ok) return result.value;
|
||||
throw new ServiceError(result.code, result.message);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
// If the timeout won the race, the transport call may still settle
|
||||
// later — a transport that ignores the abort signal keeps running.
|
||||
// Nothing awaits it again, so swallow a late rejection here rather
|
||||
// than let it surface as an unhandled promise rejection.
|
||||
callPromise.catch(() => {});
|
||||
}
|
||||
};
|
||||
},
|
||||
|
||||
@@ -34,6 +34,16 @@ export function implement<Procedures extends AnyProcedures>(
|
||||
handlers: ServiceHandlers<Procedures>,
|
||||
options: ImplementOptions,
|
||||
): ServiceImplementation<Procedures> {
|
||||
// A declared procedure with no own handler would otherwise only surface at
|
||||
// invoke time as RPC_UNKNOWN — a silent, permanent 404. Catch it now.
|
||||
for (const procedureName of Object.keys(contract.procedures)) {
|
||||
if (!Object.hasOwn(handlers, procedureName)) {
|
||||
throw new Error(
|
||||
`WRN-RPC-HANDLER: service "${contract.name}" declares procedure "${procedureName}" ` +
|
||||
`but implement() was not given a handler for it.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
return {
|
||||
contract,
|
||||
async invoke(procedureName, payload, identity) {
|
||||
@@ -69,7 +79,17 @@ export function implement<Procedures extends AnyProcedures>(
|
||||
|
||||
let input: unknown = payload;
|
||||
if (definition.input) {
|
||||
const parsed = definition.input.parse(payload as Record<string, unknown>);
|
||||
// InputSchema is structural: any custom or wrapped schema may throw
|
||||
// instead of returning { ok: false }. A throw must not escape invoke()
|
||||
// with its raw message — that text can carry internals — so it is
|
||||
// caught the same way the permission check above is.
|
||||
let parsed: { ok: boolean; value?: unknown };
|
||||
try {
|
||||
parsed = definition.input.parse(payload as Record<string, unknown>);
|
||||
} catch (error) {
|
||||
console.error(`[wrnexus] RPC input schema threw for ${String(procedureName)}`, error);
|
||||
return failure(RPC_ERROR_CODES.invalid, "Invalid input");
|
||||
}
|
||||
if (!parsed.ok) return failure(RPC_ERROR_CODES.invalid, "Invalid input");
|
||||
input = parsed.value;
|
||||
}
|
||||
|
||||
@@ -25,8 +25,16 @@ export type InProcessHandler = (
|
||||
export function inProcessTransport(handlers: Record<string, InProcessHandler>): Transport {
|
||||
return {
|
||||
async call(target, payload, options) {
|
||||
// Checked only at entry: this in-process transport does no I/O, so
|
||||
// nothing yields between here and the handler call below, and a signal
|
||||
// that aborts mid-flight is never observed. In-process tests therefore
|
||||
// cannot exercise a mid-call timeout path — that needs a real transport.
|
||||
if (options.signal?.aborted) return failure(RPC_ERROR_CODES.transport, "Call aborted");
|
||||
const handler = handlers[`${target.service}/${target.procedure}`];
|
||||
const key = `${target.service}/${target.procedure}`;
|
||||
// Object.hasOwn, not plain indexing: a prototype-inherited key (e.g.
|
||||
// "constructor/toString") would otherwise resolve to a function that
|
||||
// is not one of our handlers.
|
||||
const handler = Object.hasOwn(handlers, key) ? handlers[key] : undefined;
|
||||
if (!handler) return failure(RPC_ERROR_CODES.unknown, "Unknown procedure");
|
||||
try {
|
||||
return await handler(payload, options.identity);
|
||||
|
||||
@@ -76,7 +76,7 @@ describe("serviceClient", () => {
|
||||
expect(seenAnon).toBeUndefined();
|
||||
});
|
||||
|
||||
test("calling an undeclared procedure throws rather than issuing a call", async () => {
|
||||
test("an undeclared procedure is undefined rather than a function that throws", async () => {
|
||||
let called = false;
|
||||
const client = serviceClient(billing, {
|
||||
transport: {
|
||||
@@ -86,13 +86,91 @@ describe("serviceClient", () => {
|
||||
},
|
||||
},
|
||||
});
|
||||
const proxy = client as unknown as Record<string, (input: unknown) => Promise<unknown>>;
|
||||
await expect(proxy.deleteEverything!({})).rejects.toMatchObject({
|
||||
code: RPC_ERROR_CODES.unknown,
|
||||
});
|
||||
const proxy = client as unknown as Record<string, unknown>;
|
||||
expect(proxy.deleteEverything).toBeUndefined();
|
||||
expect(called).toBe(false);
|
||||
});
|
||||
|
||||
test("the proxy has no `then` escape — await/return does not trigger a call", async () => {
|
||||
let called = false;
|
||||
const client = serviceClient(billing, {
|
||||
transport: {
|
||||
call: async () => {
|
||||
called = true;
|
||||
return success({ invoiceId: "inv_1" });
|
||||
},
|
||||
},
|
||||
});
|
||||
const proxy = client as unknown as Record<string, unknown>;
|
||||
expect(proxy.then).toBeUndefined();
|
||||
expect(proxy.catch).toBeUndefined();
|
||||
expect(proxy.finally).toBeUndefined();
|
||||
// `await client` must resolve to the proxy object itself, not reject.
|
||||
const awaited = await client;
|
||||
expect(awaited).toBe(client);
|
||||
expect(called).toBe(false);
|
||||
});
|
||||
|
||||
test("the signal arrives at the transport", async () => {
|
||||
let seenSignal: AbortSignal | undefined;
|
||||
const client = serviceClient(billing, {
|
||||
transport: {
|
||||
call: async (_target, _payload, options) => {
|
||||
seenSignal = options.signal;
|
||||
return success({ invoiceId: "inv_1" });
|
||||
},
|
||||
},
|
||||
});
|
||||
await client.createInvoice({ amountCents: 1 });
|
||||
expect(seenSignal).toBeInstanceOf(AbortSignal);
|
||||
});
|
||||
|
||||
test("the call aborts at timeoutMs when the transport ignores the signal", async () => {
|
||||
const client = serviceClient(billing, {
|
||||
timeoutMs: 20,
|
||||
transport: {
|
||||
call: () => new Promise(() => {}), // never resolves; ignores the signal
|
||||
},
|
||||
});
|
||||
let caught: unknown;
|
||||
const start = Date.now();
|
||||
try {
|
||||
await client.createInvoice({ amountCents: 1 });
|
||||
} catch (err) {
|
||||
caught = err;
|
||||
}
|
||||
const elapsed = Date.now() - start;
|
||||
expect(caught).toBeInstanceOf(ServiceError);
|
||||
expect(caught).toMatchObject({ code: RPC_ERROR_CODES.transport });
|
||||
expect(elapsed).toBeLessThan(500);
|
||||
});
|
||||
|
||||
test("the timer is cleared on both success and failure", async () => {
|
||||
const originalClearTimeout = globalThis.clearTimeout;
|
||||
let clearCount = 0;
|
||||
globalThis.clearTimeout = ((...args: Parameters<typeof clearTimeout>) => {
|
||||
clearCount++;
|
||||
return originalClearTimeout(...args);
|
||||
}) as typeof clearTimeout;
|
||||
try {
|
||||
const okClient = serviceClient(billing, {
|
||||
transport: { call: async () => success({ invoiceId: "inv_1" }) },
|
||||
});
|
||||
await okClient.createInvoice({ amountCents: 1 });
|
||||
expect(clearCount).toBe(1);
|
||||
|
||||
const failClient = serviceClient(billing, {
|
||||
transport: { call: async () => failure(RPC_ERROR_CODES.transport, "down") },
|
||||
});
|
||||
await expect(failClient.createInvoice({ amountCents: 1 })).rejects.toBeInstanceOf(
|
||||
ServiceError,
|
||||
);
|
||||
expect(clearCount).toBe(2);
|
||||
} finally {
|
||||
globalThis.clearTimeout = originalClearTimeout;
|
||||
}
|
||||
});
|
||||
|
||||
test("the app defaults to the service name", async () => {
|
||||
let seenTarget: RpcTarget | undefined;
|
||||
const client = serviceClient(billing, {
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { RPC_ERROR_CODES } from "../src/errors.ts";
|
||||
import { RPC_ERROR_CODES, isRetryableStatus } from "../src/errors.ts";
|
||||
import { RPC_IDENTITY_HEADER } from "../src/identity.ts";
|
||||
import { httpTransport, rpcPath } from "../src/http.ts";
|
||||
import { RPC_INTERNAL_HEADER, httpTransport, rpcPath } from "../src/http.ts";
|
||||
|
||||
const target = { app: "billing", service: "billing", procedure: "createInvoice" };
|
||||
|
||||
@@ -42,4 +42,82 @@ describe("httpTransport", () => {
|
||||
test("uses the stable reserved path", () => {
|
||||
expect(rpcPath("billing", "createInvoice")).toBe("/__wrnexus/rpc/billing/createInvoice");
|
||||
});
|
||||
|
||||
test("omits the identity header entirely for an anonymous call", async () => {
|
||||
const transport = transportWith(async (request) => {
|
||||
expect(request.headers.has(RPC_IDENTITY_HEADER)).toBe(false);
|
||||
return Response.json({ ok: true, value: {} });
|
||||
});
|
||||
await transport.call(target, {}, {});
|
||||
});
|
||||
|
||||
test("sets the internal-marker header", async () => {
|
||||
const transport = transportWith(async (request) => {
|
||||
expect(request.headers.get(RPC_INTERNAL_HEADER)).toBe("1");
|
||||
return Response.json({ ok: true, value: {} });
|
||||
});
|
||||
await transport.call(target, {}, {});
|
||||
});
|
||||
|
||||
test("the full retryable-status sweep matches isRetryableStatus", async () => {
|
||||
// 600 is covered directly on isRetryableStatus in errors.test.ts — the
|
||||
// Fetch API cannot construct a Response with a status outside 200–599.
|
||||
const statuses = [200, 400, 403, 404, 408, 409, 429, 500, 503, 599];
|
||||
for (const status of statuses) {
|
||||
if (status === 200) continue; // handled by the success-path test above
|
||||
const transport = transportWith(() => new Response("x", { status }));
|
||||
const result = await transport.call(target, {}, {});
|
||||
expect(result).toMatchObject({
|
||||
ok: false,
|
||||
code: RPC_ERROR_CODES.transport,
|
||||
retryable: isRetryableStatus(status),
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
test("a network throw yields a structured failure with no host/address surviving", async () => {
|
||||
const transport = httpTransport({
|
||||
resolveOrigin: () => "http://internal-billing-host.private:4821",
|
||||
fetch: (async () => {
|
||||
throw new TypeError("fetch failed: connect ECONNREFUSED 10.0.0.7:4821");
|
||||
}) as unknown as typeof fetch,
|
||||
});
|
||||
const result = await transport.call(target, {}, {});
|
||||
expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.transport });
|
||||
if (!result.ok) {
|
||||
expect(result.message).not.toContain("10.0.0.7");
|
||||
expect(result.message).not.toContain("internal-billing-host");
|
||||
}
|
||||
});
|
||||
|
||||
test("a malformed JSON body yields a structured failure with no body content surviving", async () => {
|
||||
const transport = transportWith(() => new Response("{not json", { status: 200 }));
|
||||
const result = await transport.call(target, {}, {});
|
||||
expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.malformed });
|
||||
if (!result.ok) expect(result.message).not.toContain("{not json");
|
||||
});
|
||||
|
||||
test("an HTML error page yields a structured failure with no page content surviving", async () => {
|
||||
const transport = transportWith(
|
||||
() =>
|
||||
new Response("<html><body>500 Internal Server Error at db-host-42</body></html>", {
|
||||
status: 200,
|
||||
headers: { "content-type": "text/html" },
|
||||
}),
|
||||
);
|
||||
const result = await transport.call(target, {}, {});
|
||||
expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.malformed });
|
||||
if (!result.ok) expect(result.message).not.toContain("db-host-42");
|
||||
});
|
||||
|
||||
test("the AbortSignal reaches fetch", async () => {
|
||||
const controller = new AbortController();
|
||||
let seenSignal: AbortSignal | undefined;
|
||||
const transport = transportWith(async (request) => {
|
||||
seenSignal = request.signal;
|
||||
return Response.json({ ok: true, value: {} });
|
||||
});
|
||||
await transport.call(target, {}, { signal: controller.signal });
|
||||
expect(seenSignal).toBeDefined();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user