fix(agent-bff): resolve the environment id on demand, not at boot - #1871
fix(agent-bff): resolve the environment id on demand, not at boot#1871nbouliol wants to merge 2 commits into
Conversation
`fetchEnvironmentId()` was awaited while assembling the OAuth edge, so it was the only fatal boot error of a server whose stated posture is to boot and report gaps through /health. A Forest server unreachable for five seconds at startup left OAuth and the AI relay unmounted for the life of the process. The id is now resolved on the first request that needs it, cached on success and never on failure, so a transient outage heals itself. On /oauth/authorize a failed resolution travels back to the client as `server_error`, which is what RFC 6749 asks for once the redirect_uri is trusted; the context route keeps answering and just drops the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 new issue
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (8)
🛟 Help
|
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1076): conforms to step 2. The eager await serverClient.fetchEnvironmentId() is gone from every path — buildOAuthMiddlewares is no longer async and fetchEnvironmentId has exactly one caller left — and the resolver is threaded through all three consumers. Two clarifications on things that look like gaps and are not: the ticket's "answers 502 on that request" and this PR's server_error redirect are the same object (serverError() builds an OAuthRequestError(502, 'server_error', …)), so there is no divergence there; and the per-consumer failure policy for the AI and context routes is this PR's own design — the ticket only says those two "already take it optional" and is silent on what they do when resolution fails.
The resolver itself is right, and I checked the three things that usually go wrong in this shape: inFlight ??= is assigned synchronously before any await, so concurrent callers share one fetch; .finally clears the slot, so no rejection is ever cached; and the guard is !== undefined rather than truthiness, which combined with forest-server-client.ts:77 rejecting <= 0 means no falsy id can reach a JWT payload or the authorize URL. No unhandled rejection on any path.
What the findings are about is not the mechanism but its cost under failure, and the fact that the one genuinely new branch has no test.
Six findings inline, one below.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: the PR as a whole.
This PR decides a failure policy that now differs per consumer — boot no longer fails at all, the context route degrades by dropping the id, /oauth/authorize surfaces server_error, and the AI route relays an upstream error. Three policies for one value, decided here, recorded in no ADR: the ADR search returns nothing applicable for this area.
All three gates are met — it is hard to reverse (operators and front clients build on these semantics), surprising without context (nothing in the code explains why the three consumers differ), and a real trade-off that the ticket itself argues explicitly (self-healing against a loud boot failure). Worth a pass of /adr so the next person changing one of the three knows the other two were deliberate.
|
|
||
| return resolved; | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
While the Forest server hangs — a black-holed connection, a load balancer that never answers, exactly the "briefly unreachable" case the comment above targets — every GET /agent/v1/context blocks for up to 60 s before answering 200 without the id, every /oauth/authorize blocks 60 s before redirecting, and every AI query burns 60 s before the AI proxy is even contacted. The route this design exists to keep answering takes a minute to answer.
Mechanism: the resolver has no deadline of its own and inherits REQUEST_TIMEOUT_MS = 60_000 (forest-server-client.ts:37). Never caching a failure is the right call against bricking, but with no failure memory the cost is paid per request, and the in-flight sharing only covers concurrent callers — two sequential bootstraps pay 60 s each. Each one also emits another identical Warn, so the log fills at request rate.
ECONNREFUSED fails fast, so this is invisible in the common case; nothing bounds the case that hangs.
Two fixes, either works: give this lookup its own short deadline (it is a configuration read, not a user operation — a few seconds), or add a short negative TTL (5–10 s) so a burst pays one round trip instead of one each. The negative TTL additionally protects the SaaS from one retry per request.
There was a problem hiding this comment.
Fixed both ways in 2718d1b: fetchEnvironmentId now carries its own ENVIRONMENT_TIMEOUT_MS = 5_000 instead of inheriting the 60 s request deadline, and the resolver remembers a failure for FAILURE_TTL_MS = 5_000 so a burst pays one round trip instead of one each while still healing on the next request — the log line stays per refused request on purpose, since a request that was refused deserves a trace.
| response = await client.query({ | ||
| saasAccessToken, | ||
| environmentId, | ||
| environmentId: await resolveEnvironmentId?.(), |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
With a wrong FOREST_ENV_SECRET, the client is told The Forest server could not be reached and the log line names the AI query — so the operator debugs the AI proxy, which was never contacted. The real failure is Failed to fetch environment: 401 Unauthorized (forest-server-client.ts:65): the Forest server is reachable and is refusing. The message is not imprecise, it is wrong.
Mechanism: this await sits in client.query's argument list, therefore inside the try opened on the line above. A resolver rejection lands in the catch below, which logs AI query failed against the Forest server and throws upstreamUnreachable() — 502 network_error (bff-local-errors.ts:72). At the merge base environmentId was a plain number and this try covered only client.query.
Secondary, same cause: a resolver timeout is a DOMException named TimeoutError, not an AiProxyTimeoutError, so the instanceof check misses it and a 60 s environment-id stall returns 502 network_error instead of 504 upstream_timeout.
Resolve before the try (or in its own try) and give it its own message, the way the OAuth path already does.
There was a problem hiding this comment.
Fixed in 2718d1b: the resolution moved out of that try into resolveEnvironmentIdOrRefuse, which logs AI query refused: the Forest environment id could not be resolved and throws 502 environment_unresolved, or 504 upstream_timeout when the failure is a TimeoutError — so the AI proxy is no longer blamed for a Forest server refusal.
|
|
||
| return async function resolveOrForget(): Promise<number | undefined> { | ||
| try { | ||
| return await resolveEnvironmentId(); |
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
This catch is the only thing standing between a Forest server outage and a broken front bootstrap, and nothing exercises it. git grep 'Serving the context without an environment id' at this SHA hits this definition only.
The branch is reachable: fetchEnvironmentId rejects on an unreachable fetch and on !response.ok, the resolver propagates it with no internal catch, and test/cli-core.test.ts now proves the process boots with global.fetch rejecting — so the context route is mounted and live with a rejecting resolver. The catch fires on the first GET /agent/v1/context in that state.
What no test asserts: that the route answers 200 with meta carrying schemaRevision and no environmentId, and that the Warn carries the cause. test/context/context-routes-middleware.test.ts:36 always wires a resolver that resolves, and cli-core.test.ts stops at expect(server).toBeDefined() without issuing a request. That suite already wires createErrorMiddleware, so the missing case is a few lines — test/oauth/oauth-routes.test.ts:160-176 does exactly this for the OAuth path and is the model.
There was a problem hiding this comment.
Fixed in 2718d1b: the wrapper moved to environment-id.ts as tolerateEnvironmentIdFailure, and context-routes-middleware.test.ts now wires it over a rejecting resolver and asserts the route answers 200 with meta carrying only schemaRevision, the four collections, and the log line with its cause — plus three level-mapping cases in environment-id.test.ts.
|
|
||
| const serverClient = new ForestServerClient({ forestServerUrl, envSecret: forestEnvSecret }); | ||
| const environmentId = await serverClient.fetchEnvironmentId(); | ||
| const resolveEnvironmentId = createEnvironmentIdResolver(serverClient); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
To be clear about what this is not: that boot no longer fails is deliberate and the ticket argues it well, so no objection there. The gap is that nothing replaced the diagnostic. A permanently wrong FOREST_ENV_SECRET (a 401, which will never succeed) and a 30-second SaaS blip produce the same Warn line, at request rate, and telling them apart means reading the cause field of each occurrence. The ticket is silent on these semantics, so this is a choice this PR makes rather than one it inherits.
An operator watching a deployment therefore has no signal that distinguishes "this environment will never resolve" from "the SaaS wobbled".
Proportionate fix that keeps the lazy behaviour intact: fire one eager resolution at boot whose only job is to log — .catch attached so it cannot become an unhandled rejection, Error level on a 4xx, Warn on a network failure. Nothing becomes fatal; the deployment just says once what it found.
There was a problem hiding this comment.
The diagnostic gap is real and fixed in 2718d1b, but not with an eager boot resolution: that reintroduces the boot-time network call this PR exists to remove, and cli-core.test.ts:133-138 asserts the boot makes none. Instead the failure now carries its own verdict — fetchEnvironmentId throws EnvironmentFetchError marked permanent on a 4xx other than 429 and on an unparseable body — so a wrong FOREST_ENV_SECRET logs at Error and a SaaS blip stays a Warn, which is the signal an operator was missing, and the negative TTL keeps it from repeating at request rate.
| app.use(createErrorMiddleware({ logger: () => {} })); | ||
| app.use(createContextRoutesMiddleware({ store, environmentId })); | ||
| app.use( | ||
| createContextRoutesMiddleware({ store, resolveEnvironmentId: async () => environmentId }), |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/testing.md#Test name states the exact behavior it asserts
Applies to: packages/agent-bff/test/context/context-routes-middleware.test.ts:107 (not in this diff) — anchored on the wiring this PR changed, which is what makes the name below stale.
Line 107 still reads it('should carry the environment id the deployment resolved at boot', …), in the PR titled not at boot. This line changed its wiring from a boot-time value to a per-request resolver, so "resolved at boot" is the one thing the test no longer proves.
Consequence: the file someone opens to learn how the context payload gets its environment id documents the semantics this PR deletes — and the next person reasoning about caching or refresh reads "at boot" as the contract. Line 115 (when the deployment resolved none) carries the same flavour, less wrongly.
There was a problem hiding this comment.
Fixed in 2718d1b: renamed to should carry the environment id the resolver returns and should omit the environment id when the resolver returns none.
| try { | ||
| return await options.resolveEnvironmentId(); | ||
| } catch (error) { | ||
| options.logger('Warn', 'Could not resolve the Forest environment id', { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential
The redirect itself is right — RFC 6749 §4.1.2.1, and the id is only needed once the redirect_uri is trusted, exactly as the comment says. The consequence worth accepting knowingly: /oauth/authorize then answers 302, so any uptime check or error-rate alert keyed on status codes sees a healthy route while every login is broken, and the only server-side trace is this Warn.
A login that cannot start is not a warning. Error here costs nothing and is the difference between an incident someone notices and one a user reports.
There was a problem hiding this comment.
Agreed and changed in 2718d1b: oauth-routes.ts:125 logs at Error whatever the cause, with the comment saying why — the 302 means no status-code alert can fire while every login is broken.
The lazy resolver inherited the client's 60 s request deadline, so a Forest server that accepts the connection and never answers made every context request, every authorize redirect and every AI query pay a minute before answering — the very routes the lazy design exists to keep serving. The read now has a 5 s deadline of its own, and a failure is remembered for 5 s so a burst costs one round trip instead of one each, while still healing on the next request. The AI relay resolved the id inside the try that maps AI proxy failures, so a wrong FOREST_ENV_SECRET was reported as "The Forest server could not be reached" against a proxy that was never contacted, and a resolution timeout came back as 502 network_error. It now resolves before that try, refuses with 502 environment_unresolved (504 on a timeout) and logs the environment read by name. Nothing distinguished a failure that will never resolve from a SaaS blip. fetchEnvironmentId now throws EnvironmentFetchError carrying that verdict, so a refusal from the Forest server is logged at Error and a transport failure stays a Warn. /oauth/authorize logs at Error either way: it still answers 302, so no status-code alert can fire while every login is broken. The context route's tolerating wrapper moves next to the resolver, where the three consumers' deliberately different failure policies are documented in one place, and where it can be tested: the route answering 200 without the field had no coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On the ADR finding: the repository has no ADR practice to add to — The substance of the finding is addressed in code instead, in 2718d1b: the three failure policies are now documented together in |
2 participants
Stacked on #1870.
Why
buildOAuthMiddlewaresawaitedserverClient.fetchEnvironmentId()while assembling the edge. That made one network call the only fatal boot error of a server whose documented posture is the opposite — "a required var that is absent does not crash the server, it boots and reports the gap through /health".Worse, the failure mode is silent and permanent: a Forest server unreachable for five seconds at startup leaves OAuth and the AI relay unmounted for the life of the process, with nothing to restart since the process is otherwise healthy.
What
createEnvironmentIdResolverresolves on first use, caches on success, and never caches a failure — concurrent callers share the in-flight fetch. A transient outage now heals itself on the next request.The three consumers take a resolver instead of a number:
/oauth/authorize— a failed resolution is turned intoserver_errorand redirected back to the client, which is what RFC 6749 §4.1.2.1 asks for once the redirect_uri is validated (it is, several lines earlier), rather than a bare 500./agent/v1/contextkeeps answering and drops the field, with aWarn: the id decorates the payload, it is not what the route is for.This changes the standalone BFF too, on purpose: one behavior, one code path.
createOAuthRoutesis a public export and its options change accordingly.Tests
81 suites, 1355 tests. Two
cli-coretests asserted the old eager fetch and were rewritten to the new intent — the boot makes no network call, and an unreachable Forest server no longer prevents booting. Newenvironment-idsuite covers cache-on-success, retry-after-failure and in-flight sharing;oauth-routescovers theserver_errorredirect.Fixes PRD-1076
🤖 Generated with Claude Code
Note
Resolve environment id on demand instead of at boot in agent-bff
createEnvironmentIdResolverin environment-id.ts: a lazy, memoized resolver that caches successful fetches, does not cache failures, and deduplicates concurrent in-flight callsbuildOAuthMiddlewaresfrom async to sync and removes the eagerfetchEnvironmentId()call at boot; all middlewares now receiveresolveEnvironmentId(a resolver function) instead of a staticenvironmentIdnumbertoleratedEnvironmentIdwraps the resolver so AI and context routes log a warning and proceed withundefinedwhen resolution fails, rather than throwing/authorizehandler resolves the environment id per request and translates failures into OAuthserver_errorsemantics with loggingbuildContextandclient.querycallers handleenvironmentId: undefinedChanges since #1871 opened
Macroscope summarized a5fcc1b.