Skip to content

fix(storage): enforce mutual exclusion between Close() and Finalize() in async writers#16211

Open
kalragauri wants to merge 2 commits into
googleapis:mainfrom
kalragauri:feature/append-object
Open

fix(storage): enforce mutual exclusion between Close() and Finalize() in async writers#16211
kalragauri wants to merge 2 commits into
googleapis:mainfrom
kalragauri:feature/append-object

Conversation

@kalragauri

Copy link
Copy Markdown
Contributor

This PR addresses feedback regarding the interaction between Close() and Finalize() in AsyncWriterConnection implementations (#16163 (comment)).

Previously, if Finalize() was called followed by Close(), Close() would still append its payload, but the WriteLoop would prioritize FinalizeStep over CloseStep. The Close() future would hang indefinitely because the closed_ promise was never satisfied. This change enforces mutual exclusion between these two terminal operations, ensuring they fail fast.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Jun 29, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces validation checks in Finalize() and Close() for both buffered and resumed async writer connections to prevent invalid state transitions (such as calling Close() after Finalize(), or calling them multiple times), along with corresponding unit tests. The review feedback highlights critical race conditions in these checks because the lock is released before the futures are moved out, allowing concurrent calls to bypass the .valid() checks. It is recommended to use the existing finalize_ and close_ boolean flags instead, which are safely updated under the lock.

Comment thread google/cloud/storage/internal/async/writer_connection_buffered.cc Outdated
Comment thread google/cloud/storage/internal/async/writer_connection_buffered.cc Outdated
Comment thread google/cloud/storage/internal/async/writer_connection_resumed.cc Outdated
Comment thread google/cloud/storage/internal/async/writer_connection_resumed.cc Outdated
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.24%. Comparing base (f1a8e57) to head (5a42300).

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #16211    +/-   ##
========================================
  Coverage   92.23%   92.24%            
========================================
  Files        2265     2265            
  Lines      210205   210330   +125     
========================================
+ Hits       193888   194021   +133     
+ Misses      16317    16309     -8     

☔ 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 kalragauri force-pushed the feature/append-object branch from 0ff069d to 264f190 Compare June 30, 2026 05:38
@kalragauri kalragauri marked this pull request as ready for review June 30, 2026 05:49
@kalragauri kalragauri requested review from a team as code owners June 30, 2026 05:49
@kalragauri kalragauri requested a review from v-pratap June 30, 2026 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant