Skip to content

client: classify post-deadline transport failures as deadline_exceeded - #238

Merged
iainmcgin merged 1 commit into
mainfrom
iain/deadline-classification
Aug 21, 2026
Merged

client: classify post-deadline transport failures as deadline_exceeded#238
iainmcgin merged 1 commit into
mainfrom
iain/deadline-classification

Conversation

@iainmcgin

Copy link
Copy Markdown
Collaborator

Fixes #210.

The race

A server enforcing the same deadline aborts the RPC independently, so its RST_STREAM can arrive before the client's own timer fires. The in-flight body read then fails first, and the call reported internal — attributing to this client what was really a timeout the caller asked for.

Which of the two wins is down to timer coarseness and scheduler delay, so the same call could report either code run to run. That is what makes the gRPC client conformance Timeouts cases flaky on a loaded host, on main as much as on any branch.

The fix

The missing-grpc-status path already resolved this by deadline rather than by arrival order. This applies the same rule at the three body-read sites, through one classifier:

fn classify_body_read_error(context: &str, error: &dyn Display, deadline: Option<Instant>) -> ConnectError {
    if deadline_elapsed(deadline) {
        ConnectError::deadline_exceeded(format!("{context} after the deadline elapsed: {error}"))
    } else {
        ConnectError::internal(format!("{context}: {error}"))
    }
}

The four places that ask "is the deadline past" now share one deadline_elapsed predicate rather than three hand-inlined copies, so they cannot drift apart.

collect_body_bounded and the two Connect parse functions had no deadline to consult, so it is threaded in. It is used only to classify — with_deadline still does the enforcing, and the doc on collect_body_bounded says so to head off the obvious misreading.

The transport cause is preserved in the upgraded error. A genuine transport fault that merely happened after the deadline should stay diagnosable, and the code is what callers branch on, not the message.

Connect gets this too, not just gRPC. connect-timeout-ms produces the identical race and collect_body_bounded is shared, so scoping the fix to gRPC would have been arbitrary.

Evidence

The gRPC client conformance suite, same host, same load:

runs result
before 3 1, 3 and 5 failures — all Timeouts/..., all internal vs expected deadline_exceeded
after 5 1454 passed, 0 failed every run

No movement elsewhere: Connect client 2580/0, gRPC-Web client 2838/0, server 3600/0.

Three unit tests pin the branch directly — past the deadline gives deadline_exceeded and keeps the cause, before it stays internal, and no deadline stays internal without picking up timeout wording.

Deliberately not in scope

Two Connect sites classify a clean body EOF without a terminus as internal without consulting the deadline, where the gRPC equivalent (classify_grpc_end) does upgrade. Those fire on graceful EOF rather than a transport error, so they are a different failure mode — a close without a terminus is more likely a protocol violation than a race. Flagging it because the asymmetry should be a decision rather than an accident; happy to file a follow-up.

One note recorded in the code for future test authors: deadline_elapsed reads the real clock while with_deadline runs on tokio's, so under #[tokio::test(start_paused = true)] virtual time advances and this does not. A paused-time test that delivers a body error and expects the timeout classification will not get it.

Testing

Clippy on the pinned 1.95 toolchain, cargo test -p connectrpc --no-default-features (423), 56 test suites, lint, fmt, and the four conformance suites above.

@iainmcgin
iainmcgin marked this pull request as ready for review July 21, 2026 19:44
@iainmcgin
iainmcgin requested a review from rpb-ant July 21, 2026 19:44
christopherwxyz pushed a commit to christopherwxyz/connect-rust that referenced this pull request Jul 25, 2026
…onnectrpc#239)

The connect-rust half of anthropics/buffa#331, now that buffa 0.9.1 has
shipped.

## What changed since the first version of this PR

buffa 0.9.1 did not just raise its own bound — it **exported the whole
mechanism**. So this no longer keeps a connect-side implementation at
all. The `descriptor_limits` module and its 16 GiB constant are gone,
replaced by delegation to `buffa_codegen::{decode_request,
tooling_decode_options, decode_failure}`.

That matters beyond tidiness: `buf generate` hands the identical
`CodeGeneratorRequest` to every plugin in a run, so a schema large
enough to need raising needs it for `protoc-gen-buffa` and
`protoc-gen-connect-rust` alike. One mechanism means one setting, rather
than a connect-specific twin the user has to discover and set
separately.

## The problem

buffa 0.9 charges its element-memory budget per element on **struct
size**, not on encoded bytes. Descriptor structs are wide, so the 32 MiB
default rejected descriptor sets from ordinary schemas of a few hundred
`.proto` files.

Measured here, one 400-file schema producing a 19,749,990-byte
descriptor set through `connectrpc_build::Config`:

| element-memory budget | result |
|---|---|
| 32 MiB (buffa 0.9.0's default) | `failed to decode FileDescriptorSet:
element memory limit exceeded` |
| 1 GiB (buffa 0.9.1's tooling budget) | `OK — 1600 files generated` |

## What this does

`protoc-gen-connect-rust` decodes through
`buffa_codegen::decode_request`; `connectrpc-build`, which has no plugin
parameter string, through `tooling_decode_options`. Both get the 1 GiB
tooling bound, and both overrides:

- `element_memory_limit=<bytes|unlimited>` as a plugin option, read by
scanning the wire for the parameter *before* the decode it governs, and
- `BUFFA_ELEMENT_MEMORY_LIMIT`, which covers every plugin in the run and
is the only route into a build script.

The bound stays finite rather than lifting entirely, so a truncated or
corrupt set still fails with an error instead of exhausting memory. The
guide recommends a byte count over `unlimited` for that reason.

## Two traps this exposed, both of which would have shipped

**Our own option parser would have rejected the option.** Plugin options
are parsed *after* the decode that consumed this one, so
`element_memory_limit=` reached the `unknown plugin option` arm and
failed the build — for precisely the one user who ever sets it, whose
schema was too large to decode without it. Proven by disabling the new
arm and watching `unknown plugin option:
"element_memory_limit=unlimited"` come back.

**Test fixtures sized by element count had silently stopped testing.**
buffa 0.9.1 took `ValueView` from 48 bytes to 32, so a hardcoded
`800_000` fell from 1.14x the budget to 0.76x. Two tests merged in connectrpc#235
flipped from proving a rejection to decoding successfully. Such fixtures
now derive their count from the live `size_of`, sized by the
**smallest** type any decode in the test materialises — sizing by the
larger leaves a view decode under budget, which I did at first, and the
`stream_message` test then passed with the behaviour it checks deleted
outright.

## Also in here

- `connectrpc-build` declares `rerun-if-env-changed` for the variable it
now reads. Without it, cargo does not re-run the build script when the
variable changes, and the setting looks inert.
- The over-budget failure from a build script corrects buffa's hint,
which offers a plugin option unreachable there, and points at this
repo's guide rather than buffa's.
- `ReflectionError::ElementBudget` reports an over-budget set at runtime
as a large schema rather than as corruption. The `From<DecodeError>`
conversion is written out instead of derived, so a future `?` on a new
decode path cannot silently route around the split. Runtime reflection
stays on the untrusted default deliberately — a reflection service may
be fed descriptors by a peer.
- The generated `descriptor_pool()` picks up buffa's self-scaling bound,
which is why the multiservice example regenerates. That closes the last
carry-forward item from #331 with no code of ours.

## Testing

Both overrides exercised in **both directions**. Raising proves little
on its own — the path could be ignoring the setting and succeeding
anyway. *Lowering* via the plugin option brings the failure back, which
is the only thing that proves the option is read pre-decode.

Four suites: server conformance 3600/0, client Connect 2580/0, client
gRPC-Web 2838/0. The client gRPC `Timeouts` cases fail 3–4 here, and
reproduce identically on `main` (1, 3 and 5 failures across three runs)
— that is connectrpc#210, fixed by connectrpc#238, not this change.

Reflection driven over a real socket against both descriptor sources. 56
test suites, clippy on the pinned 1.95 toolchain, `cargo test -p
connectrpc --no-default-features` (420), fmt on the pinned nightly,
rustdoc, and `task generate:all` idempotent.

## Note for the merge

`buffa = "0.9.1"` is a hard floor — `decode_request` and
`tooling_decode_options` do not exist in 0.9.0. This homespace's
registry mirror was still serving a stale index while I worked, so local
verification ran against a path override; CI resolves against real
crates.io, where buffa 0.9.1 published cleanly, so its dependency job is
the real check on that floor.

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
A server enforcing the same deadline aborts the RPC independently, so its
RST_STREAM can arrive before the client's own timer fires. The in-flight
body read then failed first and the call reported internal, attributing
to this client what was really a timeout the caller asked for. Which of
the two won came down to timer coarseness and scheduler delay, so the
same call could report either code run to run — the gRPC client
conformance Timeouts cases failed between one and five times per run on a
loaded host, on main as much as on any branch.

The missing-grpc-status path already resolved this by deadline rather
than by arrival order. This applies the same rule at the three body-read
sites through one classifier, and the four places that ask "is the
deadline past" now share a single predicate instead of three copies of
it, so they cannot drift.

Two Connect parse functions and collect_body_bounded had no deadline to
consult, so it is threaded in. It is used only to classify; with_deadline
still enforces.

The transport cause is preserved in the upgraded error. A genuine
transport fault that merely happened after the deadline should stay
diagnosable, and the code is what callers branch on.

Connect gets this too, not just gRPC: connect-timeout-ms produces the
identical race, and collect_body_bounded is shared.

Fixes #210.

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
@iainmcgin
iainmcgin force-pushed the iain/deadline-classification branch from ae31470 to d695877 Compare August 21, 2026 02:14
@iainmcgin
iainmcgin added this pull request to the merge queue Aug 21, 2026
Merged via the queue into main with commit a49b165 Aug 21, 2026
14 checks passed
@iainmcgin
iainmcgin deleted the iain/deadline-classification branch August 21, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gRPC client maps post-deadline transport errors to internal instead of deadline_exceeded

2 participants