Skip to content

Controller tests: stop the new DB race test hanging CI when it fires - #1053

Open
forming wants to merge 1 commit into
wilbowes:mainfrom
forming:fix/controller-tests-db-race-hang
Open

forming wants to merge 1 commit into
wilbowes:mainfrom
forming:fix/controller-tests-db-race-hang

Conversation

@forming

@forming forming commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

One test in controller/tests/test_db_concurrency.py could hang CI for hours exactly when it detects the bug it exists to detect.

What

test_reads_racing_writes_do_not_raise_sqlite_misuse joins the eight writer futures with f.result(timeout=120), and called stop.set() only after that loop, inside the with concurrent.futures.ThreadPoolExecutor(max_workers=16) block:

for f in writer_futures:
    f.result(timeout=120)
stop.set()

Why it hangs

A writer that exceeds its timeout — which is exactly what a reintroduced shared-connection race produces, since the unguarded read blocks inside somebody's transaction — raises TimeoutError out of f.result(). That exception leaves the with block without ever reaching stop.set(), so ThreadPoolExecutor.__exit__ calls shutdown(wait=True), which joins all sixteen worker threads. The eight writers are done or dying, but the eight readers loop on while not stop.is_set() and only exit once stop is set. stop is never set. shutdown waits on them forever, so the run hangs until GitHub's job timeout (hours) rather than failing in two minutes with the assertion that names the bug.

The failure mode is inverted: the more reliably the test detects a regression, the less likely CI is to report it.

Fix

Set stop in a finally, so the readers are released whether the writers succeeded or timed out:

try:
    for f in writer_futures:
        f.result(timeout=120)
finally:
    stop.set()
for f in reader_futures:
    f.result(timeout=120)

The TimeoutError still propagates — the test still fails — but it now fails instead of wedging. This is the same shape the second test in the file, test_reads_see_committed_writes_only, already uses.

Verification

Full file, unmodified timeouts:

tests/test_db_concurrency.py .......        [100%]
7 passed in 3.20s

Temporary local patch (never committed, reverted afterwards) to force the timeout path: timeout=120 → timeout=0.01 and time.sleep(0.2) as the first line of writer.

Before (pristine main, same patch), killed by a 20s watchdog:

$ run_with_timeout 20 pytest tests/test_db_concurrency.py -q -k reads_racing_writes
>> HUNG: killed after 20s
EXIT=124

After (this commit, same patch):

>               f.result(timeout=0.01)
E               TimeoutError
FAILED tests/test_db_concurrency.py::test_reads_racing_writes_do_not_raise_sqlite_misuse
1 failed, 6 deselected in 1.23s
EXIT=1

So the timeout path now reports in ~1s instead of hanging. The committed diff is only the try/finally.

Full suite (controller/tests/):

2172 passed, 55 skipped, 1 deselected in 38.52s

The deselected test is tests/test_emos_update.py::test_trialcheck_passes, which compiles emos/init/trialcheck.c with the host cc. It fails identically on pristine main on macOS because the source includes sys/sysmacros.h, a glibc header that does not exist there; it is unrelated to this change and passes on the Linux CI image.

…stead of hanging

test_reads_racing_writes_do_not_raise_sqlite_misuse joined the writer futures
before setting stop. A writer timeout — exactly what a reintroduced SQLite race
produces — left the with-block with the readers still spinning on an event that
was never set, and ThreadPoolExecutor.__exit__ then blocked in shutdown(wait=True)
joining them, so the run hung until the job timeout instead of failing in two
minutes. Move stop.set() into a finally, matching the second test in the file.
@forming
forming requested a review from wilbowes as a code owner October 26, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant