From 51f20263ac3720135ee802032668fc2d73f40b97 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 11:45:01 -0700 Subject: [PATCH 1/7] fix: enforce trustworthy quality snapshots (Fixes #503) Pin comparisons to the exact PR base, fail invalid or regressed performance and coverage snapshots, and harden the E2E JSONRPC client against buffered-output loss and stderr backpressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/coverage-baseline.yml | 18 ++ .github/workflows/coverage.yml | 195 +++--------- .github/workflows/perf-baseline.yml | 39 ++- .github/workflows/perf-tests.yml | 395 ++++-------------------- crates/pet/tests/e2e_performance.rs | 178 ++++++++--- docs/QUALITY_SNAPSHOTS.md | 50 +++ scripts/quality_snapshot.py | 345 +++++++++++++++++++++ scripts/tests/test_quality_snapshot.py | 212 +++++++++++++ 8 files changed, 880 insertions(+), 552 deletions(-) create mode 100644 docs/QUALITY_SNAPSHOTS.md create mode 100644 scripts/quality_snapshot.py create mode 100644 scripts/tests/test_quality_snapshot.py diff --git a/.github/workflows/coverage-baseline.yml b/.github/workflows/coverage-baseline.yml index 2ca3b031..df758b71 100644 --- a/.github/workflows/coverage-baseline.yml +++ b/.github/workflows/coverage-baseline.yml @@ -4,6 +4,10 @@ on: push: branches: - main + - release* + - release/* + - release-* + workflow_dispatch: permissions: contents: read @@ -29,6 +33,10 @@ jobs: with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -175,6 +183,16 @@ jobs: RUST_LOG: trace shell: bash + - name: Validate Coverage Baseline + run: >- + python scripts/quality_snapshot.py coverage + --current lcov.info + --baseline lcov.info + --platform "${{ matrix.os }} baseline" + --report coverage-baseline-report.md + --summary "$GITHUB_STEP_SUMMARY" + shell: bash + - name: Upload Coverage Artifact uses: actions/upload-artifact@v4 with: diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index f8019a6d..d20c7dc8 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -23,41 +23,36 @@ jobs: include: - os: ubuntu-latest target: x86_64-unknown-linux-musl + platform: Linux + comment_header: coverage-linux - os: windows-latest target: x86_64-pc-windows-msvc + platform: Windows + comment_header: coverage-windows steps: - name: Checkout uses: actions/checkout@v4 - - name: Post Coverage Started Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') + - name: Post Coverage Started Comment uses: marocchino/sticky-pull-request-comment@v2 with: - header: coverage-linux + header: ${{ matrix.comment_header }} message: | - ## Test Coverage Report (Linux) + ## Test Coverage Report (${{ matrix.platform }}) :hourglass_flowing_sand: **Coverage analysis in progress...** - This comment will be updated with results when the analysis completes. - - - name: Post Coverage Started Comment (Windows) - if: startsWith(matrix.os, 'windows') - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: coverage-windows - message: | - ## Test Coverage Report (Windows) - - :hourglass_flowing_sand: **Coverage analysis in progress...** - - This comment will be updated with results when the analysis completes. + Comparing against exact base `${{ github.event.pull_request.base.sha }}`. - name: Set Python to PATH uses: actions/setup-python@v5 with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -198,6 +193,7 @@ jobs: shell: bash - name: Run Tests with Coverage + id: coverage run: cargo llvm-cov --features ci --lcov --output-path lcov.info -- --nocapture --test-threads=1 env: RUST_BACKTRACE: 1 @@ -205,160 +201,39 @@ jobs: shell: bash - name: Upload PR Coverage Artifact + if: always() uses: actions/upload-artifact@v4 with: name: coverage-pr-${{ matrix.os }} path: lcov.info + if-no-files-found: ignore - - name: Download Baseline Coverage + - name: Download Exact PR Base Coverage + if: always() uses: dawidd6/action-download-artifact@v6 - id: download-baseline - continue-on-error: true with: workflow: coverage-baseline.yml - branch: main + commit: ${{ github.event.pull_request.base.sha }} + workflow_conclusion: success name: coverage-baseline-${{ matrix.os }} path: baseline-coverage - - - name: Install lcov (Linux) - if: startsWith(matrix.os, 'ubuntu') - run: sudo apt-get update && sudo apt-get install -y lcov - - - name: Install lcov (Windows) - if: startsWith(matrix.os, 'windows') - run: choco install lcov -y + check_artifacts: true + search_artifacts: true + + - name: Compare Coverage Snapshot + if: always() + run: >- + python scripts/quality_snapshot.py coverage + --current lcov.info + --baseline baseline-coverage/lcov.info + --platform "${{ matrix.platform }}" + --report coverage-report.md + --summary "$GITHUB_STEP_SUMMARY" shell: bash - - name: Generate Coverage Report (Linux) - if: startsWith(matrix.os, 'ubuntu') - id: coverage-linux - run: | - # Extract PR coverage - PR_LINES=$(lcov --summary lcov.info 2>&1 | grep "lines" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - PR_FUNCTIONS=$(lcov --summary lcov.info 2>&1 | grep "functions" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - - # Extract baseline coverage (default to 0 if not available) - if [ -f baseline-coverage/lcov.info ]; then - BASELINE_LINES=$(lcov --summary baseline-coverage/lcov.info 2>&1 | grep "lines" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - BASELINE_FUNCTIONS=$(lcov --summary baseline-coverage/lcov.info 2>&1 | grep "functions" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - else - BASELINE_LINES="0" - BASELINE_FUNCTIONS="0" - fi - - # Calculate diff - LINE_DIFF=$(echo "$PR_LINES - $BASELINE_LINES" | bc) - FUNC_DIFF=$(echo "$PR_FUNCTIONS - $BASELINE_FUNCTIONS" | bc) - - # Determine delta indicator - if (( $(echo "$LINE_DIFF > 0" | bc -l) )); then - DELTA_INDICATOR=":white_check_mark:" - elif (( $(echo "$LINE_DIFF < 0" | bc -l) )); then - DELTA_INDICATOR=":x:" - else - DELTA_INDICATOR=":heavy_minus_sign:" - fi - - # Set outputs - echo "pr_lines=$PR_LINES" >> $GITHUB_OUTPUT - echo "baseline_lines=$BASELINE_LINES" >> $GITHUB_OUTPUT - echo "line_diff=$LINE_DIFF" >> $GITHUB_OUTPUT - echo "delta_indicator=$DELTA_INDICATOR" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Test Coverage Report (${{ matrix.os }})" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | Value |" >> $GITHUB_STEP_SUMMARY - echo "|--------|-------|" >> $GITHUB_STEP_SUMMARY - echo "| Current Coverage | ${PR_LINES}% |" >> $GITHUB_STEP_SUMMARY - echo "| Base Branch Coverage | ${BASELINE_LINES}% |" >> $GITHUB_STEP_SUMMARY - echo "| Delta | ${LINE_DIFF}% ${DELTA_INDICATOR} |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Generate Coverage Report (Windows) - if: startsWith(matrix.os, 'windows') - id: coverage-windows - run: | - # Extract PR coverage - $prContent = Get-Content -Path "lcov.info" -Raw - $prLinesFound = ($prContent | Select-String -Pattern "LF:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - $prLinesHit = ($prContent | Select-String -Pattern "LH:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - if ($prLinesFound -gt 0) { - $prPct = [math]::Round(($prLinesHit / $prLinesFound) * 100, 2) - } else { - $prPct = 0 - } - - # Extract baseline coverage (default to 0 if not available) - if (Test-Path "baseline-coverage/lcov.info") { - $baselineContent = Get-Content -Path "baseline-coverage/lcov.info" -Raw - $baselineLinesFound = ($baselineContent | Select-String -Pattern "LF:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - $baselineLinesHit = ($baselineContent | Select-String -Pattern "LH:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - if ($baselineLinesFound -gt 0) { - $baselinePct = [math]::Round(($baselineLinesHit / $baselineLinesFound) * 100, 2) - } else { - $baselinePct = 0 - } - } else { - $baselinePct = 0 - } - - $diff = [math]::Round($prPct - $baselinePct, 2) - - if ($diff -gt 0) { - $deltaIndicator = ":white_check_mark:" - } elseif ($diff -lt 0) { - $deltaIndicator = ":x:" - } else { - $deltaIndicator = ":heavy_minus_sign:" - } - - # Set outputs - echo "pr_lines=$prPct" >> $env:GITHUB_OUTPUT - echo "baseline_lines=$baselinePct" >> $env:GITHUB_OUTPUT - echo "line_diff=$diff" >> $env:GITHUB_OUTPUT - echo "delta_indicator=$deltaIndicator" >> $env:GITHUB_OUTPUT - - # Write step summary - echo "## Test Coverage Report (${{ matrix.os }})" >> $env:GITHUB_STEP_SUMMARY - echo "" >> $env:GITHUB_STEP_SUMMARY - echo "| Metric | Value |" >> $env:GITHUB_STEP_SUMMARY - echo "|--------|-------|" >> $env:GITHUB_STEP_SUMMARY - echo "| Current Coverage | ${prPct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Base Branch Coverage | ${baselinePct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Delta | ${diff}% ${deltaIndicator} |" >> $env:GITHUB_STEP_SUMMARY - shell: pwsh - - - name: Post Coverage Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') + - name: Post Coverage Comment + if: always() uses: marocchino/sticky-pull-request-comment@v2 with: - header: coverage-linux - message: | - ## Test Coverage Report (Linux) - - | Metric | Value | - |--------|-------| - | Current Coverage | ${{ steps.coverage-linux.outputs.pr_lines }}% | - | Base Branch Coverage | ${{ steps.coverage-linux.outputs.baseline_lines }}% | - | Delta | ${{ steps.coverage-linux.outputs.line_diff }}% ${{ steps.coverage-linux.outputs.delta_indicator }} | - - --- - ${{ steps.coverage-linux.outputs.line_diff > 0 && 'Coverage increased! Great work!' || (steps.coverage-linux.outputs.line_diff < 0 && 'Coverage decreased. Please add tests for new code.' || 'Coverage unchanged.') }} - - - name: Post Coverage Comment (Windows) - if: startsWith(matrix.os, 'windows') - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: coverage-windows - message: | - ## Test Coverage Report (Windows) - - | Metric | Value | - |--------|-------| - | Current Coverage | ${{ steps.coverage-windows.outputs.pr_lines }}% | - | Base Branch Coverage | ${{ steps.coverage-windows.outputs.baseline_lines }}% | - | Delta | ${{ steps.coverage-windows.outputs.line_diff }}% ${{ steps.coverage-windows.outputs.delta_indicator }} | - - --- - ${{ steps.coverage-windows.outputs.line_diff > 0 && 'Coverage increased! Great work!' || (steps.coverage-windows.outputs.line_diff < 0 && 'Coverage decreased. Please add tests for new code.' || 'Coverage unchanged.') }} + header: ${{ matrix.comment_header }} + path: coverage-report.md diff --git a/.github/workflows/perf-baseline.yml b/.github/workflows/perf-baseline.yml index efe5b405..ed5dfcbe 100644 --- a/.github/workflows/perf-baseline.yml +++ b/.github/workflows/perf-baseline.yml @@ -4,6 +4,10 @@ on: push: branches: - main + - release* + - release/* + - release-* + workflow_dispatch: permissions: contents: read @@ -31,6 +35,10 @@ jobs: with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -73,26 +81,33 @@ jobs: shell: bash - name: Run Performance Tests - continue-on-error: true - run: cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt + run: | + set -o pipefail + cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt env: RUST_BACKTRACE: 1 RUST_LOG: warn shell: bash - name: Extract Performance Metrics - id: metrics run: | - # Extract JSON metrics from test output - if grep -q "JSON metrics:" perf-output.txt; then - # Extract lines after "JSON metrics:" until the closing brace - sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json - echo "Metrics extracted:" - cat metrics.json - else - echo '{"server_startup_ms": 0, "full_refresh_ms": 0, "environments_count": 0}' > metrics.json - echo "No metrics found, created empty metrics" + if ! grep -q "JSON metrics:" perf-output.txt; then + echo "Performance baseline produced no JSON metrics" >&2 + exit 1 fi + sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json + python -m json.tool metrics.json > /dev/null + cat metrics.json + shell: bash + + - name: Validate Performance Baseline + run: >- + python scripts/quality_snapshot.py performance + --current metrics.json + --baseline metrics.json + --platform "${{ matrix.os }} baseline" + --report performance-baseline-report.md + --summary "$GITHUB_STEP_SUMMARY" shell: bash - name: Upload Performance Baseline Artifact diff --git a/.github/workflows/perf-tests.yml b/.github/workflows/perf-tests.yml index 89ea789e..9f5dda5a 100644 --- a/.github/workflows/perf-tests.yml +++ b/.github/workflows/perf-tests.yml @@ -24,49 +24,39 @@ jobs: include: - os: windows-latest target: x86_64-pc-windows-msvc + platform: Windows + comment_header: perf-windows - os: ubuntu-latest target: x86_64-unknown-linux-musl + platform: Linux + comment_header: perf-linux - os: macos-latest target: x86_64-apple-darwin + platform: macOS + comment_header: perf-macos steps: - name: Checkout uses: actions/checkout@v4 - - name: Post In-Progress Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') && github.event_name == 'pull_request' + - name: Post In-Progress Comment + if: github.event_name == 'pull_request' uses: marocchino/sticky-pull-request-comment@v2 with: - header: perf-linux + header: ${{ matrix.comment_header }} message: | - ## Performance Report (Linux) :hourglass_flowing_sand: + ## Performance Report (${{ matrix.platform }}) :hourglass_flowing_sand: - Running performance tests... Results will appear here when complete. - - - name: Post In-Progress Comment (Windows) - if: startsWith(matrix.os, 'windows') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-windows - message: | - ## Performance Report (Windows) :hourglass_flowing_sand: - - Running performance tests... Results will appear here when complete. - - - name: Post In-Progress Comment (macOS) - if: startsWith(matrix.os, 'macos') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-macos - message: | - ## Performance Report (macOS) :hourglass_flowing_sand: - - Running performance tests... Results will appear here when complete. + Running performance tests against baseline `${{ github.event.pull_request.base.sha }}`. - name: Set Python to PATH uses: actions/setup-python@v5 with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -109,340 +99,73 @@ jobs: shell: bash - name: Run Performance Tests - continue-on-error: true - run: cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt + id: benchmark + run: | + set -o pipefail + cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt env: RUST_BACKTRACE: 1 RUST_LOG: warn shell: bash - name: Extract Performance Metrics - id: metrics + if: steps.benchmark.outcome == 'success' run: | - # Extract JSON metrics from test output - if grep -q "JSON metrics:" perf-output.txt; then - # Extract lines after "JSON metrics:" until the closing brace - sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json - echo "Metrics extracted:" - cat metrics.json - else - echo '{"server_startup_ms": 0, "full_refresh_ms": 0, "environments_count": 0}' > metrics.json - echo "No metrics found, created empty metrics" + if ! grep -q "JSON metrics:" perf-output.txt; then + echo "Performance test produced no JSON metrics" >&2 + exit 1 fi + sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json + python -m json.tool metrics.json > /dev/null + cat metrics.json shell: bash - name: Upload PR Performance Results + if: always() uses: actions/upload-artifact@v4 with: name: perf-pr-${{ matrix.os }} path: metrics.json + if-no-files-found: ignore - - name: Download Baseline Performance + - name: Download Exact PR Base Performance + if: always() && github.event_name == 'pull_request' uses: dawidd6/action-download-artifact@v6 - id: download-baseline - continue-on-error: true with: workflow: perf-baseline.yml - branch: main + commit: ${{ github.event.pull_request.base.sha }} + workflow_conclusion: success name: perf-baseline-${{ matrix.os }} path: baseline-perf + check_artifacts: true + search_artifacts: true - - name: Generate Performance Report (Linux) - if: startsWith(matrix.os, 'ubuntu') - id: perf-linux - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - PR_STARTUP=$(jq -r '.server_startup_ms // 0' metrics.json) - PR_REFRESH=$(jq -r '.full_refresh_ms // 0' metrics.json) - PR_ENVS=$(jq -r '.environments_count // 0' metrics.json) - - # Extract P95 values from stats object (if available) - PR_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' metrics.json) - PR_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' metrics.json) - - # Extract baseline metrics (default to 0 if not available) - if [ -f baseline-perf/metrics.json ]; then - BASELINE_STARTUP=$(jq -r '.server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH=$(jq -r '.full_refresh_ms // 0' baseline-perf/metrics.json) - BASELINE_ENVS=$(jq -r '.environments_count // 0' baseline-perf/metrics.json) - BASELINE_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' baseline-perf/metrics.json) - else - BASELINE_STARTUP=0 - BASELINE_REFRESH=0 - BASELINE_ENVS=0 - BASELINE_STARTUP_P95=0 - BASELINE_REFRESH_P95=0 - fi - - # Calculate diff (positive means slowdown, negative means speedup) - STARTUP_DIFF=$(echo "$PR_STARTUP - $BASELINE_STARTUP" | bc) - REFRESH_DIFF=$(echo "$PR_REFRESH - $BASELINE_REFRESH" | bc) - - # Calculate percentage change - if [ "$BASELINE_STARTUP" != "0" ]; then - STARTUP_PCT=$(echo "scale=1; ($STARTUP_DIFF / $BASELINE_STARTUP) * 100" | bc) - else - STARTUP_PCT="N/A" - fi - - if [ "$BASELINE_REFRESH" != "0" ]; then - REFRESH_PCT=$(echo "scale=1; ($REFRESH_DIFF / $BASELINE_REFRESH) * 100" | bc) - else - REFRESH_PCT="N/A" - fi - - # Determine delta indicators (for perf, negative is good = faster) - if (( $(echo "$REFRESH_DIFF < -100" | bc -l) )); then - DELTA_INDICATOR=":rocket:" - elif (( $(echo "$REFRESH_DIFF < 0" | bc -l) )); then - DELTA_INDICATOR=":white_check_mark:" - elif (( $(echo "$REFRESH_DIFF > 500" | bc -l) )); then - DELTA_INDICATOR=":warning:" - elif (( $(echo "$REFRESH_DIFF > 100" | bc -l) )); then - DELTA_INDICATOR=":small_red_triangle:" - else - DELTA_INDICATOR=":heavy_minus_sign:" - fi - - # Set outputs - echo "pr_startup=$PR_STARTUP" >> $GITHUB_OUTPUT - echo "pr_refresh=$PR_REFRESH" >> $GITHUB_OUTPUT - echo "pr_startup_p95=$PR_STARTUP_P95" >> $GITHUB_OUTPUT - echo "pr_refresh_p95=$PR_REFRESH_P95" >> $GITHUB_OUTPUT - echo "baseline_startup=$BASELINE_STARTUP" >> $GITHUB_OUTPUT - echo "baseline_refresh=$BASELINE_REFRESH" >> $GITHUB_OUTPUT - echo "baseline_startup_p95=$BASELINE_STARTUP_P95" >> $GITHUB_OUTPUT - echo "baseline_refresh_p95=$BASELINE_REFRESH_P95" >> $GITHUB_OUTPUT - echo "startup_diff=$STARTUP_DIFF" >> $GITHUB_OUTPUT - echo "refresh_diff=$REFRESH_DIFF" >> $GITHUB_OUTPUT - echo "startup_pct=$STARTUP_PCT" >> $GITHUB_OUTPUT - echo "refresh_pct=$REFRESH_PCT" >> $GITHUB_OUTPUT - echo "delta_indicator=$DELTA_INDICATOR" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (Linux)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change |" >> $GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|--------|" >> $GITHUB_STEP_SUMMARY - echo "| Server Startup | ${PR_STARTUP}ms | ${PR_STARTUP_P95}ms | ${BASELINE_STARTUP}ms | ${STARTUP_DIFF}ms | ${STARTUP_PCT}% |" >> $GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${PR_REFRESH}ms | ${PR_REFRESH_P95}ms | ${BASELINE_REFRESH}ms | ${REFRESH_DIFF}ms | ${REFRESH_PCT}% ${DELTA_INDICATOR} |" >> $GITHUB_STEP_SUMMARY - echo "| Environments | ${PR_ENVS} | - | ${BASELINE_ENVS} | - | - |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Generate Performance Report (Windows) - if: startsWith(matrix.os, 'windows') - id: perf-windows - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - $prMetrics = Get-Content -Path "metrics.json" -Raw | ConvertFrom-Json - $prStartup = $prMetrics.server_startup_ms - $prRefresh = $prMetrics.full_refresh_ms - $prEnvs = $prMetrics.environments_count - - # Extract P95 values from stats object (if available) - $prStartupP95 = if ($prMetrics.stats.server_startup.p95) { $prMetrics.stats.server_startup.p95 } else { $prStartup } - $prRefreshP95 = if ($prMetrics.stats.full_refresh.p95) { $prMetrics.stats.full_refresh.p95 } else { $prRefresh } - - # Extract baseline metrics (default to 0 if not available) - if (Test-Path "baseline-perf/metrics.json") { - $baselineMetrics = Get-Content -Path "baseline-perf/metrics.json" -Raw | ConvertFrom-Json - $baselineStartup = $baselineMetrics.server_startup_ms - $baselineRefresh = $baselineMetrics.full_refresh_ms - $baselineEnvs = $baselineMetrics.environments_count - $baselineStartupP95 = if ($baselineMetrics.stats.server_startup.p95) { $baselineMetrics.stats.server_startup.p95 } else { $baselineStartup } - $baselineRefreshP95 = if ($baselineMetrics.stats.full_refresh.p95) { $baselineMetrics.stats.full_refresh.p95 } else { $baselineRefresh } - } else { - $baselineStartup = 0 - $baselineRefresh = 0 - $baselineEnvs = 0 - $baselineStartupP95 = 0 - $baselineRefreshP95 = 0 - } - - # Calculate diff - $startupDiff = $prStartup - $baselineStartup - $refreshDiff = $prRefresh - $baselineRefresh - - # Calculate percentage change - if ($baselineStartup -gt 0) { - $startupPct = [math]::Round(($startupDiff / $baselineStartup) * 100, 1) - } else { - $startupPct = "N/A" - } - - if ($baselineRefresh -gt 0) { - $refreshPct = [math]::Round(($refreshDiff / $baselineRefresh) * 100, 1) - } else { - $refreshPct = "N/A" - } - - # Determine delta indicator - if ($refreshDiff -lt -100) { - $deltaIndicator = ":rocket:" - } elseif ($refreshDiff -lt 0) { - $deltaIndicator = ":white_check_mark:" - } elseif ($refreshDiff -gt 500) { - $deltaIndicator = ":warning:" - } elseif ($refreshDiff -gt 100) { - $deltaIndicator = ":small_red_triangle:" - } else { - $deltaIndicator = ":heavy_minus_sign:" - } - - # Set outputs - echo "pr_startup=$prStartup" >> $env:GITHUB_OUTPUT - echo "pr_refresh=$prRefresh" >> $env:GITHUB_OUTPUT - echo "pr_startup_p95=$prStartupP95" >> $env:GITHUB_OUTPUT - echo "pr_refresh_p95=$prRefreshP95" >> $env:GITHUB_OUTPUT - echo "baseline_startup=$baselineStartup" >> $env:GITHUB_OUTPUT - echo "baseline_refresh=$baselineRefresh" >> $env:GITHUB_OUTPUT - echo "baseline_startup_p95=$baselineStartupP95" >> $env:GITHUB_OUTPUT - echo "baseline_refresh_p95=$baselineRefreshP95" >> $env:GITHUB_OUTPUT - echo "startup_diff=$startupDiff" >> $env:GITHUB_OUTPUT - echo "refresh_diff=$refreshDiff" >> $env:GITHUB_OUTPUT - echo "startup_pct=$startupPct" >> $env:GITHUB_OUTPUT - echo "refresh_pct=$refreshPct" >> $env:GITHUB_OUTPUT - echo "delta_indicator=$deltaIndicator" >> $env:GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (Windows)" >> $env:GITHUB_STEP_SUMMARY - echo "" >> $env:GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change |" >> $env:GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|--------|" >> $env:GITHUB_STEP_SUMMARY - echo "| Server Startup | ${prStartup}ms | ${prStartupP95}ms | ${baselineStartup}ms | ${startupDiff}ms | ${startupPct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${prRefresh}ms | ${prRefreshP95}ms | ${baselineRefresh}ms | ${refreshDiff}ms | ${refreshPct}% ${deltaIndicator} |" >> $env:GITHUB_STEP_SUMMARY - echo "| Environments | ${prEnvs} | - | ${baselineEnvs} | - | - |" >> $env:GITHUB_STEP_SUMMARY - shell: pwsh - - - name: Generate Performance Report (macOS) - if: startsWith(matrix.os, 'macos') - id: perf-macos - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - PR_STARTUP=$(jq -r '.server_startup_ms // 0' metrics.json) - PR_REFRESH=$(jq -r '.full_refresh_ms // 0' metrics.json) - PR_ENVS=$(jq -r '.environments_count // 0' metrics.json) - - # Extract P95 values from stats object (if available) - PR_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' metrics.json) - PR_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' metrics.json) - - # Extract baseline metrics (default to 0 if not available) - if [ -f baseline-perf/metrics.json ]; then - BASELINE_STARTUP=$(jq -r '.server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH=$(jq -r '.full_refresh_ms // 0' baseline-perf/metrics.json) - BASELINE_ENVS=$(jq -r '.environments_count // 0' baseline-perf/metrics.json) - BASELINE_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' baseline-perf/metrics.json) - else - BASELINE_STARTUP=0 - BASELINE_REFRESH=0 - BASELINE_ENVS=0 - BASELINE_STARTUP_P95=0 - BASELINE_REFRESH_P95=0 - fi - - # Calculate diff - STARTUP_DIFF=$((PR_STARTUP - BASELINE_STARTUP)) - REFRESH_DIFF=$((PR_REFRESH - BASELINE_REFRESH)) - - # Set outputs - echo "pr_startup=$PR_STARTUP" >> $GITHUB_OUTPUT - echo "pr_refresh=$PR_REFRESH" >> $GITHUB_OUTPUT - echo "pr_startup_p95=$PR_STARTUP_P95" >> $GITHUB_OUTPUT - echo "pr_refresh_p95=$PR_REFRESH_P95" >> $GITHUB_OUTPUT - echo "baseline_startup=$BASELINE_STARTUP" >> $GITHUB_OUTPUT - echo "baseline_refresh=$BASELINE_REFRESH" >> $GITHUB_OUTPUT - echo "baseline_startup_p95=$BASELINE_STARTUP_P95" >> $GITHUB_OUTPUT - echo "baseline_refresh_p95=$BASELINE_REFRESH_P95" >> $GITHUB_OUTPUT - echo "startup_diff=$STARTUP_DIFF" >> $GITHUB_OUTPUT - echo "refresh_diff=$REFRESH_DIFF" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (macOS)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta |" >> $GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|" >> $GITHUB_STEP_SUMMARY - echo "| Server Startup | ${PR_STARTUP}ms | ${PR_STARTUP_P95}ms | ${BASELINE_STARTUP}ms | ${STARTUP_DIFF}ms |" >> $GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${PR_REFRESH}ms | ${PR_REFRESH_P95}ms | ${BASELINE_REFRESH}ms | ${REFRESH_DIFF}ms |" >> $GITHUB_STEP_SUMMARY - echo "| Environments | ${PR_ENVS} | - | ${BASELINE_ENVS} | - |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Post Performance Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-linux - message: | - ## Performance Report (Linux) ${{ steps.perf-linux.outputs.delta_indicator }} - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change | - |--------|----------|----------|----------------|-------|--------| - | Server Startup | ${{ steps.perf-linux.outputs.pr_startup }}ms | ${{ steps.perf-linux.outputs.pr_startup_p95 }}ms | ${{ steps.perf-linux.outputs.baseline_startup }}ms | ${{ steps.perf-linux.outputs.startup_diff }}ms | ${{ steps.perf-linux.outputs.startup_pct }}% | - | Full Refresh | ${{ steps.perf-linux.outputs.pr_refresh }}ms | ${{ steps.perf-linux.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-linux.outputs.baseline_refresh }}ms | ${{ steps.perf-linux.outputs.refresh_diff }}ms | ${{ steps.perf-linux.outputs.refresh_pct }}% | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
- - - name: Post Performance Comment (Windows) - if: startsWith(matrix.os, 'windows') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 + - name: Download Main Performance for Manual Run + if: always() && github.event_name == 'workflow_dispatch' + uses: dawidd6/action-download-artifact@v6 with: - header: perf-windows - message: | - ## Performance Report (Windows) ${{ steps.perf-windows.outputs.delta_indicator }} - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change | - |--------|----------|----------|----------------|-------|--------| - | Server Startup | ${{ steps.perf-windows.outputs.pr_startup }}ms | ${{ steps.perf-windows.outputs.pr_startup_p95 }}ms | ${{ steps.perf-windows.outputs.baseline_startup }}ms | ${{ steps.perf-windows.outputs.startup_diff }}ms | ${{ steps.perf-windows.outputs.startup_pct }}% | - | Full Refresh | ${{ steps.perf-windows.outputs.pr_refresh }}ms | ${{ steps.perf-windows.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-windows.outputs.baseline_refresh }}ms | ${{ steps.perf-windows.outputs.refresh_diff }}ms | ${{ steps.perf-windows.outputs.refresh_pct }}% | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
+ workflow: perf-baseline.yml + branch: main + workflow_conclusion: success + name: perf-baseline-${{ matrix.os }} + path: baseline-perf + check_artifacts: true + search_artifacts: true + + - name: Compare Performance Snapshot + if: always() + run: >- + python scripts/quality_snapshot.py performance + --current metrics.json + --baseline baseline-perf/metrics.json + --platform "${{ matrix.platform }}" + --report performance-report.md + --summary "$GITHUB_STEP_SUMMARY" + shell: bash - - name: Post Performance Comment (macOS) - if: startsWith(matrix.os, 'macos') && github.event_name == 'pull_request' + - name: Post Performance Comment + if: always() && github.event_name == 'pull_request' uses: marocchino/sticky-pull-request-comment@v2 with: - header: perf-macos - message: | - ## Performance Report (macOS) - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | - |--------|----------|----------|----------------|-------| - | Server Startup | ${{ steps.perf-macos.outputs.pr_startup }}ms | ${{ steps.perf-macos.outputs.pr_startup_p95 }}ms | ${{ steps.perf-macos.outputs.baseline_startup }}ms | ${{ steps.perf-macos.outputs.startup_diff }}ms | - | Full Refresh | ${{ steps.perf-macos.outputs.pr_refresh }}ms | ${{ steps.perf-macos.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-macos.outputs.baseline_refresh }}ms | ${{ steps.perf-macos.outputs.refresh_diff }}ms | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
+ header: ${{ matrix.comment_header }} + path: performance-report.md diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index 4340c95f..38620078 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -8,13 +8,14 @@ use serde::Deserialize; use serde_json::{json, Value}; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::env; use std::io::{BufRead, BufReader, Read, Write}; use std::path::PathBuf; -use std::process::{Child, Command, Stdio}; +use std::process::{Child, ChildStdin, ChildStdout, Command, Stdio}; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; +use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; mod common; @@ -24,6 +25,7 @@ static REQUEST_ID: AtomicU32 = AtomicU32::new(1); /// Number of iterations for statistical tests const STAT_ITERATIONS: usize = 10; +const STDERR_TAIL_LINES: usize = 100; /// Statistical metrics with percentile calculations #[derive(Debug, Clone, Default)] @@ -248,6 +250,10 @@ impl SharedState { /// JSONRPC client for communicating with the pet server pub struct PetClient { process: Child, + stdin: ChildStdin, + stdout: BufReader, + stderr_tail: Arc>>, + stderr_handle: Option>, state: Arc, start_time: Instant, } @@ -266,16 +272,34 @@ impl PetClient { let start_time = Instant::now(); - let process = Command::new(&pet_exe) + let mut process = Command::new(&pet_exe) .arg("server") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() .map_err(|e| format!("Failed to spawn pet server: {}", e))?; + let stdin = process + .stdin + .take() + .expect("PET stdin must be piped by the command above"); + let stdout = process + .stdout + .take() + .expect("PET stdout must be piped by the command above"); + let stderr = process + .stderr + .take() + .expect("PET stderr must be piped by the command above"); + let stderr_tail = Arc::new(Mutex::new(VecDeque::with_capacity(STDERR_TAIL_LINES))); + let stderr_handle = spawn_stderr_reader(stderr, stderr_tail.clone()); Ok(Self { process, + stdin, + stdout: BufReader::new(stdout), + stderr_tail, + stderr_handle: Some(stderr_handle), state: Arc::new(SharedState::new()), start_time, }) @@ -299,11 +323,10 @@ impl PetClient { // Write request { - let stdin = self.process.stdin.as_mut().ok_or("Failed to get stdin")?; - stdin + self.stdin .write_all(message.as_bytes()) .map_err(|e| format!("Failed to write request: {}", e))?; - stdin + self.stdin .flush() .map_err(|e| format!("Failed to flush stdin: {}", e))?; } @@ -311,46 +334,17 @@ impl PetClient { // Clone state reference for use in the loop let state = self.state.clone(); - // Read response - handle notifications until we get our response - let stdout = self.process.stdout.as_mut().ok_or("Failed to get stdout")?; - let mut reader = BufReader::new(stdout); - + // Read response - handle notifications until we get our response. + // The reader lives for the process lifetime so read-ahead bytes are never discarded. loop { - // Read headers until empty line - let mut content_length: Option = None; - loop { - let mut header_line = String::new(); - reader - .read_line(&mut header_line) - .map_err(|e| format!("Failed to read header: {}", e))?; - - let trimmed = header_line.trim(); - if trimmed.is_empty() { - // End of headers - break; - } - - if let Some(len_str) = trimmed.strip_prefix("Content-Length: ") { - content_length = Some( - len_str - .parse() - .map_err(|e| format!("Failed to parse content length: {}", e))?, - ); + let value = read_jsonrpc_message(&mut self.stdout).map_err(|error| { + let stderr = self.stderr_output(); + if stderr.is_empty() { + error + } else { + format!("{error}; PET stderr tail:\n{stderr}") } - // Ignore Content-Type and other headers - } - - let content_length = content_length.ok_or("Missing Content-Length header")?; - - // Read body - let mut body = vec![0u8; content_length]; - reader - .read_exact(&mut body) - .map_err(|e| format!("Failed to read body: {}", e))?; - - let body_str = String::from_utf8_lossy(&body); - let value: Value = serde_json::from_str(&body_str) - .map_err(|e| format!("Failed to parse response: {}", e))?; + })?; // Check if this is a notification or our response if let Some(notif_method) = value.get("method").and_then(|m| m.as_str()) { @@ -374,6 +368,16 @@ impl PetClient { } } + fn stderr_output(&self) -> String { + self.stderr_tail + .lock() + .expect("PET stderr tail mutex poisoned") + .iter() + .cloned() + .collect::>() + .join("\n") + } + /// Configure the server pub fn configure(&mut self, config: Value) -> Result { let start = Instant::now(); @@ -433,6 +437,9 @@ impl Drop for PetClient { fn drop(&mut self) { let _ = self.process.kill(); let _ = self.process.wait(); + if let Some(stderr_handle) = self.stderr_handle.take() { + let _ = stderr_handle.join(); + } } } @@ -504,6 +511,89 @@ fn get_workspace_dir() -> PathBuf { }) } +fn read_jsonrpc_message(reader: &mut impl BufRead) -> Result { + let mut content_length = None; + loop { + let mut header_line = String::new(); + let bytes_read = reader + .read_line(&mut header_line) + .map_err(|error| format!("Failed to read header: {error}"))?; + if bytes_read == 0 { + return Err("PET stdout closed while reading a JSONRPC header".to_string()); + } + + let trimmed = header_line.trim(); + if trimmed.is_empty() { + break; + } + if let Some(length) = trimmed.strip_prefix("Content-Length: ") { + content_length = Some( + length + .parse::() + .map_err(|error| format!("Failed to parse content length: {error}"))?, + ); + } + } + + let content_length = content_length.ok_or("Missing Content-Length header")?; + let mut body = vec![0u8; content_length]; + reader + .read_exact(&mut body) + .map_err(|error| format!("Failed to read body: {error}"))?; + serde_json::from_slice(&body).map_err(|error| format!("Failed to parse response: {error}")) +} + +fn spawn_stderr_reader( + stderr: impl Read + Send + 'static, + stderr_tail: Arc>>, +) -> JoinHandle<()> { + thread::spawn(move || { + for line in BufReader::new(stderr).lines() { + let line = match line { + Ok(line) => line, + Err(error) => format!("Failed to read PET stderr: {error}"), + }; + let mut tail = stderr_tail.lock().expect("PET stderr tail mutex poisoned"); + if tail.len() == STDERR_TAIL_LINES { + tail.pop_front(); + } + tail.push_back(line); + } + }) +} + +#[test] +fn jsonrpc_reader_preserves_buffered_follow_up_message() { + let first = json!({"jsonrpc": "2.0", "id": 1, "result": {"value": 1}}); + let second = json!({"jsonrpc": "2.0", "id": 2, "result": {"value": 2}}); + let framed = [first.clone(), second.clone()] + .into_iter() + .map(|message| { + let body = serde_json::to_string(&message).unwrap(); + format!("Content-Length: {}\r\n\r\n{}", body.len(), body) + }) + .collect::(); + let mut reader = BufReader::new(std::io::Cursor::new(framed.into_bytes())); + + assert_eq!(read_jsonrpc_message(&mut reader).unwrap(), first); + assert_eq!(read_jsonrpc_message(&mut reader).unwrap(), second); +} + +#[test] +fn stderr_reader_drains_input_and_bounds_diagnostic_tail() { + let input = (0..STDERR_TAIL_LINES + 5) + .map(|index| format!("line {index}\n")) + .collect::(); + let tail = Arc::new(Mutex::new(VecDeque::new())); + let handle = spawn_stderr_reader(std::io::Cursor::new(input.into_bytes()), tail.clone()); + handle.join().unwrap(); + + let tail = tail.lock().unwrap(); + assert_eq!(tail.len(), STDERR_TAIL_LINES); + assert_eq!(tail.front().map(String::as_str), Some("line 5")); + assert_eq!(tail.back().map(String::as_str), Some("line 104")); +} + // ============================================================================ // Performance Tests // ============================================================================ diff --git a/docs/QUALITY_SNAPSHOTS.md b/docs/QUALITY_SNAPSHOTS.md new file mode 100644 index 00000000..51ac322e --- /dev/null +++ b/docs/QUALITY_SNAPSHOTS.md @@ -0,0 +1,50 @@ +# Quality snapshots + +PET uses pull-request snapshots to prevent performance and coverage drift. Each pull request is compared with artifacts produced for the exact pull-request base commit, not the latest moving `main` tip. + +## Performance gate + +The performance workflow runs 10 end-to-end JSON-RPC iterations on Linux, Windows, and macOS. A comparison is valid only when: + +- current and baseline metrics contain at least five samples for every required distribution; +- environment and manager counts match exactly; and +- the benchmark command and JSON extraction both succeed. + +A metric blocks when it exceeds both its absolute and relative budget: + +| Metric | Linux | Windows | macOS | +| --- | ---: | ---: | ---: | +| Server startup P50 | 5 ms / 100% | 10 ms / 50% | 100 ms / 50% | +| Server startup P95 | 50 ms / 200% | 50 ms / 100% | 10,000 ms / 100% | +| Full refresh P50 | 25 ms / 30% | 50 ms / 30% | 100 ms / 50% | +| Full refresh P95 | 1,000 ms / 100% | 5,000 ms / 100% | 5,000 ms / 25% | +| Time to first environment P50 | 20 ms / 100% | 25 ms / 50% | 150 ms / 50% | +| Time to first environment P95 | 250 ms / 100% | 500 ms / 100% | 10,000 ms / 100% | + +Each cell is `absolute / relative`. The budgets reflect observed GitHub-hosted runner variance from 11 consecutive main-branch baselines. Tighten them when a noisy path is fixed rather than normalizing a known regression into the baseline. + +The dual budget avoids failing on tiny percentage changes while still blocking material latency regressions. Tail metrics remain mandatory; a healthy median does not excuse a degraded P95. + +## Coverage gate + +Linux and Windows line and function coverage are compared with the exact base commit. A decrease greater than 0.01 percentage points blocks the pull request. Coverage artifacts and comments remain available for inspection even when the comparison fails. + +## Running locally + +```powershell +python -m unittest discover -s scripts/tests -p 'test_*.py' -v +python scripts/quality_snapshot.py performance --current metrics.json --baseline baseline.json --platform Windows --report report.md +python scripts/quality_snapshot.py coverage --current lcov.info --baseline baseline.info --platform Windows --report report.md +``` + +Run the E2E benchmark with: + +```powershell +cargo test --release --features ci-perf --test e2e_performance test_performance_summary -- --nocapture +``` + +The E2E client keeps one buffered stdout reader for the process lifetime and continuously drains a bounded stderr tail so protocol read-ahead and pipe backpressure cannot distort measurements. + +## Known investigations + +The persistent macOS cold-refresh tail is tracked by issue #504. Existing tail latency is represented in the baseline, but any further regression is still gated. diff --git a/scripts/quality_snapshot.py b/scripts/quality_snapshot.py new file mode 100644 index 00000000..e934d7d4 --- /dev/null +++ b/scripts/quality_snapshot.py @@ -0,0 +1,345 @@ +#!/usr/bin/env python3 +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. +"""Validate and compare PET performance and coverage snapshots.""" + +from __future__ import annotations + +import argparse +import json +import math +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Sequence + + +class SnapshotError(ValueError): + """Raised when snapshot data is missing or malformed.""" + + +@dataclass(frozen=True) +class RegressionBudget: + absolute_ms: float + relative_percent: float + + +@dataclass(frozen=True) +class MetricSpec: + label: str + group: str + percentile: str + + +@dataclass(frozen=True) +class MetricComparison: + label: str + current: float + baseline: float + budget: RegressionBudget + + @property + def delta(self) -> float: + return self.current - self.baseline + + @property + def percent_change(self) -> float: + if self.baseline == 0: + return math.inf if self.current > 0 else 0.0 + return self.delta / self.baseline * 100 + + @property + def regressed(self) -> bool: + return self.delta > self.budget.absolute_ms and self.percent_change > self.budget.relative_percent + + +PERFORMANCE_METRICS = ( + MetricSpec('Server startup P50', 'server_startup', 'p50'), + MetricSpec('Server startup P95', 'server_startup', 'p95'), + MetricSpec('Full refresh P50', 'full_refresh', 'p50'), + MetricSpec('Full refresh P95', 'full_refresh', 'p95'), + MetricSpec('Time to first environment P50', 'time_to_first_env', 'p50'), + MetricSpec('Time to first environment P95', 'time_to_first_env', 'p95'), +) +PERFORMANCE_BUDGETS = { + 'linux': ( + RegressionBudget(5, 100), + RegressionBudget(50, 200), + RegressionBudget(25, 30), + RegressionBudget(1_000, 100), + RegressionBudget(20, 100), + RegressionBudget(250, 100), + ), + 'windows': ( + RegressionBudget(10, 50), + RegressionBudget(50, 100), + RegressionBudget(50, 30), + RegressionBudget(5_000, 100), + RegressionBudget(25, 50), + RegressionBudget(500, 100), + ), + 'macos': ( + RegressionBudget(100, 50), + RegressionBudget(10_000, 100), + RegressionBudget(100, 50), + RegressionBudget(5_000, 25), + RegressionBudget(150, 50), + RegressionBudget(10_000, 100), + ), +} +COVERAGE_BUDGET_PERCENTAGE_POINTS = 0.01 + + +def platform_key(platform: str) -> str: + normalized = platform.casefold() + if 'windows' in normalized: + return 'windows' + if 'macos' in normalized: + return 'macos' + if 'linux' in normalized or 'ubuntu' in normalized: + return 'linux' + raise SnapshotError(f'Unsupported performance platform: {platform}') + + +def performance_specs(platform: str) -> list[tuple[MetricSpec, RegressionBudget]]: + return list(zip(PERFORMANCE_METRICS, PERFORMANCE_BUDGETS[platform_key(platform)], strict=True)) + + +def load_json(path: Path) -> dict[str, Any]: + try: + value = json.loads(path.read_text(encoding='utf-8')) + except FileNotFoundError as error: + raise SnapshotError(f'Snapshot file does not exist: {path}') from error + except json.JSONDecodeError as error: + raise SnapshotError(f'Snapshot file is not valid JSON: {path}: {error}') from error + if not isinstance(value, dict): + raise SnapshotError(f'Snapshot root must be an object: {path}') + return value + + +def require_number(value: Any, name: str, *, minimum: float = 0) -> float: + if isinstance(value, bool) or not isinstance(value, (int, float)) or not math.isfinite(value): + raise SnapshotError(f'{name} must be a finite number') + numeric = float(value) + if numeric < minimum: + raise SnapshotError(f'{name} must be at least {minimum}') + return numeric + + +def require_integer(value: Any, name: str, *, minimum: int = 0) -> int: + numeric = require_number(value, name, minimum=minimum) + if not numeric.is_integer(): + raise SnapshotError(f'{name} must be an integer') + return int(numeric) + + +def performance_value(snapshot: dict[str, Any], spec: MetricSpec, source: str) -> float: + stats = snapshot.get('stats') + if not isinstance(stats, dict): + raise SnapshotError(f'{source}.stats must be an object') + group = stats.get(spec.group) + if not isinstance(group, dict): + raise SnapshotError(f'{source}.stats.{spec.group} must be an object') + require_integer(group.get('count'), f'{source}.stats.{spec.group}.count', minimum=5) + return require_number(group.get(spec.percentile), f'{source}.stats.{spec.group}.{spec.percentile}') + + +def compare_performance( + current: dict[str, Any], baseline: dict[str, Any], platform: str +) -> tuple[list[MetricComparison], list[str]]: + current_envs = require_integer(current.get('environments_count'), 'current.environments_count', minimum=1) + baseline_envs = require_integer(baseline.get('environments_count'), 'baseline.environments_count', minimum=1) + current_managers = require_integer(current.get('managers_count'), 'current.managers_count') + baseline_managers = require_integer(baseline.get('managers_count'), 'baseline.managers_count') + + failures: list[str] = [] + if current_envs != baseline_envs: + failures.append(f'Environment inventory changed: current={current_envs}, baseline={baseline_envs}') + if current_managers != baseline_managers: + failures.append(f'Manager inventory changed: current={current_managers}, baseline={baseline_managers}') + + comparisons = [ + MetricComparison( + spec.label, + performance_value(current, spec, 'current'), + performance_value(baseline, spec, 'baseline'), + budget, + ) + for spec, budget in performance_specs(platform) + ] + failures.extend( + f'{comparison.label} regressed by {comparison.delta:.0f}ms ({comparison.percent_change:.1f}%)' + for comparison in comparisons + if comparison.regressed + ) + return comparisons, failures + + +def parse_lcov(path: Path) -> tuple[int, int, int, int]: + try: + lines = path.read_text(encoding='utf-8', errors='replace').splitlines() + except FileNotFoundError as error: + raise SnapshotError(f'Coverage file does not exist: {path}') from error + lines_found = lines_hit = functions_found = functions_hit = 0 + try: + for line in lines: + if line.startswith('LF:'): + lines_found += int(line[3:]) + elif line.startswith('LH:'): + lines_hit += int(line[3:]) + elif line.startswith('FNF:'): + functions_found += int(line[4:]) + elif line.startswith('FNH:'): + functions_hit += int(line[4:]) + except ValueError as error: + raise SnapshotError(f'Coverage file has a malformed summary count: {path}') from error + if lines_found == 0 or functions_found == 0: + raise SnapshotError(f'Coverage file has no line/function summary data: {path}') + if lines_hit > lines_found or functions_hit > functions_found: + raise SnapshotError(f'Coverage file has invalid hit totals: {path}') + return lines_hit, lines_found, functions_hit, functions_found + + +def coverage_percent(hit: int, found: int) -> float: + return hit / found * 100 + + +def compare_coverage(current: Path, baseline: Path) -> tuple[dict[str, float], list[str]]: + current_lh, current_lf, current_fnh, current_fnf = parse_lcov(current) + baseline_lh, baseline_lf, baseline_fnh, baseline_fnf = parse_lcov(baseline) + values = { + 'current_lines': coverage_percent(current_lh, current_lf), + 'baseline_lines': coverage_percent(baseline_lh, baseline_lf), + 'current_functions': coverage_percent(current_fnh, current_fnf), + 'baseline_functions': coverage_percent(baseline_fnh, baseline_fnf), + } + values['line_delta'] = values['current_lines'] - values['baseline_lines'] + values['function_delta'] = values['current_functions'] - values['baseline_functions'] + failures = [] + if values['line_delta'] < -COVERAGE_BUDGET_PERCENTAGE_POINTS: + failures.append(f"Line coverage decreased by {abs(values['line_delta']):.3f} percentage points") + if values['function_delta'] < -COVERAGE_BUDGET_PERCENTAGE_POINTS: + failures.append(f"Function coverage decreased by {abs(values['function_delta']):.3f} percentage points") + return values, failures + + +def status_icon(failed: bool, delta: float) -> str: + if failed: + return ':x:' + if delta < 0: + return ':white_check_mark:' + if delta > 0: + return ':small_red_triangle:' + return ':heavy_minus_sign:' + + +def performance_report( + platform: str, + comparisons: Sequence[MetricComparison], + failures: Sequence[str], + current: dict[str, Any], + baseline: dict[str, Any], +) -> str: + rows = [] + for comparison in comparisons: + rows.append( + f'| {comparison.label} | {comparison.current:.0f}ms | {comparison.baseline:.0f}ms | ' + f'{comparison.delta:+.0f}ms | {comparison.percent_change:+.1f}% | ' + f'>{comparison.budget.absolute_ms:.0f}ms and >{comparison.budget.relative_percent:.0f}% | ' + f"{status_icon(comparison.regressed, comparison.delta)} |" + ) + result = ':x: Regression detected' if failures else ':white_check_mark: Within regression budgets' + report = [ + f'## Performance Report ({platform})', + '', + f'**Result:** {result}', + '', + '| Metric | PR | Baseline | Delta | Change | Blocking budget | Status |', + '|--------|----|----------|-------|--------|-----------------|--------|', + *rows, + '', + '| Workload | PR | Baseline |', + '|----------|---:|---------:|', + f"| Environments | {current['environments_count']} | {baseline['environments_count']} |", + f"| Managers | {current['managers_count']} | {baseline['managers_count']} |", + ] + if failures: + report.extend(['', '### Blocking findings', *[f'- {failure}' for failure in failures]]) + report.extend([ + '', + '> A regression must exceed both the documented absolute and relative budget. ' + 'Environment and manager inventories must match exactly.', + ]) + return '\n'.join(report) + '\n' + + +def coverage_report(platform: str, values: dict[str, float], failures: Sequence[str]) -> str: + result = ':x: Regression detected' if failures else ':white_check_mark: Within regression budget' + report = [ + f'## Test Coverage Report ({platform})', + '', + f'**Result:** {result}', + '', + '| Metric | PR | Baseline | Delta |', + '|--------|----|----------|-------|', + f"| Lines | {values['current_lines']:.3f}% | {values['baseline_lines']:.3f}% | {values['line_delta']:+.3f}pp |", + f"| Functions | {values['current_functions']:.3f}% | {values['baseline_functions']:.3f}% | {values['function_delta']:+.3f}pp |", + ] + if failures: + report.extend(['', '### Blocking findings', *[f'- {failure}' for failure in failures]]) + report.extend(['', f'> Allowed numerical tolerance: {COVERAGE_BUDGET_PERCENTAGE_POINTS:.2f} percentage points.']) + return '\n'.join(report) + '\n' + + +def write_report(report: str, report_path: Path, summary_path: Path | None) -> None: + report_path.write_text(report, encoding='utf-8') + if summary_path is not None: + with summary_path.open('a', encoding='utf-8') as summary: + summary.write(report) + + +def run_performance(args: argparse.Namespace) -> int: + try: + current = load_json(args.current) + baseline = load_json(args.baseline) + comparisons, failures = compare_performance(current, baseline, args.platform) + report = performance_report(args.platform, comparisons, failures, current, baseline) + except SnapshotError as error: + failures = [str(error)] + report = f'## Performance Report ({args.platform})\n\n:x: **Invalid snapshot:** {error}\n' + write_report(report, args.report, args.summary) + return 1 if failures else 0 + + +def run_coverage(args: argparse.Namespace) -> int: + try: + values, failures = compare_coverage(args.current, args.baseline) + report = coverage_report(args.platform, values, failures) + except SnapshotError as error: + failures = [str(error)] + report = f'## Test Coverage Report ({args.platform})\n\n:x: **Invalid snapshot:** {error}\n' + write_report(report, args.report, args.summary) + return 1 if failures else 0 + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + subparsers = parser.add_subparsers(dest='command', required=True) + for command, handler in (('performance', run_performance), ('coverage', run_coverage)): + subparser = subparsers.add_parser(command) + subparser.add_argument('--current', type=Path, required=True) + subparser.add_argument('--baseline', type=Path, required=True) + subparser.add_argument('--platform', required=True) + subparser.add_argument('--report', type=Path, required=True) + subparser.add_argument('--summary', type=Path) + subparser.set_defaults(handler=handler) + return parser + + +def main() -> int: + args = build_parser().parse_args() + return args.handler(args) + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/scripts/tests/test_quality_snapshot.py b/scripts/tests/test_quality_snapshot.py new file mode 100644 index 00000000..4d5a63a9 --- /dev/null +++ b/scripts/tests/test_quality_snapshot.py @@ -0,0 +1,212 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import argparse +import json +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from quality_snapshot import ( # noqa: E402 + SnapshotError, + compare_coverage, + compare_performance, + load_json, + run_coverage, + run_performance, +) + + +def performance_snapshot( + *, refresh_p50=100, refresh_p95=500, startup_p50=10, startup_p95=20, + first_p50=15, first_p95=30, environments=5, managers=1 +): + return { + 'server_startup_ms': startup_p50, + 'full_refresh_ms': refresh_p50, + 'time_to_first_env_ms': first_p50, + 'environments_count': environments, + 'managers_count': managers, + 'stats': { + 'server_startup': {'count': 10, 'p50': startup_p50, 'p95': startup_p95}, + 'full_refresh': {'count': 10, 'p50': refresh_p50, 'p95': refresh_p95}, + 'time_to_first_env': {'count': 10, 'p50': first_p50, 'p95': first_p95}, + }, + } + + +def write_lcov(path, *, lines_hit, lines_found, functions_hit, functions_found): + path.write_text( + f'SF:example.rs\nLF:{lines_found}\nLH:{lines_hit}\n' + f'FNF:{functions_found}\nFNH:{functions_hit}\nend_of_record\n', + encoding='utf-8', + ) + + +class PerformanceSnapshotTests(unittest.TestCase): + def test_unchanged_snapshot_passes(self): + comparisons, failures = compare_performance(performance_snapshot(), performance_snapshot(), 'Windows') + self.assertEqual(len(comparisons), 6) + self.assertEqual(failures, []) + + def test_p50_regression_fails_when_both_budgets_are_exceeded(self): + current = performance_snapshot(refresh_p50=180) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=100), 'Windows') + self.assertTrue(any('Full refresh P50' in failure for failure in failures)) + + def test_p95_regression_fails_even_when_p50_is_unchanged(self): + current = performance_snapshot(refresh_p95=7_000) + _, failures = compare_performance(current, performance_snapshot(refresh_p95=500), 'Windows') + self.assertTrue(any('Full refresh P95' in failure for failure in failures)) + + def test_noise_inside_absolute_budget_passes(self): + current = performance_snapshot(refresh_p50=140) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=100), 'Windows') + self.assertEqual(failures, []) + + + def test_relative_budget_must_also_be_exceeded(self): + current = performance_snapshot(refresh_p50=1_060) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=1_000), 'Windows') + self.assertEqual(failures, []) + + def test_platform_specific_budget_changes_decision(self): + current = performance_snapshot(refresh_p50=140) + baseline = performance_snapshot(refresh_p50=100) + _, windows_failures = compare_performance(current, baseline, 'Windows') + _, linux_failures = compare_performance(current, baseline, 'Linux') + self.assertEqual(windows_failures, []) + self.assertTrue(any('Full refresh P50' in failure for failure in linux_failures)) + + def test_unknown_platform_is_invalid(self): + with self.assertRaises(SnapshotError): + compare_performance(performance_snapshot(), performance_snapshot(), 'unknown') + + def test_inventory_mismatch_fails(self): + current = performance_snapshot(environments=6, managers=2) + _, failures = compare_performance(current, performance_snapshot(), 'Windows') + self.assertTrue(any('Environment inventory changed' in failure for failure in failures)) + self.assertTrue(any('Manager inventory changed' in failure for failure in failures)) + + def test_missing_metric_is_invalid(self): + current = performance_snapshot() + del current['stats']['full_refresh']['p95'] + with self.assertRaises(SnapshotError): + compare_performance(current, performance_snapshot(), 'Windows') + + def test_too_few_samples_is_invalid(self): + current = performance_snapshot() + current['stats']['full_refresh']['count'] = 1 + with self.assertRaises(SnapshotError): + compare_performance(current, performance_snapshot(), 'Windows') + + +class CoverageSnapshotTests(unittest.TestCase): + def compare(self, current_values, baseline_values): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + write_lcov(current, **current_values) + write_lcov(baseline, **baseline_values) + return compare_coverage(current, baseline) + + def test_coverage_increase_passes(self): + _, failures = self.compare( + dict(lines_hit=91, lines_found=100, functions_hit=46, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertEqual(failures, []) + + def test_line_coverage_decrease_fails(self): + _, failures = self.compare( + dict(lines_hit=89, lines_found=100, functions_hit=45, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertTrue(any('Line coverage decreased' in failure for failure in failures)) + + def test_function_coverage_decrease_fails(self): + _, failures = self.compare( + dict(lines_hit=90, lines_found=100, functions_hit=44, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertTrue(any('Function coverage decreased' in failure for failure in failures)) + + def test_invalid_lcov_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + current.write_text('SF:example.rs\nend_of_record\n', encoding='utf-8') + write_lcov(baseline, lines_hit=1, lines_found=1, functions_hit=1, functions_found=1) + with self.assertRaises(SnapshotError): + compare_coverage(current, baseline) + + def test_malformed_lcov_count_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + current.write_text('SF:example.rs\nLF:not-a-number\nLH:1\nFNF:1\nFNH:1\n', encoding='utf-8') + write_lcov(baseline, lines_hit=1, lines_found=1, functions_hit=1, functions_found=1) + with self.assertRaises(SnapshotError): + compare_coverage(current, baseline) + + +class JsonSnapshotTests(unittest.TestCase): + def test_malformed_json_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / 'metrics.json' + path.write_text('{', encoding='utf-8') + with self.assertRaises(SnapshotError): + load_json(path) + + +class CommandTests(unittest.TestCase): + def test_invalid_performance_snapshot_returns_failure_and_writes_report(self): + with tempfile.TemporaryDirectory() as directory: + directory = Path(directory) + current = directory / 'current.json' + baseline = directory / 'baseline.json' + report = directory / 'report.md' + current.write_text('{', encoding='utf-8') + baseline.write_text(json.dumps(performance_snapshot()), encoding='utf-8') + + exit_code = run_performance( + argparse.Namespace( + current=current, + baseline=baseline, + platform='Windows', + report=report, + summary=None, + ) + ) + + self.assertEqual(exit_code, 1) + self.assertIn('Invalid snapshot', report.read_text(encoding='utf-8')) + + def test_coverage_regression_returns_failure_and_writes_report(self): + with tempfile.TemporaryDirectory() as directory: + directory = Path(directory) + current = directory / 'current.info' + baseline = directory / 'baseline.info' + report = directory / 'report.md' + write_lcov(current, lines_hit=89, lines_found=100, functions_hit=44, functions_found=50) + write_lcov(baseline, lines_hit=90, lines_found=100, functions_hit=45, functions_found=50) + + exit_code = run_coverage( + argparse.Namespace( + current=current, + baseline=baseline, + platform='test', + report=report, + summary=None, + ) + ) + + self.assertEqual(exit_code, 1) + self.assertIn('Blocking findings', report.read_text(encoding='utf-8')) + + +if __name__ == '__main__': + unittest.main() From e63cd2d3e836447099616e8757e6c01b7c07cd9d Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 12:08:04 -0700 Subject: [PATCH 2/7] perf: report refresh phase distributions (#504) Capture existing RefreshProgress notifications in the E2E benchmark and emit deterministic phase and locator percentile distributions so cold-tail latency can be attributed before product changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet/tests/e2e_performance.rs | 168 +++++++++++++++++++++++++++- 1 file changed, 164 insertions(+), 4 deletions(-) diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index 38620078..38ccd1f5 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -6,9 +6,12 @@ //! These tests spawn the pet server as a subprocess and communicate via JSONRPC //! to measure discovery performance from a client perspective. +use pet_core::telemetry::refresh_progress::{ + RefreshProgress, RefreshProgressPhase, RefreshProgressStatus, +}; use serde::Deserialize; use serde_json::{json, Value}; -use std::collections::{HashMap, VecDeque}; +use std::collections::{BTreeMap, HashMap, VecDeque}; use std::env; use std::io::{BufRead, BufReader, Read, Write}; use std::path::PathBuf; @@ -199,6 +202,7 @@ pub struct Manager { struct SharedState { environments: Mutex>, managers: Mutex>, + refresh_progress: Mutex>, first_env_time: Mutex>, } @@ -207,6 +211,7 @@ impl SharedState { Self { environments: Mutex::new(Vec::new()), managers: Mutex::new(Vec::new()), + refresh_progress: Mutex::new(Vec::new()), first_env_time: Mutex::new(None), } } @@ -231,9 +236,23 @@ impl SharedState { self.managers.lock().unwrap().push(mgr); } } - "log" | "telemetry" => { - // Ignore log and telemetry notifications + "telemetry" => { + if params.get("event").and_then(Value::as_str) == Some("RefreshProgress") { + if let Some(progress) = params + .get("data") + .and_then(|data| data.get("refreshProgress")) + .and_then(|value| { + serde_json::from_value::(value.clone()).ok() + }) + { + self.refresh_progress + .lock() + .expect("refresh progress mutex poisoned") + .push(progress); + } + } } + "log" => {} _ => { // Unknown notification } @@ -243,6 +262,10 @@ impl SharedState { fn clear(&self) { self.environments.lock().unwrap().clear(); self.managers.lock().unwrap().clear(); + self.refresh_progress + .lock() + .expect("refresh progress mutex poisoned") + .clear(); *self.first_env_time.lock().unwrap() = None; } } @@ -417,6 +440,14 @@ impl PetClient { self.state.managers.lock().unwrap().clone() } + fn get_refresh_progress(&self) -> Vec { + self.state + .refresh_progress + .lock() + .expect("refresh progress mutex poisoned") + .clone() + } + /// Get time from start to first environment pub fn time_to_first_env(&self) -> Option { self.state @@ -594,6 +625,102 @@ fn stderr_reader_drains_input_and_bounds_diagnostic_tail() { assert_eq!(tail.back().map(String::as_str), Some("line 104")); } +fn refresh_phase_name(phase: RefreshProgressPhase) -> &'static str { + match phase { + RefreshProgressPhase::Locators => "locators", + RefreshProgressPhase::Path => "path", + RefreshProgressPhase::GlobalVirtualEnvs => "globalVirtualEnvs", + RefreshProgressPhase::Workspaces => "workspaces", + } +} + +fn collect_refresh_progress( + progress: &[RefreshProgress], + phase_stats: &mut BTreeMap, + locator_stats: &mut BTreeMap, +) { + for event in progress + .iter() + .filter(|event| event.status == RefreshProgressStatus::Completed) + { + if let (Some(locator), Some(duration)) = (&event.locator_name, event.locator_elapsed_ms) { + locator_stats + .entry(locator.clone()) + .or_default() + .add(duration); + } else if let Some(duration) = event.phase_elapsed_ms { + phase_stats + .entry(refresh_phase_name(event.phase).to_string()) + .or_default() + .add(duration); + } + } +} + +fn statistics_json(statistics: &BTreeMap) -> BTreeMap { + statistics + .iter() + .map(|(name, metrics)| (name.clone(), metrics.to_json())) + .collect() +} + +#[test] +fn refresh_progress_notifications_are_collected() { + let state = SharedState::new(); + state.handle_notification( + "telemetry", + json!({ + "event": "RefreshProgress", + "data": { + "refreshProgress": { + "refreshId": 7, + "phase": "locators", + "status": "completed", + "elapsedMs": 25, + "locatorName": "Conda", + "locatorElapsedMs": 20 + } + } + }), + ); + + let progress = state.refresh_progress.lock().unwrap(); + assert_eq!(progress.len(), 1); + assert_eq!(progress[0].locator_name.as_deref(), Some("Conda")); + assert_eq!(progress[0].locator_elapsed_ms, Some(20)); +} + +#[test] +fn refresh_progress_aggregation_separates_phases_and_locators() { + let progress = vec![ + RefreshProgress { + refresh_id: 1, + phase: RefreshProgressPhase::Locators, + status: RefreshProgressStatus::Completed, + elapsed_ms: 30, + phase_elapsed_ms: Some(30), + locator_name: None, + locator_elapsed_ms: None, + }, + RefreshProgress { + refresh_id: 1, + phase: RefreshProgressPhase::Locators, + status: RefreshProgressStatus::Completed, + elapsed_ms: 25, + phase_elapsed_ms: None, + locator_name: Some("Conda".to_string()), + locator_elapsed_ms: Some(20), + }, + ]; + let mut phases = BTreeMap::new(); + let mut locators = BTreeMap::new(); + + collect_refresh_progress(&progress, &mut phases, &mut locators); + + assert_eq!(phases["locators"].samples, vec![30]); + assert_eq!(locators["Conda"].samples, vec![20]); +} + // ============================================================================ // Performance Tests // ============================================================================ @@ -1083,6 +1210,8 @@ fn test_performance_summary() { let mut startup_stats = StatisticalMetrics::new(); let mut refresh_stats = StatisticalMetrics::new(); let mut time_to_first_env_stats = StatisticalMetrics::new(); + let mut phase_stats = BTreeMap::new(); + let mut locator_stats = BTreeMap::new(); let mut env_count = 0usize; let mut manager_count = 0usize; @@ -1120,6 +1249,11 @@ fn test_performance_summary() { if let Some(ttfe) = client.time_to_first_env() { time_to_first_env_stats.add(ttfe.as_millis()); } + collect_refresh_progress( + &client.get_refresh_progress(), + &mut phase_stats, + &mut locator_stats, + ); println!( " Iteration {}: startup={}ms, refresh={}ms, envs={}", @@ -1130,6 +1264,21 @@ fn test_performance_summary() { ); } + for phase in ["locators", "path", "globalVirtualEnvs", "workspaces"] { + let count = phase_stats + .get(phase) + .map(StatisticalMetrics::count) + .unwrap_or_default(); + assert_eq!( + count, STAT_ITERATIONS, + "Expected one completed {phase} phase per refresh iteration" + ); + } + assert!( + !locator_stats.is_empty(), + "Expected per-locator timing in RefreshProgress telemetry" + ); + // Print statistical summary println!("\n----------------------------------------"); println!(" STATISTICS "); @@ -1139,10 +1288,19 @@ fn test_performance_summary() { if time_to_first_env_stats.count() > 0 { time_to_first_env_stats.print_summary("Time to first env"); } + for (phase, metrics) in &phase_stats { + metrics.print_summary(&format!("Phase {phase}")); + } + for (locator, metrics) in &locator_stats { + metrics.print_summary(&format!("Locator {locator}")); + } println!("Environments found: {}", env_count); println!("Managers found: {}", manager_count); println!("========================================\n"); + let phase_json = statistics_json(&phase_stats); + let locator_json = statistics_json(&locator_stats); + // Output as JSON for CI parsing // Includes both P50 values at top level (for backwards compatibility) and full stats let json_output = serde_json::to_string_pretty(&json!({ @@ -1155,7 +1313,9 @@ fn test_performance_summary() { "server_startup": startup_stats.to_json(), "full_refresh": refresh_stats.to_json(), "time_to_first_env": time_to_first_env_stats.to_json() - } + }, + "phases": phase_json, + "locators": locator_json })) .unwrap(); From f9485d08dfe3548236eb706668328c0bae73e27a Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 12:19:51 -0700 Subject: [PATCH 3/7] perf: count interpreter probe timeouts (#504) Classify existing timeout warnings into privacy-safe categories in the E2E benchmark so the macOS cold tail can be tied to exact fallback probe counts without exposing paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet/tests/e2e_performance.rs | 55 ++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index 38ccd1f5..b7f0e2cf 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -401,6 +401,15 @@ impl PetClient { .join("\n") } + fn interpreter_probe_timeout_labels(&self) -> Vec<&'static str> { + self.stderr_tail + .lock() + .expect("PET stderr tail mutex poisoned") + .iter() + .filter_map(|line| interpreter_probe_timeout_label(line)) + .collect() + } + /// Configure the server pub fn configure(&mut self, config: Value) -> Result { let start = Instant::now(); @@ -542,6 +551,39 @@ fn get_workspace_dir() -> PathBuf { }) } +fn interpreter_probe_timeout_label(line: &str) -> Option<&'static str> { + if !line.contains("Timed out after") || !line.contains("resolving Python via spawn") { + return None; + } + if line.contains("/usr/bin/python3") { + Some("usrBinPython3") + } else if line.contains("CommandLineTools") { + Some("commandLineTools") + } else if line.contains("hostedtoolcache") { + Some("hostedToolcache") + } else if line.contains("/Library/Frameworks/Python.framework") { + Some("pythonOrgFramework") + } else if line.contains("/usr/local/bin") { + Some("usrLocalBin") + } else { + Some("other") + } +} + +#[test] +fn interpreter_probe_timeouts_are_classified_without_exposing_paths() { + assert_eq!( + interpreter_probe_timeout_label( + r#"Timed out after 15s resolving Python via spawn for "/usr/bin/python3"; killing child."# + ), + Some("usrBinPython3") + ); + assert_eq!( + interpreter_probe_timeout_label("ordinary PET warning"), + None + ); +} + fn read_jsonrpc_message(reader: &mut impl BufRead) -> Result { let mut content_length = None; loop { @@ -1212,6 +1254,7 @@ fn test_performance_summary() { let mut time_to_first_env_stats = StatisticalMetrics::new(); let mut phase_stats = BTreeMap::new(); let mut locator_stats = BTreeMap::new(); + let mut probe_timeout_counts: BTreeMap = BTreeMap::new(); let mut env_count = 0usize; let mut manager_count = 0usize; @@ -1254,6 +1297,15 @@ fn test_performance_summary() { &mut phase_stats, &mut locator_stats, ); + let timeout_labels = client.interpreter_probe_timeout_labels(); + for label in &timeout_labels { + *probe_timeout_counts + .entry((*label).to_string()) + .or_default() += 1; + } + if !timeout_labels.is_empty() { + println!(" Interpreter probe timeouts: {timeout_labels:?}"); + } println!( " Iteration {}: startup={}ms, refresh={}ms, envs={}", @@ -1315,7 +1367,8 @@ fn test_performance_summary() { "time_to_first_env": time_to_first_env_stats.to_json() }, "phases": phase_json, - "locators": locator_json + "locators": locator_json, + "interpreter_probe_timeouts": probe_timeout_counts })) .unwrap(); From b923035fce7473da0adeb0657c5bacba045ca199 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 12:47:21 -0700 Subject: [PATCH 4/7] perf: resolve macOS Python shims without spawning (#504) Map Apple /usr/bin/python3 aliases through the active developer directory, remove duplicate Xcode and CommandLineTools probes, and skip unresolved shims before the spawn fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet-mac-commandlinetools/src/lib.rs | 34 ++-- crates/pet-mac-xcode/src/lib.rs | 33 ++-- crates/pet-python-utils/src/lib.rs | 1 + crates/pet-python-utils/src/macos.rs | 215 +++++++++++++++++++++ crates/pet/src/locators.rs | 53 ++++- 5 files changed, 289 insertions(+), 47 deletions(-) create mode 100644 crates/pet-python-utils/src/macos.rs diff --git a/crates/pet-mac-commandlinetools/src/lib.rs b/crates/pet-mac-commandlinetools/src/lib.rs index 12edd5de..83dc82c9 100644 --- a/crates/pet-mac-commandlinetools/src/lib.rs +++ b/crates/pet-mac-commandlinetools/src/lib.rs @@ -9,6 +9,9 @@ use pet_core::{ Locator, LocatorKind, }; use pet_fs::path::resolve_symlink; +use pet_python_utils::macos::{ + add_macos_system_python_alias, is_macos_system_python, resolve_macos_system_python_env, +}; use pet_python_utils::version; use pet_python_utils::{env::ResolvedPythonEnv, executable::find_executables}; use pet_virtualenv::is_virtualenv; @@ -107,6 +110,13 @@ impl Locator for MacCmdLineTools { if std::env::consts::OS != "macos" { return None; } + + let resolved_system_alias = if is_macos_system_python(&env.executable) { + Some(resolve_macos_system_python_env(env)?) + } else { + None + }; + let env = resolved_system_alias.as_ref().unwrap_or(env); // Assume we create a virtual env from a python install, // Then the exe in the virtual env bin will be a symlink to the homebrew python install. // Hence the first part of the condition will be true, but the second part will be false. @@ -165,29 +175,6 @@ impl Locator for MacCmdLineTools { let mut resolved_environments = vec![]; - // We know /usr/bin/python3 can end up pointing to this same Python exe as well - // Hence look for those symlinks as well. - // Unfortunately /usr/bin/python3 is not a real symlink - // Hence we must spawn and verify it points to the same Python exe. - for possible_exes in [PathBuf::from("/usr/bin/python3")] { - if !symlinks.contains(&possible_exes) { - if let Some(resolved_env) = ResolvedPythonEnv::from(&possible_exes) { - if symlinks.contains(&resolved_env.executable) { - resolved_environments.push(resolved_env.clone()); - - symlinks.push(possible_exes); - // Use the latest accurate information we have. - version = Some(resolved_env.version); - prefix = Some(resolved_env.prefix); - arch = if resolved_env.is64_bit { - Some(Architecture::X64) - } else { - Some(Architecture::X86) - }; - } - } - } - } // Similarly the final exe can be /Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9 // & we might have another file `python3` in that bin directory which would point to the same exe. // Lets get those as well. @@ -205,6 +192,7 @@ impl Locator for MacCmdLineTools { } } + add_macos_system_python_alias(&mut symlinks); symlinks.sort(); symlinks.dedup(); diff --git a/crates/pet-mac-xcode/src/lib.rs b/crates/pet-mac-xcode/src/lib.rs index 9ed76de8..e83ac0f3 100644 --- a/crates/pet-mac-xcode/src/lib.rs +++ b/crates/pet-mac-xcode/src/lib.rs @@ -9,6 +9,9 @@ use pet_core::{ Locator, LocatorKind, }; use pet_fs::path::resolve_symlink; +use pet_python_utils::macos::{ + add_macos_system_python_alias, is_macos_system_python, resolve_macos_system_python_env, +}; use pet_python_utils::version; use pet_python_utils::{env::ResolvedPythonEnv, executable::find_executables}; use pet_virtualenv::is_virtualenv; @@ -38,6 +41,13 @@ impl Locator for MacXCode { if std::env::consts::OS != "macos" { return None; } + + let resolved_system_alias = if is_macos_system_python(&env.executable) { + Some(resolve_macos_system_python_env(env)?) + } else { + None + }; + let env = resolved_system_alias.as_ref().unwrap_or(env); // Assume we create a virtual env from a python install, // Then the exe in the virtual env bin will be a symlink to the homebrew python install. // Hence the first part of the condition will be true, but the second part will be false. @@ -98,28 +108,6 @@ impl Locator for MacXCode { let mut resolved_environments = vec![]; - // We know /usr/bin/python3 can end up pointing to this same Python exe as well - // Hence look for those symlinks as well. - // Unfortunately /usr/bin/python3 is not a real symlink - // Hence we must spawn and verify it points to the same Python exe. - for possible_exes in [PathBuf::from("/usr/bin/python3")] { - if !symlinks.contains(&possible_exes) { - if let Some(resolved_env) = ResolvedPythonEnv::from(&possible_exes) { - if symlinks.contains(&resolved_env.executable) { - resolved_environments.push(resolved_env.clone()); - symlinks.push(possible_exes); - // Use the latest accurate information we have. - version = Some(resolved_env.version); - prefix = Some(resolved_env.prefix); - arch = if resolved_env.is64_bit { - Some(Architecture::X64) - } else { - Some(Architecture::X86) - }; - } - } - } - } // Similarly the final exe can be /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9 // & we might have another file `python3` in that bin directory which would point to the same exe. // Lets get those as well. @@ -137,6 +125,7 @@ impl Locator for MacXCode { } } + add_macos_system_python_alias(&mut symlinks); symlinks.sort(); symlinks.dedup(); diff --git a/crates/pet-python-utils/src/lib.rs b/crates/pet-python-utils/src/lib.rs index 9647595c..3383b948 100644 --- a/crates/pet-python-utils/src/lib.rs +++ b/crates/pet-python-utils/src/lib.rs @@ -7,5 +7,6 @@ pub mod env; pub mod executable; pub mod fs_cache; mod headers; +pub mod macos; pub mod platform_dirs; pub mod version; diff --git a/crates/pet-python-utils/src/macos.rs b/crates/pet-python-utils/src/macos.rs new file mode 100644 index 00000000..48b3b975 --- /dev/null +++ b/crates/pet-python-utils/src/macos.rs @@ -0,0 +1,215 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +use std::{ + env, + path::{Path, PathBuf}, +}; + +use pet_core::env::PythonEnv; +use pet_fs::path::{resolve_any_symlink, resolve_symlink}; + +const SYSTEM_PYTHON_DIR: &str = "/usr/bin"; +const XCODE_SELECT_LINK: &str = "/var/db/xcode_select_link"; +const DEFAULT_XCODE_DEVELOPER_DIR: &str = "/Applications/Xcode.app/Contents/Developer"; +const DEFAULT_COMMAND_LINE_TOOLS_DIR: &str = "/Library/Developer/CommandLineTools"; + +pub fn is_macos_system_python(executable: &Path) -> bool { + let mut components = executable.components(); + matches!(components.next(), Some(std::path::Component::RootDir)) + && matches!(components.next(), Some(std::path::Component::Normal(part)) if part == "usr") + && matches!(components.next(), Some(std::path::Component::Normal(part)) if part == "bin") + && matches!(components.next(), Some(std::path::Component::Normal(name)) if is_macos_python_name(name)) + && components.next().is_none() +} + +fn is_macos_python_name(name: &std::ffi::OsStr) -> bool { + let Some(name) = name.to_str() else { + return false; + }; + if name == "python3" { + return true; + } + let Some(minor) = name.strip_prefix("python3.") else { + return false; + }; + !minor.is_empty() && minor.bytes().all(|byte| byte.is_ascii_digit()) +} + +pub fn resolve_macos_system_python(executable: &Path) -> Option { + if std::env::consts::OS != "macos" || !is_macos_system_python(executable) { + return None; + } + let developer_dir = active_developer_dir()?; + selected_python_with(executable, &developer_dir, Path::is_file) +} + +pub fn resolve_macos_system_python_env(env: &PythonEnv) -> Option { + let executable = resolve_macos_system_python(&env.executable)?; + let mut resolved = PythonEnv::new(executable, env.prefix.clone(), env.version.clone()); + let mut aliases = env.symlinks.clone().unwrap_or_default(); + aliases.push(env.executable.clone()); + aliases.sort(); + aliases.dedup(); + resolved.symlinks = Some(aliases); + Some(resolved) +} + +pub fn add_macos_system_python_alias(symlinks: &mut Vec) { + let alias = PathBuf::from(SYSTEM_PYTHON_DIR).join("python3"); + let Some(selected) = resolve_macos_system_python(&alias) else { + return; + }; + let resolved = resolve_symlink(&selected).unwrap_or_else(|| selected.clone()); + add_alias_if_target_matches(symlinks, alias, &selected, &resolved); +} + +fn active_developer_dir() -> Option { + let environment = env::var_os("DEVELOPER_DIR").map(PathBuf::from); + let selected = resolve_any_symlink(&PathBuf::from(XCODE_SELECT_LINK)); + active_developer_dir_with(environment, selected, Path::is_dir) +} + +fn active_developer_dir_with( + environment: Option, + selected: Option, + is_dir: impl Fn(&Path) -> bool, +) -> Option { + environment + .into_iter() + .chain(selected) + .chain([ + PathBuf::from(DEFAULT_XCODE_DEVELOPER_DIR), + PathBuf::from(DEFAULT_COMMAND_LINE_TOOLS_DIR), + ]) + .map(normalize_developer_dir) + .find(|path| is_dir(path)) +} + +fn normalize_developer_dir(path: PathBuf) -> PathBuf { + if path.extension().is_some_and(|extension| extension == "app") { + path.join("Contents").join("Developer") + } else { + path + } +} + +fn selected_python_with( + alias: &Path, + developer_dir: &Path, + mut is_file: impl FnMut(&Path) -> bool, +) -> Option { + if !is_macos_system_python(alias) { + return None; + } + let candidate = developer_dir + .join("usr") + .join("bin") + .join(alias.file_name()?); + is_file(&candidate).then_some(candidate) +} + +fn add_alias_if_target_matches( + symlinks: &mut Vec, + alias: PathBuf, + selected: &Path, + resolved: &Path, +) { + if symlinks + .iter() + .any(|path| path == selected || path == resolved) + { + symlinks.push(alias); + symlinks.sort(); + symlinks.dedup(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn system_python_requires_a_python_name_directly_under_usr_bin() { + assert!(is_macos_system_python(Path::new("/usr/bin/python3"))); + assert!(is_macos_system_python(Path::new("/usr/bin/python3.12"))); + assert!(!is_macos_system_python(Path::new("/usr/bin/python"))); + assert!(!is_macos_system_python(Path::new("/usr/local/bin/python3"))); + assert!(!is_macos_system_python(Path::new( + "/usr/bin/python3-config" + ))); + } + + #[test] + fn developer_dir_prefers_environment_and_normalizes_app_bundle() { + let selected = active_developer_dir_with( + Some(PathBuf::from("/Applications/Xcode_16.app")), + Some(PathBuf::from(DEFAULT_COMMAND_LINE_TOOLS_DIR)), + |_| true, + ); + + assert_eq!( + selected, + Some(PathBuf::from( + "/Applications/Xcode_16.app/Contents/Developer" + )) + ); + } + + #[test] + fn developer_dir_falls_back_to_selected_link_then_standard_locations() { + let selected = active_developer_dir_with( + None, + Some(PathBuf::from( + "/Applications/Xcode_Beta.app/Contents/Developer", + )), + |_| true, + ); + assert_eq!( + selected, + Some(PathBuf::from( + "/Applications/Xcode_Beta.app/Contents/Developer" + )) + ); + + let fallback = active_developer_dir_with(None, None, |path| { + path == Path::new(DEFAULT_COMMAND_LINE_TOOLS_DIR) + }); + assert_eq!( + fallback, + Some(PathBuf::from(DEFAULT_COMMAND_LINE_TOOLS_DIR)) + ); + } + + #[test] + fn selected_python_maps_alias_without_spawning() { + let developer_dir = Path::new(DEFAULT_COMMAND_LINE_TOOLS_DIR); + let expected = developer_dir.join("usr/bin/python3"); + let mut file_checks = 0; + + let selected = selected_python_with(Path::new("/usr/bin/python3"), developer_dir, |path| { + file_checks += 1; + path == expected + }); + + assert_eq!(selected, Some(expected)); + assert_eq!(file_checks, 1); + } + + #[test] + fn alias_is_added_only_for_a_matching_selected_target() { + let selected = PathBuf::from("/Library/Developer/CommandLineTools/usr/bin/python3"); + let resolved = PathBuf::from( + "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9", + ); + let alias = PathBuf::from("/usr/bin/python3"); + let mut symlinks = vec![resolved.clone()]; + + add_alias_if_target_matches(&mut symlinks, alias.clone(), &selected, &resolved); + assert!(symlinks.contains(&alias)); + + let mut unrelated = vec![PathBuf::from("/opt/homebrew/bin/python3")]; + add_alias_if_target_matches(&mut unrelated, alias.clone(), &selected, &resolved); + assert!(!unrelated.contains(&alias)); + } +} diff --git a/crates/pet/src/locators.rs b/crates/pet/src/locators.rs index a0b84205..98e1b4bf 100644 --- a/crates/pet/src/locators.rs +++ b/crates/pet/src/locators.rs @@ -20,11 +20,12 @@ use pet_pixi::Pixi; use pet_poetry::Poetry; use pet_pyenv::PyEnv; use pet_python_utils::env::ResolvedPythonEnv; +use pet_python_utils::macos::is_macos_system_python; use pet_uv::Uv; use pet_venv::Venv; use pet_virtualenv::VirtualEnv; use pet_virtualenvwrapper::VirtualEnvWrapper; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use tracing::{info_span, instrument}; @@ -138,7 +139,9 @@ pub fn identify_python_environment_using_locators( // We try to get the interpreter info, hoping that the real exe returned might be identifiable. let _resolve_span = info_span!("resolve_python_env", executable = %executable.display()).entered(); - if let Some(resolved_env) = ResolvedPythonEnv::from(&executable) { + if let Some(resolved_env) = + resolve_interpreter_if_allowed(&executable, std::env::consts::OS, ResolvedPythonEnv::from) + { let env = resolved_env.to_python_env(); if let Some(env) = locators.iter().find_map(|loc| loc.try_from(&env)) { trace!("Env ({:?}) in Path resolved as {:?}", executable, env.kind); @@ -175,6 +178,22 @@ pub fn identify_python_environment_using_locators( None } +fn resolve_interpreter_if_allowed( + executable: &Path, + operating_system: &str, + resolve_interpreter: impl Fn(&Path) -> Option, +) -> Option { + if operating_system == "macos" && is_macos_system_python(executable) { + trace!( + "Skipping unresolved macOS system Python shim without spawning: {:?}", + executable + ); + None + } else { + resolve_interpreter(executable) + } +} + fn create_unknown_env( resolved_env: ResolvedPythonEnv, fallback_category: Option, @@ -243,3 +262,33 @@ fn find_symlinks(_executable: &PathBuf) -> Option> { // Lets wait and see if this is necessary. None } + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::Cell; + + #[test] + fn unresolved_macos_system_python_does_not_spawn() { + let calls = Cell::new(0); + let result = resolve_interpreter_if_allowed(Path::new("/usr/bin/python3"), "macos", |_| { + calls.set(calls.get() + 1); + None + }); + + assert!(result.is_none()); + assert_eq!(calls.get(), 0); + } + + #[test] + fn unresolved_non_macos_python_still_uses_fallback_resolver() { + let calls = Cell::new(0); + let result = resolve_interpreter_if_allowed(Path::new("/usr/bin/python3"), "linux", |_| { + calls.set(calls.get() + 1); + None + }); + + assert!(result.is_none()); + assert_eq!(calls.get(), 1); + } +} From 39d7b890714cb04aa9ce1843b8e8bcb6cc82dd9f Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 14:36:38 -0700 Subject: [PATCH 5/7] test: cover public macOS resolver guards (#504) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet-python-utils/src/macos.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/pet-python-utils/src/macos.rs b/crates/pet-python-utils/src/macos.rs index 48b3b975..507fdc33 100644 --- a/crates/pet-python-utils/src/macos.rs +++ b/crates/pet-python-utils/src/macos.rs @@ -140,6 +140,15 @@ mod tests { ))); } + #[test] + fn public_resolvers_reject_non_system_python() { + let executable = Path::new("/usr/local/bin/python3"); + assert!(resolve_macos_system_python(executable).is_none()); + + let env = PythonEnv::new(executable.to_path_buf(), None, None); + assert!(resolve_macos_system_python_env(&env).is_none()); + } + #[test] fn developer_dir_prefers_environment_and_normalizes_app_bundle() { let selected = active_developer_dir_with( From 4890d3979ea29d3017595e97e28322a7215a069f Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 14:57:35 -0700 Subject: [PATCH 6/7] test: isolate performance diagnostics from timing (#504) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet/tests/e2e_performance.rs | 147 ++++++++++++++++++++-------- docs/QUALITY_SNAPSHOTS.md | 3 +- 2 files changed, 109 insertions(+), 41 deletions(-) diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index b7f0e2cf..14609b08 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -14,7 +14,7 @@ use serde_json::{json, Value}; use std::collections::{BTreeMap, HashMap, VecDeque}; use std::env; use std::io::{BufRead, BufReader, Read, Write}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Child, ChildStdin, ChildStdout, Command, Stdio}; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; @@ -203,15 +203,17 @@ struct SharedState { environments: Mutex>, managers: Mutex>, refresh_progress: Mutex>, + capture_refresh_progress: bool, first_env_time: Mutex>, } impl SharedState { - fn new() -> Self { + fn new(capture_refresh_progress: bool) -> Self { Self { environments: Mutex::new(Vec::new()), managers: Mutex::new(Vec::new()), refresh_progress: Mutex::new(Vec::new()), + capture_refresh_progress, first_env_time: Mutex::new(None), } } @@ -236,7 +238,7 @@ impl SharedState { self.managers.lock().unwrap().push(mgr); } } - "telemetry" => { + "telemetry" if self.capture_refresh_progress => { if params.get("event").and_then(Value::as_str) == Some("RefreshProgress") { if let Some(progress) = params .get("data") @@ -284,6 +286,14 @@ pub struct PetClient { impl PetClient { /// Spawn the pet server and create a client pub fn spawn() -> Result { + Self::spawn_with_options(false) + } + + fn spawn_with_refresh_progress() -> Result { + Self::spawn_with_options(true) + } + + fn spawn_with_options(capture_refresh_progress: bool) -> Result { let pet_exe = get_pet_executable(); if !pet_exe.exists() { @@ -323,7 +333,7 @@ impl PetClient { stdout: BufReader::new(stdout), stderr_tail, stderr_handle: Some(stderr_handle), - state: Arc::new(SharedState::new()), + state: Arc::new(SharedState::new(capture_refresh_progress)), start_time, }) } @@ -706,26 +716,80 @@ fn statistics_json(statistics: &BTreeMap) -> BTreeMa .collect() } +fn record_interpreter_probe_timeouts( + client: &PetClient, + probe_timeout_counts: &mut BTreeMap, +) { + let timeout_labels = client.interpreter_probe_timeout_labels(); + for label in &timeout_labels { + *probe_timeout_counts + .entry((*label).to_string()) + .or_default() += 1; + } + if !timeout_labels.is_empty() { + println!(" Interpreter probe timeouts: {timeout_labels:?}"); + } +} + +fn collect_refresh_diagnostics( + workspace_dir: &Path, + cache_dir: &Path, + phase_stats: &mut BTreeMap, + locator_stats: &mut BTreeMap, + probe_timeout_counts: &mut BTreeMap, +) { + let diagnostic_cache_dir = cache_dir.join("refresh-progress"); + let _ = std::fs::remove_dir_all(&diagnostic_cache_dir); + std::fs::create_dir_all(&diagnostic_cache_dir) + .expect("Failed to create refresh diagnostic cache dir"); + + println!("\nCollecting untimed refresh diagnostics..."); + for iteration in 0..STAT_ITERATIONS { + let mut client = + PetClient::spawn_with_refresh_progress().expect("Failed to spawn diagnostic server"); + client + .configure(json!({ + "workspaceDirectories": [workspace_dir], + "cacheDirectory": diagnostic_cache_dir + })) + .expect("Failed to configure diagnostic server"); + let (result, _) = client + .refresh(None) + .expect("Failed to run diagnostic refresh"); + + collect_refresh_progress(&client.get_refresh_progress(), phase_stats, locator_stats); + record_interpreter_probe_timeouts(&client, probe_timeout_counts); + println!( + " Diagnostic iteration {}: refresh={}ms, envs={}", + iteration + 1, + result.duration, + client.get_environments().len() + ); + } +} + #[test] -fn refresh_progress_notifications_are_collected() { - let state = SharedState::new(); - state.handle_notification( - "telemetry", - json!({ - "event": "RefreshProgress", - "data": { - "refreshProgress": { - "refreshId": 7, - "phase": "locators", - "status": "completed", - "elapsedMs": 25, - "locatorName": "Conda", - "locatorElapsedMs": 20 - } +fn refresh_progress_notifications_are_collected_only_when_enabled() { + let notification = json!({ + "event": "RefreshProgress", + "data": { + "refreshProgress": { + "refreshId": 7, + "phase": "locators", + "status": "completed", + "elapsedMs": 25, + "locatorName": "Conda", + "locatorElapsedMs": 20 } - }), - ); + } + }); + + let disabled_state = SharedState::new(false); + disabled_state.handle_notification("telemetry", notification.clone()); + assert!(disabled_state.refresh_progress.lock().unwrap().is_empty()); + let state = SharedState::new(true); + state.handle_notification("telemetry", notification); let progress = state.refresh_progress.lock().unwrap(); assert_eq!(progress.len(), 1); assert_eq!(progress[0].locator_name.as_deref(), Some("Conda")); @@ -1255,8 +1319,7 @@ fn test_performance_summary() { let mut phase_stats = BTreeMap::new(); let mut locator_stats = BTreeMap::new(); let mut probe_timeout_counts: BTreeMap = BTreeMap::new(); - let mut env_count = 0usize; - let mut manager_count = 0usize; + let mut expected_inventory = None; let cache_dir = get_test_cache_dir(); let _ = std::fs::remove_dir_all(&cache_dir); @@ -1286,36 +1349,40 @@ fn test_performance_summary() { let (result, _) = client.refresh(None).expect("Failed to refresh"); refresh_stats.add(result.duration); - env_count = client.get_environments().len(); - manager_count = client.get_managers().len(); + let inventory = (client.get_environments().len(), client.get_managers().len()); + if let Some(expected) = expected_inventory { + assert_eq!( + inventory, expected, + "Environment and manager inventory changed after iteration 1" + ); + } else { + expected_inventory = Some(inventory); + } if let Some(ttfe) = client.time_to_first_env() { time_to_first_env_stats.add(ttfe.as_millis()); } - collect_refresh_progress( - &client.get_refresh_progress(), - &mut phase_stats, - &mut locator_stats, - ); - let timeout_labels = client.interpreter_probe_timeout_labels(); - for label in &timeout_labels { - *probe_timeout_counts - .entry((*label).to_string()) - .or_default() += 1; - } - if !timeout_labels.is_empty() { - println!(" Interpreter probe timeouts: {timeout_labels:?}"); - } + record_interpreter_probe_timeouts(&client, &mut probe_timeout_counts); println!( " Iteration {}: startup={}ms, refresh={}ms, envs={}", i + 1, startup_time, result.duration, - env_count + inventory.0 ); } + let (env_count, manager_count) = + expected_inventory.expect("Performance summary must run at least one iteration"); + collect_refresh_diagnostics( + &workspace_dir, + &cache_dir, + &mut phase_stats, + &mut locator_stats, + &mut probe_timeout_counts, + ); + for phase in ["locators", "path", "globalVirtualEnvs", "workspaces"] { let count = phase_stats .get(phase) diff --git a/docs/QUALITY_SNAPSHOTS.md b/docs/QUALITY_SNAPSHOTS.md index 60791236..79461ffa 100644 --- a/docs/QUALITY_SNAPSHOTS.md +++ b/docs/QUALITY_SNAPSHOTS.md @@ -46,7 +46,8 @@ cargo test --release --features ci-perf --test e2e_performance test_performance_ ``` The E2E client keeps one buffered stdout reader for the process lifetime and continuously drains a bounded stderr tail so protocol read-ahead and pipe backpressure cannot distort measurements. +Phase and locator telemetry is collected in separate, untimed refreshes so diagnostic processing cannot backpressure the timed JSON-RPC refreshes. ## Known investigations -The persistent macOS cold-refresh tail is tracked by issue #504. Existing tail latency is represented in the baseline, but any further regression is still gated. +The macOS cold-refresh tail is tracked by issue #504. Phase and locator distributions plus privacy-safe interpreter timeout counts verify that the tail does not recur. From c0f026eee1cc9413525cc05e6e3556e16b7f0f56 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 15:07:50 -0700 Subject: [PATCH 7/7] test: retain interpreter timeout diagnostics (#504) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet/tests/e2e_performance.rs | 62 ++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index 14609b08..7a6cf453 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -278,6 +278,7 @@ pub struct PetClient { stdin: ChildStdin, stdout: BufReader, stderr_tail: Arc>>, + interpreter_probe_timeouts: Arc>>, stderr_handle: Option>, state: Arc, start_time: Instant, @@ -325,13 +326,19 @@ impl PetClient { .take() .expect("PET stderr must be piped by the command above"); let stderr_tail = Arc::new(Mutex::new(VecDeque::with_capacity(STDERR_TAIL_LINES))); - let stderr_handle = spawn_stderr_reader(stderr, stderr_tail.clone()); + let interpreter_probe_timeouts = Arc::new(Mutex::new(BTreeMap::new())); + let stderr_handle = spawn_stderr_reader( + stderr, + stderr_tail.clone(), + interpreter_probe_timeouts.clone(), + ); Ok(Self { process, stdin, stdout: BufReader::new(stdout), stderr_tail, + interpreter_probe_timeouts, stderr_handle: Some(stderr_handle), state: Arc::new(SharedState::new(capture_refresh_progress)), start_time, @@ -411,13 +418,11 @@ impl PetClient { .join("\n") } - fn interpreter_probe_timeout_labels(&self) -> Vec<&'static str> { - self.stderr_tail + fn interpreter_probe_timeout_counts(&self) -> BTreeMap { + self.interpreter_probe_timeouts .lock() - .expect("PET stderr tail mutex poisoned") - .iter() - .filter_map(|line| interpreter_probe_timeout_label(line)) - .collect() + .expect("interpreter probe timeout mutex poisoned") + .clone() } /// Configure the server @@ -629,6 +634,7 @@ fn read_jsonrpc_message(reader: &mut impl BufRead) -> Result { fn spawn_stderr_reader( stderr: impl Read + Send + 'static, stderr_tail: Arc>>, + interpreter_probe_timeouts: Arc>>, ) -> JoinHandle<()> { thread::spawn(move || { for line in BufReader::new(stderr).lines() { @@ -636,6 +642,13 @@ fn spawn_stderr_reader( Ok(line) => line, Err(error) => format!("Failed to read PET stderr: {error}"), }; + if let Some(label) = interpreter_probe_timeout_label(&line) { + *interpreter_probe_timeouts + .lock() + .expect("interpreter probe timeout mutex poisoned") + .entry(label.to_string()) + .or_default() += 1; + } let mut tail = stderr_tail.lock().expect("PET stderr tail mutex poisoned"); if tail.len() == STDERR_TAIL_LINES { tail.pop_front(); @@ -664,17 +677,32 @@ fn jsonrpc_reader_preserves_buffered_follow_up_message() { #[test] fn stderr_reader_drains_input_and_bounds_diagnostic_tail() { - let input = (0..STDERR_TAIL_LINES + 5) - .map(|index| format!("line {index}\n")) - .collect::(); + let timeout_line = + r#"Timed out after 15s resolving Python via spawn for "/usr/bin/python3"; killing child."#; + let input = format!( + "{timeout_line}\n{}", + (0..STDERR_TAIL_LINES + 5) + .map(|index| format!("line {index}\n")) + .collect::() + ); let tail = Arc::new(Mutex::new(VecDeque::new())); - let handle = spawn_stderr_reader(std::io::Cursor::new(input.into_bytes()), tail.clone()); + let timeout_counts = Arc::new(Mutex::new(BTreeMap::new())); + let handle = spawn_stderr_reader( + std::io::Cursor::new(input.into_bytes()), + tail.clone(), + timeout_counts.clone(), + ); handle.join().unwrap(); let tail = tail.lock().unwrap(); assert_eq!(tail.len(), STDERR_TAIL_LINES); assert_eq!(tail.front().map(String::as_str), Some("line 5")); assert_eq!(tail.back().map(String::as_str), Some("line 104")); + drop(tail); + assert_eq!( + timeout_counts.lock().unwrap().get("usrBinPython3"), + Some(&1) + ); } fn refresh_phase_name(phase: RefreshProgressPhase) -> &'static str { @@ -720,14 +748,12 @@ fn record_interpreter_probe_timeouts( client: &PetClient, probe_timeout_counts: &mut BTreeMap, ) { - let timeout_labels = client.interpreter_probe_timeout_labels(); - for label in &timeout_labels { - *probe_timeout_counts - .entry((*label).to_string()) - .or_default() += 1; + let timeout_counts = client.interpreter_probe_timeout_counts(); + for (label, count) in &timeout_counts { + *probe_timeout_counts.entry(label.clone()).or_default() += count; } - if !timeout_labels.is_empty() { - println!(" Interpreter probe timeouts: {timeout_labels:?}"); + if !timeout_counts.is_empty() { + println!(" Interpreter probe timeouts: {timeout_counts:?}"); } }