Repository navigation
fix(inkless:storage): count retried GCS error responses - #789
Merged
Merged
Conversation
The Google HTTP client invokes response interceptors after its internal retry loop. A credential refresh can therefore consume a 401 response without advancing the GCS error counters, and the credentials handler reinstalls itself after refreshing a token. Record unsuccessful responses before the client decides whether to retry. Delegate to the existing credentials handler and restore the metrics wrapper afterward so later failures from the same request are also counted. Keep the response interceptor responsible for operation counters. Describe other-errors as 4xx responses other than 429. This matches the implementation and excludes protocol responses such as 308 Resume Incomplete. Co-authored-by: Cursor <cursoragent@cursor.com>
giuseppelillo
force-pushed
the
fix/inkless-gcs-error-metrics
branch
from
September 8, 2026 09:17
039f295 to
0812233
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The handler lifecycle, metric classification, documentation, and targeted retry coverage are consistent.
Pull request overview
Updates GCS metrics to count every failed HTTP response, including retried authorization failures.
Changes:
- Adds a metrics-aware unsuccessful-response handler.
- Preserves operation counting after retries.
- Updates metric documentation and retry tests.
File summaries
| File | Description |
|---|---|
MetricCollector.java |
Counts errors before retries and preserves credential handling. |
MetricRegistry.java |
Clarifies other-errors classification. |
MetricCollectorTest.java |
Verifies repeated 401 responses are counted. |
metrics.rst |
Updates generated metric documentation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jeqo
reviewed
Sep 8, 2026
Contributor
There was a problem hiding this comment.
Fix looks good. I left a couple of comments to decide the direction. Otherwise LGTM.
Batch delete sub-responses pass through the unsuccessful-response handler. A missing object is an idempotent delete success, but the handler counts its 404 as an error. The object-delete counter also counts only confirmed results, which omits refused sub-requests even though GCS bills and rate-limits them as operations. Skip absent batch deletes when recording provider errors, and count every returned batch sub-request as an object-delete operation. Keep failed deletes out of the confirmed-key result so FileCleaner retries them. Cover refused and already-absent batches with exact error and operation totals. Co-authored-by: Cursor <cursoragent@cursor.com>
jeqo
approved these changes
Sep 9, 2026
jeqo
pushed a commit
that referenced
this pull request
Sep 9, 2026
* fix(inkless:storage): count retried GCS error responses The Google HTTP client invokes response interceptors after its internal retry loop. A credential refresh can therefore consume a 401 response without advancing the GCS error counters, and the credentials handler reinstalls itself after refreshing a token. Record unsuccessful responses before the client decides whether to retry. Delegate to the existing credentials handler and restore the metrics wrapper afterward so later failures from the same request are also counted. Keep the response interceptor responsible for operation counters. Describe other-errors as 4xx responses other than 429. This matches the implementation and excludes protocol responses such as 308 Resume Incomplete. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(inkless:storage): align GCS batch delete metrics Batch delete sub-responses pass through the unsuccessful-response handler. A missing object is an idempotent delete success, but the handler counts its 404 as an error. The object-delete counter also counts only confirmed results, which omits refused sub-requests even though GCS bills and rate-limits them as operations. Skip absent batch deletes when recording provider errors, and count every returned batch sub-request as an object-delete operation. Keep failed deletes out of the confirmed-key result so FileCleaner retries them. Cover refused and already-absent batches with exact error and operation totals. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
jeqo
pushed a commit
that referenced
this pull request
Sep 9, 2026
* fix(inkless:storage): count retried GCS error responses The Google HTTP client invokes response interceptors after its internal retry loop. A credential refresh can therefore consume a 401 response without advancing the GCS error counters, and the credentials handler reinstalls itself after refreshing a token. Record unsuccessful responses before the client decides whether to retry. Delegate to the existing credentials handler 8000 and restore the metrics wrapper afterward so later failures from the same request are also counted. Keep the response interceptor responsible for operation counters. Describe other-errors as 4xx responses other than 429. This matches the implementation and excludes protocol responses such as 308 Resume Incomplete. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(inkless:storage): align GCS batch delete metrics Batch delete sub-responses pass through the unsuccessful-response handler. A missing object is an idempotent delete success, but the handler counts its 404 as an error. The object-delete counter also counts only confirmed results, which omits refused sub-requests even though GCS bills and rate-limits them as operations. Skip absent batch deletes when recording provider errors, and count every returned batch sub-request as an object-delete operation. Keep failed deletes out of the confirmed-key result so FileCleaner retries them. Cover refused and already-absent batches with exact error and operation totals. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
jeqo
pushed a commit
that referenced
this pull request
Sep 9, 2026
* fix(inkless:storage): count retried GCS error responses The Google HTTP client invokes response interceptors after its internal retry loop. A credential refresh can therefore consume a 401 response without advancing the GCS error counters, and the credentials handler reinstalls itself after refreshing a token. Record unsuccessful responses before the client decides whether to retry. Delegate to the existing credentials handler and restore the metrics wrapper afterward so later failures from the same request are also counted. Keep the response interceptor responsible for operation counters. Describe other-errors as 4xx responses other than 429. This matches the implementation and excludes protocol responses such as 308 Resume Incomplete. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(inkless:storage): align GCS batch delete metrics Batch delete sub-responses pass through the unsuccessful-response handler. A missing object is an idempotent delete success, but the handler counts its 404 as an error. The object-delete counter also counts only confirmed results, which omits refused sub-requests even though GCS bills and rate-limits them as operations. Skip absent batch deletes when recording provider errors, and count every returned batch sub-request as an object-delete operation. Keep failed deletes out of the confirmed-key result so FileCleaner retries them. Cover refused and already-absent batches with exact error and operation totals. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
PR #783 added GCS error counters through an
HttpResponseInterceptor. The Google HTTP client invokes that interceptor only afterHttpRequest.execute()finishes its internal retry loop. When GCS returns401,HttpCredentialsAdaptercan refresh the token and retry the same request before the interceptor runs, so a successful refresh hides the failed response and repeated authorization failures are undercounted.The credentials handler also reinstalls itself after a refresh. Wrapping it without restoring the metrics wrapper would therefore count only the first
401from a request.Batch requests add a second distinction. The outer POST bypasses the request initializer, but each failed multipart sub-response invokes the unsuccessful-response handler. A
404delete means the object is already absent, andStorageBatchtreats that outcome as a confirmed idempotent delete, so counting it as a provider error creates noise during cleaner retries. GCS still bills and rate-limits every submitted delete sub-request, including refused requests, so the operation counter must include all of them.Changes
HttpUnsuccessfulResponseHandler, which runs before the client decides whether to retry each response.other-errors, while continuing to count refused and throttled sub-requests.object-deleteoperation; confirmed deletions remain available through cleaner metrics.other-errorsas non-4294xx responses, excluding absent batch deletes. Protocol responses such as308 Resume Incompletealso remain excluded.Testing
./gradlew :storage:inkless:test --tests io.aiven.inkless.storage_backend.gcs.MetricCollectorTest --tests io.aiven.inkless.storage_backend.gcs.integration.GcsErrorHandlingTest --tests io.aiven.inkless.storage_backend.gcs.integration.GcsStorageMetricsTest./gradlew :storage:inkless:genInklessMetricsDoc