fix(server): report agent stream 5xx failures to Sentry - #3363
Conversation
Request handlers catch every error and convert it to an HTTP response, so nothing escapes to a global handler. Sentry only sees uncaught exceptions and unhandled rejections, which means every per-request failure has been invisible by construction. Confirming evidence: every issue in the veryfront-server Sentry project today is a startup/bootstrap failure. The handled/unhandled boundary was drawn at "is it a VeryfrontError" rather than "is it 5xx". That is right for the common case, since most VeryfrontErrors are 4xx validation and auth outcomes that would flood Sentry, but it swallows server errors too. Draw the boundary at status instead. Both 5xx exits of the agent stream catch block now report: - the typed branch, alongside the logging added in #3359 - the generic fallback, where an unexpected TypeError produces a bare message with no slug, detail, or stack in the log Each event carries slug, category, detail, project id, project slug and the run id, so it is actionable without a Loki dive. `detail` is included because errorToResponse deliberately strips it from 5xx bodies; that stripping is unchanged and asserted by the existing test. Attributes go through the reporter's sanitizer, which strips URL credentials, redacts sensitive keys and truncates values. Reporting is optional. captureApplicationError is a no-op when no reporter is installed, which is the normal case for framework users running veryfront without Sentry configured. Scope is deliberately one path. A survey found agent-stream.handler.ts is the only file under src/server/handlers/ using isVeryfrontError or errorToResponse; only channel-dispatch-request.ts, agent-run-resume.handler.ts and agent-run-cancel.handler.ts convert a caught error to a 500 at all. Those are follow-ups once events are confirmed landing in Sentry.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe agent stream handler now reports structured application errors for 5xx failures. Tests verify reporting context for typed 503 and unexpected 500 failures, and suppression for typed 400 failures. ChangesAgent stream observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AgentStreamHandler
participant ApplicationErrorReporter
participant HTTPResponse
AgentStreamHandler->>ApplicationErrorReporter: report structured 5xx failure context
ApplicationErrorReporter-->>AgentStreamHandler: optional capture result
AgentStreamHandler->>HTTPResponse: return existing error response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/handlers/request/agent-stream.handler.ts`:
- Around line 742-744: Update the error-reporting path around
reportAgentStreamFailure and its getPathRunId call to catch malformed URL
parsing failures. When parsing the run ID throws, continue reporting the failure
without setting requestId, while preserving the existing run ID behavior for
valid paths and the generic 500 response.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9a76af9-f719-4a79-a1d6-7d307fb172a3
📒 Files selected for processing (2)
src/server/handlers/request/agent-stream.handler.test.tssrc/server/handlers/request/agent-stream.handler.ts
There was a problem hiding this comment.
Pull request overview
This PR ensures server-side failures in the agent stream request handler are visible to the optional application-error reporter (for example, Sentry) even when the handler catches and converts errors into HTTP responses.
Changes:
- Add
reportAgentStreamFailure()to capture 5xx agent stream failures viacaptureApplicationError()with run-id correlation and sanitized attributes. - Report both typed 5xx
VeryfrontErrorfailures and unexpected non-VeryfrontErrorfailures under distinct boundaries. - Add tests covering: typed 5xx reporting, untyped 500 reporting, and silence for typed 4xx errors.
Verification
- Not run in this review environment.
- Suggested next step: run the repo’s narrow/unit test command for this area (at minimum the agent-stream handler test file), then broaden if needed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/server/handlers/request/agent-stream.handler.ts | Adds best-effort application-error reporting for agent stream 5xx failures with correlation attributes (including run id as requestId). |
| src/server/handlers/request/agent-stream.handler.test.ts | Adds tests to ensure 5xx is reported once with expected context, unexpected errors are reported once, and 4xx remains unreported. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // `pathRunId` is scoped to the try block, so the run id is re-derived from | ||
| // the URL here. It is the identifier that ties an event to a single run. | ||
| const runId = getPathRunId(new URL(req.url).pathname); | ||
| if (details.slug) attributes["error.slug"] = details.slug; |
There was a problem hiding this comment.
Correct, and fixed in 1b0a8b2 — you and CodeRabbit found this independently.
Confirmed the mechanism rather than assuming it: /api/control-plane/runs/run_%ZZ/stream matches RUN_STREAM_PATH_REGEX, and decodeURIComponent("run_%ZZ") throws URIError: URI malformed. The first decode throws inside the try and lands in the generic catch; the reporter then decoded a second time and threw past the catch, replacing the 500 with an uncaught failure.
Your framing is the right one and is now the invariant the code follows: reporting is strictly best-effort and must never throw. The decode is guarded and the event is sent without requestId when it fails.
Locked with a regression test — reverting only the guard reproduces the escaping URIError, so it fails for the right reason.
getPathRunId calls decodeURIComponent, which throws URIError on a malformed percent escape such as /api/control-plane/runs/run_%ZZ/stream. That path still matches the handler route, so the decode throws inside the try, lands in the generic catch, and the reporter then decoded it a second time — this time throwing straight past the catch and destroying the 500 response it was meant to describe. Reporting is diagnostic and must never replace the failure that triggered it. Catch the decode and report without a requestId instead. Replaces the plain-TypeError generic-branch test, which this case subsumes: it reaches the same branch while also covering the throw. Also trims the commentary on the reporting helper to what is not derivable from the code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/handlers/request/agent-stream.handler.ts:1047
- The fallback error response hard-codes status 500 while the surrounding code already uses
HTTP_INTERNAL_SERVER_ERRORfor reporting. Using the constant here avoids a magic number and keeps the reported status and response status coupled if the constant ever changes.
reportAgentStreamFailure(error, req, ctx, {
boundary: "agent.stream.handler",
status: HTTP_INTERNAL_SERVER_ERROR,
});
return this.respond(builder.json({ error: "Internal agent stream failed" }, 500));
The problem
Request handlers catch every error and convert it to an HTTP response. Sentry only sees what escapes to a global handler — uncaught exceptions and unhandled rejections. A caught-and-converted error never escapes, so every per-request failure is invisible in Sentry by construction.
Confirming evidence: every issue currently in the
veryfront-serverSentry project is a startup/bootstrap failure (VERYFRONT_TRUST_FORWARDED_HEADERS,AddrInUse, config-load errors). Those throw outside a request handler. Nothing per-request is there.This is what made the isolated-runtime incident expensive: agent chat on staging returned 500 on every attempt for hours, with nothing in Sentry, nothing useful in logs, and no detail in the response. It was found only because a user reported broken chat.
The design flaw
The handled/unhandled boundary was drawn at "is it a
VeryfrontError" rather than "is it 5xx".That's right for the common case — most
VeryfrontErrors are 4xx (validation, auth) and reporting them would flood Sentry. But it swallows server errors too, which are exactly the ones worth paging on.The boundary should be status, not error type. 5xx reports; 4xx doesn't.
The change
Both 5xx exits of the agent-stream catch block now report:
isVeryfrontError+status >= 500agent.stream.requestagent.stream.handlerTypeErrorproduces a bare one-line log with no slug, detail, or stack — the case where a captured stack is worth the mostEach event carries
slug,category,detail,project.id,project.slug,http.statusand the run id asrequestId, so an event is actionable without a Loki dive.Notes for review
detailis in the event but still not in the response.errorToResponsestripsdetailfrom 5xx bodies atsrc/errors/http-error.ts:104-106. That is unchanged, and the existing test still asserts the caller never receives it. Forwarding it to Sentry is the whole point — it is the field that identified the isolated-runtime root cause.PII. All 7
detailvalues this handler constructs are static string literals — no interpolated tokens, paths, or user input. Errors from deeper code are the residual risk, so the guarantee rests on the sanitizer rather than that audit:sanitizeTelemetryAttributesstrips URL credentials, redacts sensitive key names, and truncates every value. This is the same treatmentdetailalready gets in Loki today.Sentry is optional.
captureApplicationErroris a no-op when no reporter is installed and swallows its own failures internally — so framework users running veryfront without Sentry configured are unaffected, and a reporter fault can never replace the application failure that triggered it. No call-site guard needed.No double-reporting. The two branches are mutually exclusive — the typed branch returns, so the generic path cannot also fire for the same error. Nothing inside the
trycaptures-and-rethrows. This is asserted behaviorally (captures.length === 1) rather than guarded with aWeakSet, so a future rethrow turns the test red instead of being silently absorbed. If a second capture site ever appears here, dedupe belongs incaptureApplicationErroritself.Tests
Three cases in
agent-stream.handler.test.ts, modelled on the existing 5xx logging test — injecting a throw viaensureProjectDiscoveryand installing a stub viasetApplicationErrorReporter:VeryfrontErrorreports exactly once, with full contextVeryfrontErrorreports exactly onceRed proven by flipping the condition to
>= 600: the typed 5xx test fails, the untyped one correctly still passes (different branch). Restored, all 45 steps in the file pass.Gates:
deno check --no-lock,deno lint,deno fmt --check,deno task lint:test-typecheck(baseline holds, 0 new). The test file was also typechecked directly to confirm it is not grandfathered.Scope
Deliberately one path. A survey found
agent-stream.handler.tsis the only file undersrc/server/handlers/usingisVeryfrontError/errorToResponse. Only three other request handlers convert a caught error to a 500 at all:channel-dispatch-request.ts,agent-run-resume.handler.ts,agent-run-cancel.handler.ts.Those are follow-ups once events are confirmed landing in Sentry with useful context — not a blanket instrumentation pass in this PR.
Summary by CodeRabbit