FIX: Make SQL Server CI setup retry-safe

[Jahnvi Thakkar (jahnvi480)]

Work Item / Issue Reference

AB#47797

---

Summary

Replace duplicated Unix SQL-container setup with one shared, ownership-aware helper and one bounded SQL-only retry, including macOS SQL2025, Linux matrices, and coverage. Capture redacted failure evidence before removing the failed container, recreate only job-owned containers, bound readiness/commands/cleanup, and propagate cancellation and permanent failure.

Keep dependency installation, native builds, and pytest outside the retry boundary. Preserve macOS build/setup overlap, optional AzureSQL behavior, existing ARM build retry settings, amd64 SQL containers, and current database/image selections. Isolate final SQL cleanup from test-container cleanup and retain safe image/state/log evidence without publishing dumps or credentials.

The PR changes only the pipeline YAML and shared SQL setup helper. No test files are added or modified. Verification uses temporary development checks kept outside this PR, normal PR validation, and separate hosted Linux/macOS SQL2025 recovery and permanent-failure experiments. The local Windows x64/Python 3.13 native build and Black gate passed. Hosted results and review feedback are being assessed; the PR remains a draft.

This is recovery mitigation for observed SQL setup failures, not a fix for the underlying SQL-engine crash or a guarantee that hosted failures cannot recur.

[Copilot]

🔵 Needs a closer look

Three moderate issues remain in container removal timing, name matching, and log retrieval.

Pull request overview

Updates Unix SQL Server CI setup with a shared, ownership-aware helper and bounded retry/recovery behavior.

Changes:

• Adds bounded setup, cleanup, diagnostics, cancellation, and redaction handling.
• Replaces duplicated pipeline container logic.
• Adds helper and pipeline regression coverage.

File summaries

File	Summary
tests/test_sql_container_setup.py	Tests helper recovery, cleanup, deadlines, redaction, and signals.
tests/test_sql_container_pipeline.py	Verifies pipeline wiring and preserved CI behavior.
eng/scripts/setup_sql_container.py	Implements SQL container lifecycle, retries, diagnostics, and cleanup. Findings: share the removal/verification deadline correctly, escape the exact-name filter, and cap Docker log retrieval.
eng/pipelines/pr-validation-pipeline.yml	Integrates the helper across Unix CI jobs.

Review details

Suppressed comments (3)

eng/scripts/setup_sql_container.py:369

• This 30-second deadline is shared by remove()'s docker rm and its subsequent find_owned() verification (up to two more Docker commands). If removal takes more than roughly 15 seconds, verification starts after the deadline and turns a removable same-job stale container into a terminal setup failure. Give the whole remove-and-verify operation the remaining phase budget instead of a deadline that only covers the rm command.

                self.remove(stale, deadline=min(self.phase_deadline, time.monotonic() + 30))

eng/scripts/setup_sql_container.py:249

• The exact-name Docker filter interpolates self.args.name as a regular expression, but the argument validator allows .. For a valid name such as sqlserver.foo, the dot matches any character, so find_owned() can select a different container (or reject a foreign similarly named container) instead of the requested exact name. Escape the name before embedding it in the filter.

            f"name=^/{self.args.name}$",

eng/scripts/setup_sql_container.py:279

• docker logs is requested without --tail or --since, so a noisy or crash-looping SQL container can make the helper transfer its entire historical log before SafeCapture discards most of it. That can consume the 20-second diagnostics budget and delay recovery/removal; cap the Docker-side log retrieval (for example, to the existing 200-line convention) while still applying redaction.

            result = self.docker_command(
                "logs", container.identifier, timeout=20, check=False, deadline=deadline
            )

• Files reviewed: 4/4 changed files
• Comments generated: 0
• 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: 8220 out of 9874 📁 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: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%
mssql_python.pooling.py: 90.1%

---

🔗 Quick Links

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues remain in the shared SQL container helper.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

eng/scripts/setup_sql_container.py:181

• After the launcher exits, this treats a reader thread that is still alive after 200 ms as proof that a descendant owns the pipe. SafeCapture.read can still be draining a large, normal docker logs response in that window, so the command is misclassified as descendants holding output and becomes a non-retryable failure even when no descendant exists. Track EOF separately, or wait for the reader to finish before classifying descendants, rather than using this arbitrary liveness check.

                reader.join(timeout=max(0, min(0.2, (deadline - time.monotonic()) / 4)))
                if reader.is_alive():
                    descendant_output = True
                    reaped = self.stop(process, deadline)

eng/scripts/setup_sql_container.py:418

• A failed readiness probe is followed by a sleep that can consume the entire ready_deadline, after which the loop raises without probing again. SQL Server becoming ready during that final sleep is therefore reported as a timeout even though it became ready within the configured window; the previous pipeline explicitly performed a final probe. Reserve time for and perform a final bounded probe, or otherwise avoid sleeping past the last probe.

            time.sleep(max(0, min(2, ready_deadline - time.monotonic())))
        raise SetupFailure("SQL readiness deadline exhausted\n" + last_output)

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

[Copilot]

🟡 Changes recommended

SQL authentication can fail due to an empty password, and the unescaped container-name filter can target the wrong container.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

eng/scripts/setup_sql_container.py:430

• This adds a new retry/cleanup state machine with signal, timeout, ownership, and process-teardown branches, but no checked-in tests exercise this helper. The repository does unit-test comparable eng/scripts modules (for example tests/test_029_bundled_binary_audit.py and tests/test_030_pe_machine_assert.py), so please add focused fake-command tests for retry after readiness failure, owner mismatch, cancellation, and cleanup deadlines; otherwise regressions here are only detected by hosted Unix CI.

        for number in (1, 2):
            self.log(f"SQL setup attempt {number}/2")
            attempt_end = min(
                self.deadline,
                time.monotonic() + (900 if self.args.colima else 600),

eng/scripts/setup_sql_container.py:249

• The Docker name filter is a regular expression, but the validated name is interpolated without escaping. A valid name such as foo.bar therefore matches fooXbar; if that match has this owner, lookup and cleanup can operate on a different container. Escape the name before constructing the exact-match filter.

            f"name=^/{self.args.name}$",

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

[Copilot]

🟡 Changes recommended

Automated tests are needed for the helper’s failure branches before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

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

[Gaurav Sharma (bewithgaurav)]

this addresses the SQL-only retry scope, but the new startup handling breaks macOS before SQL setup begins. requesting changes to fix that path before merging.

[Copilot]

🟡 Changes recommended

A critical teardown issue, a container cleanup race, and missing focused tests remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

eng/scripts/setup_sql_container.py:169

• This adds an 825-line process/timeout/retry/ownership helper with no automated tests. The repository already tests standalone eng/scripts modules by file-path loading (for example, tests/test_029_bundled_binary_audit.py:21-50), so add focused mocked tests for cancellation/teardown, redaction, owner-safe cleanup, and retry/deadline branches; otherwise this safety-critical behavior is only exercised by hosted SQL experiments.

class Commands:
    def __init__(self, password):
        self.password = password

eng/scripts/setup_sql_container.py:751

• After a create/start command times out, creation_requested triggers cleanup, but cleanup treats one immediate lookup that finds no container as success. The module explicitly allows the Docker daemon to finish creating a container after its CLI is killed; if that happens after this lookup, the next attempt can collide with the same name and consume the second attempt without a fresh container. Wait/recheck for the owned container within the cleanup budget before retrying.

                if self.creation_requested:
                    try:
                        self.cleanup()

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

[Copilot]

🟡 Changes recommended

Unresolved lifecycle, cancellation, cleanup, capture, and test-coverage concerns block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

eng/scripts/setup_sql_container.py:734

• This new 825-line helper is now the single gate for every Unix SQL matrix, but the PR adds no tests for its retry, ownership, cancellation, or cleanup behavior. Please add isolated unit tests using a fake Commands implementation (the repository already loads and tests eng/scripts helpers by file path) covering unowned-name refusal, cleanup after a timed-out create, one retry only, and cancellation propagation; otherwise regressions here can leak a failed container or affect another job's container.

    def setup(self):
        self.prepare_runtime()
        if self.args.cleanup:
            self.cleanup(evidence=False)
            self.log("Owned SQL container cleanup complete (or already absent)")

eng/scripts/setup_sql_container.py:242

• The signal handlers installed by main() remain active while stop() performs these TERM/KILL waits. If SIGINT/SIGTERM arrives during this teardown, Cancelled can escape from process.wait() before the child and its output reader are finished; main() then starts Docker cleanup while the original command/group may still be alive. Defer cancellation until teardown completes, then re-raise it.

        try:
            process.wait(timeout=max(0, min(1, (deadline - time.monotonic()) / 2)))
        except subprocess.TimeoutExpired:
            pass
        signal_owned("KILL")

eng/scripts/setup_sql_container.py:205

• When process.poll() reports a reaped child and os.getpgid() raises ProcessLookupError, this branch falls through to os.killpg(process.pid, ...) below. The numeric group ID can be reused between the failed lookup and that signal, allowing timeout cleanup to target an unrelated process group; this contradicts the stated guarantee that a reaped PID is not targeted. Avoid signalling a stale group ID after the leader is reaped, or use an identity-safe descendant cleanup mechanism.

            reaped = process.poll() is not None
            try:
                group = os.getpgid(process.pid)
            except ProcessLookupError:
                # Descendants may still hold the original group and stdout.

eng/scripts/setup_sql_container.py:575

• When docker create times out, creation_requested is set before the call, but this cleanup path performs only one immediate lookup. The daemon can finish creating the job-owned container after that lookup, so the next attempt can race the still-in-flight name and fail with a conflict; with only two attempts, a recoverable timeout becomes a permanent failure. Poll for the owned container during the cleanup window before retrying.

        container = self.find_owned()

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

[Copilot]

🔵 Needs a closer look

Address the three moderate findings covering credential propagation, teardown safety, and focused helper tests.

Review details

Suppressed comments (3)

eng/scripts/setup_sql_container.py:207

• process.poll() can reap the child before this getpgid() check. If that raises ProcessLookupError, control falls through to os.killpg(process.pid, ...) below; after reaping, the PID may be reused, so timeout teardown can signal an unrelated process group. That contradicts the stated guarantee that a reaped PID is not targeted. Preserve and validate the original process-group identity before reaping, or otherwise skip the group signal once the child is reaped.

            reaped = process.poll() is not None
            try:
                group = os.getpgid(process.pid)
            except ProcessLookupError:
                # Descendants may still hold the original group and stdout.
                pass

eng/scripts/setup_sql_container.py:752

• This adds an 826-line retry, timeout, cancellation, redaction, ownership, and cleanup state machine without tests. The repository already unit-tests eng/scripts helpers by loading them from file paths (for example tests/test_029_bundled_binary_audit.py:25-45 and tests/test_030_pe_machine_assert.py:20-37), so please add focused mocked-command tests for the success/retry/cleanup and teardown-safety paths before relying on this in every SQL CI matrix.

    def setup(self):
        self.prepare_runtime()
        if self.args.cleanup:
            self.cleanup(evidence=False)
            self.log("Owned SQL container cleanup complete (or already absent)")
            return
        for number in (1, 2):
            self.creation_requested = False
            self.log(f"SQL setup attempt {number}/2")
            attempt_end = min(
                self.deadline,
                time.monotonic() + (900 if self.args.colima else 600),
            )
            self.phase_deadline = attempt_end - 100
            try:
                self.attempt()
            except SetupFailure as exc:
                self.log(f"Attempt {number}/2 failed: {exc}")
                self.phase_deadline = attempt_end
                if self.creation_requested:
                    try:
                        self.cleanup()

eng/scripts/setup_sql_container.py:630

• docker exec does not resolve a bare --env name from the host environment; it expects an entry in NAME=value form. This therefore runs sqlcmd without SQLCMDPASSWORD, so every readiness probe and database initialization fails with an authentication error. Pass the secret through a Docker-supported secure channel while preserving the helper's requirement that it not appear in argv.

        return self.docker_command(
            "exec",
            "--env",
            "SQLCMDPASSWORD",

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

[Copilot]

🔵 Needs a closer look

Three unresolved moderate findings affect safety-critical retry, cancellation, cleanup, and test coverage behavior.

Review details

Suppressed comments (3)

eng/scripts/setup_sql_container.py:735

• This new helper carries the safety-critical retry, ownership, signal-cancellation, redaction, and teardown behavior, but the PR adds no automated coverage for any of those paths. The repository already unit-tests eng/scripts helpers by loading them from file paths (for example, tests/test_035_conda_macho_assert.py:21-43), so please add focused mocked tests for retry cleanup, ownership conflicts, cancellation, and bounded command failures before relying on this in every Unix CI matrix.

    def setup(self):
        self.prepare_runtime()
        if self.args.cleanup:
            self.cleanup(evidence=False)
            self.log("Owned SQL container cleanup complete (or already absent)")

eng/scripts/setup_sql_container.py:206

• When process.poll() has already reaped the child and os.getpgid() then raises ProcessLookupError, this branch leaves group_blocked false and line 227 uses the now-unowned PID as a process-group ID. If that PID has been reused, timeout/cancellation cleanup can signal an unrelated process group, contradicting the stated guarantee that reaped PIDs are never targeted. Mark the group blocked for the reaped case; only use the PID fallback when the child is still unreaped.

            try:
                group = os.getpgid(process.pid)
            except ProcessLookupError:
                # Descendants may still hold the original group and stdout.

eng/scripts/setup_sql_container.py:468

• Docker command exit statuses are not the same as signals delivered to this helper: docker exec propagates the exit status of the process inside the container, so 130/143 can represent an ordinary SQL/readiness failure. Treating those values as Cancelled skips the configured retry and returns a cancellation result instead of following the normal check/polling path. Cancellation is already raised by the installed signal handler, so nonzero command results should remain command failures.

        if result.returncode in (-signal.SIGINT, -signal.SIGTERM, 130, 143):
            raise Cancelled(
                signal.SIGINT if result.returncode in (-signal.SIGINT, 130) else signal.SIGTERM
            )

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

Got 2 approval and the CI is green