Skip to content

fix(storage): do not retry permanent errors in async writer resume - #16340

Open
v-pratap wants to merge 1 commit into
googleapis:mainfrom
v-pratap:fix-async-writer-permanent-error-retry
Open

fix(storage): do not retry permanent errors in async writer resume#16340
v-pratap wants to merge 1 commit into
googleapis:mainfrom
v-pratap:fix-async-writer-permanent-error-retry

Conversation

@v-pratap

Copy link
Copy Markdown
Contributor

Summary

This PR fixes a bug in Storage async writer connections (AsyncWriterConnectionResumed and AsyncWriterConnectionBuffered) where permanent errors (such as FAILED_PRECONDITION) were being retried during streaming write operations.

Cause

Previously, when a stream error occurred during Write, Flush, Close, Finalize, or Query, Resume(Status const& s) was called unconditionally without checking if s was a permanent failure. For appendable uploads (BidiWriteObject), issuing a new BidiWriteObject RPC on Resume() requested writer exclusivity from the server, causing concurrent writers to continuously steal exclusivity back and forth from each other in an infinite loop upon receiving FAILED_PRECONDITION.

Fix

  • Updated Resume(Status const& s) in writer_connection_resumed.cc and writer_connection_buffered.cc to check if s is a permanent failure (via AsyncRetryPolicy / AsyncStatusTraits) before attempting Resume().
  • If s is a permanent failure (such as FAILED_PRECONDITION), the connection immediately terminates with status s via SetError(...) without opening a new stream.
  • Added unit tests WriteConnectionResumed.PermanentErrorNoResume and WriteConnectionBuffered.PermanentErrorNoResume to verify that permanent errors on active streams do not trigger resume attempts.

@v-pratap
v-pratap requested review from a team as code owners August 11, 2026 06:59
@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Aug 11, 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 OpenTelemetry tracing and metrics support to the asynchronous OpenObject class, adding a new OpenObjectTelemetry helper class and corresponding unit tests. It also updates the buffered and resumed writer connections to prevent resume attempts on permanent failures. The review feedback correctly identifies a build issue where GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY is unconditionally defined as PUBLIC in CMake, which would break builds when OpenTelemetry is disabled. Additionally, the reviewer suggests correcting the header inclusion in open_object.cc to include the trace provider instead of the metrics provider, as the file only retrieves the current span and does not record metrics directly.

Comment thread google/cloud/storage/google_cloud_cpp_storage_grpc.cmake Outdated
Comment thread google/cloud/storage/internal/async/open_object.cc Outdated
@v-pratap
v-pratap force-pushed the fix-async-writer-permanent-error-retry branch 2 times, most recently from ecf19dc to 34f75fb Compare August 11, 2026 07:13
AsyncWriterConnectionResumed and AsyncWriterConnectionBuffered previously
unconditionally invoked Resume() when stream operations returned an error,
even if the error was a permanent failure such as FAILED_PRECONDITION.

For appendable uploads (BidiWriteObject), issuing a new BidiWriteObject
RPC on Resume() caused the client to request writer exclusivity again,
resulting in concurrent writers endlessly stealing exclusivity back and
forth from each other on FAILED_PRECONDITION errors.

This change checks if the error status is a permanent failure (via
AsyncRetryPolicy / AsyncStatusTraits) before attempting Resume(). If
permanent, the upload immediately terminates with the error status.
@v-pratap
v-pratap force-pushed the fix-async-writer-permanent-error-retry branch from 34f75fb to 4af6792 Compare August 11, 2026 07:16
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.26168% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.23%. Comparing base (154f9ef) to head (4af6792).

Files with missing lines Patch % Lines
...e/internal/async/writer_connection_resumed_test.cc 92.85% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16340   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files        2227     2227           
  Lines      209283   209378   +95     
=======================================
+ Hits       193026   193125   +99     
+ Misses      16257    16253    -4     

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

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