fix(gax): enforce the call deadline on the REST transport - #9334
Open
shivanee-p wants to merge 4 commits into
Open
shivanee-p wants to merge 4 commits into
shivanee-p wants to merge 4 commits into
Conversation
`addTimeoutArg` computes `options.deadline` for every call and `CallSettings.timeout` defaults to 30s, but the fallback stub never read it. gRPC enforces its own deadline and cancels with DEADLINE_EXCEEDED; REST did not, so an endpoint that accepted the connection and then went quiet left the request outstanding forever, stranding the promise or callback waiting on it, and any span bound to that callback with it. The deadline was already being passed in. The stub's parameter names were inverted relative to gax's `UnaryCall` order: the third argument is what gRPC calls `options` and is the one carrying the deadline, but it was named `_metadata` and discarded, while the second carries the metadata that becomes request headers and was named `options`. Rename both so the argument that matters is identifiable. Forward the remaining time to gaxios as `timeout`. gaxios v7 arms an `AbortSignal.timeout` and merges it with the existing cancel signal through `AbortSignal.any`, so `cancel()` is unaffected. An already-expired deadline clamps to 1ms rather than 0, because gaxios reads `timeout: 0` as "no timeout" and would otherwise silently drop the bound in the case that most needs it. Translate the resulting abort into a GoogleError carrying Status.DEADLINE_EXCEEDED. Nothing downstream understands a DOMException named TimeoutError: `retryCodes` matching, caller `err.code` checks and the tracer's `error.type` all key off the numeric gRPC status. The translation is skipped when no deadline was forwarded, so an unrelated timeout is never relabelled, and the AbortError from `cancel()` keeps its existing handling. Server-streaming RPCs are excluded. The signal stays armed once the response body starts flowing, so forwarding the deadline would abort a healthy long-lived stream mid-read. That leaves REST streams unbounded where gRPC bounds them; closing that gap is user-visible and belongs in its own change. Six tests cover the forwarding, both cases where no timeout should be set, the expired-deadline clamp and the error translation. Each was mutation-tested by deliberately breaking the corresponding behavior and confirming the matching assertion fails.
shivanee-p
added this pull request to stack #9335
September 15, 2026 01:51
shivanee-p
requested review from
quirogas
and removed request for
a team and
feywind
September 15, 2026 01:52
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements call deadline forwarding for fallback service stubs in gax, translating timed-out requests into DEADLINE_EXCEEDED errors to align with gRPC behavior, and adds comprehensive unit tests. The review feedback suggests making the error type-checking more robust to handle different execution contexts, and correcting a type mismatch and potential runtime bug where metadata values are accessed as arrays but typed as strings, which could lead to truncated header values.
shivanee-p
marked this pull request as draft
September 15, 2026 01:53
The previous commit renamed the parameters on the exported
`FallbackServiceStub` interface and narrowed the third one from `{}` to
`{deadline?: Date}`. That is a public type change: an object literal passed as
the third argument by downstream code would now trip excess-property checks,
and implementors of the interface would see a narrower contract. Neither is
needed to enforce the deadline.
Restore the interface to its original shape. The accurate parameter names stay
inside `generateServiceStub`, where they are an implementation detail, with a
comment recording that the interface declares the middle two arguments the
other way round. That inversion is why the deadline-bearing argument read as
metadata and went unused for so long.
Also add a test that drives the call through `createApiCall` rather than
handing the stub a deadline directly. The existing tests all fabricate
`{deadline}` themselves, so they cover the stub but not the hand-off from
`addTimeoutArg`, which is the seam where the deadline was actually being
dropped. Commenting out the assignment in `addTimeoutArg` leaves all six of
them passing and fails only the new one.
The DEADLINE_EXCEEDED translation added in cd42784 never ran. It decided whether a deadline had expired by inspecting the error, and the error it was written against does not occur. Measured end to end against a server that accepts the connection and then never replies: rejected after 1513ms (timeout was 1500ms) constructor : GaxiosError name : Error code : undefined message : The operation was aborted. cause.name : AbortError node-fetch discards signal.reason and throws its own AbortError. gaxios wraps that in a GaxiosError, which never sets its own `name` (so it stays the inherited 'Error') and copies `code` only from a DOMException cause, which this is not. So the predicate's `instanceof Error` guard was not merely fragile across realms or for serialized errors, it was gating on evidence that never arrives. Every timeout returned the raw transport error. Worse, the evidence cannot be made to arrive. A deadline expiry and a caller's cancel() produce byte-identical errors: same constructor, same name, same undefined code, same cause.name. No amount of sniffing can separate them, because the distinction does not exist in the error. It exists only in the caller, which armed the timer. So arm it explicitly. Rather than hand `timeout` to gaxios and let it build the AbortSignal internally, build the same signal here, set a flag when it fires, and merge it with the cancel signal. The request behaves identically; the difference is that we now know which of the two aborts happened. The predicate is gone, along with the error inspection it existed to do. The clamp moves from Math.max(1, ...) to Math.max(0, ...). Passing 0 to gaxios meant "no timeout", but AbortSignal.timeout(0) aborts on the next tick, which is the right answer for a deadline that has already passed. Negative values still have to be clamped: AbortSignal.timeout throws RangeError on them. This also fixes the cancel guard in the body-read handler, which tested `err.name !== 'AbortError'`. For the reason above that name is 'Error', so the check never matched and a cancelled call still reported an error. It now uses the recorded state. The outer handler deliberately keeps no cancel suppression, matching its previous behavior; whether a cancelled call should report CANCELLED the way gRPC does is a separate, user-visible question. Tests: the fixtures fabricated a TimeoutError that production never produces, which is exactly why mutation-tested unit tests still passed against dead code. They now assert on the signal the stub owns and reject with the measured error shape. Two cases were added that the old approach could not have satisfied: cancelling a call that has a deadline armed must not report DEADLINE_EXCEEDED, and a cancelled call must not report an error at all. Each of the six behaviors was mutation-tested by breaking it and confirming the corresponding assertion fails: the timeout flag never set, the old err.name guard restored, the timeout signal not attached, server streams bounded, the expired-deadline clamp removed, and the translation applied without checking the flag. All six were caught. Verified against a real silent server through the real stub: a direct call with a 1500ms deadline settles at 1507ms with code 4, the same call through createApiCall with CallSettings.timeout 1200ms settles at 1203ms with code 4, and a cancel() with a 60s deadline armed settles at 53ms and is left untranslated.
The metadata parameter was typed `{[name: string]: string}` while the
code read it as `metadata[key][0]`. Both could not be right, and the
disagreement was hiding two silent data-loss bugs.
gRPC metadata is multi-valued. `buildMetadata` normalizes every value
to an array for exactly that reason, with the comment "Since gRPC
expects each header to be an array, we are doing the same for fallback
here", and it appends when a header appears more than once. Reading
index 0 discarded everything after the first value.
Measured through the real stub with a recording transport:
buildMetadata output what the stub sent
x-multi = ["a","b","c"] x-multi = "a"
x-plain = "hello" x-plain = "h"
The second row is the type error made visible: indexing a string
yields its first character, so a caller who passed a plain string, as
the declared type invited, silently sent one character of it.
Widen the type to `string | string[]` and handle both. Arrays are
appended so all values survive; the Headers API joins them with ', '.
The delete before appending preserves the previous semantics, where
`set` replaced whatever the request encoder had put there rather than
accumulating onto it.
The exported `FallbackServiceStub` interface is unchanged. It types
this parameter as `{}`, which already permits both shapes.
Four mutation tests, all caught: restoring the original single-value
read fails the multi-valued and plain-string assertions, appending
without clearing fails the override assertion, sending only element
zero fails the multi-valued assertion, and indexing a plain string
fails the plain-string assertion.
shivanee-p
marked this pull request as ready for review
September 15, 2026 02:53
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
1 participant
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.
Update bug where REST calls didn't enforce deadlines. Since oTel spans were bound to the lifetime of calls, it's discovered that when the fallback service stub is used, it's not verified that the call is completed
Fixes:
metadatafrom being dropped from wrong type definition