Repository navigation
fix(sandbox): count only grpc-js's own statuses as gateway outages - #4066
Conversation
A gateway interceptor can answer with grpc-js's own wording (for example UNAVAILABLE 'Connection dropped'); that answer carries the response's content-type and date as status metadata, while a status grpc-js makes up has none. Require empty metadata before matching the details. A call that still had no connection at its deadline (DEADLINE_EXCEEDED 'Waiting for LB pick') was never sent, so it is an outage too; the outage error now keeps the actual gRPC status. Drop the unreachable 'No addresses resolved' alternative. 'Failed to connect' stays ordinary: a plaintext client at a TLS port reads the same. Wire cases for read ECONNRESET, an unresolvable name, a silent listener, server-sent lookalikes and post-header trailers kill the pattern, anchor, status and metadata mutants (findings 2048-2050).
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR distinguishes gateway replies from client-generated transport failures and treats an unsent request deadline as a gateway outage. Example: The gateway replies with UNAVAILABLE and the exact details “Connection dropped”.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it repairs source-confirmed gateway failure classification on main, and no actionable introduced defect was found. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe controller's OpenShell client converts gRPC failures into dependency errors; the revision worker uses their class to choose budgeted retries or deadline-bound waiting. flowchart TB
Work[Revision work] --> Worker[Controller worker]
Worker --> Compute[Compute Driver]
Compute --> Client[OpenShell client]
Client --> Gateway[OpenShell gateway]
Gateway --> Classification[Failure classification]
Classification --> Worker
Technical reviewBest possible solution: Land the focused classifier repair in the existing OpenShell client while preserving the worker's established retry policies. Do we have a high-confidence way to reproduce the issue? Current-main classification and the pinned grpc-js implementation establish both failure paths; the PR reports base-fail/head-pass wire cases, but this review executed no target code. Is this the best way to solve the issue? The repair uses the existing classifier and error owner, preserves sanitized diagnostics and status consumers, and adds meaningful transport coverage without changing Driver contracts or retry budgets. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 24862992720b. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 29, 2026, 12:33 PM ET / 16:33 UTC. |
…ordinary failures A request the gateway received but never answered times out with grpc-js's remote_addr deadline text, not "Waiting for LB pick"; it must stay on the attempt budget. A refused mTLS identity must never be a Sandbox gateway outage. Both cases were only implied: a matcher that took any DEADLINE_EXCEEDED, or any TLS/alert wording, survived the wire tests.
CI resolves localhost to 127.0.0.1 and then ::1. Nothing listens on ::1 in this test, and grpc-js reports only the last address's error, so a refused mTLS identity at localhost can surface as "connect ECONNREFUSED ::1:<port>" and read as an unanswered request. The IP channel has one address, so it still proves that a refused identity is an ordinary failure.
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Problem
The OpenShell client decides whether a failed gateway request is a Sandbox gateway outage (
SANDBOX_GATEWAY_UNAVAILABLE: a revision pass waits for it instead of spending its attempt budget). The decision matched the gRPC details text only:Connection dropped,read ECONNRESET,Stream refused by server,Name resolution failed ...,No connection established. Last error: connect ECONNREFUSED ...) counted as an outage. A gateway interceptor relays the reason and status it chooses, so a policy refusal worded like that deferred revision passes until the deployment deadline.DEADLINE_EXCEEDED ... Waiting for LB pickand spent the attempt budget. The same blackhole with a longer deadline (DeleteSandbox, 120 s) showed up asconnect ETIMEDOUTand counted as an outage.read ECONNRESETor unresolvable-name case, so dropping those patterns, the^...$anchor or the status-code check went unnoticed.Change
gatewayDidNotAnsweralso requires that the status has no metadata. grpc-js gives a status it makes up empty metadata. A status the gateway sends before any response message is a trailers-only response, and grpc-js keeps its other headers (content-type,date) as metadata. tonic sends interceptor refusals and handler errors this way.DEADLINE_EXCEEDEDwith grpc-js's exactDeadline exceeded after <n>s[,<phase>: <n>s]*,Waiting for LB picktext. That text means the request was never sent.OpenShellGatewayUnavailableErrornow keeps the real gRPC status (4 or 14), so the protected-Git expiry check still reports such a request astimeout.No addresses resolvedalternative: grpc-js's DNS lookup rejects a name that has no addresses ("Name resolution failed"), and an IP endpoint always has one address, so this client can never get that text.No connection established. Last error: Failed to connect (<ts>). A plaintext client at a TLS-only port gets exactly the same details as a stale port-forward, so the request stays on the attempt budget. Envoy'sno healthy upstream/ empty-message UNAVAILABLE is also unchanged: a proxy sends it, and the controller connects to the gateway Service directly.... alert certificate required) are not on the allowlist, so they stay ordinary failures.Verification
tests/conformance/openshell-gateway-wire.test.mjs, outage test, new wire cases:read ECONNRESET): outage..invalidService name: outage.DEADLINE_EXCEEDED,grpcStatus4): outage. This fails on main.Connection dropped by gateway policy,policy: Connection dropped, INTERNALConnection dropped): ordinary.DEADLINE_EXCEEDEDnamingremote_addr, so the request was sent): ordinary,grpcStatus4.OpenShell mTLS ... refuses missing or foreign identities: on the IP channel, each refused client identity is now asserted to be an ordinary failure, never a Sandbox gateway outage. Atlocalhost(which can also resolve to::1, where nothing listens in the test) grpc-js reports only the last address's error, which can be the refused::1connect; that limit predates this change and is tracked separately.DEADLINE_EXCEEDEDcounted as an outage, anyNo connection establishedtext counted as an outage, and any TLS/alert wording counted as an outage. All three were killed; the first only with the new never-answered case, and the TLS one now also by the refused client identities.tsc --build tsconfig.json tests/tsconfig.json) passed. eslint and prettier are clean on the touched files.Risks
ECONNREFUSEDthat such a policy already causes on CNIs that reject, and asconnect ETIMEDOUTwith longer deadlines. The deployment still fails, withSANDBOX_GATEWAY_UNAVAILABLE.