[ajayky-os]

Summary

Acquires mu_ in ObjectDescriptorImpl::Read() before reading metadata_ and read_object_spec_ (and before checking prewarmed_ranges_), enforces the lock precondition on CreateHashValidator() via std::unique_lock<std::mutex> const&, and locks mu_ in ObjectDescriptorImpl::StreamSize().

Motivation

1. Data race and SIGSEGV in ObjectDescriptorImpl::Read() / CreateHashValidator():
   • ObjectDescriptorImpl::OnRead() and ObjectDescriptorImpl::Resume() mutate metadata_ (metadata_ = std::move(*response->mutable_metadata())) and read_object_spec_ (mutable_read_handle() / ApplyRedirectErrors()) on background gRPC completion threads while holding mu_.
   • Previously, ObjectDescriptorImpl::Read() inspected metadata_ (has_value(), has_finalize_time(), size()), invoked CreateHashValidator() (reading metadata_->checksums().crc32c() and metadata_->checksums().md5_hash()), and read read_object_spec_.bucket() / read_object_spec_.object() before acquiring mu_.
   • When a concurrent stream establishment (MakeSubsequentStream() / OnResume()) or BidiReadObjectResponse move-assigns metadata_ while another thread calls Read(), heap-allocated protobuf strings such as the 16-byte raw md5_hash (which exceeds the 15-byte SSO buffer on libstdc++) can be swapped/freed mid-read inside CreateHashValidator() / MD5FromProto() (Base64Encode()), causing a SIGSEGV.
   • Checking prewarmed_ranges_ immediately after acquiring mu_ also avoids allocating and discarding HashFunction, HashValidator, and ReadRange objects on pre-warmed cache hits.
2. Unlocked MultiStreamManager access in ObjectDescriptorImpl::StreamSize():
   • MultiStreamManager is not internally synchronized and requires callers to hold mu_, so StreamSize() now acquires std::scoped_lock<std::mutex> lk(mu_).

Changes

• google/cloud/storage/internal/async/object_descriptor_impl.{h,cc}:
  • Acquire std::unique_lock<std::mutex> lk(mu_) in ObjectDescriptorImpl::Read() before accessing prewarmed_ranges_, metadata_, and read_object_spec_.
  • Pass std::unique_lock<std::mutex> const& to ObjectDescriptorImpl::CreateHashValidator() (matching AssurePendingStreamQueued()) to enforce at compile time that mu_ is held, and guard metadata_.has_value() defensively.
  • Lock mu_ in ObjectDescriptorImpl::StreamSize().
• google/cloud/storage/internal/async/object_descriptor_impl_test.cc:
  • Add ConcurrentReadAndMetadataUpdate and ConcurrentMultiStreamReadAndStreamSize multi-threaded unit tests exercising concurrent Read(), MakeSubsequentStream(), metadata(), and StreamSize() calls against background OnRead() metadata and read_handle updates.

Verification

• Verified ObjectDescriptorImpl.ConcurrentReadAndMetadataUpdate reproduces the ThreadSanitizer data race prior to this change (Object::InternalSwap in ObjectDescriptorImpl::OnRead at object_descriptor_impl.cc:516 vs unlocked read in ObjectDescriptorImpl::Read at object_descriptor_impl.cc:261).
• Verified all object_descriptor and connection_impl_open async unit test suites pass cleanly under --config=tsan (--runs_per_test=5).

[gemini-code-assist]

Code Review

This pull request improves thread safety in ObjectDescriptorImpl by ensuring that hash validator creation and stream size queries are properly synchronized under the mu_ mutex. It also adds robust concurrent tests to verify these changes. The review feedback highlights several violations of the repository's style guide regarding the use of auto where explicit types should be used instead, specifically for obscured return types (hash_function, hash_validator, and reader) and primitive types (idx).

[codecov]

Codecov Report

❌ Patch coverage is 99.39759% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.45%. Comparing base (b1c1e25) to head (f99a74a).

Files with missing lines	Patch %	Lines
...rage/internal/async/object_descriptor_impl_test.cc	99.35%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16625      +/-   ##
==========================================
- Coverage   92.45%   92.45%   -0.01%     
==========================================
  Files        2269     2269              
  Lines      222034   222192     +158     
==========================================
+ Hits       205292   205426     +134     
- Misses      16742    16766      +24     

☔ 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.