Skip to content

Commit 40891ac

Browse files
committed
test: correct the "java concurrency bug" note, and run the crash tests properly
I diagnosed this wrong. The eight table_buffering files that failed at -j 6 were not a concurrency bug in the buffered path, and not specific to any SDK. table_buffering_{worker_crash,pool_recovery}.test are gated on VGI_TEST_DEDICATED_WORKER because their `crash_on_process` fixture SIGKILLs the worker serving it, and both files ATTACH ${VGI_TEST_WORKER} — the LAUNCHER location on these lanes, not a dedicated binary. Setting the gate var while VGI_TEST_WORKER is a shared-worker location un-skips them, so the SIGKILL takes out the one process every concurrent DuckDB is talking to. The other files then report "RPC response stream EOF" / "Broken pipe": collateral damage, not a race of their own. Serially it hides, because the launcher just respawns the worker for the next test. run_tests.py already leaves the var unset for shared-worker transports (its env.setdefault only fires for a bare-path VGI_TEST_WORKER); vgi-java's Makefile was setting it unconditionally and defeating that, which is the actual bug and is fixed in that repo. My own attempt to wire it into test_python hit exactly the same wall, and I recorded the wrong reason for backing it out. So: correct the comment, and stop losing the two tests. test_python_crash runs them over the SUBPROCESS transport, where each DuckDB process owns a private worker it can watch die — the shape they were written for — and test_python chains it after the main suite. Same fix vgi-java applied. 2 passed.
1 parent 61eecc1 commit 40891ac

1 file changed

Lines changed: 32 additions & 9 deletions

File tree

‎Makefile‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ VGI_TS_DIR ?= $(HOME)/Development/vgi-typescript
413413
VGI_JAVA_DIR ?= $(HOME)/vgi-java
414414
VGI_RUST_DIR ?= $(HOME)/Development/vgi-rust
415415

416-
.PHONY: test_python test_go test_typescript test_java test_rust test_languages
416+
.PHONY: test_python test_python_crash test_go test_typescript test_java test_rust test_languages
417417

418418
# Python uses this repo's default worker set. It does NOT just chain to
419419
# test_launcher, which invokes `unittest` bare: a lane that stops running tests
@@ -425,14 +425,25 @@ VGI_RUST_DIR ?= $(HOME)/Development/vgi-rust
425425
#
426426
# Python runs 304 today.
427427
#
428-
# VGI_TEST_DEDICATED_WORKER is DECLARED as an expected skip rather than wired.
429-
# Two tests want a non-launcher worker, and pointing the var at the raw `uv run`
430-
# command does make them run — but the extra interpreter spawns push this lane
431-
# over some threshold and eight table_buffering files start failing. That is the
432-
# same cluster that fails in the java lane at -j 6 (java therefore runs serial;
433-
# see its Makefile), so it is a real concurrency bug in the buffered path under
434-
# a shared launcher worker, worth fixing on its own. Until it is, this lane does
435-
# not trade 2 executed tests for 8 flaky ones.
428+
# VGI_TEST_DEDICATED_WORKER is DECLARED as an expected skip, not wired — and
429+
# wiring it here would be a BUG, not a coverage win.
430+
#
431+
# It gates table_buffering_{worker_crash,pool_recovery}.test, whose
432+
# `crash_on_process` fixture SIGKILLs the worker serving it. Both files ATTACH
433+
# ${VGI_TEST_WORKER} — the LAUNCHER location on this lane — not a dedicated
434+
# binary, so the victim is the ONE shared worker every concurrent DuckDB process
435+
# is talking to. Setting the var un-skips them and the survivors then fail with
436+
# "RPC response stream EOF" / "Broken pipe": collateral damage, not a race of
437+
# their own. That is exactly the eight-file failure I first mistook for a
438+
# concurrency bug in the buffered path; it is neither a concurrency bug nor
439+
# specific to any SDK. run_tests.py deliberately leaves the var unset for
440+
# shared-worker transports (see its env.setdefault guard), and setting it here
441+
# defeats that.
442+
#
443+
# To actually run those two, point VGI_TEST_WORKER itself at a bare path so each
444+
# DuckDB process owns a private worker it can watch die — see vgi-java's
445+
# `test-crash` target, which runs exactly those files over the subprocess
446+
# transport after the main suite.
436447
VGI_PYTHON_MIN_EXECUTED ?= 300
437448
test_python:
438449
VGI_TRANSACTOR_DB_DIR="$$(mktemp -d)" \
@@ -452,6 +463,18 @@ test_python:
452463
"test/*" \
453464
"~test/sql/integration/writable/*" \
454465
"~test/sql/vgi_worker_pool.test"
466+
@$(MAKE) test_python_crash
467+
468+
# The two crash/pool-recovery files the launcher lane must skip, run over the
469+
# SUBPROCESS transport where each DuckDB process owns a private worker it can
470+
# watch die. That is the shape they were written for; on a shared-worker lane
471+
# they would SIGKILL the process serving every concurrent test. Mirrors
472+
# vgi-java's `test-crash`.
473+
test_python_crash:
474+
VGI_TEST_WORKER="$(VGI_TEST_WORKER)" \
475+
python3 scripts/run_tests.py -j 2 --min-executed 2 \
476+
"test/sql/integration/table_in_out/table_buffering_worker_crash.test" \
477+
"test/sql/integration/table_in_out/table_buffering_pool_recovery.test"
455478

456479
test_go:
457480
$(MAKE) -C $(VGI_GO_DIR) test

0 commit comments

Comments
 (0)