Conversation
3 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } catch (error) { | ||
| // The document may not exist yet when the transition lands, and only then is a retry worth | ||
| // anything: a network failure loses the transition permanently. | ||
| if (error instanceof NotFoundError && attempt < MAX_STATUS_ATTEMPTS) { |
There was a problem hiding this comment.
🟡 Medium activity-log/activity-logs-creator.ts:183
When updateActivityLogStatus encounters a transient transport or 5xx failure, the activity log remains permanently pending even though the audited operation has finished, corrupting audit status and action-failure statistics. updateStatus retries only NotFoundError, so these recoverable failures reach the fire-and-forget catcher immediately; add bounded retries for transient status-update failures while preserving non-retryable errors.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/activity-log/activity-logs-creator.ts around line 183:
When `updateActivityLogStatus` encounters a transient transport or 5xx failure, the activity log remains permanently `pending` even though the audited operation has finished, corrupting audit status and action-failure statistics. `updateStatus` retries only `NotFoundError`, so these recoverable failures reach the fire-and-forget catcher immediately; add bounded retries for transient status-update failures while preserving non-retryable errors.
The BFF wrote no activity log at all, so a user fetching data or triggering an action through it left no audit trail. Wrap list, relation list and action execute with the mcp-server pattern: a pending log awaited before the operation, a fire-and-forget status transition after it, blocking a write whose log cannot be created and proceeding on a read. A lazy resolver lands the Forest server bearer for both auth modes in one place, and the drain reachable through stop() keeps a status transition from dying with the process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
EmbeddedBff.stop() dropped the callback and returned, so the activity-log status transitions the BFF fires without await died with the process and left their entries pending. The standalone deployment reaches the drain through its own stop(); the embedded one had no path to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8493042 to
0df7cec
Compare
| this.bff = null; | ||
| this.stopped = true; | ||
|
|
||
| await bff?.drainActivityLogs?.(); |
There was a problem hiding this comment.
🟡 Medium src/embedded-bff.ts:170
stop() can return while an already-dispatched audited request is still running, leaving its activity log permanently pending. The request registers its status transition only when operation() completes, but drainActivityLogs() snapshots before that registration; wait for in-flight requests to quiesce before draining, or make the drainer wait for later registrations.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent/src/embedded-bff.ts around line 170:
`stop()` can return while an already-dispatched audited request is still running, leaving its activity log permanently `pending`. The request registers its status transition only when `operation()` completes, but `drainActivityLogs()` snapshots before that registration; wait for in-flight requests to quiesce before draining, or make the drainer wait for later registrations.
… the search the agent runs The token resolver turned every ensureFreshServerAccess failure into session_expired, so a Forest server blip logged OAuth users out instead of failing the audit write alone. Only a 401 means re-authenticating helps; anything else is now audit_unavailable, which is what the write path already knows how to report. A whitespace-only search was recorded as a search although buildListAgentQuery drops it, so a filter ran while the trail claimed a search. Both now ask the same predicate. The drainer settled one snapshot, but a transition is registered only once its request finishes, so an embedded stop() could return before work it should have waited for. It tracks the requests too, and loops until nothing is left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An expired api-key token answered 401, which the refusal check read as a rejection and turned into 403, taking the read surface down for the whole resolve-cache window. Only a 403 is a refusal now, and a 401 drops the cached identity so the next request re-resolves. A missing saasAccessToken no longer advertises a retry that cannot succeed, the audit failure logs name the rendering and the entry, the write path logs before it throws, and the token resolver reports the cause it was hiding behind a mapped error. A blank filter stops counting as a filtered read, the pending-log guard checks the index the transition needs, and stop() is bounded so one busy connection cannot hold the drain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1150): conforms. The three audited routes, the action selection, the approval special case, the two new errors and the lazy two-mode resolver all match the ticket, and count / relation count / form stay unaudited as it asks.
stop() spends one deadline across the connection close and the drain, so a slow audit store can no longer hold the process past it, and what was still in flight is named when it expires. A Forest server that mints no audit credential is reported once at Warn instead of an Error and a Warn, an absent audit route answers without a retry hint, an empty id or index fails the guard like a missing one, and an audit 401 invalidates a key at most once per cache window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
All six findings from the previous round are verified closed on this head, each with its regression pinned by a new case: the shared shutdown deadline, the single Warn for a deployment that mints no audit credential, the absent audit route answering without a retry hint, the empty id or index failing the guard, and the throttled key invalidation. The untested failed-shutdown branch now has its own case.
One unrelated job is red, LLM Integration Tests (ai-proxy), on a package this branch does not touch. Linting & Testing (agent-bff) and the BFF integration tests pass.
…timer The per-key invalidation window grew without bound while the entries it guards are capped, and the status retry kept the event loop alive past the shutdown grace the drain deadline enforces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
agent-bffwrote no activity log at all — zero occurrences ofactivity/ActivityLog/activityLogsServicein its source. A user who fetched data or triggered an action through the BFF left no audit trail, and the agent it proxies to writes none either (routes/access/audit-trail.tsonly reads the trail). Onlymcp-serverandworkflow-executorwere writing logs.Fixes PRD-1150
Depends on ForestAdmin/forestadmin-server#8483, which supplies the credential and the
BFFsource value. Implemented against that contract and mocked in tests, so this branch is reviewable now but must not ship before it.Audited surface
Strict parity with mcp-server: three routes.
actionlistsearchif the body carries one, elsefilter, elseindexrelations/:rel/listlistRelatedData, parent id as the record, label naming the relation and its refinementsactions/:name/executeaction, record ids, label naming the actioncount,relations/:rel/countandactions/:name/formwrite nothing, deliberately. mcp-server has no standalone count tool — count is folded inside the list tool's single log — and does not auditget-action-formeither. The accepted consequence is that a filteredcountcalled on its own stays unaudited, which is an information oracle the MCP surface does not expose; auditing it would double the audit volume, since a table page-load fireslistandcountas two separate requests and every pagination click replays both.How it is wired
New
src/activity-log/: the service (its own instance, becauseForestAdminClientOptionshas noheadersfield and the source must travel as one), the creator holding the action→type map and the fail policy, the wrapper, the drainer, and a composition root so the route middlewares take a writer and never see the service or the token plumbing.src/auth/forest-server-token-middleware.tslands a lazyctx.state.resolveForestServerToken, memoised per request. API-key mode returns the token that came with the resolve response; OAuth mode callsensureFreshServerAccess. Lazy because/health, permissions, context, the OpenAPI document and the docs route audit nothing and must not pay a session lookup — and permissions is hit on every page load.Fail policy, verbatim from mcp-server: a write whose pending log cannot be created is blocked (503
audit_unavailable, new, mirroringpermissions_unavailable); a read proceeds with a warning; an authorization refusal propagates even for a read (403audit_not_authorized, new). Both statuses were already declared on these routes andRetry-Afterwas already wired for 503, so only the OpenAPI descriptions needed extending.Points worth a reviewer's attention
Approval needed an explicit special case. mcp-server treats
approvalRequestedas a success, so its entry endscompleted. The BFF throwsactionRequiresApproval, which verbatim wrapping would record asfailed— the same business event recorded differently depending on the channel, making action-failure statistics unusable. The log is markedcompletedbefore the rethrow.activityLogsis a required option on both route middlewares rather than reached fromctx.state, so a wiring mistake cannot silently disable auditing. That is why the existing construction sites in the test suite now pass an explicit passthrough. The token resolver is what stays off their dependency lists.Execute wraps the whole sequence,
loadActionandsetFieldsincluded, matchingexecute-action.ts. So an unknown action, or an agent down at form load, produces afailedentry for an attempt that never touched data. That is the intent — capture the attempt.installShutdownHandlersreplaces the previously installed pair instead of adding one.runCliis called many times in a single test file and would otherwise accumulate signal listeners; one process runs one BFF, so replacement is also the right production semantics.An API-key write with no token answers 503, not 401. There is no session to have expired — the resolve response simply predates the server change.
Known follow-ups, not addressed here
Metricsport hasincrementandgaugebut no duration.cli-core.tsintobuild-bff.tsand is already in review. Land it first and rebase this.Verification
tsc --noEmitclean;yarn workspace @forestadmin/agent-bff test→ 91 suites, 1698 tests passing; targeted eslint clean.🤖 Generated with Claude Code
Note
Add activity-log auditing for data reads and action executions in
agent-bffActivityLogWriter,ActivityLogDrainer, andwithActivityLogto create pending activity logs before audited operations and mark them completed or failed aftercreateDataRoutesMiddleware(list, relation-list) andcreateActionRoutesMiddleware(execute only; form loads are not audited); both now require anActivityLogWriterdependencycreateForestServerTokenMiddlewarefor lazy Forest server token resolution: API-key requests use the stored token, OAuth requests resolve via session lifecycle; both paths memoize a single shared resolution promiseinvalidateApiKeyIdentity,ResolveCache.invalidate,ApiKeyAuthenticator.invalidate) triggered on 401 from the activity-log service, so stale cached tokens are discardedauditUnavailable(503, with 5s retry-after) andauditNotAuthorized(403) error factories, and updates the OpenAPI error descriptionsBFFHttpServer.stopto close idle connections immediately, force-destroy remaining connections after a 10s deadline (configurable viashutdownTimeoutMs), then drain activity-log work;Agent.stopandEmbeddedBff.stopnow await this draincreateDataRoutesMiddlewareandcreateActionRoutesMiddlewarenow require anActivityLogWriter— all in-tree callers are updated but out-of-tree consumers must supply one; read operations fail-open (warn and proceed) when activity-log creation fails, while write/action operations fail-closed (reject withauditUnavailable)Changes since #1885 opened
ActivityLogDrainerto track operations with human-readable descriptions and support deadline-bounded draining [3a9a68b]createResolveCache()to prevent rapid repeated invalidations within the positive TTL window [3a9a68b]Macroscope summarized c5930cc.