[ajayky-os]

Summary

Fixes a bug where GrpcStub unary RPCs (GetBucketMetadata, GetObjectMetadata, ListObjects, PatchObject, DeleteObject, ComposeObject, RewriteObject, CreateResumableUpload, QueryResumableUpload, DeleteResumableUpload, SignBlob, etc.) and AsyncConnectionImpl / RewriterConnectionImpl unary RPCs left grpc::ClientContext::deadline() at gpr_inf_future (std::chrono::system_clock::time_point::max()) when TransferStallTimeoutOption was configured. Streaming RPCs (ReadObject, WriteObject as used by InsertObjectMedia, BidiWriteObject) are unchanged and continue to use their per-chunk stall timers.

When an HTTP/2 connection stalls after TCP/TLS/HTTP2 SETTINGS handshake completes (or when an intermediate network element blackholes HTTP/2 response frames while TCP ACKs the request frames), unary gRPC calls without a ClientContext deadline hang indefinitely inside RetryClient::GetObjectMetadata / RestRetryLoop without ever firing DeadlineExceeded or triggering a retry.

Behavior Note

• gRPC unary RPCs (both synchronous storage::Client and storage::AsyncClient, including RewriteObject, ComposeObject, and IAM SignBlob) now honor TransferStallTimeoutOption as a per-attempt deadline on grpc::ClientContext. Because the storage default options (DefaultOptionsGrpc / DefaultOptionsAsync) set a default TransferStallTimeoutOption (120s), unary gRPC calls now have a finite per-attempt deadline by default.
• To disable the unary deadline, configure TransferStallTimeoutOption(std::chrono::seconds(0)) (or any negative value). To override the deadline for specific calls, set a deadline on grpc::ClientContext via GrpcSetupOption (or GrpcSetupPollOption for poll calls).
• Updated TransferStallTimeoutOption documentation in google/cloud/storage/options.h to note this per-attempt deadline behavior for gRPC unary RPCs.

Root Causes Fixed

1. Missing unary RPC deadline in ApplyQueryParameters (configure_client_context.h):
   • Added ApplyUnaryRpcDeadline(grpc::ClientContext& ctx, Options const& options) which applies options.get<TransferStallTimeoutOption>() as a relative deadline on ctx when timeout > std::chrono::seconds::zero() and no explicit deadline has already been set (ctx.deadline() == time_point::max()).
   • Added IsStreamingRequest<Request> trait (ReadObjectRangeRequest, InsertObjectMediaRequest, UploadChunkRequest) so ApplyQueryParameters only sets a ClientContext deadline on unary RPCs (streaming RPCs continue to use their per-chunk stall timers).
   • Invoked ApplyUnaryRpcDeadline before internal::ConfigureContext(ctx, options) so that any user-provided GrpcSetupOption callback can override the deadline.
2. Missing ApplyQueryParameters in 4 GrpcStub unary RPC methods (stub.cc):
   • GrpcStub::CreateResumableUpload, QueryResumableUpload, DeleteResumableUpload, and SignBlob previously constructed a raw grpc::ClientContext without calling ApplyQueryParameters, bypassing both TransferStallTimeoutOption and GrpcSetupOption.
3. Async unary RPCs (async/connection_impl.cc, async/rewriter_connection_impl.cc):
   • Applied ApplyUnaryRpcDeadline(*context, *options) to AsyncConnectionImpl::GetBucket, ComposeObject, DeleteObject, StartResumableWrite, QueryWriteStatus, and RewriterConnectionImpl::Iterate.

(Note: A separate issue where GroupOptions(Options&, Tail&&...) moved from non-const lvalue Options objects has been split into #16601.)

Testing

• Unit tests:
  • configure_client_context_test: Tests unary deadline application, zero and negative (seconds(-1)) timeouts leaving deadline at time_point::max(), pre-existing deadline preservation, GrpcSetupOption override precedence, and streaming request exclusion (ReadObjectRangeRequest, InsertObjectMediaRequest, UploadChunkRequest).
  • stub_test: Tests TransferStallTimeoutOption and GrpcSetupOption across GrpcStub::GetBucketMetadata, CreateResumableUpload, QueryResumableUpload, DeleteResumableUpload, and SignBlob.
  • async/connection_impl_test, async/connection_impl_upload_test, & async/rewriter_connection_impl_test: Tests TransferStallTimeoutOption deadline application on async unary RPCs (GetBucket, ComposeObject, DeleteObject, StartResumableWrite with 0s and 1s, QueryWriteStatus, and RewriterConnectionImpl::Iterate).
• Live End-to-End & Tarpit Verification (us-central1 GCE VM + GCS bucket):
  • Verified live unary and streaming gRPC calls (GetBucketMetadata, ListObjects, InsertObject, GetObjectMetadata, PatchObject, ComposeObject, RewriteObjectBlocking, CreateResumableUpload, QueryResumableUpload, DeleteResumableUpload, DeleteObject, and streaming ReadObject, plus AsyncClient equivalents) from a us-central1 GCE VM against a live GCS bucket, inspecting per-call grpc::ClientContext::deadline().
  • Verified against a simulated HTTP/2 tarpit server (completing TCP + HTTP/2 SETTINGS preface and draining request frames without replying) that synchronous and all 6 asynchronous unary RPCs time out per attempt and return StatusCode::kDeadlineExceeded after exhausting the retry policy instead of hanging indefinitely.

[gemini-code-assist]

Code Review

This pull request refactors the application of unary RPC deadlines across the storage client, centralizing the logic via ApplyUnaryRpcDeadline using TransferStallTimeoutOption. It also fixes an issue in GroupOptions where non-const lvalue Options were being moved-from. The feedback highlights a critical issue in AsyncStartResumableWrite where replacing ConfigureContext with ApplyUnaryRpcDeadline inadvertently drops other essential context configurations like credentials. Additionally, it is recommended to use a more robust and idiomatic comparison for the timeout duration to safely handle negative values and avoid direct .count() calls.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.38%. Comparing base (608aeef) to head (87d664e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16600      +/-   ##
==========================================
- Coverage   92.40%   92.38%   -0.03%     
==========================================
  Files        2266     2266              
  Lines      220810   220948     +138     
==========================================
+ Hits       204036   204115      +79     
- Misses      16774    16833      +59     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kalragauri]

nit: a few places in the PR description no longer match main or the current diff.

Also, TransferStallTimeoutOption (not in the diff) is documented as a no-progress "stall" timeout (

google-cloud-cpp/google/cloud/storage/options.h

Lines 453 to 472 in 608aeef

	/**
	* Sets the transfer stall timeout.
	*
	* If a transfer (upload, download, or request) *stalls*, i.e., no bytes are
	* sent or received for a significant period, it may be better to restart the
	* transfer as this may indicate a network glitch. For downloads the
	* google::cloud::storage::DownloadStallTimeoutOption takes precedence.
	*
	* For large requests (e.g. downloads in the GiB to TiB range) this is a better
	* configuration parameter than a simple timeout, as the transfers will take
	* minutes or hours to complete. Relying on a timeout value for them would not
	* work, as the timeout would be too large to be useful. For small requests,
	* this is as effective as a timeout parameter, but maybe unfamiliar and thus
	* harder to reason about.
	*
	* @ingroup storage-options
	*/
	struct TransferStallTimeoutOption {
	using Type = std::chrono::seconds;
	};

), but for gRPC unary RPCs it is now an absolute per-attempt deadline that is on by default (120s).

Should we add a sentence there that gRPC unary RPCs apply it as a per-attempt deadline and that std::chrono::seconds(0) disables it?

[ajayky-os]

nit: a few places in the PR description no longer match main or the current diff.

Also, TransferStallTimeoutOption (not in the diff) is documented as a no-progress "stall" timeout (

google-cloud-cpp/google/cloud/storage/options.h

Lines 453 to 472 in 608aeef

	/**
	* Sets the transfer stall timeout.
	*
	* If a transfer (upload, download, or request) *stalls*, i.e., no bytes are
	* sent or received for a significant period, it may be better to restart the
	* transfer as this may indicate a network glitch. For downloads the
	* google::cloud::storage::DownloadStallTimeoutOption takes precedence.
	*
	* For large requests (e.g. downloads in the GiB to TiB range) this is a better
	* configuration parameter than a simple timeout, as the transfers will take
	* minutes or hours to complete. Relying on a timeout value for them would not
	* work, as the timeout would be too large to be useful. For small requests,
	* this is as effective as a timeout parameter, but maybe unfamiliar and thus
	* harder to reason about.
	*
	* @ingroup storage-options
	*/
	struct TransferStallTimeoutOption {
	using Type = std::chrono::seconds;
	};

), but for gRPC unary RPCs it is now an absolute per-attempt deadline that is on by default (120s).
Should we add a sentence there that gRPC unary RPCs apply it as a per-attempt deadline and that std::chrono::seconds(0) disables it?

Thanks for the review! Updated the PR description (removed InsertObject from the unary list, changed storage_experimental::AsyncClient -> storage::AsyncClient, updated > seconds::zero(), and refreshed the Testing section) and added a note to TransferStallTimeoutOption in options.h documenting the per-attempt deadline behavior for gRPC unary RPCs and that std::chrono::seconds(0) disables it.