Skip to content

Add streaming APIs to build cache provider interface and all cache plugins#5746

Open
Copilot wants to merge 33 commits intomainfrom
copilot/stream-cache-entry-for-http-plugin
Open

Add streaming APIs to build cache provider interface and all cache plugins#5746
Copilot wants to merge 33 commits intomainfrom
copilot/stream-cache-entry-for-http-plugin

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026 •
edited
Loading

  • Address review comments: fix response.resume race, stream retry bug, error logging, stream cleanup, and nits
  • Add unit tests for streaming cache functionality
    • HttpBuildCacheProvider tests: Add streaming GET/SET tests, credential fallback skip for streams, write-not-allowed check, retry behavior (maxAttempts=1 for streams) — expanded from 2 to 13 tests
    • AmazonS3Client tests: Add streaming GET/SET tests (getObjectStreamAsync, uploadObjectStreamAsync), UNSIGNED-PAYLOAD signing, no-retry for stream uploads — expanded from 32 to 37 tests (50 total with credentials and provider tests)
    • HttpBuildCacheProvider tests: Fill gaps in buffer-based tests: successful GET returning buffer, trySetCacheEntryBufferAsync (success, write-not-allowed, retry on server error)
    • Build and run tests to verify

Copilot AI and others added 4 commits April 5, 2026 03:35
@iclanton
feat: add streaming support to build cache provider interface and HTT…
6fd3096 
…P plugin

Add optional tryGetCacheEntryStreamByIdAsync and trySetCacheEntryStreamAsync
methods to ICloudBuildCacheProvider. Implement streaming in HttpBuildCacheProvider
and update OperationBuildCache to prefer streaming when available. Add
fetchStreamAsync to WebClient and stream write support to
FileSystemBuildCacheProvider.

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/050e10a7-3cad-4da4-93e5-9941453283b9

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
@iclanton
fix lint warnings, add change files and API report update
2462253 
Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/050e10a7-3cad-4da4-93e5-9941453283b9

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
@iclanton
address code review feedback: improve type annotations and JSDoc
5c783a9 
Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/050e10a7-3cad-4da4-93e5-9941453283b9

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
@iclanton
Replace manual change files with rush change --bulk generated change …
1da21c9 
…file

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/798065bf-040f-462e-8ea9-b9ca5659196d

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Apr 5, 2026
@iclanton
Copy link
Copy Markdown
Member

iclanton commented Apr 5, 2026

@copilot - add APIs to FileSystem in node-core-library for the fs APIs you're making use of in this PR. Ideally, none of our projects (except for FileSystem itself) should use fs. Everything should use FileSystem. The change to node-core-library will also need a changefile. Take a look at the conventions in FileSystem and follow that pattern.

@iclanton
Add createReadStream/createWriteStream to FileSystem in node-core-lib…
468caf7 
…rary and update consumers to use FileSystem instead of fs directly

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/5ad20d4d-c9a4-4855-bb13-8dd9e2c1350b

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 5, 2026

@copilot - add APIs to FileSystem in node-core-library for the fs APIs you're making use of in this PR. Ideally, none of our projects (except for FileSystem itself) should use fs. Everything...

Added createReadStream() and createWriteStream() static methods to FileSystem in node-core-library, along with FileSystemReadStream and FileSystemWriteStream type aliases (following the same pattern as FileSystemStats and FolderItem). Updated FileSystemBuildCacheProvider and OperationBuildCache to use the new FileSystem APIs instead of importing from node:fs directly. Change file for @rushstack/node-core-library included as a minor bump. Commit: 468caf7

Copilot AI requested a review from iclanton April 5, 2026 04:32
@iclanton
Copy link
Copy Markdown
Member

iclanton commented Apr 5, 2026

@copilot - are there async variants of those fs APIs? Should we expose and use those too?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 5, 2026

@copilot - are there async variants of those fs APIs? Should we expose and use those too?

No, fs.createReadStream() and fs.createWriteStream() don't have async variants — they're already inherently asynchronous. They return stream objects immediately and all the actual I/O happens asynchronously through the stream events/pipeline. The fs/promises module doesn't include them for this reason.

@iclanton
Copy link
Copy Markdown
Member

iclanton commented Apr 5, 2026

@copilot - take a look at the other FileSystem APIs. Many have an optional options parameter with useful utilities. For example, the write operations have an option to create the parent folder if it doesn't exist. Consider adding useful options to these new APIs

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 5, 2026

@copilot - take a look at the other FileSystem APIs. Many have an optional options parameter with useful utilities. For example, the write operations have an option to create the parent folder if it...

Added IFileSystemCreateWriteStreamOptions with ensureFolderExists to createWriteStream(), following the same pattern as IFileSystemWriteBinaryFileOptions and other write APIs. Updated FileSystemBuildCacheProvider to use the new option instead of manual ensureFolderAsync(). For createReadStream, there aren't useful options from the existing FileSystem conventions (read methods use encoding/convertLineEndings which don't apply to raw streams). Commit: 91161d8

@iclanton
Copy link
Copy Markdown
Member

iclanton commented Apr 5, 2026

@copilot - can we add stream support to the other cache plugins?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 5, 2026

@copilot - can we add stream support to the other cache plugins?

Added streaming support to both remaining cache plugins:

Amazon S3 plugin (733ef5c):

  • Refactored AmazonS3Client._makeRequestAsync to extract signing into _buildSignedRequest
  • Added _makeStreamRequestAsync using WebClient.fetchStreamAsync() with UNSIGNED-PAYLOAD for AWS Sig V4 signing of streaming uploads
  • Added getObjectStreamAsync() (with retry support) and uploadObjectStreamAsync() (single attempt, since consumed streams can't be replayed)
  • Added tryGetCacheEntryStreamByIdAsync and trySetCacheEntryStreamAsync to AmazonS3BuildCacheProvider

Azure Storage plugin (733ef5c):

  • Added tryGetCacheEntryStreamByIdAsync using blobClient.download() with readableStreamBody
  • Added trySetCacheEntryStreamAsync using blockBlobClient.uploadStream()
  • Added _logBlobError helper for consistent error handling across buffer and stream methods

All three cache plugins (HTTP, S3, Azure) now implement both streaming methods. All existing tests pass (45 S3, 10 Azure).

Copilot AI changed the title Add streaming APIs to build cache provider interface and HTTP plugin Add streaming APIs to build cache provider interface and all cache plugins Apr 5, 2026
iclanton
iclanton reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

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

Thanks for the streaming build cache work — the interface design with optional streaming methods and graceful fallback is clean. A few issues I spotted during review, two of which look like functional bugs.

iclanton
iclanton reviewed Apr 6, 2026
View reviewed changes
@iclanton
Fix CI: Move WebClient private members to module-level for rush-sdk t…
96b5351 
…ype compatibility

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/d8f5e137-3cb0-4ed9-8391-50d7a33d804e

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
Copilot AI requested a review from iclanton April 6, 2026 02:59
@iclanton
Address review comments: fix response.resume race, stream retry bug, …
1add881 
…error logging, stream cleanup, and nits

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/d0190bf7-a346-4a71-93bf-d5375d98b552

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>
iclanton and others added 7 commits April 5, 2026 22:03
@iclanton @claude
Fix buffer path error message to show specific unsupported encoding v…
b5caf17 
…alue

The streaming path was fixed in 8becd08 but the buffer path still
printed the entire encodings array instead of the individual value.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Extract _getObjectName and _validateWriteAllowed helpers in S3 provider
912d5bb 
Deduplicate the S3 prefix logic (repeated 4 times) into a helper,
and extract the write-permission guard (repeated in buffer and stream
set methods) to match the HTTP provider's pattern.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Extract _getContentEncodings helper to deduplicate encoding parsing
7d80859 
Both the buffer and streaming response paths duplicated the same
Content-Encoding header parsing logic. Extract into a shared helper
that returns a parsed string array or undefined.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Scope cacheEntryBuffer to its branch, use cloudCacheHit flag
d729a97 
Replace the outer-scoped cacheEntryBuffer (which was only used as
a boolean flag at the cache-miss check) with an explicit cloudCacheHit
boolean. This scopes the buffer into the else branch and makes the
streaming path set the flag consistently too.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Reuse a single WebClient instance in HttpBuildCacheProvider
3d1e6b5 
A new WebClient was being constructed on every call to
_makeHttpCoreRequestAsync. Since the provider never configures any
WebClient instance state, a single class member suffices.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Use property shorthand in HttpBuildCacheProvider
fe53e65 
Replace verbose `terminal: terminal`, `headers: headers`, `body: body`,
and `credential: credential` with ES6 shorthand property syntax.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton @claude
Fix unit test issues in streaming cache tests
ea777a0 
- Fix credential fallback test: mock CredentialCache so cached credentials
  are available, making the test actually validate the stream-body guard
  (previously, the test passed trivially because _tryGetCredentialsAsync
  would throw before making a second request)
- Fix 504 statusText from 'BadGateway' to 'Gateway Timeout'
- Replace fragile S3 upload snapshot that captured Readable internals
  (breaks on Node.js upgrades) with targeted assertions on URL, verb,
  headers, and body identity
- Replace fail() + try/catch with expect().rejects.toThrow()
- Move Readable import to top of S3 test file instead of per-test
  dynamic import

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@iclanton iclanton marked this pull request as ready for review April 6, 2026 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iclanton iclanton Awaiting requested review from iclanton iclanton is a code owner

@octogonz octogonz Awaiting requested review from octogonz octogonz is a code owner

@apostolisms apostolisms Awaiting requested review from apostolisms apostolisms is a code owner

@dmichon-msft dmichon-msft Awaiting requested review from dmichon-msft dmichon-msft is a code owner

@jxanthony jxanthony Awaiting requested review from jxanthony jxanthony is a code owner

@bmiddha bmiddha Awaiting requested review from bmiddha bmiddha is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@iclanton iclanton

Copilot code review Copilot

Labels

None yet

Projects

Bug Triage
Status: Needs triage

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iclanton @github-advanced-security