[andrewro-google]

In PartialResultSetResume::Read (spanner and bigtable), when a retryable streaming error occurred and the retry policy's deadline expired during backoff (sleep_for), the loop reset last_status_ and created a new unread PartialResultSetReader before checking IsExhausted(). Subsequent calls to Finish() returned OK from the unread reader, causing PartialResultSetSource::Create to return a misleading INTERNAL: PartialResultSetSource response contained no metadata error before metadata arrived, or silently truncating results mid-stream. Check IsExhausted() immediately after backoff before resetting last_status_ or creating a new reader.

In PartialResultSetSource::ReadFromStream (spanner and bigtable), when a stream ended with buffered incomplete row or uncommitted row data, check reader_->Finish() first and surface any non-OK RPC Status rather than masking it with INTERNAL ("PartialResultSetSource stream ended at a point that is not on a row boundary" / "Stream ended with uncommitted rows.").

Includes deterministic zero-sleep unit tests for both libraries covering retry deadline expiration during backoff and streaming RPC errors on incomplete rows.

[gemini-code-assist]

Code Review

This pull request refactors the retry loops in both Bigtable and Spanner's PartialResultSetResume to check if the retry policy is exhausted before recreating the reader, addressing issues where the retry deadline expires during backoff. It also updates PartialResultSetSource to correctly surface RPC errors when a stream ends with uncommitted or incomplete rows, and adds comprehensive unit tests. The reviewer suggested using break instead of returning false directly from the loop in Spanner's PartialResultSetResume to align with the Bigtable implementation and avoid potential compiler warnings.

[gemini-code-assist]

To improve consistency with the Bigtable implementation and prevent potential compiler warnings (such as 'control reaches end of non-void function') or future maintenance issues if a break is ever introduced inside the loop, consider using break; instead of returning false directly from inside the loop, and keep the return false; statement at the end of the function.

    if (retry_policy_prototype_->IsExhausted()) break;
    resumption = true;
    last_status_.reset();
    child_ = factory_(*resume_token);
  }
  return false;
}

[andrewro-google]

I disagree. The consistency between bigtable and spanner isn't important. I fixed both because the bug is in both. If a break is ever introduced the code won't compile since there's no return statement after the loop.

[andrewro-google]

@scotthart PTAL when you have a chance — thanks!