Skip to content

fix(scheduler): let a caller that holds a slot work on the task it waits for - #1482

Merged
adsharma merged 2 commits into
LadybugDB:mainfrom
zahariash:fix/nested-query-one-thread
Nov 9, 2026
Merged

adsharma merged 2 commits into
LadybugDB:mainfrom
zahariash:fix/nested-query-one-thread

Conversation

@zahariash

Copy link
Copy Markdown
Contributor

This work was produced with the help of language models.

Fixes #1481.

IMPORT DATABASE and PROJECT_GRAPH run a query inside their own task and wait for it; when such tasks hold every slot, nobody can run the queued one. #1297 fixed the plain case. On a one-thread database, IMPORT DATABASE still hangs when the query has a timeout, because the import then runs on the worker, or when CALL threads is above one, because its statements then want more than one thread.

Fix. A thread that holds a slot (a worker, a caller running a task inline, or a launched thread that took over its caller's slot) registers on the queued task and runs it before waiting; idle workers can still join. With launchNewWorkerThread the new thread already works in the caller's place, so that path is unchanged and threads stay within max_num_threads. Ordinary queries pay one thread-local read per queued pipeline. No layout change: TaskScheduler's layout is extension ABI.

Timeouts. Each statement of an import restarts the timer and clears the interrupt. The worker running the import used to watch each statement's clock while another worker ran it; it now runs the statement itself. So the second commit keeps the waiting client thread watching after its first interrupt: it checks the clock again every 100 ms, and a statement that runs past the timeout is interrupted even if it started after the first deadline. A query past its deadline therefore wakes its waiting thread every 100 ms until it stops; the interval bounds how late such a statement is noticed. The two commits go in together: the first alone loses that per-statement check with two or more threads. The waiting thread reads the statement timer without synchronization while the worker restarts it; that race already exists on main with two or more threads, and both timeout tests exercise it.

Tests. ApiTest.ImportDatabaseWithOneThreadTimesOut (expects Import database failed: Interrupted.) and ImportDatabaseWithOneThreadAndThreadsSetToTwo, and TaskSchedulerTest.NestedTaskRunsOnCallerWhenNoSlotIsFree, NestedTaskWithTimeoutRunsOnWorker and NestedTaskRunsOnLaunchedThread, hang on main. LaunchedThreadStandsInForCaller fails if the caller also runs a launched thread's task, and QueuedTasksRunInParallelOnceInlineSlotsFree fails without the slot-holder check. TimeoutInterruptsStatementStartedAfterDeadline fails without the second commit. Full ctest passes; the export/import, copy_large, gds and rec_joins tests pass under MAX_NUM_THREADS=1.

zahariash and others added 2 commits October 31, 2026 01:46
…its for

IMPORT DATABASE and PROJECT_GRAPH run a query from inside their own task. When
every slot is held by such callers, the nested task is queued and nobody can take
it. On a one-thread database IMPORT DATABASE hangs this way when the query has a
timeout (the import then runs on the worker) or when the connection's threads
setting is above one (its statements then want more than one thread).

A thread that holds a slot (a worker, a caller running a task inline, or a
launched thread that took over its caller's slot) now registers on the queued
task and runs it before waiting; idle workers can still join. With
launchNewWorkerThread the new thread already works in the caller's place, so
that path is unchanged and the number of threads in tasks stays within
max_num_threads.

Tests: two imports on a one-thread database, one under a timeout and one with
threads set to two, and scheduler tests for a nested task with no free slot,
under a timeout, and on a launched thread; all five hang without this change. A
sixth fails if the caller also works on a launched thread's task.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IMPORT DATABASE runs each statement through executeNoLock, which clears the
interrupt flag and restarts the timer. A caller that saw the deadline pass
interrupted once and then waited without a timeout, so an interrupt that landed
between two statements, or in a phase that does not check the flag, was lost.
Before the parent commit the worker running the import still timed each
statement in its own wait loop when another worker ran it; now that worker runs
the statement itself. The caller now checks the clock again every 100 ms after
interrupting, so a statement that started after the deadline is interrupted when
its own time runs out.

Test: a statement that the worker runs itself after the deadline is
interrupted; it fails on the parent commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adsharma

adsharma commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Two non-blocking follow-ups (from batch-4 review):

  1. Timer race (timer.h:37-43 + client_context.cpp:120-127): startTime is a plain time_point — waiter reads while worker writes. Pre-existing, but the 100 ms recheck widens it to 1-thread DBs at 10 reads/s. Consider atomic/mutex or a comment documenting the benign-race assumption next to getTimeoutRemainingInMS.
  2. 100 ms quantum (task_scheduler.cpp:INTERRUPT_RECHECK_INTERVAL_MS): bounds late-notice of past-deadline statements and delays any post-deadline wakeup by up to 100 ms. One-line comment on why 100 (vs 10/1000), or a latency assertion, would close it.

@adsharma
adsharma merged commit 8a8ecc0 into LadybugDB:main Nov 9, 2026
4 checks passed
@zahariash

Copy link
Copy Markdown
Contributor Author

Both are in #1556.

  1. Timer's start time is now atomic, and ActiveQuery::reset() no longer zeroes it before startTimer(). On main, TSan flags the race in the re-check test and in an import of many short statements; with the change it reports nothing.
  2. INTERRUPT_RECHECK_INTERVAL_MS has a comment now. Completion notifies the waiter, so the interval delays no wakeup. A statement that starts after the deadline is still interrupted at its own deadline unless its timeout is under 100 ms; then it gets at most 100 ms. 1000 ms would stretch that to a second, and 10 ms would help only those short timeouts, with ten times the wakeups.

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.

Bug: IMPORT DATABASE never returns with max_num_threads=1 and a query timeout

2 participants