Skip to content

fix(storage): track persisted byte offset for async writer flush promises - #16417

Merged
kalragauri merged 3 commits into
googleapis:mainfrom
kalragauri:feat/flush-race-fix
Sep 10, 2026
Merged

kalragauri merged 3 commits into
googleapis:mainfrom
kalragauri:feat/flush-race-fix

Conversation

@kalragauri

Copy link
Copy Markdown
Contributor

In AsyncWriterConnectionBufferedState and AsyncWriterConnectionResumedState, SetFlushed() completed pending Flush() promises using a simple FIFO pop_front(). Because server-side persisted_size confirmations are cumulative byte offsets rather than 1:1 request/response tokens, a subsequent Flush() could be prematurely satisfied by an earlier acknowledgment while its buffered writes were still in-flight.

This PR does the following:

  • Introduces a PendingFlush struct to pair each Flush() promise with its expected target_offset (buffer_offset_ + resend_buffer_.size()).
  • Updates SetFlushed() to dequeue and satisfy only promises whose target_offset <= persisted_size.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Sep 7, 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 updates both AsyncWriterConnectionBufferedState and AsyncWriterConnectionResumedState to track outstanding Flush() promises alongside their stream target offsets using a new PendingFlush struct. This ensures that multiple queued flush operations are only completed when the server's persisted size reaches or exceeds their respective target offsets. Unit and integration tests have been added to verify this behavior. Feedback is provided regarding a compilation error in writer_connection_resumed.cc where persisted_size (a StatusOr<std::int64_t>) is passed directly to SetFlushed which expects std::int64_t; it is recommended to explicitly check persisted_size.ok() before extracting the value.

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

codecov Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.60140% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.28%. Comparing base (4d385e8) to head (8c2fe77).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...torage/internal/async/writer_connection_resumed.cc 92.59% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16417      +/-   ##
==========================================
+ Coverage   92.26%   92.28%   +0.02%     
==========================================
  Files        2246     2246              
  Lines      212734   212774      +40     
==========================================
+ Hits       196273   196366      +93     
+ Misses      16461    16408      -53     

☔ 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 marked this pull request as ready for review September 7, 2026 12:11
@kalragauri
kalragauri requested review from a team as code owners September 7, 2026 12:11
@kalragauri
kalragauri enabled auto-merge (squash) September 10, 2026 09:44
@kalragauri
kalragauri merged commit fc892cf into googleapis:main Sep 10, 2026
64 of 66 checks passed
@kalragauri
kalragauri deleted the feat/flush-race-fix branch September 11, 2026 06:47

This branch was previously deployed

1 inactive deployment
false — 8c2fe778 Deployed Sep 10, 2026 by kalragauri via Save PR ref #11621
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.

2 participants