[el-marvin]

A flush from a second thread, racing an execute on the same connection, sometimes came back with the engine's message instead of the client's:

ResourceInUseException: connection has an open result; drain or destroy it before starting a new query

The appender tests saw it in CI as a flake. Reproducing it showed worse outcomes of the same race. With a second thread retrying con.table("t") or app.flush() without pause while the first thread runs SELECT hold(i) FROM range(1) t(i) with a Python UDF that waits, the process hung within a few iterations: the first thread in Executor::CancelTasks under engine::Result::~Result, with the GIL held, and the worker running the UDF waiting for that GIL. The reproducer with a busy Python thread and a 0.1 ms switch interval hangs the old code at iteration 39 of 800 and runs all 800 on this branch, every flush refused as busy. A second execute from the other thread passed the busy check, waited for the whole first call, then superseded its result, so the first thread's fetchall() returned the second thread's row. And con.to_arrow_table() on a DB-API result, refused as busy by the same race, had already taken the rows out of the result, so a retry got nothing: with a second thread binding a relation without pause, between 13 and 163 fetches per 15 s lost their rows on the old code; on this branch every retried fetch returns them.

The client refuses a second caller instead of letting it wait: ConnectionClaim is the per-call busy flag, and a statement is supposed to hold it from its submission to the end of the engine work the call drives. StatementGroup::RunAllButLast and the seam's Submit do that through SubmitClaimed and RetainClaimed or SettleClaimed. Three paths took two claims instead. execute submitted under one claim, which Session::Submit dropped on return, reacquired the GIL, released it again and claimed a second time in Result::Settle. executemany did the same around Session::Submit(Prepared &) and Result::Retain. The DB-API Arrow fetch checked the transaction under one claim, took the rows out of the result under none, submitted the statement over them under a second claim and built the Arrow schema under a third. The engine starts a statement as soon as it is submitted whenever it has worker threads, so another thread's call could claim in the gap and either run or be refused by the engine for a result it never held. When the other thread still held the claim as Settle claimed, execute itself was refused as busy; that refusal came before the settling code that closes a failed result, so the pending result was destroyed later, after the GIL had been retaken, and closing it waited for the UDF's task, which waited for the GIL.

The three paths now take one claim before submitting and keep it through settling or retaining, as the two compliant paths already did; the Arrow fetch keeps it from its checks through the take, the statement, the settle or retain and the schema build, so a refusal there comes before the rows are taken, and ends it before the conversion into pyarrow, which runs Python and holds the connection no longer. Session keeps only the claimed submit, for a statement, a prepared statement and a collection, and Result only the claimed settle and retain, each asserting in a debug build that the caller holds the claim, so the next caller cannot rebuild the gap; StatementGroup gains RefuseParametersClaimed, and Result and DuckDBPyResult claimed variants of the take and of the Arrow schema build. A refusal or failure inside the claimed stretch closes the pending result there, with the GIL released.

The connection mutex changes for the submitting calls too. One submitting call is in flight on a connection at a time: execute, executemany and the DB-API Arrow fetch record their thread on arrival, and a second one arriving from another thread is refused as busy at once, before any wait. The reason is the stretch of such a call outside any claim, where it parses, supersedes the old result, converts its parameters in Python and installs the result: a caller that passed the busy check there used to wait for the whole call and then replace its result. Behind any other holder of the mutex a submitting call still waits as before: a thread reading description or rowcount, or fetching rows already in the client, holds the mutex while it waits for the GIL back, and an execute beside such a reader never failed and still does not. Because the refusal is decided on arrival, it holds with a reader thread present as well: two submitting calls queued behind a reader no longer let the loser wait through the winner's statement. The Arrow fetch counts as submitting only until its claim ends; the conversion into pyarrow afterwards is waited for like a reader. A nested call from the thread that holds the mutex still enters, when that thread holds no claim and runs in no callback.

What changes for a program that shares one connection between threads, as a list: these calls are refused as busy instead of waiting when they meet another of them in flight on another thread: execute, executemany, the DB-API Arrow fetches to_arrow_table and to_arrow_reader, and through execute also begin, commit inside an explicit transaction, rollback, checkpoint and append, and the module level duckdb.execute and duckdb.executemany on the shared default connection. The messages do not change: a collision while a statement runs is refused with the client's busy message, as it already was when the collision landed during settling. The fetches of rows already in the client, description, rowcount, close and the registering of a UDF keep waiting on the mutex as before, and the relation methods, sql and the appender never took it. What a single thread sees does not change either.

On the statement path the branch runs fewer instructions, not more: counted in process with a perf_event_open counter on one thread, best of several rounds alternating the two builds, execute("SELECT 1") costs 766,104 to 766,121 instructions per call on the old code and 764,799 to 764,851 on this branch, and an executemany of one row inserts 468,385 to 468,397 per row against 468,161 to 468,189: one GIL release instead of two between submission and settling, and no GIL release around an uncontended mutex. Wall times per call swap order between rounds on a loaded machine and show no difference.

The tests. Two run in a child process with a timeout, since the failure mode is a hang: one lets a second thread append and flush without pause while the first thread submits a statement whose UDF runs on a worker, and asserts that every refusal is the busy one and every row arrives, with execute and again with executemany as the first thread's call; another lets the second thread call execute without pause and asserts that no call waited and that the first thread reads its own row afterwards. On the old code the first hangs and the second reports a call that waited and a foreign row, in every run. A third child process test fetches each result as Arrow while a second thread binds a relation without pause, retries a refused fetch and asserts that every fetch returns its rows; on the old code it fails within seconds. Four run in process: the first thread's parameter object waits inside __iter__ until the second thread's call has returned, so that thread holds the mutex and no claim, and a second execute must be refused while the first thread reads its own row; a thread reads description every half millisecond while another loops execute for a second, and no execute may be refused; a reader thread, a thread looping a held statement and a thread looping execute run for four seconds, and no execute may wait for the held statement; and a relation must run, and then an execute, while a large result is converted into pyarrow. test_another_thread_is_refused_while_a_statement_runs now contends through a relation, which takes the claim and not the mutex, so it sees the claim, and the lazy polars scan test asserts the busy message, which the claim held through settling always gives.

[el-marvin]

Changes requested.

What I reviewed. Round 1, on 7fa2cfe, the single commit on top of the merge base 9ca5aa3, which is also the tip of v2.0-cyanoptera. I built the head and the merge base the same way (release, the repository's two step uv sync), and a copy of the head with switches, each off unless an environment variable turns it on, that reopen or weaken one part of the change at a time.

The premise holds, and the core of the fix is right and at the right layer. On the merge base, execute drops its claim between submitting and settling. The reproducer from the review of #760, a flush from a second thread while a busy Python thread spins with sys.setswitchinterval(0.0001) at 4 threads, hangs the merge base at iteration 39 of 800 with the main thread in con.execute and the UDF in release.wait. The head runs all 800 iterations, every flush refused as busy. Holding one claim from submission to settling or retaining, as StatementGroup::RunAllButLast (src/engine/connection.cpp:514) and the seam's Submit (src/pyconnection/seam.cpp:784) already did, is the rule this code base has, and removing the unclaimed Session::Submit, Result::Settle and Result::Retain keeps the next caller from reopening the gap. With the window between submission and settling widened to 200 ms while the claim is held, every call from a second thread (appender flush and creation, query_appender, sql, table, execute, create_function) is refused as busy at once, at 1 and at 8 threads, and the first thread reads its own row. With the claim dropped across the same 200 ms, relation calls run in the gap, the flush gets the engine's open result refusal, and the hang returns on the first attempt.

What must change. The second mechanism, the refusal on the connection mutex (IfHeld::REFUSE with mutex_held_by_submitting_call and submitting_entries), does not keep its promise once a third thread holds the mutex (finding 1). And half of what the title names has no test: reopening the executemany gap or the Arrow fetch gap leaves every touched test green (finding 2).

A behaviour change for the maintainer to decide. Outside the claimed stretch, a submitting call behind another submitting call used to wait for the mutex and then run; now it is refused as busy. That covers execute, executemany, the DB-API Arrow fetch, and through ExecuteFromString also begin, commit, rollback and checkpoint, and the module level duckdb.execute and duckdb.executemany on the shared default connection. Code that shares one connection between threads gets more ResourceInUseExceptions than before, in exchange for never having its result replaced by a call that waited. The merge base already refused during the claimed stretches, so the model does not change, only how much of each call it covers. Finding 1 applies if the maintainer keeps this rule.

Findings

1. A submitting call that finds the mutex held by a reader still waits for the whole of another submitting call, and is refused only afterwards (src/include/duckdb_python/pyconnection/pyconnection.hpp:303).
Severity: should fix. Category: correctness. Evidence: ran. Fix: root-cause. Origin: repository. Base: widened.
Decide the refusal when the call arrives, before it waits, so that the counter is no longer needed; the evidence and a sketch are in the inline comment.

2. The executemany and Arrow fetch halves of the change, the counter and the release of the mark have no test: reopening each of them leaves every touched test green.
Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: repository. Base: introduced.
Add a test for each that fails on the merge base: executemany with a second thread flushing or binding, the Arrow fetch with a second thread binding, and the three thread case of finding 1.

I built a copy of the head with switches, each off unless an environment variable turns it on, and ran the two touched files with the Arrow and DB-API fetch tests (test_execute_contract.py, test_appender.py, test_arrow_fetch.py, test_arrow_fetch_recordbatch.py, test_dbapi_fetch.py, test_statement_order.py) at -n 8:

• No switch (the head): 557 passed, 1 skipped.
• REVIEW_GAP_EXECUTE_MS=50, execute submits under one claim, sleeps and settles under a second: 2 failed, test_a_flush_refused_while_another_thread_runs_a_statement_keeps_the_rows and test_a_flush_that_collides_with_a_submitted_statement_is_refused_as_busy.
• REVIEW_GAP_EXECUTEMANY_MS=50, the same gap in ExecuteInternal: 557 passed.
• REVIEW_GAP_ARROW_MS=50, the Arrow fetch drops its claim before the take and claims again before the statement over the rows: 557 passed.
• REVIEW_MUTEX_WAIT=1, the three calls wait for the mutex as before: 2 failed, the second execute test and the parameter conversion test.
• REVIEW_NO_COUNT=1, no counter check after the wait: 557 passed.
• REVIEW_KEEP_MARK=1, the Arrow fetch keeps its mark through the conversion into pyarrow: 557 passed.
• REVIEW_NO_CLOSE=1, no res->Close() in SubmitAndDrive's catch: 557 passed.

The execute gap is caught only by the two appender tests. test_another_thread_is_refused_while_a_statement_runs moved to the default thread count "so its statement runs from submission on", but its contender is con.execute("SELECT 1"), which the mutex refuses before the claim is asked, so it passes with the gap reopened too. A relation call or a flush as the contender would let it see the claim.

The description says the Arrow fetch has no test "that fails on the old code with a useful probability, since the window between the take and the claim runs no Python code". The window needs no Python code of its own: the merge base releases and retakes the GIL between the take and the second claim, and any other Python thread can run there. A probe that loops con.execute("SELECT * FROM range(5) t(i)") and con.to_arrow_table() for 15 s while a second thread loops con.sql("SELECT 1"), with sys.setswitchinterval(0.0001), and retries a refused fetch:

base: table 290 iterations in 15.0s {... 'rows 5': 124, 'rows 5 after a refusal': 3, 'rows None after a refusal': 163}
base: table 19 iterations in 15.2s {... 'rows 5': 6, 'rows None after a refusal': 13}
head: table 806 iterations in 15.0s {... 'rows 5': 689, 'rows 5 after a refusal': 117}

On the merge base, between 13 and 163 fetches in 15 s lose their rows to a refusal; on the head, every retried fetch returns its five rows. A child process test of that shape, asserting that every retried fetch returns its rows, would fail on the merge base within seconds. The executemany test can take the shape of the new appender test with executemany as the first thread's call. The close in the catch matters only after the parameter refusal, which needs a statement that expands to nothing (an IMPORT DATABASE of an empty export), so I do not ask for a test of it.

The statement path

execute and executemany are on the hot path, so I measured them: Release builds of the head and the merge base made the same way, one thread, 100,000 calls of con.execute("SELECT 1") and an executemany of 100,000 one row inserts, best of five, alternating runs, with the user space instructions of the calling thread counted by a perf_event_open counter opened in the process.

• execute, instructions per call (fixed hash seed, three runs each): merge base 765,628 to 765,639, head 763,506 to 763,560.
• execute, best wall time per call: merge base 113 to 131 us, head 118 to 140 us.
• executemany, instructions per row (two runs each): merge base 468,693, head 467,823 to 467,837.
• executemany, best wall time per row: merge base 79.2 to 80.7 us, head 81.3 us.

The head runs 0.3 percent fewer instructions per execute and 0.2 percent fewer per row: it releases the GIL once instead of twice between submission and settling, and an uncontended try_lock replaces a GIL release around the mutex. The wall times swapped order between rounds on a box whose load moved between 16 and 196, so I read no regression into them. The description gives no figures; please add the before and after numbers for these two cases, so the record carries them.

Smaller things

• The comment on IfHeld::REFUSE (src/include/duckdb_python/pyconnection/pyconnection.hpp:275) says the holder "may be between two claims of its statement". After this change a statement holds one claim; what the refusal covers is the stretch of the call outside any claim (parsing, superseding the old result, converting parameters, installing the result), as the parameter conversion test shows.
• ArrowSchemaHolder (src/pyconnection.cpp:1513) does what the engine's ArrowSchemaWrapper already does, which src/python_udf.cpp:140 and src/include/duckdb_python/engine/result.hpp:189 use.
• converted->BuildArrowSchemaClaimed(schema.schema) (src/pyconnection.cpp:1581) takes the context lock with the GIL held. The merge base did the same through RunWithArrowSchema and FetchArrowArrayStream, so nothing new, but the build could move into the gil_scoped_release block just above, as src/pyconnection/seam.cpp:1189 builds its schema, and rule 1 of "The GIL and the engine" in AGENTS.md, which names only two such paths, would hold again.
• With Session::Submit, Result::Settle and Result::Retain gone, the claimed entry points are the only way in, but nothing checks that the caller holds the claim. A D_ASSERT on the connection state's busy flag in SubmitClaimed, SettleClaimed and RetainClaimed would make a later caller that forgets session.Claim() fail in the assertion build instead of reopening the gap.
• The two new in process tests and the changed one open duckdb.connect() at the default thread count, while the child process tests set four threads so that "a one-CPU host must still get some". On a one core runner test_another_thread_is_refused_while_a_statement_runs again runs its statement only when it is settled.
• test_a_lazy_polars_frame_scanned_on_its_own_connection_errors accepts either refusal (REFUSED is "open result" or "busy"). With the claim held from submission to settling, its count(*) scan is always refused as busy; with the gap reopened it gets the open result refusal instead. Asserting BUSY there would let it notice a reopened gap.

Outside findings not taken

• code-review: the extra res->Close() in SubmitAndDrive's catch can replace the query's error when a close throws. Not reachable in ordinary use: QueryResult::Close throws only when aborting the transaction fails, and CloseAfterFailure has already closed the result after a settle or retain failure.
• code-review: refusal is opted into at three call sites while the DB-API fetches still wait, so execute against a streaming fetchdf is still decided by timing. The base does the same, and the description states that readers are waited for on purpose. The behaviour change paragraph above leaves the rule to the maintainer.
• code-review: begin, commit, rollback, checkpoint and the module level functions inherit the refusal with no test. Taken into the behaviour change paragraph rather than as a finding: they reach it only through Execute.
• code-review: SubmitAndDrive's template parameter buys nothing, and the claim, lock and release boilerplate repeats. Style.
• code-review: BuildArrowSchema calls TryGetContext twice, and TakeChunks checks for failure twice. Neither is on a path that runs per row.
• Codex: a queued submitter can wait indefinitely when the winner's UDF waits for something the queued thread does after its execute returns. The wait itself is finding 1; the indefinite form needs a UDF that waits on another thread's later call, which I did not take as a separate hang.

Prerequisites in the engine

None. The engine starts a submitted statement on its worker threads when it has more threads than external threads; the gap was in the client.

What I verified

Head and range. Head 7fa2cfe, range 9ca5aa3..7fa2cfe, one commit, ten files, no merge commit. The merge base is the tip of v2.0-cyanoptera, and the branch merges into it without conflict.

Builds. The head's release build in the tree, a release build of the merge base, and a release copy of the head with these switches: REVIEW_GAP_EXECUTE_MS, REVIEW_GAP_EXECUTEMANY_MS and REVIEW_GAP_ARROW_MS drop the claim at the place the merge base dropped it (with one GIL round trip, as the merge base had) and sleep; REVIEW_HELD_SLEEP_MS sleeps right after submission, or in the Arrow fetch after the take, with the claim still held; REVIEW_MUTEX_WAIT, REVIEW_NO_COUNT, REVIEW_KEEP_MARK and REVIEW_NO_CLOSE turn off one piece each. With no switch set, the copy passed the touched files with tests/fast/arrow and test_dbapi_fetch.py: 8044 passed, 33 skipped, 12 xfailed. The touched C++ files compile with -DPy_LIMITED_API=0x030C0000 on the head and on the merge base.

The new tests on the merge base (the head's two test files against the merge base build): test_a_flush_that_collides_with_a_submitted_statement_is_refused_as_busy hangs and its child is killed after 90 s; test_a_second_execute_is_refused_instead_of_waiting fails with submitted: 14 and outcomes: ['first thread read [(7,)]', 'waited']; test_a_second_execute_during_parameter_conversion_is_refused_at_once fails with timed_out == [True] and outcome ['ran']. test_a_thread_reading_the_description_does_not_make_execute_fail and the changed test_another_thread_is_refused_while_a_statement_runs pass there, as expected for a guard against overreach and a test that does not see the claim. The description's account of the base holds.

The two touched files on the head at -n 8: 359 passed, 1 skipped in 14.13s. The appender test whose flake started this, test_a_flush_refused_while_another_thread_runs_a_statement_keeps_the_rows, now at the default thread count, and the new collision test, 400 runs each at -n 8: 800 passed in 120.29s.

The whole fast suite on the head at -n 8: 1 failed, 17604 passed, 113 skipped, 164 xfailed in 801.42s. The failure is test_query_progress, with "query finished too quick": run ten times on its own it fails 5 of 10 on the head and 4 of 10 on the merge base, since on this 192 core machine its query ends before progress is read. Not caused by this change.

Contention, per entry point (8 threads, three rounds per pair): a first thread runs a statement whose UDF holds it, through execute with a string, execute with a Statement object, SQL PREPARE and EXECUTE, executemany, a relation's fetchall, and the DB-API to_arrow_table and to_arrow_reader (these two at one thread, so the UDF holds the take); a second thread calls execute, executemany, an appender flush, sql, table or to_arrow_table. On the head all 42 pairs give an immediate busy refusal and the first thread's own rows, 126 of 126.

Errors between submission and settling (8 threads): a bind error, a parameter of the wrong type, too few parameters, parameters for a statement that takes none, a UDF raising on a worker in execute and in executemany, a constraint error in executemany, an error during the Arrow take and during the Arrow reader's settle, and an interrupt from _thread.interrupt_main() during execute, executemany, the Arrow fetch and a relation fetch. After each, execute, executemany, the Arrow fetch, a relation and an appender flush on the same connection run and return the right rows, on the head and on the merge base alike.

The lazy polars test's shape (test_a_lazy_polars_frame_scanned_on_its_own_connection_errors, at 8 threads): neither build hung in 120 runs on four cores shared with 16 busy loops, nor in 500 iterations in one process beside a busy Python thread with a 0.1 ms switch interval; every run was refused and the connection usable. With the execute gap reopened by 50 ms the scan of the frame lands in it in 40 of 40 runs, refused with the engine's open result message; with the head's claim held 50 ms longer it is refused as busy in 40 of 40.

Outside reviews. The code-review skill at xhigh effort on the range in the tree (12 findings), and Codex read only at xhigh reasoning effort (one finding). Both found the wait of finding 1 by reading; I reproduced it.

CI at the time of review: no check had failed. Lint, the full API build with assertions, the sdist, the seed wheels for Linux and Windows and the abi3 sanity jobs had passed; the two macOS seed jobs were pending.

What held

• The hang is gone. The busy Python thread shape ran 800 iterations on the head with every flush refused as busy, where the merge base hung at iteration 39; the two thread reproducer with a second thread calling con.table("t") without pause ran five attempts with the window widened to 200 ms under the claim, 7,438 to 7,447 refusals each and no hang, where reopening the gap hangs it on the first attempt.
• No claim leaks on an error path, including an interrupt in the middle of each of the four paths (see above).
• The Arrow fetch keeps its rows. On the merge base a fetch refused after the take loses its rows; on the head every refused fetch was retried and returned its rows, 117 of 117.
• Two submitting calls without a third thread never wait. In 10 s, 192,367 trivial executes beside a looping held statement were refused at once, none waited; on the merge base 50 of 53 waited and then ran, replacing the other thread's result.
• The single thread case. The uncontended try_lock never refuses, and the instruction counts above drop slightly.
• The schema ownership in the Arrow fetch. The holder releases a schema that was built and never handed over, the table import takes it over, and the reader moves it into the stream and clears the source's release.

[el-marvin]

A submitting call that finds the mutex held by a reader still waits for the whole of another submitting call, and is refused only afterwards, so "refused instead of waiting" holds for two threads but not for three.

Severity: should fix. Category: correctness. Evidence: ran. Fix: root-cause. Origin: repository. Base: widened.

The refusal is decided once, before the wait:

			auto entries_before = conn.submitting_entries.load();
			if (lock_.try_lock()) {
				MarkSubmitting();
				return;
			}
			if (conn.mutex_held_by_submitting_call.load()) {
				engine::ThrowBusy();
			}
			WaitForMutex(true, entries_before);

When a reader (description, rowcount, a fetch of rows already in the client) holds the mutex, two submitting calls both read the flag as false and both block in lock_.lock(). The one that gets the mutex marks itself and runs its statement; the other stays blocked for that whole statement, and the counter only refuses it once it finally gets the mutex. The counter keeps it from replacing the first call's result, which is the merge base's worst outcome, but it does not keep it from waiting.

A probe, 8 threads, run for 10 s on the head build, with and without the third thread:

con = duckdb.connect(config={"memory_limit": "8GB", "threads": 8})
con.create_function("hold", lambda x: time.sleep(0.2) or x, [BIGINT], BIGINT)

def reader():  # third thread
    while not stop.is_set():
        with contextlib.suppress(duckdb.ResourceInUseException):
            con.description

def a_loop():
    while not stop.is_set():
        with contextlib.suppress(duckdb.ResourceInUseException):
            con.execute("SELECT hold(i) + 10 FROM range(1) t(i)")

def b_loop():  # counts each outcome, and "waited" when the call took over 0.1 s
    while not stop.is_set():
        began = time.monotonic()
        try:
            con.execute("SELECT 7"); kind = "ran"
        except duckdb.ResourceInUseException:
            kind = "busy"
        outcomes[(kind, "waited" if time.monotonic() - began > 0.1 else "at once")] += 1

no reader A: {'ran': 44} B: {('busy', 'at once'): 192367}
reader    A: {'ran': 47} B: {('busy', 'at once'): 109011, ('busy', 'waited'): 21}

"waited" means the call took more than 0.1 s before it raised. With a third thread reading con.description in a loop, 21 calls of B waited for A's statement and were then refused. On the merge base the same probe gives ('ran', 'waited'): 50 with or without the reader: B waits for A and then replaces its result.

The description says the count "makes this exact", and test_a_second_execute_is_refused_instead_of_waiting asserts that no call waits; neither holds once a third thread takes the mutex. By reading, the comment at line 330, "Marked before the GIL is retaken, so no contender reads the mutex as held by a waiting call", does not hold either: a contender that holds the GIL can run its try_lock and read the flag between lock_.lock() returning and MarkSubmitting().

Ask: decide the refusal when the call arrives, not after it has waited. A submitting call can mark itself before it waits for the mutex, with an exchange on the flag that refuses when another submitting call has already marked (letting the marking thread's own nested calls through, as the recursive mutex does now). That makes the counter unnecessary. The alternative is to admit submitting calls through the claim itself, held for the whole call; either way one rule decides, and a test with a reader thread pins it.