Skip to content

fix(mssql): stop capturing a known connection-failure class in validate_credentials - #114828

Merged
trunk-io[bot] merged 2 commits into
masterfrom
posthog/fix-mssql-validate-credentials-eof-capture
Oct 10, 2026
Merged

trunk-io[bot] merged 2 commits into
masterfrom
posthog/fix-mssql-validate-credentials-eof-capture

Conversation

@Gilbert09

Copy link
Copy Markdown
Member

Problem

A customer checking their MSSQL connection (the incremental-fields setup flow) gets reported to error tracking as a bug when SQL Server drops the connection mid-handshake, even though the identical failure is already treated as an expected, non-retryable condition everywhere else.

The error: pymssql.OperationalError carrying DB-Lib 20017 ("Unexpected EOF from the server") paired with DB-Lib 20002 ("Adaptive Server connection failed").

Changes

  • validate_credentials keeps its own MSSQLErrors map, separate from get_non_retryable_errors, to turn a driver error into setup-time guidance. It had no entry for "Adaptive Server connection failed", so a credential check hitting it fell through to capture_exception instead of returning the same generic guidance an unmatched error already gets.
  • Adds that pattern to MSSQLErrors, mirroring the equivalent entry already in get_non_retryable_errors for the sync path.
  • Extracts the generic "Could not connect to MS SQL" fallback string into a _GENERIC_CONNECTION_ERROR constant, reused by the new entry and the two call sites that already returned it. Mechanical.

How did you test this code?

Test rationale: added test_adaptive_server_connection_failed_returns_generic_message_without_capturing, parallel to the existing test_firewall_block_returns_actionable_message_without_capturing. It reproduces the exact DB-Lib 20017/20002 message from the error-tracking issue and asserts validate_credentials returns the generic guidance without calling capture_exception — the regression this PR fixes.

Ran locally: hogli test products/warehouse_sources/backend/temporal/data_imports/sources/mssql/tests/test_mssql.py (67 passed), ruff check/ruff format --check on both changed files, and mypy on source.py — all clean. Not run: a live MSSQL connection, since the failure is driver-level and fully covered by the unit test.

No duplicate: searched open PRs by keyword (mssql, exception type, module path) and the maintainer's full open-PR list (gh pr list --state open --author Gilbert09) — nothing touches this source's validate_credentials path.

Release status

  • No feature flag controls this change

Docs update

None — no user-facing behavior or documented workflow changed; this only removes error-tracking noise for an already-classified failure.

🤖 Agent context

Autonomy: Fully autonomous

Agent: Claude Code, Sonnet 5

Triaged from a live error-tracking webhook. Invoked /writing-tests before adding the test case and /writing-pr-descriptions for this body. The only decision point was where to add the suppression: get_non_retryable_errors/get_retryable_errors already covered the sync path, so the gap was specifically validate_credentials's separate MSSQLErrors map, which Postgres and MySQL's equivalent code shows is the intended place to mirror that classification for the setup-time flow.

…te_credentials

`validate_credentials` matches `OperationalError` against its own `MSSQLErrors` map, which did not list "Adaptive Server connection failed" (DB-Lib 20002) even though `get_non_retryable_errors` already classifies it for the sync path. A credential check that hit this error fell through to `capture_exception`, reporting an expected connection drop as a bug.

Adds the pattern to `MSSQLErrors` with the same generic message the unmatched-error fallback already returns, and extracts that fallback string into a constant shared by both call sites and the new entry.

Generated-By: PostHog Desktop
Task-Id: ea41fbf7-1d47-40ae-9bc9-572d91c4fe3c
@Gilbert09 Gilbert09 added the stamphog Request AI approval (no full review) label Oct 10, 2026 — with Talyn App
@trunk-io

trunk-io Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@parameterai

parameterai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Risk: No findings

This PR adds a "Adaptive Server connection failed" entry to the MSSQL source's validate_credentials error map so a known driver-level connect failure returns generic guidance instead of being captured to error tracking, plus extracts the reused message into a constant. The change is confined to fixed-string error mapping; no security-relevant surface is touched and all returned strings are constants, not driver output.

Sentinel reviewed 951018f · Review settings

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Contained error-reporting fix with regression coverage; no open substantive review concerns. Credential acceptance, host restrictions, and ingestion behavior remain unchanged.

  • Author wrote 100% of the modified lines and has 2 merged PRs in these paths (familiarity STRONG).
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 11L, 1F substantive, 32L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (32L, 2F, single-area, fix)
stamphog 2.4.1 .stamphog/policy.yml @ 951018f · reviewed head 951018f

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Backend coverage — 83.0% of changed backend lines covered — 2 uncovered

🧪 Backend test coverage

Patch coverage — changed backend lines (products + core): █████████████████░░░ 83.0% (10 / 12)

File Patch Uncovered changed lines
products/warehouse_sources/backend/temporal/data_imports/sources/mssql/source.py 33.3% 382, 391

🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 160518418359574 -n patch-coverage), or the coverage-data block at the end of this comment.

Per-product line coverage (touched products)
Product Coverage Lines
demo ███████████░░░░░░░░░ 55.9% 1,508 / 2,700
batch_exports ████████████████░░░░ 80.1% 21,382 / 26,689
engineering_analytics █████████████████░░░ 84.9% 11,355 / 13,374
mcp_analytics █████████████████░░░ 85.7% 4,844 / 5,651
warehouse_suggestions █████████████████░░░ 87.3% 2,252 / 2,580
posthog_ai ██████████████████░░ 87.7% 4,226 / 4,817
notebooks ██████████████████░░ 88.4% 15,201 / 17,196
today ██████████████████░░ 88.6% 2,866 / 3,233
cdp ██████████████████░░ 88.7% 5,081 / 5,729
mcp_registry ██████████████████░░ 89.0% 1,596 / 1,794
product_tours ██████████████████░░ 89.1% 1,337 / 1,500
cohorts ██████████████████░░ 89.6% 8,458 / 9,438
signals ██████████████████░░ 89.8% 67,354 / 75,014
dashboards ██████████████████░░ 89.8% 7,107 / 7,911
data_modeling ██████████████████░░ 90.1% 10,839 / 12,026
tasks ██████████████████░░ 90.6% 80,614 / 89,007
data_warehouse ██████████████████░░ 90.8% 14,831 / 16,339
canvas ██████████████████░░ 90.9% 7,748 / 8,522
exports ██████████████████░░ 91.0% 9,698 / 10,663
business_knowledge ██████████████████░░ 91.0% 8,791 / 9,656
autoresearch ██████████████████░░ 91.1% 10,139 / 11,125
managed_warehouse ██████████████████░░ 91.2% 11,053 / 12,114
error_tracking ██████████████████░░ 91.6% 16,797 / 18,328
wizard ██████████████████░░ 91.8% 6,008 / 6,548
streamlit_apps ██████████████████░░ 91.8% 3,087 / 3,362
stamphog ██████████████████░░ 92.0% 8,242 / 8,963
conversations ██████████████████░░ 92.0% 29,714 / 32,284
early_access_features ███████████████████░ 92.6% 1,339 / 1,446
alerts ███████████████████░ 92.9% 7,246 / 7,797
review_hog ███████████████████░ 93.0% 14,864 / 15,990
surveys ███████████████████░ 93.1% 6,626 / 7,118
web_analytics ███████████████████░ 93.1% 24,096 / 25,877
notifications ███████████████████░ 93.2% 1,152 / 1,236
approvals ███████████████████░ 93.4% 4,526 / 4,848
cross_project_dashboards ███████████████████░ 93.4% 880 / 942
slack_app ███████████████████░ 93.5% 14,854 / 15,882
context_layer ███████████████████░ 93.7% 3,409 / 3,640
marketing_analytics ███████████████████░ 93.8% 20,074 / 21,402
messaging ███████████████████░ 93.8% 4,468 / 4,762
billing_alerts ███████████████████░ 93.9% 2,090 / 2,226
customer_analytics ███████████████████░ 93.9% 26,079 / 27,773
mcp_store ███████████████████░ 93.9% 9,008 / 9,593
experiments ███████████████████░ 94.2% 33,676 / 35,760
ai_observability ███████████████████░ 94.2% 26,434 / 28,069
replay_vision ███████████████████░ 94.3% 29,048 / 30,807
logs ███████████████████░ 94.5% 15,850 / 16,770
actions ███████████████████░ 94.6% 973 / 1,029
endpoints ███████████████████░ 94.8% 9,298 / 9,812
reminders ███████████████████░ 94.8% 760 / 802
growth ███████████████████░ 94.8% 11,268 / 11,880
skills ███████████████████░ 94.9% 7,031 / 7,410
annotations ███████████████████░ 95.0% 816 / 859
tracing ███████████████████░ 95.2% 3,631 / 3,815
data_catalog ███████████████████░ 95.5% 4,419 / 4,628
access_control ███████████████████░ 95.5% 7,779 / 8,143
product_analytics ███████████████████░ 95.7% 28,676 / 29,952
alerts_platform ███████████████████░ 95.8% 5,013 / 5,234
revenue_analytics ███████████████████░ 95.9% 1,878 / 1,959
data_quality ███████████████████░ 95.9% 8,569 / 8,933
warehouse_sources ███████████████████░ 96.4% 453,815 / 470,829
pulse ███████████████████░ 97.4% 2,023 / 2,078
metrics ████████████████████ 97.7% 4,406 / 4,509
analytics_platform ████████████████████ 98.0% 2,775 / 2,833

Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.

@trunk-io

trunk-io Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

Copy link
Copy Markdown
Member Author

/trunk merge

@trunk-io
trunk-io Bot merged commit c0faaf5 into master Oct 10, 2026
271 checks passed
@trunk-io
trunk-io Bot deleted the posthog/fix-mssql-validate-credentials-eof-capture branch October 10, 2026 07:36
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-10 07:53 UTC Run
prod-us ✅ Deployed 2026-10-10 08:02 UTC Run
prod-eu ✅ Deployed 2026-10-10 08:03 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant