Closed
Conversation
sdk/storage/azure_storage_blob/src/clients/block_blob_client.rs
Outdated
Show resolved
Hide resolved
LarryOsterman
approved these changes
Mar 20, 2026
LarryOsterman
approved these changes
Mar 23, 2026
dacd0dc to
2a215bb
Compare
heaths
commented
Mar 25, 2026
heaths
added a commit
that referenced
this pull request
Mar 25, 2026
…4027) When `System.Debug` is enabled in AzDO, pipeline tests run without any enhanced logging, making it difficult to diagnose failures in the Rust test code or the `test-proxy` process. ## Changes - **`eng/pipelines/templates/variables/rust.yml`**: Update `RUST_LOG` to use `trace` when `System.Debug == true`, and `info` otherwise (previously `debug` / empty string). This is the single source of truth for `RUST_LOG` across all pipelines. - **`ci.tests.yml` / `live.tests.yml`**: Add a conditional step that sets `Logging__LogLevel__Default=Debug` (ASP.NET Core log level for `test-proxy`) when `System.Debug == true`. The step runs before the test proxy starts and before tests execute so both processes inherit the variable. - **`ci.tests.yml`**: Add a "Dump Test Proxy Audit Logs" step (`GET /Audit/Logs` via `Invoke-WebRequest`) conditioned on `System.Debug == true`, alongside the existing log dump step, with error handling so a failure doesn't mask the original test result. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Enable tracing and more information when System.Debug is checked in AzDO</issue_title> > <issue_description>When we check `System.Debug` (env var `SYSTEM_DEBUG`, I believe) in Azure DevOps, we should set env vars like in #4025 and set `RUST_LOG=trace` to log more information for diagnostics so we can more easily troubleshoot issues like we saw for #4001.</issue_description> > > <agent_instructions>Get changes to the pipeline in #4025 to determine what environment variables to set when System.Debug is checked in Azure Pipelines. There's an Azure Pipelines variable for that or I believe you can also use the SYSTEM_DEBUG environment variable, but you should figure that out. Open a PR that updates the appropriate pipeline under eng/pipelines to set the env vars before starting the test proxy or running tests.</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@heaths</author><body> > We could add a step to our local build scripts under `eng/pipelines` that would export environment variables `RUST_LOG` and whatever #4025 is setting to have ASP.NET's HTTP server log more information, which test-proxy supports.</body></comment_new> > </comments> > </details> > **Custom agent used: agentic-workflows** > GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #4026 <!-- START COPILOT CODING AGENT TIPS --> --- 📱 Kick off Copilot coding agent tasks wherever you are with [GitHub Mobile](https://gh.io/cca-mobile-docs), available on iOS and Android. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: heaths <1532486+heaths@users.noreply.github.com> Co-authored-by: Heath Stewart <heaths@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
LarryOsterman
approved these changes
Mar 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors async body/stream handling across core HTTP and storage layers to better support seekable streaming uploads (e.g., file-backed streams), and updates call sites/tests/examples accordingly.
Changes:
- Made
SeekableStream::len/is_emptyandBody::len/is_emptyasync and updated dependent policies, clients, and tests. - Added buffered
FileStream<T>plus a TokioFileReaderadapter to enable streaming file uploads without loading entire files into memory. - Updated Azure Storage blob partitioned upload pipeline to work with async length, and added an end-to-end upload example using streaming.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure_storage_blob_test/src/lib.rs | Updates test helpers to use async len() for seekable bodies. |
| sdk/storage/azure_storage_blob/src/streams/partitioned_stream.rs | Makes constructor async and caches total length for partition buffering. |
| sdk/storage/azure_storage_blob/src/partitioned_transfer/upload.rs | Updates partitioned upload flow to use async body length and async PartitionedStream::new. |
| sdk/storage/azure_storage_blob/src/clients/block_blob_client.rs | Updates content-length computations to await async len(). |
| sdk/storage/azure_storage_blob/examples/blob_storage_upload_file.rs | New example showing in-memory vs streamed file upload (Tokio + FileStream). |
| sdk/storage/azure_storage_blob/Cargo.toml | Adds example dependencies/features (e.g., clap, tokio/fs). |
| sdk/keyvault/azure_security_keyvault_keys/src/authorizer.rs | Adapts Key Vault auth flow to async Body::is_empty/len. |
| sdk/identity/azure_identity/src/managed_identity_credential.rs | Test adjustment for header iteration typing. |
| sdk/cosmos/azure_data_cosmos/src/fault_injection/http_client.rs | Minor refactor of request cloning/forwarding. |
| sdk/core/typespec_macros/tests/data/safe-debug-tests/tests/safe-debug.rs | Gates a test to avoid running under the debug feature. |
| sdk/core/typespec_macros/tests/data/safe-debug-tests/Cargo.toml | Converts a bench entry into a feature-gated test entry. |
| sdk/core/typespec_client_core/src/stream/tokio.rs | Adds Tokio FileReader implementing SeekableStream + AsyncRead/AsyncSeek. |
| sdk/core/typespec_client_core/src/stream/mod.rs | Adds new stream modules and makes SeekableStream::len/is_empty async. |
| sdk/core/typespec_client_core/src/stream/file_stream.rs | Adds buffered FileStream<T> implementing AsyncRead/AsyncSeek/SeekableStream. |
| sdk/core/typespec_client_core/src/stream/bytes_stream.rs | Adds AsyncSeek for BytesStream and Body conversion. |
| sdk/core/typespec_client_core/src/http/request/mod.rs | Makes Body::len/is_empty async; custom Clone for Body; adjusts tests. |
| sdk/core/typespec_client_core/src/http/policies/transport.rs | Updates empty-body content-length logic to await async is_empty(). |
| sdk/core/typespec_client_core/src/http/policies/retry/mod.rs | Updates retry tests for async + adds coverage for retry body reset behavior. |
| sdk/core/typespec_client_core/src/http/clients/reqwest.rs | Minor refactor around body cloning for reqwest request building. |
| sdk/core/typespec_client_core/src/http/clients/mod.rs | Removes outdated doc text about implementers cloning request internals. |
| sdk/core/typespec_client_core/examples/core_binary_data_request.rs | Updates example to use new FileReader/FileStream approach. |
| sdk/core/typespec_client_core/examples/common/mod.rs | Removes old example support module. |
| sdk/core/typespec_client_core/examples/common/fs.rs | Removes old custom file streaming implementation used by examples. |
| sdk/core/typespec_client_core/Cargo.toml | Extends tokio feature to include tokio/fs; wires examples/feature flags. |
| sdk/core/typespec_client_core/CHANGELOG.md | Documents new stream adapters and the async len/is_empty breaking change. |
| sdk/core/azure_core_test/src/stream.rs | Updates test stream implementation to async SeekableStream::len. |
| sdk/core/azure_core/src/http/policies/auth/bearer_token_policy.rs | Updates auth challenge retry test to async body length checks. |
| sdk/core/azure_core/benches/benchmarks.rs | Adjusts benchmark request construction for updated request/body behavior. |
| sdk/core/azure_core/CHANGELOG.md | Mirrors TypeSpec client core changelog entries for stream/body changes. |
| Cargo.lock | Adds clap to dependency graph. |
| .vscode/cspell.json | Dictionary tweaks (ordering + added words). |
Comments suppressed due to low confidence (1)
sdk/core/typespec_client_core/src/http/request/mod.rs:289
RequestContentstops derivingClonewhenjsonis disabled, while thejson-enabled version is stillClone. Since the struct only containsBody+PhantomData, this looks like an accidental API regression and can break downstream code that relies onRequestContent: Clonein--no-default-features/--no-jsonbuilds. Consider restoring#[derive(Clone, Debug)](or adding a manualimpl Clone) for the non-jsonvariant for API consistency.
/// The body content of a service client request.
///
/// This allows callers to pass a model to serialize or raw content to client methods.
#[cfg(not(feature = "json"))]
#[derive(Debug)]
pub struct RequestContent<T, F> {
body: Body,
phantom: PhantomData<(T, F)>,
}
sdk/storage/azure_storage_blob/src/streams/partitioned_stream.rs
Outdated
Show resolved
Hide resolved
sdk/storage/azure_storage_blob/src/partitioned_transfer/upload.rs
Outdated
Show resolved
Hide resolved
sdk/core/azure_core/src/http/policies/auth/bearer_token_policy.rs
Outdated
Show resolved
Hide resolved
demoray
reviewed
Mar 26, 2026
Updated the HTTP client implementation to accept mutable requests, allowing for more efficient request handling. This change improves the ability to modify request bodies and headers directly, enhancing performance and reducing unnecessary cloning. Additionally, several async methods were updated to ensure proper asynchronous behavior, particularly in the body handling and stream management. This refactor aims to streamline the request execution process and improve overall code maintainability. Fixes Azure#3971
This commit introduces the `FileStream` and `FileReader` types to facilitate asynchronous file reading and seeking. The `FileStream` provides a buffered stream adapter, while `FileReader` wraps a standard file for async operations. These additions enhance the library's capabilities for handling file I/O in an asynchronous context, improving performance and usability.
We don't need it, and don't necessarily want to have to maintain it for a general purpose file buffer.
No real need for it, and would be slower than a sync read into a buffer. If large files are necessary, use tokio::fs::File or similar async file with a reader like we have for `stream::tokio`.
Changing the pipeline policies to take a `&mut Request` allowed an inadvertent regression that cleared the request body and completely broke retries, which we only caught with test-proxy. Thus, I also added a test.
Member
Author
|
Rebased to keep this somewhat current as I work on a separate PR. |
Member
Author
|
Replaced by #4057 |
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.
Add
FileStreamAPI for easier streaming of request bodies e.g., from a file including aFileReaderfortokio::fs::File. Makes some methods likeSeekableStream::len()async since underlying APIs for file access are async.Fixes #3971