Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 29 additions & 7 deletions packages/agent-bff/src/build-bff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -318,6 +323,7 @@ function buildAgentRouteMiddlewares(
bundle: ReadModelBundle | undefined,
config: BFFConfig,
logger: Logger,
permissionsCache: PermissionsCache,
): Middleware[] {
if (!bundle) {
logger(
Expand All @@ -337,7 +343,7 @@ function buildAgentRouteMiddlewares(
forestServerUrl: apiKeyConfig.forestServerUrl,
envSecret: apiKeyConfig.forestEnvSecret,
}),
cache: new PermissionsCache(),
cache: permissionsCache,
logger,
});

Expand Down Expand Up @@ -381,26 +387,32 @@ 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);
// One store for the whole edge. The document only unfolds when the agent is reachable too: with no
// 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 }),
Expand Down Expand Up @@ -430,10 +442,19 @@ function buildAgentMiddlewares(
: []),
...aiMiddlewares,
createTimezoneMiddleware({ defaultTimezone }),
...buildAgentRouteMiddlewares(bundle, config, logger),
...buildAgentRouteMiddlewares(bundle, config, logger, permissionsCache),
];

return chain.map(agentScoped);
return {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Opus 5 (claude-opus-5): Violates conventionsskills/conventions/testing.md#Cover error and edge paths, not only the happy path

The functional branch of invalidate() has no test. The one added case exercises the other branch — the no-op returned when the agent edge is not mounted — and asserts only not.toThrow(). Nothing anywhere asserts that on a mounted edge invalidate() actually reaches store.invalidate() and permissionsCache.clear().

So the wiring this PR restructured is uncovered: swap permissionsCache.clear() for a no-op, or revert the hoisted PermissionsCache back to one constructed inside buildAgentRouteMiddlewares, and the suite stays green — while every host that calls invalidate() keeps serving permission decisions from before the invalidation until the 15-minute TTL expires. That the cleared instance is the same one handed to the route middlewares is the load-bearing fact of the change, and it is exactly what no test pins.

The second no-op branch is uncovered too: with FOREST_AUTH_SECRET present but the read-model bundle absent, bundle?.store.invalidate() is a silent no-op and only the permissions clear runs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: test/build-bff-invalidate.test.ts captures what createPermissionsRoutesMiddleware was handed and asserts invalidate() empties that exact cache and invalidates that exact store, plus the FOREST_AUTH_SECRET-present-but-no-bundle branch (no permissions route mounted, still safe to invalidate) and the log line. Both mutations you named now fail the suite.

middlewares: chain.map(agentScoped),
invalidate: () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Opus 5 (claude-opus-5): Should fix

An invalidation is a significant state transition and it leaves no trace — no log, no metric — while switching off the one gauge that exists. logger is already in scope on this line and unused.

Worse than absent, the telemetry is misleading: emitAge() publishes nothing when ageSeconds() is undefined, which is the case as soon as entry is null, so schema_cache_age_seconds stops being published after a clear() until the first successful refresh. On a dashboard a gap in that series reads as "process dead" or "scrape lost", not "invalidation in progress".

The operator who gets "the schema is wrong" cannot establish that an invalidation happened, when, or whether the refetch that followed succeeded — the first question they will ask, and the only one the telemetry cannot answer.

Related, same blind spot: the deliberate 'Forest returned an empty schema' string never reaches anyone. It becomes the cause of SchemaUnavailableError, which schemaUnavailable() replaces with a fresh error, which the error middleware serialises without logging. So "the SaaS returned an empty array", "the SaaS is down" and "the env secret is wrong" are one undifferentiated schema_cache_refresh_error counter — minutes of diagnosis versus hours. Injecting the logger that createReadModel already has into SchemaCache and logging once in that catch, with the cause and whether stale was served, covers both halves.

One line here for the invalidation itself:

logger('Info', 'Dropping the SaaS read caches on host request');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both halves fixed: invalidate() logs the line you wrote, and SchemaCache now takes the logger createReadModel already had and logs the cause plus whether stale was served in the refresh catch, so an empty schema, an unreachable SaaS and a wrong env secret are distinguishable. The gauge gap closes as a side effect of keeping the entry (the finding above) — ageSeconds() keeps reporting through an invalidation, and the age it reports is now honestly the age of what would be served if the refetch failed.

// 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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Opus 5 (claude-opus-5): Must fix

Applies to: packages/agent-bff/src/permissions/permissions-routes-middleware.ts:93 (not in this diff) — anchored on the clear this PR adds, which is the half that is guardless.

A restart that revoked an access can leave that user authorised for up to 15 more minutes, and nothing anywhere says so.

resolvePermissions() awaits client.fetchPermissions() then calls cache.set(permissions) unconditionally. Sequence: a request is waiting on that fetch; the host restarts the agent and calls invalidate(), which empties the entry; the in-flight fetch resolves with the pre-restart permission set and writes it with a fresh storedAt, so a full PERMISSIONS_CACHE_TTL_MS of validity. The entry is a single shared one, so one in-flight request repopulates it for every caller, and getFresh() then serves it without refetching. The window is one SaaS round-trip — which is precisely the moment a host calls invalidate(), a restart under traffic.

The mirror case is just as real: an access just granted can stay blocked for 15 minutes via rejectedUserIds → 403 forest_identity_not_allowed, with no hint that the cause was an invalidation that got overtaken.

What makes this a defect rather than a design choice is the asymmetry inside this same commit: SchemaCache was given exactly this guard, and CapabilitiesCache already had one. PermissionsCache is the only one of the three without it, and it is the one with a security consequence.

Same pattern as the schema cache: a generation counter bumped in clear(), captured before the await, checked before set(). The result still goes to the current caller — only the shared write is skipped.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as you described: PermissionsCache carries a generation, clear() bumps it, and set(permissions, generation) takes the generation read before the fetch and skips the shared write when it moved — the current caller still gets its result. The argument is required rather than optional so the guard cannot be forgotten the way it was here; covered at the cache level and through the route.

},
};
}

/**
Expand Down Expand Up @@ -462,7 +483,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())] : [];
Expand Down Expand Up @@ -490,5 +512,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 };
}
19 changes: 18 additions & 1 deletion packages/agent-bff/src/permissions/permissions-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -67,6 +83,7 @@ export default class PermissionsCache {

clear(): void {
this.entry = undefined;
this.generationValue += 1;
}

private getFreshEntry(): CacheEntry | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -90,7 +91,7 @@ async function resolvePermissions({
users: resolved.users,
};

cache.set(permissions);
cache.set(permissions, generation);

return { permissions, fromFreshFetch: true };
}
Expand Down
2 changes: 1 addition & 1 deletion packages/agent-bff/src/read-model/create-read-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
12 changes: 12 additions & 0 deletions packages/agent-bff/src/read-model/read-model-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ export default class ReadModelStore {
return (await this.getSchemaSnapshot()).readModel;
}

/**
* 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();
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
this.capabilitiesCache.clear();
this.builtRevision = -1;
}

async getSchemaSnapshot(): Promise<SchemaSnapshot> {
const collections = await this.schemaCache.get();
const { revision } = this.schemaCache;
Expand Down
86 changes: 77 additions & 9 deletions packages/agent-bff/src/read-model/schema-cache.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,42 @@
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';

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';

export interface SchemaCacheOptions {
fetcher: SchemaFetcher;
metrics: Metrics;
logger?: Logger;
now?: () => number;
ttlMs?: number;
}

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;
}

/**
Expand All @@ -30,22 +48,32 @@ 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;

private entry: CacheEntry | null = null;
private inFlight: Promise<ForestSchemaCollection[]> | null = null;
private revisionValue = 0;

constructor({ fetcher, metrics, now = Date.now, ttlMs = ONE_DAY_MS }: SchemaCacheOptions) {
private generation = 0;
private revalidatingUntil = 0;

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<ForestSchemaCollection[]> {
if (this.entry && this.now() - this.entry.fetchedAt < this.ttlMs) {
if (this.entry && this.now() < this.entry.expiresAt) {
this.emitAge();

return this.entry.collections;
Expand All @@ -54,6 +82,23 @@ export default class SchemaCache {
return this.refresh();
}

/**
* 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.generation += 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Opus 5 (claude-opus-5): Should fix

Applies to: packages/agent-bff/src/read-model/schema-cache.ts:93-101 (refresh(), not changed by this diff) — anchored on the clear() that does not reset it.

A read that arrives strictly after invalidate() has returned is served the schema the invalidation just declared stale.

refresh() keys reuse on inFlight alone, and clear() does not drop it. So: request A misses, starts doRefresh() capturing generation 0; invalidate() runs, entry nulled, generation 1; request B misses (entry is null) and finds inFlight non-null, so it joins A's promise — the one that has already decided not to write. The fetch resolves with the pre-restart schema; the write is correctly skipped, but return collections is outside the guard, so both A and B get the stale array. Secondary cost: the entry stays null, so a third read pays another fetch.

B is a request the host would expect to see the new schema, since it arrived after the invalidation completed. The symptom is the one the feature exists to remove — a collection the restarted agent now exposes still 404s.

Stamp the in-flight promise with the generation that created it and start a fresh fetch when it no longer matches.

Untested: 'should not let a fetch started before the clear repopulate the cache' awaits pending without asserting what it resolved with, and no test issues a second get() between the clear() and the release. Adding that second get() fails today.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: clear() now detaches inFlight, with an identity-guarded cleanup in refresh() so the abandoned promise cannot null a newer one — request B starts its own fetch and gets the post-invalidation schema. The existing test now asserts what the pre-clear read resolved with, and a new one issues that second get() between the clear and the release; removing the detach makes it fail.

this.inFlight = null;
this.revalidatingUntil = this.now() + REVALIDATION_WINDOW_MS;

if (this.entry) this.entry = { ...this.entry, expiresAt: this.now() };
}

ageSeconds(): number | undefined {
if (!this.entry) return undefined;

Expand All @@ -66,15 +111,21 @@ export default class SchemaCache {

private async refresh(): Promise<ForestSchemaCollection[]> {
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<ForestSchemaCollection[]> = this.doRefresh().finally(() => {
if (this.inFlight === pending) this.inFlight = null;
});

this.inFlight = pending;
}

return this.inFlight;
}

private async doRefresh(): Promise<ForestSchemaCollection[]> {
const { generation } = this;

try {
const collections = await this.fetcher.fetchSchema();

Expand All @@ -83,16 +134,29 @@ 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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Opus 5 (claude-opus-5): Should fix

Applies to: packages/agent-bff/src/read-model/read-model-store.ts:86 (not in this diff) — anchored on the guard that now skips the revision bump.

This PR introduces a generation change that leaves revision untouched, and revision is the one thing ReadModelStore uses to notice a generation change. Its own comment states the contract: "The revision is the discriminator, not the read-model identity", and the retry guard tests only this.schemaCache.revision !== revision.

Because revisionValue += 1 sits inside this generation check, the skipped-write path bumps nothing. Continuing the in-flight sequence above, inside getCapabilities: getReadModel() resolves through the skipped write and returns the old read model (revision unchanged, so no rebuild and no capabilitiesCache.clear()); capabilities are then fetched from the restarted agent and cached; the guard sees an unchanged revision and does not retry. The caller gets new-generation capabilities paired with an old-generation read model — the allow-list and primary keys — which is the split-brain the store's class doc claims it prevents.

It self-heals on the next request, so this is not perpetuating, but the documented invariant is broken for that request and the comment that guarantees it is now wrong. That comment's reasoning assumed the only source of a dropped write was a refresh, which always bumps the revision; clear() adds a second source that does not.

Shortest honest fix: have ReadModelStore.invalidate() clear the capabilities itself and force the rebuild, rather than inferring it from the revision — this.capabilitiesCache.clear(); this.builtRevision = -1; — and correct the comment. That also removes the warm-cache case where no rebuild happens at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed your way: ReadModelStore.invalidate() now clears the capabilities itself and resets builtRevision, so the drop no longer rides on a revision change, and the comment says that. One residual I could not close cheaply: the request whose schema read was already in flight still pairs its pre-invalidation collections with capabilities from the restarted agent — no post-hoc guard sees it, since the generation had already moved by the time its read-model resolved. Closing that means having get() re-read on a generation change rather than returning what it read, which is a larger change than this fix.

const fetchedAt = this.now();

this.entry = { collections, fetchedAt, expiresAt: fetchedAt + this.ttlFor(fetchedAt) };
this.revisionValue += 1;
this.emitAge();
}

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();

Expand All @@ -104,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);
Expand Down
Loading
Loading