Skip to content

Comments

fix(downloader): record bucket success after response validation#75

Merged
shikhar merged 1 commit intomainfrom
gh-69
Feb 19, 2026
Merged

fix(downloader): record bucket success after response validation#75
shikhar merged 1 commit intomainfrom
gh-69

Conversation

@shikhar
Copy link
Member

@shikhar shikhar commented Feb 19, 2026

Fixes #69

  • record bucket stats only after full handle_result outcome is known
  • treat content-range validation and body-streaming failures as failed attempts in bucket metrics
  • fold failure-metrics assertion into the existing range-mismatch unit test

@greptile-apps
Copy link

greptile-apps bot commented Feb 19, 2026

Greptile Summary

This PR fixes a bug where bucket success metrics were recorded prematurely before content-range validation and body streaming completed.

Key changes:

  • Deferred bucketed_stats.observe() call until after all validation and body streaming is complete
  • Wrapped the success path in an async block to ensure all potential failures (content-range mismatches, body streaming errors) are captured before metrics recording
  • Enhanced test_handle_result_range_mismatch to assert that range validation failures correctly increment consecutive_failures metric

Impact:

  • Content-range validation failures and body streaming errors now correctly count as failed attempts in bucket metrics
  • This ensures circuit breaker logic and bucket prioritization work correctly even when responses initially appear successful but fail during validation/streaming

Confidence Score: 5/5

  • This PR is safe to merge with no issues found
  • The change correctly fixes a timing bug in metrics recording by deferring observation until after all validation completes. The logic is sound, test coverage validates the fix, and the implementation follows Rust best practices with proper error handling.
  • No files require special attention

Important Files Changed

Filename Overview
src/object_store/downloader.rs Deferred bucket metrics recording to ensure content-range validation and body-streaming failures are correctly counted as failures. Added test assertion for failure metrics.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[handle_result called] --> B{Result type?}
    B -->|Ok| C[Start async block]
    B -->|Err| D[Map service error to DownloadError]
    C --> E[Validate content-range header]
    E -->|Invalid range| F[Return RangeNotSatisfied]
    E -->|Valid| G[Stream body data]
    G -->|Stream error| H[Return BodyStreaming error]
    G -->|Success| I[Validate body length]
    I -->|Mismatch| J[Return BodyStreaming error]
    I -->|Match| K[Return Ok ObjectPiece]
    F --> L[Record failure in bucketed_stats]
    H --> L
    J --> L
    D --> L
    K --> M[Record success in bucketed_stats]
    L --> N[Return final_result]
    M --> N
Loading

Last reviewed commit: 68024af

@shikhar shikhar merged commit 9eeeaf4 into main Feb 19, 2026
5 checks passed
@shikhar shikhar deleted the gh-69 branch February 19, 2026 03:53
@github-actions github-actions bot mentioned this pull request Feb 19, 2026
shikhar pushed a commit that referenced this pull request Feb 19, 2026
## 🤖 New release

* `cachey`: 0.9.3 -> 0.10.0

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.10.0](0.9.3...0.10.0)
- 2026-02-19

### Added

- [**breaking**] use default eviction policy
([#73](#73))

### Fixed

- *(downloader)* record bucket success after response validation
([#75](#75))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Detail Bug] Downloader prematurely records success before validation/body, skewing bucket stats

1 participant