From fb2f5495dcba03e0f7070e4498d6a29bb46cdc83 Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Sat, 24 Oct 2026 13:35:03 +0000 Subject: [PATCH 1/7] feat(storage): add client feature tracking header The service cannot currently tell when a client used read hedging or the multi-stream optimization for bidi reads, which makes it impossible to measure the impact of these features from the server side. Other client libraries (e.g. google-cloud-go, `x-goog-storage-go-features`) already report this via a small bitmask header. This change adds `x-goog-storage-cpp-features`, sent as a REST header and as gRPC metadata. The value is a 4-byte big-endian uint32 bitmask encoded as URL-safe base64 (RFC 4648 section 5, `-`/`_` alphabet, with `=` padding). The header is omitted when no tracked feature is in use. Bits (see `storage/internal/tracked_features.h`, the single source of truth): * bit 0, `kMultistreamInMrd`: set on every `BidiReadObject` stream an async `ObjectDescriptor` opens while `storage::EnableMultiStreamOptimizationOption` is true. * bit 1, `kReadHedging`: set on `ReadObject` requests (REST and gRPC) issued by the sync client while read hedging is in effect. The sync path sets the header on the `RestContext`, and `AddIdempotencyToken()` now forwards it (alongside the idempotency token) into the `grpc::ClientContext`, so both transports are covered by a single code path. --- .../storage/google_cloud_cpp_storage.bzl | 2 + .../storage/google_cloud_cpp_storage.cmake | 3 + .../storage/internal/async/connection_impl.cc | 13 ++++ .../async/connection_impl_open_test.cc | 67 ++++++++++++++++++ .../cloud/storage/internal/connection_impl.cc | 40 ++++++++--- .../storage/internal/connection_impl_test.cc | 49 +++++++++++++ .../internal/grpc/configure_client_context.cc | 26 +++++-- .../grpc/configure_client_context_test.cc | 23 +++++++ .../storage/internal/tracked_features.cc | 42 +++++++++++ .../cloud/storage/internal/tracked_features.h | 65 +++++++++++++++++ .../storage/internal/tracked_features_test.cc | 69 +++++++++++++++++++ .../storage/storage_client_unit_tests.bzl | 1 + 12 files changed, 382 insertions(+), 18 deletions(-) create mode 100644 google/cloud/storage/internal/tracked_features.cc create mode 100644 google/cloud/storage/internal/tracked_features.h create mode 100644 google/cloud/storage/internal/tracked_features_test.cc diff --git a/google/cloud/storage/google_cloud_cpp_storage.bzl b/google/cloud/storage/google_cloud_cpp_storage.bzl index eb5a1c80118e2..a30997d7b3a15 100644 --- a/google/cloud/storage/google_cloud_cpp_storage.bzl +++ b/google/cloud/storage/google_cloud_cpp_storage.bzl @@ -110,6 +110,7 @@ google_cloud_cpp_storage_hdrs = [ "internal/storage_connection.h", "internal/tracing_connection.h", "internal/tracing_object_read_source.h", + "internal/tracked_features.h", "internal/tuple_filter.h", "internal/well_known_parameters_impl.h", "lifecycle_rule.h", @@ -223,6 +224,7 @@ google_cloud_cpp_storage_srcs = [ "internal/storage_connection.cc", "internal/tracing_connection.cc", "internal/tracing_object_read_source.cc", + "internal/tracked_features.cc", "internal/win32/hash_function_impl.cc", "lifecycle_rule.cc", "list_buckets_reader.cc", diff --git a/google/cloud/storage/google_cloud_cpp_storage.cmake b/google/cloud/storage/google_cloud_cpp_storage.cmake index cf2a90beace6f..6801cbc9674fe 100644 --- a/google/cloud/storage/google_cloud_cpp_storage.cmake +++ b/google/cloud/storage/google_cloud_cpp_storage.cmake @@ -188,6 +188,8 @@ add_library( internal/tracing_connection.h internal/tracing_object_read_source.cc internal/tracing_object_read_source.h + internal/tracked_features.cc + internal/tracked_features.h internal/tuple_filter.h internal/well_known_parameters_impl.h internal/win32/hash_function_impl.cc @@ -481,6 +483,7 @@ if (BUILD_TESTING) internal/storage_connection_test.cc internal/tracing_connection_test.cc internal/tracing_object_read_source_test.cc + internal/tracked_features_test.cc internal/tuple_filter_test.cc lifecycle_rule_test.cc list_buckets_extended_reader_test.cc diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 6cf944aecefe0..77fde1760ca07 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -54,6 +54,7 @@ #include "google/cloud/storage/internal/retry_logging.h" #include "google/cloud/storage/internal/storage_stub.h" #include "google/cloud/storage/internal/storage_stub_factory.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/options.h" #include "google/cloud/internal/async_read_write_stream_timeout.h" #include "google/cloud/internal/async_retry_loop.h" @@ -61,6 +62,7 @@ #include "google/cloud/internal/async_streaming_write_rpc_timeout.h" #include "google/cloud/internal/make_status.h" #include +#include #include #include #include @@ -191,6 +193,17 @@ OpenStreamFactory MakeOpenStreamFactory( std::shared_ptr context, google::cloud::internal::ImmutableOptions options, DummyRequest const&) mutable { + // Every stream the descriptor opens (the first one, resumes, and + // the extra streams of the multi-stream optimization) goes through + // this factory, so this is where the service learns which client + // features are in play for the read. + std::uint32_t features = 0; + if (options->get()) { + features |= FeatureBit(TrackedFeature::kMultistreamInMrd); + } + if (features != 0) { + context->AddMetadata(kFeaturesHeader, EncodeFeatureMask(features)); + } auto open = std::make_shared( *stub, cq, std::move(context), std::move(options), request); // Extend the lifetime of the coroutine until it finishes. diff --git a/google/cloud/storage/internal/async/connection_impl_open_test.cc b/google/cloud/storage/internal/async/connection_impl_open_test.cc index fc301451ebc9c..95df99c2ea566 100644 --- a/google/cloud/storage/internal/async/connection_impl_open_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_open_test.cc @@ -13,11 +13,13 @@ // limitations under the License. #include "google/cloud/mocks/mock_async_streaming_read_write_rpc.h" +#include "google/cloud/storage/async/options.h" #include "google/cloud/storage/async/retry_policy.h" #include "google/cloud/storage/internal/async/connection_impl.h" #include "google/cloud/storage/internal/async/default_options.h" #include "google/cloud/storage/internal/async/options.h" #include "google/cloud/storage/internal/grpc/ctype_cord_workaround.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/testing/canonical_errors.h" #include "google/cloud/storage/testing/mock_resume_policy.h" #include "google/cloud/storage/testing/mock_storage_stub.h" @@ -29,10 +31,13 @@ #include "google/cloud/testing_util/mock_completion_queue_impl.h" #include "google/cloud/testing_util/scoped_log.h" #include "google/cloud/testing_util/status_matchers.h" +#include "google/cloud/testing_util/validate_metadata.h" #include "absl/strings/match.h" #include #include +#include #include +#include #include namespace google { @@ -49,6 +54,7 @@ using ::google::cloud::testing_util::IsProtoEqual; using ::google::cloud::testing_util::MockCompletionQueueImpl; using ::google::cloud::testing_util::ScopedLog; using ::google::cloud::testing_util::StatusIs; +using ::google::cloud::testing_util::ValidateMetadataFixture; using ::google::protobuf::TextFormat; using ::testing::AllOf; using ::testing::AtLeast; @@ -57,10 +63,12 @@ using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::InvokeWithoutArgs; using ::testing::IsEmpty; +using ::testing::Key; using ::testing::NiceMock; using ::testing::Not; using ::testing::NotNull; using ::testing::Optional; +using ::testing::Pair; using BidiReadStream = google::cloud::AsyncStreamingReadWriteRpc< google::storage::v2::BidiReadObjectRequest, @@ -396,6 +404,65 @@ TEST(AsyncConnectionImplTest, StopOnPermanentError) { EXPECT_THAT(log.ExtractLines(), Not(Contains(HasSubstr("[gcs-retry]")))); } +// Opens a stream with @p options and returns the metadata the connection set +// on the `grpc::ClientContext`. The stream fails with a permanent error, so a +// single `AsyncBidiReadObject()` call is all the test needs to observe. +std::multimap OpenAndCaptureMetadata( + Options options) { + auto constexpr kExpectedRequest = R"pb( + bucket: "test-only-invalid" + object: "test-object" + )pb"; + + ValidateMetadataFixture fixture; + std::multimap metadata; + AsyncSequencer sequencer; + auto mock = std::make_shared(); + EXPECT_CALL(*mock, AsyncBidiReadObject) + .WillOnce([&](CompletionQueue const&, + std::shared_ptr const& context, + google::cloud::internal::ImmutableOptions const&) { + metadata = fixture.GetMetadata(*context); + return MakeErrorStream(sequencer, PermanentError()); + }); + + auto mock_cq = std::make_shared(); + auto connection = std::make_shared( + CompletionQueue(mock_cq), std::shared_ptr(), mock, + TestOptions(std::move(options))); + + auto request = google::storage::v2::BidiReadObjectSpec{}; + EXPECT_TRUE(TextFormat::ParseFromString(kExpectedRequest, &request)); + auto pending = connection->Open({std::move(request), connection->options()}); + + auto next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Start"); + next.first.set_value(); + + next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Finish"); + next.first.set_value(); + + EXPECT_THAT(pending.get(), StatusIs(PermanentError().code())); + return metadata; +} + +TEST(AsyncConnectionImplTest, OpenFeatureHeaderWithMultiStream) { + auto const expected = + EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)); + EXPECT_THAT( + OpenAndCaptureMetadata( + Options{}.set(true)), + Contains(Pair(kFeaturesHeader, expected))); +} + +TEST(AsyncConnectionImplTest, OpenFeatureHeaderWithoutMultiStream) { + EXPECT_THAT( + OpenAndCaptureMetadata( + Options{}.set(false)), + Not(Contains(Key(kFeaturesHeader)))); +} + TEST(AsyncConnectionImplTest, TooManyTransientErrors) { ScopedLog log; diff --git a/google/cloud/storage/internal/connection_impl.cc b/google/cloud/storage/internal/connection_impl.cc index 9069a117cd92c..5e77a79f0e26e 100644 --- a/google/cloud/storage/internal/connection_impl.cc +++ b/google/cloud/storage/internal/connection_impl.cc @@ -18,6 +18,7 @@ #include "google/cloud/storage/internal/hedged_read_metrics.h" #include "google/cloud/storage/internal/retry_logging.h" #include "google/cloud/storage/internal/retry_object_read_source.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/parallel_upload.h" #include "google/cloud/internal/filesystem.h" #include "google/cloud/internal/opentelemetry.h" @@ -27,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -407,9 +409,29 @@ StatusOr> StorageConnectionImpl::ReadObject( ReadObjectRangeRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto const enable_hedging = + current->get(); + auto const delay = current->get(); + auto const max_hedges = + current->get(); + auto const max_buffer = + current->get(); + bool const hedging_active = + enable_hedging && max_hedges > 0 && hedge_pool_ && read_pool_; + + // Tell the service which client-side features shaped this read. Only the + // features actually in effect are reported, and the header is omitted when + // there are none. + std::uint32_t features = 0; + if (hedging_active) { + features |= storage_internal::FeatureBit( + storage_internal::TrackedFeature::kReadHedging); + } + auto const features_value = storage_internal::EncodeFeatureMask(features); + auto self = shared_from_this(); auto const* where = __func__; - auto factory = [self = shared_from_this(), current, where]( + auto factory = [self = shared_from_this(), current, where, features_value]( ReadObjectRangeRequest const& request, RetryPolicy& retry_policy, BackoffPolicy& backoff_policy) { auto const idempotency = @@ -427,10 +449,14 @@ StatusOr> StorageConnectionImpl::ReadObject( request.object_name())); return RestRetryLoop( logging_retry_policy, backoff_policy, idempotency, - [self, token = self->MakeIdempotencyToken()]( + [self, token = self->MakeIdempotencyToken(), features_value]( rest_internal::RestContext& context, Options const& options, ReadObjectRangeRequest const& request) { context.AddHeader(kIdempotencyTokenHeader, token); + if (!features_value.empty()) { + context.AddHeader(storage_internal::kFeaturesHeader, + features_value); + } return self->stub_->ReadObject(context, options, request); }, *current, request, where); @@ -479,15 +505,7 @@ StatusOr> StorageConnectionImpl::ReadObject( std::move(retry_policy), std::move(backoff_policy))); }; - auto const enable_hedging = - current->get(); - auto const delay = current->get(); - auto const max_hedges = - current->get(); - auto const max_buffer = - current->get(); - - if (!enable_hedging || max_hedges <= 0 || !hedge_pool_ || !read_pool_) { + if (!hedging_active) { return child_factory(position.offset, position.generation); } diff --git a/google/cloud/storage/internal/connection_impl_test.cc b/google/cloud/storage/internal/connection_impl_test.cc index 074f49a8d919f..8a1d75a781afb 100644 --- a/google/cloud/storage/internal/connection_impl_test.cc +++ b/google/cloud/storage/internal/connection_impl_test.cc @@ -14,6 +14,7 @@ #include "google/cloud/storage/internal/connection_impl.h" #include "google/cloud/storage/internal/tracing_connection.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/options.h" #include "google/cloud/storage/testing/canonical_errors.h" #include "google/cloud/storage/testing/mock_client.h" @@ -57,6 +58,7 @@ using ::testing::ByMove; using ::testing::ElementsAre; using ::testing::Eq; using ::testing::HasSubstr; +using ::testing::IsEmpty; using ::testing::Not; using ::testing::Property; using ::testing::Return; @@ -825,6 +827,53 @@ ReadSourceResult MakeReadResult(std::string const& payload, char* buf) { HttpResponse{HttpStatusCode::kOk, {}, {}}}; } +// The feature tracking header reports read hedging to the service, and only +// when hedging is in effect. The primary attempt is opened lazily by the read +// pool, so the header is observed on the first `Read()`. +std::vector FeatureHeaderForReadObject(Options options) { + auto captured = std::make_shared>(); + auto mock = std::make_unique(); + EXPECT_CALL(*mock, options).Times(AtLeast(0)); + EXPECT_CALL(*mock, ReadObject) + .WillOnce([captured](rest_internal::RestContext& context, Options const&, + ReadObjectRangeRequest const&) { + *captured = + context.GetHeader(storage_internal::kFeaturesHeader).values(); + auto source = std::make_unique(); + EXPECT_CALL(*source, Read).WillOnce([](char* buf, std::size_t) { + return MakeReadResult("payload", buf); + }); + return StatusOr>(std::move(source)); + }); + auto client = StorageConnectionImpl::Create(std::move(mock), options); + std::vector buffer(100); + google::cloud::internal::OptionsSpan const span( + client->options().set( + std::chrono::seconds(30))); + StatusOr> source = + client->ReadObject(ReadObjectRangeRequest("test-bucket", "test-object")); + EXPECT_THAT(source, IsOk()); + if (!source) return *captured; + EXPECT_THAT((*source)->Read(buffer.data(), buffer.size()), IsOk()); + return *captured; +} + +TEST(RetryClientTest, ReadObjectFeatureHeaderWithHedging) { + auto const expected = + storage_internal::EncodeFeatureMask(storage_internal::FeatureBit( + storage_internal::TrackedFeature::kReadHedging)); + EXPECT_THAT(FeatureHeaderForReadObject( + BasicTestPolicies() + .set(true) + .set(1) + .set(1)), + ElementsAre(expected)); +} + +TEST(RetryClientTest, ReadObjectFeatureHeaderWithoutHedging) { + EXPECT_THAT(FeatureHeaderForReadObject(BasicTestPolicies()), IsEmpty()); +} + TEST(RetryClientTest, HedgedReadRecordsMetricsOnGlobalMeterProvider) { auto dispatched = std::make_unique>(); auto won = std::make_unique>(); diff --git a/google/cloud/storage/internal/grpc/configure_client_context.cc b/google/cloud/storage/internal/grpc/configure_client_context.cc index 20ebc3a88f3ed..0cd5680786c44 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/grpc/configure_client_context.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/internal/url_encode.h" #include @@ -36,15 +37,26 @@ void ApplyRoutingHeadersImpl(grpc::ClientContext& context, auto constexpr kIdempotencyTokenHeader = "x-goog-gcs-idempotency-token"; +namespace { + +/// Copies every value of @p name from @p context into @p ctx. +void CopyHeader(grpc::ClientContext& ctx, + rest_internal::RestContext const& context, + std::string const& name) { + auto const& headers = context.headers(); + auto const l = headers.find(name); + if (l == headers.end()) return; + for (auto const& v : l->second.values()) ctx.AddMetadata(name, v); +} + +} // namespace + void AddIdempotencyToken(grpc::ClientContext& ctx, rest_internal::RestContext const& context) { - auto const& headers = context.headers(); - auto const l = headers.find(kIdempotencyTokenHeader); - if (l != headers.end()) { - for (auto const& v : l->second.values()) { - ctx.AddMetadata(kIdempotencyTokenHeader, v); - } - } + CopyHeader(ctx, context, kIdempotencyTokenHeader); + // The connection layer sets the client feature mask on the `RestContext` so + // it is transport-agnostic; forward it alongside the idempotency token. + CopyHeader(ctx, context, kFeaturesHeader); } void AddIdempotencyToken(grpc::ClientContext& ctx, std::string const& token) { diff --git a/google/cloud/storage/internal/grpc/configure_client_context_test.cc b/google/cloud/storage/internal/grpc/configure_client_context_test.cc index d3d8a5391b670..4ed26262f1212 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context_test.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context_test.cc @@ -16,6 +16,7 @@ #include "google/cloud/storage/internal/hash_function.h" #include "google/cloud/storage/internal/object_requests.h" #include "google/cloud/storage/internal/sign_blob_requests.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/grpc_options.h" #include "google/cloud/testing_util/validate_metadata.h" #include @@ -64,6 +65,28 @@ TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenString) { Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); } +TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenForwardsFeaturesHeader) { + grpc::ClientContext ctx; + auto const context = + rest_internal::RestContext{} + .AddHeader("x-goog-gcs-idempotency-token", "token-123") + .AddHeader(kFeaturesHeader, "AAAAAg=="); + AddIdempotencyToken(ctx, context); + auto metadata = GetMetadata(ctx); + EXPECT_THAT(metadata, + Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); + EXPECT_THAT(metadata, Contains(Pair(kFeaturesHeader, "AAAAAg=="))); +} + +TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenNoFeaturesHeader) { + grpc::ClientContext ctx; + auto const context = rest_internal::RestContext{}.AddHeader( + "x-goog-gcs-idempotency-token", "token-123"); + AddIdempotencyToken(ctx, context); + auto metadata = GetMetadata(ctx); + EXPECT_THAT(metadata, Not(Contains(Pair(kFeaturesHeader, _)))); +} + TEST_F(GrpcConfigureClientContext, ApplyQueryParametersEmpty) { grpc::ClientContext ctx; ApplyQueryParameters( diff --git a/google/cloud/storage/internal/tracked_features.cc b/google/cloud/storage/internal/tracked_features.cc new file mode 100644 index 0000000000000..bbd21d26ab9b4 --- /dev/null +++ b/google/cloud/storage/internal/tracked_features.cc @@ -0,0 +1,42 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/storage/internal/tracked_features.h" +#include "google/cloud/internal/base64_transforms.h" +#include + +namespace google { +namespace cloud { +namespace storage_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +std::uint32_t FeatureBit(TrackedFeature feature) { + return std::uint32_t{1} << static_cast(feature); +} + +std::string EncodeFeatureMask(std::uint32_t mask) { + if (mask == 0) return {}; + std::array const bytes{ + static_cast((mask >> 24) & 0xFF), + static_cast((mask >> 16) & 0xFF), + static_cast((mask >> 8) & 0xFF), + static_cast(mask & 0xFF), + }; + return google::cloud::internal::UrlsafeBase64EncodeWithPadding(bytes); +} + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace storage_internal +} // namespace cloud +} // namespace google diff --git a/google/cloud/storage/internal/tracked_features.h b/google/cloud/storage/internal/tracked_features.h new file mode 100644 index 0000000000000..8050ed062c8cd --- /dev/null +++ b/google/cloud/storage/internal/tracked_features.h @@ -0,0 +1,65 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_TRACKED_FEATURES_H +#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_TRACKED_FEATURES_H + +#include "google/cloud/storage/version.h" +#include +#include + +namespace google { +namespace cloud { +namespace storage_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +/** + * The header (REST) and metadata key (gRPC) carrying the client feature mask. + * + * The value is a 4-byte big-endian `std::uint32_t` bitmask, encoded as + * URL-safe base64 (RFC 4648 section 5, `-` and `_` alphabet) with `=` padding. + * The header is omitted when no tracked feature is in use. + */ +inline constexpr char kFeaturesHeader[] = "x-goog-storage-cpp-features"; + +/** + * Client-side features reported to the service. + * + * Each enumerator is the bit position (not the bit value) of the feature in + * the mask. This file is the single source of truth for the assignment, and + * is parsed by tooling outside this repository: keep each enumerator on one + * line, in the form `kName = N,`, and never reuse or renumber a bit. + */ +enum class TrackedFeature : std::uint32_t { + kMultistreamInMrd = 0, + kReadHedging = 1, +}; + +/// Returns the bit value (`1 << position`) for @p feature. +std::uint32_t FeatureBit(TrackedFeature feature); + +/** + * Encodes @p mask as the value of `kFeaturesHeader`. + * + * Returns an empty string when @p mask is zero, in which case the header must + * not be sent. + */ +std::string EncodeFeatureMask(std::uint32_t mask); + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace storage_internal +} // namespace cloud +} // namespace google + +#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_TRACKED_FEATURES_H diff --git a/google/cloud/storage/internal/tracked_features_test.cc b/google/cloud/storage/internal/tracked_features_test.cc new file mode 100644 index 0000000000000..c699643814d27 --- /dev/null +++ b/google/cloud/storage/internal/tracked_features_test.cc @@ -0,0 +1,69 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/storage/internal/tracked_features.h" +#include +#include +#include + +namespace google { +namespace cloud { +namespace storage_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +namespace { + +using ::testing::IsEmpty; + +TEST(TrackedFeatures, HeaderName) { + EXPECT_EQ(std::string(kFeaturesHeader), "x-goog-storage-cpp-features"); +} + +TEST(TrackedFeatures, FeatureBit) { + EXPECT_EQ(FeatureBit(TrackedFeature::kMultistreamInMrd), std::uint32_t{1}); + EXPECT_EQ(FeatureBit(TrackedFeature::kReadHedging), std::uint32_t{2}); +} + +TEST(TrackedFeatures, EncodeZeroIsEmpty) { + EXPECT_THAT(EncodeFeatureMask(0), IsEmpty()); +} + +// The exact strings are part of the contract with the service and with the +// other client libraries: 4-byte big-endian, URL-safe alphabet, with padding. +TEST(TrackedFeatures, EncodeKnownValues) { + EXPECT_EQ(EncodeFeatureMask(1), "AAAAAQ=="); + EXPECT_EQ(EncodeFeatureMask(2), "AAAAAg=="); + EXPECT_EQ(EncodeFeatureMask(4), "AAAABA=="); + EXPECT_EQ(EncodeFeatureMask(0xF8), "AAAA-A=="); + EXPECT_EQ(EncodeFeatureMask(0xFFFFFFFF), "_____w=="); + EXPECT_EQ(EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)), + "AAAAAQ=="); + EXPECT_EQ(EncodeFeatureMask(FeatureBit(TrackedFeature::kReadHedging)), + "AAAAAg=="); +} + +TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { + // Exercise every 6-bit group value; none of the outputs may contain the + // standard alphabet's `+` or `/`. + for (std::uint32_t mask = 1; mask != 0; mask <<= 1) { + auto const encoded = EncodeFeatureMask(mask | (mask >> 1) | (mask >> 2)); + EXPECT_EQ(encoded.find_first_of("+/"), std::string::npos) << encoded; + EXPECT_EQ(encoded.size(), 8U) << encoded; + } +} + +} // namespace +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace storage_internal +} // namespace cloud +} // namespace google diff --git a/google/cloud/storage/storage_client_unit_tests.bzl b/google/cloud/storage/storage_client_unit_tests.bzl index 6968b7ca16b4f..d0511b00c9bc2 100644 --- a/google/cloud/storage/storage_client_unit_tests.bzl +++ b/google/cloud/storage/storage_client_unit_tests.bzl @@ -95,6 +95,7 @@ storage_client_unit_tests = [ "internal/storage_connection_test.cc", "internal/tracing_connection_test.cc", "internal/tracing_object_read_source_test.cc", + "internal/tracked_features_test.cc", "internal/tuple_filter_test.cc", "lifecycle_rule_test.cc", "list_buckets_extended_reader_test.cc", From df2f450a10f5303a394896ba7d93af4ce1f4d1f0 Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Sun, 25 Oct 2026 03:46:54 +0000 Subject: [PATCH 2/7] refactor(storage): single unnamed namespace in configure_client_context Fold the header-copy helper and the idempotency token constant into the file's existing unnamed namespace instead of opening a second one. No behavior change. --- .../storage/internal/grpc/configure_client_context.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/google/cloud/storage/internal/grpc/configure_client_context.cc b/google/cloud/storage/internal/grpc/configure_client_context.cc index 0cd5680786c44..72e8ef8d10f88 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context.cc @@ -22,6 +22,9 @@ namespace cloud { namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { + +auto constexpr kIdempotencyTokenHeader = "x-goog-gcs-idempotency-token"; + void ApplyRoutingHeadersImpl(grpc::ClientContext& context, std::string const& bucket_name, RoutingHeaderOptions const& options) { @@ -33,13 +36,8 @@ void ApplyRoutingHeadersImpl(grpc::ClientContext& context, } context.AddMetadata("x-goog-request-params", std::move(params)); } -} // namespace - -auto constexpr kIdempotencyTokenHeader = "x-goog-gcs-idempotency-token"; - -namespace { -/// Copies every value of @p name from @p context into @p ctx. +// Copies every value of the @p name header from @p context into @p ctx. void CopyHeader(grpc::ClientContext& ctx, rest_internal::RestContext const& context, std::string const& name) { From 9d090e9d11ac38c2dc61d0f3eebff951d078a20e Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Sun, 25 Oct 2026 03:46:55 +0000 Subject: [PATCH 3/7] test(storage): cover feature header on every transport path Fill the gaps in the test coverage for `x-goog-storage-cpp-features`: * `tracked_features_test`: pin the encodings for masks 3, 0xFC, and a high-byte value (big-endian), place sextets 62 and 63 in every position to prove the URL-safe alphabet, and `static_assert` that the enumerators are distinct bit positions below 32. * `connection_impl_test` (sync, REST `GenericStub`): the header is absent when hedging is enabled but `MaxReadHedgesOption` is 0, when the connection has no hedging thread pools, and on non-`ReadObject` RPCs even with hedging in effect. * `grpc/stub_test` (sync, gRPC): `GrpcStub::ReadObject()` forwards the header from the `RestContext` into the `grpc::ClientContext`, and sends no metadata entry at all when the `RestContext` has none. * `async/connection_impl_open_test`: with the multi-stream optimization on, every `BidiReadObject` stream a descriptor opens carries the header, not just the first one. * `async/connection_impl_read_test`: the plain `ReadObject()` and `ReadObjectRange()` paths never send it. --- .../async/connection_impl_open_test.cc | 105 ++++++++++++++++++ .../async/connection_impl_read_test.cc | 80 +++++++++++++ .../storage/internal/connection_impl_test.cc | 102 ++++++++++++++--- .../cloud/storage/internal/grpc/stub_test.cc | 42 +++++++ .../storage/internal/tracked_features_test.cc | 78 +++++++++++++ 5 files changed, 394 insertions(+), 13 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl_open_test.cc b/google/cloud/storage/internal/async/connection_impl_open_test.cc index 95df99c2ea566..40f1563f4e6ee 100644 --- a/google/cloud/storage/internal/async/connection_impl_open_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_open_test.cc @@ -59,7 +59,9 @@ using ::google::protobuf::TextFormat; using ::testing::AllOf; using ::testing::AtLeast; using ::testing::Contains; +using ::testing::Each; using ::testing::ElementsAre; +using ::testing::Ge; using ::testing::HasSubstr; using ::testing::InvokeWithoutArgs; using ::testing::IsEmpty; @@ -69,6 +71,7 @@ using ::testing::Not; using ::testing::NotNull; using ::testing::Optional; using ::testing::Pair; +using ::testing::SizeIs; using BidiReadStream = google::cloud::AsyncStreamingReadWriteRpc< google::storage::v2::BidiReadObjectRequest, @@ -463,6 +466,108 @@ TEST(AsyncConnectionImplTest, OpenFeatureHeaderWithoutMultiStream) { Not(Contains(Key(kFeaturesHeader)))); } +// The header is not something only the first stream carries. With the +// multi-stream optimization on, the descriptor opens an extra stream in the +// background as soon as the first one is up, and later re-opens streams on +// resume. All of them go through the same factory, so this test opens a +// descriptor and checks every `AsyncBidiReadObject()` call it triggers. +TEST(AsyncConnectionImplTest, OpenFeatureHeaderOnEveryDescriptorStream) { + auto constexpr kExpectedRequest = R"pb( + bucket: "test-only-invalid" + object: "test-object" + )pb"; + auto const expected = + EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)); + + ValidateMetadataFixture fixture; + std::vector> metadata; + AsyncSequencer sequencer; + + // The stream the caller opens on. It serves one response and then stays + // open, as in a healthy session, until the test releases the parked read. + auto parked_stream = [&sequencer] { + auto stream = std::make_unique>(); + ON_CALL(*stream, Start).WillByDefault(InvokeWithoutArgs([] { + return make_ready_future(true); + })); + ON_CALL(*stream, Write).WillByDefault([](auto const&, auto) { + return make_ready_future(true); + }); + ON_CALL(*stream, Read) + .WillByDefault(InvokeWithoutArgs([&sequencer, n = 0]() mutable { + using Response = google::storage::v2::BidiReadObjectResponse; + if (n++ == 0) { + return make_ready_future(std::optional(Response{})); + } + return sequencer.PushBack("Read").then( + [](auto) { return std::optional(); }); + })); + ON_CALL(*stream, Finish).WillByDefault(InvokeWithoutArgs([] { + return make_ready_future(Status{}); + })); + return std::unique_ptr(std::move(stream)); + }; + + // The streams the descriptor opens in the background. They never start, so + // the open retries and eventually gives up. That is fine: only the metadata + // each attempt was sent with matters here. + auto failing_stream = [] { + auto stream = std::make_unique>(); + ON_CALL(*stream, Start).WillByDefault(InvokeWithoutArgs([] { + return make_ready_future(false); + })); + ON_CALL(*stream, Finish).WillByDefault(InvokeWithoutArgs([] { + return make_ready_future(TransientError()); + })); + return std::unique_ptr(std::move(stream)); + }; + + auto mock = std::make_shared(); + EXPECT_CALL(*mock, AsyncBidiReadObject) + // `AtLeast(2)` matters: with a single stream the test would pass + // without exercising the descriptor's own opens at all. + .Times(AtLeast(2)) + .WillOnce([&](CompletionQueue const&, + std::shared_ptr const& context, + google::cloud::internal::ImmutableOptions const&) { + metadata.push_back(fixture.GetMetadata(*context)); + return parked_stream(); + }) + .WillRepeatedly([&](CompletionQueue const&, + std::shared_ptr const& context, + google::cloud::internal::ImmutableOptions const&) { + metadata.push_back(fixture.GetMetadata(*context)); + return failing_stream(); + }); + + auto mock_cq = std::make_shared(); + EXPECT_CALL(*mock_cq, MakeRelativeTimer) + .WillRepeatedly([](std::chrono::nanoseconds) { + return make_ready_future( + StatusOr( + std::chrono::system_clock::now())); + }); + auto connection = std::make_shared( + CompletionQueue(mock_cq), std::shared_ptr(), mock, + TestOptions( + Options{}.set(true))); + + auto request = google::storage::v2::BidiReadObjectSpec{}; + ASSERT_TRUE(TextFormat::ParseFromString(kExpectedRequest, &request)); + auto p = connection->Open({std::move(request), connection->options()}).get(); + ASSERT_THAT(p, IsOkAndHolds(NotNull())); + auto descriptor = *std::move(p); + descriptor.reset(); + + // Release the parked read so the stream can wind down. + auto next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Read"); + next.first.set_value(false); + + ASSERT_THAT(metadata, SizeIs(Ge(2))); + EXPECT_THAT(metadata, Each(Contains(Pair(kFeaturesHeader, expected)))); +} + TEST(AsyncConnectionImplTest, TooManyTransientErrors) { ScopedLog log; diff --git a/google/cloud/storage/internal/async/connection_impl_read_test.cc b/google/cloud/storage/internal/async/connection_impl_read_test.cc index 8dec0635ee755..0bebcd964ef97 100644 --- a/google/cloud/storage/internal/async/connection_impl_read_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_read_test.cc @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "google/cloud/storage/async/options.h" #include "google/cloud/storage/async/retry_policy.h" #include "google/cloud/storage/internal/async/connection_impl.h" #include "google/cloud/storage/internal/async/default_options.h" #include "google/cloud/storage/internal/crc32c.h" #include "google/cloud/storage/internal/grpc/ctype_cord_workaround.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/testing/canonical_errors.h" #include "google/cloud/storage/testing/mock_hash_function.h" #include "google/cloud/storage/testing/mock_resume_policy.h" @@ -57,9 +59,12 @@ using ::google::cloud::testing_util::StatusIs; using ::google::cloud::testing_util::ValidateMetadataFixture; using ::google::protobuf::TextFormat; using ::testing::AllOf; +using ::testing::Contains; using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::IsEmpty; +using ::testing::Key; +using ::testing::Not; using ::testing::Optional; using ::testing::ResultOf; using ::testing::Return; @@ -625,6 +630,81 @@ TEST_F(AsyncConnectionImplTest, ReadObjectRangePermanentError) { EXPECT_THAT(pending.get(), StatusIs(PermanentError().code())); } +// The multi-stream optimization only applies to object descriptors. The +// plain `ReadObject()` and `ReadObjectRange()` paths must not report it even +// though the option is set (it is on by default). +TEST_F(AsyncConnectionImplTest, ReadObjectDoesNotSendFeaturesHeader) { + AsyncSequencer sequencer; + auto mock = std::make_shared(); + EXPECT_CALL(*mock, AsyncReadObject) + .WillOnce([&](CompletionQueue const&, + std::shared_ptr const& context, + google::cloud::internal::ImmutableOptions const& options, + google::storage::v2::ReadObjectRequest const&) { + EXPECT_TRUE( + options->get()); + EXPECT_THAT(GetMetadata(*context), + Not(Contains(Key(storage_internal::kFeaturesHeader)))); + return MakeErrorReadStream(sequencer, PermanentError()); + }); + + internal::AutomaticallyCreatedBackgroundThreads pool(1); + auto connection = MakeTestConnection( + pool.cq(), mock, + Options{}.set(true)); + auto pending = connection->ReadObject( + {google::storage::v2::ReadObjectRequest{}, connection->options()}); + ASSERT_TRUE(pending.is_ready()); + auto r = pending.get(); + ASSERT_STATUS_OK(r); + auto reader = *std::move(r); + auto data = reader->Read(); + + auto next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Start"); + next.first.set_value(false); + + next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Finish"); + next.first.set_value(true); + + auto response = data.get(); + EXPECT_THAT(response, VariantWith(StatusIs(PermanentError().code()))); +} + +TEST_F(AsyncConnectionImplTest, ReadObjectRangeDoesNotSendFeaturesHeader) { + AsyncSequencer sequencer; + auto mock = std::make_shared(); + EXPECT_CALL(*mock, AsyncReadObject) + .WillOnce([&](CompletionQueue const&, + std::shared_ptr const& context, + google::cloud::internal::ImmutableOptions const& options, + google::storage::v2::ReadObjectRequest const&) { + EXPECT_TRUE( + options->get()); + EXPECT_THAT(GetMetadata(*context), + Not(Contains(Key(storage_internal::kFeaturesHeader)))); + return MakeErrorReadStream(sequencer, PermanentError()); + }); + + internal::AutomaticallyCreatedBackgroundThreads pool(1); + auto connection = MakeTestConnection( + pool.cq(), mock, + Options{}.set(true)); + auto pending = connection->ReadObjectRange( + {google::storage::v2::ReadObjectRequest{}, connection->options()}); + ASSERT_FALSE(pending.is_ready()); + auto next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Start"); + next.first.set_value(false); + + next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, "Finish"); + next.first.set_value(true); + + EXPECT_THAT(pending.get(), StatusIs(PermanentError().code())); +} + // `ReadObjectRange()` on a 0-byte object must return the object metadata sent // by the service, even though there is no data. TEST_F(AsyncConnectionImplTest, ReadObjectRangeZeroByteObjectKeepsMetadata) { diff --git a/google/cloud/storage/internal/connection_impl_test.cc b/google/cloud/storage/internal/connection_impl_test.cc index 8a1d75a781afb..4b23ab6ee0dc3 100644 --- a/google/cloud/storage/internal/connection_impl_test.cc +++ b/google/cloud/storage/internal/connection_impl_test.cc @@ -830,7 +830,12 @@ ReadSourceResult MakeReadResult(std::string const& payload, char* buf) { // The feature tracking header reports read hedging to the service, and only // when hedging is in effect. The primary attempt is opened lazily by the read // pool, so the header is observed on the first `Read()`. -std::vector FeatureHeaderForReadObject(Options options) { +// +// The connection is created with @p connection_options, which decides whether +// the hedging thread pools exist. The read runs with @p call_options layered +// on top, which decides whether hedging is requested for this call. +std::vector FeatureHeaderForReadObject(Options connection_options, + Options call_options) { auto captured = std::make_shared>(); auto mock = std::make_unique(); EXPECT_CALL(*mock, options).Times(AtLeast(0)); @@ -845,11 +850,12 @@ std::vector FeatureHeaderForReadObject(Options options) { }); return StatusOr>(std::move(source)); }); - auto client = StorageConnectionImpl::Create(std::move(mock), options); + auto client = StorageConnectionImpl::Create(std::move(mock), + std::move(connection_options)); std::vector buffer(100); google::cloud::internal::OptionsSpan const span( - client->options().set( - std::chrono::seconds(30))); + google::cloud::internal::MergeOptions(std::move(call_options), + client->options())); StatusOr> source = client->ReadObject(ReadObjectRangeRequest("test-bucket", "test-object")); EXPECT_THAT(source, IsOk()); @@ -858,20 +864,90 @@ std::vector FeatureHeaderForReadObject(Options options) { return *captured; } +Options HedgingConnectionOptions() { + return BasicTestPolicies() + .set(true) + .set(1) + .set(1); +} + +// A hedge delay long enough that the primary always wins, so the mock sees +// exactly one `ReadObject()` call. +Options HedgingCallOptions() { + return Options{}.set( + std::chrono::seconds(30)); +} + TEST(RetryClientTest, ReadObjectFeatureHeaderWithHedging) { - auto const expected = - storage_internal::EncodeFeatureMask(storage_internal::FeatureBit( - storage_internal::TrackedFeature::kReadHedging)); + // Pinned to the literal encoding: the service decodes the value by + // contract, not by calling this library. + EXPECT_EQ(storage_internal::EncodeFeatureMask(storage_internal::FeatureBit( + storage_internal::TrackedFeature::kReadHedging)), + "AAAAAg=="); + EXPECT_THAT(FeatureHeaderForReadObject(HedgingConnectionOptions(), + HedgingCallOptions()), + ElementsAre("AAAAAg==")); +} + +TEST(RetryClientTest, ReadObjectFeatureHeaderWithoutHedging) { + EXPECT_THAT(FeatureHeaderForReadObject(BasicTestPolicies(), Options{}), + IsEmpty()); +} + +// Hedging is enabled but can never dispatch a hedge: the feature is not in +// effect, so it must not be reported. +TEST(RetryClientTest, ReadObjectFeatureHeaderWithZeroMaxHedges) { + EXPECT_THAT( + FeatureHeaderForReadObject( + HedgingConnectionOptions(), + HedgingCallOptions().set( + 0)), + IsEmpty()); +} + +// Hedging requested per-call, but the connection was created without it and +// so has no hedging thread pools. The read falls back to a plain read, and +// the header must say so by being absent. +TEST(RetryClientTest, ReadObjectFeatureHeaderWithoutHedgingPools) { EXPECT_THAT(FeatureHeaderForReadObject( - BasicTestPolicies() + BasicTestPolicies(), + HedgingCallOptions() .set(true) - .set(1) - .set(1)), - ElementsAre(expected)); + .set(1)), + IsEmpty()); } -TEST(RetryClientTest, ReadObjectFeatureHeaderWithoutHedging) { - EXPECT_THAT(FeatureHeaderForReadObject(BasicTestPolicies()), IsEmpty()); +// Read hedging only applies to `ReadObject()`. A connection with hedging in +// effect must not report it on any other RPC. +TEST(RetryClientTest, FeatureHeaderNotSentOnOtherRpcs) { + auto mock = std::make_unique(); + EXPECT_CALL(*mock, options).Times(AtLeast(0)); + EXPECT_CALL(*mock, GetObjectMetadata) + .WillOnce([](rest_internal::RestContext& context, Options const&, + GetObjectMetadataRequest const&) { + EXPECT_THAT(context.GetHeader(storage_internal::kFeaturesHeader), + IsEmpty()); + return StatusOr(PermanentError()); + }); + EXPECT_CALL(*mock, DeleteObject) + .WillOnce([](rest_internal::RestContext& context, Options const&, + DeleteObjectRequest const&) { + EXPECT_THAT(context.GetHeader(storage_internal::kFeaturesHeader), + IsEmpty()); + return StatusOr(PermanentError()); + }); + auto client = StorageConnectionImpl::Create(std::move(mock), + HedgingConnectionOptions()); + google::cloud::internal::OptionsSpan const span( + google::cloud::internal::MergeOptions(HedgingCallOptions(), + client->options())); + + EXPECT_THAT(client->GetObjectMetadata( + GetObjectMetadataRequest("test-bucket", "test-object")), + StatusIs(PermanentError().code())); + EXPECT_THAT( + client->DeleteObject(DeleteObjectRequest("test-bucket", "test-object")), + StatusIs(PermanentError().code())); } TEST(RetryClientTest, HedgedReadRecordsMetricsOnGlobalMeterProvider) { diff --git a/google/cloud/storage/internal/grpc/stub_test.cc b/google/cloud/storage/internal/grpc/stub_test.cc index 039902bc85749..1837a7ed81e68 100644 --- a/google/cloud/storage/internal/grpc/stub_test.cc +++ b/google/cloud/storage/internal/grpc/stub_test.cc @@ -14,6 +14,7 @@ #include "google/cloud/storage/internal/grpc/stub.h" #include "google/cloud/storage/grpc_plugin.h" +#include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/storage/testing/mock_storage_stub.h" #include "google/cloud/credentials.h" #include "google/cloud/grpc_options.h" @@ -607,6 +608,47 @@ TEST_F(GrpcClientTest, ReadObject) { QuotaUser("test-quota-user"))); } +// The connection layer records the client features in effect for a read on +// the `RestContext`. The gRPC stub must forward them as metadata, the same +// way it forwards the idempotency token. +TEST_F(GrpcClientTest, ReadObjectForwardsFeaturesHeader) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, ReadObject) + .WillOnce([this](auto context, Options const&, + v2::ReadObjectRequest const&) { + EXPECT_THAT(GetMetadata(*context), + UnorderedElementsAre( + Pair(kIdempotencyTokenHeader, "test-token-1234"), + Pair(storage_internal::kFeaturesHeader, "AAAAAg=="))); + return std::make_unique(); + }); + auto client = CreateTestClient(mock); + auto context = + TestContext().AddHeader(storage_internal::kFeaturesHeader, "AAAAAg=="); + auto stream = client->ReadObject( + context, TestOptions(), + storage::internal::ReadObjectRangeRequest("test-bucket", "test-object")); +} + +// Without the header on the `RestContext` there must be no metadata entry at +// all, not an empty one. +TEST_F(GrpcClientTest, ReadObjectWithoutFeaturesHeader) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, ReadObject) + .WillOnce( + [this](auto context, Options const&, v2::ReadObjectRequest const&) { + EXPECT_THAT(GetMetadata(*context), + UnorderedElementsAre( + Pair(kIdempotencyTokenHeader, "test-token-1234"))); + return std::make_unique(); + }); + auto client = CreateTestClient(mock); + auto context = TestContext(); + auto stream = client->ReadObject( + context, TestOptions(), + storage::internal::ReadObjectRangeRequest("test-bucket", "test-object")); +} + TEST_F(GrpcClientTest, ListObjects) { auto mock = std::make_shared(); EXPECT_CALL(*mock, ListObjects) diff --git a/google/cloud/storage/internal/tracked_features_test.cc b/google/cloud/storage/internal/tracked_features_test.cc index c699643814d27..852590e446fdd 100644 --- a/google/cloud/storage/internal/tracked_features_test.cc +++ b/google/cloud/storage/internal/tracked_features_test.cc @@ -14,6 +14,7 @@ #include "google/cloud/storage/internal/tracked_features.h" #include +#include #include #include @@ -24,16 +25,66 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { using ::testing::IsEmpty; +using ::testing::Not; + +// Every tracked feature. Add new enumerators here so the tests below cover +// them. +constexpr std::array kAllFeatures = { + TrackedFeature::kMultistreamInMrd, + TrackedFeature::kReadHedging, +}; + +// The enumerators are bit positions in a 32-bit mask, so each must fit and no +// two may collide. Both are checked at compile time; a bad enumerator fails +// the build rather than the test. +constexpr bool AllBitPositionsFit() { + for (auto feature : kAllFeatures) { + if (static_cast(feature) >= 32) return false; + } + return true; +} + +constexpr bool AllBitPositionsDistinct() { + for (std::size_t i = 0; i != kAllFeatures.size(); ++i) { + for (std::size_t j = i + 1; j != kAllFeatures.size(); ++j) { + if (kAllFeatures[i] == kAllFeatures[j]) return false; + } + } + return true; +} + +static_assert(AllBitPositionsFit(), + "TrackedFeature values must be bit positions below 32"); +static_assert(AllBitPositionsDistinct(), + "TrackedFeature values must not be reused"); TEST(TrackedFeatures, HeaderName) { EXPECT_EQ(std::string(kFeaturesHeader), "x-goog-storage-cpp-features"); } +// The bit assignment is a contract with the service: moving a feature to a +// different bit would silently change what the service records. TEST(TrackedFeatures, FeatureBit) { EXPECT_EQ(FeatureBit(TrackedFeature::kMultistreamInMrd), std::uint32_t{1}); EXPECT_EQ(FeatureBit(TrackedFeature::kReadHedging), std::uint32_t{2}); } +TEST(TrackedFeatures, FeatureBitsAreSingleDistinctBits) { + std::uint32_t seen = 0; + for (auto feature : kAllFeatures) { + auto const bit = FeatureBit(feature); + // Exactly one bit set. + EXPECT_NE(bit, 0U); + EXPECT_EQ(bit & (bit - 1), 0U) << "feature bit " << bit; + // Not shared with any other feature. + EXPECT_EQ(seen & bit, 0U) << "feature bit " << bit; + seen |= bit; + } +} + +// A zero mask means "no header": `EncodeFeatureMask()` returns an empty string +// so callers can distinguish it from any real mask, all of which encode to 8 +// characters. TEST(TrackedFeatures, EncodeZeroIsEmpty) { EXPECT_THAT(EncodeFeatureMask(0), IsEmpty()); } @@ -43,13 +94,30 @@ TEST(TrackedFeatures, EncodeZeroIsEmpty) { TEST(TrackedFeatures, EncodeKnownValues) { EXPECT_EQ(EncodeFeatureMask(1), "AAAAAQ=="); EXPECT_EQ(EncodeFeatureMask(2), "AAAAAg=="); + EXPECT_EQ(EncodeFeatureMask(3), "AAAAAw=="); EXPECT_EQ(EncodeFeatureMask(4), "AAAABA=="); + // 0xF8 and 0xFC put 62 and 63 in a sextet: the two characters that differ + // between the standard and the URL-safe alphabets. EXPECT_EQ(EncodeFeatureMask(0xF8), "AAAA-A=="); + EXPECT_EQ(EncodeFeatureMask(0xFC), "AAAA_A=="); EXPECT_EQ(EncodeFeatureMask(0xFFFFFFFF), "_____w=="); +} + +TEST(TrackedFeatures, EncodeTrackedFeatures) { EXPECT_EQ(EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)), "AAAAAQ=="); EXPECT_EQ(EncodeFeatureMask(FeatureBit(TrackedFeature::kReadHedging)), "AAAAAg=="); + EXPECT_EQ(EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd) | + FeatureBit(TrackedFeature::kReadHedging)), + "AAAAAw=="); +} + +// The value is big-endian: the low byte of the mask is the last byte encoded. +TEST(TrackedFeatures, EncodeIsBigEndian) { + EXPECT_EQ(EncodeFeatureMask(0x01000000), "AQAAAA=="); + EXPECT_EQ(EncodeFeatureMask(0x00000001), "AAAAAQ=="); + EXPECT_NE(EncodeFeatureMask(0x01000000), EncodeFeatureMask(0x00000001)); } TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { @@ -60,6 +128,16 @@ TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { EXPECT_EQ(encoded.find_first_of("+/"), std::string::npos) << encoded; EXPECT_EQ(encoded.size(), 8U) << encoded; } + // A 32-bit value spans five full sextets, starting at bits 26, 20, 14, 8, + // and 2. Put 62 and 63, the two sextets that differ between the alphabets, + // in each of them. + for (int shift : {26, 20, 14, 8, 2}) { + for (std::uint32_t sextet : {0x3EU, 0x3FU}) { + auto const encoded = EncodeFeatureMask(sextet << shift); + EXPECT_THAT(encoded, Not(IsEmpty())); + EXPECT_EQ(encoded.find_first_of("+/"), std::string::npos) << encoded; + } + } } } // namespace From 412834e57344a249915dfe8494744f834ef92d71 Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 26 Oct 2026 06:40:24 +0000 Subject: [PATCH 4/7] refactor(storage): forward RestContext headers via an allowlist Rename `AddIdempotencyToken(grpc::ClientContext&, RestContext const&)` to `ApplyRestContextHeaders()`, since it now forwards more than the token. The headers it copies are an explicit allowlist (`kForwardedHeaders`: idempotency token and client feature mask), so REST-only headers on the `RestContext` never leak into gRPC metadata. The string overload of `AddIdempotencyToken()` is unchanged. Also adds `AddFeaturesHeader(grpc::ClientContext&, std::uint32_t)`; it lives here rather than in `tracked_features.h` because `google_cloud_cpp_storage` does not depend on gRPC. --- .../internal/grpc/configure_client_context.cc | 23 ++++++-- .../internal/grpc/configure_client_context.h | 24 +++++++- .../grpc/configure_client_context_test.cc | 58 +++++++++++++------ google/cloud/storage/internal/grpc/stub.cc | 54 ++++++++--------- 4 files changed, 105 insertions(+), 54 deletions(-) diff --git a/google/cloud/storage/internal/grpc/configure_client_context.cc b/google/cloud/storage/internal/grpc/configure_client_context.cc index 72e8ef8d10f88..60f8919dd1b35 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context.cc @@ -15,7 +15,11 @@ #include "google/cloud/storage/internal/grpc/configure_client_context.h" #include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/internal/url_encode.h" +#include +#include #include +#include +#include namespace google { namespace cloud { @@ -25,6 +29,11 @@ namespace { auto constexpr kIdempotencyTokenHeader = "x-goog-gcs-idempotency-token"; +// The `RestContext` headers forwarded to gRPC as metadata. Anything else on +// the `RestContext` is specific to the REST transport. +constexpr std::array kForwardedHeaders{kIdempotencyTokenHeader, + kFeaturesHeader}; + void ApplyRoutingHeadersImpl(grpc::ClientContext& context, std::string const& bucket_name, RoutingHeaderOptions const& options) { @@ -49,18 +58,20 @@ void CopyHeader(grpc::ClientContext& ctx, } // namespace -void AddIdempotencyToken(grpc::ClientContext& ctx, - rest_internal::RestContext const& context) { - CopyHeader(ctx, context, kIdempotencyTokenHeader); - // The connection layer sets the client feature mask on the `RestContext` so - // it is transport-agnostic; forward it alongside the idempotency token. - CopyHeader(ctx, context, kFeaturesHeader); +void ApplyRestContextHeaders(grpc::ClientContext& ctx, + rest_internal::RestContext const& context) { + for (auto const* name : kForwardedHeaders) CopyHeader(ctx, context, name); } void AddIdempotencyToken(grpc::ClientContext& ctx, std::string const& token) { ctx.AddMetadata(kIdempotencyTokenHeader, token); } +void AddFeaturesHeader(grpc::ClientContext& ctx, std::uint32_t mask) { + if (mask == 0) return; + ctx.AddMetadata(kFeaturesHeader, EncodeFeatureMask(mask)); +} + void ApplyRoutingHeaders( grpc::ClientContext& context, storage::internal::InsertObjectMediaRequest const& request) { diff --git a/google/cloud/storage/internal/grpc/configure_client_context.h b/google/cloud/storage/internal/grpc/configure_client_context.h index f919a0cd79544..c7bc7a009d485 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context.h +++ b/google/cloud/storage/internal/grpc/configure_client_context.h @@ -24,6 +24,8 @@ #include "google/storage/v2/storage.pb.h" #include #include +#include +#include #include namespace google { @@ -36,13 +38,29 @@ struct RoutingHeaderOptions { std::string routing_token; }; -/// Configures @p ctx using @p context. -void AddIdempotencyToken(grpc::ClientContext& ctx, - rest_internal::RestContext const& context); +/** + * Copies the headers the connection layer sets on the `RestContext` + * (idempotency token, client feature mask) into the gRPC `ClientContext`. + * + * Only an explicit allowlist of headers is forwarded; the `RestContext` may + * carry headers that are meaningful to the REST transport alone. + */ +void ApplyRestContextHeaders(grpc::ClientContext& ctx, + rest_internal::RestContext const& context); /// Configures @p ctx using @p token. void AddIdempotencyToken(grpc::ClientContext& ctx, std::string const& token); +/** + * Sets `storage_internal::kFeaturesHeader` on @p ctx to the encoding of + * @p mask. + * + * Does nothing when @p mask is zero: the header is omitted, not sent empty. + * This is the gRPC counterpart of the `RestContext` overload declared in + * `internal/tracked_features.h`. + */ +void AddFeaturesHeader(grpc::ClientContext& ctx, std::uint32_t mask); + template struct IsStreamingRequest : std::false_type {}; diff --git a/google/cloud/storage/internal/grpc/configure_client_context_test.cc b/google/cloud/storage/internal/grpc/configure_client_context_test.cc index 4ed26262f1212..e5f279499a870 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context_test.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context_test.cc @@ -47,44 +47,66 @@ class GrpcConfigureClientContext : public ::testing::Test { ValidateMetadataFixture validate_metadata_fixture_; }; -TEST_F(GrpcConfigureClientContext, AddIdempotencyToken) { +TEST_F(GrpcConfigureClientContext, ApplyRestContextHeadersIdempotencyToken) { grpc::ClientContext ctx; auto const context = rest_internal::RestContext{}.AddHeader( "x-goog-gcs-idempotency-token", "token-123"); - AddIdempotencyToken(ctx, context); - auto metadata = GetMetadata(ctx); - EXPECT_THAT(metadata, + ApplyRestContextHeaders(ctx, context); + EXPECT_THAT(GetMetadata(ctx), Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); } -TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenString) { - grpc::ClientContext ctx; - AddIdempotencyToken(ctx, "token-123"); - auto metadata = GetMetadata(ctx); - EXPECT_THAT(metadata, - Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); -} - -TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenForwardsFeaturesHeader) { +TEST_F(GrpcConfigureClientContext, ApplyRestContextHeadersFeatures) { grpc::ClientContext ctx; auto const context = rest_internal::RestContext{} .AddHeader("x-goog-gcs-idempotency-token", "token-123") .AddHeader(kFeaturesHeader, "AAAAAg=="); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto metadata = GetMetadata(ctx); EXPECT_THAT(metadata, Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); EXPECT_THAT(metadata, Contains(Pair(kFeaturesHeader, "AAAAAg=="))); } -TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenNoFeaturesHeader) { +TEST_F(GrpcConfigureClientContext, ApplyRestContextHeadersNoFeatures) { grpc::ClientContext ctx; auto const context = rest_internal::RestContext{}.AddHeader( "x-goog-gcs-idempotency-token", "token-123"); - AddIdempotencyToken(ctx, context); - auto metadata = GetMetadata(ctx); - EXPECT_THAT(metadata, Not(Contains(Pair(kFeaturesHeader, _)))); + ApplyRestContextHeaders(ctx, context); + EXPECT_THAT(GetMetadata(ctx), Not(Contains(Pair(kFeaturesHeader, _)))); +} + +// Only the allowlisted headers cross over; anything else on the `RestContext` +// is specific to the REST transport. +TEST_F(GrpcConfigureClientContext, ApplyRestContextHeadersIgnoresOthers) { + grpc::ClientContext ctx; + auto const context = rest_internal::RestContext{} + .AddHeader("x-goog-gcs-idempotency-token", "t") + .AddHeader("x-test-only-rest-header", "v"); + ApplyRestContextHeaders(ctx, context); + EXPECT_THAT(GetMetadata(ctx), + Not(Contains(Pair("x-test-only-rest-header", _)))); +} + +TEST_F(GrpcConfigureClientContext, AddIdempotencyTokenString) { + grpc::ClientContext ctx; + AddIdempotencyToken(ctx, "token-123"); + EXPECT_THAT(GetMetadata(ctx), + Contains(Pair("x-goog-gcs-idempotency-token", "token-123"))); +} + +TEST_F(GrpcConfigureClientContext, AddFeaturesHeader) { + grpc::ClientContext ctx; + AddFeaturesHeader(ctx, FeatureBit(TrackedFeature::kMultistreamInMrd)); + EXPECT_THAT(GetMetadata(ctx), Contains(Pair(kFeaturesHeader, "AAAAAQ=="))); +} + +// A zero mask means the header is omitted, not sent empty. +TEST_F(GrpcConfigureClientContext, AddFeaturesHeaderZeroMask) { + grpc::ClientContext ctx; + AddFeaturesHeader(ctx, 0); + EXPECT_THAT(GetMetadata(ctx), Not(Contains(Pair(kFeaturesHeader, _)))); } TEST_F(GrpcConfigureClientContext, ApplyQueryParametersEmpty) { diff --git a/google/cloud/storage/internal/grpc/stub.cc b/google/cloud/storage/internal/grpc/stub.cc index 4b8a81db4beb0..17a1b4bb210b0 100644 --- a/google/cloud/storage/internal/grpc/stub.cc +++ b/google/cloud/storage/internal/grpc/stub.cc @@ -206,7 +206,7 @@ StatusOr GrpcStub::ListBuckets( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->ListBuckets(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response); @@ -218,7 +218,7 @@ StatusOr GrpcStub::CreateBucket( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->CreateBucket(ctx, options, proto); if (response) return FromProto(*response, options); // GCS returns kFailedPrecondition when the bucket already exists. I filed @@ -247,7 +247,7 @@ StatusOr GrpcStub::DeleteBucket( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto status = stub_->DeleteBucket(ctx, options, proto); if (!status.ok()) return status; return storage::internal::EmptyResponse{}; @@ -259,7 +259,7 @@ StatusOr GrpcStub::UpdateBucket( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->UpdateBucket(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -279,7 +279,7 @@ StatusOr GrpcStub::GetNativeBucketIamPolicy( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->GetIamPolicy(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response); @@ -291,7 +291,7 @@ StatusOr GrpcStub::SetNativeBucketIamPolicy( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->SetIamPolicy(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response); @@ -304,7 +304,7 @@ GrpcStub::TestBucketIamPermissions( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->TestIamPermissions(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response); @@ -316,7 +316,7 @@ StatusOr GrpcStub::LockBucketRetentionPolicy( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->LockBucketRetentionPolicy(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -344,7 +344,7 @@ StatusOr GrpcStub::InsertObjectMedia( auto ctx = std::make_shared(); ApplyQueryParameters(*ctx, options, request); - AddIdempotencyToken(*ctx, context); + ApplyRestContextHeaders(*ctx, context); ApplyRoutingHeaders(*ctx, request); auto stream = stub_->WriteObject(std::move(ctx), options); @@ -410,7 +410,7 @@ StatusOr GrpcStub::CopyObject( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->RewriteObject(ctx, options, *proto); if (!response) return std::move(response).status(); if (!response->done()) { @@ -435,7 +435,7 @@ GrpcStub::ReadObject(rest_internal::RestContext& context, storage::internal::ReadObjectRangeRequest const& request) { auto ctx = std::make_shared(); ApplyQueryParameters(*ctx, options, request); - AddIdempotencyToken(*ctx, context); + ApplyRestContextHeaders(*ctx, context); auto proto_request = ToProto(request); if (!proto_request) return std::move(proto_request).status(); auto stream = stub_->ReadObject(std::move(ctx), options, *proto_request); @@ -474,7 +474,7 @@ StatusOr GrpcStub::ListObjects( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->ListObjects(ctx, options, proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -486,7 +486,7 @@ StatusOr GrpcStub::DeleteObject( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->DeleteObject(ctx, options, proto); if (!response.ok()) return response; return storage::internal::EmptyResponse{}; @@ -499,7 +499,7 @@ StatusOr GrpcStub::UpdateObject( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->UpdateObject(ctx, options, *proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -512,7 +512,7 @@ StatusOr GrpcStub::MoveObject( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->MoveObject(ctx, options, *proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -533,7 +533,7 @@ StatusOr GrpcStub::ComposeObject( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->ComposeObject(ctx, options, *proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -546,7 +546,7 @@ StatusOr GrpcStub::RewriteObject( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->RewriteObject(ctx, options, *proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -559,7 +559,7 @@ StatusOr GrpcStub::RestoreObject( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->RestoreObject(ctx, options, *proto); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -574,7 +574,7 @@ GrpcStub::CreateResumableUpload( grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->StartResumableWrite(ctx, options, *proto_request); if (!response.ok()) return std::move(response).status(); @@ -588,7 +588,7 @@ GrpcStub::QueryResumableUpload( storage::internal::QueryResumableUploadRequest const& request) { grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->QueryWriteStatus(ctx, options, ToProto(request)); if (!response) return std::move(response).status(); return FromProto(*response, options); @@ -599,7 +599,7 @@ StatusOr GrpcStub::DeleteResumableUpload( storage::internal::DeleteResumableUploadRequest const& request) { grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = stub_->CancelResumableWrite(ctx, options, ToProto(request)); if (!response) return std::move(response).status(); return storage::internal::EmptyResponse{}; @@ -626,7 +626,7 @@ StatusOr GrpcStub::UploadChunk( auto ctx = std::make_shared(); ApplyQueryParameters(*ctx, options, request); - AddIdempotencyToken(*ctx, context); + ApplyRestContextHeaders(*ctx, context); ApplyRoutingHeaders(*ctx, request); auto stream = stub_->WriteObject(std::move(ctx), options); @@ -1033,7 +1033,7 @@ StatusOr GrpcStub::SignBlob( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); auto response = iam_stub_->SignBlob(ctx, proto); if (!response) return std::move(response).status(); return FromProto(*response); @@ -1074,7 +1074,7 @@ StatusOr GrpcStub::GetBucketMetadataImpl( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); return stub_->GetBucket(ctx, options, proto); } @@ -1085,7 +1085,7 @@ StatusOr GrpcStub::PatchBucketImpl( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); return stub_->UpdateBucket(ctx, options, *proto); } @@ -1095,7 +1095,7 @@ StatusOr GrpcStub::GetObjectMetadataImpl( auto proto = ToProto(request); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); return stub_->GetObject(ctx, options, proto); } @@ -1106,7 +1106,7 @@ StatusOr GrpcStub::PatchObjectImpl( if (!proto) return std::move(proto).status(); grpc::ClientContext ctx; ApplyQueryParameters(ctx, options, request); - AddIdempotencyToken(ctx, context); + ApplyRestContextHeaders(ctx, context); return stub_->UpdateObject(ctx, options, *proto); } From 7f1294e52c4e3938a3c0c69e170648e9917894dd Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 26 Oct 2026 06:40:25 +0000 Subject: [PATCH 5/7] refactor(storage): add AddFeaturesHeader() helpers * `FeatureBit()` is now `constexpr` in the header, and the header `static_assert`s that the enumerators are distinct bit positions below 32 (the test-side duplicates are gone). * `AddFeaturesHeader(RestContext&, mask)` and the gRPC overload are no-ops for a zero mask, so callers no longer re-check for the empty encoding. The sync `ReadObject()` and the async open-stream factory use them instead of hand-rolled blocks. * Drop the redundant `& 0xFF` after `static_cast` in `EncodeFeatureMask()`, and the duplicated encoding literal assertion in `connection_impl_test`. --- .../storage/internal/async/connection_impl.cc | 4 +- .../cloud/storage/internal/connection_impl.cc | 20 ++++---- .../storage/internal/connection_impl_test.cc | 7 +-- .../storage/internal/tracked_features.cc | 18 +++---- .../cloud/storage/internal/tracked_features.h | 29 ++++++++++-- .../storage/internal/tracked_features_test.cc | 47 +++++++++---------- 6 files changed, 71 insertions(+), 54 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 77fde1760ca07..40b536c920906 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -201,9 +201,7 @@ OpenStreamFactory MakeOpenStreamFactory( if (options->get()) { features |= FeatureBit(TrackedFeature::kMultistreamInMrd); } - if (features != 0) { - context->AddMetadata(kFeaturesHeader, EncodeFeatureMask(features)); - } + AddFeaturesHeader(*context, features); auto open = std::make_shared( *stub, cq, std::move(context), std::move(options), request); // Extend the lifetime of the coroutine until it finishes. diff --git a/google/cloud/storage/internal/connection_impl.cc b/google/cloud/storage/internal/connection_impl.cc index 5e77a79f0e26e..91fc48e2b434f 100644 --- a/google/cloud/storage/internal/connection_impl.cc +++ b/google/cloud/storage/internal/connection_impl.cc @@ -48,6 +48,9 @@ namespace { using ::google::cloud::Idempotency; using ::google::cloud::internal::MergeOptions; using ::google::cloud::rest_internal::RestRetryLoop; +using ::google::cloud::storage_internal::AddFeaturesHeader; +using ::google::cloud::storage_internal::FeatureBit; +using ::google::cloud::storage_internal::TrackedFeature; // Returns an error if the response contains an unexpected (or invalid) // committed size. @@ -420,18 +423,16 @@ StatusOr> StorageConnectionImpl::ReadObject( enable_hedging && max_hedges > 0 && hedge_pool_ && read_pool_; // Tell the service which client-side features shaped this read. Only the - // features actually in effect are reported, and the header is omitted when - // there are none. + // features actually in effect are reported; `kReadHedging` means hedging is + // in effect for this read, not that a hedge was dispatched. std::uint32_t features = 0; if (hedging_active) { - features |= storage_internal::FeatureBit( - storage_internal::TrackedFeature::kReadHedging); + features |= FeatureBit(TrackedFeature::kReadHedging); } - auto const features_value = storage_internal::EncodeFeatureMask(features); auto self = shared_from_this(); auto const* where = __func__; - auto factory = [self = shared_from_this(), current, where, features_value]( + auto factory = [self = shared_from_this(), current, where, features]( ReadObjectRangeRequest const& request, RetryPolicy& retry_policy, BackoffPolicy& backoff_policy) { auto const idempotency = @@ -449,14 +450,11 @@ StatusOr> StorageConnectionImpl::ReadObject( request.object_name())); return RestRetryLoop( logging_retry_policy, backoff_policy, idempotency, - [self, token = self->MakeIdempotencyToken(), features_value]( + [self, token = self->MakeIdempotencyToken(), features]( rest_internal::RestContext& context, Options const& options, ReadObjectRangeRequest const& request) { context.AddHeader(kIdempotencyTokenHeader, token); - if (!features_value.empty()) { - context.AddHeader(storage_internal::kFeaturesHeader, - features_value); - } + AddFeaturesHeader(context, features); return self->stub_->ReadObject(context, options, request); }, *current, request, where); diff --git a/google/cloud/storage/internal/connection_impl_test.cc b/google/cloud/storage/internal/connection_impl_test.cc index 4b23ab6ee0dc3..0f59b5d082378 100644 --- a/google/cloud/storage/internal/connection_impl_test.cc +++ b/google/cloud/storage/internal/connection_impl_test.cc @@ -879,11 +879,8 @@ Options HedgingCallOptions() { } TEST(RetryClientTest, ReadObjectFeatureHeaderWithHedging) { - // Pinned to the literal encoding: the service decodes the value by - // contract, not by calling this library. - EXPECT_EQ(storage_internal::EncodeFeatureMask(storage_internal::FeatureBit( - storage_internal::TrackedFeature::kReadHedging)), - "AAAAAg=="); + // "AAAAAg==" is `FeatureBit(TrackedFeature::kReadHedging)`; the encoding + // itself is pinned in `tracked_features_test.cc`. EXPECT_THAT(FeatureHeaderForReadObject(HedgingConnectionOptions(), HedgingCallOptions()), ElementsAre("AAAAAg==")); diff --git a/google/cloud/storage/internal/tracked_features.cc b/google/cloud/storage/internal/tracked_features.cc index bbd21d26ab9b4..9b66e0319f949 100644 --- a/google/cloud/storage/internal/tracked_features.cc +++ b/google/cloud/storage/internal/tracked_features.cc @@ -21,21 +21,23 @@ namespace cloud { namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN -std::uint32_t FeatureBit(TrackedFeature feature) { - return std::uint32_t{1} << static_cast(feature); -} - std::string EncodeFeatureMask(std::uint32_t mask) { if (mask == 0) return {}; std::array const bytes{ - static_cast((mask >> 24) & 0xFF), - static_cast((mask >> 16) & 0xFF), - static_cast((mask >> 8) & 0xFF), - static_cast(mask & 0xFF), + static_cast(mask >> 24), + static_cast(mask >> 16), + static_cast(mask >> 8), + static_cast(mask), }; return google::cloud::internal::UrlsafeBase64EncodeWithPadding(bytes); } +void AddFeaturesHeader(rest_internal::RestContext& context, + std::uint32_t mask) { + if (mask == 0) return; + context.AddHeader(kFeaturesHeader, EncodeFeatureMask(mask)); +} + GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud diff --git a/google/cloud/storage/internal/tracked_features.h b/google/cloud/storage/internal/tracked_features.h index 8050ed062c8cd..8d96c6b25752c 100644 --- a/google/cloud/storage/internal/tracked_features.h +++ b/google/cloud/storage/internal/tracked_features.h @@ -16,6 +16,7 @@ #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_TRACKED_FEATURES_H #include "google/cloud/storage/version.h" +#include "google/cloud/internal/rest_context.h" #include #include @@ -46,17 +47,39 @@ enum class TrackedFeature : std::uint32_t { kReadHedging = 1, }; +// The mask is 32 bits wide, so every enumerator must be a position below 32, +// and no two features may share a position. +static_assert(static_cast(TrackedFeature::kMultistreamInMrd) < + 32, + "TrackedFeature values must be bit positions below 32"); +static_assert(static_cast(TrackedFeature::kReadHedging) < 32, + "TrackedFeature values must be bit positions below 32"); +static_assert(TrackedFeature::kMultistreamInMrd != TrackedFeature::kReadHedging, + "TrackedFeature values must not be reused"); + /// Returns the bit value (`1 << position`) for @p feature. -std::uint32_t FeatureBit(TrackedFeature feature); +constexpr std::uint32_t FeatureBit(TrackedFeature feature) { + return std::uint32_t{1} << static_cast(feature); +} /** * Encodes @p mask as the value of `kFeaturesHeader`. * - * Returns an empty string when @p mask is zero, in which case the header must - * not be sent. + * Returns an empty string when @p mask is zero. Callers that just want the + * header set (or not) should prefer `AddFeaturesHeader()`, which handles that + * case. */ std::string EncodeFeatureMask(std::uint32_t mask); +/** + * Sets `kFeaturesHeader` on @p context to the encoding of @p mask. + * + * Does nothing when @p mask is zero: the header is omitted, not sent empty. + * The gRPC equivalent lives in `internal/grpc/configure_client_context.h`, + * as this library does not depend on gRPC. + */ +void AddFeaturesHeader(rest_internal::RestContext& context, std::uint32_t mask); + GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud diff --git a/google/cloud/storage/internal/tracked_features_test.cc b/google/cloud/storage/internal/tracked_features_test.cc index 852590e446fdd..d0cb10a21b89f 100644 --- a/google/cloud/storage/internal/tracked_features_test.cc +++ b/google/cloud/storage/internal/tracked_features_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/tracked_features.h" +#include "google/cloud/internal/rest_context.h" #include #include #include @@ -24,39 +25,23 @@ namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { +using ::testing::ElementsAre; using ::testing::IsEmpty; using ::testing::Not; // Every tracked feature. Add new enumerators here so the tests below cover -// them. +// them. The header `static_assert`s that the positions are distinct and below +// 32; this list drives the runtime checks on the derived bit values. constexpr std::array kAllFeatures = { TrackedFeature::kMultistreamInMrd, TrackedFeature::kReadHedging, }; -// The enumerators are bit positions in a 32-bit mask, so each must fit and no -// two may collide. Both are checked at compile time; a bad enumerator fails -// the build rather than the test. -constexpr bool AllBitPositionsFit() { - for (auto feature : kAllFeatures) { - if (static_cast(feature) >= 32) return false; - } - return true; -} - -constexpr bool AllBitPositionsDistinct() { - for (std::size_t i = 0; i != kAllFeatures.size(); ++i) { - for (std::size_t j = i + 1; j != kAllFeatures.size(); ++j) { - if (kAllFeatures[i] == kAllFeatures[j]) return false; - } - } - return true; -} - -static_assert(AllBitPositionsFit(), - "TrackedFeature values must be bit positions below 32"); -static_assert(AllBitPositionsDistinct(), - "TrackedFeature values must not be reused"); +// `FeatureBit()` is `constexpr`, so the bit values are usable at compile time. +static_assert(FeatureBit(TrackedFeature::kMultistreamInMrd) == 1, + "kMultistreamInMrd is bit 0"); +static_assert(FeatureBit(TrackedFeature::kReadHedging) == 2, + "kReadHedging is bit 1"); TEST(TrackedFeatures, HeaderName) { EXPECT_EQ(std::string(kFeaturesHeader), "x-goog-storage-cpp-features"); @@ -140,6 +125,20 @@ TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { } } +TEST(TrackedFeatures, AddFeaturesHeaderRestContext) { + rest_internal::RestContext context; + AddFeaturesHeader(context, FeatureBit(TrackedFeature::kReadHedging)); + EXPECT_THAT(context.GetHeader(kFeaturesHeader).values(), + ElementsAre("AAAAAg==")); +} + +// A zero mask means the header is omitted, not sent empty. +TEST(TrackedFeatures, AddFeaturesHeaderRestContextZeroMask) { + rest_internal::RestContext context; + AddFeaturesHeader(context, 0); + EXPECT_THAT(context.GetHeader(kFeaturesHeader), IsEmpty()); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal From b14c257350dab04ae5e17bb5a576636a2e4be83c Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 26 Oct 2026 08:02:42 +0000 Subject: [PATCH 6/7] style(storage): use explicit types per style guide Spell out primitive and function-return types instead of `auto` in the lines this change added, per .gemini/styleguide.md. --- .../storage/internal/async/connection_impl_open_test.cc | 4 ++-- google/cloud/storage/internal/connection_impl.cc | 7 ++++--- .../storage/internal/grpc/configure_client_context.cc | 4 ++-- google/cloud/storage/internal/tracked_features_test.cc | 9 +++++---- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl_open_test.cc b/google/cloud/storage/internal/async/connection_impl_open_test.cc index 40f1563f4e6ee..f25746d763cee 100644 --- a/google/cloud/storage/internal/async/connection_impl_open_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_open_test.cc @@ -451,7 +451,7 @@ std::multimap OpenAndCaptureMetadata( } TEST(AsyncConnectionImplTest, OpenFeatureHeaderWithMultiStream) { - auto const expected = + std::string const expected = EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)); EXPECT_THAT( OpenAndCaptureMetadata( @@ -476,7 +476,7 @@ TEST(AsyncConnectionImplTest, OpenFeatureHeaderOnEveryDescriptorStream) { bucket: "test-only-invalid" object: "test-object" )pb"; - auto const expected = + std::string const expected = EncodeFeatureMask(FeatureBit(TrackedFeature::kMultistreamInMrd)); ValidateMetadataFixture fixture; diff --git a/google/cloud/storage/internal/connection_impl.cc b/google/cloud/storage/internal/connection_impl.cc index 91fc48e2b434f..0f5331211b41d 100644 --- a/google/cloud/storage/internal/connection_impl.cc +++ b/google/cloud/storage/internal/connection_impl.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -412,12 +413,12 @@ StatusOr> StorageConnectionImpl::ReadObject( ReadObjectRangeRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); - auto const enable_hedging = + bool const enable_hedging = current->get(); auto const delay = current->get(); - auto const max_hedges = + int const max_hedges = current->get(); - auto const max_buffer = + std::size_t const max_buffer = current->get(); bool const hedging_active = enable_hedging && max_hedges > 0 && hedge_pool_ && read_pool_; diff --git a/google/cloud/storage/internal/grpc/configure_client_context.cc b/google/cloud/storage/internal/grpc/configure_client_context.cc index 60f8919dd1b35..6f90110a2df97 100644 --- a/google/cloud/storage/internal/grpc/configure_client_context.cc +++ b/google/cloud/storage/internal/grpc/configure_client_context.cc @@ -53,14 +53,14 @@ void CopyHeader(grpc::ClientContext& ctx, auto const& headers = context.headers(); auto const l = headers.find(name); if (l == headers.end()) return; - for (auto const& v : l->second.values()) ctx.AddMetadata(name, v); + for (std::string const& v : l->second.values()) ctx.AddMetadata(name, v); } } // namespace void ApplyRestContextHeaders(grpc::ClientContext& ctx, rest_internal::RestContext const& context) { - for (auto const* name : kForwardedHeaders) CopyHeader(ctx, context, name); + for (char const* name : kForwardedHeaders) CopyHeader(ctx, context, name); } void AddIdempotencyToken(grpc::ClientContext& ctx, std::string const& token) { diff --git a/google/cloud/storage/internal/tracked_features_test.cc b/google/cloud/storage/internal/tracked_features_test.cc index d0cb10a21b89f..af5897ba85331 100644 --- a/google/cloud/storage/internal/tracked_features_test.cc +++ b/google/cloud/storage/internal/tracked_features_test.cc @@ -56,8 +56,8 @@ TEST(TrackedFeatures, FeatureBit) { TEST(TrackedFeatures, FeatureBitsAreSingleDistinctBits) { std::uint32_t seen = 0; - for (auto feature : kAllFeatures) { - auto const bit = FeatureBit(feature); + for (TrackedFeature feature : kAllFeatures) { + std::uint32_t const bit = FeatureBit(feature); // Exactly one bit set. EXPECT_NE(bit, 0U); EXPECT_EQ(bit & (bit - 1), 0U) << "feature bit " << bit; @@ -109,7 +109,8 @@ TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { // Exercise every 6-bit group value; none of the outputs may contain the // standard alphabet's `+` or `/`. for (std::uint32_t mask = 1; mask != 0; mask <<= 1) { - auto const encoded = EncodeFeatureMask(mask | (mask >> 1) | (mask >> 2)); + std::string const encoded = + EncodeFeatureMask(mask | (mask >> 1) | (mask >> 2)); EXPECT_EQ(encoded.find_first_of("+/"), std::string::npos) << encoded; EXPECT_EQ(encoded.size(), 8U) << encoded; } @@ -118,7 +119,7 @@ TEST(TrackedFeatures, EncodeUsesUrlSafeAlphabet) { // in each of them. for (int shift : {26, 20, 14, 8, 2}) { for (std::uint32_t sextet : {0x3EU, 0x3FU}) { - auto const encoded = EncodeFeatureMask(sextet << shift); + std::string const encoded = EncodeFeatureMask(sextet << shift); EXPECT_THAT(encoded, Not(IsEmpty())); EXPECT_EQ(encoded.find_first_of("+/"), std::string::npos) << encoded; } From c2629b15cd8129df8131de994f9cd9c65ac2635e Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Tue, 27 Oct 2026 07:03:10 +0000 Subject: [PATCH 7/7] refactor(storage): single constexpr feature list and clearer docs * `kAllTrackedFeatures` lists every feature in the header, and one `constexpr` validator (`TrackedFeaturesAreValid()`) behind a single `static_assert` replaces the pairwise asserts: every position is below 32 and the OR of all bits has one bit per feature. The test iterates the same list instead of its own copy. * Document that bit positions are per library (the service keeps a separate bit-to-feature table per client header, populated per release by a tracker that parses this file), and that the service decodes only the URL-safe base64 alphabet. --- .../cloud/storage/internal/tracked_features.h | 49 ++++++++++++++----- .../storage/internal/tracked_features_test.cc | 11 +---- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/google/cloud/storage/internal/tracked_features.h b/google/cloud/storage/internal/tracked_features.h index 8d96c6b25752c..ab31772fc3213 100644 --- a/google/cloud/storage/internal/tracked_features.h +++ b/google/cloud/storage/internal/tracked_features.h @@ -17,6 +17,8 @@ #include "google/cloud/storage/version.h" #include "google/cloud/internal/rest_context.h" +#include +#include #include #include @@ -30,7 +32,9 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN * * The value is a 4-byte big-endian `std::uint32_t` bitmask, encoded as * URL-safe base64 (RFC 4648 section 5, `-` and `_` alphabet) with `=` padding. - * The header is omitted when no tracked feature is in use. + * The service decodes only the URL-safe alphabet; a value in the standard + * alphabet (`+` and `/`) is rejected. The header is omitted when no tracked + * feature is in use. */ inline constexpr char kFeaturesHeader[] = "x-goog-storage-cpp-features"; @@ -38,24 +42,43 @@ inline constexpr char kFeaturesHeader[] = "x-goog-storage-cpp-features"; * Client-side features reported to the service. * * Each enumerator is the bit position (not the bit value) of the feature in - * the mask. This file is the single source of truth for the assignment, and - * is parsed by tooling outside this repository: keep each enumerator on one - * line, in the form `kName = N,`, and never reuse or renumber a bit. + * the mask. Bit positions are specific to this library: the service keeps a + * separate bit-to-feature table per client header (that is, per library), + * populated per release by an internal tracker that parses this file. + * Positions therefore need not match other libraries (e.g. the Go SDK uses + * bit 1 for a different feature). Never renumber or reuse a bit once released, + * and keep each enumerator on one line, in the form `kName = N,`. */ enum class TrackedFeature : std::uint32_t { kMultistreamInMrd = 0, kReadHedging = 1, }; -// The mask is 32 bits wide, so every enumerator must be a position below 32, -// and no two features may share a position. -static_assert(static_cast(TrackedFeature::kMultistreamInMrd) < - 32, - "TrackedFeature values must be bit positions below 32"); -static_assert(static_cast(TrackedFeature::kReadHedging) < 32, - "TrackedFeature values must be bit positions below 32"); -static_assert(TrackedFeature::kMultistreamInMrd != TrackedFeature::kReadHedging, - "TrackedFeature values must not be reused"); +/// Every tracked feature. Add new enumerators here too. +inline constexpr std::array kAllTrackedFeatures{ + TrackedFeature::kMultistreamInMrd, TrackedFeature::kReadHedging}; + +/** + * Returns true if every feature is a bit position below 32 and no two + * features share a position. + * + * The mask is 32 bits wide, so a position of 32 or more would be silently + * dropped, and two features on the same bit would be indistinguishable. + */ +constexpr bool TrackedFeaturesAreValid() { + std::uint32_t all_bits = 0; + for (TrackedFeature feature : kAllTrackedFeatures) { + if (static_cast(feature) >= 32) return false; + all_bits |= std::uint32_t{1} << static_cast(feature); + } + // With distinct positions the OR has exactly one bit per feature. + std::size_t set_bits = 0; + for (; all_bits != 0; all_bits &= all_bits - 1) ++set_bits; + return set_bits == kAllTrackedFeatures.size(); +} + +static_assert(TrackedFeaturesAreValid(), + "TrackedFeature values must be distinct bit positions below 32"); /// Returns the bit value (`1 << position`) for @p feature. constexpr std::uint32_t FeatureBit(TrackedFeature feature) { diff --git a/google/cloud/storage/internal/tracked_features_test.cc b/google/cloud/storage/internal/tracked_features_test.cc index af5897ba85331..eccc4df1e6f4b 100644 --- a/google/cloud/storage/internal/tracked_features_test.cc +++ b/google/cloud/storage/internal/tracked_features_test.cc @@ -15,7 +15,6 @@ #include "google/cloud/storage/internal/tracked_features.h" #include "google/cloud/internal/rest_context.h" #include -#include #include #include @@ -29,14 +28,6 @@ using ::testing::ElementsAre; using ::testing::IsEmpty; using ::testing::Not; -// Every tracked feature. Add new enumerators here so the tests below cover -// them. The header `static_assert`s that the positions are distinct and below -// 32; this list drives the runtime checks on the derived bit values. -constexpr std::array kAllFeatures = { - TrackedFeature::kMultistreamInMrd, - TrackedFeature::kReadHedging, -}; - // `FeatureBit()` is `constexpr`, so the bit values are usable at compile time. static_assert(FeatureBit(TrackedFeature::kMultistreamInMrd) == 1, "kMultistreamInMrd is bit 0"); @@ -56,7 +47,7 @@ TEST(TrackedFeatures, FeatureBit) { TEST(TrackedFeatures, FeatureBitsAreSingleDistinctBits) { std::uint32_t seen = 0; - for (TrackedFeature feature : kAllFeatures) { + for (TrackedFeature feature : kAllTrackedFeatures) { std::uint32_t const bit = FeatureBit(feature); // Exactly one bit set. EXPECT_NE(bit, 0U);