Skip to content

Commit a7c62f7

Browse files
committed
fix(driver-sql): close the SQLite connection a refused connect() opened
Claude-Session: https://claude.ai/code/session_01WQM1Hwm3eDA8xPhFuZGmZ6 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 03966ce commit a7c62f7

2 files changed

Lines changed: 268 additions & 14 deletions

File tree

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* A SQLite `connect()` that refuses its file closes what it opened before it
5+
* rejects (#25084).
6+
*
7+
* The refusal ({@link SqliteDatabaseFileError}: `SQLITE_CANTOPEN`,
8+
* `SQLITE_NOTADB`, `SQLITE_CORRUPT`) is the answer of the connection's first
9+
* statement, so the file was opened to give it. The pool used to keep that
10+
* better-sqlite3 connection: for a damaged file whose header says WAL, SQLite
11+
* kept its `-shm` and an empty `-wal` beside it until something called
12+
* `disconnect()`, and a caller that never does — the datasource connection
13+
* service's degraded path — kept them for the life of the process.
14+
*
15+
* Pinned with no `disconnect()` before any reading: what is left behind is
16+
* read straight after the rejection.
17+
*/
18+
19+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
20+
import {
21+
existsSync, mkdtempSync, readdirSync, readFileSync, readlinkSync, renameSync, rmSync, symlinkSync, truncateSync,
22+
writeFileSync,
23+
} from 'node:fs';
24+
import { tmpdir } from 'node:os';
25+
import { join } from 'node:path';
26+
import { SqlDriver } from './sql-driver.js';
27+
import { SqliteDatabaseFileError } from './sqlite-database-file.js';
28+
29+
const drivers: SqlDriver[] = [];
30+
function make(filename: string): SqlDriver {
31+
const d = new SqlDriver({ client: 'better-sqlite3', connection: { filename }, useNullAsDefault: true });
32+
drivers.push(d);
33+
return d;
34+
}
35+
36+
/** `connect()`'s rejection, or a failure saying it resolved. */
37+
async function refusalOf(driver: SqlDriver): Promise<any> {
38+
try {
39+
await driver.connect();
40+
} catch (error) {
41+
return error;
42+
}
43+
throw new Error('connect() resolved for a file the datasource cannot use');
44+
}
45+
46+
function expectFileRefusal(error: any, filename: string, code: string): void {
47+
expect(error).toBeInstanceOf(SqliteDatabaseFileError);
48+
expect(error.code).toBe(code);
49+
expect(error.filename).toBe(filename);
50+
expect(error.message).toContain(filename);
51+
}
52+
53+
/** A real SQLite database in WAL (this driver's journal mode), holding one row in `t`. */
54+
async function seedDatabase(filename: string): Promise<void> {
55+
const seed = new SqlDriver({ client: 'better-sqlite3', connection: { filename }, useNullAsDefault: true });
56+
await seed.connect();
57+
await seed.execute('CREATE TABLE t (a INTEGER)');
58+
await seed.execute('INSERT INTO t (a) VALUES (7)');
59+
await seed.disconnect();
60+
}
61+
62+
/** A seeded database cut short of the pages its schema names: its header still says WAL. */
63+
async function damagedWalDatabase(filename: string): Promise<Buffer> {
64+
await seedDatabase(filename);
65+
truncateSync(filename, 4096);
66+
const before = readFileSync(filename);
67+
// Byte 18 is the header's write version: 2 is WAL.
68+
expect(before[18]).toBe(2);
69+
return before;
70+
}
71+
72+
/**
73+
* This process's open file descriptors on `filename` (or on any of its SQLite
74+
* siblings), read from `/proc/self/fd` — the operating system's own count of
75+
* open handles, so no driver internals are trusted to report themselves.
76+
*/
77+
function openHandlesOn(filename: string): string[] {
78+
const fds = '/proc/self/fd';
79+
const open: string[] = [];
80+
for (const fd of readdirSync(fds)) {
81+
let target: string;
82+
try {
83+
target = readlinkSync(join(fds, fd));
84+
} catch {
85+
continue; // the descriptor readdir itself used, already closed
86+
}
87+
if (target.startsWith(filename)) open.push(target);
88+
}
89+
return open;
90+
}
91+
92+
let dir: string;
93+
beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'os-25084-')); });
94+
afterEach(async () => {
95+
vi.unstubAllEnvs();
96+
vi.restoreAllMocks();
97+
await Promise.all(drivers.splice(0).map((d) => d.disconnect().catch(() => {})));
98+
rmSync(dir, { recursive: true, force: true });
99+
});
100+
101+
describe('SqlDriver.connect — a refused SQLite file leaves no open handle behind (#25084)', () => {
102+
it('a damaged file whose header says WAL: same SQLITE_CORRUPT refusal, and no -shm / -wal sibling once connect() rejects (NODE_ENV=production)', async () => {
103+
vi.stubEnv('NODE_ENV', 'production');
104+
const filename = join(dir, 'archive.db');
105+
const before = await damagedWalDatabase(filename);
106+
107+
const error = await refusalOf(make(filename));
108+
109+
expectFileRefusal(error, filename, 'SQLITE_CORRUPT');
110+
expect((error.cause as { code?: string }).code).toBe('SQLITE_CORRUPT');
111+
expect(error.message).toContain((error.cause as Error).message);
112+
// Read before any disconnect(): the refused connection is already closed.
113+
expect(readdirSync(dir)).toEqual(['archive.db']);
114+
expect(readFileSync(filename).equals(before)).toBe(true);
115+
});
116+
117+
describe.runIf(existsSync('/proc/self/fd'))('the handle itself, counted by the operating system', () => {
118+
it('SQLITE_NOTADB: the same refusal, and no descriptor left open on the file', async () => {
119+
const filename = join(dir, 'archive.db');
120+
writeFileSync(filename, 'archive notes — plain text, not a SQLite database\n');
121+
122+
const error = await refusalOf(make(filename));
123+
124+
expectFileRefusal(error, filename, 'SQLITE_NOTADB');
125+
expect((error.cause as { code?: string }).code).toBe('SQLITE_NOTADB');
126+
expect(openHandlesOn(filename)).toEqual([]);
127+
expect(readdirSync(dir)).toEqual(['archive.db']);
128+
});
129+
130+
it('SQLITE_CORRUPT: the same refusal, and no descriptor left open on the file or its siblings', async () => {
131+
const filename = join(dir, 'archive.db');
132+
await damagedWalDatabase(filename);
133+
134+
expectFileRefusal(await refusalOf(make(filename)), filename, 'SQLITE_CORRUPT');
135+
expect(openHandlesOn(filename)).toEqual([]);
136+
});
137+
138+
it('SQLITE_CANTOPEN: the same refusal, and no descriptor left open', async () => {
139+
// A link to a file in a directory that does not exist: SQLite cannot open it.
140+
const filename = join(dir, 'archive.db');
141+
symlinkSync(join(dir, 'missing', 'target.db'), filename);
142+
143+
const error = await refusalOf(make(filename));
144+
145+
expectFileRefusal(error, filename, 'SQLITE_CANTOPEN');
146+
expect(openHandlesOn(filename)).toEqual([]);
147+
expect(openHandlesOn(join(dir, 'missing'))).toEqual([]);
148+
});
149+
150+
it('CONTROL: a healthy file connects and keeps its handle until disconnect()', async () => {
151+
const filename = join(dir, 'archive.db');
152+
await seedDatabase(filename);
153+
154+
const driver = make(filename);
155+
await driver.connect();
156+
expect(await driver.execute('SELECT a FROM t')).toEqual([{ a: 7 }]);
157+
expect(openHandlesOn(filename)).not.toEqual([]);
158+
159+
await driver.disconnect();
160+
expect(openHandlesOn(filename)).toEqual([]);
161+
});
162+
});
163+
164+
it('the refused instance connects again: the same refusal while the file is bad, and a working connection once it is repaired', async () => {
165+
// A host-adopted driver is connected again on the SAME object
166+
// (`createPrebuiltDriverFactory`), so releasing must leave it usable.
167+
const filename = join(dir, 'archive.db');
168+
await damagedWalDatabase(filename);
169+
const driver = make(filename);
170+
171+
expectFileRefusal(await refusalOf(driver), filename, 'SQLITE_CORRUPT');
172+
expectFileRefusal(await refusalOf(driver), filename, 'SQLITE_CORRUPT');
173+
expect(readdirSync(dir)).toEqual(['archive.db']);
174+
175+
// The operator restores the file from a good copy.
176+
const good = join(dir, 'restored.db');
177+
await seedDatabase(good);
178+
rmSync(filename);
179+
renameSync(good, filename);
180+
181+
await driver.connect();
182+
expect(await driver.execute('SELECT a FROM t')).toEqual([{ a: 7 }]);
183+
});
184+
185+
it('a release that itself fails is logged at warn and the original refusal goes on unchanged', async () => {
186+
const filename = join(dir, 'archive.db');
187+
await damagedWalDatabase(filename);
188+
const driver = make(filename);
189+
const knex = (driver as any).knex;
190+
const realDestroy = knex.destroy.bind(knex);
191+
const releaseFailure = new Error('pool would not close');
192+
vi.spyOn(knex, 'destroy').mockRejectedValueOnce(releaseFailure).mockImplementation(realDestroy);
193+
const warn = vi.spyOn((driver as any).logger, 'warn');
194+
195+
const error = await refusalOf(driver);
196+
197+
expectFileRefusal(error, filename, 'SQLITE_CORRUPT');
198+
expect(error).not.toBe(releaseFailure);
199+
expect(warn).toHaveBeenCalledWith(expect.any(String), releaseFailure);
200+
});
201+
202+
it('CONTROL: a healthy WAL file connects and keeps working, its -shm / -wal held open beside it', async () => {
203+
const filename = join(dir, 'archive.db');
204+
await seedDatabase(filename);
205+
206+
const driver = make(filename);
207+
await driver.connect();
208+
await driver.execute('INSERT INTO t (a) VALUES (8)');
209+
expect(await driver.execute('SELECT a FROM t ORDER BY a')).toEqual([{ a: 7 }, { a: 8 }]);
210+
expect(readdirSync(dir).sort()).toEqual(['archive.db', 'archive.db-shm', 'archive.db-wal']);
211+
});
212+
});

‎packages/drivers/driver-sql/src/sql-driver.ts‎

Lines changed: 56 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7004,22 +7004,64 @@ export class SqlDriver implements IDataDriver {
70047004
// ===================================
70057005

70067006
async connect(): Promise<void> {
7007-
// Ensure the database directory exists before any query can trigger
7008-
// better-sqlite3 to open the file (e.g. loadMetaFromDb on startup).
7009-
await this.ensureDatabaseExists();
7007+
try {
7008+
// Ensure the database directory exists before any query can trigger
7009+
// better-sqlite3 to open the file (e.g. loadMetaFromDb on startup).
7010+
await this.ensureDatabaseExists();
70107011

7011-
if (this.isSqlite) {
7012-
// Both pragmas below are persistent properties of the FILE, so one
7013-
// connection setting them is enough for every process that follows.
7014-
// Each is READ first and set only when the file answers differently, so
7015-
// a connect to a file an earlier boot already configured writes nothing
7016-
// to it (#21734) — one rule for every connect, read-only boots included.
7017-
// A `false` means the very first statement on this connection failed —
7018-
// in practice a native addon that cannot load — so asking it for another
7019-
// pragma would only repeat the same warning.
7020-
if (await this.applyAutoVacuumIncremental()) {
7021-
await this.applySqliteJournalMode();
7012+
if (this.isSqlite) {
7013+
// Both pragmas below are persistent properties of the FILE, so one
7014+
// connection setting them is enough for every process that follows.
7015+
// Each is READ first and set only when the file answers differently, so
7016+
// a connect to a file an earlier boot already configured writes nothing
7017+
// to it (#21734) — one rule for every connect, read-only boots included.
7018+
// A `false` means the very first statement on this connection failed —
7019+
// in practice a native addon that cannot load — so asking it for another
7020+
// pragma would only repeat the same warning.
7021+
if (await this.applyAutoVacuumIncremental()) {
7022+
await this.applySqliteJournalMode();
7023+
}
70227024
}
7025+
} catch (error) {
7026+
if (this.isSqlite) await this.releaseRefusedSqliteConnection();
7027+
throw error;
7028+
}
7029+
}
7030+
7031+
/**
7032+
* Close what a refused SQLite `connect()` opened, before the refusal leaves
7033+
* it (#25084).
7034+
*
7035+
* A file refused by its first statement ({@link SQLITE_FILE_REFUSALS}) was
7036+
* opened to answer it: the pool keeps that better-sqlite3 connection, and for
7037+
* a file whose header says WAL, SQLite keeps its `-shm` and an empty `-wal`
7038+
* beside it for as long as the connection is open. No caller is owed that
7039+
* handle — the connect failed — and a caller that does not `disconnect()` a
7040+
* handle whose `connect()` rejected (the datasource connection service's
7041+
* degraded path) used to keep it for the life of the process.
7042+
*
7043+
* The pool is destroyed and a fresh, empty one put in its place, so the
7044+
* instance is left as it was constructed: nothing open, and a later
7045+
* `connect()` opens the file again and gives its own answer. Destroying
7046+
* alone would not: every later statement would answer "Unable to acquire a
7047+
* connection", which carries no code, so a second `connect()` would log it
7048+
* and resolve over a datasource that cannot run one query. A host-adopted
7049+
* instance is connected again on the same object (`createPrebuiltDriverFactory`
7050+
* in `@objectstack/service-datasource`), so that second `connect()` is real.
7051+
*
7052+
* SQLite only: it is the one dialect whose `connect()` can reject holding a
7053+
* connection. A server dialect rejects there only when the connection itself
7054+
* could not be made, so its pool holds nothing to release.
7055+
*
7056+
* Never throws: the refusal the caller is owed is the error `connect()`
7057+
* raised, so a release that fails is logged and that error goes on unchanged.
7058+
*/
7059+
private async releaseRefusedSqliteConnection(): Promise<void> {
7060+
try {
7061+
await this.knex.destroy();
7062+
this.knex.initialize();
7063+
} catch (e) {
7064+
this.logger.warn('Failed to close the SQLite connection a refused connect() opened', e);
70237065
}
70247066
}
70257067

0 commit comments

Comments
 (0)