bulkcopy API rejects timeout=0 despite spec and py-core both defining it as "no timeout"

[bewithgaurav]

Describe the bug

Cursor.bulkcopy() and Cursor.bulkcopy_arrow() reject timeout=0, but 0 is documented as "no timeout" in the BCP API spec (discussion #414) and is explicitly implemented as infinite in mssql-py-core. The rejection is a validation bound in the Python layer only.

Reported by @drienkop on #414.

Exception message: ValueError: timeout must be positive, got 0
Stack trace: raised by mssql_python/cursor.py before any py-core call

The guard:

if timeout <= 0:
    raise ValueError(f"timeout must be positive, got {timeout}")

This is a day 0 bug, and not a regression.
if timeout <= 0 has been there in cursor.py since BCP landed in #402.
The guard now exists in two places, since bulkcopy_arrow (#665) copied it.

To reproduce

import mssql_python

conn = mssql_python.connect(CONNECTION_STRING)
cur = conn.cursor()
cur.execute("CREATE TABLE dbo.t0 (id INT, name NVARCHAR(20))")
conn.commit()

cur.bulkcopy("dbo.t0", [(1, "a")], timeout=0)
# ValueError: timeout must be positive, got 0

py-core itself has no problem with 0. Bypassing the Python validator and calling PyCoreCursor.bulkcopy directly:

timeout=30 (baseline)    -> OK rows_copied=50,000  in 0.172s
timeout=0                -> OK rows_copied=50,000  in 0.113s
timeout=-1               -> OverflowError: can't convert negative int to unsigned
400k rows with timeout=0 -> OK rows_copied=400,000 in 0.570s

A 0ms deadline would have failed instantly. It did not.

Expected behavior

timeout=0 is accepted and means no timeout, matching the spec and the core implementation. Negative values stay rejected, since py-core takes a u32.

mssql-rs implements this deliberately at every layer.

mssql-tds/src/connection/bulk_copy_state.rs:

/// A value of 0 means infinite timeout.
pub fn from_seconds(timeout_sec: u32) -> Self {
    if timeout_sec == 0 {
        Self::new(None)          // None = no deadline
    } else {
        Self::new(Some(Duration::from_secs(timeout_sec as u64)))
    }
}

pub fn is_expired(&self) -> bool {
    self.deadline.is_some_and(|d| Instant::now() >= d)   // no deadline -> never expires
}

mssql-tds/src/connection/bulk_copy.rs:

// Initialize timeout state for this operation
// A timeout of 0 means infinite (no timeout)
self.timeout_state = Some(BulkCopyTimeoutState::from_seconds(self.options.timeout_sec));

with unit tests asserting it (test_timeout_state_from_seconds, test_infinite_timeout_never_expires, test_default_is_infinite):

// Zero means infinite
let infinite = BulkCopyTimeoutState::from_seconds(0);
assert!(!infinite.is_expired());
assert!(infinite.remaining_ms().is_none());

And mssql-py-core/src/cursor.rs forwards the value unchanged (timeout: Duration::from_secs(timeout)), so a 0 reaches from_seconds(0) intact.

Suggested fix, mirroring what batch_size already does, in both bulkcopy and bulkcopy_arrow:

if not isinstance(timeout, int):
    raise TypeError(f"timeout must be a non-negative integer, got {type(timeout).__name__}")
if timeout < 0:
    raise ValueError(f"timeout must be non-negative, got {timeout}")

Verified locally against SQL Server 2022:

timeout=0   bulkcopy -> OK rows=1
timeout=30  bulkcopy -> OK rows=1
timeout=-1  bulkcopy -> ValueError: timeout must be non-negative, got -1
timeout=1.5 bulkcopy -> TypeError: timeout must be a non-negative integer, got float
timeout=0   arrow    -> OK rows=1
timeout=-1  arrow    -> ValueError: timeout must be non-negative, got -1

84/84 pass across test_019, test_020 and test_024. black clean.

Further technical details

Python version: 3.13.11
SQL Server version: SQL Server 2022 (RTM-CU18) 16.0.4185.3
Operating system: macOS 26.5.2 (arm64), SQL Server in Docker

Revised Additional context section:

Additional context

This is the bulkcopy operation timeout only. connect_timeout is a separate parameter on a different code path, and _build_pycore_context deliberately leaves it unset when 0 for its own reasons. Those reasons don't apply here, so the two shouldn't be reconciled as one change.

The docstrings for both methods say "Operation timeout in seconds. Default is 30" and should gain the 0 = no timeout note as part of the fix.

Note that tests/test_024_bulkcopy_arrow.py::test_timeout_non_positive currently asserts the buggy behavior and needs to change, splitting into a negative-rejected case and a zero-accepted case.

[github-actions]

Hi @bewithgaurav, thank you for opening this issue!

Our team will review it shortly. We aim to triage all new issues within 24-48 hours and get back to you.

If you have additional information to share, please feel free to update the issue.

Thank you for your patience!