Skip to content

Commit f12422e

Browse files
rustyconoverclaude
andcommitted
fix(client): clean up parallel table-function scans closed early
_table_function_parallel reads each worker's producer stream on a reader thread. Closing the generator before the scan finished left those threads reading and the streams open; the Client.stop() that followed drained the same stream from the main thread. Two readers on one pipe hung about one run in five, and a pooled worker abandoned mid-stream corrupted the next call on it (TransportError reading `bind`). The generator body now runs in try/finally. On an early exit (generator closed, or a worker failed) it sets a stop event; each reader notices it within one batch (producer streams advance one tick per batch) and cancels its stream from its own thread, so the stream never has two readers. The threads are joined, then streams and secondary workers released as on normal completion. A reader still alive after the join timeout gets its worker killed rather than a second reader on the pipe. Found by vgi-polars (scans cut short by .head() across splits). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent d074c60 commit f12422e

2 files changed

Lines changed: 147 additions & 36 deletions

File tree

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
# Copyright 2025, 2026 Query Farm LLC - https://query.farm
2+
3+
"""Closing a ``table_function`` generator before its stream ends must not hang.
4+
5+
``Client.table_function`` reads each worker's producer stream on a background
6+
thread. Before the fix, closing the generator early left those threads reading,
7+
and the ``Client.stop()`` that followed drained the same stream from the main
8+
thread: two readers on one pipe, and whichever missed the end of stream blocked
9+
forever (roughly one run in five). Each case here repeats the early close under
10+
a watchdog, then checks the pooled worker still serves a complete scan.
11+
"""
12+
13+
from __future__ import annotations
14+
15+
import threading
16+
from collections.abc import Callable
17+
18+
import pyarrow as pa
19+
import pytest
20+
21+
from tests.conftest import SUBPROCESS_FIXTURE_WORKER
22+
from vgi.arguments import Arguments
23+
from vgi.client.client import Client, _default_pool
24+
25+
_ITERATIONS = 25
26+
_WATCHDOG_SECONDS = 120.0
27+
28+
29+
def _run_with_watchdog(body: Callable[[], None]) -> None:
30+
"""Run ``body`` on a daemon thread; fail (rather than hang the suite) if it doesn't finish."""
31+
errors: list[BaseException] = []
32+
33+
def target() -> None:
34+
try:
35+
body()
36+
except BaseException as e: # noqa: BLE001 - re-raised on the test thread below
37+
errors.append(e)
38+
39+
thread = threading.Thread(target=target, daemon=True)
40+
thread.start()
41+
thread.join(_WATCHDOG_SECONDS)
42+
assert not thread.is_alive(), "closing a table_function scan early hung"
43+
if errors:
44+
raise errors[0]
45+
46+
47+
def _close_after_first_batch(function_name: str, arguments: Arguments) -> None:
48+
for _ in range(_ITERATIONS):
49+
with Client(SUBPROCESS_FIXTURE_WORKER, pool=_default_pool) as client:
50+
scan = client.table_function(function_name=function_name, schema_path=["main"], arguments=arguments)
51+
first = next(scan)
52+
assert first.num_rows > 0
53+
scan.close()
54+
55+
56+
def _full_scan_still_works() -> None:
57+
with Client(SUBPROCESS_FIXTURE_WORKER, pool=_default_pool) as client:
58+
batches = list(
59+
client.table_function(
60+
function_name="sequence",
61+
schema_path=["main"],
62+
arguments=Arguments(positional=(pa.scalar(4),)),
63+
)
64+
)
65+
assert pa.Table.from_batches(batches).column("n").to_pylist() == [0, 1, 2, 3]
66+
67+
68+
@pytest.mark.parametrize(
69+
("function_name", "arguments"),
70+
[
71+
# One worker, many small batches.
72+
("sequence", Arguments(positional=(pa.scalar(100_000),), named={"batch_size": pa.scalar(10)})),
73+
# Several workers (one per partition), each with batches still to come.
74+
("partitioned_sequence", Arguments(positional=(pa.scalar(30_000),))),
75+
],
76+
ids=["single_worker", "multi_worker"],
77+
)
78+
def test_early_close_does_not_hang(function_name: str, arguments: Arguments) -> None:
79+
"""A scan closed after its first batch stops cleanly, and the worker pool stays usable."""
80+
_run_with_watchdog(lambda: _close_after_first_batch(function_name, arguments))
81+
_run_with_watchdog(_full_scan_still_works)

‎vgi/client/client.py‎

Lines changed: 66 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3273,22 +3273,34 @@ def _table_function_parallel(
32733273
# AnnotatedBatch (not just .batch) so custom_metadata survives the
32743274
# cross-thread handoff for batch_metadata_callback below.
32753275
output_queue: Queue[AnnotatedBatch | BaseException | None] = Queue()
3276+
# Set when the consumer stops before every worker finished (the
3277+
# generator was closed early, or a worker failed). Each reader thread is
3278+
# its stream's only reader, so it is also the one that ends the stream:
3279+
# a producer stream advances one tick per batch, so a reader notices the
3280+
# flag within one batch and cancels from its own thread. Ending it from
3281+
# the consumer's thread instead would put two readers on one pipe.
3282+
stop = threading.Event()
32763283

32773284
def read_worker_output(worker: WorkerConnection) -> None:
32783285
"""Thread function that reads all output from a single worker."""
32793286
try:
3280-
if worker.stream is None:
3287+
stream = worker.stream
3288+
if stream is None:
32813289
output_queue.put(None)
32823290
return
32833291

3284-
for output in worker.stream:
3292+
for output in stream:
32853293
_logger.debug(
32863294
"received_output_from_worker worker_index=%s num_rows=%s",
32873295
worker.worker_index,
32883296
output.batch.num_rows,
32893297
)
32903298
if output.batch.num_rows > 0:
32913299
output_queue.put(output)
3300+
if stop.is_set():
3301+
_logger.debug("table_function_worker_cancelled worker_index=%s", worker.worker_index)
3302+
stream.cancel()
3303+
break
32923304

32933305
output_queue.put(None) # Signal completion
32943306
except StopIteration:
@@ -3308,41 +3320,59 @@ def read_worker_output(worker: WorkerConnection) -> None:
33083320
thread.start()
33093321
threads.append(thread)
33103322

3311-
# Collect outputs from all workers until all are done
3312-
workers_finished = 0
3313-
while workers_finished < num_workers:
3314-
result = output_queue.get()
3315-
3316-
# Check for exceptions from worker threads
3317-
if isinstance(result, BaseException):
3318-
if isinstance(result, RpcError):
3319-
raise ClientError.from_rpc_error(result) from result
3320-
raise ClientError(f"Worker thread failed: {result}") from result
3321-
3322-
# None signals a worker finished
3323-
if result is None:
3324-
workers_finished += 1
3325-
_logger.debug(
3326-
"worker_finished workers_finished=%s total_workers=%s",
3327-
workers_finished,
3328-
num_workers,
3329-
)
3330-
continue
3331-
3332-
if batch_metadata_callback is not None:
3333-
batch_metadata_callback(result.custom_metadata)
3334-
yield result.batch
3335-
3336-
self._join_threads(threads)
3337-
_logger.debug("all_table_function_workers_complete")
3323+
# Collect outputs from all workers until all are done. The finally
3324+
# block also runs when the consumer closes the generator early
3325+
# (GeneratorExit at the yield) or a worker fails: the reader threads are
3326+
# told to stop and joined before any stream is touched here.
3327+
completed = False
3328+
try:
3329+
workers_finished = 0
3330+
while workers_finished < num_workers:
3331+
result = output_queue.get()
3332+
3333+
# Check for exceptions from worker threads
3334+
if isinstance(result, BaseException):
3335+
if isinstance(result, RpcError):
3336+
raise ClientError.from_rpc_error(result) from result
3337+
raise ClientError(f"Worker thread failed: {result}") from result
3338+
3339+
# None signals a worker finished
3340+
if result is None:
3341+
workers_finished += 1
3342+
_logger.debug(
3343+
"worker_finished workers_finished=%s total_workers=%s",
3344+
workers_finished,
3345+
num_workers,
3346+
)
3347+
continue
33383348

3339-
# Close streams and secondary workers
3340-
for worker in all_workers:
3341-
if worker.stream is not None:
3342-
worker.stream.close()
3343-
worker.stream = None
3344-
self._close_secondary_workers()
3345-
_logger.debug("parallel_table_function_complete")
3349+
if batch_metadata_callback is not None:
3350+
batch_metadata_callback(result.custom_metadata)
3351+
yield result.batch
3352+
completed = True
3353+
finally:
3354+
if not completed:
3355+
stop.set()
3356+
self._join_threads(threads)
3357+
_logger.debug("all_table_function_workers_complete completed=%s", completed)
3358+
3359+
# Close streams and secondary workers. A stream a reader cancelled is
3360+
# already closed, so close() is a no-op for it.
3361+
for worker, thread in zip(all_workers, threads, strict=True):
3362+
if thread.is_alive():
3363+
# Still reading after the join timeout: the stream's state is
3364+
# unknown and its reader still owns it. Kill the worker so
3365+
# neither that reader nor a later stop() blocks on the pipe.
3366+
_logger.warning("table_function_worker_killed worker_index=%s", worker.worker_index)
3367+
if worker.proc is not None:
3368+
worker.proc.kill()
3369+
worker.stream = None
3370+
continue
3371+
if worker.stream is not None:
3372+
worker.stream.close()
3373+
worker.stream = None
3374+
self._close_secondary_workers()
3375+
_logger.debug("parallel_table_function_complete")
33463376

33473377
# ==================================================================
33483378
# Custom COPY formats

0 commit comments

Comments
 (0)