Skip to content

Commit 0b5b9d4

Browse files
authored
Fix race condition in query timeouts (#218)
The previous fix (0a3d2bd) eagerly released TimeoutGuard when an iterator finished, but a race remained: the background task could peek the heap, see an active entry, and call conn.interrupt() while the query was finishing on another thread. Because SQLite's interrupt flag is connection-scoped, a late-arriving signal would pend on the connection and fire on the next operation — typically a prepare() or EXPLAIN issued right after a timed-out stmt.get(), surfacing as a spurious SQLITE_INTERRUPT. Fix: - Make QueryTimeoutManager per-connection (holds Weak<Connection>) and track a single `current_operation_id`. The background thread only interrupts when the expired entry's id matches the currently registered operation; older entries are drained without side effects. - Serialize operations on a connection with an execution_lock held across prepare(), execute_batch(), and Statement.{get,all,run}. This gives "current operation" a well-defined meaning and closes the race between guard release and interrupt dispatch. - Defense-in-depth: if prepare() returns SQLITE_INTERRUPT, probe the connection with SELECT 1 (up to three times) to consume any signal that still slipped through before retrying. - Swap tokio Notify/task + per-entry AtomicBool for std::thread + Condvar. The manager is single-consumer and no longer needs an async runtime. Tests: - Statement.get() with both defaultQueryTimeout and per-query queryTimeout options (async + sync). - Regression test: 20 iterations of timing out stmt.get(..., { queryTimeout: 50 }) and immediately running EXPLAIN QUERY PLAN on the same connection — each EXPLAIN must succeed.
2 parents 58bcccf + e5e90f8 commit 0b5b9d4

4 files changed

Lines changed: 457 additions & 261 deletions

File tree

integration-tests/tests/async.test.js

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,29 @@ test.serial("Query timeout option interrupts long-running query", async (t) => {
416416
db.close();
417417
});
418418

419+
test.serial("Query timeout option interrupts long-running Statement.get()", async (t) => {
420+
const [db, errorType] = await connect(":memory:", { defaultQueryTimeout: 100 });
421+
const stmt = await db.prepare(`
422+
WITH RECURSIVE numbers(value) AS (
423+
SELECT 1
424+
UNION ALL
425+
SELECT value + 1 FROM numbers WHERE value < 1000000000
426+
)
427+
SELECT sum(value) FROM numbers;
428+
`);
429+
430+
const error = await t.throwsAsync(async () => {
431+
await stmt.get();
432+
});
433+
t.truthy(error);
434+
t.true(error instanceof Error);
435+
t.true(error instanceof errorType);
436+
t.true(error.message.toLowerCase().includes("interrupt"));
437+
t.is(error.code, "SQLITE_INTERRUPT");
438+
439+
db.close();
440+
});
441+
419442
test.serial("Query timeout option allows short-running query", async (t) => {
420443
const [db] = await connect(":memory:", { defaultQueryTimeout: 100 });
421444
const stmt = await db.prepare("SELECT 1 AS value");
@@ -464,6 +487,57 @@ test.serial("Per-query timeout option interrupts long-running Statement.all()",
464487
db.close();
465488
});
466489

490+
test.serial("Per-query timeout option interrupts long-running Statement.get()", async (t) => {
491+
const [db, errorType] = await connect(":memory:");
492+
const stmt = await db.prepare(`
493+
WITH RECURSIVE numbers(value) AS (
494+
SELECT 1
495+
UNION ALL
496+
SELECT value + 1 FROM numbers WHERE value < 1000000000
497+
)
498+
SELECT sum(value) FROM numbers;
499+
`);
500+
501+
const error = await t.throwsAsync(async () => {
502+
await stmt.get(undefined, { queryTimeout: 100 });
503+
});
504+
t.truthy(error);
505+
t.true(error instanceof Error);
506+
t.true(error instanceof errorType);
507+
t.true(error.message.toLowerCase().includes("interrupt"));
508+
t.is(error.code, "SQLITE_INTERRUPT");
509+
510+
db.close();
511+
});
512+
513+
test.serial("Timeout on Statement.get() does not leak into later prepare()/EXPLAIN", async (t) => {
514+
t.timeout(30_000);
515+
const [db, errorType] = await connect(":memory:");
516+
const longRunningStmt = await db.prepare(`
517+
WITH RECURSIVE numbers(value) AS (
518+
SELECT 1
519+
UNION ALL
520+
SELECT value + 1 FROM numbers WHERE value < 1000000000
521+
)
522+
SELECT sum(value) FROM numbers;
523+
`);
524+
525+
for (let i = 0; i < 20; i++) {
526+
const error = await t.throwsAsync(async () => {
527+
await longRunningStmt.get(undefined, { queryTimeout: 50 });
528+
});
529+
t.truthy(error);
530+
t.true(error instanceof errorType);
531+
t.true(error.message.toLowerCase().includes("interrupt"));
532+
533+
const explainStmt = await db.prepare("EXPLAIN QUERY PLAN SELECT 1");
534+
const explainRows = await explainStmt.all();
535+
t.true(explainRows.length > 0);
536+
}
537+
538+
db.close();
539+
});
540+
467541
test.serial("Per-query timeout option is accepted by Database.exec()", async (t) => {
468542
const [db] = await connect(":memory:");
469543
await db.exec("SELECT 1", { queryTimeout: 100 });

integration-tests/tests/sync.test.js

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,34 @@ test.serial("Query timeout option interrupts long-running query", async (t) => {
479479
db.close();
480480
});
481481

482+
test.serial("Query timeout option interrupts long-running Statement.get()", async (t) => {
483+
if (t.context.provider === "sqlite") {
484+
t.assert(true);
485+
return;
486+
}
487+
488+
const [db, errorType] = await connect(":memory:", { defaultQueryTimeout: 100 });
489+
const stmt = db.prepare(`
490+
WITH RECURSIVE numbers(value) AS (
491+
SELECT 1
492+
UNION ALL
493+
SELECT value + 1 FROM numbers WHERE value < 1000000000
494+
)
495+
SELECT sum(value) FROM numbers;
496+
`);
497+
498+
const error = t.throws(() => {
499+
stmt.get();
500+
});
501+
t.truthy(error);
502+
t.true(error instanceof Error);
503+
t.true(error instanceof errorType);
504+
t.true(error.message.toLowerCase().includes("interrupt"));
505+
t.is(error.code, "SQLITE_INTERRUPT");
506+
507+
db.close();
508+
});
509+
482510
test.serial("Query timeout option allows short-running query", async (t) => {
483511
if (t.context.provider === "sqlite") {
484512
t.assert(true);
@@ -513,6 +541,67 @@ test.serial("Per-query timeout option interrupts long-running Statement.all()",
513541
db.close();
514542
});
515543

544+
test.serial("Per-query timeout option interrupts long-running Statement.get()", async (t) => {
545+
if (t.context.provider === "sqlite") {
546+
t.assert(true);
547+
return;
548+
}
549+
550+
const [db, errorType] = await connect(":memory:");
551+
const stmt = db.prepare(`
552+
WITH RECURSIVE numbers(value) AS (
553+
SELECT 1
554+
UNION ALL
555+
SELECT value + 1 FROM numbers WHERE value < 1000000000
556+
)
557+
SELECT sum(value) FROM numbers;
558+
`);
559+
560+
const error = t.throws(() => {
561+
stmt.get(undefined, { queryTimeout: 100 });
562+
});
563+
t.truthy(error);
564+
t.true(error instanceof Error);
565+
t.true(error instanceof errorType);
566+
t.true(error.message.toLowerCase().includes("interrupt"));
567+
t.is(error.code, "SQLITE_INTERRUPT");
568+
569+
db.close();
570+
});
571+
572+
test.serial("Timeout on Statement.get() does not leak into later prepare()/EXPLAIN", async (t) => {
573+
if (t.context.provider === "sqlite") {
574+
t.assert(true);
575+
return;
576+
}
577+
578+
t.timeout(30_000);
579+
const [db, errorType] = await connect(":memory:");
580+
const longRunningStmt = db.prepare(`
581+
WITH RECURSIVE numbers(value) AS (
582+
SELECT 1
583+
UNION ALL
584+
SELECT value + 1 FROM numbers WHERE value < 1000000000
585+
)
586+
SELECT sum(value) FROM numbers;
587+
`);
588+
589+
for (let i = 0; i < 20; i++) {
590+
const error = t.throws(() => {
591+
longRunningStmt.get(undefined, { queryTimeout: 50 });
592+
});
593+
t.truthy(error);
594+
t.true(error instanceof errorType);
595+
t.true(error.message.toLowerCase().includes("interrupt"));
596+
597+
const explainStmt = db.prepare("EXPLAIN QUERY PLAN SELECT 1");
598+
const explainRows = explainStmt.all();
599+
t.true(explainRows.length > 0);
600+
}
601+
602+
db.close();
603+
});
604+
516605
test.serial("Per-query timeout option is accepted by Database.exec()", async (t) => {
517606
if (t.context.provider === "sqlite") {
518607
t.assert(true);

0 commit comments

Comments
 (0)