Faster test-recording restore via shared clone - #59341
Conversation
Replaces the per-package sequential `test-proxy restore` loop in
`set-artifact-packages.yml` with a single shared bare git clone that
fans out to all packages via local hardlinks / object alternates.
For each unique assets repo, the new `Restore-RecordingsShared.ps1`:
1. Maintains one shared bare clone in
`$(Agent.TempDirectory)/.assets-shared/<owner__repo>`.
2. Fetches all required tags in chunks of 30 in one `git fetch` per
chunk using explicit `+refs/tags/<tag>:refs/tags/<tag>` refspecs.
3. Materializes each package via
`git clone --local --shared --no-checkout <shared> <package>/.assets`
followed by `git -C <package>/.assets checkout <tag> -- .`
Restored file content is functionally equivalent to the per-package
`test-proxy restore` (validated by the original spec via SHA-256 of
every restored file, and locally via an apples-to-apples benchmark on
8 real packages):
- Baseline (sequential test-proxy restore): 111.9s
- New (shared clone, cold): 9.5s (11.8x faster)
- New (shared clone, warm): 6.8s (16.4x faster)
The script fails loudly (throws + non-zero exit) on any failure - no
silent fallback to producing potentially-wrong recordings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes CI test-recording restore by replacing the per-package sequential test-proxy restore loop with a single PowerShell script that uses a shared bare git clone per assets repo and fans out per-package .assets directories via git clone --local --shared.
Changes:
- Added
eng/scripts/Restore-RecordingsShared.ps1to restore recordings using a shared bare clone plus batched tag fetches. - Updated
eng/pipelines/templates/steps/set-artifact-packages.ymlto call the new shared-restore script instead of looping over packages and invokingtest-proxy restore.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| eng/scripts/Restore-RecordingsShared.ps1 | New shared-clone restore implementation (batched tag fetch + local shared clones). |
| eng/pipelines/templates/steps/set-artifact-packages.yml | Swaps per-package restore loop for a single call to the new shared restore script. |
Jesse Squire (jsquire)
left a comment
There was a problem hiding this comment.
I think copilot hit a couple of the things that I'd call out. Because the assets.json is part of the local working set and this will run in CI, it would be good to think through any validation and sanitization of values used from it.
We'll want to constrain to reasonable efforts and pragmatism. Even in the worst case, an attacker could only compromise an ephemeral sandbox with no access to company resources, tokens, etc. So, defense-in-depth rather than vulnerability.
Many assets.json files use "Tag": "" as a valid placeholder for packages that have no recordings yet (66 of 336 across sdk/). The previous validation threw for both missing-and-empty, which would break CI for those packages. test-proxy itself treats empty Tag as a no-op clone with nothing checked out. This mirrors that behavior: only throw when the Tag (or AssetsRepo) property is absent. When Tag is empty/whitespace, log a skip message and remove any stale .assets directory so the package starts in a known-empty state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses three review-bot/reviewer comments:
1. Path-traversal risk in shared-clone directory name:
AssetsRepo flows into both a filesystem path (Join-Path under
SharedCloneRoot) and a remote URL. Validate it upfront against
'<owner>/<repo>' (letters/digits/'.'/'_'/'-') and explicitly reject
'.'/'..' segments so a malicious assets.json can't escape the shared
clone root.
2. Option injection in 'git checkout <tag>':
Validate Tag via 'git check-ref-format refs/tags/<tag>' (rejects
whitespace, control chars, '..', '@{', etc.). Also pass the tag as
'refs/tags/<tag>' to the checkout so even tags starting with '-' can't
be parsed as a git option.
3. Cosmetic: align the FetchChunkSize param spacing with the other params.
Smoke-tested 10 malicious-input cases (all rejected) plus the valid path
and empty-Tag path (still work). Local benchmark on 8 packages confirms
no regression: baseline 114.6s, cold 10.4s (~11x), warm 7.3s (~16x) -
the ~1s difference vs the previous run is the 8 'git check-ref-format'
calls.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The shared-clone restore (#59341) fails on Windows agents for packages whose recordings live at deep paths in Azure/azure-sdk-assets, because git's per-package clone+checkout hits MAX_PATH (260) on default Windows agents (LongPathsEnabled=0, no core.longpaths set). Apply Option 3 from the bug report: - Set core.longpaths=true on the shared bare clone (idempotent, applied to both freshly-initialized and reused clones). - For each per-package clone, pass -c core.longpaths=true on the clone and checkout, plus 'git config core.longpaths true' post-clone. - Configure cone-mode sparse-checkout to <AssetsRepoPrefixPath>/<DirectoryPath> before checkout so only the package's own subtree is materialized - matches test-proxy restore semantics and reduces disk/IO. - Drop the '-- .' pathspec from the final checkout so HEAD detaches to the tag cleanly with sparsity applied. Touches the README of Azure.ResourceManager.ServiceFabricManagedClusters (a canonical long-path package from the bug report) so CI exercises the deep-path restore on a Windows agent and we can verify the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…checkout + core.longpaths) (#59370) * Fix Windows MAX_PATH failure in Restore-RecordingsShared.ps1 The shared-clone restore (#59341) fails on Windows agents for packages whose recordings live at deep paths in Azure/azure-sdk-assets, because git's per-package clone+checkout hits MAX_PATH (260) on default Windows agents (LongPathsEnabled=0, no core.longpaths set). Apply Option 3 from the bug report: - Set core.longpaths=true on the shared bare clone (idempotent, applied to both freshly-initialized and reused clones). - For each per-package clone, pass -c core.longpaths=true on the clone and checkout, plus 'git config core.longpaths true' post-clone. - Configure cone-mode sparse-checkout to <AssetsRepoPrefixPath>/<DirectoryPath> before checkout so only the package's own subtree is materialized - matches test-proxy restore semantics and reduces disk/IO. - Drop the '-- .' pathspec from the final checkout so HEAD detaches to the tag cleanly with sparsity applied. Touches the README of Azure.ResourceManager.ServiceFabricManagedClusters (a canonical long-path package from the bug report) so CI exercises the deep-path restore on a Windows agent and we can verify the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Revert temporary README touch on ServiceFabricManagedClusters CI on the prior commit confirmed the Restore-RecordingsShared.ps1 fix worked on Windows agents for this deep-path package - all 5 Windows test jobs succeeded the 'Restore Recordings (shared clone)' step. Now removing the README touch so this PR only contains the actual fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Faster test-recording restore via shared clone
Replaces the per-package sequential `test-proxy restore` loop in
`set-artifact-packages.yml` with a single shared bare git clone that
fans out to all packages via local hardlinks / object alternates.
For each unique assets repo, the new `Restore-RecordingsShared.ps1`:
1. Maintains one shared bare clone in
`$(Agent.TempDirectory)/.assets-shared/<owner__repo>`.
2. Fetches all required tags in chunks of 30 in one `git fetch` per
chunk using explicit `+refs/tags/<tag>:refs/tags/<tag>` refspecs.
3. Materializes each package via
`git clone --local --shared --no-checkout <shared> <package>/.assets`
followed by `git -C <package>/.assets checkout <tag> -- .`
Restored file content is functionally equivalent to the per-package
`test-proxy restore` (validated by the original spec via SHA-256 of
every restored file, and locally via an apples-to-apples benchmark on
8 real packages):
- Baseline (sequential test-proxy restore): 111.9s
- New (shared clone, cold): 9.5s (11.8x faster)
- New (shared clone, warm): 6.8s (16.4x faster)
The script fails loudly (throws + non-zero exit) on any failure - no
silent fallback to producing potentially-wrong recordings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Treat empty Tag as no-op instead of a hard error
Many assets.json files use "Tag": "" as a valid placeholder for packages
that have no recordings yet (66 of 336 across sdk/). The previous
validation threw for both missing-and-empty, which would break CI for
those packages. test-proxy itself treats empty Tag as a no-op clone with
nothing checked out.
This mirrors that behavior: only throw when the Tag (or AssetsRepo)
property is absent. When Tag is empty/whitespace, log a skip message and
remove any stale .assets directory so the package starts in a known-empty
state.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Validate AssetsRepo/Tag and use refs/tags for checkout
Addresses three review-bot/reviewer comments:
1. Path-traversal risk in shared-clone directory name:
AssetsRepo flows into both a filesystem path (Join-Path under
SharedCloneRoot) and a remote URL. Validate it upfront against
'<owner>/<repo>' (letters/digits/'.'/'_'/'-') and explicitly reject
'.'/'..' segments so a malicious assets.json can't escape the shared
clone root.
2. Option injection in 'git checkout <tag>':
Validate Tag via 'git check-ref-format refs/tags/<tag>' (rejects
whitespace, control chars, '..', '@{', etc.). Also pass the tag as
'refs/tags/<tag>' to the checkout so even tags starting with '-' can't
be parsed as a git option.
3. Cosmetic: align the FetchChunkSize param spacing with the other params.
Smoke-tested 10 malicious-input cases (all rejected) plus the valid path
and empty-Tag path (still work). Local benchmark on 8 packages confirms
no regression: baseline 114.6s, cold 10.4s (~11x), warm 7.3s (~16x) -
the ~1s difference vs the previous run is the 8 'git check-ref-format'
calls.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…checkout + core.longpaths) (Azure#59370) * Fix Windows MAX_PATH failure in Restore-RecordingsShared.ps1 The shared-clone restore (Azure#59341) fails on Windows agents for packages whose recordings live at deep paths in Azure/azure-sdk-assets, because git's per-package clone+checkout hits MAX_PATH (260) on default Windows agents (LongPathsEnabled=0, no core.longpaths set). Apply Option 3 from the bug report: - Set core.longpaths=true on the shared bare clone (idempotent, applied to both freshly-initialized and reused clones). - For each per-package clone, pass -c core.longpaths=true on the clone and checkout, plus 'git config core.longpaths true' post-clone. - Configure cone-mode sparse-checkout to <AssetsRepoPrefixPath>/<DirectoryPath> before checkout so only the package's own subtree is materialized - matches test-proxy restore semantics and reduces disk/IO. - Drop the '-- .' pathspec from the final checkout so HEAD detaches to the tag cleanly with sparsity applied. Touches the README of Azure.ResourceManager.ServiceFabricManagedClusters (a canonical long-path package from the bug report) so CI exercises the deep-path restore on a Windows agent and we can verify the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Revert temporary README touch on ServiceFabricManagedClusters CI on the prior commit confirmed the Restore-RecordingsShared.ps1 fix worked on Windows agents for this deep-path package - all 5 Windows test jobs succeeded the 'Restore Recordings (shared clone)' step. Now removing the README touch so this PR only contains the actual fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Replaces the per-package sequential
test-proxy restoreloop ineng/pipelines/templates/steps/set-artifact-packages.ymlwith a single shared baregitclone that fans out to all packages via local hardlinks / object alternates.The work and CI measurements behind this change are documented in detail in the design note that drove it (CI runs through three pipeline generations across 20-, 50-, and 100-package test batches).
What changes
eng/scripts/Restore-RecordingsShared.ps1eng/pipelines/templates/steps/set-artifact-packages.yml— the per-artifactforeachblock becomes a single call to the new script.For each unique assets repo, the new script:
$(Agent.TempDirectory)/.assets-shared/<owner__repo>(git init --bare+git remote add origin https://github.com/<repo>.git).git fetchper chunk, using explicit+refs/tags/<tag>:refs/tags/<tag>refspecs (no--depth=1— shallow clones forcegit clone --localto fall back to upload-pack, defeating the optimization).--local --sharedreuses objects from the shared clone via alternates instead of re-running upload-pack against GitHub.The script validates the shared clone on startup (recreating if the remote URL is wrong or the directory is corrupt), pre-deletes any stale per-package
.assets, and fails loudly (throws + non-zero exit) on any failure — there is no silent fallback to producing potentially-wrong recordings. It also prepends-c safe.bareRepository=allso the script works on hosts wheresafe.bareRepository=explicitis set.Performance
CI: per-job Restore Recordings duration (from the design note)
≈ −89% mean / −84% max / −87% total at 100 packages.
Local apples-to-apples benchmark (8 packages, this change vs. spec PR)
Sequential
test-proxy restorevs. one call to the new script, both starting from a clean temp folder, against the same 8 real packages (Identity, KeyVault×4, AppConfiguration, Tables, ServiceBus):test-proxy restore)Correctness
Per the design note: equivalence was verified by SHA-256-hashing every file in every restored
.assetsdirectory across both strategies — 1885 / 1885 files matched. A full ib job run on the 100-package PR with the new restore produced test counts within 0.2% of the baseline (single transient NuGet error, not test-related).This PR's local benchmark also confirms
.assetsdirectories are populated with the expected layout (the assets-repo content at the specified tag).Why this is a clear win
Rollout