diff --git a/src/adapters/providers/manifest.ts b/src/adapters/providers/manifest.ts index 820af57..fbfe562 100644 --- a/src/adapters/providers/manifest.ts +++ b/src/adapters/providers/manifest.ts @@ -198,30 +198,36 @@ export function manifestProvider(manifest: ProviderManifest): ProviderAdapter { ...authHeader(manifest.auth, ctx.accessToken ?? ''), 'x-request-id': ctx.requestId, }, + // A vendor that accepts the connection and never answers + // would otherwise hold the handler for undici's default. + // AbortSignal rather than a raced promise, because this has + // to cancel the request and not merely stop waiting on it. + // followRedirects spreads init, so it survives every hop. + signal: AbortSignal.timeout(VALIDATE_TIMEOUT_MS), }, ) - } catch { + } catch (err) { + if ((err as Error)?.name === 'TimeoutError') { + throw fail( + 'upstream_timeout', + `${manifest.prefix} did not answer the key check within ${VALIDATE_TIMEOUT_MS}ms`, + ) + } throw fail('upstream_error', `${manifest.prefix} validation request failed`) } - // invalid_arguments, not reauth_required: the key being refused - // arrived in this request, so it is a bad argument rather than a - // stored credential that went stale. - if (res.status === 401) { - throw fail('invalid_arguments', `${manifest.prefix} refused the api key`) - } - if (res.status === 403) { - // A bare 403 does not mean a bad key for every vendor, which is - // the whole reason errors.forbidden exists. - throw fail( - manifest.errors?.forbidden ?? 'invalid_arguments', - `${manifest.prefix} refused the api key`, - ) - } - if (!res.ok) { - throw fail('upstream_error', `${manifest.prefix} validation request failed`) + // Drained either way: an unread body holds the socket, and on a + // failure the text is the only thing that says why. + const text = await res.text().catch(() => '') + let body: unknown + try { + body = JSON.parse(text) + } catch { + body = undefined } - // Nothing reads this body, and an unread one holds the socket. - await res.body?.cancel() + // invalid_arguments rather than reauth_required: the key being + // refused arrived in this request, so it is a bad argument and not + // a stored credential that went stale. + throwIfFailed(manifest, res, fail, body, res.ok ? undefined : text, 'invalid_arguments') }, } : {}), @@ -271,6 +277,10 @@ export function manifestProvider(manifest: ProviderManifest): ProviderAdapter { type Fail = (code: ErrorCode, message: string, retryAfter?: number) => GatewayError const MAX_HOPS = 5 +/** Shorter than the 30s a tool call gets: this one runs inside an HTTP request + * somebody is waiting on, and a key that needs half a minute to check is + * already an answer. */ +const VALIDATE_TIMEOUT_MS = 10_000 /** * Redirects are followed here rather than by fetch, for one reason: a redirect @@ -630,33 +640,24 @@ async function binaryResult(fail: Fail, prefix: string, res: Response): Promise< } } -async function readResponse( +/** + * Every way a response can be a failure, in one place, so a new vendor rule + * lands once instead of in each caller. The executor and the api key validator + * differ on exactly one thing, which is what `credentialCode` carries: a stored + * credential that stopped working wants reauth_required, and a key that arrived + * in the request being served is a bad argument instead. + */ +function throwIfFailed( manifest: ProviderManifest, - tool: ToolManifest, - paging: Pagination | undefined, - cursor: string | number | undefined, res: Response, fail: Fail, - callerChoseFields = false, -): Promise { + body: unknown, + failureText: string | undefined, + credentialCode: ErrorCode, +): void { const rules = manifest.errors ?? {} const failure = rules.bodyFailure - // Parsed before the status is looked at, because Slack answers 200 with - // {ok: false} and the status carries no signal at all. - // Read once, whatever the outcome. On a failure the text is the only thing - // that says why, and a Response body cannot be read twice. - const failureText = res.ok ? undefined : await res.text().catch(() => '') - const body = res.ok - ? await readOk(tool, res) - : ((): unknown => { - try { - return JSON.parse(failureText ?? '') - } catch { - return undefined - } - })() - if (failure && getPath(body, failure.path) === failure.equals) { const raw = getPath(body, failure.codeFrom) const code = (typeof raw === 'string' ? failure.codes[raw] : undefined) ?? 'upstream_error' @@ -672,13 +673,13 @@ async function readResponse( throw fail('rate_limited', `${manifest.prefix} rate limit reached`, retryAfterFrom(res, rules)) } if (res.status === 401) { - throw fail('reauth_required', `${manifest.prefix} rejected the credential`) + throw fail(credentialCode, `${manifest.prefix} rejected the credential`) } if (res.status === 403) { - const code = rules.forbidden ?? 'reauth_required' + const code = rules.forbidden ?? credentialCode throw fail( code, - code === 'reauth_required' + code === credentialCode ? `${manifest.prefix} rejected the credential` : `${manifest.prefix} refused the request, and reconnecting will not help`, ) @@ -707,6 +708,33 @@ async function readResponse( : `${manifest.prefix} returned ${res.status}`, ) } +} + +async function readResponse( + manifest: ProviderManifest, + tool: ToolManifest, + paging: Pagination | undefined, + cursor: string | number | undefined, + res: Response, + fail: Fail, + callerChoseFields = false, +): Promise { + // Parsed before the status is looked at, because Slack answers 200 with + // {ok: false} and the status carries no signal at all. + // Read once, whatever the outcome. On a failure the text is the only thing + // that says why, and a Response body cannot be read twice. + const failureText = res.ok ? undefined : await res.text().catch(() => '') + const body = res.ok + ? await readOk(tool, res) + : ((): unknown => { + try { + return JSON.parse(failureText ?? '') + } catch { + return undefined + } + })() + + throwIfFailed(manifest, res, fail, body, failureText, 'reauth_required') const keep = (value: unknown) => (callerChoseFields ? value : project(value, tool.fields)) diff --git a/test/providers/manifest.test.ts b/test/providers/manifest.test.ts index a3e3fb8..48a0c32 100644 --- a/test/providers/manifest.test.ts +++ b/test/providers/manifest.test.ts @@ -83,6 +83,70 @@ describe('manifest executor: requests', () => { }) }) + it('reports a rate limited key check as rate_limited with the retry hint', async () => { + // The validator used to answer 502 here, so a client retried immediately + // and made it worse. It shares the executor's mapping now. + const validator = withTools([], { + validate: { request: 'GET /users/@me' }, + errors: { retryAfter: [{ header: 'retry-after', as: 'seconds' }] }, + }) + const upstream = fakeUpstream([ + { match: /users\/@me/, status: 429, body: {}, headers: { 'retry-after': '30' } }, + ]) + + const err = await validator + .validateKey?.({ ...ctx(upstream), accessToken: 'k' }) + .catch((e) => e) + expect(err.code).toBe('rate_limited') + expect(err.details.retryAfter).toBe(30) + }) + + it('carries the vendor reason out of a failed key check', async () => { + const validator = withTools([], { validate: { request: 'GET /users/@me' } }) + const upstream = fakeUpstream([ + { match: /users\/@me/, status: 500, raw: 'upstream exploded' }, + ]) + + const err = await validator + .validateKey?.({ ...ctx(upstream), accessToken: 'k' }) + .catch((e) => e) + expect(err.code).toBe('upstream_error') + expect(err.message).toContain('upstream exploded') + }) + + it('gives the key check a deadline and cancels it rather than only waiting', async () => { + const validator = withTools([], { validate: { request: 'GET /users/@me' } }) + let seen: AbortSignal | undefined + const fetching = (async (_url: string, init?: RequestInit) => { + seen = init?.signal ?? undefined + return new Response('{}', { status: 200, headers: { 'content-type': 'application/json' } }) + }) as typeof fetch + + await validator.validateKey?.({ + workspaceId: 'ws-1', + requestId: 'req-1', + accessToken: 'k', + fetch: fetching, + }) + // An AbortSignal aborts the request. A raced promise would leave the socket + // held, which is what this is here to stop happening again. + expect(seen).toBeInstanceOf(AbortSignal) + }) + + it('maps an aborted key check to upstream_timeout, not a generic failure', async () => { + const validator = withTools([], { validate: { request: 'GET /users/@me' } }) + const timingOut = (async () => { + const err = new Error('The operation was aborted due to timeout') + err.name = 'TimeoutError' + throw err + }) as typeof fetch + + const err = await validator + .validateKey?.({ workspaceId: 'ws-1', requestId: 'req-1', accessToken: 'k', fetch: timingOut }) + .catch((e) => e) + expect(err.code).toBe('upstream_timeout') + }) + it('puts query API keys on the validation request', async () => { const validator = withTools([], { validate: { request: 'GET /users/@me' },