Skip to content

feat(storage): log additional bytes received from GCS in read path#13427

Merged
Dhriti07 merged 12 commits into
mainfrom
feature/storage-extra-bytes-logging
Jun 25, 2026
Merged

feat(storage): log additional bytes received from GCS in read path#13427
Dhriti07 merged 12 commits into
mainfrom
feature/storage-extra-bytes-logging

Conversation

@nidhiii-27

Copy link
Copy Markdown
Contributor

Fixes bug 475824752. Tracks the number of bytes received from the transport layer and emits a warning log if GCS sends more bytes than requested by the user during explicit range requests.

[Generated-by: AI]

@nidhiii-27 nidhiii-27 requested review from a team as code owners June 11, 2026 09:38

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

Copy link
Copy Markdown
Contributor

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 adds logging warnings to ApiaryUnbufferedReadableByteChannel and GapicUnbufferedReadableByteChannel when more bytes are received from GCS than requested. The review feedback points out that the logging logic in the close() methods of both classes is not exception-safe. If a runtime exception occurs during logging, it could prevent critical resource cleanup and state updates from executing, potentially causing resource leaks. The reviewer recommends wrapping the logging logic in a try-finally block to ensure cleanup is always executed safely.

@nidhiii-27

Copy link
Copy Markdown
Contributor Author

Addressed all PR review comments.

@nidhiii-27 nidhiii-27 marked this pull request as ready for review June 16, 2026 05:50
@Dhriti07

Copy link
Copy Markdown
Contributor

Can you add a tests for Apiary path as well ?

@nidhiii-27

Copy link
Copy Markdown
Contributor Author

Added unit tests verifying the extra bytes logging for the Apiary path in ApiaryUnbufferedReadableByteChannelTest.

Co-authored by AI Agent

Fixes bug 475824752. Tracks the number of bytes received from the transport layer and emits a warning log if GCS sends more bytes than requested by the user during explicit range requests.

[Generated-by: AI]
@nidhiii-27 nidhiii-27 force-pushed the feature/storage-extra-bytes-logging branch from 0f0989a to 2f4a5c8 Compare June 19, 2026 04:51
long requestedLength = apiaryReadRequest.getByteRangeSpec().length();
if (requestedLength >= 0
&& requestedLength < ByteRangeSpec.EFFECTIVE_INFINITY
&& totalBytesReadFromNetwork > requestedLength) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you double-check if we are actually measuring the total bytes sent by GCS over the network? I was wondering if it might just be measuring the bytes our client chooses to consume from the InputStream instead.

@nidhiii-27 nidhiii-27 Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right—in Apiary, totalBytesReadFromNetwork strictly measures the bytes our client actively consumes from the InputStream, not the raw bytes arriving over the network socket.

Because Apiary uses a pull-based HTTP stream (unlike Gapic's background gRPC thread which tracks fetchOffset directly off the wire), if GCS sends extra bytes but the user's application stops reading exactly at the requested limit and calls close(), we won't see those extra bytes and the warning won't trigger. We only catch the over-fetch if the application continues reading until it hits EOF.

Given the limitations of the HTTP client's pull model, this is the closest proxy we have for detecting over-fetching.

Co-authored by AI Agent

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that this is measuring the client consumed bytes, Does that mean that this check will never be triggered ? Can we check how other languages like go and handling these cases or if they have a different architecture that doesn't have this problem ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Triggering of the warning: Yes, this check will trigger if the client application reads in a loop until EOF (i.e. until read() returns -1), which is standard practice when consuming streams or when using copy utilities. If the caller manually stops reading at exactly the requested length and immediately closes the stream, the warning won't fire. But this is the closest proxy we have to track if GCS sent extra bytes.
  2. How Go handles this: Go's implementation is identical. The Go client tracks the read count at the user-facing Reader layer, decrementing a remain counter on each Read() call. On Close(), if remain is less than 0, it logs a warning: storage: received %d more bytes than requested from GCS... (see reader.go:398).

Co-authored by AI Agent

@Dhriti07 Dhriti07 merged commit 9492aa2 into main Jun 25, 2026
197 checks passed
@Dhriti07 Dhriti07 deleted the feature/storage-extra-bytes-logging branch June 25, 2026 16:41
lqiu96 pushed a commit that referenced this pull request Jun 30, 2026
…13427)

Fixes bug 475824752. Tracks the number of bytes received from the
transport layer and emits a warning log if GCS sends more bytes than
requested by the user during explicit range requests.

[Generated-by: AI]

---------

Co-authored-by: Dhriti07 <[email protected]>
lqiu96 pushed a commit that referenced this pull request Jul 7, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>1.88.0</summary>

##
[1.88.0](v1.87.1...v1.88.0)
(2026-07-06)


### Features

* **auth:** Add support for Regional Access Boundaries
([#13499](#13499))
([b721e43](b721e43))
* automate libraries-bom tag and release creation on release publish
([#13655](#13655))
([7a7e2ff](7a7e2ff))
* **bigquery-jdbc:** add `EnableProjectDiscovery` connection property
for metadata methods
([#13344](#13344))
([ab9669a](ab9669a))
* **bigquery-jdbc:** migrate statement execution thread tracking to
connection-scoped executor
([#13454](#13454))
([341e51a](341e51a))
* **bigquery-jdbc:** respect standard JVM trustStore properties by
default
([#13435](#13435))
([7af3224](7af3224))
* **bigquery-jdbc:** support connection-scoped executor isolation and
dynamic queue warnings
([#13413](#13413))
([79e26b8](79e26b8))
* **bigquery:** add internal listProjects API to core client
([#13429](#13429))
([3580407](3580407))
* **bigtable:** route point read rows to shim
([#13542](#13542))
([2c7e5f5](2c7e5f5))
* **deps:** add jspecify to shared dependencies
([#13412](#13412))
([582053d](582053d))
* enable self-signed JWTs by default in ServiceOptions
([#13338](#13338))
([110d2b7](110d2b7))
* **gapic-generator:** Add NullMarked annotation to generated classes
([#13517](#13517))
([825dadd](825dadd))
* **google/cloud/agentregistry/v1:** onboard a new library
([#13509](#13509))
([6728816](6728816))
* scale up connection worker pool based on latency
([#13384](#13384))
([eb379b3](eb379b3))
* **spanner:** add settings for gRCP keep-alive
([#13643](#13643))
([9d78307](9d78307))
* **spanner:** auth login support for Spanner Omni endpoints
([#13470](#13470))
([55930b4](55930b4))
* **storage:** add checksum validation in the json read channel
([#13270](#13270))
([872d7b7](872d7b7))
* **storage:** add checksum validation on json read paths
([#13269](#13269))
([396b042](396b042))
* **storage:** add full object checksum validation for bidi flow
([#13266](#13266))
([9113d80](9113d80))
* **storage:** add full object checksum validation for grpc flow
([#13265](#13265))
([a5ba606](a5ba606))
* **storage:** Adding CumulativeHasher wrapper class for Full object …
([#13239](#13239))
([bd40324](bd40324))
* **storage:** adding disabling option for bidi reads
([#13506](#13506))
([591cae0](591cae0))
* **storage:** log additional bytes received from GCS in read path
([#13427](#13427))
([9492aa2](9492aa2))
* **storage:** update compose sample to support deleteSourceObjects
option
([#13493](#13493))
([50a8658](50a8658))
* use self-signed JWTs in Spanner MutableCredentials
([#13381](#13381))
([29543a2](29543a2))


### Bug Fixes

* Add logging-logback to gapic-libraries-bom
([#13663](#13663))
([512e370](512e370))
* **auth:** Fix UserCredentials serialization clientSecret leak
([#13465](#13465))
([2d9d01c](2d9d01c))
* **auth:** handle missing APPDATA on Windows gracefully
([#13471](#13471))
([77e84a9](77e84a9)),
closes
[#12565](#12565)
* **bigquery-jdbc:** add proper version to BigQueryDriver
([#13294](#13294))
([9fd84fc](9fd84fc))
* **bigquery-jdbc:** avoid rollback on statement close in manual commit
mode
([#13503](#13503))
([79bbee1](79bbee1))
* **bigquery-jdbc:** correct `FilterTablesOnDefaultDataset` fallback
logic
([#13625](#13625))
([f7eb23d](f7eb23d))
* **bigquery-jdbc:** ensure largeResults are handled in
PreparedStatement
([#13496](#13496))
([d3e7a19](d3e7a19))
* **bigquery-jdbc:** ensure test uses unique dataset & cleans up
([#13587](#13587))
([a6bb362](a6bb362))
* **bigquery-jdbc:** explicitly set location when available for
temporary dataset
([#13508](#13508))
([40cd0a7](40cd0a7))
* **bigquery-jdbc:** propagate connection proxy settings to auth library
([#13539](#13539))
([87dda5d](87dda5d))
* **bigquery-jdbc:** shade org.slf4j
([#13547](#13547))
([7ef1312](7ef1312))
* **bigquery-jdbc:** update format validation tests to create tables for
the test
([#13588](#13588))
([4bb3a61](4bb3a61))
* **bigquery-jdbc:** update Maven Central artifact link in the README
([#13563](#13563))
([8707a65](8707a65))
* **bigquery:** retry cancel job and routine creation on transient HTTP
5xx errors
([#13416](#13416))
([35eb041](35eb041))
* **bigquery:** route JOB_CREATION_REQUIRED through fast query path
([#13437](#13437))
([64cde7f](64cde7f))
* **bigquery:** Update fast query path to allow jobTimeoutMs in the
request
([#13502](#13502))
([1a6f4d5](1a6f4d5))
* **bigtable:** honour session_load override when server-returned
session_load is 0
([#13629](#13629))
([b56f9b8](b56f9b8))
* **bqjdbc:** pass rowsInPage with TableResult
([#13238](#13238))
([203a91e](203a91e))
* **ci:** build dependencies before convergence check
([#13638](#13638))
([9de3209](9de3209))
* **ci:** ignore SNAPSHOT suffix in flatten-plugin check
([#13639](#13639))
([bb65627](bb65627))
* **ci:** run root install in dependency convergence check
([#13666](#13666))
([86d35e4](86d35e4))
* **ci:** skip existing-versions-check for gapic-generator-java-root
([#13590](#13590))
([a1f3b8a](a1f3b8a))
* **datastore:** Configure TraceExporter with Datastore credentials in
E2E tests
([#13627](#13627))
([d281a62](d281a62))
* **datastore:** disable built-in OpenTelemetry SDK when metrics export
is disabled
([#13543](#13543))
([53b7142](53b7142))
* **datastore:** reduce flakiness of E2E tracing tests
([#13645](#13645))
([8afc0ab](8afc0ab))
* **deps:** update dependency
com.google.apis:google-api-services-bigquery to v2-rev20260612-2.0.0
([#13570](#13570))
([ca5ff79](ca5ff79))
* **deps:** update dependency com.google.apis:google-api-services-dns to
v1-rev20260616-2.0.0
([#12796](#12796))
([0dd60ca](0dd60ca))
* **deps:** update dependency com.google.cloud:google-cloud-pubsub-bom
to v1.151.0
([#12097](#12097))
([d169006](d169006))
* **deps:** update dependency com.google.cloud:libraries-bom to v26.84.0
([#12663](#12663))
([95c7774](95c7774))
* **deps:** update first-party storage dependencies to
v1-rev20260524-2.0.0
([#13406](#13406))
([bdeedb8](bdeedb8))
* exclude java-vertexai from existing versions check
([#13632](#13632))
([edaa4e3](edaa4e3))
* fallback on VPC
([#13567](#13567))
([e8c428d](e8c428d))
* **java-cloud-bom:** execute pre-install pass in GHA release notes
generator
([#13608](#13608))
([c365c10](c365c10))
* **release:** check remote tags using git ls-remote to prevent already
exists error
([#13574](#13574))
([6cf0567](6cf0567))
* **spanner:** fail fast when inline-begin DML returns no transaction id
([#13536](#13536))
([94315ce](94315ce))
* **spanner:** pin inline read-write transactions to default endpoint
([#13562](#13562))
([6908dee](6908dee))
* **spanner:** remove explicit tink version to resolve dependency
convergence conflict
([#13602](#13602))
([863c26e](863c26e))
* **spanner:** update dynamic channel pool default configuration
([#13646](#13646))
([37b77c3](37b77c3))


### Dependencies

* Add org.bouncycastle:bcprov-jdk18on to java-shared-dependencies
([#13531](#13531))
([e3d2082](e3d2082))
* **auth:** Update commons-codec to v1.18.0
([#13598](#13598))
([fd89957](fd89957))
* Update gapic-showcase to v0.41.0
([#13582](#13582))
([09faaac](09faaac))
* Upgrade google-http-client to v2.1.1
([#13578](#13578))
([6abef19](6abef19))


### Documentation

* add instructions for manual code generation checks in CI
([#13596](#13596))
([f6162e2](f6162e2))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants