From a5fcc1b0352fc5fbfbb73e1e6dc258b4f31e27f0 Mon Sep 17 00:00:00 2001 From: Nicolas Bouliol Date: Tue, 1 Sep 2026 23:19:25 +0200 Subject: [PATCH 1/2] fix(agent-bff): resolve the environment id on demand, not at boot `fetchEnvironmentId()` was awaited while assembling the OAuth edge, so it was the only fatal boot error of a server whose stated posture is to boot and report gaps through /health. A Forest server unreachable for five seconds at startup left OAuth and the AI relay unmounted for the life of the process. The id is now resolved on the first request that needs it, cached on success and never on failure, so a transient outage heals itself. On /oauth/authorize a failed resolution travels back to the client as `server_error`, which is what RFC 6749 asks for once the redirect_uri is trusted; the context route keeps answering and just drops the field. Co-Authored-By: Claude Opus 5 (1M context) --- .../agent-bff/src/ai/ai-routes-middleware.ts | 7 +-- packages/agent-bff/src/build-bff.ts | 53 +++++++++++++++---- .../src/context/context-routes-middleware.ts | 9 +++- .../agent-bff/src/oauth/environment-id.ts | 33 ++++++++++++ packages/agent-bff/src/oauth/oauth-routes.ts | 22 +++++++- .../test/ai/ai-routes-middleware.test.ts | 10 +++- packages/agent-bff/test/cli-core.test.ts | 26 ++++++--- .../context/context-routes-middleware.test.ts | 4 +- .../test/oauth/environment-id.test.ts | 41 ++++++++++++++ .../agent-bff/test/oauth/oauth-routes.test.ts | 24 ++++++++- 10 files changed, 202 insertions(+), 27 deletions(-) create mode 100644 packages/agent-bff/src/oauth/environment-id.ts create mode 100644 packages/agent-bff/test/oauth/environment-id.test.ts diff --git a/packages/agent-bff/src/ai/ai-routes-middleware.ts b/packages/agent-bff/src/ai/ai-routes-middleware.ts index 9eadfdc198..7741cd7be9 100644 --- a/packages/agent-bff/src/ai/ai-routes-middleware.ts +++ b/packages/agent-bff/src/ai/ai-routes-middleware.ts @@ -1,6 +1,7 @@ import type AiProxyClient from './ai-proxy-client'; import type { AiProxyResponse } from './ai-proxy-client'; import type { BffAccessTokenPayload } from '../oauth/bff-token'; +import type { EnvironmentIdResolver } from '../oauth/environment-id'; import type ForestServerClient from '../oauth/forest-server-client'; import type { SessionStore } from '../oauth/session-store'; import type { Logger } from '../ports/logger-port'; @@ -28,7 +29,7 @@ export interface AiRoutesMiddlewareOptions { client: AiProxyClient; sessionStore: SessionStore; serverClient: ForestServerClient; - environmentId?: number; + resolveEnvironmentId?: EnvironmentIdResolver; logger: Logger; } @@ -104,7 +105,7 @@ export default function createAiRoutesMiddleware({ client, sessionStore, serverClient, - environmentId, + resolveEnvironmentId, logger, }: AiRoutesMiddlewareOptions): Middleware { return async function aiRoutesMiddleware(ctx, next) { @@ -129,7 +130,7 @@ export default function createAiRoutesMiddleware({ try { response = await client.query({ saasAccessToken, - environmentId, + environmentId: await resolveEnvironmentId?.(), renderingId, body: ctx.request.body, }); diff --git a/packages/agent-bff/src/build-bff.ts b/packages/agent-bff/src/build-bff.ts index dc8ddd17af..cd2274c26f 100644 --- a/packages/agent-bff/src/build-bff.ts +++ b/packages/agent-bff/src/build-bff.ts @@ -1,4 +1,5 @@ import type { BFFConfig } from './config/env-config'; +import type { EnvironmentIdResolver } from './oauth/environment-id'; import type { SessionStore } from './oauth/session-store'; import type { UnfoldSource } from './openapi/unfolded-document'; import type { Logger } from './ports/logger-port'; @@ -30,6 +31,7 @@ import BODY_LIMIT, { AI_BODY_LIMIT } from './http/body-limit'; import createErrorMiddleware from './http/error-middleware'; import createHealthRoute from './http/health-route'; import createVersionHeaderMiddleware from './http/version-header-middleware'; +import createEnvironmentIdResolver from './oauth/environment-id'; import ForestServerClient from './oauth/forest-server-client'; import createOAuthRoutes from './oauth/oauth-routes'; import createInMemorySessionStore from './oauth/session-store'; @@ -134,11 +136,11 @@ interface OAuthSession { interface OAuthEdge { middlewares: Middleware[]; - environmentId?: number; + resolveEnvironmentId?: EnvironmentIdResolver; session?: OAuthSession; } -async function buildOAuthMiddlewares(config: BFFConfig, logger: Logger): Promise { +function buildOAuthMiddlewares(config: BFFConfig, logger: Logger): OAuthEdge { const oauthConfig = resolveOAuthConfig(config); if (!oauthConfig) { @@ -151,7 +153,7 @@ async function buildOAuthMiddlewares(config: BFFConfig, logger: Logger): Promise oauthConfig; const serverClient = new ForestServerClient({ forestServerUrl, envSecret: forestEnvSecret }); - const environmentId = await serverClient.fetchEnvironmentId(); + const resolveEnvironmentId = createEnvironmentIdResolver(serverClient); const sessionStore = createInMemorySessionStore({ cipher: createTokenCipher(tokenEncryptionKey), @@ -164,13 +166,13 @@ async function buildOAuthMiddlewares(config: BFFConfig, logger: Logger): Promise sessionStore, forestAppUrl, authSecret: forestAuthSecret, - environmentId, + resolveEnvironmentId, logger, }); return { middlewares: [oauthRoutes], - environmentId, + resolveEnvironmentId, session: { store: sessionStore, serverClient, forestServerUrl }, }; } @@ -318,7 +320,7 @@ function buildAgentRouteMiddlewares( } function buildAiMiddlewares(config: BFFConfig, oauth: OAuthEdge, logger: Logger): Middleware[] { - const { session, environmentId } = oauth; + const { session, resolveEnvironmentId } = oauth; if (!session) { logger('Warn', 'AI query route disabled: the deployment carries no OAuth session'); @@ -336,12 +338,37 @@ function buildAiMiddlewares(config: BFFConfig, oauth: OAuthEdge, logger: Logger) client, sessionStore: session.store, serverClient: session.serverClient, - environmentId, + resolveEnvironmentId, logger, }), ]; } +/** + * The context payload carries the environment id when there is one, and drops it when the Forest + * server cannot be reached — the route itself must keep answering either way. + */ +function toleratedEnvironmentId( + oauth: OAuthEdge, + logger: Logger, +): (() => Promise) | undefined { + const { resolveEnvironmentId } = oauth; + + if (!resolveEnvironmentId) return undefined; + + return async function resolveOrForget(): Promise { + try { + return await resolveEnvironmentId(); + } catch (error) { + logger('Warn', 'Serving the context without an environment id', { + cause: error instanceof Error ? error.message : String(error), + }); + + return undefined; + } + }; +} + function buildAgentMiddlewares( config: BFFConfig, logger: Logger, @@ -349,7 +376,6 @@ function buildAgentMiddlewares( aiMiddlewares: Middleware[], ): Middleware[] { const { forestAuthSecret, defaultTimezone } = config; - const { environmentId } = oauth; if (!forestAuthSecret) { logger('Warn', 'Agent edge disabled: FOREST_AUTH_SECRET is missing'); @@ -373,7 +399,14 @@ function buildAgentMiddlewares( source, hasAiQueryRoute: aiMiddlewares.length > 0, }), - ...(bundle ? [createContextRoutesMiddleware({ store: bundle.store, environmentId })] : []), + ...(bundle + ? [ + createContextRoutesMiddleware({ + store: bundle.store, + resolveEnvironmentId: toleratedEnvironmentId(oauth, logger), + }), + ] + : []), ...aiMiddlewares, createTimezoneMiddleware({ defaultTimezone }), ...buildAgentRouteMiddlewares(bundle, config, logger), @@ -397,7 +430,7 @@ export default async function buildBff({ }); } - const oauth = await buildOAuthMiddlewares(config, logger); + const oauth = buildOAuthMiddlewares(config, logger); const aiMiddlewares = buildAiMiddlewares(config, oauth, logger); const agentMiddlewares = buildAgentMiddlewares(config, logger, oauth, aiMiddlewares); const agentErrorMiddleware = diff --git a/packages/agent-bff/src/context/context-routes-middleware.ts b/packages/agent-bff/src/context/context-routes-middleware.ts index e3c131d6b1..954483f567 100644 --- a/packages/agent-bff/src/context/context-routes-middleware.ts +++ b/packages/agent-bff/src/context/context-routes-middleware.ts @@ -8,12 +8,16 @@ const CONTEXT_ROUTE = '/agent/v1/context'; export interface ContextRoutesMiddlewareOptions { store: ReadModelStore; - environmentId?: number; + /** + * Resolves to `undefined` rather than rejecting: the id decorates the payload, so a Forest server + * that cannot answer must not take the whole bootstrap route down with it. + */ + resolveEnvironmentId?: () => Promise; } export default function createContextRoutesMiddleware({ store, - environmentId, + resolveEnvironmentId, }: ContextRoutesMiddlewareOptions): Middleware { return async function contextRoutesMiddleware(ctx, next) { if (ctx.path !== CONTEXT_ROUTE || ctx.method !== 'GET') { @@ -23,6 +27,7 @@ export default function createContextRoutesMiddleware({ } const { collections, readModel, revision } = await resolveSchemaSnapshot(store); + const environmentId = await resolveEnvironmentId?.(); ctx.status = 200; ctx.body = buildContext(collections, readModel, { schemaRevision: revision, environmentId }); diff --git a/packages/agent-bff/src/oauth/environment-id.ts b/packages/agent-bff/src/oauth/environment-id.ts new file mode 100644 index 0000000000..191863a25c --- /dev/null +++ b/packages/agent-bff/src/oauth/environment-id.ts @@ -0,0 +1,33 @@ +import type ForestServerClient from './forest-server-client'; + +/** Resolves the Forest environment id, on demand rather than at boot. */ +export type EnvironmentIdResolver = () => Promise; + +/** + * Fetch the environment id at most once per success, and never cache a failure: a Forest server + * that is briefly unreachable must not disable the routes that need the id for the life of the + * process, which is what resolving it at boot did. Concurrent callers share the in-flight fetch. + */ +export default function createEnvironmentIdResolver( + client: Pick, +): EnvironmentIdResolver { + let environmentId: number | undefined; + let inFlight: Promise | null = null; + + return async function resolveEnvironmentId(): Promise { + if (environmentId !== undefined) return environmentId; + + inFlight ??= client + .fetchEnvironmentId() + .then(resolved => { + environmentId = resolved; + + return resolved; + }) + .finally(() => { + inFlight = null; + }); + + return inFlight; + }; +} diff --git a/packages/agent-bff/src/oauth/oauth-routes.ts b/packages/agent-bff/src/oauth/oauth-routes.ts index f1c170b6b4..70fbc6c2e1 100644 --- a/packages/agent-bff/src/oauth/oauth-routes.ts +++ b/packages/agent-bff/src/oauth/oauth-routes.ts @@ -1,3 +1,4 @@ +import type { EnvironmentIdResolver } from './environment-id'; import type ForestServerClient from './forest-server-client'; import type { ServerTokens } from './forest-server-client'; import type { SessionStore } from './session-store'; @@ -16,6 +17,7 @@ import { invalidClient, invalidGrant, invalidRequest, + serverError, sessionExpired, sessionInvalidated, toErrorBody, @@ -32,7 +34,7 @@ export interface OAuthRoutesOptions { sessionStore: SessionStore; forestAppUrl: string; authSecret: string; - environmentId: number; + resolveEnvironmentId: EnvironmentIdResolver; logger: Logger; } @@ -110,6 +112,22 @@ function mapIdentityError(error: unknown): OAuthRequestError { return new OAuthRequestError(502, 'identity_resolution_failed', 'Failed to resolve identity'); } +/** + * The id is only needed once the redirect_uri is trusted, so a resolution failure travels back to + * the client as `server_error` (RFC 6749 §4.1.2.1) instead of a bare 500. + */ +async function resolveEnvironmentId(options: OAuthRoutesOptions): Promise { + try { + return await options.resolveEnvironmentId(); + } catch (error) { + options.logger('Warn', 'Could not resolve the Forest environment id', { + cause: error instanceof Error ? error.message : String(error), + }); + + throw serverError('Could not resolve the Forest environment', error); + } +} + function redirectAuthorizeError( ctx: Context, redirectUri: string, @@ -181,7 +199,7 @@ async function handleAuthorize(ctx: Context, options: OAuthRoutesOptions): Promi authorizeUrl.searchParams.set('code_challenge', codeChallenge); authorizeUrl.searchParams.set('code_challenge_method', 'S256'); authorizeUrl.searchParams.set('state', state); - authorizeUrl.searchParams.set('environmentId', String(options.environmentId)); + authorizeUrl.searchParams.set('environmentId', String(await resolveEnvironmentId(options))); ctx.redirect(authorizeUrl.toString()); } catch (error) { diff --git a/packages/agent-bff/test/ai/ai-routes-middleware.test.ts b/packages/agent-bff/test/ai/ai-routes-middleware.test.ts index 79f90bbf46..01c0544627 100644 --- a/packages/agent-bff/test/ai/ai-routes-middleware.test.ts +++ b/packages/agent-bff/test/ai/ai-routes-middleware.test.ts @@ -70,7 +70,15 @@ function buildApp( await next(); }); - app.use(createAiRoutesMiddleware({ client, sessionStore, serverClient, environmentId, logger })); + app.use( + createAiRoutesMiddleware({ + client, + sessionStore, + serverClient, + resolveEnvironmentId: async () => environmentId, + logger, + }), + ); return { app, sessionStore, logger }; } diff --git a/packages/agent-bff/test/cli-core.test.ts b/packages/agent-bff/test/cli-core.test.ts index b03dd472a0..1f414be9df 100644 --- a/packages/agent-bff/test/cli-core.test.ts +++ b/packages/agent-bff/test/cli-core.test.ts @@ -113,7 +113,6 @@ describe('runCli', () => { } satisfies NodeJS.ProcessEnv; it('should wire OAuth routes (no disabled warning) and boot', async () => { - const fetchEnvironmentId = stubEnvironmentIdFetch(); const logs: string[] = []; const logger: Logger = (_level, message) => { @@ -125,20 +124,35 @@ describe('runCli', () => { try { expect(server).toBeDefined(); expect(logs).not.toContain('OAuth routes disabled: required configuration is missing'); - expect(fetchEnvironmentId).toHaveBeenCalledTimes(1); } finally { await server.stop(); } }); - it('should propagate a fetchEnvironmentId failure out of runCli', async () => { + it('should not reach the Forest server while booting', async () => { + const fetchEnvironmentId = stubEnvironmentIdFetch(); + + const server = await runCli({ ...FULL_ENV }, noopLogger); + + try { + expect(fetchEnvironmentId).not.toHaveBeenCalled(); + } finally { + await server.stop(); + } + }); + + it('should still boot when the Forest server is unreachable', async () => { global.fetch = jest .fn() .mockRejectedValue(new Error('forest server unreachable')) as unknown as typeof fetch; - await expect(runCli({ ...FULL_ENV }, noopLogger)).rejects.toThrow( - 'forest server unreachable', - ); + const server = await runCli({ ...FULL_ENV }, noopLogger); + + try { + expect(server).toBeDefined(); + } finally { + await server.stop(); + } }); }); diff --git a/packages/agent-bff/test/context/context-routes-middleware.test.ts b/packages/agent-bff/test/context/context-routes-middleware.test.ts index 8090e3c391..a0ccd7f442 100644 --- a/packages/agent-bff/test/context/context-routes-middleware.test.ts +++ b/packages/agent-bff/test/context/context-routes-middleware.test.ts @@ -32,7 +32,9 @@ function makeRouteOnlyApp(fetchSchema: jest.Mock, environmentId?: number, now?: const app = new Koa(); app.use(createErrorMiddleware({ logger: () => {} })); - app.use(createContextRoutesMiddleware({ store, environmentId })); + app.use( + createContextRoutesMiddleware({ store, resolveEnvironmentId: async () => environmentId }), + ); return { app, schemaCache }; } diff --git a/packages/agent-bff/test/oauth/environment-id.test.ts b/packages/agent-bff/test/oauth/environment-id.test.ts new file mode 100644 index 0000000000..3e694b7cb8 --- /dev/null +++ b/packages/agent-bff/test/oauth/environment-id.test.ts @@ -0,0 +1,41 @@ +import createEnvironmentIdResolver from '../../src/oauth/environment-id'; + +describe('createEnvironmentIdResolver', () => { + describe('when the fetch succeeds', () => { + it('should fetch once and serve the cached id afterwards', async () => { + const fetchEnvironmentId = jest.fn().mockResolvedValue(42); + const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }); + + await expect(resolve()).resolves.toBe(42); + await expect(resolve()).resolves.toBe(42); + + expect(fetchEnvironmentId).toHaveBeenCalledTimes(1); + }); + }); + + describe('when the fetch fails', () => { + it('should reject and resolve on a later call, since the failure is not cached', async () => { + const fetchEnvironmentId = jest + .fn() + .mockRejectedValueOnce(new Error('forest server unreachable')) + .mockResolvedValue(7); + const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }); + + await expect(resolve()).rejects.toThrow('forest server unreachable'); + await expect(resolve()).resolves.toBe(7); + + expect(fetchEnvironmentId).toHaveBeenCalledTimes(2); + }); + }); + + describe('when two callers arrive before the first fetch settles', () => { + it('should share a single fetch', async () => { + const fetchEnvironmentId = jest.fn().mockResolvedValue(1); + const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }); + + await expect(Promise.all([resolve(), resolve()])).resolves.toEqual([1, 1]); + + expect(fetchEnvironmentId).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/agent-bff/test/oauth/oauth-routes.test.ts b/packages/agent-bff/test/oauth/oauth-routes.test.ts index 71432ef22d..52142efe52 100644 --- a/packages/agent-bff/test/oauth/oauth-routes.test.ts +++ b/packages/agent-bff/test/oauth/oauth-routes.test.ts @@ -85,6 +85,7 @@ function buildApp( serverClient: ForestServerClient, forestAppUrl: string = APP_URL, onTokenRequest?: () => void, + resolveEnvironmentId: () => Promise = async () => 99, ) { const logs: LogLine[] = []; @@ -115,7 +116,7 @@ function buildApp( sessionStore, forestAppUrl, authSecret: AUTH_SECRET, - environmentId: 99, + resolveEnvironmentId, logger, }), ); @@ -156,6 +157,25 @@ describe('oauth-routes GET /oauth/authorize', () => { expect(location.searchParams.get('state')).toBe('state-xyz'); }); + it('should redirect server_error to the client when the environment id cannot be resolved', async () => { + const { app, logs } = buildApp(stubServerClient(), APP_URL, undefined, async () => { + throw new Error('forest server unreachable'); + }); + + const response = await request(app.callback()).get('/oauth/authorize').query(AUTHORIZE_QUERY); + + expect(response.status).toBe(302); + const location = new URL(response.headers.location); + expect(location.origin + location.pathname).toBe(REDIRECT_URI); + expect(location.searchParams.get('error')).toBe('server_error'); + expect(location.searchParams.get('state')).toBe('state-xyz'); + expect(logs).toContainEqual({ + level: 'Warn', + message: 'Could not resolve the Forest environment id', + context: { cause: 'forest server unreachable' }, + }); + }); + it('should preserve a path prefix already present in the Forest app url', async () => { const { app } = buildApp(stubServerClient(), 'https://app.forestadmin.com/prefix'); @@ -610,7 +630,7 @@ describe('oauth-routes POST /oauth/token', () => { sessionStore, forestAppUrl: APP_URL, authSecret: AUTH_SECRET, - environmentId: 99, + resolveEnvironmentId: async () => 99, logger: () => undefined, }), ); From 2718d1bc724642b9336b2a3425a7462937f0fc84 Mon Sep 17 00:00:00 2001 From: Nicolas Bouliol Date: Wed, 2 Sep 2026 16:53:18 +0200 Subject: [PATCH 2/2] fix(agent-bff): bound and classify the environment id resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lazy resolver inherited the client's 60 s request deadline, so a Forest server that accepts the connection and never answers made every context request, every authorize redirect and every AI query pay a minute before answering — the very routes the lazy design exists to keep serving. The read now has a 5 s deadline of its own, and a failure is remembered for 5 s so a burst costs one round trip instead of one each, while still healing on the next request. The AI relay resolved the id inside the try that maps AI proxy failures, so a wrong FOREST_ENV_SECRET was reported as "The Forest server could not be reached" against a proxy that was never contacted, and a resolution timeout came back as 502 network_error. It now resolves before that try, refuses with 502 environment_unresolved (504 on a timeout) and logs the environment read by name. Nothing distinguished a failure that will never resolve from a SaaS blip. fetchEnvironmentId now throws EnvironmentFetchError carrying that verdict, so a refusal from the Forest server is logged at Error and a transport failure stays a Warn. /oauth/authorize logs at Error either way: it still answers 302, so no status-code alert can fire while every login is broken. The context route's tolerating wrapper moves next to the resolver, where the three consumers' deliberately different failure policies are documented in one place, and where it can be tested: the route answering 200 without the field had no coverage. Co-Authored-By: Claude Opus 5 (1M context) --- .../agent-bff/src/ai/ai-routes-middleware.ts | 32 +++++- packages/agent-bff/src/build-bff.ts | 31 +----- .../agent-bff/src/http/bff-local-errors.ts | 8 ++ .../src/oauth/environment-fetch-error.ts | 38 +++++++ .../agent-bff/src/oauth/environment-id.ts | 58 +++++++++- .../src/oauth/forest-server-client.ts | 43 ++++++-- packages/agent-bff/src/oauth/oauth-routes.ts | 6 +- .../test/ai/ai-routes-middleware.test.ts | 55 +++++++++- .../context/context-routes-middleware.test.ts | 37 ++++++- .../test/oauth/environment-id.test.ts | 100 +++++++++++++++++- .../agent-bff/test/oauth/oauth-routes.test.ts | 2 +- 11 files changed, 359 insertions(+), 51 deletions(-) create mode 100644 packages/agent-bff/src/oauth/environment-fetch-error.ts diff --git a/packages/agent-bff/src/ai/ai-routes-middleware.ts b/packages/agent-bff/src/ai/ai-routes-middleware.ts index 7741cd7be9..eba4ad4e24 100644 --- a/packages/agent-bff/src/ai/ai-routes-middleware.ts +++ b/packages/agent-bff/src/ai/ai-routes-middleware.ts @@ -11,11 +11,13 @@ import { AiProxyTimeoutError } from './ai-proxy-client'; import { requireRenderingId } from '../auth/auth-mode'; import { unauthorized } from '../http/bff-http-error'; import { + environmentUnresolved, oauthRequired, upstreamError, upstreamTimeout, upstreamUnreachable, } from '../http/bff-local-errors'; +import { environmentFailureLevel } from '../oauth/environment-fetch-error'; import { OAuthRequestError } from '../oauth/oauth-error'; import ensureFreshServerAccess from '../oauth/session-lifecycle'; @@ -56,6 +58,32 @@ function describeFailure(error: unknown, depthLeft = MAX_CAUSE_DEPTH): string { return `${error.name}: ${clamp(error.message)}${tail}`; } +/** + * Resolved outside the `try` that maps AI proxy failures: a Forest server refusing the environment + * read is not the AI proxy being unreachable, and telling the operator otherwise sends them + * debugging a component that was never contacted. + */ +async function resolveEnvironmentIdOrRefuse( + resolveEnvironmentId: EnvironmentIdResolver | undefined, + logger: Logger, +): Promise { + if (!resolveEnvironmentId) return undefined; + + try { + return await resolveEnvironmentId(); + } catch (error) { + logger( + environmentFailureLevel(error), + 'AI query refused: the Forest environment id could not be resolved', + { cause: describeFailure(error) }, + ); + + if (error instanceof Error && error.name === 'TimeoutError') throw upstreamTimeout(); + + throw environmentUnresolved(); + } +} + async function resolveSessionAccessToken( principal: BffAccessTokenPayload, store: SessionStore, @@ -125,12 +153,14 @@ export default function createAiRoutesMiddleware({ logger, ); + const environmentId = await resolveEnvironmentIdOrRefuse(resolveEnvironmentId, logger); + let response: AiProxyResponse; try { response = await client.query({ saasAccessToken, - environmentId: await resolveEnvironmentId?.(), + environmentId, renderingId, body: ctx.request.body, }); diff --git a/packages/agent-bff/src/build-bff.ts b/packages/agent-bff/src/build-bff.ts index cd2274c26f..9c07a49f77 100644 --- a/packages/agent-bff/src/build-bff.ts +++ b/packages/agent-bff/src/build-bff.ts @@ -31,7 +31,7 @@ import BODY_LIMIT, { AI_BODY_LIMIT } from './http/body-limit'; import createErrorMiddleware from './http/error-middleware'; import createHealthRoute from './http/health-route'; import createVersionHeaderMiddleware from './http/version-header-middleware'; -import createEnvironmentIdResolver from './oauth/environment-id'; +import createEnvironmentIdResolver, { tolerateEnvironmentIdFailure } from './oauth/environment-id'; import ForestServerClient from './oauth/forest-server-client'; import createOAuthRoutes from './oauth/oauth-routes'; import createInMemorySessionStore from './oauth/session-store'; @@ -344,31 +344,6 @@ function buildAiMiddlewares(config: BFFConfig, oauth: OAuthEdge, logger: Logger) ]; } -/** - * The context payload carries the environment id when there is one, and drops it when the Forest - * server cannot be reached — the route itself must keep answering either way. - */ -function toleratedEnvironmentId( - oauth: OAuthEdge, - logger: Logger, -): (() => Promise) | undefined { - const { resolveEnvironmentId } = oauth; - - if (!resolveEnvironmentId) return undefined; - - return async function resolveOrForget(): Promise { - try { - return await resolveEnvironmentId(); - } catch (error) { - logger('Warn', 'Serving the context without an environment id', { - cause: error instanceof Error ? error.message : String(error), - }); - - return undefined; - } - }; -} - function buildAgentMiddlewares( config: BFFConfig, logger: Logger, @@ -403,7 +378,9 @@ function buildAgentMiddlewares( ? [ createContextRoutesMiddleware({ store: bundle.store, - resolveEnvironmentId: toleratedEnvironmentId(oauth, logger), + resolveEnvironmentId: oauth.resolveEnvironmentId + ? tolerateEnvironmentIdFailure(oauth.resolveEnvironmentId, logger) + : undefined, }), ] : []), diff --git a/packages/agent-bff/src/http/bff-local-errors.ts b/packages/agent-bff/src/http/bff-local-errors.ts index 5dafd88646..7f5547ffac 100644 --- a/packages/agent-bff/src/http/bff-local-errors.ts +++ b/packages/agent-bff/src/http/bff-local-errors.ts @@ -89,3 +89,11 @@ export function actionRequiresApproval( ): BffHttpError { return new BffHttpError(403, 'action_requires_approval', message, { details }); } + +export function environmentUnresolved(): BffHttpError { + return new BffHttpError( + 502, + 'environment_unresolved', + 'The Forest environment could not be resolved', + ); +} diff --git a/packages/agent-bff/src/oauth/environment-fetch-error.ts b/packages/agent-bff/src/oauth/environment-fetch-error.ts new file mode 100644 index 0000000000..399e8e17a9 --- /dev/null +++ b/packages/agent-bff/src/oauth/environment-fetch-error.ts @@ -0,0 +1,38 @@ +/** Rate limiting is the one 4xx that heals on its own. */ +const RETRYABLE_CLIENT_STATUS = 429; + +function isPermanentStatus(status: number): boolean { + return status >= 400 && status < 500 && status !== RETRYABLE_CLIENT_STATUS; +} + +/** + * A failed environment id read, told apart by whether retrying can ever help: a Forest server that + * answers and refuses is a configuration error that will never resolve, anything else is transport. + * Consumers log the first at `Error` and the second at `Warn`, which is the only thing that lets an + * operator distinguish a wrong FOREST_ENV_SECRET from a SaaS blip. + */ +export default class EnvironmentFetchError extends Error { + readonly permanent: boolean; + + static fromStatus(status: number, statusText: string): EnvironmentFetchError { + return new EnvironmentFetchError( + `Failed to fetch environment: ${status} ${statusText}`, + isPermanentStatus(status), + ); + } + + constructor(message: string, permanent: boolean) { + super(message); + this.name = 'EnvironmentFetchError'; + this.permanent = permanent; + } +} + +/** Anything that is not a refusal from the Forest server — a socket error, a timeout — may heal. */ +export function isPermanentEnvironmentFailure(error: unknown): boolean { + return error instanceof EnvironmentFetchError && error.permanent; +} + +export function environmentFailureLevel(error: unknown): 'Error' | 'Warn' { + return isPermanentEnvironmentFailure(error) ? 'Error' : 'Warn'; +} diff --git a/packages/agent-bff/src/oauth/environment-id.ts b/packages/agent-bff/src/oauth/environment-id.ts index 191863a25c..dbf411c421 100644 --- a/packages/agent-bff/src/oauth/environment-id.ts +++ b/packages/agent-bff/src/oauth/environment-id.ts @@ -1,29 +1,58 @@ import type ForestServerClient from './forest-server-client'; +import type { Logger } from '../ports/logger-port'; + +import { environmentFailureLevel } from './environment-fetch-error'; /** Resolves the Forest environment id, on demand rather than at boot. */ export type EnvironmentIdResolver = () => Promise; /** - * Fetch the environment id at most once per success, and never cache a failure: a Forest server - * that is briefly unreachable must not disable the routes that need the id for the life of the - * process, which is what resolving it at boot did. Concurrent callers share the in-flight fetch. + * How long a failure is remembered. Short enough that a transient outage still heals on the next + * request — the whole point of resolving lazily — long enough that a burst of requests costs the + * Forest server one round trip instead of one each, and costs the caller one deadline instead of + * one per request. + */ +const FAILURE_TTL_MS = 5_000; + +/** + * Fetch the environment id at most once per success, and hold a failure only for `FAILURE_TTL_MS`: + * a Forest server that is briefly unreachable must not disable the routes that need the id for the + * life of the process, which is what resolving it at boot did. Concurrent callers share the + * in-flight fetch. + * + * The three consumers deliberately disagree on what a failure means, because the id is worth a + * different amount to each: `/oauth/authorize` cannot mint a login without it and surfaces + * `server_error`, the AI relay cannot address the proxy without it and refuses the query, and + * `/agent/v1/context` merely decorates its payload with it and drops the field (see + * `tolerateEnvironmentIdFailure`). */ export default function createEnvironmentIdResolver( client: Pick, + now: () => number = Date.now, ): EnvironmentIdResolver { let environmentId: number | undefined; let inFlight: Promise | null = null; + let failedAt: number | undefined; + let failure: unknown; return async function resolveEnvironmentId(): Promise { if (environmentId !== undefined) return environmentId; + if (failedAt !== undefined && now() - failedAt < FAILURE_TTL_MS) throw failure; inFlight ??= client .fetchEnvironmentId() .then(resolved => { environmentId = resolved; + failedAt = undefined; return resolved; }) + .catch(error => { + failedAt = now(); + failure = error; + + throw error; + }) .finally(() => { inFlight = null; }); @@ -31,3 +60,26 @@ export default function createEnvironmentIdResolver( return inFlight; }; } + +/** + * The context payload carries the environment id when there is one, and drops it when the Forest + * server cannot be reached — the route itself must keep answering either way. A refusal from the + * Forest server is logged at `Error` because it will never resolve on its own; a transport failure + * stays a `Warn`. + */ +export function tolerateEnvironmentIdFailure( + resolveEnvironmentId: EnvironmentIdResolver, + logger: Logger, +): () => Promise { + return async function resolveOrForget(): Promise { + try { + return await resolveEnvironmentId(); + } catch (error) { + logger(environmentFailureLevel(error), 'Serving the context without an environment id', { + cause: error instanceof Error ? error.message : String(error), + }); + + return undefined; + } + }; +} diff --git a/packages/agent-bff/src/oauth/forest-server-client.ts b/packages/agent-bff/src/oauth/forest-server-client.ts index 06fd7989e1..ad3accdaf8 100644 --- a/packages/agent-bff/src/oauth/forest-server-client.ts +++ b/packages/agent-bff/src/oauth/forest-server-client.ts @@ -3,9 +3,10 @@ import type { UserInfo } from '@forestadmin/forestadmin-client'; import createForestAdminClient from '@forestadmin/forestadmin-client'; import jsonwebtoken from 'jsonwebtoken'; +import EnvironmentFetchError from './environment-fetch-error'; import OAuthExchangeError from './oauth-exchange-error'; -export { OAuthExchangeError }; +export { EnvironmentFetchError, OAuthExchangeError }; export interface RegisteredClient { client_id: string; @@ -36,8 +37,20 @@ export interface ForestServerClientOptions { const DEFAULT_HEADERS = { 'Content-Type': 'application/json' } as const; const REQUEST_TIMEOUT_MS = 60_000; -function fetchWithTimeout(url: string, init: RequestInit): Promise { - return fetch(url, { ...init, signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS) }); +/** + * The environment id sits on the critical path of routes that only decorate their payload with it, + * and it is a configuration read rather than a user operation: it gets a deadline of its own, short + * enough that a Forest server which accepts the connection and then never answers costs seconds + * instead of a minute per request. + */ +const ENVIRONMENT_TIMEOUT_MS = 5_000; + +function fetchWithTimeout( + url: string, + init: RequestInit, + timeoutMs = REQUEST_TIMEOUT_MS, +): Promise { + return fetch(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }); } export default class ForestServerClient { @@ -56,26 +69,36 @@ export default class ForestServerClient { } async fetchEnvironmentId(): Promise { - const response = await fetchWithTimeout(this.url('/liana/environment'), { - method: 'GET', - headers: { ...DEFAULT_HEADERS, 'forest-secret-key': this.envSecret }, - }); + const response = await fetchWithTimeout( + this.url('/liana/environment'), + { + method: 'GET', + headers: { ...DEFAULT_HEADERS, 'forest-secret-key': this.envSecret }, + }, + ENVIRONMENT_TIMEOUT_MS, + ); if (!response.ok) { - throw new Error(`Failed to fetch environment: ${response.status} ${response.statusText}`); + throw EnvironmentFetchError.fromStatus(response.status, response.statusText); } const body = (await response.json()) as { data?: { id?: string | number } }; const id = body.data?.id; if (typeof id !== 'number' && (typeof id !== 'string' || !/^\d+$/.test(id))) { - throw new Error('Failed to parse environment id from the Forest server response'); + throw new EnvironmentFetchError( + 'Failed to parse environment id from the Forest server response', + true, + ); } const environmentId = Number(id); if (!Number.isInteger(environmentId) || environmentId <= 0) { - throw new Error('Failed to parse environment id from the Forest server response'); + throw new EnvironmentFetchError( + 'Failed to parse environment id from the Forest server response', + true, + ); } return environmentId; diff --git a/packages/agent-bff/src/oauth/oauth-routes.ts b/packages/agent-bff/src/oauth/oauth-routes.ts index 70fbc6c2e1..61e55d46f3 100644 --- a/packages/agent-bff/src/oauth/oauth-routes.ts +++ b/packages/agent-bff/src/oauth/oauth-routes.ts @@ -114,13 +114,15 @@ function mapIdentityError(error: unknown): OAuthRequestError { /** * The id is only needed once the redirect_uri is trusted, so a resolution failure travels back to - * the client as `server_error` (RFC 6749 §4.1.2.1) instead of a bare 500. + * the client as `server_error` (RFC 6749 §4.1.2.1) instead of a bare 500. Logged at `Error` whatever + * the cause: the route still answers 302, so no status-code alert can fire, and every login is + * broken until the id resolves. */ async function resolveEnvironmentId(options: OAuthRoutesOptions): Promise { try { return await options.resolveEnvironmentId(); } catch (error) { - options.logger('Warn', 'Could not resolve the Forest environment id', { + options.logger('Error', 'Could not resolve the Forest environment id', { cause: error instanceof Error ? error.message : String(error), }); diff --git a/packages/agent-bff/test/ai/ai-routes-middleware.test.ts b/packages/agent-bff/test/ai/ai-routes-middleware.test.ts index 01c0544627..331f8a9f64 100644 --- a/packages/agent-bff/test/ai/ai-routes-middleware.test.ts +++ b/packages/agent-bff/test/ai/ai-routes-middleware.test.ts @@ -12,6 +12,7 @@ import RealAiProxyClient, { AiProxyTimeoutError } from '../../src/ai/ai-proxy-cl import createAiRoutesMiddleware, { AI_QUERY_ROUTE } from '../../src/ai/ai-routes-middleware'; import { AI_BODY_LIMIT } from '../../src/http/body-limit'; import createErrorMiddleware from '../../src/http/error-middleware'; +import EnvironmentFetchError from '../../src/oauth/environment-fetch-error'; import { restoreFetchAfterEach, stubFetch } from '../helpers/fetch-stub'; const SAAS_ACCESS_TOKEN = 'saas-access-token'; @@ -43,6 +44,7 @@ interface AppContext { authMode?: 'oauth' | 'api-key'; renderingId?: string; environmentId?: number; + resolveEnvironmentId?: () => Promise; } function buildApp( @@ -53,6 +55,7 @@ function buildApp( authMode = 'oauth', renderingId = '42', environmentId = 7, + resolveEnvironmentId = async () => environmentId, }: AppContext, ) { const sessionStore = store ?? makeSessionStore({ saasAccessToken: freshAccessToken() }); @@ -75,7 +78,7 @@ function buildApp( client, sessionStore, serverClient, - resolveEnvironmentId: async () => environmentId, + resolveEnvironmentId, logger, }), ); @@ -183,6 +186,56 @@ describe('createAiRoutesMiddleware', () => { }); }); + describe('when the Forest server refuses the environment id read', () => { + it('should answer 502 environment_unresolved and never contact the AI proxy', async () => { + const { app, query } = makeApp({ + resolveEnvironmentId: async () => { + throw EnvironmentFetchError.fromStatus(401, 'Unauthorized'); + }, + }); + + const response = await request(app.callback()).post(AI_QUERY_ROUTE).send({ messages: [] }); + + expect(response.status).toBe(502); + expect(response.body.error.type).toBe('environment_unresolved'); + expect(query).not.toHaveBeenCalled(); + }); + + it('should log at Error naming the environment read, not the AI query', async () => { + const { app, logger } = makeApp({ + resolveEnvironmentId: async () => { + throw EnvironmentFetchError.fromStatus(401, 'Unauthorized'); + }, + }); + + await request(app.callback()).post(AI_QUERY_ROUTE).send({ messages: [] }); + + expect(logger).toHaveBeenCalledWith( + 'Error', + 'AI query refused: the Forest environment id could not be resolved', + { cause: 'EnvironmentFetchError: Failed to fetch environment: 401 Unauthorized' }, + ); + }); + }); + + describe('when the environment id read times out', () => { + it('should answer 504 upstream_timeout rather than a network error', async () => { + const timeout = new Error('The operation was aborted due to timeout'); + timeout.name = 'TimeoutError'; + const { app, query } = makeApp({ + resolveEnvironmentId: async () => { + throw timeout; + }, + }); + + const response = await request(app.callback()).post(AI_QUERY_ROUTE).send({ messages: [] }); + + expect(response.status).toBe(504); + expect(response.body.error.type).toBe('upstream_timeout'); + expect(query).not.toHaveBeenCalled(); + }); + }); + describe('when the session is valid', () => { it('should forward the session access token, not the incoming Authorization header', async () => { const saasAccessToken = freshAccessToken(); diff --git a/packages/agent-bff/test/context/context-routes-middleware.test.ts b/packages/agent-bff/test/context/context-routes-middleware.test.ts index a0ccd7f442..d4b217c115 100644 --- a/packages/agent-bff/test/context/context-routes-middleware.test.ts +++ b/packages/agent-bff/test/context/context-routes-middleware.test.ts @@ -11,6 +11,7 @@ import createAuthModeMiddleware from '../../src/auth/auth-mode-middleware'; import createContextRoutesMiddleware from '../../src/context/context-routes-middleware'; import createPerKeyOriginMiddleware from '../../src/cors/per-key-origin'; import createErrorMiddleware from '../../src/http/error-middleware'; +import { tolerateEnvironmentIdFailure } from '../../src/oauth/environment-id'; import CapabilitiesCache from '../../src/read-model/capabilities-cache'; import ReadModelStore from '../../src/read-model/read-model-store'; import SchemaCache, { ONE_DAY_MS } from '../../src/read-model/schema-cache'; @@ -104,7 +105,7 @@ describe('contextRoutesMiddleware', () => { expect(response.body.meta).toEqual({ schemaRevision: 1 }); }); - it('should carry the environment id the deployment resolved at boot', async () => { + it('should carry the environment id the resolver returns', async () => { const { app } = makeRouteOnlyApp(jest.fn().mockResolvedValue(schema), 42); const response = await request(app.callback()).get(ROUTE); @@ -112,7 +113,7 @@ describe('contextRoutesMiddleware', () => { expect(response.body.meta).toEqual({ schemaRevision: 1, environmentId: 42 }); }); - it('should omit the environment id when the deployment resolved none', async () => { + it('should omit the environment id when the resolver returns none', async () => { const { app } = makeRouteOnlyApp(jest.fn().mockResolvedValue(schema)); const response = await request(app.callback()).get(ROUTE); @@ -120,6 +121,38 @@ describe('contextRoutesMiddleware', () => { expect(response.body.meta).toEqual({ schemaRevision: 1 }); }); + it('should still answer the contract when the environment id cannot be resolved', async () => { + const logs: { level: string; message: string; context?: unknown }[] = []; + const { store } = makeStore(jest.fn().mockResolvedValue(schema)); + + const app = new Koa(); + app.use(createErrorMiddleware({ logger: () => {} })); + app.use( + createContextRoutesMiddleware({ + store, + resolveEnvironmentId: tolerateEnvironmentIdFailure( + async () => { + throw new Error('forest server unreachable'); + }, + (level, message, context) => logs.push({ level, message, context }), + ), + }), + ); + + const response = await request(app.callback()).get(ROUTE); + + expect(response.status).toBe(200); + expect(response.body.meta).toEqual({ schemaRevision: 1 }); + expect(response.body.collections).toHaveLength(4); + expect(logs).toEqual([ + { + level: 'Warn', + message: 'Serving the context without an environment id', + context: { cause: 'forest server unreachable' }, + }, + ]); + }); + it('should fetch the schema once on a cold cache and never again while it stays warm', async () => { const fetchSchema = jest.fn().mockResolvedValue(schema); const { app } = makeRouteOnlyApp(fetchSchema); diff --git a/packages/agent-bff/test/oauth/environment-id.test.ts b/packages/agent-bff/test/oauth/environment-id.test.ts index 3e694b7cb8..e495f97e5f 100644 --- a/packages/agent-bff/test/oauth/environment-id.test.ts +++ b/packages/agent-bff/test/oauth/environment-id.test.ts @@ -1,4 +1,21 @@ -import createEnvironmentIdResolver from '../../src/oauth/environment-id'; +import type { Logger } from '../../src/ports/logger-port'; + +import EnvironmentFetchError from '../../src/oauth/environment-fetch-error'; +import createEnvironmentIdResolver, { + tolerateEnvironmentIdFailure, +} from '../../src/oauth/environment-id'; + +const FAILURE_TTL_MS = 5_000; + +function makeLogger() { + const logs: { level: string; message: string; context?: unknown }[] = []; + + const logger: Logger = (level, message, context) => { + logs.push({ level, message, context }); + }; + + return { logger, logs }; +} describe('createEnvironmentIdResolver', () => { describe('when the fetch succeeds', () => { @@ -14,16 +31,32 @@ describe('createEnvironmentIdResolver', () => { }); describe('when the fetch fails', () => { - it('should reject and resolve on a later call, since the failure is not cached', async () => { + it('should serve the remembered failure without refetching for the whole failure ttl', async () => { + const fetchEnvironmentId = jest + .fn() + .mockRejectedValue(new Error('forest server unreachable')); + let clock = 1_000_000; + const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }, () => clock); + + await expect(resolve()).rejects.toThrow('forest server unreachable'); + clock += FAILURE_TTL_MS - 1; + await expect(resolve()).rejects.toThrow('forest server unreachable'); + + expect(fetchEnvironmentId).toHaveBeenCalledTimes(1); + }); + + it('should retry and resolve once the failure ttl has elapsed, since the failure is not cached for good', async () => { const fetchEnvironmentId = jest .fn() .mockRejectedValueOnce(new Error('forest server unreachable')) .mockResolvedValue(7); - const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }); + let clock = 1_000_000; + const resolve = createEnvironmentIdResolver({ fetchEnvironmentId }, () => clock); await expect(resolve()).rejects.toThrow('forest server unreachable'); - await expect(resolve()).resolves.toBe(7); + clock += FAILURE_TTL_MS; + await expect(resolve()).resolves.toBe(7); expect(fetchEnvironmentId).toHaveBeenCalledTimes(2); }); }); @@ -39,3 +72,62 @@ describe('createEnvironmentIdResolver', () => { }); }); }); + +describe('tolerateEnvironmentIdFailure', () => { + describe('when the resolver succeeds', () => { + it('should serve the id untouched and log nothing', async () => { + const { logger, logs } = makeLogger(); + + await expect(tolerateEnvironmentIdFailure(async () => 42, logger)()).resolves.toBe(42); + expect(logs).toEqual([]); + }); + }); + + describe('when the Forest server refuses the read', () => { + it('should serve no id and log at Error, since a refusal will never resolve on its own', async () => { + const { logger, logs } = makeLogger(); + const resolve = tolerateEnvironmentIdFailure(async () => { + throw EnvironmentFetchError.fromStatus(401, 'Unauthorized'); + }, logger); + + await expect(resolve()).resolves.toBeUndefined(); + expect(logs).toEqual([ + { + level: 'Error', + message: 'Serving the context without an environment id', + context: { cause: 'Failed to fetch environment: 401 Unauthorized' }, + }, + ]); + }); + }); + + describe('when the Forest server cannot be reached', () => { + it('should serve no id and log at Warn, since transport failures heal', async () => { + const { logger, logs } = makeLogger(); + const resolve = tolerateEnvironmentIdFailure(async () => { + throw new Error('forest server unreachable'); + }, logger); + + await expect(resolve()).resolves.toBeUndefined(); + expect(logs).toEqual([ + { + level: 'Warn', + message: 'Serving the context without an environment id', + context: { cause: 'forest server unreachable' }, + }, + ]); + }); + }); + + describe('when a rate limit is what refused the read', () => { + it('should log at Warn, since 429 is the one 4xx that heals', async () => { + const { logger, logs } = makeLogger(); + const resolve = tolerateEnvironmentIdFailure(async () => { + throw EnvironmentFetchError.fromStatus(429, 'Too Many Requests'); + }, logger); + + await expect(resolve()).resolves.toBeUndefined(); + expect(logs[0].level).toBe('Warn'); + }); + }); +}); diff --git a/packages/agent-bff/test/oauth/oauth-routes.test.ts b/packages/agent-bff/test/oauth/oauth-routes.test.ts index 52142efe52..2ceef09962 100644 --- a/packages/agent-bff/test/oauth/oauth-routes.test.ts +++ b/packages/agent-bff/test/oauth/oauth-routes.test.ts @@ -170,7 +170,7 @@ describe('oauth-routes GET /oauth/authorize', () => { expect(location.searchParams.get('error')).toBe('server_error'); expect(location.searchParams.get('state')).toBe('state-xyz'); expect(logs).toContainEqual({ - level: 'Warn', + level: 'Error', message: 'Could not resolve the Forest environment id', context: { cause: 'forest server unreachable' }, });