Make TransferredBytes be the top of the list in BinLabel

[ysaito1001]

Motivation and Context

awslabs/aws-sdk-rust#1202

Description

The issue above demonstrated the incorrect BinLabel ordering in LogBuffer, the underlying data structure we use for stall stream protection.

The following trace logs are generated from executing the reproduction steps in the issue above. In the file labeled "no_sleep," we have commented out std::thread::sleep(std::time::Duration::from_millis(120)); from the reproducer so the updated code can be tested as the happy path.

s3_throughput_min_repro_no_sleep.log

s3_throughput_min_repro_with_sleep.log

In both files, it’s important to note that Bins assigned TransferredBytes can be overwritten by Pending due to ThroughputLogs::push. Once a Bin is labeled as Pending, it cannot be re-labeled.

When this occurs, the only way to avoid the stall stream protection check going into the grace period is for time to advance beyond the current Bin's resolution, the LogBuffer pushes a new Bin during catch_up , and this new Bin hopefully gets assigned a TransferredBytes. However, this new Bin could also be overwritten by Pending in a subsequent call to MinimumThroughputDownloadBody::poll_data, which can trigger the the grace period if the overall LogBuffer looks like it's violated the stall stream protection check.

The reproducer without sleep does not fail the stall stream protection obviously because the execution completes way before the grace period ends, but more importantly because the execution periodically assigns new TransferredBytes Bins in the throughput logs. This effectively resets the grace period for the stall stream protection (search for throughput recovered; exiting grace period in the s3_throughput_min_repro_no_sleep.log). However, with sleep, Bins labeled as TransferredBytes are frequently (and almost immediately) overwritten by Pending. This results in the execution being unable to exit the grace period, ultimately leading to a stall stream protection error.

To resolve this, we make TransferredBytes be the top priority in BinLabel. This means once a new Bin has earned TransferredBytes, it's green for that time resolution and that it should not be revoked by Pending overwriting it to make it look like no bytes transferred during that time.

Testing

• Existing tests in CI
• Added unit tests for BinLabel ordering and for ThroughputLogs
• Passed the customer's reproduction step
• To confirm the stall stream protection for download still works, I switched off WiFi while running the customer's reproducer (with sleep) and it successfully failed with the stall stream protection error:

---- s3_throughput_min_repro stdout ----
2024-10-08T23:29:24.999477Z DEBUG aws_smithy_runtime::client::http::body::minimum_throughput::http_body_0_4_x: current throughput: 0 B/s is below minimum: 1 B/s
2024-10-08T23:29:24.999513Z TRACE aws_smithy_runtime::client::http::body::minimum_throughput::http_body_0_4_x: received poll pending
2024-10-08T23:29:24.999530Z DEBUG aws_smithy_runtime::client::http::body::minimum_throughput::http_body_0_4_x: current throughput: 0 B/s is below minimum: 1 B/s
2024-10-08T23:29:25.081811Z TRACE aws_smithy_runtime::client::http::body::minimum_throughput::http_body_0_4_x: received poll pending
2024-10-08T23:29:25.081938Z DEBUG aws_smithy_runtime::client::http::body::minimum_throughput::http_body_0_4_x: current throughput: 0 B/s is below minimum: 1 B/s
test s3_throughput_min_repro ... FAILED
...
called `Result::unwrap()` on an `Err` value: Custom { kind: Other, error: Error { kind: StreamingError(ThroughputBelowMinimum { expected: Throughput { bytes_read: 1, per_time_elapsed: 1s }, actual: Throughput { bytes_read: 0, per_time_elapsed: 1s } }) } }

Checklist

• For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.
• For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Client Test
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python
• No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Client Test
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python
• No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

[landonxjames]

Looks good! One non-blocking request, would it be possible to create a test that replicates the reproduction from awslabs/aws-sdk-rust#1202? Might be too difficult since we would need some kind of mocked server to talk to? In that case I would prioritize getting this out since it is currently causing customer issues.

[ysaito1001]

Looks good! One non-blocking request, would it be possible to create a test that replicates the reproduction from awslabs/aws-sdk-rust#1202? Might be too difficult since we would need some kind of mocked server to talk to? In that case I would prioritize getting this out since it is currently causing customer issues.

I think it would, using test utilities John originally wrote (it's in integration tests of aws-smithy-runtime).

In that case I would prioritize getting this out since it is currently causing customer issues.

Since I've verified the customer repro and added relevant unit tests, let's get it out first to customers' hands. I'll work on adding an integ test in the next PR.