Skip to content

Fix default range request callback report - #685

Merged
TingDaoK merged 7 commits into
write-to-fdfrom
fix-default-range
Sep 21, 2026
Merged

TingDaoK merged 7 commits into
write-to-fdfrom
fix-default-range

Conversation

@TingDaoK

Copy link
Copy Markdown
Contributor

Issue #, if available:

typedef int(aws_s3_meta_request_receive_body_callback_fn)(

    /* The meta request that the callback is being issued for. */
    struct aws_s3_meta_request *meta_request,

    /* The body data for this chunk of the object. */
    const struct aws_byte_cursor *body,

    /* The byte index of the object that this refers to. For example, for an HTTP message that has a range header, the
       first chunk received will have a range_start that matches the range header's range-start.*/
    uint64_t range_start,

    /* User data specified by aws_s3_meta_request_options.*/
    void *user_data);

Description of changes:

  • The range_start in the body callback is the byte index of the object that this refers to.
  • It's confusing, but hey, the ship has sailed since first we add it. Not complain yet
  • The auto range get respects this, but the default type get is not.
  • Fixing the default type get to respect this.
  • Add test so that with a range to get that write to a file will not mess up with file offset
  • Add test to make sure the body callback respect the range correctly.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@TingDaoK
TingDaoK requested a review from a team as a code owner September 18, 2026 20:45
@TingDaoK TingDaoK changed the title Fix default range Fix default range request callback report Sep 18, 2026
Comment thread source/s3_meta_request.c Outdated
* for itself: 0 is what it holds before anything resolves it and also what a whole-object download
* resolves it to. Reaching here first would place the body at its absolute object offset rather than
* the base position, with nothing about the outcome looking wrong. */
AWS_ASSERT(meta_request->recv_file_object_offset_origin_resolved);

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.

AWS_ASSERT gets removed on non-debug builds. Any reason not to keep this and the below check in place for release builds and fail loudly?

@sbSteveK sbSteveK 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.

Curious about only debug fails. Otherwise looks good.

@codecov-commenter

codecov-commenter commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.99%. Comparing base (2141dda) to head (ef8b9ec).

Files with missing lines Patch % Lines
source/s3_meta_request.c 84.61% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           write-to-fd     #685      +/-   ##
===============================================
- Coverage        89.03%   88.99%   -0.04%     
===============================================
  Files               23       23              
  Lines             8591     8609      +18     
===============================================
+ Hits              7649     7662      +13     
- Misses             942      947       +5     
Files with missing lines Coverage Δ
source/s3_auto_ranged_get.c 95.02% <100.00%> (+0.01%) ⬆️
source/s3_default_meta_request.c 94.24% <100.00%> (+0.21%) ⬆️
source/s3_meta_request.c 90.33% <84.61%> (-0.25%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TingDaoK
TingDaoK merged commit 1642596 into write-to-fd Sep 21, 2026
36 of 39 checks passed
@TingDaoK
TingDaoK deleted the fix-default-range branch September 21, 2026 16:47
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.

3 participants