Skip to content

Commit 47af746

Browse files
committed
test: stop SIGKILLing the shared launcher JVM; run the lane in parallel
This lane was pinned serial because -j 6 reproducibly failed 8 table_buffering files, which read as a java-worker concurrency bug. It was not one. There is no race, no shared static, no SQLite contention — the framework is clean. FIXTURE_ENV set VGI_TEST_DEDICATED_WORKER unconditionally. That var gates table_buffering_{worker_crash,pool_recovery}.test, whose `crash_on_process` fixture runs `kill -9 <own pid>` and halt(137) by design. Both files ATTACH ${VGI_TEST_WORKER} — the launch: location — not the dedicated binary, so the victim was the ONE shared launcher JVM every concurrent DuckDB process was talking to. The survivors reported "RPC response stream EOF" / "Broken pipe", i.e. the collateral damage. Serially it hid: the next ATTACH respawns the JVM, and the two crash tests even "passed" while silently testing launcher respawn instead of subprocess-pool recovery. run_tests.py deliberately leaves that var unset for shared-worker transports (its env.setdefault only fires for a bare-path VGI_TEST_WORKER), and setting it here defeated the guard. ci/run-integration.sh had the same defect on its launch lane while its http lane already got it right. So: don't set it (the two files self-skip, already allow-listed in the coverage gate), add a `test-crash` target running exactly those two over the SUBPROCESS transport where each DuckDB process owns a worker it can watch die, and run it after the main suite. Same step added to ci/run-integration.sh's launch lane. JAVA_JOBS 1 -> 6. Verified: 293 passed, 29 skipped, 0 failed, plus 2 from test-crash. Coverage goes UP (294 executed before, 295 now) and the two crash tests finally exercise what their headers describe. ~87s warm vs ~1820s serial.
1 parent dc264c0 commit 47af746

2 files changed

Lines changed: 63 additions & 22 deletions

File tree

Makefile

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,18 @@ BAD_PROTOCOL_LOCATION := $(LAUNCHER_PREFIX)$(WRAPPERS)/vgi-worker-bad-protoc
3939
# Every fixture-worker variable the shared suite reads, in one place so `test`
4040
# and `test-single` cannot drift apart.
4141
#
42-
# VGI_TEST_DEDICATED_WORKER is deliberately NOT a launch: location — the
43-
# crash/pool-recovery tests need a worker this process owns and can watch die,
44-
# which a shared launcher worker is not.
42+
# VGI_TEST_DEDICATED_WORKER is deliberately NOT set here, and setting it is a
43+
# bug. It gates table_buffering_{worker_crash,pool_recovery}.test, whose
44+
# `crash_on_process` fixture SIGKILLs the worker serving it — but both files
45+
# ATTACH the VGI_TEST_WORKER location, NOT the dedicated binary, so on this lane the
46+
# victim is the ONE shared launcher JVM. Serially that is survivable (the next
47+
# ATTACH respawns it); under -j it tears the worker out from under every
48+
# concurrent DuckDB process, which is exactly the 8-file "table_buffering fails
49+
# at -j 6" symptom this lane was pinned serial for. run_tests.py already
50+
# leaves the var unset for shared-worker transports (launch:/unix:///http://) —
51+
# see its `env.setdefault` guard — and setting it here defeated that. The two
52+
# files self-skip instead (allow-listed in COVERAGE_GATE below); vgi-go and
53+
# vgi-typescript do the same on their launcher lanes.
4554
#
4655
# VGI_ATTACH_OPTIONS_REQUIRED_WORKER is the same wrapper as
4756
# VGI_ATTACH_OPTIONS_WORKER: that worker serves the `attach_options_required`
@@ -57,7 +66,6 @@ FIXTURE_ENV := \
5766
VGI_WORKER_BIN=$(EXAMPLE_WORKER) \
5867
VGI_TEST_BRANCH_DIR=$(VGI_TEST_BRANCH_DIR) \
5968
VGI_TEST_WORKER=$(EXAMPLE_LOCATION) \
60-
VGI_TEST_DEDICATED_WORKER=$(EXAMPLE_WORKER) \
6169
VGI_VERSIONED_WORKER=$(VERSIONED_LOCATION) \
6270
VGI_VERSIONED_TABLES_WORKER=$(VERSIONED_TABLES_LOCATION) \
6371
VGI_ATTACH_OPTIONS_WORKER=$(ATTACH_OPTIONS_LOCATION) \
@@ -66,7 +74,7 @@ FIXTURE_ENV := \
6674
VGI_BAD_PROTOCOL_WORKER=$(BAD_PROTOCOL_LOCATION) \
6775
VGI_REQUIRE_LAUNCHER_TRANSPORT=1
6876

69-
.PHONY: build smoke test test-single clean
77+
.PHONY: build smoke test test-crash test-single clean
7078

7179
## Build all worker dist images.
7280
build:
@@ -125,19 +133,15 @@ smoke: build
125133
# Java runs 294 today.
126134
JAVA_MIN_EXECUTED ?= 290
127135

128-
# Serial, deliberately. This lane ran under `unittest -f <filelist>` (one file
129-
# at a time) before it moved to run_tests.py, and it stays that way because
130-
# raising it to -j 6 fails 8 table_buffering files — reproducibly, and only
131-
# under concurrency: the same 16 files pass at -j 1.
132-
#
133-
# That is a java-worker bug, not a property of the launcher transport, and it
134-
# should be fixed rather than lived with: vgi-typescript runs the same suite
135-
# over the same `launch:` transport at JOBS=8 and is green, so one shared
136-
# launcher worker serving several concurrent DuckDB processes is supported.
137-
# Java's table-buffering path apparently is not safe across them.
138-
#
139-
# Bump this to 6 once that is fixed — the lane is minutes slower serially.
140-
JAVA_JOBS ?= 1
136+
# Parallel. This lane was pinned serial because raising it to -j 6 failed 8
137+
# table_buffering files reproducibly, which read as a java-worker concurrency
138+
# bug. It was not: FIXTURE_ENV set VGI_TEST_DEDICATED_WORKER, which un-skipped
139+
# table_buffering_{worker_crash,pool_recovery}.test, whose crash_on_process
140+
# fixture SIGKILLs the ONE shared launcher JVM every concurrent DuckDB process
141+
# is talking to. The survivors reported "RPC response stream EOF" / "Broken
142+
# pipe" — i.e. the collateral damage, not a race of their own. See the
143+
# VGI_TEST_DEDICATED_WORKER note above.
144+
JAVA_JOBS ?= 6
141145
COVERAGE_GATE := --min-executed $(JAVA_MIN_EXECUTED) \
142146
--allow-skip 'require spatial' \
143147
--allow-skip 'require-env VGI_DOCKER_IMAGE' \
@@ -166,6 +170,21 @@ test: build
166170
@cd $(HOME)/Development/vgi && $(FIXTURE_ENV) \
167171
python3 scripts/run_tests.py -j $(JAVA_JOBS) $(COVERAGE_GATE) \
168172
"test/sql/integration/*" "~test/sql/integration/simple_writable/*"
173+
@$(MAKE) --no-print-directory test-crash
174+
175+
## The two crash files the main lane necessarily skips, run the only way they
176+
## are meaningful: over the SUBPROCESS transport, where VGI_TEST_WORKER is a
177+
## bare path and every DuckDB process forks its own private worker child. Both
178+
## ATTACH ${VGI_TEST_WORKER} and then have the worker SIGKILL itself, so on the
179+
## launcher transport the victim is the shared JVM (see the note above) — the
180+
## main lane leaves VGI_TEST_DEDICATED_WORKER unset so they self-skip there, and
181+
## this target is what actually exercises them. run_tests.py derives
182+
## VGI_TEST_DEDICATED_WORKER itself from the bare path, so it is not set here.
183+
test-crash: build
184+
@cd $(HOME)/Development/vgi && VGI_TEST_WORKER=$(EXAMPLE_WORKER) \
185+
python3 scripts/run_tests.py -j 2 --min-executed 2 \
186+
"test/sql/integration/table_in_out/table_buffering_worker_crash.test" \
187+
"test/sql/integration/table_in_out/table_buffering_pool_recovery.test"
169188

170189
## Run a single sqllogictest by file name.
171190
test-single: build

ci/run-integration.sh

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,19 @@ boot_http_worker() {
116116

117117
case "$TRANSPORT" in
118118
launch)
119-
# VGI_TEST_DEDICATED_WORKER is a plain (non-pooled) worker for the crash/
120-
# pool-recovery tests; the three wrappers route the one binary into the
121-
# versioned / versioned_tables / attach_options catalogs.
119+
# The three wrappers route the one binary into the versioned /
120+
# versioned_tables / attach_options catalogs.
121+
#
122+
# NB: VGI_TEST_DEDICATED_WORKER stays UNSET here, for the same reason it is
123+
# unset on the http lane below. It used to be exported as the bare binary
124+
# path, on the theory that it named a private worker for the crash tests —
125+
# but table_buffering_{worker_crash,pool_recovery}.test ATTACH
126+
# ${VGI_TEST_WORKER}, not the dedicated path, so setting the var merely
127+
# un-skipped a fixture (crash_on_process) that SIGKILLs the ONE shared
128+
# launcher JVM serving the whole run. Serially that only cost the launcher a
129+
# respawn; run the suite with any parallelism and it takes every concurrent
130+
# DuckDB process down with it.
122131
export VGI_TEST_WORKER="launch:${VGI_WORKER_BIN}"
123-
export VGI_TEST_DEDICATED_WORKER="${VGI_WORKER_BIN}"
124132
export VGI_VERSIONED_WORKER="launch:${HERE}/wrappers/vgi-worker-versioned"
125133
export VGI_VERSIONED_TABLES_WORKER="launch:${HERE}/wrappers/vgi-worker-versioned-tables"
126134
export VGI_ATTACH_OPTIONS_WORKER="launch:${HERE}/wrappers/vgi-worker-attach-options"
@@ -357,6 +365,20 @@ if [ "$TRANSPORT" = "launch" ]; then
357365
run_unittest "test/sql/integration/http/gzip_fallback.test" )
358366
fi
359367

368+
# The two buffering crash files skip on every shared-worker transport (their
369+
# crash_on_process fixture SIGKILLs whatever worker serves it). They are only
370+
# meaningful over the SUBPROCESS transport, where VGI_TEST_WORKER is a bare path
371+
# and each DuckDB process forks a private worker child it can watch die — so
372+
# they get their own invocation with the bare binary, exactly like gzip_fallback
373+
# above gets its own http worker. Same subshell isolation: MAIN_EXECUTED and the
374+
# floor below are the main suite's, not this run's.
375+
if [ "$TRANSPORT" = "launch" ]; then
376+
echo "Running the buffering crash files (subprocess transport, private worker) ..."
377+
( export VGI_TEST_WORKER="$VGI_WORKER_BIN" VGI_TEST_DEDICATED_WORKER="$VGI_WORKER_BIN"
378+
run_unittest "test/sql/integration/table_in_out/table_buffering_worker_crash.test"
379+
run_unittest "test/sql/integration/table_in_out/table_buffering_pool_recovery.test" )
380+
fi
381+
360382
# Executed-case floor on the main suite — the collapse-detector. Reached only if
361383
# the runs above passed (a failure already aborted via set -e); MAIN_EXECUTED
362384
# holds the main invocation's count (the gzip subshell does not touch it).

0 commit comments

Comments
 (0)