Skip to content

Fixes for S3 uploading bugs.#1984

Merged
daviesrob merged 3 commits intosamtools:developfrom
whitwham:s3_upload_wrong_return
Feb 5, 2026
Merged

Fixes for S3 uploading bugs.#1984
daviesrob merged 3 commits intosamtools:developfrom
whitwham:s3_upload_wrong_return

Conversation

@whitwham
Copy link
Member

@whitwham whitwham commented Feb 2, 2026

First bug prevented the finalisation of the upload in AWS. This was caused by not removing two header entries which caused the signature to be miscalculated.

Second was caused by failure to re-check the error state after a redirection.

Hopefully this should fix #1979.

First bug prevented the finalisation of the upload in AWS.  This was caused by not removing two header entries which caused the signature to be miscalculated.

Second was caused by failure to re-check the error state after a redirection.
hfile_s3.c Outdated
ret = -1;
cret = curl_easy_getinfo(fp->curl, CURLINFO_RESPONSE_CODE, &response_code);

fprintf(stderr, "*** GOT HERE %ld***\n", response_code);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this got left in by mistake...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sigh, yes.

hfile_s3.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think we should bail out here it ret != 0. If initialise_upload() has failed, the http request may not even have been made so there's not much point in trying to get the response code. It might also simplify the core below by removing a layer of if () statement.

Similarly, if initialise_upload() gets called again below then we should also bail out there if it fails.

hfile_s3.c Outdated
}

// reget the response code (may not have changed)
cret = curl_easy_getinfo(fp->curl, CURLINFO_RESPONSE_CODE, &response_code);
Copy link
Member

Choose a reason for hiding this comment

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

It probably isn't too harmful, but ideally we'd only do this if we've called initialise_upload() again (and it worked).

hfile_s3.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

While testing this, I got a 307 Temporary Redirect response. According to the docs this can happen "while the DNS server is being updated", probably by trying to access a bucket that's only just been created. Handling it like 301 Moved Permanently seems to work (i.e. just check for both response codes here, and also in s3_open_read()).

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good find.

@daviesrob daviesrob merged commit 396e5c9 into samtools:develop Feb 5, 2026
9 checks passed
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.

S3 Multipart Upload: s3w+https, s3w+http, s3w works in 1.22.1 fails 1.23

2 participants