/azp run runtime

copilot's analysis:

● 🎉 All 21/21 batches PASSED!

  ┌─────────────────────┬────────────────────┬─────────────────┬─────────────────────┐
  │ Metric              │ This run           │ Previous run    │ Unbatched baseline  │
  ├─────────────────────┼────────────────────┼─────────────────┼─────────────────────┤
  │ Work items          │ 21                 │ 21              │ 172                 │
  ├─────────────────────┼────────────────────┼─────────────────┼─────────────────────┤
  │ Passed              │ 21/21              │ 19/21           │ —                   │
  ├─────────────────────┼────────────────────┼─────────────────┼─────────────────────┤
  │ Total machine time  │ 192m (3.2 hrs)     │ 183m (3.0 hrs)  │ 437m (7.3 hrs)      │
  ├─────────────────────┼────────────────────┼─────────────────┼─────────────────────┤
  │ Longest batch       │ Batch-5: 24m42s    │ Batch-8: 17m24s │ —                   │
  ├─────────────────────┼────────────────────┼─────────────────┼─────────────────────┤
  │ Wall-clock (approx) │ ~25m               │ ~25m            │ ~25m + queue spread │
  └─────────────────────┴────────────────────┴─────────────────┴─────────────────────┘

  Summary: All batches pass with the new timeout formula. Total machine time is 192 minutes — 56% savings vs unbatched (437m). Batch-5 is the longest at ~25m, well within the 30m minimum timeout. The previous
  run's 2 timeout failures are resolved.

The tests counts look good too:

● Here are the results across all 3 runs:

  ┌──────────────────┬────────────────────┬──────────────────┬─────────────────┐
  │                  │ Unbatched baseline │ Previous (19/21) │ Current (21/21) │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Work items       │ 172                │ 21               │ 21              │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Suites           │ 172                │ 169              │ 172             │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Tests run        │ 259,527            │ 253,647          │ 259,527         │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Passed           │ 256,963            │ 251,910          │ 256,963         │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Failed           │ 0                  │ 0                │ 0               │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Skipped          │ 2,564              │ 1,737            │ 2,564           │
  ├──────────────────┼────────────────────┼──────────────────┼─────────────────┤
  │ Machine time     │ 437m (7.3h)        │ 183m (3.0h)      │ 192m (3.2h)     │
  └──────────────────┴────────────────────┴──────────────────┴─────────────────┘

  Key takeaways:

   - Current run matches the unbatched baseline exactly: 259,527 tests run, 256,963 passed, 0 failed
   - Previous run was missing 3 suites (5,880 tests) due to 2 batch timeouts — now fully resolved
   - 56% machine time savings with zero test loss

🤖 Copilot Code Review — PR #126157

Holistic Assessment

Motivation: Reducing Helix queue pressure from ~172 to ~21 work items (88% reduction) is a well-justified infrastructure improvement. Helix per-work-item overhead (scheduling, payload download, artifact upload, queue slot acquisition) is real and documented. The claimed 56% total machine time savings (~245 minutes) is plausible and supported by the per-batch breakdown in the PR description.

Approach: Greedy bin-packing (Longest Processing Time) by file size is a well-known heuristic for makespan minimization and is appropriate here. The WasmBatchRunner.sh wrapper is a thin loop around each suite's existing RunTests.sh, keeping the coupling surface minimal. The HelixCommand.Replace('./RunTests.sh', ...) approach correctly preserves the env var setup and dev-certs chain from the original command. Gating behind WasmBatchLibraryTests (defaulting true only for CoreCLR+Chrome) limits blast radius, and the opt-out via /p:WasmBatchLibraryTests=false is a good escape hatch.

Summary: ⚠️ Needs Human Review. The implementation is structurally sound and addresses all feedback from previous review rounds (timeout calibration, HELIX_WORKITEM_UPLOAD_ROOT restoration, stale batch staging cleanup). No blocking correctness bugs were found by the primary reviewer. However, one sub-agent flagged a potential timeout regression for large solo batches that warrants human verification, and a few robustness improvements are recommended. A human reviewer should confirm the timeout behavior for isolated large suites (>50 MB) against actual CI data.

Detailed Findings

⚠️ Timeout for Large Solo Batches — Verify Against Actual CI Data

Files: ComputeBatchTimeout.cs, GroupWorkItems.cs

Large items (>50 MB) are isolated into solo batches with negative BatchIds. Because each solo batch contains exactly one suite, ComputeBatchTimeout computes Math.Max(30, 1 * 20) = 30 minutes. The pre-batching per-item timeout for browser CoreCLR is 90 minutes (sendtohelixhelp.proj:42).

If any of the large solo suites currently needs more than 30 minutes (e.g., System.Security.Cryptography.Tests runs ~17 min, but CI variability exists), this is a regression. The PR description's per-batch breakdown shows Batch--1 at 13m29s — well under 30 min — but a human should verify this holds under CI load variability.

Consider either: passing the original _workItemTimeout to ComputeBatchTimeout for count == 1 solo batches, or verifying the 30-minute minimum is adequate for the largest suites with margin.

(Flagged by Claude Sonnet 4.6)

✅ HelixCommand Substitution — Correct

HelixCommand for non-Windows browser tests is constructed as <prefix> && dotnet dev-certs https && ./RunTests.sh (from sendtohelixhelp.proj:207-223). Since IncludeHelixCorrelationPayload is false in sendtohelix-browser.targets:57, there is no --runtime-path suffix — so HelixCommand always ends with exactly ./RunTests.sh. The .Replace('./RunTests.sh', 'chmod +x WasmBatchRunner.sh && ./WasmBatchRunner.sh') correctly targets this literal string. The build-time <Error> guard (line 347) validates the replacement succeeded.

(Confirmed by both Claude Opus 4.6 and Claude Sonnet 4.6)

✅ Batch Runner Script — Well-Structured

WasmBatchRunner.sh correctly:

• Preserves ORIGINAL_UPLOAD_ROOT before the loop and restores it after (line 80)
• Isolates each suite's test results via per-suite HELIX_WORKITEM_UPLOAD_ROOT subdirectories
• Captures unzip exit codes and handles extraction failures gracefully
• Cleans up extracted suite directories after each run (rm -rf "$suiteDir") to manage disk pressure
• Runs all suites even if some fail, then reports aggregate results
• Exits 1 if any suite failed, 0 otherwise

The for zipFile in "$BATCH_DIR"/*.zip glob correctly matches only suite ZIPs (not WasmBatchRunner.sh itself).

(Confirmed by both reviewers)

✅ Bin-Packing Algorithm — Sound

GroupWorkItems.cs implements standard LPT greedy bin-packing: sort items by size descending, assign each to the batch with the smallest current total. Large items (>50 MB) are isolated into solo batches with negative IDs. The File.Exists check gracefully handles missing files (defaults to size 0). The defensive clamp on BatchSize and LargeThreshold prevents bad MSBuild property values from causing exceptions.

✅ Build Integration — Correct

The HelixTestTasksAssemblyPath in Directory.Build.props includes $(NetCoreAppToolCurrent) in the path, matching the default AppendTargetFrameworkToOutputPath=true behavior of the project. The pretest.proj conditionally builds it for TargetOS == 'browser'. The UsingTask declarations correctly reference the assembly path.

✅ Stale Staging Cleanup — Addressed

RemoveDir Directories="$(IntermediateOutputPath)helix-batches/" (line 329) cleans stale batch staging from previous runs before creating fresh directories. This prevents stale ZIPs from previous groupings from being repackaged.

💡 Add Existence Check for RunTests.sh

File: eng/testing/WasmBatchRunner.sh

If a suite ZIP is malformed or lacks RunTests.sh, the chmod +x RunTests.sh silently fails and ./RunTests.sh exits 127 with a cryptic shell error. Adding an explicit check would improve CI diagnostics:

if [[ ! -f "RunTests.sh" ]]; then
    echo "ERROR: RunTests.sh not found in $suiteDir after extraction"
    suiteExitCode=1
    # ... handle as failure
fi

This is a robustness improvement, not blocking — in practice all test suite ZIPs contain RunTests.sh.

(Flagged by Claude Sonnet 4.6)

💡 Consider Log.LogWarning for Missing ZIP Files

File: GroupWorkItems.cs

if (File.Exists(item.ItemSpec))
    size = new FileInfo(item.ItemSpec).Length;

A missing ZIP is silently treated as 0 bytes and packed into a batch. If a test archive wasn't built (e.g., due to a prior build failure), a Log.LogWarning when File.Exists returns false would make this visible at build time instead of at Helix runtime.

(Flagged by Claude Sonnet 4.6)

💡 Consider Explicit <Timeout> in HelixWorkItem

File: sendtohelix-browser.targets (lines 354-357)

<HelixWorkItem Include="@(_WasmTimedBatchItem)">
    <PayloadArchive>...</PayloadArchive>
    <Command>$(_WasmBatchHelixCommand)</Command>
    <!-- Timeout flows through from _WasmTimedBatchItem metadata -->
</HelixWorkItem>

The Timeout metadata flows through via MSBuild item metadata passthrough, which works correctly. However, every other HelixWorkItem in this file has an explicit <Timeout> child element. Adding <Timeout>%(Timeout)</Timeout> would make the intent explicit and match the surrounding style.

(Flagged by Claude Sonnet 4.6)

Generated by Code Review for issue #126157 · ◷

Lets get this in to relieve CI. I will do another batch of improvements in followup PR, where I would like to enable it on mono legs too.