FIX: Preserve Arrow reader fetch exceptions (#712)

mssql_python/cursor.py

@@ -3088,54 +3088,52 @@ def batch_generator():
                 # body.  This is the single canonical cleanup site.
                 cur = cursor_ref[0]
                 cursor_ref[0] = None
-                if cur is None or cur.closed or cur.hstmt is None:
-                    return
-
-                # 1) Drain diagnostics produced by the (possibly cancelled)
-                #    fetch *before* SQL_CLOSE so we don't lose them.
-                try:
-                    cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
-                except Exception as e:  # pylint: disable=broad-exception-caught
-                    logger.debug("arrow_reader cleanup: pre-close diag drain failed: %s", e)
-
-                # 2) Release the server-side cursor & locks while keeping the
-                #    HSTMT and prepared plan intact, so the parent Cursor can
-                #    be re-executed.
-                try:
-                    cur.hstmt._close_cursor()  # pylint: disable=protected-access
-                except Exception as e:  # pylint: disable=broad-exception-caught
-                    # Elevated to WARNING: unlike the diag-drain failures
-                    # (which only cost us some warning text), a failed
-                    # SQLFreeStmt(SQL_CLOSE) leaves the server-side cursor
-                    # and its locks/tempdb resources open on SQL Server
-                    # until this parent Cursor is closed or re-executed.
-                    # DEBUG is typically disabled in production, so that
-                    # leak would be invisible; WARNING makes it visible.
-                    logger.warning(
-                        "arrow_reader cleanup: _close_cursor failed (%s); "
-                        "server-side cursor may remain open until this "
-                        "Cursor is closed or re-executed",
-                        e,
-                    )
-
-                # 3) Drain diagnostics produced by SQL_CLOSE itself.  This
-                #    runs unconditionally because SQL_CLOSE can return
-                #    SQL_SUCCESS_WITH_INFO (a *success* code) and still leave
-                #    warning records on the HSTMT diag stack; the previous
-                #    "only on failure" path would silently drop those.
-                try:
-                    cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
-                except Exception as e:  # pylint: disable=broad-exception-caught
-                    logger.debug("arrow_reader cleanup: post-close diag drain failed: %s", e)
+                if not cur.closed and cur.hstmt is not None:
+                    # 1) Drain diagnostics produced by the (possibly cancelled)
+                    #    fetch *before* SQL_CLOSE so we don't lose them.
+                    try:
+                        cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
+                    except Exception as e:  # pylint: disable=broad-exception-caught
+                        logger.debug("arrow_reader cleanup: pre-close diag drain failed: %s", e)
+
+                    # 2) Release the server-side cursor & locks while keeping the
+                    #    HSTMT and prepared plan intact, so the parent Cursor can
+                    #    be re-executed.
+                    try:
+                        cur.hstmt._close_cursor()  # pylint: disable=protected-access
+                    except Exception as e:  # pylint: disable=broad-exception-caught
+                        # Elevated to WARNING: unlike the diag-drain failures
+                        # (which only cost us some warning text), a failed
+                        # SQLFreeStmt(SQL_CLOSE) leaves the server-side cursor
+                        # and its locks/tempdb resources open on SQL Server
+                        # until this parent Cursor is closed or re-executed.
+                        # DEBUG is typically disabled in production, so that
+                        # leak would be invisible; WARNING makes it visible.
+                        logger.warning(
+                            "arrow_reader cleanup: _close_cursor failed (%s); "
+                            "server-side cursor may remain open until this "
+                            "Cursor is closed or re-executed",
+                            e,
+                        )
 
-                # 4) Reset cursor bookkeeping to a clean "no result set"
-                #    state.  rowcount becomes -1 to signal that the prior
-                #    result is no longer meaningful.
-                try:
-                    cur._clear_rownumber()  # pylint: disable=protected-access
-                    cur.rowcount = -1
-                except Exception as e:  # pylint: disable=broad-exception-caught
-                    logger.debug("arrow_reader cleanup: bookkeeping reset failed: %s", e)
+                    # 3) Drain diagnostics produced by SQL_CLOSE itself.  This
+                    #    runs unconditionally because SQL_CLOSE can return
+                    #    SQL_SUCCESS_WITH_INFO (a *success* code) and still leave
+                    #    warning records on the HSTMT diag stack; the previous
+                    #    "only on failure" path would silently drop those.
+                    try:
+                        cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
+                    except Exception as e:  # pylint: disable=broad-exception-caught
+                        logger.debug("arrow_reader cleanup: post-close diag drain failed: %s", e)
+
+                    # 4) Reset cursor bookkeeping to a clean "no result set"
+                    #    state.  rowcount becomes -1 to signal that the prior
+                    #    result is no longer meaningful.
+                    try:
+                        cur._clear_rownumber()  # pylint: disable=protected-access
+                        cur.rowcount = -1
+                    except Exception as e:  # pylint: disable=broad-exception-caught
+                        logger.debug("arrow_reader cleanup: bookkeeping reset failed: %s", e)
 
         gen = batch_generator()
         inner = pyarrow.RecordBatchReader.from_batches(schema, gen)

tests/test_004_cursor.py

@@ -10,7 +10,9 @@
 
 import pytest
 import os
+import warnings
 from datetime import datetime, date, time, timedelta, timezone
+from pathlib import Path
 import time as time_module
 import decimal
 from contextlib import closing
@@ -108,6 +110,15 @@
 ]
 
 
+def test_package_sources_compile_with_warnings_as_errors():
+    """Every package source must compile when warnings are promoted to errors."""
+    package_dir = Path(__file__).parents[1] / "mssql_python"
+    for source in sorted(package_dir.glob("*.py")):
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            compile(source.read_text(encoding="utf-8"), str(source), "exec")
+
+
 def drop_table_if_exists(cursor, table_name):
     """Drop the table if it exists"""
     try:

tests/test_004_cursor_arrow.py

@@ -688,6 +688,21 @@ def close(self):
 #     leak the server-side cursor or crash close()
 
 
+def test_arrow_reader_propagates_fetch_error_when_parent_cursor_is_closed(conn_str):
+    """Closing the parent cursor must not turn the next fetch error into end-of-stream."""
+    conn = mssql_python.connect(conn_str)
+    try:
+        tmp = conn.cursor()
+        reader = tmp.execute("select top 5 1 a from sys.objects").arrow_reader(batch_size=2)
+        _ = reader.read_next_batch()
+        tmp.close()
+
+        with pytest.raises(mssql_python.ProgrammingError, match="cursor is closed"):
+            reader.read_next_batch()
+    finally:
+        conn.close()
+
+
 def test_arrow_reader_getattr_refuses_private_names(cursor: mssql_python.Cursor):
     """__getattr__ refuses leading-underscore names so a partially-constructed
     instance during __del__ cannot recurse forever trying to resolve its own