fix(mssql): harden error handling and observability across throttling, description maps, and unguarded query paths

[akashverma0786]

Describe your changes:

No linked issue — these are six independent reliability/observability hardening fixes for the mssql connector, found during a manual connector-reliability audit (error handling, fault tolerance, and observability standards). Happy to open issues retroactively if preferred.

Error classification & observability (3 commits, already merged into this branch):

• SQLSERVER_ERRORS didn't recognize Azure SQL's DTU/vCore resource-governance throttling errors (10928/10929/10936, including the pyodbc text-only fallback and elastic-pool wording), so a throttled customer got a generic/unclassified error instead of an actionable diagnosis.
• Description-map load failures (schema/database/stored-procedure comments) were logged at DEBUG, so a real permission or connection issue on that path was invisible by default. Raised to WARNING.
• The ingestAllDatabases per-database engine loop never logged which query-log source (Query Store vs plan-cache DMVs) was picked for each database, unlike the single-database path which already logs this.

Unguarded query executions that could crash a run outright (3 new fixes, this commit):

• _databases_to_scan() (query_parser.py) had no exception handling around the database-listing query that drives ingestAllDatabases usage/lineage runs. A transient failure (network blip, timeout, deadlock — not permissions, see verification below) crashed the entire lineage/usage extraction instead of falling back to the single already-connected engine, which the code already does for the "zero databases returned" case.
• get_sqlalchemy_engine_dateformat() (utils.py) had no exception handling around its DBCC USEROPTIONS probe, so a transient connection failure at that exact moment crashed MssqlUsageSource/lineage source construction outright — even though a documented DEFAULT_DATETIME_FORMAT fallback sat right next to it, unused.
• get_stored_procedures() (metadata.py) had no exception handling around its listing query. A real, reproducible permission gap (DENY SELECT ON sys.sql_modules, which correlates with the VIEW DEFINITION requirement our public docs don't currently call out) crashed the whole generator uncaught, failing the entire workflow (WorkflowExecutionError) over one schema's stored procedures.

I made them because each is a real, verified gap — see "Manual testing performed" below for how each was actually reproduced (not just read from the code) before being fixed, and re-verified live afterward. Two related findings from the same audit (get_database_names's single-database branch, and _per_database_engines's engine-creation placement) were investigated with equal rigor and turned out to be false positives: live testing showed both are already fully covered by test_connection()'s pre-flight checks and the HAS_DBACCESS/state=0 filter already baked into the database-listing query, so no code change was made for those two.

Type of change:

• Bug fix
• Improvement

High-level design:

N/A — six small, independent changes, no shared design decisions:

• ingestion/src/metadata/ingestion/source/database/mssql/connection.py: one new when() rule in the existing SQLSERVER_ERRORS ErrorPack, tightened across two follow-up commits.
• ingestion/src/metadata/ingestion/source/database/mssql/metadata.py: one log-level change (debug → warning) for description maps; get_stored_procedures()'s listing query wrapped in try/except reporting via self.status.warning() (not status.failed(), deliberately — see below).
• ingestion/src/metadata/ingestion/source/database/mssql/query_parser.py: one log line added per branch for Query Store selection; _databases_to_scan() wrapped in try/except falling back to the existing "no databases scanned" path.
• ingestion/src/metadata/ingestion/source/database/mssql/utils.py: get_sqlalchemy_engine_dateformat() wrapped in try/except, returning None (its declared Optional[str] already allowed this) so existing callers fall back to their documented default.

One deliberate API choice worth calling out: get_stored_procedures()'s failure path uses self.status.warning(key, reason), not self.status.failed(StackTraceError(...)). Status.calculate_success() only counts failures, not warnings — using failed() (which is what the sibling get_database_names multi-database branch already uses) would make a single schema's stored-procedure permission gap capable of tripping WorkflowExecutionError for the whole run, which is disproportionate to what's actually lost. warning() keeps the gap visible in the pipeline status without failing the run. Verified live both ways (see manual testing).

Tests:

Use cases covered

• An Azure SQL connection hitting its DTU/vCore resource limit gets a specific "Azure SQL resource limit reached (throttled)" diagnosis instead of a generic error.
• A description-map load failure is now visible at WARNING instead of silently swallowed at DEBUG.
• An ingestAllDatabases run logs, per database, whether Query Store or plan-cache DMVs were used for query history.
• A transient failure listing databases for an ingestAllDatabases usage/lineage run degrades to the single configured engine instead of crashing the run.
• A transient failure probing the server's dateformat falls back to the documented default instead of crashing usage/lineage source construction.
• A permission gap on stored-procedure definitions (VIEW DEFINITION / sys.sql_modules) for one schema is reported as a warning and the rest of ingestion (databases, schemas, tables) completes normally, with the workflow still reporting overall success.

Unit tests

• I added unit tests for the new/changed logic.
• Files added/updated: ingestion/tests/unit/source/database/mssql/test_connection.py (3 tests, throttling classification, from the earlier commits), ingestion/tests/unit/topology/database/test_mssql.py (4 new tests this commit: test_get_stored_procedures_degrades_gracefully_on_query_failure, test_databases_to_scan_degrades_gracefully_on_query_failure, TestMssqlDateformatProbe::test_returns_dateformat_value / test_returns_none_when_probe_errors).
• Ran the full file locally: pytest tests/unit/topology/database/test_mssql.py → 52 passed (48 pre-existing + 4 new). ruff check clean on all changed files.

Backend integration tests

• Not applicable (no backend API changes).

Ingestion integration tests

• Not applicable — see manual testing below for real-driver, real-database verification, which is where these bugs actually needed to be reproduced (unguarded exception paths don't reproduce meaningfully under mocks).

Playwright (UI) tests

• Not applicable (no UI changes).

Manual testing performed

For each of the three new fixes, reproduced the original crash live against a real SQL Server 2022 Docker container using the actual metadata ingest / metadata usage CLI (not unit tests), then re-ran the identical scenario after the fix:

1. get_stored_procedures() (highest-confidence repro): created a login, ran DENY SELECT ON sys.sql_modules against it. Before fix: metadata ingest raised WorkflowExecutionError, exit code 1, uncaught OperationalError (SQL error 229) propagating out of the generator. After fix (first pass, using status.failed): still exit code 1 — same disproportionate-failure problem, just with a cleaner message. After the status.warning correction: Warnings: 3, Success %: 100.0, exit code 0; databases/schemas/tables ingest normally.
2. _databases_to_scan(): tried to force a real permission-based failure on sys.databases (DENY SELECT ON sys.databases, DENY VIEW ANY DATABASE) — neither raises for a login with any database access; SQL Server filters rows there rather than denying the query. Confirmed the real trigger is transient/infra, not permissions. Regression-tested the fix live with a two-database ingestAllDatabases: true usage run — 100% success, both databases correctly listed.
3. get_sqlalchemy_engine_dateformat(): confirmed live that DBCC USEROPTIONS requires zero permissions even for a heavily locked-down login, ruling out a permission-based repro; the only realistic trigger is a connection failure in the narrow window between test_connection() and this call. Verified the fix via deterministic code tracing (unconditional try/except) plus a live regression run (100% success, unaffected happy path).
4. Also live-tested two related audit findings that turned out to be false positives (no code change made): a nonexistent/inaccessible single configured database is caught cleanly by test_connection() before get_database_names() ever runs; and an inaccessible second database in ingestAllDatabases mode is silently and correctly excluded by the existing HAS_DBACCESS/state=0 filter before _per_database_engines() ever sees it — confirmed with a live two-database run where the inaccessible one never appeared in any log or error.
5. Ran metadata usage/metadata ingest baseline (happy-path) runs before and after every change to confirm no regressions — all at 100% success.
6. Cleaned up all test logins/databases/permissions from the shared test container afterward; verified it's back to its original state.

UI screen recording / screenshots:

Not applicable.

Checklist:

• I have read the CONTRIBUTING document.
• My PR title is Fixes <issue-number>: <short explanation> — no issue filed, see note above.
• My PR is linked to a GitHub issue via Fixes #<issue-number> above — no issue filed, see note above.
• I have commented on my code, particularly in hard-to-understand areas.
• For JSON Schema changes: not applicable, no schema changes in this PR.
• For UI changes: not applicable, no UI changes.
• I have added tests (unit / integration / Playwright as applicable) and listed them above.

Note on merge order: this branch touches some of the same files (metadata.py, query_parser.py, connection.py, test_mssql.py/test_connection.py) as other open mssql PRs (#31724, #31762, #31767). No two PRs edit the exact same lines as of this writing, but whichever merges last will need a routine rebase.

Greptile Summary

The PR hardens the MSSQL connector’s error handling and observability.

• Classifies Azure SQL resource-throttling errors across supported driver error shapes, including pyodbc text-only errors.
• Degrades gracefully when stored-procedure listing, database discovery, or date-format probing fails.
• Promotes description-map failures to warnings and logs per-database query-history source selection.
• Adds focused unit coverage for the new classification and fallback paths.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported pyodbc throttling gap is addressed by text fallbacks that are reached through the production classifier.

Important Files Changed

Filename	Overview
ingestion/src/metadata/ingestion/source/database/mssql/connection.py	Adds numeric and text-based Azure SQL throttling classification, completing the previously reported pyodbc path.
ingestion/src/metadata/ingestion/source/database/mssql/metadata.py	Makes description failures visible and converts stored-procedure listing failures into schema-scoped warnings.
ingestion/src/metadata/ingestion/source/database/mssql/query_parser.py	Logs per-database query-history routing and falls back to the configured engine when database discovery fails.
ingestion/src/metadata/ingestion/source/database/mssql/utils.py	Returns the existing optional fallback value when the MSSQL date-format probe fails.
ingestion/tests/unit/source/database/mssql/test_connection.py	Covers throttling classification through pytds, pymssql, and pyodbc error representations.
ingestion/tests/unit/topology/database/test_mssql.py	Adds tests for graceful stored-procedure, database-discovery, and date-format failure handling.

_{Reviews (6): Last reviewed commit: "Merge branch 'main' into mssql-improveme..." | Re-trigger Greptile}

[github-actions]

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

• No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

[gitar-bot]

💡 Quality: WARNING on description-map failure may be noisy under ingestAllDatabases

Escalating the description-map load failure from DEBUG to WARNING (metadata.py:213) improves visibility, but under ingestAllDatabases an inaccessible system database (e.g. model, whose guest account is disabled by default) will emit a WARNING on every run for each of the three description types — the author's own manual testing confirmed 3 WARNING lines per run against model. This is an expected, benign condition being surfaced at WARNING, which risks alarm fatigue. Consider suppressing/downgrading to DEBUG for known-inaccessible system databases while keeping WARNING for genuine user-database failures, or including enough context in the message to make the expected-vs-unexpected distinction clear.

_{Was this helpful? React with 👍 / 👎}

[github-actions]

✅ Playwright Results — workflow succeeded

Validated commit e88579be9b9b522175854baa158a47188fe06c2f in Playwright run 32493379450, attempt 1.

✅ 110 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 49m 40s

⏱️ Max setup 5m 53s · max shard execution 12m 43s · max shard-job elapsed before upload 19m 59s · reporting 3s

🌐 216.59 requests/attempt · 1.79 app boots/UI scenario · 0.00% common-shard skew

Optimization targets still in progress:

• Browser traffic was 216.59 requests per attempt (convergence target: fewer than 200).
• Application boot ratio was 1.79 per UI scenario (216 boots / 121 scenarios; convergence target: at most 1).

Shard	Passed	Failed	Flaky	Skipped	Lifecycle failed	Lifecycle flaky
✅ Shard chromium-01	46	0	0	0	0	0
✅ Shard ingestion-01	28	0	0	0	0	0
✅ Shard ingestion-02	36	0	0	0	0	0

📦 Download artifacts

How to debug locally

# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

[gitar-bot]

Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Improves MSSQL connector reliability by classifying Azure SQL throttling errors, promoting description-map load failures to warnings, and logging per-database query-history source selection. Consider reviewing whether the description-map failure warning could become noisy during multi-database ingestion.

💡 Quality: WARNING on description-map failure may be noisy under ingestAllDatabases

📄 ingestion/src/metadata/ingestion/source/database/mssql/metadata.py:211-213

Escalating the description-map load failure from DEBUG to WARNING (metadata.py:213) improves visibility, but under ingestAllDatabases an inaccessible system database (e.g. model, whose guest account is disabled by default) will emit a WARNING on every run for each of the three description types — the author's own manual testing confirmed 3 WARNING lines per run against model. This is an expected, benign condition being surfaced at WARNING, which risks alarm fatigue. Consider suppressing/downgrading to DEBUG for known-inaccessible system databases while keeping WARNING for genuine user-database failures, or including enough context in the message to make the expected-vs-unexpected distinction clear.

✅ 1 resolved

✅ Edge Case: Broad substring "has been reached" may misclassify errors

📄 ingestion/src/metadata/ingestion/source/database/mssql/connection.py:127-132
The text fallback Matchers.contains("has been reached") is a case-insensitive substring match against the full exception chain, so any non-throttling SQL Server error whose message happens to contain this generic phrase (on a pyodbc connection where no error number is exposed) would be diagnosed as "Azure SQL resource limit reached (throttled)". Impact is limited because this is the last rule (first-match-wins) and the number-based match covers pytds/pymssql, but consider tightening the phrase (e.g. "limit for the database is"/"limit for the elastic pool is") to reduce the chance of a wrong diagnosis on the numberless pyodbc path.

🤖 Prompt for agents

Code Review: Improves MSSQL connector reliability by classifying Azure SQL throttling errors, promoting description-map load failures to warnings, and logging per-database query-history source selection. Consider reviewing whether the description-map failure warning could become noisy during multi-database ingestion.

1. 💡 Quality: WARNING on description-map failure may be noisy under ingestAllDatabases
   Files: ingestion/src/metadata/ingestion/source/database/mssql/metadata.py:211-213

   Escalating the description-map load failure from DEBUG to WARNING (metadata.py:213) improves visibility, but under `ingestAllDatabases` an inaccessible system database (e.g. `model`, whose `guest` account is disabled by default) will emit a WARNING on every run for each of the three description types — the author's own manual testing confirmed 3 WARNING lines per run against `model`. This is an expected, benign condition being surfaced at WARNING, which risks alarm fatigue. Consider suppressing/downgrading to DEBUG for known-inaccessible system databases while keeping WARNING for genuine user-database failures, or including enough context in the message to make the expected-vs-unexpected distinction clear.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud