Skip to content

FIX: Coordinate Arrow reader cross-thread shutdown - #892

Open
Jahnvi Thakkar (jahnvi480) wants to merge 10 commits into
mainfrom
jahnvi/arrow-close-race-triage
Open

Jahnvi Thakkar (jahnvi480) wants to merge 10 commits into
mainfrom
jahnvi/arrow-close-race-triage

Conversation

@jahnvi480

@jahnvi480 Jahnvi Thakkar (jahnvi480) commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

N/A; tracked by the GitHub issue below.

GitHub Issue: #890


Summary

Coordinate Arrow reader close with the complete batch operation, preventing idle SQLCancel from closing a result before metadata/fetch work finishes.

  • Cancel only an active fetch and fence fetch completion against outstanding SQLCancel. Idle close returns before weak-handle acquisition, GIL release, or mutex acquisition.
  • Serialize complete iterator advancement and cleanup, including delegated reads and exported Arrow C streams. Reentrant close on the reading thread defers cleanup until advancement unwinds. Closing a never-started generator also performs cleanup.
  • Check both the existing result-description identity and weak statement identity. Stale readers cannot consume, close, or reset bookkeeping for a reexecuted cursor, a subsequent result set, or a replacement statement after failed execute. Explicit parent-cursor close still raises ProgrammingError. No new execute-path generation counter or strong statement ownership is introduced.
  • Stop on HY008 even with SQL_SUCCESS_WITH_INFO. Recognize HY007/HY010/24000 as shutdown results only after this reader requested close and issued cancellation. Preserve unrelated diagnostics regardless of record ordering.
  • Run native-boundary probes in Windows PR CI using the default-OFF MSSQL_ARROW_TEST_INTERPOSITION option. Requested but unavailable controls fail instead of skipping; ordinary Release builds expose no test pointer API.
  • Bound concurrency probes in subprocesses, guard from_stream cases on supported PyArrow versions lacking that API, and add a real-driver cancellation test. It observes the same outstanding SQLFetch across a DMV round trip while the reader is blocked by a known lock owner, invokes the real SQLCancel, and verifies termination and cursor reuse before releasing the blocking transaction. LocalDB CI receives the required DMV-read permissions.

The native completion fence and Python batch lock protect different boundaries and remain necessary. No public API/type-stub changes or throughput improvement claims.

Latest review fixes and validation

Revision: 4f5a5b11241770d4b2ad33e2cd0932e03537988f (after merging current main).

  • Removed the Unix exported-symbol fallback. Only the default-OFF compiled _arrow_test_pointer_addresses API enables interposition. Ordinary builds skip; explicitly requested but missing controls fail. Focused regressions guard both policies without loading exported globals.
  • Accept both SQL_SUCCESS and SQL_SUCCESS_WITH_INFO for exactly one SQLCancel. The live blocked-fetch probe now runs with the driver's actual result and with an injected success-with-info return after a genuinely successful real SQLCancel; both still require termination and cursor reuse before releasing the blocking transaction.
  • .\build.bat x64 from mssql_python\pybind with MSSQL_ARROW_TEST_INTERPOSITION=1: passed.
  • python -m pytest tests\test_004_cursor_arrow.py -m "not slow" -q --tb=short with compiled controls: 82 passed, 1 skipped per provider (msodbcsql18 and mssql-odbc, separate processes).
  • .\build.bat x64 with the flag unset: passed.
  • The same pytest command on the ordinary Release build: 71 passed, 12 skipped per provider. Eleven cases require the compiled controls; ordinary runs do not acquire a new DMV-permission requirement.
  • python -m pre_commit run black-check --all-files --hook-stage pre-push: passed, including the push hook.
  • git diff --check: passed.

All three newly added regression cases failed before these fixes. A separate requested-controls/ordinary-build negative run failed with the expected missing-controls message rather than skipping. The full repository suite and Linux/macOS runtime validation were not run locally; fresh CI is pending for this revision. Earlier-revision evidence follows.

Validation (previous revision)

Revision: d84b6ebc231d11456ac3ccb482507fb389fc50c6. Windows x64, Python 3.13.17, PyArrow 25.0.1, live SQL Server. Both msodbcsql18 and mssql-odbc ran in separate processes.

  • .\build.bat x64 from mssql_python\pybind, with MSSQL_ARROW_TEST_INTERPOSITION=1: passed, clean Release build using the Windows PR CI opt-in.
  • python -m pytest tests\test_004_cursor_arrow.py -m "not slow" -q --tb=short, with that build: 79 passed, 1 skipped per provider, including all native-boundary cases, the real blocked-driver-fetch probe, bounded concurrency probes, and stress coverage.
  • .\build.bat x64 with MSSQL_ARROW_TEST_INTERPOSITION unset: passed, clean ordinary Release build.
  • python -c "from mssql_python import ddbc_bindings; assert not hasattr(ddbc_bindings.module, '_arrow_test_pointer_addresses')": passed with the ordinary build.
  • python -m pytest tests\test_004_cursor_arrow.py -m "not slow" -q --tb=short, with the ordinary build: 69 passed, 11 skipped per provider. Ten skips require interposition, explicitly enabled in Windows PR CI.
  • python -m pytest tests\test_039_rs_runtime_pipelines.py::test_modified_pipeline_yaml_parses -q --tb=short: 1 passed. Parsed configuration checks confirmed Windows test controls and LocalDB DMV grants.
  • python -m pre_commit run black-check --all-files --hook-stage pre-push: passed, including the push hook.
  • python -m pre_commit validate-config: passed.
  • git diff --check: passed.

Additional controls: both replacement-statement regressions failed before the weak-identity fix; both C-stream parameters skipped when from_stream was deliberately absent; deliberately frozen cleanup was terminated by a subprocess timeout. PyArrow 14 itself was not installed on this Python 3.13 environment. Earlier controls reproduced stale-result cleanup, reentrant deadlock, and cancellation-first mixed-diagnostic failures before their fixes.

An intermediate full Arrow run caught a parent-closed exception regression in the new identity check; it was corrected before the final clean runs above. The full repository suite and Linux/macOS runtime validation were not run locally. The previous revision passed its remote PR validation matrix; the new revision requires a fresh CI run.

Existing scanner warning

DevSkim DS121708 flags an existing Arrow metadata memcpy. The flagged copies and allocation sizes are unchanged from the PR base; this fix introduces no memcpy calls. No scanner suppression or unrelated buffer-copy rewrite is included.

Fence reader cancellation against native fetch completion, serialize batch advancement with cleanup, and cover idle and partial-success cancellation without suppressing unrelated errors.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 09:07
@github-actions github-actions Bot added the pr-size: large Substantial code update label Nov 12, 2026
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

PR Performance Report

✅ No regression detected

No consistent slowdowns detected across all 2 environments.

0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS

Coverage: 2 of 2 environments completed. Advisory result; does not block merging.

Performance diagnostics

Phase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed.

No affected phases or call-count changes were recorded.

All database tasks and timings

Unix / SQL Server 2022

Database task Before After Paired change Result
Connection opening 10.235 ms 10.863 ms +5.6% no signal
SELECT queries 1.041 ms 1.012 ms -2.1% no signal
Row insertion 30.861 ms 30.772 ms +0.0% no signal
Executemany inserts 161.952 ms 160.926 ms -1.2% no signal
Fetch-all queries 119.225 ms 119.386 ms -0.3% no signal
Row-by-row fetching 4.957 ms 4.965 ms +0.1% no signal
Batched row fetching 116.334 ms 117.373 ms +2.2% no signal
Transaction commit and rollback 106.771 ms 106.245 ms -0.8% no signal
Arrow row fetching 88.467 ms 93.667 ms +6.6% no signal
100,000-row insertion 363.362 ms 346.170 ms -0.8% no signal
Row fetching in batches of 100 126.457 ms 123.425 ms -2.1% no signal
Row fetching in batches of 10,000 138.205 ms 136.573 ms -0.5% no signal
Repeated positional queries 32.495 ms 32.290 ms +0.6% no signal
Repeated named-parameter queries 32.421 ms 32.729 ms +1.0% no signal
Legacy 100,000-row insertion 258.438 ms 275.849 ms +0.5% no signal
Insertion with explicit input sizes 386.279 ms 387.124 ms +0.2% no signal
Joined aggregation queries 182.840 ms 180.464 ms -0.6% no signal
Large joined-result fetching 191.303 ms 193.916 ms +2.0% no signal
1.2-million-row fetching 3487.212 ms 3492.320 ms +0.3% no signal
Common table expression queries 5.396 ms 5.418 ms -1.2% no signal
256 KiB VARCHAR(MAX) / fetchall() 1.580 ms 1.542 ms -2.4% no signal
10,000 scalar values / fetchval() (debug disabled) 16.830 ms 16.772 ms -0.8% no signal
10,000 output-converted rows / fetchall() 19.513 ms 19.379 ms -0.2% no signal
10,000 output-converted rows / fetchmany(1000) 23.744 ms 23.678 ms -0.1% no signal
10,000 INT/NVARCHAR rows / fetchmany(1) 24.489 ms 24.349 ms -0.7% no signal
10,000 rows x 32 columns / dict(row._mapping) (warm) 32.422 ms 32.403 ms -0.6% no signal

Unix / SQL Server 2025

Database task Before After Paired change Result
Connection opening 81.644 ms 81.127 ms -1.4% no signal
SELECT queries 1.003 ms 1.019 ms +1.6% no signal
Row insertion 30.403 ms 30.448 ms +0.1% no signal
Executemany inserts 155.823 ms 159.845 ms +4.2% no signal
Fetch-all queries 123.675 ms 120.752 ms -2.8% no signal
Row-by-row fetching 5.021 ms 4.984 ms -0.2% no signal
Batched row fetching 118.113 ms 116.967 ms -0.4% no signal
Transaction commit and rollback 107.291 ms 107.028 ms -0.2% no signal
Arrow row fetching 89.610 ms 90.235 ms -3.1% no signal
100,000-row insertion 361.932 ms 361.537 ms +1.5% no signal
Row fetching in batches of 100 122.594 ms 128.532 ms +4.7% no signal
Row fetching in batches of 10,000 132.008 ms 142.825 ms +1.0% no signal
Repeated positional queries 31.814 ms 32.081 ms +1.3% no signal
Repeated named-parameter queries 32.313 ms 32.407 ms +0.5% no signal
Legacy 100,000-row insertion 261.982 ms 257.276 ms -1.6% no signal
Insertion with explicit input sizes 403.498 ms 405.023 ms +0.4% no signal
Joined aggregation queries 166.688 ms 165.472 ms -0.3% no signal
Large joined-result fetching 196.244 ms 190.730 ms -3.8% no signal
1.2-million-row fetching 3496.482 ms 3521.907 ms +0.6% no signal
Common table expression queries 5.123 ms 5.196 ms +1.5% no signal
256 KiB VARCHAR(MAX) / fetchall() 1.869 ms 1.910 ms +4.6% no signal
10,000 scalar values / fetchval() (debug disabled) 16.876 ms 17.300 ms +2.5% no signal
10,000 output-converted rows / fetchall() 20.427 ms 19.570 ms -5.0% no signal
10,000 output-converted rows / fetchmany(1000) 23.390 ms 23.595 ms +1.0% no signal
10,000 INT/NVARCHAR rows / fetchmany(1) 24.313 ms 24.707 ms +1.7% no signal
10,000 rows x 32 columns / dict(row._mapping) (warm) 32.417 ms 32.295 ms -2.9% no signal
Build and measurement details

ADO build 192000

PR head: ceb4955cd796721d66aed482a5f0058c6cc13953
Base: 54237fd6932fe5e198fd7f591bf980d0117b8b77
Measured merge: 3a73e2a23faf33632c37f9a5324dd81d2b80ca5f

  • Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4325.4; 5 paired comparisons and 1 warmup.
  • Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5065.9; 5 paired comparisons and 1 warmup.

A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent.

The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes.

Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency.

Raw samples and logs are attached to the ADO run as profiler-* artifacts.

Comment thread mssql_python/pybind/ddbc_bindings.cpp Dismissed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Closing or collecting an old reader can invalidate a newer query result, and reentrant close can deadlock the reading thread.

3 open issues
What changed in this PR

This PR addresses #890 by coordinating Arrow reader shutdown across the Python iterator and native ODBC fetch paths, without changing the public API.

Changes:

  • Adds per-reader close state and waits for active cancellation before continuing native work.
  • Serializes batch advancement and cleanup, including delegated reads and exported Arrow C streams.
  • Adds synchronized regression tests for shutdown boundaries, cancellation diagnostics, and parent-cursor reuse, while retaining timing-based stress coverage.
File Description
tests/​test_004_cursor_arrow.py Adds controlled shutdown and cancellation regression tests.
mssql_python/​pybind/​ddbc_bindings.cpp Coordinates native fetch cancellation and classifies shutdown diagnostics.
mssql_python/​cursor.py Synchronizes reader iteration and cleanup with native close requests.

Review effort: Balanced

Comment thread mssql_python/cursor.py Resolved
Comment thread mssql_python/cursor.py Resolved
Comment thread tests/test_004_cursor_arrow.py Resolved
Copilot AI balanced review requested due to automatic review settings November 12, 2026 09:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Closing an old reader can disrupt a newer cursor result, and mixed fetch diagnostics can raise the wrong error when cancellation appears first.

6 open issues

Review effort: Balanced

Comment thread mssql_python/cursor.py Resolved
Comment thread mssql_python/pybind/ddbc_bindings.cpp Resolved
Comment thread mssql_python/pybind/ddbc_bindings.cpp Resolved
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

82%


🎯 Overall Coverage

86%


📈 Total Lines Covered: 11636 out of 13522
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/cursor.py (90.2%): Missing lines 121,127,349,364-366,3258,3421
  • mssql_python/pybind/ddbc_bindings.cpp (76.6%): Missing lines 5409,5423-5429,5433-5436,5438-5441,5444,5451-5452,5476-5477,5490-5491,5725-5726

Summary

  • Total: 189 lines
  • Missing: 33 lines
  • Coverage: 82%

mssql_python/cursor.py

Lines 117-125

  117                     self.close()
  118 
  119     @property
  120     def gi_frame(self):
! 121         return self._generator.gi_frame
  122 
  123     def close(self):
  124         with self._lock:
  125             if self._generator.gi_running:

Lines 123-131

  123     def close(self):
  124         with self._lock:
  125             if self._generator.gi_running:
  126                 # A signal handler on the reading thread must let __next__ unwind.
! 127                 return False
  128             self._generator.close()
  129             # Closing a generator that has never been advanced skips its finally.
  130             self._cleanup()
  131             return True

Lines 345-353

  345         # the cleanup steps below fail and we end up retried later.
  346         self._closed = True
  347         try:
  348             self._close_requested.request_close()
! 349         except Exception as e:  # pylint: disable=broad-exception-caught
  350             logger.warning("arrow_reader.close: SQLCancel raised: %s", e)
  351 
  352         # Wait for any iterator advancement before closing the generator. The
  353         # native close request interrupts a blocked fetch without taking this lock.

Lines 360-370

  360         gen = self._generator
  361         if gen is not None:
  362             try:
  363                 if gen.close() is False:
! 364                     self._cursor = None
! 365                     self._inner = None
! 366                     return
  367             except Exception as e:  # pylint: disable=broad-exception-caught
  368                 logger.debug("arrow_reader.close: generator.close raised: %s", e)
  369                 if getattr(gen, "gi_frame", None) is not None:
  370                     # Generator still alive — leave _generator (and _cursor,

Lines 3254-3262

  3254             self.hstmt, capsules, max(batch_size, 0), char_c_type, self.messages, reader_state
  3255         )
  3256         check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
  3257         if reader_state is not None and ret == _SQL_NO_DATA:
! 3258             return None
  3259 
  3260         batch = pyarrow.RecordBatch._import_from_c_capsule(*capsules)
  3261 
  3262         # Update rownumber for the number of rows actually fetched

Lines 3417-3425

  3417                     ):
  3418                         return
  3419                     batch = cur._fetch_arrow_batch(batch_size, close_requested)
  3420                     if batch is None or close_requested.closed:
! 3421                         return
  3422                     if batch.num_rows == 0:
  3423                         exhausted = True
  3424                         return
  3425                     yield batch

mssql_python/pybind/ddbc_bindings.cpp

Lines 5405-5413

  5405     return era * 146097 + static_cast<int>(doe) - 719468;
  5406 }
  5407 
  5408 class ArrowReaderState {
! 5409     std::weak_ptr<SqlHandle> handle;
  5410     std::mutex cancelMutex;
  5411     std::atomic<bool> fetching{false};
  5412     bool closeRequested = false;  // Accessed with the GIL held.
  5413     bool cancelSent = false;      // Published by the fetch-completion fence.

Lines 5419-5448

  5419     bool cancelled() const { return cancelSent; }
  5420 
  5421     void requestClose() {
  5422         closeRequested = true;
! 5423         if (!fetching.load())
! 5424             return;
! 5425         auto statement = handle.lock();
! 5426         if (!statement)
! 5427             return;
! 5428         // An idle SQLCancel can close the result set. Do not issue it between
! 5429         // batches (or while the fetch thread is processing a completed rowset).
  5430         py::gil_scoped_release release;
  5431         std::lock_guard<std::mutex> lock(cancelMutex);
  5432         if (fetching.load() && !cancelSent && SQLCancel_ptr) {
! 5433             statement->cancel();
! 5434             cancelSent = true;
! 5435         }
! 5436     }
  5437 
! 5438     SQLRETURN fetch(SQLHSTMT hStmt) {
! 5439         fetching.store(true);
! 5440         py::gil_scoped_release release;
! 5441         SQLRETURN ret = SQLFetch_ptr(hStmt);
  5442         {
  5443             // SQLCancel may still be running after SQLFetch returns. Neither
! 5444             // diagnostics nor another ODBC call may race that cancellation.
  5445             std::lock_guard<std::mutex> lock(cancelMutex);
  5446             fetching.store(false);
  5447         }
  5448         return ret;

Lines 5447-5456

  5447         }
  5448         return ret;
  5449     }
  5450 };
! 5451 
! 5452 struct ArrowFetchDiagnostics {
  5453     SQLSMALLINT cancellationRecord = 0;
  5454     SQLSMALLINT errorRecord = 0;
  5455 };

Lines 5472-5481

  5472             return result;
  5473         }
  5474         auto matches = [&](const char* expected) {
  5475             for (int i = 0; i < 5; ++i) {
! 5476                 if (state[i] != expected[i])
! 5477                     return false;
  5478             }
  5479             return true;
  5480         };
  5481         // A cancel can win just before entry to SQLFetch, closing the cursor.

Lines 5486-5495

  5486              (matches("HY007") || matches("HY010") || matches("24000")))) {
  5487             if (!result.cancellationRecord)
  5488                 result.cancellationRecord = record;
  5489         } else if ((state[0] != '0' || state[1] != '1') && !result.errorRecord) {
! 5490             result.errorRecord = record;
! 5491         }
  5492     }
  5493     if (!result.errorRecord)
  5494         result.errorRecord = 1;
  5495     return result;

Lines 5721-5730

  5721             py::gil_scoped_release release;
  5722             ret = SQLFetch_ptr(hStmt);
  5723         }
  5724         if (ret == SQL_SUCCESS_WITH_INFO || ret == SQL_ERROR) {
! 5725             bool closing = readerState && readerState->closed();
! 5726             auto diagnostics = GetArrowCancellationDiagnostics(
  5727                 hStmt, closing && readerState->cancelled());
  5728             if (diagnostics.cancellationRecord) {
  5729                 CaptureFetchDiagnostics(hStmt, SQL_SUCCESS_WITH_INFO, messages);
  5730                 if (closing && !diagnostics.errorRecord)


📋 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.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python._native_fetch.py: 77.2%
mssql_python.__init__.py: 80.8%
mssql_python.pybind.ddbc_bindings.cpp: 81.4%
mssql_python.pybind.row_mapping_cache.hpp: 81.5%
mssql_python.pybind.connection.connection_pool.cpp: 82.5%
mssql_python.pybind.connection.connection.cpp: 83.1%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Preserve result ownership and unrelated diagnostics, defer reentrant cleanup, and exercise native cancellation boundaries in Windows CI.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 09:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

A stale reader can access or close a replacement statement, and the tests have compatibility, deadlock-timeout, and live-driver cancellation coverage gaps.

2 open issues
3 previously missed issues

In code that hasn't changed since last review

Medium severity Test cancellation of a synchronized in-flight SQLFetch

mssql_python/​pybind/​ddbc_bindings.cpp:5439

The deterministic cancellation tests do not exercise cancellation of an actual in-flight driver fetch: their interposed fetch returns from real_fetch before waiting, and their cancel replaces SQLCancel entirely. The remaining real-driver test still relies on a sleep, so it cannot establish that this call was blocked when close occurred. Please add a synchronized live-driver case that cancels a genuinely outstanding SQLFetch and verifies both termination and cursor reuse.

Medium severity Guard the PyArrow 14-incompatible from_stream cases

tests/​test_004_cursor_arrow.py:570

RecordBatchReader.from_stream was added in PyArrow 15, but the project's PyArrow extra allows version 14. With that supported version, both c_stream cases fail with AttributeError before exercising shutdown. Skip this parameter when from_stream is unavailable, as test_arrow_reader_pycapsule_protocol already does.

Medium severity Bound deadlock-prone concurrency probes with a subprocess timeout

tests/​test_004_cursor_arrow.py:626

If the shutdown code deadlocks, the timed joins expire but the unconditional reader.close() then waits on the same iterator lock indefinitely, so this test never reaches its thread-liveness assertion and can hang the entire test job. Run this concurrency probe in a subprocess with a timeout, as for the native cancellation probe, so a deadlock is reported as a bounded failure.

4 resolved since last review

Review effort: Balanced

Check weak statement identity, bound concurrency probes, support PyArrow 14, and exercise real blocked SQLFetch cancellation before releasing the server lock.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 15:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The interposition tests can run without explicit opt-in and fail for logins lacking DMV permissions, while the cancellation probe rejects a valid successful return code.

4 open issues

Review effort: Balanced

Comment thread tests/test_004_cursor_arrow.py Resolved
Comment thread tests/test_004_cursor_arrow.py Resolved
Copilot AI balanced review requested due to automatic review settings November 13, 2026 05:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The Unix symbol fallback enables a privileged DMV probe without opt-in and can hide missing compiled test controls.

5 open issues

Review effort: Balanced

Comment thread tests/test_004_cursor_arrow.py Resolved
Remove exported-symbol fallback and test both missing-control policies. Exercise SQL_SUCCESS_WITH_INFO after real driver cancellation while preserving the exactly-once assertion.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 13, 2026 06:30
@jahnvi480
Jahnvi Thakkar (jahnvi480) marked this pull request as ready for review November 13, 2026 06:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

A fetch-completion race may obscure diagnostics, while a cleanup test may no longer exercise its intended failure path, warranting human assessment.

2 open issues
3 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 13, 2026 07:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The identity guard causes the cleanup regression test to skip the close-cursor failure path, leaving that behavior unverified.

3 open issues

Review effort: Balanced

Comment thread mssql_python/cursor.py Resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The shutdown implementation addresses the reported interleavings and is consistent with the applicable Arrow reader lifecycle and ODBC cancellation requirements. One Medium regression-coverage gap remains: the new statement-identity guard makes test_arrow_reader_cleanup_survives_close_cursor_failure pass without reaching its injected SQL_CLOSE failure or checking recovery. The existing inline thread has the requested correction: inject failure on the original handle type and assert the close attempt, warning, and bookkeeping reset. No production functional defect was established.

Recommendation: Request Changes
Design compliance: Compliant

Inject failure on the original statement type and assert one close attempt, the warning, bookkeeping reset, idempotent close, and parent cursor recovery. Keep production ownership guards unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 13, 2026 09:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-thread fetch/cancel ordering and diagnostics require live-driver validation, particularly around the completion-to-cancellation window, with Linux/macOS coverage and fresh CI still pending.

No open issues 🎉

3 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 13, 2026 11:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-thread native cancellation and cleanup still need live-driver validation on Linux and macOS, with fresh cross-platform CI pending.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 13, 2026 17:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cross-thread cancellation and statement-lifetime changes require live-driver, cross-platform validation, but CI is pending and the necessary tests cannot run in this environment.

No open issues 🎉

Review effort: Balanced

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

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants