From e0125efdd090f21896c211edf07807b790c6c35a Mon Sep 17 00:00:00 2001 From: Nicolas Bouliol Date: Tue, 1 Sep 2026 23:35:51 +0200 Subject: [PATCH 1/2] feat(agent-bff): let a host drop what was read from the SaaS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema is cached for 24h and the permissions for 15 minutes, which is the right default when nothing signals a change. A host embedding the BFF has that signal — an agent restarting on a customization refresh — and had no way to act on it. `buildBff` now returns an `invalidate`. Two details make it work rather than look like it does: a fetch already in flight when the invalidation lands is not allowed to repopulate the cache, and the schema is re-read every few seconds for the following minute, because the SaaS the BFF reads from may not have recorded the new schema yet and one refetch would pin the old one for another day. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent-bff/src/build-bff.ts | 33 ++++++-- .../src/read-model/read-model-store.ts | 9 ++ .../agent-bff/src/read-model/schema-cache.ts | 40 ++++++++- packages/agent-bff/test/build-bff.test.ts | 11 +++ .../test/read-model/read-model-store.test.ts | 25 ++++++ .../test/read-model/schema-cache.test.ts | 83 +++++++++++++++++++ 6 files changed, 190 insertions(+), 11 deletions(-) diff --git a/packages/agent-bff/src/build-bff.ts b/packages/agent-bff/src/build-bff.ts index 3a977c2ec9..283be9cdb3 100644 --- a/packages/agent-bff/src/build-bff.ts +++ b/packages/agent-bff/src/build-bff.ts @@ -68,6 +68,11 @@ export interface BuildBffOptions { export interface Bff { callback: BffCallback; + /** + * Forget what was read from the SaaS. A host that knows the schema just moved — an agent + * restarting on a customization refresh — calls this instead of waiting out the 24h TTL. + */ + invalidate(): void; } const SESSION_TTL_SECONDS = 24 * 60 * 60; @@ -318,6 +323,7 @@ function buildAgentRouteMiddlewares( bundle: ReadModelBundle | undefined, config: BFFConfig, logger: Logger, + permissionsCache: PermissionsCache, ): Middleware[] { if (!bundle) { logger( @@ -337,7 +343,7 @@ function buildAgentRouteMiddlewares( forestServerUrl: apiKeyConfig.forestServerUrl, envSecret: apiKeyConfig.forestEnvSecret, }), - cache: new PermissionsCache(), + cache: permissionsCache, logger, }); @@ -381,19 +387,24 @@ function buildAiMiddlewares(config: BFFConfig, oauth: OAuthEdge, logger: Logger) ]; } +interface AgentEdge { + middlewares: Middleware[]; + invalidate(): void; +} + function buildAgentMiddlewares( config: BFFConfig, logger: Logger, oauth: OAuthEdge, aiMiddlewares: Middleware[], basePath: string, -): Middleware[] { +): AgentEdge { const { forestAuthSecret, defaultTimezone } = config; if (!forestAuthSecret) { logger('Warn', 'Agent edge disabled: FOREST_AUTH_SECRET is missing'); - return []; + return { middlewares: [], invalidate: () => undefined }; } const apiKeyStep = buildApiKeyMiddleware(config, logger) ?? createApiKeyUnavailableGuard(logger); @@ -401,6 +412,7 @@ function buildAgentMiddlewares( // AGENT_URL every data path answers 501, so concrete paths would advertise a dead surface. const bundle = resolveReadModelBundle(config, logger); const source = toUnfoldSource(bundle, config, logger); + const permissionsCache = new PermissionsCache(); const chain: Middleware[] = [ createAuthModeMiddleware({ authSecret: forestAuthSecret }), @@ -430,10 +442,16 @@ function buildAgentMiddlewares( : []), ...aiMiddlewares, createTimezoneMiddleware({ defaultTimezone }), - ...buildAgentRouteMiddlewares(bundle, config, logger), + ...buildAgentRouteMiddlewares(bundle, config, logger, permissionsCache), ]; - return chain.map(agentScoped); + return { + middlewares: chain.map(agentScoped), + invalidate: () => { + bundle?.store.invalidate(); + permissionsCache.clear(); + }, + }; } /** @@ -462,7 +480,8 @@ export default async function buildBff({ const oauth = buildOAuthMiddlewares(config, logger); const aiMiddlewares = buildAiMiddlewares(config, oauth, logger); - const agentMiddlewares = buildAgentMiddlewares(config, logger, oauth, aiMiddlewares, mountPath); + const agentEdge = buildAgentMiddlewares(config, logger, oauth, aiMiddlewares, mountPath); + const agentMiddlewares = agentEdge.middlewares; const hasAgentEdge = agentMiddlewares.length > 0; const agentErrorMiddleware = hasAgentEdge ? [agentScoped(createErrorMiddleware({ logger }))] : []; const agentJsonOnlyGuard = hasAgentEdge ? [agentScoped(createJsonOnlyGuard())] : []; @@ -490,5 +509,5 @@ export default async function buildBff({ const app = new Koa(); for (const middleware of middlewares) app.use(middleware); - return { callback: app.callback() }; + return { callback: app.callback(), invalidate: agentEdge.invalidate }; } diff --git a/packages/agent-bff/src/read-model/read-model-store.ts b/packages/agent-bff/src/read-model/read-model-store.ts index 3f3b677d53..76a48b0de1 100644 --- a/packages/agent-bff/src/read-model/read-model-store.ts +++ b/packages/agent-bff/src/read-model/read-model-store.ts @@ -34,6 +34,15 @@ export default class ReadModelStore { return (await this.getSchemaSnapshot()).readModel; } + /** + * Forget the cached schema. The capabilities follow on their own: the next snapshot sees a new + * revision and clears them, which is also what keeps the allow-list and the capabilities from + * splitting across generations. + */ + invalidate(): void { + this.schemaCache.clear(); + } + async getSchemaSnapshot(): Promise { const collections = await this.schemaCache.get(); const { revision } = this.schemaCache; diff --git a/packages/agent-bff/src/read-model/schema-cache.ts b/packages/agent-bff/src/read-model/schema-cache.ts index 3b8351e297..b0fb54a1e2 100644 --- a/packages/agent-bff/src/read-model/schema-cache.ts +++ b/packages/agent-bff/src/read-model/schema-cache.ts @@ -6,6 +6,16 @@ import SchemaUnavailableError from './errors'; export const ONE_DAY_MS = 24 * 60 * 60 * 1000; +/** + * How long after a `clear()` the cache keeps re-reading the schema, and how often. An invalidation + * says "the agent just changed its schema", but the SaaS the BFF reads from may not have finished + * recording it: caching whatever comes back on the very next read would pin the old schema for a + * full day. Within this window a read costs one request every few seconds, which is nothing next to + * serving a collection the agent exposes and the BFF 404s. + */ +export const REVALIDATION_WINDOW_MS = 60 * 1000; +export const REVALIDATION_TTL_MS = 5 * 1000; + export const SCHEMA_CACHE_REFRESH_ERROR = 'schema_cache_refresh_error'; export const SCHEMA_CACHE_AGE_SECONDS = 'schema_cache_age_seconds'; @@ -36,6 +46,8 @@ export default class SchemaCache { private entry: CacheEntry | null = null; private inFlight: Promise | null = null; private revisionValue = 0; + private generation = 0; + private revalidatingUntil = 0; constructor({ fetcher, metrics, now = Date.now, ttlMs = ONE_DAY_MS }: SchemaCacheOptions) { this.fetcher = fetcher; @@ -45,7 +57,7 @@ export default class SchemaCache { } async get(): Promise { - if (this.entry && this.now() - this.entry.fetchedAt < this.ttlMs) { + if (this.entry && this.now() - this.entry.fetchedAt < this.currentTtlMs()) { this.emitAge(); return this.entry.collections; @@ -54,6 +66,20 @@ export default class SchemaCache { return this.refresh(); } + /** + * Drop what is cached and re-read it eagerly for a while. Called when something outside knows the + * schema moved — an agent restarting on a customization refresh, say. + */ + clear(): void { + this.entry = null; + this.generation += 1; + this.revalidatingUntil = this.now() + REVALIDATION_WINDOW_MS; + } + + private currentTtlMs(): number { + return this.now() < this.revalidatingUntil ? REVALIDATION_TTL_MS : this.ttlMs; + } + ageSeconds(): number | undefined { if (!this.entry) return undefined; @@ -75,6 +101,8 @@ export default class SchemaCache { } private async doRefresh(): Promise { + const { generation } = this; + try { const collections = await this.fetcher.fetchSchema(); @@ -83,9 +111,13 @@ export default class SchemaCache { // failed fetch and fall through to the failure path below. if (collections.length === 0) throw new Error('Forest returned an empty schema'); - this.entry = { collections, fetchedAt: this.now() }; - this.revisionValue += 1; - this.emitAge(); + // Skip the write if a clear() happened while this fetch was in flight: it read the schema the + // invalidation declared stale, and caching it now would undo the invalidation. + if (this.generation === generation) { + this.entry = { collections, fetchedAt: this.now() }; + this.revisionValue += 1; + this.emitAge(); + } return collections; } catch (error) { diff --git a/packages/agent-bff/test/build-bff.test.ts b/packages/agent-bff/test/build-bff.test.ts index 5ac7d15cad..5d34b1ca7b 100644 --- a/packages/agent-bff/test/build-bff.test.ts +++ b/packages/agent-bff/test/build-bff.test.ts @@ -175,6 +175,17 @@ describe('buildBff', () => { }); }); + describe('invalidate', () => { + it('should be safe on a deployment whose agent edge is not mounted', async () => { + const bff = await buildBff({ + config: parseConfig({ ...VALID_ENV, FOREST_AUTH_SECRET: undefined }), + logger: noopLogger, + }); + + expect(() => bff.invalidate()).not.toThrow(); + }); + }); + describe('when BFF_ALLOWED_ORIGINS carries a malformed entry', () => { it('should warn once with the rejected entries', async () => { const logger = jest.fn(); diff --git a/packages/agent-bff/test/read-model/read-model-store.test.ts b/packages/agent-bff/test/read-model/read-model-store.test.ts index 5fa5b90d5f..370a11c751 100644 --- a/packages/agent-bff/test/read-model/read-model-store.test.ts +++ b/packages/agent-bff/test/read-model/read-model-store.test.ts @@ -274,4 +274,29 @@ describe('ReadModelStore', () => { expect(store.ageSeconds()).toBe(7); }); }); + + describe('invalidate', () => { + it('should re-read the schema on the next snapshot', async () => { + const fetchSchema = jest.fn().mockResolvedValue(makeSchema('users')); + const store = build(fetchSchema); + await store.getSchemaSnapshot(); + + store.invalidate(); + await store.getSchemaSnapshot(); + + expect(fetchSchema).toHaveBeenCalledTimes(2); + }); + + it('should drop the capabilities with it, since they belong to the schema generation', async () => { + const fetchSchema = jest.fn().mockResolvedValue(makeSchema('users')); + const store = build(fetchSchema); + const capabilities = jest.fn().mockResolvedValue({ fields: [] }); + await store.getCapabilities('users', capabilities); + + store.invalidate(); + await store.getCapabilities('users', capabilities); + + expect(capabilities).toHaveBeenCalledTimes(2); + }); + }); }); diff --git a/packages/agent-bff/test/read-model/schema-cache.test.ts b/packages/agent-bff/test/read-model/schema-cache.test.ts index b0cfc1fe7a..ac6a950967 100644 --- a/packages/agent-bff/test/read-model/schema-cache.test.ts +++ b/packages/agent-bff/test/read-model/schema-cache.test.ts @@ -6,6 +6,8 @@ import { makeMetrics, makeSchema } from './fixtures'; import SchemaUnavailableError from '../../src/read-model/errors'; import SchemaCache, { ONE_DAY_MS, + REVALIDATION_TTL_MS, + REVALIDATION_WINDOW_MS, SCHEMA_CACHE_AGE_SECONDS, SCHEMA_CACHE_REFRESH_ERROR, } from '../../src/read-model/schema-cache'; @@ -242,4 +244,85 @@ describe('SchemaCache', () => { expect(cache.revision).toBe(1); }); }); + + describe('clear', () => { + it('should re-read the schema on the next get', async () => { + const cache = build(); + fetcher.fetchSchema.mockResolvedValue(makeSchema('users')); + await cache.get(); + + cache.clear(); + await cache.get(); + + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(2); + }); + + it('should keep re-reading during the revalidation window, since the SaaS may still be catching up', async () => { + const cache = build(); + fetcher.fetchSchema.mockResolvedValue(makeSchema('users')); + await cache.get(); + cache.clear(); + await cache.get(); + + clock += REVALIDATION_TTL_MS; + await cache.get(); + + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(3); + }); + + it('should go back to the long TTL once the window is over', async () => { + const cache = build(); + fetcher.fetchSchema.mockResolvedValue(makeSchema('users')); + await cache.get(); + cache.clear(); + await cache.get(); + + clock += REVALIDATION_WINDOW_MS; + await cache.get(); + const afterWindow = fetcher.fetchSchema.mock.calls.length; + clock += REVALIDATION_TTL_MS; + await cache.get(); + + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(afterWindow); + }); + + it('should not let a fetch started before the clear repopulate the cache', async () => { + const cache = build(); + const stale = makeSchema('stale'); + let releaseStale: (collections: ForestSchemaCollection[]) => void = () => undefined; + fetcher.fetchSchema.mockReturnValueOnce( + new Promise(resolve => { + releaseStale = resolve; + }), + ); + + const pending = cache.get(); + cache.clear(); + releaseStale(stale); + await pending; + + fetcher.fetchSchema.mockResolvedValue(makeSchema('fresh')); + const result = await cache.get(); + + expect(result).toEqual(makeSchema('fresh')); + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(2); + }); + + it('should not bump the revision for a fetch the clear invalidated', async () => { + const cache = build(); + let releaseStale: (collections: ForestSchemaCollection[]) => void = () => undefined; + fetcher.fetchSchema.mockReturnValueOnce( + new Promise(resolve => { + releaseStale = resolve; + }), + ); + + const pending = cache.get(); + cache.clear(); + releaseStale(makeSchema('stale')); + await pending; + + expect(cache.revision).toBe(0); + }); + }); }); From 7cbaf8a71b5ea8c91ed09b7da5ce1337ada38de2 Mon Sep 17 00:00:00 2001 From: Nicolas Bouliol Date: Wed, 2 Sep 2026 17:03:17 +0200 Subject: [PATCH 2/2] fix(agent-bff): make an invalidation survive its own race and its own failure Review of the invalidation found four holes, all in the gap between "the entry is gone" and "the next read is trustworthy". A read landing after invalidate() joined the fetch that had read the invalidated schema, because clear() left inFlight attached. It is now detached, with an identity-guarded cleanup so the abandoned promise cannot null a newer one. The short TTL was evaluated against the clock at read time, so on a quiet deployment the entry written while the SaaS was still catching up became a 24h entry the moment the window closed - the exact read the window exists to distrust. The TTL is now decided when the entry is written and carried on it. clear() dropped the entry, which also dropped the last good schema the class falls back on. A restart during a SaaS blip took every data route to 503. The entry is now expired rather than removed: reads still all go through a refresh, a failed one still has something to serve. PermissionsCache was the only one of the three caches without a generation guard, and the one with a security consequence: a fetch in flight when the invalidation landed rewrote the shared entry with pre-restart permissions for a full 15 minutes. It now takes the generation read before the fetch, as a required argument so it cannot be omitted. ReadModelStore.invalidate() no longer infers the capabilities drop from a revision change - a skipped write moves no revision - and an invalidation now leaves a log line, next to the refresh failure cause that was being swallowed on its way to a bare error counter. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent-bff/src/build-bff.ts | 3 + .../src/permissions/permissions-cache.ts | 19 +++- .../permissions-routes-middleware.ts | 3 +- .../src/read-model/create-read-model.ts | 2 +- .../src/read-model/read-model-store.ts | 9 +- .../agent-bff/src/read-model/schema-cache.ts | 60 ++++++++-- .../test/build-bff-invalidate.test.ts | 92 ++++++++++++++++ .../permissions/permissions-cache.test.ts | 31 +++++- .../permissions-routes-middleware.test.ts | 49 +++++---- .../test/read-model/read-model-store.test.ts | 17 +++ .../test/read-model/schema-cache.test.ts | 103 +++++++++++++++++- 11 files changed, 340 insertions(+), 48 deletions(-) create mode 100644 packages/agent-bff/test/build-bff-invalidate.test.ts diff --git a/packages/agent-bff/src/build-bff.ts b/packages/agent-bff/src/build-bff.ts index 283be9cdb3..a0650f290a 100644 --- a/packages/agent-bff/src/build-bff.ts +++ b/packages/agent-bff/src/build-bff.ts @@ -448,6 +448,9 @@ function buildAgentMiddlewares( return { middlewares: chain.map(agentScoped), invalidate: () => { + // The one trace an invalidation leaves: the operator asked "the schema is wrong" needs to + // establish that one happened and when, and the age gauge cannot say it. + logger('Info', 'Dropping the SaaS read caches on host request'); bundle?.store.invalidate(); permissionsCache.clear(); }, diff --git a/packages/agent-bff/src/permissions/permissions-cache.ts b/packages/agent-bff/src/permissions/permissions-cache.ts index 587ae8cad3..6dda9be477 100644 --- a/packages/agent-bff/src/permissions/permissions-cache.ts +++ b/packages/agent-bff/src/permissions/permissions-cache.ts @@ -31,6 +31,7 @@ export default class PermissionsCache { private readonly ttlMs: number; private entry: CacheEntry | undefined; + private generationValue = 0; constructor({ now = Date.now, ttlMs = PERMISSIONS_CACHE_TTL_MS }: PermissionsCacheOptions = {}) { this.now = now; @@ -49,7 +50,22 @@ export default class PermissionsCache { return this.getFreshEntry()?.permissions; } - set(permissions: EvaluatedPermissions): void { + /** + * The generation a fetch must be started against. Read it before the fetch and hand it back to + * `set`, so a response that crossed a `clear()` cannot become the shared entry. + */ + get generation(): number { + return this.generationValue; + } + + /** + * Store a fetched payload, unless a `clear()` landed while the fetch was in flight: it read the + * permissions the invalidation declared stale, and with a single shared entry one late write would + * hand a revoked access to every caller for a full TTL. + */ + set(permissions: EvaluatedPermissions, generation: number): void { + if (generation !== this.generationValue) return; + const previous = this.getFreshEntry(); const carriesTheSameUsers = previous !== undefined && sameUsers(previous.permissions.users, permissions.users); @@ -67,6 +83,7 @@ export default class PermissionsCache { clear(): void { this.entry = undefined; + this.generationValue += 1; } private getFreshEntry(): CacheEntry | undefined { diff --git a/packages/agent-bff/src/permissions/permissions-routes-middleware.ts b/packages/agent-bff/src/permissions/permissions-routes-middleware.ts index 4c0b1b778b..c0c6e067ed 100644 --- a/packages/agent-bff/src/permissions/permissions-routes-middleware.ts +++ b/packages/agent-bff/src/permissions/permissions-routes-middleware.ts @@ -70,6 +70,7 @@ async function resolvePermissions({ logger: Logger; }): Promise<{ permissions: EvaluatedPermissions; fromFreshFetch: boolean }> { let resolved: EnvironmentAndUserPermissions; + const { generation } = cache; try { resolved = await client.fetchPermissions(); @@ -90,7 +91,7 @@ async function resolvePermissions({ users: resolved.users, }; - cache.set(permissions); + cache.set(permissions, generation); return { permissions, fromFreshFetch: true }; } diff --git a/packages/agent-bff/src/read-model/create-read-model.ts b/packages/agent-bff/src/read-model/create-read-model.ts index 45c3aec4c4..197795653c 100644 --- a/packages/agent-bff/src/read-model/create-read-model.ts +++ b/packages/agent-bff/src/read-model/create-read-model.ts @@ -32,7 +32,7 @@ export default function createReadModel({ // Wrap once so a throwing metrics backend can never break business logic anywhere in the bundle. const resolvedMetrics = safeMetrics(metrics ?? createConsoleMetrics(logger)); const fetcher = new ForestSchemaClient({ forestServerUrl, envSecret }); - const schemaCache = new SchemaCache({ fetcher, metrics: resolvedMetrics, now }); + const schemaCache = new SchemaCache({ fetcher, metrics: resolvedMetrics, logger, now }); const capabilitiesCache = new CapabilitiesCache({ now }); const store = new ReadModelStore(schemaCache, capabilitiesCache); const actionEndpointResolver = new ActionEndpointResolver( diff --git a/packages/agent-bff/src/read-model/read-model-store.ts b/packages/agent-bff/src/read-model/read-model-store.ts index 76a48b0de1..e3814508b0 100644 --- a/packages/agent-bff/src/read-model/read-model-store.ts +++ b/packages/agent-bff/src/read-model/read-model-store.ts @@ -35,12 +35,15 @@ export default class ReadModelStore { } /** - * Forget the cached schema. The capabilities follow on their own: the next snapshot sees a new - * revision and clears them, which is also what keeps the allow-list and the capabilities from - * splitting across generations. + * Forget the cached schema and the capabilities that belong to it, then force the read-model to be + * rebuilt. The capabilities are dropped here rather than left to the next revision change: a + * `clear()` also drops a schema write that was in flight, and a dropped write moves nothing the + * snapshot can notice, so inferring the invalidation from the revision would miss it. */ invalidate(): void { this.schemaCache.clear(); + this.capabilitiesCache.clear(); + this.builtRevision = -1; } async getSchemaSnapshot(): Promise { diff --git a/packages/agent-bff/src/read-model/schema-cache.ts b/packages/agent-bff/src/read-model/schema-cache.ts index b0fb54a1e2..52aa351ee7 100644 --- a/packages/agent-bff/src/read-model/schema-cache.ts +++ b/packages/agent-bff/src/read-model/schema-cache.ts @@ -1,4 +1,5 @@ import type { SchemaFetcher } from './forest-schema-client'; +import type { Logger } from '../ports/logger-port'; import type { Metrics } from '../ports/metrics-port'; import type { ForestSchemaCollection } from '@forestadmin/forestadmin-client'; @@ -22,6 +23,7 @@ export const SCHEMA_CACHE_AGE_SECONDS = 'schema_cache_age_seconds'; export interface SchemaCacheOptions { fetcher: SchemaFetcher; metrics: Metrics; + logger?: Logger; now?: () => number; ttlMs?: number; } @@ -29,6 +31,12 @@ export interface SchemaCacheOptions { interface CacheEntry { collections: ForestSchemaCollection[]; fetchedAt: number; + /** + * Decided when the entry is written, not when it is read: a read taken while the revalidation + * window was open cannot be trusted for a day, and must not be promoted to a long-lived entry the + * moment the window closes. + */ + expiresAt: number; } /** @@ -40,6 +48,7 @@ interface CacheEntry { export default class SchemaCache { private readonly fetcher: SchemaFetcher; private readonly metrics: Metrics; + private readonly logger: Logger; private readonly now: () => number; private readonly ttlMs: number; @@ -49,15 +58,22 @@ export default class SchemaCache { private generation = 0; private revalidatingUntil = 0; - constructor({ fetcher, metrics, now = Date.now, ttlMs = ONE_DAY_MS }: SchemaCacheOptions) { + constructor({ + fetcher, + metrics, + logger = () => undefined, + now = Date.now, + ttlMs = ONE_DAY_MS, + }: SchemaCacheOptions) { this.fetcher = fetcher; this.metrics = metrics; + this.logger = logger; this.now = now; this.ttlMs = ttlMs; } async get(): Promise { - if (this.entry && this.now() - this.entry.fetchedAt < this.currentTtlMs()) { + if (this.entry && this.now() < this.entry.expiresAt) { this.emitAge(); return this.entry.collections; @@ -67,17 +83,20 @@ export default class SchemaCache { } /** - * Drop what is cached and re-read it eagerly for a while. Called when something outside knows the - * schema moved — an agent restarting on a customization refresh, say. + * Expire what is cached and re-read it eagerly for a while. Called when something outside knows + * the schema moved — an agent restarting on a customization refresh, say. + * + * The entry is expired, not dropped: every read now goes through a refresh, so nothing the host + * declared stale is served while the SaaS answers, but the class keeps its safety net — a refresh + * that fails still has a last good schema to fall back on. Dropping it would turn a restart + * during a SaaS blip into a `503 schema_unavailable` on every data route. */ clear(): void { - this.entry = null; this.generation += 1; + this.inFlight = null; this.revalidatingUntil = this.now() + REVALIDATION_WINDOW_MS; - } - private currentTtlMs(): number { - return this.now() < this.revalidatingUntil ? REVALIDATION_TTL_MS : this.ttlMs; + if (this.entry) this.entry = { ...this.entry, expiresAt: this.now() }; } ageSeconds(): number | undefined { @@ -92,9 +111,13 @@ export default class SchemaCache { private async refresh(): Promise { if (!this.inFlight) { - this.inFlight = this.doRefresh().finally(() => { - this.inFlight = null; + // Identity-guarded, because `clear()` detaches the in-flight fetch: a read that lands after an + // invalidation must start its own, not join the one that read the invalidated schema. + const pending: Promise = this.doRefresh().finally(() => { + if (this.inFlight === pending) this.inFlight = null; }); + + this.inFlight = pending; } return this.inFlight; @@ -114,7 +137,9 @@ export default class SchemaCache { // Skip the write if a clear() happened while this fetch was in flight: it read the schema the // invalidation declared stale, and caching it now would undo the invalidation. if (this.generation === generation) { - this.entry = { collections, fetchedAt: this.now() }; + const fetchedAt = this.now(); + + this.entry = { collections, fetchedAt, expiresAt: fetchedAt + this.ttlFor(fetchedAt) }; this.revisionValue += 1; this.emitAge(); } @@ -122,9 +147,16 @@ export default class SchemaCache { return collections; } catch (error) { this.metrics.increment(SCHEMA_CACHE_REFRESH_ERROR); + // The counter alone cannot tell "the SaaS returned an empty array" from "the SaaS is down" + // from "the env secret is wrong", and the cause is otherwise swallowed: it becomes the `cause` + // of a `SchemaUnavailableError` the error middleware serialises without logging. + this.logger('Warn', 'Schema refresh failed', { + cause: error instanceof Error ? `${error.name}: ${error.message}` : String(error), + servedStale: this.entry !== null, + }); // Warm cache: keep serving the last good schema (stale), do not poison — the next read - // re-attempts because `fetchedAt` is unchanged and the entry stays expired. + // re-attempts because `expiresAt` is unchanged and the entry stays expired. if (this.entry) { this.emitAge(); @@ -136,6 +168,10 @@ export default class SchemaCache { } } + private ttlFor(fetchedAt: number): number { + return fetchedAt < this.revalidatingUntil ? REVALIDATION_TTL_MS : this.ttlMs; + } + private emitAge(): void { const age = this.ageSeconds(); if (age !== undefined) this.metrics.gauge(SCHEMA_CACHE_AGE_SECONDS, age); diff --git a/packages/agent-bff/test/build-bff-invalidate.test.ts b/packages/agent-bff/test/build-bff-invalidate.test.ts new file mode 100644 index 0000000000..03f044f7a0 --- /dev/null +++ b/packages/agent-bff/test/build-bff-invalidate.test.ts @@ -0,0 +1,92 @@ +import type { EvaluatedPermissions } from '../src/permissions/permissions-cache'; +import type { PermissionsRoutesMiddlewareOptions } from '../src/permissions/permissions-routes-middleware'; +import type { Logger } from '../src/ports/logger-port'; + +import buildBff from '../src/build-bff'; +import { restoreFetchAfterEach, stubEnvironmentIdFetch } from './helpers/fetch-stub'; +import { parseConfig } from '../src/config/env-config'; +import createPermissionsRoutesMiddleware from '../src/permissions/permissions-routes-middleware'; + +jest.mock('../src/permissions/permissions-routes-middleware', () => ({ + __esModule: true, + default: jest.fn(() => async (_ctx: unknown, next: () => Promise) => next()), +})); + +const VALID_ENV = { + FOREST_AUTH_SECRET: 'auth-secret', + FOREST_ENV_SECRET: 'env-secret', + FOREST_SERVER_URL: 'https://api.forestadmin.com', + FOREST_APP_URL: 'https://app.forestadmin.com', + AGENT_URL: 'https://agent.example.com', + BFF_TOKEN_ENCRYPTION_KEY: Buffer.alloc(32).toString('base64'), +} satisfies NodeJS.ProcessEnv; + +const PERMISSIONS = { + actionPermissions: { + isDevelopment: false, + actionsGloballyAllowed: new Set(), + actionsByRole: new Map(), + }, + users: [{ id: 1, roleId: 7 }], +} as unknown as EvaluatedPermissions; + +const noopLogger: Logger = () => undefined; + +const mounted = createPermissionsRoutesMiddleware as unknown as jest.Mock; + +function permissionsRouteOptions(): PermissionsRoutesMiddlewareOptions { + return mounted.mock.calls[0][0] as PermissionsRoutesMiddlewareOptions; +} + +describe('buildBff invalidate', () => { + restoreFetchAfterEach(); + + beforeEach(() => { + mounted.mockClear(); + stubEnvironmentIdFetch(); + }); + + describe('when the agent edge is mounted', () => { + it('should drop the entry from the very cache the permissions route reads', async () => { + const bff = await buildBff({ config: parseConfig(VALID_ENV), logger: noopLogger }); + const { cache } = permissionsRouteOptions(); + cache.set(PERMISSIONS, cache.generation); + + bff.invalidate(); + + expect(cache.getFresh()).toBeUndefined(); + }); + + it('should invalidate the very store the permissions route reads', async () => { + const bff = await buildBff({ config: parseConfig(VALID_ENV), logger: noopLogger }); + const { store } = permissionsRouteOptions(); + const invalidate = jest.spyOn(store, 'invalidate'); + + bff.invalidate(); + + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('should log the invalidation, the only trace an operator can correlate against', async () => { + const logger = jest.fn(); + const bff = await buildBff({ config: parseConfig(VALID_ENV), logger }); + logger.mockClear(); + + bff.invalidate(); + + expect(logger).toHaveBeenCalledWith('Info', 'Dropping the SaaS read caches on host request'); + }); + }); + + describe('when the read-model bundle is absent but the auth secret is present', () => { + it('should mount no permissions route and stay safe to invalidate', async () => { + const bff = await buildBff({ + config: parseConfig({ ...VALID_ENV, FOREST_ENV_SECRET: undefined }), + logger: noopLogger, + }); + + expect(() => bff.invalidate()).not.toThrow(); + expect(mounted).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/agent-bff/test/permissions/permissions-cache.test.ts b/packages/agent-bff/test/permissions/permissions-cache.test.ts index 14ee5953fe..2c51d8cac9 100644 --- a/packages/agent-bff/test/permissions/permissions-cache.test.ts +++ b/packages/agent-bff/test/permissions/permissions-cache.test.ts @@ -25,7 +25,7 @@ describe('PermissionsCache', () => { let clock = 1_000; const cache = new PermissionsCache({ now: () => clock }); - cache.set(PERMISSIONS); + cache.set(PERMISSIONS, cache.generation); clock += PERMISSIONS_CACHE_TTL_MS - 1; expect(cache.getFresh()).toBe(PERMISSIONS); @@ -37,7 +37,7 @@ describe('PermissionsCache', () => { let clock = 1_000; const cache = new PermissionsCache({ now: () => clock }); - cache.set(PERMISSIONS); + cache.set(PERMISSIONS, cache.generation); clock += PERMISSIONS_CACHE_TTL_MS; expect(cache.getFresh()).toBeUndefined(); @@ -49,8 +49,8 @@ describe('PermissionsCache', () => { const cache = new PermissionsCache(); const latest = { ...PERMISSIONS }; - cache.set(PERMISSIONS); - cache.set(latest); + cache.set(PERMISSIONS, cache.generation); + cache.set(latest, cache.generation); expect(cache.size).toBe(1); expect(cache.getFresh()).toBe(latest); @@ -61,11 +61,32 @@ describe('PermissionsCache', () => { it('should drop the stored entry', () => { const cache = new PermissionsCache(); - cache.set(PERMISSIONS); + cache.set(PERMISSIONS, cache.generation); cache.clear(); expect(cache.getFresh()).toBeUndefined(); expect(cache.size).toBe(0); }); }); + + describe('when a fetch started before a clear tries to store its result', () => { + it('should refuse the write, since it read the permissions the clear declared stale', () => { + const cache = new PermissionsCache(); + const { generation } = cache; + + cache.clear(); + cache.set(PERMISSIONS, generation); + + expect(cache.getFresh()).toBeUndefined(); + }); + + it('should accept a fetch started after that clear', () => { + const cache = new PermissionsCache(); + + cache.clear(); + cache.set(PERMISSIONS, cache.generation); + + expect(cache.getFresh()).toBe(PERMISSIONS); + }); + }); }); diff --git a/packages/agent-bff/test/permissions/permissions-routes-middleware.test.ts b/packages/agent-bff/test/permissions/permissions-routes-middleware.test.ts index 2a43d4d28a..44831ee50f 100644 --- a/packages/agent-bff/test/permissions/permissions-routes-middleware.test.ts +++ b/packages/agent-bff/test/permissions/permissions-routes-middleware.test.ts @@ -82,6 +82,13 @@ function fetcherOf( }; } +function seed(cache: PermissionsCache, users: UserPermissionV4[]): void { + cache.set( + { actionPermissions: generateActionsFromPermissions(NORMAL_MODE), users }, + cache.generation, + ); +} + function principalMiddleware(id: number | null): Middleware { return async (ctx, next) => { if (id !== null) { @@ -203,10 +210,7 @@ describe('createPermissionsRoutesMiddleware', () => { describe('when a caller absent from the cached users payload calls in', () => { it('should refetch once and serve them rather than returning a stale 403', async () => { const cache = new PermissionsCache(); - cache.set({ - actionPermissions: generateActionsFromPermissions(NORMAL_MODE), - users: [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4], - }); + seed(cache, [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4]); const client = fetcherOf({ environmentPermissions: NORMAL_MODE, users: USERS }); const response = await request(appOf({ client, cache, callerId: VIEWER_ID }).callback()) @@ -239,10 +243,7 @@ describe('createPermissionsRoutesMiddleware', () => { const app = appOf({ client, cache, callerId: VIEWER_ID }); await request(app.callback()).get(ROUTE); - cache.set({ - actionPermissions: generateActionsFromPermissions(NORMAL_MODE), - users: USERS, - }); + seed(cache, USERS); const afterRefresh = await request(app.callback()).get(ROUTE); expect(afterRefresh.status).toBe(200); @@ -293,10 +294,7 @@ describe('createPermissionsRoutesMiddleware', () => { describe('when the SaaS is down and the stale cache does not cover the caller', () => { it('should keep refetching rather than recording a rejection it never confirmed', async () => { const cache = new PermissionsCache(); - cache.set({ - actionPermissions: generateActionsFromPermissions(NORMAL_MODE), - users: [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4], - }); + seed(cache, [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4]); const client = fetcherOf(new Error('SaaS down')); const app = appOf({ client, cache, callerId: VIEWER_ID }); @@ -311,10 +309,7 @@ describe('createPermissionsRoutesMiddleware', () => { describe('when the refetch triggered by an unknown caller fails', () => { it('should keep serving the cached permissions to the callers they cover', async () => { const cache = new PermissionsCache(); - cache.set({ - actionPermissions: generateActionsFromPermissions(NORMAL_MODE), - users: [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4], - }); + seed(cache, [{ id: CALLER_ID, roleId: ADMIN_ROLE } as UserPermissionV4]); const client = fetcherOf(new Error('SaaS down')); const unknown = await request(appOf({ client, cache, callerId: VIEWER_ID }).callback()) @@ -415,10 +410,7 @@ describe('createPermissionsRoutesMiddleware', () => { const cache = new PermissionsCache(); const client: PermissionsFetcher = { fetchPermissions: jest.fn(async () => { - cache.set({ - actionPermissions: generateActionsFromPermissions(NORMAL_MODE), - users: USERS, - }); + seed(cache, USERS); throw new Error('SaaS down'); }), @@ -556,7 +548,7 @@ describe('createPermissionsRoutesMiddleware', () => { }); describe('when the schema is refreshed while the permissions are being fetched', () => { - it('should answer from the refreshed read model, not the superseded one', async () => { + function invalidatedMidFetch() { const cache = new PermissionsCache(); const refreshed = new ReadModel([ collection('orders', [column('id')], [action('Refund order', '/refund')]), @@ -574,11 +566,24 @@ describe('createPermissionsRoutesMiddleware', () => { }), }; + return { cache, client, store }; + } + + it('should answer from the refreshed read model, not the superseded one', async () => { + const { cache, client, store } = invalidatedMidFetch(); + const response = await request(appOf({ client, cache, store }).callback()).get(ROUTE); expect(response.status).toBe(200); expect(Object.keys(response.body.collections)).toEqual(['orders']); - expect(cache.size).toBe(1); + }); + + it('should not repopulate the shared cache with what that fetch read', async () => { + const { cache, client, store } = invalidatedMidFetch(); + + await request(appOf({ client, cache, store }).callback()).get(ROUTE); + + expect(cache.size).toBe(0); }); }); diff --git a/packages/agent-bff/test/read-model/read-model-store.test.ts b/packages/agent-bff/test/read-model/read-model-store.test.ts index 370a11c751..4b3da75769 100644 --- a/packages/agent-bff/test/read-model/read-model-store.test.ts +++ b/packages/agent-bff/test/read-model/read-model-store.test.ts @@ -298,5 +298,22 @@ describe('ReadModelStore', () => { expect(capabilities).toHaveBeenCalledTimes(2); }); + + it('should drop the capabilities even when the revision does not move', async () => { + const fetchSchema = jest + .fn() + .mockResolvedValueOnce(makeSchema('users')) + .mockRejectedValue(new Error('boom')); + const store = build(fetchSchema); + const capabilities = jest.fn().mockResolvedValue({ fields: [] }); + await store.getCapabilities('users', capabilities); + const before = await store.getSchemaSnapshot(); + + store.invalidate(); + await store.getCapabilities('users', capabilities); + + expect((await store.getSchemaSnapshot()).revision).toBe(before.revision); + expect(capabilities).toHaveBeenCalledTimes(2); + }); }); }); diff --git a/packages/agent-bff/test/read-model/schema-cache.test.ts b/packages/agent-bff/test/read-model/schema-cache.test.ts index ac6a950967..ca8b900217 100644 --- a/packages/agent-bff/test/read-model/schema-cache.test.ts +++ b/packages/agent-bff/test/read-model/schema-cache.test.ts @@ -1,3 +1,4 @@ +import type { Logger } from '../../src/ports/logger-port'; import type { Metrics } from '../../src/ports/metrics-port'; import type { SchemaFetcher } from '../../src/read-model/forest-schema-client'; import type { ForestSchemaCollection } from '@forestadmin/forestadmin-client'; @@ -24,8 +25,8 @@ describe('SchemaCache', () => { fetcher = { fetchSchema: jest.fn() }; }); - function build(): SchemaCache { - return new SchemaCache({ fetcher: fetcher as SchemaFetcher, metrics, now }); + function build(logger: Logger = () => undefined): SchemaCache { + return new SchemaCache({ fetcher: fetcher as SchemaFetcher, metrics, logger, now }); } describe('cold cache', () => { @@ -123,6 +124,23 @@ describe('SchemaCache', () => { expect(metrics.increment).toHaveBeenCalledWith(SCHEMA_CACHE_REFRESH_ERROR); }); + it('should log the cause and that the stale schema was served', async () => { + const logger = jest.fn(); + fetcher.fetchSchema + .mockResolvedValueOnce(makeSchema('users')) + .mockRejectedValueOnce(new Error('boom')); + const cache = build(logger); + + await cache.get(); + clock += ONE_DAY_MS; + await cache.get(); + + expect(logger).toHaveBeenCalledWith('Warn', 'Schema refresh failed', { + cause: 'Error: boom', + servedStale: true, + }); + }); + it('should re-attempt on the next read and serve the fresh schema once it succeeds', async () => { const good = makeSchema('users'); const fresh = makeSchema('users-v2'); @@ -188,6 +206,18 @@ describe('SchemaCache', () => { expect(cache.ageSeconds()).toBeUndefined(); }); + it('should log the empty schema as the cause, since the counter cannot tell it from an outage', async () => { + const logger = jest.fn(); + fetcher.fetchSchema.mockResolvedValue([]); + + await expect(build(logger).get()).rejects.toBeInstanceOf(SchemaUnavailableError); + + expect(logger).toHaveBeenCalledWith('Warn', 'Schema refresh failed', { + cause: 'Error: Forest returned an empty schema', + servedStale: false, + }); + }); + it('should keep serving the last good schema when a refresh returns empty', async () => { const good = makeSchema('users'); fetcher.fetchSchema.mockResolvedValueOnce(good).mockResolvedValueOnce([]); @@ -299,7 +329,8 @@ describe('SchemaCache', () => { const pending = cache.get(); cache.clear(); releaseStale(stale); - await pending; + + await expect(pending).resolves.toEqual(stale); fetcher.fetchSchema.mockResolvedValue(makeSchema('fresh')); const result = await cache.get(); @@ -308,6 +339,72 @@ describe('SchemaCache', () => { expect(fetcher.fetchSchema).toHaveBeenCalledTimes(2); }); + it('should start its own fetch for a read that lands after the clear, not join the invalidated one', async () => { + const cache = build(); + const stale = makeSchema('stale'); + let releaseStale: (collections: ForestSchemaCollection[]) => void = () => undefined; + fetcher.fetchSchema.mockReturnValueOnce( + new Promise(resolve => { + releaseStale = resolve; + }), + ); + + const beforeClear = cache.get(); + cache.clear(); + fetcher.fetchSchema.mockResolvedValue(makeSchema('fresh')); + const afterClear = cache.get(); + releaseStale(stale); + + await expect(beforeClear).resolves.toEqual(stale); + await expect(afterClear).resolves.toEqual(makeSchema('fresh')); + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(2); + }); + + it('should not promote a read taken inside the window to the long TTL once the window closes', async () => { + const cache = build(); + fetcher.fetchSchema.mockResolvedValue(makeSchema('users')); + await cache.get(); + + cache.clear(); + clock += REVALIDATION_TTL_MS; + await cache.get(); + clock += REVALIDATION_WINDOW_MS; + await cache.get(); + + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(3); + }); + + it('should keep the last good schema as a fallback when the refresh after a clear fails', async () => { + const good = makeSchema('users'); + fetcher.fetchSchema.mockResolvedValueOnce(good).mockRejectedValue(new Error('boom')); + const cache = build(); + await cache.get(); + + cache.clear(); + const result = await cache.get(); + + expect(result).toBe(good); + expect(metrics.increment).toHaveBeenCalledWith(SCHEMA_CACHE_REFRESH_ERROR); + }); + + it('should keep re-reading after serving that fallback, so the invalidation is not defeated', async () => { + const good = makeSchema('users'); + const fresh = makeSchema('users-v2'); + fetcher.fetchSchema + .mockResolvedValueOnce(good) + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce(fresh); + const cache = build(); + await cache.get(); + + cache.clear(); + await cache.get(); + const result = await cache.get(); + + expect(result).toBe(fresh); + expect(fetcher.fetchSchema).toHaveBeenCalledTimes(3); + }); + it('should not bump the revision for a fetch the clear invalidated', async () => { const cache = build(); let releaseStale: (collections: ForestSchemaCollection[]) => void = () => undefined;