deps: update grpc to 1.80.0 - #471
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
WalkthroughThis PR refreshes the vendored gRPC codebase, updating build metadata, TLS and credential APIs, client-channel and HTTP/2 transport internals, channelz and telemetry output, C++ wrapper call plumbing, C# code generation options, and a generated protobuf accessor in the agent header. ChangesVendored gRPC refresh
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deps/grpc/src/core/client_channel/subchannel_stream_client.cc (1)
109-126:⚠️ Potential issue | 🔴 CriticalHandler state becomes inconsistent when call creation fails: OnCallStartLocked() is invoked before verifying call creation succeeds.
Line 113 calls
OnCallStartLocked(), which transitions the handler state (e.g., health check status to "connecting"). However, whencall_state_->StartCallLocked()fails at line 121, the code at line 126 only deletescall_state_and returns—it does not invokeCallEndedLocked(), which is the proper cleanup path that notifies the handler of the failure. This leaves the handler in an intermediate state with no active call and no retry mechanism. The comment assumes the caller will recreate thisSubchannelStreamClientupon reconnection, but recovery depends on an explicit subchannel state change that may never occur if the connection remains nominally alive while call creation silently fails.Call the proper cleanup: invoke
CallEndedLocked(/*retry=*/false)before returning at line 126, or deferOnCallStartLocked()until after call creation succeeds.Also applies to: 194-207
🤖 Prompt for 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. In `@deps/grpc/src/core/client_channel/subchannel_stream_client.cc` around lines 109 - 126, In StartCallLocked, OnCallStartLocked is invoked before verifying CallState::StartCallLocked succeeded, leaving handlers inconsistent on failure; fix by either deferring the call to event_handler_->OnCallStartLocked(this) until after bool call_started = call_state_->StartCallLocked() returns true, or if keeping the current ordering, ensure proper cleanup by calling CallEndedLocked(false) before deleting call_state_.release() when call_started is false so the handler is notified and state is consistent; apply the same change to the other similar block that uses call_state_.release()/OnCallStartLocked().deps/grpc/src/core/BUILD (1)
4308-4318:⚠️ Potential issue | 🔴 CriticalBazel target
//:http_connect_client_handshakerdoes not exist.Both
http_proxy_mapper(line 4309) andxds_http_proxy_mapper(line 4337) declare dependencies on//:http_connect_client_handshaker, but this target has no definition in the repository. Without a root BUILD file, Bazel analysis will fail on these targets. Either define the target at the root Bazel package level or correct the dependency path to reference an existing target (e.g., a target indeps/grpc/src/core/BUILD).🤖 Prompt for 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. In `@deps/grpc/src/core/BUILD` around lines 4308 - 4318, The BUILD file references a non-existent Bazel target //:http_connect_client_handshaker as a dependency of http_proxy_mapper and xds_http_proxy_mapper; either add a top-level target definition named http_connect_client_handshaker in the repository root BUILD (exporting the correct srcs and deps) or change the dependency for the http_proxy_mapper and xds_http_proxy_mapper rules to point to the correct existing target (for example a target defined in deps/grpc/src/core/BUILD such as :http_connect_client_handshaker_local or the actual target name that supplies the handshaker implementation).
🟠 Major comments (19)
deps/grpc/src/core/client_channel/subchannel_stream_limiter.cc-62-62 (1)
62-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock admissions when in-flight is already above the limit.
Line 62 uses
==, so after a dynamic max reduction, new RPCs can still be admitted while already over quota. This should reject on>=.Suggested fix
- if (rpcs_in_flight == max_concurrent_streams) return false; + if (rpcs_in_flight >= max_concurrent_streams) return false;🤖 Prompt for 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. In `@deps/grpc/src/core/client_channel/subchannel_stream_limiter.cc` at line 62, The admission check currently only rejects when rpcs_in_flight == max_concurrent_streams, which allows new RPCs when rpcs_in_flight > max_concurrent_streams after a dynamic reduction; change the condition to reject when rpcs_in_flight >= max_concurrent_streams (i.e., replace the equality check with a >=) in the function that performs the admission test (the code using rpcs_in_flight and max_concurrent_streams in subchannel_stream_limiter.cc) so no new RPCs are admitted when in-flight is at or above the limit.deps/grpc/include/grpc/module.modulemap-11-11 (1)
11-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate both
credentials_cpp.handprivate_key_signer.hbehind C++-only submodules.Both headers expose C++ STL and Abseil types without C++ gating.
credentials_cpp.husesstd::optional<std::string>whileprivate_key_signer.hincludes C++ class definitions withstd::shared_ptr,std::variant, and Abseil dependencies. Exporting them directly fromframework module grpcbreaks C/Objective-C module imports that parse in non-C++ mode.Proposed fix
framework module grpc { umbrella header "grpc.h" ... - header "credentials_cpp.h" + explicit module credentials_cpp { + requires cplusplus + header "credentials_cpp.h" + export * + } ... - header "private_key_signer.h" + explicit module private_key_signer { + requires cplusplus + header "private_key_signer.h" + export * + }🤖 Prompt for 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. In `@deps/grpc/include/grpc/module.modulemap` at line 11, The public module currently exposes C++-only headers directly (credentials_cpp.h, private_key_signer.h) which breaks non-C++ imports; create a C++-only submodule (e.g., "grpc.cpp" or "grpc_cxx") with the attribute or directive that requires C++ (cplusplus) and move/export the two headers there, then remove them from the top-level framework module export so the parent "module grpc" only exposes C/ObjC-safe headers; ensure the new submodule is nested under the main grpc module and still exported for C++ consumers so symbols in credentials_cpp.h and private_key_signer.h stay available only when importing with C++ enabled.deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h-70-76 (1)
70-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
initial_max_concurrent_streams_in equality checks.
UpdateMaxConcurrentStreams()usesinitial_max_concurrent_streams_, butoperator==ignores it. That allows two settings objects to compare equal while producing different results on later updates.Suggested fix
bool operator==(const Http2Settings& rhs) const { return header_table_size_ == rhs.header_table_size_ && + initial_max_concurrent_streams_ == + rhs.initial_max_concurrent_streams_ && max_concurrent_streams_ == rhs.max_concurrent_streams_ && initial_window_size_ == rhs.initial_window_size_ &&Also applies to: 170-171
🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h` around lines 70 - 76, operator== currently omits initial_max_concurrent_streams_, so two Http2Settings objects can compare equal even though UpdateMaxConcurrentStreams(uint32_t) will behave differently; update the equality comparison(s) (the operator== implementation(s) that compare settings) to include initial_max_concurrent_streams_ alongside max_concurrent_streams_ (and any other existing fields) so objects with different initial_max_concurrent_streams_ no longer compare equal.deps/grpc/src/core/call/metadata_batch.cc-62-63 (1)
62-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep forwarded headers redacted in debug output.
Allow-listing
X-Forwarded-ForandX-Forwarded-Hostexposes potentially sensitive client/network identifiers in logs.Suggested fix
- if (key == XForwardedForMetadata::key()) return true; - if (key == XForwardedHostMetadata::key()) return true;🤖 Prompt for 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. In `@deps/grpc/src/core/call/metadata_batch.cc` around lines 62 - 63, The code currently allow-lists X-Forwarded-For and X-Forwarded-Host in the metadata redaction logic (the comparisons against XForwardedForMetadata::key() and XForwardedHostMetadata::key()), which exposes sensitive client identifiers; remove or negate those checks in metadata_batch.cc so those keys are NOT treated as safe for debug output (i.e., delete the two lines that return true for XForwardedForMetadata::key() and XForwardedHostMetadata::key(), or explicitly treat them as sensitive in the redaction branch), and ensure the redaction path for metadata (the surrounding function that decides which keys are printed) now redacts these keys instead of printing them.deps/grpc/src/core/credentials/call/call_creds_registry_init.cc-152-154 (1)
152-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the access token in
ToString().This serializes the raw bearer token into debug text, which is likely to end up in logs or validation output. Please mask it before this config becomes observable.
Suggested fix
std::string ToString() const override { - return absl::StrCat("{token=\"", token_, "\"}"); + return "{token=\"<redacted>\"}"; }🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/call/call_creds_registry_init.cc` around lines 152 - 154, The current ToString() implementation returns the raw bearer token (token_) which can leak secrets; update ToString() to avoid serializing the full token by returning a masked value instead (e.g., a constant "<redacted>" or a masked form that shows only the last N characters/length), so replace the direct use of token_ in ToString() with a redaction strategy that prevents logging the raw token.deps/grpc/src/core/ext/transport/chttp2/transport/goaway.cc-123-132 (1)
123-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWake GOAWAY waiters on trigger failure.
TriggerWriteCycle()forcesgoaway_statetokDonewhen the write trigger fails, but it never drainswakers. Any waiter already blocked on GOAWAY completion can stay asleep forever on this path.Suggested fix
absl::Status status = goaway_interface->TriggerWriteCycle(); if (!status.ok()) { GRPC_HTTP2_GOAWAY_LOG << "TriggerWriteCycle failed with status: " << status; goaway_state = GoawayState::kDone; + WaitSet::WakeupSet wakers_to_wakeup = wakers.TakeWakeupSet(); + wakers_to_wakeup.Wakeup(); }🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/goaway.cc` around lines 123 - 132, In GoawayManager::Context::TriggerWriteCycle(), when goaway_interface->TriggerWriteCycle() returns a non-ok status and you set goaway_state = GoawayState::kDone, also drain and wake all entries in the wakers queue so any waiters blocked on GOAWAY completion are notified; locate the wakers container in Context (e.g., wakers) and after setting goaway_state to kDone iterate over it (or swap with an empty list) and run each waiter callback/closure or invoke their wake method, ensuring thread-safety consistent with existing synchronization used by Context.deps/grpc/src/core/credentials/transport/channel_creds_registry.h-43-48 (1)
43-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
Equals()withproto_type()too.
Builder::RegisterChannelCredsFactory()now allows proto-only factories with an emptytype(). In that caseoperator==()can callEquals()on unrelated concrete configs whenever both havetype() == "", which risks false equality or unsafe down-casts inEquals()implementations that relied on the old same-type precondition.Suggested fix
bool operator==(const ChannelCredsConfig& other) const { if (type() != other.type()) return false; + if (proto_type() != other.proto_type()) return false; return Equals(other); }🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/transport/channel_creds_registry.h` around lines 43 - 48, operator== on ChannelCredsConfig can call Equals() for two different concrete types when both type() == "" (proto-only factories); update ChannelCredsConfig::operator== to also compare proto_type() (return false if proto_type() differs) before calling Equals(), so Equals() is only invoked for configs with matching proto_type(); reference ChannelCredsConfig::operator==, proto_type(), type(), Equals(), and Builder::RegisterChannelCredsFactory in your change.deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc-96-104 (1)
96-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't exact-match SANs against an empty SNI.
When no SNI source is resolved, Lines 187-205 still construct
XdsCertificateVerifierwith"". Ifauto_sni_san_validation()is enabled, Lines 96-104 then do an exact DNS SAN match on the empty string, which turns the “no SNI applicable” case into an authentication failure instead of falling back to the configured SAN matchers.💡 Suggested fix
- if (xds_certificate_provider_->auto_sni_san_validation()) { + if (xds_certificate_provider_->auto_sni_san_validation() && + !sni_name_.empty()) { if (!XdsVerifySubjectAlternativeNames( request->peer_info.san_names.dns_names, request->peer_info.san_names.dns_names_size, {StringMatcher::Create(StringMatcher::Type::kExact, sni_name_, true) .value()})) {Also applies to: 187-205
🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc` around lines 96 - 104, The code is exact-matching SANs against an empty SNI (sni_name_) which forces failures when no SNI is resolved; update the logic so that the exact StringMatcher for sni_name_ is only added when sni_name_ is non-empty: in the block that calls XdsVerifySubjectAlternativeNames (and in the XdsCertificateVerifier construction site that currently passes sni_name_ as ""), guard creation/usage of the exact-match StringMatcher on sni_name_ with a check like if (!sni_name_.empty()) so that when SNI is absent the code falls back to the configured SAN matchers instead of performing an exact-match on "". Ensure the change is applied at both the SNV verification call (where XdsVerifySubjectAlternativeNames is invoked) and where the verifier/constructor is built with sni_name_.deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h-268-275 (1)
268-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
delayed_ping_spawned_even whenTriggerWrite()fails.The cleanup runs in the last
TrySeqstep, so any error returned byTriggerWrite()skips it and leavesdelayed_ping_spawned_stucktrue. After one transient write failure, no future delayed ping can be scheduled.💡 Suggested fix
auto DelayedPingPromise(const Duration wait) { - return TrySeq( - Sleep(wait), - [this]() mutable { return ping_interface_->TriggerWrite(); }, - [this]() { - delayed_ping_spawned_ = false; - return absl::OkStatus(); - }); + return TrySeq(Sleep(wait), [this]() mutable { + auto status = ping_interface_->TriggerWrite(); + delayed_ping_spawned_ = false; + return status; + }); }🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h` around lines 268 - 275, In DelayedPingPromise ensure delayed_ping_spawned_ is cleared even if ping_interface_->TriggerWrite() fails: change the sequence so the cleanup that sets delayed_ping_spawned_ = false always runs (e.g., run TriggerWrite(), capture its absl::Status, then unconditionally reset delayed_ping_spawned_ and return the captured status) instead of relying on the final TrySeq step that is skipped on error; update the lambda chain in DelayedPingPromise to perform the reset in a finally-like way so transient TriggerWrite failures do not leave delayed_ping_spawned_ stuck true.deps/grpc/src/compiler/csharp_generator.cc-469-475 (1)
469-475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetect
Foo/FooAsynccollisions before appending the suffix.With
append_async_suffixenabled, a service that contains bothFooandFooAsyncmethods will generate two server methods namedFooAsync. The generated base class and both binder helpers become uncompilable in that case. The same suffix-appending logic appears in three locations: lines 469–475, 714–720, and 763–769.Precompute the generated server-side names for all methods in the service and detect duplicates before code emission. This prevents collisions and ensures valid C# output regardless of the input proto definitions.
🤖 Prompt for 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. In `@deps/grpc/src/compiler/csharp_generator.cc` around lines 469 - 475, Precompute the server-side method names for every method in the service using the same append_async_suffix logic (i.e., use append_async_suffix and the existing method_name computation) and build a map from generated name to original methods; detect any duplicates (e.g., Foo and FooAsync producing the same generated name) and fail generation with a clear error that lists the conflicting RPC names and the service, instead of appending suffixes blindly. Replace the ad-hoc suffix logic around method_name (the snippet using append_async_suffix and method->name()) so the code at the three locations (the existing method_name computation blocks and the binder helper emission sites) consults the precomputed name map; on duplicate detection abort or emit a descriptive diagnostic so generated C# is not produced with colliding FooAsync definitions. Ensure the error references append_async_suffix, the service name, and the conflicting RPC names so callers can fix the proto or disable suffixing.deps/grpc/src/core/channelz/channelz.h-340-345 (1)
340-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
<vector>directly in this header.Line 344 declares
std::vector<Element>, butchannelz.hdoes not include<vector>. This violates header self-containment and can break translation units that include it standalone, relying instead on transitive includes that may change.💡 Suggested fix
`#include` <set> `#include` <string> `#include` <type_traits> `#include` <utility> +#include <vector>🤖 Prompt for 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. In `@deps/grpc/src/core/channelz/channelz.h` around lines 340 - 345, The header channelz.h uses std::vector (the member additional_info_ and Element) but does not directly include <vector>, breaking header self-containment; fix by adding an `#include` <vector> to the top of channelz.h (alongside other includes) so std::vector is declared for Element and additional_info_, ensuring the header can be included standalone.deps/grpc/src/core/ext/filters/stateful_session/stateful_session_filter.h-73-85 (1)
73-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
<string>explicitly in this header.Lines 82-83 add
std::stringmembers (cookie_nameandpath), butstateful_session_filter.hdoes not directly include<string>. This creates a build fragility risk—the header may compile only because other includes transitively providestd::string, but this violates header self-containment and can break under different compiler configurations or include orders.💡 Suggested fix
`#include` <grpc/support/port_platform.h> `#include` <stddef.h> +#include <string> `#include` <utility>🤖 Prompt for 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. In `@deps/grpc/src/core/ext/filters/stateful_session/stateful_session_filter.h` around lines 73 - 85, The header is missing a direct include of <string>, so add an explicit include for it at the top of the file so the std::string members in struct Config (cookie_name, path) and any uses of std::string in methods like ToString() and Equals() are well-formed; update stateful_session_filter.h to `#include` <string> (ensuring the include sits with the other standard headers) to make the header self-contained.deps/grpc/src/core/credentials/transport/tls/grpc_tls_certificate_provider.h-217-218 (1)
217-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
root_certificates_to represent the "unset" state.The default-constructed
absl::StatusOrcreates akUnknownerror status, not an OK status. This differs fromroot_cert_info_above, which is explicitly initialized with= nullptrto represent the valid "no roots configured" state. Without initialization,root_certificates_starts in an error state, causing premature failures in validation or distribution paths before any update is applied.The
absl::StatusOrdefault constructor (fromstatusor.hline 665) initializes toStatus(absl::StatusCode::kUnknown, ""), notOK.Fix
absl::StatusOr<std::shared_ptr<tsi::RootCertInfo>> root_certificates_ - ABSL_GUARDED_BY(mu_); + ABSL_GUARDED_BY(mu_) = nullptr;🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/transport/tls/grpc_tls_certificate_provider.h` around lines 217 - 218, root_certificates_ is default-constructed into a kUnknown error Status; change its declaration so it is initialized as an OK absl::StatusOr that contains a null std::shared_ptr to represent the valid "no roots configured" state (i.e., construct the StatusOr with a null std::shared_ptr<tsi::RootCertInfo> value); update the member declaration for root_certificates_ (absl::StatusOr<std::shared_ptr<tsi::RootCertInfo>> root_certificates_) accordingly so code that checks/uses root_certificates_ sees an OK state with a null payload rather than an error.deps/grpc/src/core/credentials/transport/tls/grpc_tls_certificate_provider.cc-573-596 (1)
573-596:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsafe
DownCastwithout type validation.These C API functions use
DownCastto convert the provider toInMemoryCertificateProvider*, but there's no validation that the provider is actually of that type. If a caller passes aFileWatcherCertificateProvider(or other type), this will result in undefined behavior.Consider adding a type check before the downcast, or using
dynamic_castwith null check to returnfalsegracefully for wrong provider types.Suggested fix for set_root_certificate
bool grpc_tls_certificate_provider_in_memory_set_root_certificate( grpc_tls_certificate_provider* provider, const char* root_cert) { grpc_core::ExecCtx exec_ctx; + if (provider == nullptr || + provider->type() != grpc_core::InMemoryCertificateProvider::StaticType()) { + return false; + } auto in_memory_provider = grpc_core::DownCast<grpc_core::InMemoryCertificateProvider*>(provider); return in_memory_provider ->UpdateRoot(std::make_shared<tsi::RootCertInfo>(root_cert)) .ok(); }🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/transport/tls/grpc_tls_certificate_provider.cc` around lines 573 - 596, Both C API functions grpc_tls_certificate_provider_in_memory_set_root_certificate and grpc_tls_certificate_provider_in_memory_set_identity_certificate unsafely call grpc_core::DownCast to InMemoryCertificateProvider without validating the provider type; add a type check before casting (e.g., verify the provider is an InMemoryCertificateProvider via an appropriate runtime-type predicate or safe dynamic_cast) and if the provider is not the expected type return false (and in the identity case ensure pem_key_cert_pairs is cleaned up when appropriate) instead of performing the DownCast and invoking UpdateRoot or UpdateIdentityKeyCertPair on the wrong object.deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h-109-113 (1)
109-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWake blocked enqueuers when clearing the queue.
Clear()drops buffered entries without touchingwaker_. If anEnqueueMessage()promise is parked on a full queue and the stream is reset,HandleResetStreamLocked()clears the queue here, but the producer is never re-polled to observeIsEnqueueClosed(), so it can stay pending indefinitely.Proposed fix
void Clear() { while (queue_.Pop().has_value()) { } + tokens_consumed_ = 0; + auto waker = std::move(waker_); GRPC_DCHECK(IsEmpty()); + waker.Wakeup(); }🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h` around lines 109 - 113, Clear() currently pops all items but never notifies blocked enqueuers, so producers waiting in EnqueueMessage() can remain pending after HandleResetStreamLocked() clears the queue; after draining queue_ with queue_.Pop() in Clear(), invoke the stream waker (waker_) to wake blocked enqueuers so they re-poll and observe IsEnqueueClosed(); update Clear() to call the appropriate wake/notify method on waker_ (e.g., waker_.Wake()/WakeAll()/Notify() as provided by the waker implementation) immediately after the draining loop and before the final GRPC_DCHECK(IsEmpty()).deps/grpc/src/core/call/metadata_batch.h-345-352 (1)
345-352:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't model
x-forwarded-foras singular.Line 348 sets
kRepeatable = false, so the known-header append path will overwrite earlierx-forwarded-forvalues instead of preserving the full proxy chain when multiple header instances are present. That loses hop information that downstream auth, routing, or audit code may rely on. Please either make this trait repeatable end-to-end or canonicalize repeated values into a single comma-separated field during parse.🤖 Prompt for 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. In `@deps/grpc/src/core/call/metadata_batch.h` around lines 345 - 352, The XForwardedForMetadata trait currently sets kRepeatable = false which causes later writes to overwrite earlier x-forwarded-for header entries; change behavior so the full proxy chain is preserved by either setting XForwardedForMetadata::kRepeatable = true (so the known-header append path will keep multiple instances) or, if you prefer a single-field representation, update the metadata parse/merge logic for the "x-forwarded-for" key to canonicalize multiple instances into one comma-separated value (preserving original ordering) and ensure CompressionTraits/Stability code still accepts that canonicalized form; locate and update the XForwardedForMetadata definition and any metadata parse/merge function that handles SimpleSliceBasedMetadata or known-header appending to implement the chosen approach.deps/grpc/src/core/ext/transport/chttp2/transport/hpack_encoder.cc-566-576 (1)
566-576:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSurface oversized-entry rejection to the caller.
Lines 567-575 detect an append failure, but
MaybeAppend()only logs and drops the field. That makesRawEncoder::Encode()look successful even when trailers likegrpc-messageor application metadata were silently omitted, which is hard to diagnose and can change wire-visible behavior. Please return aboolorabsl::Statushere and let the caller decide whether to fail the call or intentionally suppress the field.As per coding guidelines, "Functions should return an error code to indicate failure using bool ... absl::Status, absl::StatusOr ..." and "Prefer absl::Status or absl::StatusOr for cross-layer code error handling in gRPC Core".
🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/hpack_encoder.cc` around lines 566 - 576, MaybeAppend currently drops oversized entries and only logs the event; change RawEncoder::MaybeAppend to return absl::Status (or bool per guideline) instead of void, return an error (e.g., absl::StatusCode::kInvalidArgument or a clear error) when CheckLength(buffer.Length()) fails, and call buffer_.TakeAndAppend on success; update callers such as RawEncoder::Encode to check and propagate that status (or fail the Encode) instead of treating the encode as successful; ensure uses of CheckLength and buffer_.TakeAndAppend are adjusted to the new return type and add/update tests to assert that oversized metadata produces a failing status.deps/grpc/src/core/call/call_filters.h-1577-1578 (1)
1577-1578:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInterceptor constructors reject lvalue callables; this breaks the API.
In
ServerTrailingMetadataInterceptor<Fn>andClientInitialMetadataInterceptor<Fn>(lines 1577–1578 and 1605–1606), the constructors useFn&&, which is a plain rvalue reference—not a forwarding reference—sinceFnis already a fixed class template parameter.When
AddOnServerTrailingMetadata()orAddOnClientInitialMetadata()are called with an lvalue callable:
- The builder template deduces
Fnas a reference type (e.g.,T&)std::decay_t<Fn>strips it toT- The constructor becomes
ServerTrailingMetadataInterceptor<T>(T&& fn)std::forward<Fn>(fn)forwards the lvalue asT&- But the rvalue reference parameter
T&&cannot accept an lvalueChange the constructors to accept by value and move:
explicit ServerTrailingMetadataInterceptor(Fn fn) : fn_(std::move(fn)) {} explicit ClientInitialMetadataInterceptor(Fn fn) : fn_(std::move(fn)) {}🤖 Prompt for 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. In `@deps/grpc/src/core/call/call_filters.h` around lines 1577 - 1578, The constructors for ServerTrailingMetadataInterceptor<Fn> and ClientInitialMetadataInterceptor<Fn> currently take Fn&& which is not a forwarding reference here and rejects lvalue callables; change both constructors to take Fn by value and move into fn_ (i.e., replace the parameter type from Fn&& to Fn and initialize fn_ with std::move(fn)) so AddOnServerTrailingMetadata/AddOnClientInitialMetadata callers that pass lvalues work correctly.deps/grpc/src/core/client_channel/subchannel.h-663-683 (1)
663-683:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing standard headers for the new container types.
Lines 663-664 and line 683 add
std::optionalandstd::dequeto this header, but the file does not include<optional>or<deque>. This creates a dependency on transitive includes, which can cause fragile build failures as include order changes.💡 Suggested fix
+#include <deque> `#include` <functional> `#include` <map> `#include` <memory> +#include <optional>🤖 Prompt for 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. In `@deps/grpc/src/core/client_channel/subchannel.h` around lines 663 - 683, The header now uses std::optional (retry_timer_handle_) and std::deque (queued_calls_) but does not include the corresponding standard headers; add the missing includes for <optional> and <deque> at the top of the file so these types are provided directly (avoid relying on transitive includes) and ensure compilation is robust for symbols retry_timer_handle_ and queued_calls_.
🟡 Minor comments (8)
deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.h-140-142 (1)
140-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a direct
<vector>include for the new return type.This header exposes
std::vector<tsi_ssl_pem_key_cert_pair>at line 140 but does not directly include<vector>, making it fragile to include-order changes and violating header self-containment principles.Proposed patch
`#include` <grpc/support/port_platform.h> `#include` <stddef.h> +#include <vector>🤖 Prompt for 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. In `@deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.h` around lines 140 - 142, The header declares a function returning std::vector<tsi_ssl_pem_key_cert_pair> (grpc_convert_grpc_to_tsi_cert_pairs) but doesn't include <vector>, breaking header self-containment; fix it by adding a direct `#include` <vector> at the top of the header so the std::vector return type is always defined regardless of include order, and keep existing forward declarations/other includes unchanged.deps/grpc/src/compiler/csharp_plugin.cc-87-88 (1)
87-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
append_async_suffixvalues instead of silently treating invalid input asfalse.Invalid values currently fall through as
false, which can hide user misconfiguration in generator flags.🔧 Proposed fix
} else if (options[i].first == "append_async_suffix") { - append_async_suffix = (options[i].second == "true"); + if (options[i].second.empty() || options[i].second == "true") { + append_async_suffix = true; + } else if (options[i].second == "false") { + append_async_suffix = false; + } else { + *error = "Invalid value for append_async_suffix: " + options[i].second; + return false; + } } else {🤖 Prompt for 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. In `@deps/grpc/src/compiler/csharp_plugin.cc` around lines 87 - 88, The code silently treats any non-"true" value for the generator flag when options[i].first == "append_async_suffix" as false; change the handling to validate options[i].second explicitly (accept only "true" or "false"), set append_async_suffix accordingly, and on any other value emit a clear error (e.g., log to stderr or using the existing error/usage reporting path) and abort/return failure so misconfiguration isn't silently ignored.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc-565-572 (1)
565-572:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
HasAnyMetrics()should account for all requested metric keys.The new keys are added to
requested_metrics_, but this check still only considers a subset. If an endpoint exposes only one of the newly added keys, metrics collection is disabled incorrectly.Suggested fix
return delivery_rate_.has_value() || rtt_.has_value() || min_rtt_.has_value() || data_notsent_.has_value() || byte_offset_.has_value() || congestion_window_.has_value() || - snd_ssthresh_.has_value() || packet_retx_.has_value(); + snd_ssthresh_.has_value() || packet_retx_.has_value() || + packet_spurious_retx_.has_value() || packet_sent_.has_value() || + packet_delivered_.has_value() || packet_delivered_ce_.has_value() || + data_retx_.has_value() || data_sent_.has_value() || + pacing_rate_.has_value() || reordering_.has_value() || + recurring_retrans_.has_value() || busy_usec_.has_value() || + rwnd_limited_usec_.has_value() || sndbuf_limited_usec_.has_value() || + is_delivery_rate_app_limited_.has_value();🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc` around lines 565 - 572, HasAnyMetrics() currently checks only an old subset of metric members and ignores newly added metric keys, causing endpoints exposing only new keys to be treated as having no metrics; update HasAnyMetrics (in the class that owns requested_metrics_) to account for all metric members by either adding the new metric member checks to the boolean expression or, better, iterate over requested_metrics_ and return true if any requested key corresponds to a present optional metric (e.g., consult delivery_rate_, rtt_, min_rtt_, data_notsent_, byte_offset_, congestion_window_, snd_ssthresh_, packet_retx_ plus the newly added metric members) so the method reflects the full set of requested metrics.deps/grpc/src/core/client_channel/subchannel_stream_client.h-99-103 (1)
99-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale constructor docs.
Line 99 and Line 100 still describe
interested_parties, but that parameter is no longer part of the constructor signature.📝 Proposed fix
- // Does not take ownership of interested_parties; the caller is responsible - // for ensuring that it will outlive the SubchannelStreamClient. + // Does not take ownership of `tracer`; the caller must ensure it outlives + // SubchannelStreamClient.🤖 Prompt for 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. In `@deps/grpc/src/core/client_channel/subchannel_stream_client.h` around lines 99 - 103, The constructor comment above SubchannelStreamClient::SubchannelStreamClient is stale: remove the reference to the non-existent interested_parties parameter and update the ownership semantics to match the current signature (WeakRefCountedPtr<Subchannel> subchannel, std::unique_ptr<CallEventHandler> event_handler, const char* tracer); specifically, edit the comment lines that mention "Does not take ownership of interested_parties; the caller is responsible for ensuring that it will outlive the SubchannelStreamClient" so it instead documents the actual parameters (who owns subchannel and event_handler, and any lifetime guarantees for tracer) to accurately reflect the constructor signature.deps/grpc/src/core/ext/transport/chttp2/GEMINI.md-173-173 (1)
173-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape the
[PH2][P0]token.Markdown parses
[PH2][P0]as reference-link syntax here, which is why markdownlint raises MD052. Wrap it in backticks or escape the brackets.🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/GEMINI.md` at line 173, The markdown line contains an unescaped token "[PH2][P0]" which is being interpreted as a reference link; update the text in GEMINI.md to escape or code-format that token (e.g., wrap [PH2][P0] in backticks or replace brackets with escaped brackets) so the literal token is shown and MD052 is resolved; target the exact token "[PH2][P0]" in the line marked "# TODO(tjagtap) [PH2][P0] Fix this".deps/grpc/src/core/ext/transport/chttp2/GEMINI.md-258-259 (1)
258-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the malformed
Keepalive/Pingtable rows.Line 258 is missing the trailing pipe, and Line 259 has too few cells. That breaks table rendering and triggers MD055/MD056.
🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/GEMINI.md` around lines 258 - 259, The two malformed table rows in GEMINI.md need to be corrected: for the "Keepalive" row (the one starting with "Keepalive | Loop | Keepalive Loop | 1 | If Keepalive is enabled, after constructor | Lifetime of the transport | Transport Close") add the missing trailing pipe to close the row, and for the "Ping" row (the one starting with "Ping | Timeout + Misc | | 4 | Sending a ping request | Timeout or a specific duration |") ensure it has the correct number of cells to match the table (fill the empty cell(s) or reorder so there are the same columns as other rows) so both rows have the same number of pipe-separated columns and render properly.deps/grpc/include/grpcpp/security/tls_certificate_provider.h-184-189 (1)
184-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the new in-memory provider docs.
This comment block still describes PEM-only keys and has a duplicated fragment, which no longer matches the
IdentityKeyOrSignerCertPairoverloads.📝 Suggested fix
// Returns an OK status if the following conditions hold: // - the root certificates consist of one or more valid PEM blocks, and // - every identity key-cert pair has a certificate chain that consists of - // chain that consists of valid PEM blocks and has a private key is a valid - // PEM block. + // valid PEM blocks and has a private key that is either a valid PEM block + // or a non-null PrivateKeySigner instance. absl::Status ValidateCredentials() const;🤖 Prompt for 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. In `@deps/grpc/include/grpcpp/security/tls_certificate_provider.h` around lines 184 - 189, The comment for ValidateCredentials() is outdated and contains a duplicated fragment and PEM-only wording; update the doc comment above absl::Status ValidateCredentials() const to remove the duplicate "chain that consists of" phrase and to reflect the new IdentityKeyOrSignerCertPair overloads (i.e., root_certs can be one or more valid PEM blocks, and each identity entry may be either an identity key-cert pair or a signer-cert pair where the signer is not necessarily a PEM private key), describing valid forms and requirements accordingly; ensure you reference ValidateCredentials and IdentityKeyOrSignerCertPair in the text so readers know which function/structure the rules apply to.deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h-592-608 (1)
592-608:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCopy-paste error: Client log macro and message used in server transport.
The
SecurityFrameLoopmethod usesGRPC_HTTP2_CLIENT_DLOGand logs "Http2ClientTransport" but this is the server transport implementation.Suggested fix
auto SecurityFrameLoop() { - GRPC_HTTP2_CLIENT_DLOG << "Http2ClientTransport::SecurityFrameLoop Factory"; + GRPC_HTTP2_SERVER_DLOG << "Http2ServerTransport::SecurityFrameLoop Factory"; return AssertResultType<Empty>(Loop([this]() {🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h` around lines 592 - 608, The log in SecurityFrameLoop is a copy-paste from the client: replace the client macro and message with the server equivalents by changing GRPC_HTTP2_CLIENT_DLOG and the "Http2ClientTransport::SecurityFrameLoop Factory" string to the server macro and message (e.g., GRPC_HTTP2_SERVER_DLOG and "Http2ServerTransport::SecurityFrameLoop Factory") inside the SecurityFrameLoop method so the log correctly reflects the server transport.
🧹 Nitpick comments (1)
deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings_promises.h (1)
211-220: ⚡ Quick winInitialize the out-parameter on every call.
should_spawn_security_frame_loopis only assigned on the first-settings transition. Leaving it untouched on later calls makes the result depend on the caller's previous value, so a staletruecan re-trigger the loop.♻️ Suggested fix
http2::Http2ErrorCode MaybeReportAndApplyBufferedPeerSettings( grpc_event_engine::experimental::EventEngine* event_engine, bool& should_spawn_security_frame_loop) { + should_spawn_security_frame_loop = false; http2::Http2ErrorCode status = settings_.ApplyIncomingSettings( std::exchange(pending_peer_settings_, {})); if (state_ == SettingsState::kFirstPeerSettingsReceived) { MaybeReportInitialSettings(event_engine); state_ = SettingsState::kReady; should_spawn_security_frame_loop = IsSecurityFrameExpected(); } return status; }🤖 Prompt for 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. In `@deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings_promises.h` around lines 211 - 220, In MaybeReportAndApplyBufferedPeerSettings, initialize the out-parameter should_spawn_security_frame_loop at the start of the function (e.g., set it to false) so it cannot carry a stale value from the caller; keep the existing behavior where it is set to IsSecurityFrameExpected() when state_ == SettingsState::kFirstPeerSettingsReceived after calling settings_.ApplyIncomingSettings and MaybeReportInitialSettings.
|
@coderabbitai you should not be reviewing dep PR's. Approve |
|
(ᵔᴥᵔ) ✏️ Learnings added
✅ Actions performedComments resolved and changes approved. |
PR-URL: #471 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
72d15c9 to
ad25589
Compare
2 participants
Summary by CodeRabbit
Chores
New Features
Improvements