Skip to content

Commit 0a3d2bd

Browse files
authored
Fix stale query timeout guards interrupting unrelated queries (#217)
The TimeoutGuard held by RowsIterator was only released when the JS garbage collector collected the native object. In a tight loop of stmt.all() calls (which uses iterate() internally), exhausted iterators would pile up unreachable but not yet GC'd. When their timeout deadline fired — potentially hundreds of milliseconds after the query finished — conn.interrupt() would kill whatever unrelated query happened to be running at that moment, producing spurious SQLITE_INTERRUPT errors well below the configured timeout. Fix: eagerly release the TimeoutGuard as soon as the iterator is exhausted (next() returns None), encounters an error, or is explicitly closed via the iterator return() protocol (break/throw in for...of). - Rust: change `_timeout_guard: Option<TimeoutGuard>` to `Mutex<Option<TimeoutGuard>>` so close() and next() can both release the guard through a shared reference. Expose close() via napi. - promise.js / compat.js: wrap the native iterator with an idempotent close helper and implement return() for early-termination cleanup. - Add integration test that reproduces the original bug: 100 sequential stmt.all() calls with a 500ms timeout, each finishing in ~115ms, which reliably triggered stale-guard interrupts before this fix.
2 parents 21485e8 + 5596b09 commit 0a3d2bd

File tree

5 files changed

+136
-32
lines changed

5 files changed

+136
-32
lines changed

compat.js

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -309,14 +309,7 @@ class Statement {
309309
try {
310310
const { params, queryOptions } = splitBindParameters(bindParameters);
311311
const it = statementIterateSync(this.stmt, params, queryOptions);
312-
return {
313-
next: () => iteratorNextSync(it),
314-
[Symbol.iterator]() {
315-
return {
316-
next: () => iteratorNextSync(it),
317-
}
318-
},
319-
};
312+
return wrappedIter(it);
320313
} catch (err) {
321314
throw convertError(err);
322315
}
@@ -331,11 +324,17 @@ class Statement {
331324
try {
332325
const result = [];
333326
const iterator = this.iterate(...bindParameters);
334-
let next;
335-
while (!(next = iterator.next()).done) {
336-
result.push(next.value);
327+
try {
328+
let next;
329+
while (!(next = iterator.next()).done) {
330+
result.push(next.value);
331+
}
332+
return result;
333+
} finally {
334+
if (typeof iterator.return === "function") {
335+
iterator.return();
336+
}
337337
}
338-
return result;
339338
} catch (err) {
340339
throw convertError(err);
341340
}
@@ -365,6 +364,26 @@ class Statement {
365364
}
366365
}
367366

367+
function wrappedIter(it) {
368+
return {
369+
next() {
370+
return iteratorNextSync(it);
371+
},
372+
return(value) {
373+
if (typeof it.close === "function") {
374+
it.close();
375+
}
376+
return {
377+
done: true,
378+
value,
379+
};
380+
},
381+
[Symbol.iterator]() {
382+
return this;
383+
},
384+
};
385+
}
386+
368387
module.exports = Database;
369388
module.exports.SqliteError = SqliteError;
370389
module.exports.Authorization = Authorization;

index.d.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,32 @@ export declare class Database {
8080
* - Legacy format: `{ [tableName: string]: 0 | 1 }`
8181
* - Full format: `{ rules: AuthRule[], defaultPolicy?: 0 | 1 | 2 }`
8282
* - `null` to remove the authorizer
83+
*
84+
* Pattern fields (`table`, `column`, `entity`) accept a plain string for
85+
* exact matching, or `{ glob: "pattern" }` for glob matching with `*` and `?`.
86+
*
87+
* # Examples
88+
*
89+
* ```javascript
90+
* const { Authorization, Action } = require('libsql');
91+
*
92+
* // Legacy table-level allow/deny
93+
* db.authorizer({ "users": Authorization.ALLOW });
94+
*
95+
* // Rule-based with glob patterns
96+
* db.authorizer({
97+
* rules: [
98+
* { action: Action.READ, table: "users", column: "password", policy: Authorization.IGNORE },
99+
* { action: Action.INSERT, table: { glob: "logs_*" }, policy: Authorization.ALLOW },
100+
* { action: Action.READ, policy: Authorization.ALLOW },
101+
* { action: Action.SELECT, policy: Authorization.ALLOW },
102+
* ],
103+
* defaultPolicy: Authorization.DENY,
104+
* });
105+
*
106+
* // Remove authorizer
107+
* db.authorizer(null);
108+
* ```
83109
*/
84110
authorizer(config: unknown): void
85111
/**
@@ -173,6 +199,7 @@ export declare class Statement {
173199
}
174200
/** A raw iterator over rows. The JavaScript layer wraps this in a iterable. */
175201
export declare class RowsIterator {
202+
close(): void
176203
next(): Promise<Record>
177204
}
178205
export declare class Record {

integration-tests/tests/async.test.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,30 @@ test.serial("Query timeout option allows short-running query", async (t) => {
423423
db.close();
424424
});
425425

426+
test.serial("Stale timeout guard from exhausted iterator does not interrupt later queries", async (t) => {
427+
t.timeout(30_000);
428+
const [db] = await connect(":memory:", { defaultQueryTimeout: 1_000 });
429+
430+
// Insert test data.
431+
await db.exec("CREATE TABLE t(x INTEGER)");
432+
const insert = await db.prepare("INSERT INTO t VALUES (?)");
433+
for (let i = 0; i < 2_000; i++) {
434+
await insert.run(i);
435+
}
436+
437+
// Run many sequential queries via stmt.all() (which uses iterate() internally).
438+
// Each query finishes well under the timeout, but if the RowsIterator's
439+
// TimeoutGuard is not released until GC, stale guards will fire and
440+
// interrupt unrelated later queries.
441+
const stmt = await db.prepare("SELECT * FROM t ORDER BY x ASC");
442+
for (let i = 0; i < 150; i++) {
443+
const rows = await stmt.all();
444+
t.is(rows.length, 2_000);
445+
}
446+
447+
db.close();
448+
});
449+
426450
test.serial("Per-query timeout option interrupts long-running Statement.all()", async (t) => {
427451
const [db, errorType] = await connect(":memory:");
428452
const stmt = await db.prepare(

promise.js

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -354,18 +354,7 @@ class Statement {
354354
try {
355355
const { params, queryOptions } = splitBindParameters(bindParameters);
356356
const it = await this.stmt.iterate(params, queryOptions);
357-
return {
358-
next() {
359-
return it.next();
360-
},
361-
[Symbol.asyncIterator]() {
362-
return {
363-
next() {
364-
return it.next();
365-
}
366-
};
367-
}
368-
};
357+
return wrappedIter(it);
369358
} catch (err) {
370359
throw convertError(err);
371360
}
@@ -380,11 +369,17 @@ class Statement {
380369
try {
381370
const result = [];
382371
const iterator = await this.iterate(...bindParameters);
383-
let next;
384-
while (!(next = await iterator.next()).done) {
385-
result.push(next.value);
372+
try {
373+
let next;
374+
while (!(next = await iterator.next()).done) {
375+
result.push(next.value);
376+
}
377+
return result;
378+
} finally {
379+
if (typeof iterator.return === "function") {
380+
await iterator.return();
381+
}
386382
}
387-
return result;
388383
} catch (err) {
389384
throw convertError(err);
390385
}
@@ -414,6 +409,26 @@ class Statement {
414409
}
415410
}
416411

412+
function wrappedIter(it) {
413+
return {
414+
next() {
415+
return it.next();
416+
},
417+
return(value) {
418+
if (typeof it.close === "function") {
419+
it.close();
420+
}
421+
return {
422+
done: true,
423+
value,
424+
};
425+
},
426+
[Symbol.asyncIterator]() {
427+
return this;
428+
}
429+
};
430+
}
431+
417432
module.exports = {
418433
Action,
419434
Authorization,

src/lib.rs

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ use std::{
3434
str::FromStr,
3535
sync::{
3636
atomic::{AtomicBool, Ordering},
37-
Arc,
37+
Arc, Mutex,
3838
},
3939
time::Duration,
4040
};
@@ -1391,7 +1391,7 @@ pub struct RowsIterator {
13911391
safe_ints: bool,
13921392
raw: bool,
13931393
pluck: bool,
1394-
_timeout_guard: Option<TimeoutGuard>,
1394+
timeout_guard: Mutex<Option<TimeoutGuard>>,
13951395
}
13961396

13971397
#[napi]
@@ -1410,14 +1410,23 @@ impl RowsIterator {
14101410
safe_ints,
14111411
raw,
14121412
pluck,
1413-
_timeout_guard: timeout_guard,
1413+
timeout_guard: Mutex::new(timeout_guard),
14141414
}
14151415
}
14161416

14171417
#[napi]
14181418
pub async fn next(&self) -> Result<Record> {
14191419
let mut rows = self.rows.lock().await;
1420-
let row = rows.next().await.map_err(Error::from)?;
1420+
let row = match rows.next().await {
1421+
Ok(row) => row,
1422+
Err(err) => {
1423+
self.release_timeout_guard();
1424+
return Err(Error::from(err).into());
1425+
}
1426+
};
1427+
if row.is_none() {
1428+
self.release_timeout_guard();
1429+
}
14211430
Ok(Record {
14221431
row,
14231432
column_names: self.column_names.clone(),
@@ -1426,6 +1435,16 @@ impl RowsIterator {
14261435
pluck: self.pluck,
14271436
})
14281437
}
1438+
1439+
#[napi]
1440+
pub fn close(&self) {
1441+
self.release_timeout_guard();
1442+
}
1443+
1444+
fn release_timeout_guard(&self) {
1445+
let mut guard = self.timeout_guard.lock().unwrap();
1446+
guard.take();
1447+
}
14291448
}
14301449

14311450
/// Retrieve next row from an iterator synchronously. Needed for better-sqlite3 API compatibility.

0 commit comments

Comments
 (0)