Repository navigation
fix(host-service): classify an unreachable cloud API as 503, not a 500 - #6735
Merged
Merged
Conversation
host.info calls the cloud API to resolve the organization. Transport failures from that call were unhandled, so a captive portal, a dropped connection, or broken DNS on the user's machine surfaced as INTERNAL_SERVER_ERROR from a procedure the desktop polls, and Sentry reported the user's network as a host-service bug. Translate at the boundary: walk the tRPC client error's cause chain for Node/undici transport codes and rethrow as SERVICE_UNAVAILABLE, keeping the underlying message. Anything without a matching code -- an application error the cloud returned, a response body we could not parse -- keeps reporting as a 500. Refs HOST-SERVICE-1B Refs HOST-SERVICE-46 Refs HOST-SERVICE-43 Claude-Session: https://claude.ai/code/session_01MLQT5zzzaRZgrnU4Tbz27Y
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe host service now detects selected cloud transport failures, converts them to ChangesCloud unreachable handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change classifies specific cloud transport failures as 503 while preserving existing behavior for application and unknown errors; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant hostRouter
participant getOrganization
participant cloudApi
participant rethrowCloudUnreachable
hostRouter->>getOrganization: request organization
getOrganization->>cloudApi: getByIdFromJwt.query
cloudApi-->>getOrganization: organization or error
getOrganization->>rethrowCloudUnreachable: classify caught error
rethrowCloudUnreachable-->>getOrganization: SERVICE_UNAVAILABLE or unchanged error
getOrganization-->>hostRouter: organization or rethrown error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
added a commit
that referenced
this pull request
Sep 20, 2026
…being unreachable (#7680) ## Problem `host.info` calls the cloud API to resolve the organization. When that request dies in transport, the host-service already knows not to report it: #6735 added `rethrowCloudUnreachable`, which walks the cause chain for a transport code and rethrows as `SERVICE_UNAVAILABLE` so the tRPC Sentry middleware leaves it alone. One transport code was missing from that set. A machine whose socket to the API was torn down mid-request surfaced as a fresh host-service `INTERNAL_SERVER_ERROR` issue, i.e. reported as our bug. ## Root cause The failure arrives as the usual three-link chain — `TRPCClientError("fetch failed")` → `TypeError("fetch failed")` → a Node socket error carrying `code: "ENOTCONN"`. That is the identical shape to the `read ECONNRESET` case the classifier already covers (and which its tests already name), but `ENOTCONN` was never in `UNREACHABLE_CODES`, so `findUnreachableCause` returned `null` and the error fell through to the middleware as a 500. The middleware's contract is explicit: anything still `INTERNAL_SERVER_ERROR` at that boundary is a bug, and the remedy is to fix the missing translation at the throw site rather than filter at the middleware. This is that missing translation. ## Fix One entry added to the existing code set, plus the matching row in the existing table-driven test. Nothing else: only the code this issue actually produced is added. Sibling codes that have never been observed here (`EPIPE`, `ECONNABORTED`, …) are deliberately left out — a classifier that matches more than the condition it names would silence real failures, which is why the set is grown from observed evidence one code at a time. `ENOTCONN` on a read means the socket was not connected when the read happened: the peer, a proxy, or the network between the user and the API dropped it. It cannot describe a defect in host-service's own call, and the only errors that reach this classifier are the ones thrown by that single cloud API call. No behaviour change for any consumer: the desktop's `readHostServiceInfo` already catches a failing `host.info` and omits the optional fields, and `isHostServiceConnectionError` already treats `SERVICE_UNAVAILABLE` as retryable/connection-level. ## Verification Test row added first, and confirmed failing against unchanged source: ``` (fail) rethrowCloudUnreachable > ENOTCONN is the cloud being unreachable [0.09ms] Expected: "SERVICE_UNAVAILABLE" Received: undefined 21 pass 1 fail ``` After the one-line source change: ``` $ bun test src/trpc/router/host/ 22 pass 0 fail 39 expect() calls Ran 22 tests across 1 file. [23.00ms] ``` ``` $ bunx biome check packages/host-service/src/trpc/router/host/cloud-api-error.ts packages/host-service/src/trpc/router/host/cloud-api-error.test.ts Checked 2 files in 15ms. No fixes applied. $ cd packages/host-service && bun run typecheck $ tsc --noEmit --emitDeclarationOnly false (clean) ``` No user-facing strings touched, so `check:i18n` does not apply. Refs HOST-SERVICE-6S
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
host.infois a small procedure the desktop polls: it reports which host this is, and which organization it belongs to. Resolving the organization means an outbound tRPC call to our cloud API, and that call's transport failures were unhandled. Any network problem between the user's machine and us escapedgetOrganizationas a rawTRPCClientError: fetch failed, became anINTERNAL_SERVER_ERROR, and — under the #6164 contract, where a 500 means "we have a bug" — got reported to Sentry as a host-service defect.The captured causes are unambiguously the user's network, not our software:
Error: read ETIMEDOUTon an established socket.HeadersTimeoutError.Hostname/IP does not match certificate's altnames, for a hostname that is not ours.Why fix rather than archive again. Two earlier issues in this family were archived on 2026-08-19. They came back, because the Sentry grouping key follows the innermost transport error — so each new flavour of network failure from this one throw site opens a new issue. Archiving does not converge; it is unbounded manual triage. And the classification is wrong on the merits: someone's captive portal is not an internal server error.
Reach. The failing builds are 1.22.0 / 1.23.0 / 1.24.1; the package is at 1.24.1, so this ships in the next host-service release and reaches machines as the desktop app auto-updates and respawns its bundled host-service. The existing issues will keep firing from older builds until the fleet rolls forward.
Root cause
getOrganizationawaitedapi.organization.getByIdFromJwt.query(...)with no adapter at the boundary. The one-hour success cache means this only fires when the call actually goes out.Fix
One adapter,
cloud-api-error.ts, applied at that single call site.The producer is a tRPC client over Node's HTTP stack — we do not own it, so the error has to be reconstructed rather than recognised. The most structural signal available is the machine-readable
codeNode and undici attach to exactly these failures, which sits three levels down thecausechain:TRPCClientError("fetch failed")→TypeError("fetch failed")→ the coded error. The adapter walks that chain structurally (duck-typed, neverinstanceof— the links are not ours) and matchescodeagainst a closed set. No message regexes.Matched:
ECONNREFUSED,ECONNRESET,ETIMEDOUT,ENOTFOUND,EAI_AGAIN,EHOSTUNREACH,ENETUNREACH,UND_ERR_CONNECT_TIMEOUT,UND_ERR_HEADERS_TIMEOUT, and the certificate-validation codes a TLS-intercepting portal or proxy produces:ERR_TLS_CERT_ALTNAME_INVALID,UNABLE_TO_VERIFY_LEAF_SIGNATURE,UNABLE_TO_GET_ISSUER_CERT,UNABLE_TO_GET_ISSUER_CERT_LOCALLY,SELF_SIGNED_CERT_IN_CHAIN,DEPTH_ZERO_SELF_SIGNED_CERT.A match becomes
SERVICE_UNAVAILABLE— already this package's idiom for "the thing you need to reach is not reachable" — carrying the underlying message, which is the informative one (fetch failedis not). Behaviour is otherwise untouched: same cache, samePRECONDITION_FAILEDwhen the organization is genuinely missing, no retries or backoff added.What deliberately still reports as a 500
SyntaxErrorwith no transport code attached, so the adapter does not match it and it stays a 500. This is intentional: at this layer a non-JSON response body is indistinguishable from our own cloud genuinely returning something broken, and silencing it would hide a real server-side bug. It is being archived separately.TRPCClientErrorbuilt from an error response, with no transport code anywhere in the chain — they keep reporting.CERT_HAS_EXPIRED/CERT_NOT_YET_VALID— excluded on purpose, and the one judgement call here. Interception announces itself as an untrusted issuer or the wrong hostname; an expired certificate is at least as likely to be ours, and if our certificate lapses this procedure should be one of the things that screams.Worth naming the residual over-match:
ECONNRESET/ECONNREFUSEDcould in principle be our own edge misbehaving rather than the user's network. From the host's vantage point that still is "could not reach the cloud", and cloud-side monitoring owns that failure — it is not something a desktop host-service should be diagnosing.No typed cause
Thrown as a plain
new TRPCError({ code, message })with nocause: { kind }. Nothing reads one: the only desktop callers ofhost.infoareuseRemoteHostStatus, which branches onisSuccessalone and falls through toreadyotherwise, and the tray menu, which doescatch { return null }. Akindnobody reads is code maintained for nothing; it can be added when a renderer branch wants it.Scope
Deliberately confined to this one procedure's outbound call. Other host-service procedures also call the cloud API —
workspaces.ts:456(task.byId),workspaces.ts:501(v2Workspace.trackCreated),project.ts:453(v2Project.findByGitHubRemote), plus fire-and-forget calls inworkspace.ts,notifications.ts, andlocal-workspace-store.tsthat already swallow their own rejections. They are left alone; each deserves its own look at what its caller should see. Classifying insidecreateApiClient's link chain would have covered them all in one move, and was rejected for exactly that reason — it would silently reclassify call sites nobody has reviewed.Verification
The negative tests are the point of this change, so both directions were checked against a strawman first.
Strawman 1 — message regex on
fetch failed(the last-resort approach this task rules out). Two negatives fail, and every positive fails too, because the client error's message isfetch failedand the real message is unreachable from there:Strawman 2 — blanket "anything this call throws is unreachable" (the tempting wrong fix). All five negatives fail:
With the shipped adapter — fixtures built with the real
TRPCClientErrorand real captured shapes, not mocks:(
--writewas run once for formatting, then re-checked clean as above. Repo-widebun run lintwas skipped per the known cross-worktree bun-lock hang.)Refs HOST-SERVICE-1B
Refs HOST-SERVICE-46
Refs HOST-SERVICE-43
https://claude.ai/code/session_01MLQT5zzzaRZgrnU4Tbz27Y
Summary by cubic
Classifies cloud API transport failures in host.info as 503 Service Unavailable instead of 500 Internal Server Error, so Sentry stops flagging user network issues as
host-servicebugs. Previously, aTRPCClientError: fetch failedbubbled up as a 500.TRPCClientError → TypeError → coded errorcause chain and maps Node/undicitransport codes (timeouts, DNS, connection resets/refusals, and untrusted TLS issuer/hostname) toSERVICE_UNAVAILABLE, preserving the underlying message; no regex.host.infoorganization lookup call site; other host-service cloud calls are unchanged by design.host-servicerelease bundled in the desktop app; older builds will continue emitting 500s until auto-update advances.Written for commit 57f7e4c. Summary will update on new commits.
Summary by CodeRabbit