fix(connections): give the key check a deadline and the executor's error mapping

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<ToolResult> {
+  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<ToolResult> {
+  // 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))
 

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' },