fix(ingestion): apply AUTOCOMMIT via create_engine so releasing a connection cannot assert - #31535
fix(ingestion): apply AUTOCOMMIT via create_engine so releasing a connection cannot assert#31535ulixius9 wants to merge 1 commit into
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
github-actions
Bot
added
Ingestion
safe to test
✅ Playwright Results — workflow succeededValidated commit ✅ 110 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking 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 29s ⏱️ Max setup 3m 9s · max shard execution 12m 14s · max shard-job elapsed before upload 17m 45s · reporting 4s 🌐 213.83 requests/attempt · 1.79 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…nection cannot assert Ingestion engines opt into AUTOCOMMIT so read-only crawls stop pinning AccessShareLock for the length of a run (#29092, #29658). That was applied with engine.update_execution_options, which makes it a per-connection execution option: SQLAlchemy then tracks it as an IsolationLevelCharacteristic and restores the level when the connection returns to the pool. Only the create_engine kwarg records the level on the dialect, so with the execution option that field stays unset and the restore falls back to dialect.default_isolation_level -- which Dialect.initialize leaves as None on any dialect whose get_isolation_level raises NotImplementedError. MSDialect.get_isolation_level raises exactly that whenever its opening probe -- SELECT name FROM sys.system_views WHERE name IN ('dm_exec_sessions', 'dm_pdw_nodes_exec_sessions') -- returns no row or errors, which is the reported state on Azure Synapse. Every mssql+pyodbc connection then died in reset_isolation_level on `assert self.default_isolation_level is not None` the moment it was released -- after the probe query had already succeeded. Test connection reported "Failed to connect, please validate the credentials" on perfectly good credentials, and metadata ingestion never got past CheckAccess. Reproduced against SQL Server 2022 over ODBC Driver 18: a login whose probe succeeds is unaffected either way, while a login whose probe errors takes the AssertionError before this change and connects cleanly after it. Dialect support for AUTOCOMMIT can only be probed once the engine has resolved the dialect, so the engine is built twice when it applies; create_engine opens no connection, so discarding the first costs nothing. test_create_generic_db_connection_applies_autocommit now asserts the behaviour #29092 asked for -- a second connection sees an uncommitted write -- instead of the execution-option plumbing, which this change deliberately moves.
b7877fd to
e70ff99
Compare
Code Review ✅ ApprovedApplies AUTOCOMMIT via create_engine rather than execution options to prevent connection pool reset assertions on dialects like Azure Synapse. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Describe your changes
Ingestion engines opt into
AUTOCOMMITso read-only crawls stop pinningAccessShareLockfor the length of a run (#29092, #29658). It was applied withengine.update_execution_options(isolation_level="AUTOCOMMIT"), which makes it a per-connection execution option. SQLAlchemy then tracks it as anIsolationLevelCharacteristicand restores the level when the connection returns to the pool.Only the
create_engine(isolation_level=...)kwarg records the level on the dialect (_on_connect_isolation_level), and that is what the restore path reads. Supplied as an execution option, the field stays unset andreset_isolation_levelfalls back todialect.default_isolation_level— whichDialect.initializeleaves asNoneon any dialect whoseget_isolation_levelraisesNotImplementedError.MSDialect.get_isolation_levelraises exactly that whenever its opening probe returns no row or errors:That is the reported state on Azure Synapse. Every
mssql+pyodbcconnection then died on release:The probe query had already succeeded, so the user-facing result was
Failed to connect, please validate the credentialson perfectly good credentials, and metadata ingestion never got pastCheckAccess. This affects anymssql+pyodbctarget where the isolation level cannot be read back.Dialect support for
AUTOCOMMITcan only be probed once the engine has resolved the dialect, so the engine is built twice when it applies.create_engineopens no connection, so discarding the first one costs nothing; the call itself is unchanged, just moved behind a localbuild_engine(**extra).Type of change
Checklist
Verification
Against a live SQL Server 2022 over ODBC Driver 18
Three logins on the same instance, differing only in whether the isolation-level probe succeeds, each driven through the real
create_generic_db_connection:default_isolation_levelmainGRANT VIEW SERVER STATE['dm_exec_sessions']'READ COMMITTED'db_datareaderonly['dm_exec_sessions']'READ COMMITTED'ProgrammingError→NotImplementedErrorNoneThe third row is the field failure, reproduced on a real dialect and driver, and fixed.
Note rows 1–2: permissions are not the trigger. A principal without
VIEW SERVER STATEstill reads its own session row fromsys.dm_exec_sessions, so granting it is not a workaround. What matters is only whether the probe resolves at all.Unit tests
The new test fails on
mainat the same frame and passes with the fix:Wider sweep —
ingestion/tests/unit/{test_connection_builders.py,source/database,topology/database}:1566 passed, with the same 7 failures and 31 collection errors (missing optional drivers) present onmainbefore the change.ruff check/ruff format --check/basedpyrightclean on both files.Test change worth a look
test_create_generic_db_connection_applies_autocommitassertedengine.get_execution_options()["isolation_level"] == "AUTOCOMMIT"— the exact plumbing this PR deliberately moves. It now asserts the behaviour #29092 actually asked for: a second connection sees a write that was never committed. That assertion passes both before and after this change, so it still guards the original fix.Backport
The regression is in every branch carrying #29658, including
1.13.