This PR adds an end-to-end CI benchmarking + reporting pipeline for the internal profiler, shifting from historical baseline comparisons to paired base/PR measurements and publishing an advisory regression report comment on PRs.

Changes:

• Introduces benchmarks/profiler_ci.py to build base + candidate revisions (profiling-enabled) and run a fixed 20-workload suite in isolated processes, producing a bounded report.json.
• Adds benchmarks/profiler_report.py and a privileged pull_request_target workflow + script to validate ADO artifacts as data and update a SHA-bound PR comment.
• Updates the Azure DevOps PR validation pipeline to produce and publish paired profiler measurement artifacts across the targeted platform/SQL legs.

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (1)

benchmarks/profiler_ci.py:47

• TarFile.extractall(filter=...) was added in Python 3.12, but this repository supports Python 3.10+ and the README presents this as a local command. On Python 3.10/3.11 every paired comparison fails before building; use a compatible extraction path or explicitly require Python 3.12+.

            tar.extractall(path, filter="data")

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (2)

benchmarks/profiler_ci.py:48

• This runner is documented for the repository's supported Python >=3.10, but the filter= keyword for TarFile.extractall was added in Python 3.12. On Python 3.10/3.11 every base/candidate checkout fails with TypeError before any benchmark runs; gate this call by Python version or use a compatible safe-extraction helper.

            tar.extractall(path, filter="data")

benchmarks/profiler_report.py:32

• The report contract duplicates the workload names maintained by profiler_workloads.registry(). If a future change updates only one list, the worker will either generate a report that validate() rejects or fail to produce a scenario that validation requires; add a contract test or derive the authoritative case set from one shared registry.

CASES = (
    "connect",
    "select",
    "insert",
    "executemany",
    "fetchall",
    "fetchone",
    "fetchmany",
    "commit_rollback",
    "arrow",
    "insertmanyvalues",
    "fetchmany_100",
    "fetchmany_10000",
    "prepared_qmark",
    "prepared_named",
    "legacy_insertmany",
    "setinputsizes",
    "join_aggregation",
    "large_fetch",
    "fetch_1_2m",
    "cte",

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (3)

.github/workflows/pr-profiler-report.yml:26

• This privileged pull_request_target job runs with pull-requests: write, so the action implementation itself must be immutable; a mutable @v5 tag can change outside a reviewed base commit and execute different code. Existing workflows pin this action to a commit SHA (for example .github/workflows/lint-check.yml:36).

      - uses: actions/setup-python@v5

benchmarks/profiler_ci.py:26

• The deadline is smaller than the documented worst-case work. run() can perform 12 worker passes, each allowed up to WORKER_TIMEOUT (10 minutes), plus a 15-minute base build and preflight, which can require 136 minutes; local runs also build the candidate and need another 15 minutes. With the current 80-minute deadline, a slow but still permitted worker is killed before all five pairs complete, and the README's local command cannot reliably finish. Align the worker timeout/budget and the 90-minute pipeline step (or explicitly make local mode use a separate budget) so the advertised complete comparison is achievable.

BENCHMARK_TIMEOUT = 80 * 60
WORKER_TIMEOUT = 10 * 60

benchmarks/profiler_ci.py:259

• When --scenarios is supplied, the worker emits only that subset, but this unconditional complete status produces a report that profiler_report.validate() must reject because complete reports require every CASES entry. The documented local subset path therefore finishes with an invalid report; mark subset runs as incomplete (or add a validated scenario-list schema).

    report["status"] = "complete"

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (1)

.github/scripts/post_profiler_comment.py:177

• This loop treats every status == "completed" run as terminal, including result == "canceled". If ADO cancels that run and queues a newer run for the same PR head, the publisher immediately reads the canceled run's likely-missing artifacts and posts a final incomplete report instead of waiting for the replacement; the coverage poller already handles this replacement case. Continue polling/select a newer exact-head build when the selected run is canceled before finalizing.

        if build and build["status"] == "completed":
            break

• Files reviewed: 11/11 changed files
• Comments generated: 4
• Review effort level: Lite

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/pr-code-coverage.yml:140

• The polling loop accepts an artifact and breaks before checking the selected build's lifecycle. If a canceled ADO run has already published a coverage artifact, this path consumes that partial/stale artifact and never searches for the newer replacement that the later cancellation branch is intended to select. Check the build status before accepting the artifact, or discard the artifact when the run is canceled and continue replacement polling.

.github/scripts/extract_coverage_artifact.py:42

• The XML selector falls back to every .xml member when no filename contains coverage, so an archive containing only test-results XML is treated as a valid coverage report. This can feed unrelated data to diff-cover instead of rejecting the artifact; restrict candidates to the known unified-coverage/coverage.xml, coverage.xml, or coverage-named files.

        elif kind == "xml" and path.suffix.lower() == ".xml":
            name = path.name.lower()
            priority = (
                0
                if str(path).endswith("unified-coverage/coverage.xml")
                else (1 if name == "coverage.xml" else 2 if "coverage" in name else 3)
            )
            candidates.append((priority, member))

.github/scripts/post_profiler_comment.py:234

• A deeply nested but size-bounded report.json can make json.loads raise RecursionError. That exception is not handled here, so one malformed leg aborts the publisher instead of being recorded as an invalid artifact while valid legs are still rendered; include RecursionError in the per-leg rejection path.

        except (ValueError, KeyError, TypeError, URLError, zipfile.BadZipFile):
            # Invalid data is visibly incomplete, never converted to a success verdict.
            issues.append(leg + " (invalid artifact)")

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (5)

.github/scripts/extract_coverage_artifact.py:30

• The candidate filter does not exclude ZIP directory entries. A member such as Code Coverage Report/index.html/ or coverage.xml/ is normalized by PurePosixPath to the same basename/suffix as a file, so it can be selected and copied as an empty report; the workflow then treats selection as successful and fails later in parsing. Exclude member.is_dir() before adding candidates.

            or member.file_size > MAX_REPORT_BYTES

.github/workflows/pr-code-coverage.yml:180

• ADO_URL is copied verbatim from the replacement build's _links.web.href and is later interpolated into the PR comment's Markdown link. The length/newline checks do not constrain the host or Markdown syntax, so malformed or compromised API metadata can publish an arbitrary link; construct this URL from the already validated numeric BUILD_ID and the fixed ADO host (as the normal path does), or apply the same URL allowlist used by the publisher.

                  ADO_URL=$(jq -r '._links.web.href // empty' <<< "$REPLACEMENT")
                  if [[ -z "$ADO_URL" || ${#ADO_URL} -gt 500 || "$ADO_URL" == *$'\n'* || "$ADO_URL" == *$'\r'* ]]; then
                    echo "Invalid replacement Azure DevOps build URL"
                    exit 1

benchmarks/README.md:15

• This replaces the existing documentation for benchmarks/perf-benchmarking.py instead of adding the profiler section. The standalone script remains in the repository, so this change leaves it undocumented; retain its heading/description and introduce the profiler comparisons as the next section.

### 2. Profiler benchmark comparisons

Profiler benchmarks are engineering infrastructure, separate from these standalone
scripts and from the runtime profiler. See
[`eng/profiler_benchmarks/README.md`](../eng/profiler_benchmarks/README.md).
Their reviewer-facing output is the impact-first **PR Performance Report**.

eng/profiler_benchmarks/README.md:33

• This documentation contradicts the publisher contract: a completed ADO run with result == "failed" is still processed, and validated artifacts can produce a partial or complete advisory report (post_profiler_comment.py:229-253, report.py:294-320). Please describe failed runs as usable when their authenticated artifacts validate, rather than saying all failed results remain unavailable.

heads. Missing, malformed, canceled, or failed results remain unavailable.

eng/profiler_benchmarks/report.py:193

• Malformed reports with a missing required key are not normalized to the validator's ValueError contract: for example, omitting environment raises KeyError at this indexing operation. assess() happens to catch that, but the standalone report CLI calls validate() directly and emits an unhandled traceback; use guarded lookups (and the same treatment for the later scenarios, layer, and counter fields) so invalid data is rejected consistently.

            env = sample["environment"]
            if not isinstance(env, dict) or set(env) != {

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (2)

.github/scripts/post_profiler_comment.py:284

• The publisher only waits two minutes for the artifact listing after the ADO build becomes completed, then publishes an unavailable result. Artifact publication/listing is eventually consistent and can lag a completed build; this contradicts the documented 220-minute publication window and can discard valid paired results. Keep artifact polling within the existing overall deadline (while preserving the workflow's publication buffer) instead of starting a fixed 120-second deadline here.

    artifact_deadline = time.monotonic() + 120

eng/profiler_benchmarks/report.py:318

• A ZIP with a valid central directory but corrupt deflate data can raise zlib.error from archive.read(member). That exception is not in this validation catch list, so a malformed PR artifact can abort assess() before it publishes the final unavailable report and leave the sticky comment pending; normalize decompression errors as an invalid artifact here.

        except (
            KeyError,
            RecursionError,
            TypeError,
            ValueError,

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (3)

eng/pipelines/pr-validation-pipeline.yml:423

• The PR summary says the existing artifact names are preserved, but the pipeline replaces the established perf-baseline-* artifacts with profiler-* names (the analogous Windows/macOS/Linux changes are also present). That is a contract change for ADO consumers and reruns; either retain the existing names or update the description and all dependent contracts explicitly.

      artifact: 'profiler-Windows-$(sqlVersion)'

eng/pipelines/pr-validation-pipeline.yml:409

• This benchmark step is not PR-only: the pipeline is also triggered by pushes to main, and the condition below still runs it for every SQL matrix leg. Those main-branch artifacts cannot be selected by post_profiler_comment.py, which requires triggerInfo.pr.number and triggerInfo.pr.sourceSha, so each merge now incurs up to the 90-minute benchmark budget without producing a PR report. Please gate these expensive benchmark/publication steps to PR validation runs, or add an explicit in-repo consumer for main runs.

      python -m eng.profiler_benchmarks.controller --reuse-candidate --leg "Windows-$(sqlVersion)" --output profiler-results

eng/profiler_benchmarks/controller.py:81

• If the process exits between the timeout and taskkill, taskkill returns nonzero but process.poll() is no longer None, so this branch raises RuntimeError even though the tree is already gone. That race can turn a completed/clean timeout into a failed benchmark leg; treat an already-exited process as successfully terminated and only escalate when it is still alive.

        if result.returncode:
            if process.poll() is None:
                process.kill()
                process.wait()
            raise RuntimeError(f"Failed to terminate build process tree: {result.stdout.strip()}")

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (2)

.github/scripts/post_profiler_comment.py:233

• complete treats every completed build whose result is not exactly canceled as publishable. Because build_items does not require or validate result, a malformed/unsupported terminal response (for example a missing or unknown result) can proceed to provenance and artifact validation and potentially publish a verdict. Accept only the known terminal results (succeeded, partiallySucceeded, and failed) and finalize unsupported completed results as unavailable rather than treating them as complete.

            complete = (
                build is not None
                and build.get("status") == "completed"
                and build.get("result") != "canceled"
            )

eng/pipelines/pr-validation-pipeline.yml:65

• These matrix values are unconditional, so the pipeline's main-branch runs (the trigger: main at lines 3-7) now build the profiling extension and publish profiling-named binding artifacts even though only the benchmark/artifact steps below are gated to Build.Reason == 'PullRequest'. That changes the main test binary and adds profiler overhead, contrary to the stated requirement that main-branch builds retain their existing flow; gate the profiling build/check/artifact selection on PR runs, and apply the same gating to the macOS/Linux build assignments.

        profilerBuild: '1'
        profilerCheck: 'on'
        bindingArtifact: 'ddbc_bindings-profiling-SQL2022'

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (4)

.github/workflows/pr-code-coverage.yml:73

• The new head matching only selects the build for the event that started this run. Because this workflow has no per-PR concurrency cancellation or final current-head check, a long-running coverage run for head A can finish after head B is pushed and overwrite the sticky coverage comment for the PR with results from superseded code. Cancel older runs per PR or revalidate the current PR head immediately before posting.

            BUILD_INFO=$(jq -c --arg PR "$PR_NUMBER" --arg SHA "$PR_HEAD_SHA" --arg BRANCH "$PR_BRANCH" '
              [.value[]? | select(
                .definition.id == 2128 and .sourceBranch == $BRANCH and
                (.triggerInfo["pr.number"] | tostring) == $PR and
                .triggerInfo["pr.sourceSha"] == $SHA
              )] | .[0] // empty' <<< "$API_RESPONSE")

eng/pipelines/pr-validation-pipeline.yml:825

• The Ubuntu container is also always built with profiling enabled, so main runs receive the same native/test-flow change even though the benchmark step itself is PR-only. This contradicts the stated rollout constraint that main-branch builds retain the existing flow; gate PROFILER_BUILD=1 and the corresponding check on Build.Reason == 'PullRequest'.

      if [ "$(distroName)" = "Ubuntu" ]; then
        PROFILER_BUILD=1
        PROFILER_CHECK=on

eng/pipelines/pr-validation-pipeline.yml:64

• These profiling matrix values are unconditional, while this pipeline also triggers on main. Consequently main-branch runs now build profiling binaries, run the profiler check/tests, and publish differently named binding artifacts, rather than retaining the existing test flow described in the PR summary. Gate the profiling configuration (and related artifact naming) on Build.Reason == 'PullRequest' if main should remain on the normal build.

        profilerBuild: '1'
        profilerCheck: 'on'

eng/pipelines/pr-validation-pipeline.yml:563

• This hard-codes profiling for every macOS matrix run, including the top-level main trigger. That changes the native build and enables profiler-only tests on main even though the PR summary says main retains its existing test flow and only PR validation receives the benchmark changes. Make this build mode conditional on pull-request validation, or otherwise preserve the normal main-branch build.

      ( cd mssql_python/pybind && ENABLE_PROFILING=1 ./build.sh )

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Lite

Suppressed comments (1)

.github/workflows/pr-code-coverage.yml:366

• This XML step consumes the first URL from a filter that also matches the HTML Code Coverage Report artifact. If ADO lists that artifact first, copy_report(..., "xml") rejects it with No coverage xml even though the unified XML artifact is present, so the coverage job fails based on artifact ordering. Select an XML-specific artifact or try the matching URLs until the XML reader succeeds.

            if ! python .github/scripts/extract_coverage_artifact.py xml "$COVERAGE_XML_ARCHIVE" "$COVERAGE_XML"; then

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (2)

eng/profiler_benchmarks/report.py:178

• These regex checks assume the commit and suite-identity fields are strings. A malformed report such as head_commit: 1 makes re.fullmatch raise TypeError, which escapes validate() and produces an unhandled traceback in the standalone report CLI instead of the validator's normal ValueError rejection. Check the value types before applying the regex (or normalize this TypeError in validate).

    for key in ("head_commit", "source_commit", "base_commit"):
        if not re.fullmatch(r"[0-9a-f]{40}", report.get(key, "")):
            raise ValueError("Invalid commit identity")
    if not re.fullmatch(r"[0-9a-f]{64}", report.get("suite_hash", "")):
        raise ValueError("Invalid workload identity")

eng/profiler_benchmarks/report.py:319

• A valid ZIP member using an unsupported compression method raises NotImplementedError from archive.read (and truncated members can raise EOFError), but neither exception is handled here. Because profiler artifacts are treated as untrusted data, one malformed artifact can abort the publisher before it posts the unavailable/invalid-artifact result; normalize these zipfile read failures as invalid artifacts.

        except (
            KeyError,
            RecursionError,
            TypeError,
            ValueError,

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Lite