feat(agent-bff): let a host drop what was read from the SaaS - #1874
feat(agent-bff): let a host drop what was read from the SaaS#1874nbouliol wants to merge 2 commits into
Conversation
2 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Coverage Impact Unable to calculate total coverage change because base branch coverage was not found. Modified Files with Diff Coverage (6)
🛟 Help
|
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1076): step 6 is implemented in shape — SchemaCache.clear() with a generation guard, PermissionsCache.clear(), and CapabilitiesCache correctly left alone since the store already drops it on a revision change. The generation guard is real and correct: doRefresh captures the generation on entry and refuses the write if it moved.
Where it falls short of step 6 is the last sentence of that step — "revalidate on a short TTL after an invalidation instead of trusting one refetch: postSchema propagation is not instant, so a single post-restart fetch can re-cache the old schema for another 24h". The window as built only delivers that under sustained traffic; the first finding has the executed trace. Worth knowing before merge, because it is the mechanism the step exists for.
One thing the ticket leaves to you and I want to flag as not a finding: it is silent on the in-flight and empty-result semantics, and on the concrete window/TTL numbers. The 60s/5s pair is your call, not a deviation.
Also checked and clean, so it does not get re-litigated: the window boundary is exclusive and consistent with its test; a second clear() resets the window forward, which is the sensible semantic; the window cannot be entered without a clear(); ttlMs < REVALIDATION_TTL_MS would invert the window but is unreachable, since production never passes ttlMs; and api-key/resolve-cache is rightly left out, its TTLs being seconds.
Claude Opus 5 (claude-opus-5): Preferential
Applies to: the PR as a whole.
Worth an ADR: choosing drop the entry over mark the entry for refresh for the BFF schema cache — accepting a schema_unavailable window when the SaaS is unreachable right after an invalidation, in exchange for never serving a schema the host has declared stale.
It has a genuinely rejected alternative, and the interesting part is that an accepted ADR in forestadmin-server resolved the same trade the other way: docs/adr/2026-08-26-out-of-band-layout-writes-mark-for-refresh-instead-of-deleting-the-cache-row.md chose mark-for-refresh precisely because deleting "looks self-healing but is not". Its failure mode does not reproduce here — a cold read throws a typed error rather than serving a successful empty payload, which is the better half of this design — but the shape of the decision is the same one, and the next person to touch this will want to know it was made knowingly.
| } | ||
|
|
||
| private currentTtlMs(): number { | ||
| return this.now() < this.revalidatingUntil ? REVALIDATION_TTL_MS : this.ttlMs; |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
The revalidation window does not survive its own closing, so on a low-traffic deployment the invalidation is silently defeated and the pre-restart schema is pinned for a full 24h.
The TTL is evaluated at read time against a window that may already have closed, instead of being bound to the entry that was written while it was open. There is no timer in this class — currentTtlMs() is only ever consulted from get() — so revalidation happens only if requests arrive. Transcribed the logic and ran it, with a SaaS that has not finished recording the restart until t=30s:
A - quiet: one request at t=5s, next long after
t= 5000ms ttl= 5000 fetch -> OLD-schema
t= 80000ms ttl=86400000 cache -> OLD-schema
t= 40000000ms ttl=86400000 cache -> OLD-schema fetches=1
B - busy: repeated requests inside the window
t=5s/12s/25s fetch OLD, t=40s fetch NEW, t=80s cache NEW fetches=4
C - no traffic during the window
t=61000ms ttl=86400000 fetch -> single post-restart refetch, 24h TTL
B is the case the design has in mind and it works. A is the one that hurts: the entry written at t=5s — while the window was open and the SaaS was still catching up, i.e. exactly the read the mechanism is built to distrust — becomes a 24h entry the moment the window closes. C reduces to the "single refetch pins the old schema" case step 6 quotes as the thing to avoid. Staging, dev, and a quiet client all land in A or C.
Who observes: the first user opening a collection the customization just added — 404, or a missing field, for up to 24h. Nothing on the operator side, because schema_cache_age_seconds reports a healthy age; the entry exists, it is just from a read that was too early.
Cheapest fix that keeps the design: bind the short TTL to the entry rather than to the clock — remember that the current entry was written during a revalidation window, and force one refresh() on the first get() after the window closes.
On the tests: 'should go back to the long TTL once the window is over' is correct about what it names — the long TTL genuinely is restored. What no test covers is the consequence above, that the entry it now serves is the one written at the start of the window.
There was a problem hiding this comment.
Fixed: the TTL is now bound to the entry — expiresAt is computed at write time from whether the window was open — instead of being evaluated against the clock at read time, so your case A entry written at t=5s expires at t=10s and the read at t=80s refetches; case C is left as designed, since its single refetch happens at least 60s after the invalidation rather than being the immediately-post-restart read the window exists to distrust.
| middlewares: chain.map(agentScoped), | ||
| invalidate: () => { | ||
| bundle?.store.invalidate(); | ||
| permissionsCache.clear(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| */ | ||
| clear(): void { | ||
| this.entry = null; | ||
| this.generation += 1; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * schema moved — an agent restarting on a customization refresh, say. | ||
| */ | ||
| clear(): void { | ||
| this.entry = null; |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
Nulling the entry removes the class's own safety net at the moment it is most likely to be needed. The contract a few lines up is "the last good schema keeps being served until a refresh succeeds", implemented by the warm branch in doRefresh's catch. After a clear() there is nothing warm, so a failing refresh throws SchemaUnavailableError → 503 schema_unavailable on every agent data route, for as long as the SaaS stays unreachable.
The trigger is agent.restart() on a customization refresh — routine, not exceptional, and the moment right after a restart is exactly when the network to the SaaS is least settled. So a host that restarts during a SaaS blip takes its whole agent edge down, where before it would have served a slightly stale schema.
To be fair to the design: this is a deliberate trade and the good half of it is real — a cold read raises a typed error rather than serving a successful empty payload, and doRefresh refuses an empty schema, so there is never a silently-wrong surface. It is a loud window, not a quiet one, and the revalidation window bounds it.
Still worth keeping both properties, which costs a few lines: keep the dropped entry as a stale fallback and only remove its validity, so the invalidation still forces a refetch but a failed refetch does not turn an invalidation into an outage. At minimum, say so in the PR description — it is an availability change a reader would not infer from "let a host drop what was read from the SaaS".
There was a problem hiding this comment.
Adopted mark-for-refresh: clear() now expires the entry instead of dropping it, so every read still goes through a refresh but a failed one falls back on the last good schema rather than 503-ing every data route — same resolution as the forestadmin-server ADR you linked, and the PR description now says so. Two tests pin it: the fallback being served after a failed post-invalidation refresh, and the cache still re-reading afterwards so the invalidation is not quietly defeated.
| return chain.map(agentScoped); | ||
| return { | ||
| middlewares: chain.map(agentScoped), | ||
| invalidate: () => { |
There was a problem hiding this comment.
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');There was a problem hiding this comment.
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.
| ]; | ||
|
|
||
| return chain.map(agentScoped); | ||
| return { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — skills/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.
There was a problem hiding this comment.
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.
|
On the ADR (review body, preferential): no ADR, because the trade got resolved the other way. On the spec gap: the window now delivers under low traffic too, since the short TTL is carried on the entry rather than checked against the clock at read time. Case C is unchanged and intended — with no traffic during the window, the single refetch lands at least 60s after the invalidation, which is the window's premise, not the immediately-post-restart read it exists to distrust. |
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
All eight findings verified on 5395e96: expiresAt is decided at write time so a read taken inside the revalidation window is not promoted to a 24h entry; clear() expires the entry rather than dropping it, keeping the stale fallback, and detaches inFlight with an identity-guarded cleanup; PermissionsCache.set takes a required generation; ReadModelStore.invalidate() clears the capabilities and resets builtRevision itself; the invalidation logs, SchemaCache logs the refresh cause and whether stale was served; build-bff-invalidate.test.ts pins the shared instances. The residual you documented — a read already in flight pairing old collections with new capabilities — is acceptable as a follow-up.
951f8ba to
3dc296d
Compare
5395e96 to
a2b5b2b
Compare
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) <noreply@anthropic.com>
… 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) <noreply@anthropic.com>
a2b5b2b to
cf2a9cc
Compare
2 participants
Stacked on #1873.
Why
The BFF caches the Forest schema for 24h (
schema-cache.ts) and the permission hints for 15 minutes (permissions-cache.ts). Fine when nothing announces a change — but an agent embedding the BFF does get that announcement: it restarts ononRefreshCustomizations. Without a way to act on it, the same process would serve a new collection from the agent and 404 it from the BFF for up to a day.What
buildBffreturnsinvalidate(). It clears the schema cache, the capabilities and the permissions cache.ReadModelStoredrops the capabilities explicitly rather than letting the next revision change do it: aclear()can also discard a schema write that was in flight, and a discarded write moves no revision for the snapshot to notice.Three details do the actual work:
state the invalidation just declared stale.
CapabilitiesCachealready guarded this with ageneration counter;
SchemaCacheandPermissionsCachenow do the same.SchemaCachealsodetaches its in-flight fetch, so a read arriving after
invalidate()returned starts its ownrather than joining the invalidated one, and does not bump its revision for a write it skipped.
postSchemapropagation is not instantaneous, so the read rightafter a restart can legitimately return the old schema — and caching it would pin it for another
24h. For a minute after an invalidation the TTL drops to 5s. That short TTL is decided when the
entry is written, not checked against the clock when it is read: otherwise an entry written
early in the window would be promoted to a 24h entry the moment the window closed, which on a
quiet deployment defeats the whole mechanism.
clear()expires the cached schema instead ofdropping it. Every read still goes through a refresh, so nothing the host declared stale is
served while the SaaS answers — but a refresh that fails still has the last good schema to fall
back on, which is the pre-existing contract of the class. Dropping the entry would take every
agent data route to
503 schema_unavailablewhenever a restart coincided with a SaaS blip, and arestart is routine. This is the same trade as
forestadmin-server's2026-08-26-out-of-band-layout-writes-mark-for-refresh-instead-of-deleting-the-cache-rowADR, resolved the same way.
invalidate()also logs one line, andSchemaCachenow logs why a refresh failed — an emptyschema, an unreachable SaaS and a wrong env secret were one undifferentiated error counter.
Tests
83 suites, 1389 tests.
schema-cachecovers the re-read, the revalidation window and its end, an entry written inside the window not surviving it, the in-flight guard, a read landing after the clear starting its own fetch, the stale fallback on a failed post-invalidation refresh, and the revision not moving for a skipped write.permissions-cacheandpermissions-routes-middlewarecover the generation guard;read-model-storecovers the capabilities being dropped even when the revision does not move;build-bff-invalidatepins the wiring —invalidate()empties the very cache and store the permissions route was handed.Fixes PRD-1076
🤖 Generated with Claude Code
Note
Add
invalidateto agent-bff for dropping SaaS-derived stateinvalidatecallback to the BFF that clears the schema cache, capabilities, and shared permissions cache so a host can discard state after a schema move. Deployments without the auth secret get a no-op callback.PermissionsCacheso a fetch started beforeclearcan no longer repopulate the cache after invalidation.SchemaCache.clearsimilarly advances a generation, expires the existing entry without deleting it, and opens a 1-minute revalidation window during which successful reads use a 5-second TTL.SchemaCachefreshness now uses entry-levelexpiresAtinstead offetchedAt+ TTL, andPermissionsCache.setrequires a generation argument — in-tree callers in permissions-routes-middleware.ts are updated; any out-of-tree callers must pass the captured generation.Macroscope summarized cf2a9cc.