FIX: Publish PR Performance Reports from ready artifacts

[Gaurav Sharma (bewithgaurav)]

Work Item / Issue Reference

ADO Work Item: AB#44819

---

Summary

Stabilize PR Performance Reports around the reliable Unix signal and artifact readiness.

• Remove whole-suite source-tree equality checks while retaining exact PR number, head, ADO definition, repository, branch, build, merge topology, report provenance, bounded artifact, platform, SQL version, and measurement validation.
• Exclude noisy hosted Windows timing from routine PR profiling while retaining functional Windows validation.
• Report Unix SQL Server 2022/2025 slowdowns and improvements using the same 20% median change, 1 ms absolute change, and 80% pair-agreement requirements.
• Poll profiler artifacts while the aggregate ADO build is still running and publish immediately when both required Unix artifacts exist.
• After aggregate completion, allow a full two-minute artifact propagation grace before publishing a partial result.
• Finalize exact-head reports after merge and terminalize abandoned or superseded pending comments without overwriting completed reports.
• Bound repeated build/artifact service failures and canceled-build replacement polling.

This PR's own report may remain unavailable because the trusted publisher on main still expects the previous four-artifact and suite-hash contract. Subsequent PRs will use the Unix-only artifact model.

Validation

115 profiler CI contract tests passed. YAML parsing, Black, Flake8, and diff checks passed.

[github-actions]

PR Performance Report

Performance assessment pending.

Waiting for the matching performance run for head f9e7eb9d09f1dae23e4eb783ae91026c646e814d.

[Copilot]

🟡 Changes recommended

Four moderate issues remain in the publication and comment-state handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates PR performance-report publication to rely on ready profiler artifacts instead of whole-suite equality checks.

Changes:

• Adds artifact polling, propagation grace periods, and merge finalization.
• Handles abandoned/superseded PRs and bounded service failures.
• Updates reporting code, documentation, and contract tests.

File summaries

File	Summary
tests/test_036_profiler_ci.py	Updates tests for the revised publication behavior.
eng/profiler_benchmarks/report.py	Removes suite identity validation.
eng/profiler_benchmarks/README.md	Documents artifact-readiness publication.
eng/profiler_benchmarks/controller.py	Stops emitting suite hashes.
.github/scripts/post_profiler_comment.py	Implements polling and terminal states; requires fixes for comment race handling, usable download URL validation, merged-base derivation, and preserving existing final reports.

Review details

Suppressed comments (3)

.github/scripts/post_profiler_comment.py:124

• If the PR head or base changes after the comment list is read but before this revalidation, current_body() returns None and this branch exits without updating the already-found marker comment. That can leave the initial “Performance assessment pending” comment permanently pending when the superseding run is canceled or never publishes; the post-write path below already terminalizes this race. Reuse that conditional superseded-state update for the existing comment before returning here.

        message = current_body()
        if message is None:
            return

.github/scripts/post_profiler_comment.py:307

• Readiness is based only on artifact names. An ADO item can have a resource without a usable, non-empty downloadUrl; it then passes this check, the loop breaks even while the build is running, and assess immediately publishes a partial report instead of waiting for usable artifacts. Require each required item to have a non-empty download URL before declaring readiness.

            required = {"profiler-" + leg for leg in reporting.LEGS}
            # Artifact readiness is the report signal; unrelated matrix legs do
            # not need to finish before the four profiler legs are assessed.
            if required <= {item["name"] for item in artifacts}:

.github/scripts/post_profiler_comment.py:125

• When a closed, unmerged PR is observed, current_body() returns the terminal message without considering the existing comment body, so this branch can overwrite an already-published final report (or another terminal state) with “closed before assessment completed.” The requested terminalization is for pending comments; preserve an existing non-pending report and only replace the pending comment.

    if comment:
        message = current_body()
        if message is None:
            return
        github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": message})

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

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 100%

🎯 Overall Coverage 83%

📈 Total Lines Covered: 8639 out of 10295 📁 Project: mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 78.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.row.py: 83.4%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in comment preservation, grace-boundary polling, SHA validation, and stale-head handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

.github/scripts/post_profiler_comment.py:176

• If an existing completed report is updated while the PR is open, the PR can close before this verification and current_body() returns closed_message(), causing this branch to overwrite that completed report with an unavailable terminal state. Preserve completed comments here; only terminalize a comment that was pending before the write, while the supersession branch may separately handle a newly-created report whose head changed.

    if verified is not None and verified != message:
        github(f"issues/comments/{comment_id}", method="PATCH", data={"body": verified})
    elif verified is None:
        latest = github(f"issues/comments/{comment_id}")
        if isinstance(latest, dict) and latest.get("body") == message:

.github/scripts/post_profiler_comment.py:267

• The < loop condition exits exactly when the two-minute grace expires, so after the poll at roughly T+90 seconds there is no artifact request at T+120. Artifacts that propagate during that final interval are therefore ignored and a partial report is published even though all four artifacts were available before the grace ended; perform a final artifact poll at the grace boundary before deciding readiness.

    while time.monotonic() < (
        max(deadline, completed_at + ARTIFACT_GRACE_SECONDS)
        if completed_at is not None
        else deadline
    ):

.github/scripts/post_profiler_comment.py:281

• This validation only checks that head and base are dictionaries. If GitHub returns a malformed response with a missing or invalid head.sha, current_head becomes None (or a bad string) and this early return silently leaves the pending comment forever instead of retrying and reaching the bounded terminal fallback. Validate both SHA fields before treating a mismatch as a superseded revision.

            current_head = pr["head"].get("sha")
            current_base = pr["base"].get("sha")
            pr_base = current_base
            if current_head != head:
                return

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

[Copilot]

🔵 Needs a closer look

Unresolved publication lifecycle and artifact URL validation issues remain in the posting script.

Review details

Suppressed comments (3)

.github/scripts/post_profiler_comment.py:281

• When the PR head changes during the polling loop, this return leaves the pending comment created at line 254 in place indefinitely. The new superseded-terminalization logic only runs inside publish; invoke it here (with the old head) before returning so a stale run cannot leave an assessment pending comment behind.

                # Supersede the pending comment through the compare-and-update
                # path instead of leaving it posted for the stale head.

.github/scripts/post_profiler_comment.py:334

• A non-empty string is treated as a usable artifact URL here, but fetch() rejects URLs that are not HTTPS or outside the permitted artifact hosts. If ADO returns a placeholder or malformed URL for all four legs, this breaks while the build is still running and immediately publishes invalid legs instead of waiting for usable URLs.

            failures = 0
            required = {"profiler-" + leg for leg in reporting.LEGS}
            usable = {
                item["name"]
                for item in artifacts
                if isinstance(item["resource"].get("downloadUrl"), str)

.github/scripts/post_profiler_comment.py:161

• current_body() returns the caller-supplied body for an open PR, so message == closed_message() does not prove that the PR is still closed. A close/reopen race between the checks can therefore overwrite a pending comment with the closed terminal state, and the verification below repeats the same ambiguity; re-read and require the actual PR state to remain closed before this PATCH.

            if comment["body"] != pending_message(head):
                return
            latest = github(f"issues/comments/{comment['id']}")
            if not isinstance(latest, dict) or latest.get("body") != comment["body"]:
                return
        github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": message})
        comment_id = comment["id"]

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

[Copilot]

🔵 Needs a closer look

The comment lifecycle has an unresolved edge case that can leave a closed PR permanently marked “assessment pending.”

Review details

Suppressed comments (1)

.github/scripts/post_profiler_comment.py:159

• When a PR is closed after its head has advanced, the existing bot comment can still be the pending message for the previous head. This guard compares only with pending_message(head), so run()'s terminal unavailable() call returns without replacing that old pending comment, leaving a permanent "assessment pending" state. Match an unchanged pending body independent of the current head while retaining the compare-and-update re-read.

            if comment["body"] != pending_message(head):
                return
            latest = github(f"issues/comments/{comment['id']}")
            if not isinstance(latest, dict) or latest.get("body") != comment["body"]:
                return

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate findings affect artifact readiness, merge validation, pipeline setup, and the two-leg publication contract.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Reconcile the PR description with the two-leg model · New
• After merge, base.sha is tied to a mutable base branch ref. If another commit lands on main…

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate findings remain in report labeling and comment lifecycle/readiness handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 1

Open (4)

• Prevent overwriting completed profiler reports · New
• Reconcile the PR description with the two-leg model
• After merge, base.sha is tied to a mutable base branch ref. If another commit lands on main…
• Update stale comment about profiler legs · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved critical trust-boundary and comment-update risks remain, along with artifact-readiness concerns.

Review effort: Lite
Findings: 2

Open (2)

• Use conditional update to prevent comment overwrite races · New
• Restore authenticated workload identity for trusted performance reports · New

Resolved since last review (4)

• Prevent overwriting completed profiler reports
• Reconcile the PR description with the two-leg model
• After merge, base.sha is tied to a mutable base branch ref. If another commit lands on main…
• Update stale comment about profiler legs

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two moderate findings remain in comment overwrite protection and mixed-result highlighting.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Restore authenticated workload identity for trusted performance reports
• Use conditional update to prevent comment overwrite races

[Jahnvi Thakkar (jahnvi480)]

LGTM! Approving