sqlite: StatementSync#iterate fails with Error: statement has been finalized

[melusc]

Version

v23.10.0

Platform

- Microsoft Windows NT 10.0.26100.0 x64
- Microsoft Windows Server 2022 10.0.20348 (Github actions)
- Ubuntu 24.04.2 LTS (Github actions)

Subsystem

sqlite

What steps will reproduce the bug?

index.js

// = Setup =
import { DatabaseSync } from "node:sqlite";

const database = new DatabaseSync(":memory:");

database.exec(`
	CREATE TABLE users (
		user_id  INTEGER PRIMARY KEY AUTOINCREMENT,
		name     TEXT NOT NULL UNIQUE
	);
`);

for (let index = 0; index < 1e3; ++index) {
	database.prepare(`INSERT INTO users (name) VALUES (:name);`).run({
		name: Math.random().toString(36).slice(2),
	});
}

// Double check all 1000 rows are inserted
console.log(database.prepare("SELECT count(*) as row_count FROM users;").get());

// = Actual buggy behaviour starts here =

// const iter = [...database.prepare('SELECT * from users;').iterate()][Symbol.iterator]();
const iter = database.prepare("SELECT * from users;").iterate();

for (const row of iter) {
	console.log(row.user_id, row.name);
}

node index.js

How often does it reproduce? Is there a required condition?

Happens every time. Sometimes it errors, sometimes it just stops unexpectedly without an error.

It doesn't reproduce if you uncomment the line where I fetch all rows immediately by spreading it into an array and then turning it back into an iterator, so the issue comes from reading each value over time.

What is the expected behavior? Why is that the expected behavior?

It should not error and it should print 1000 rows.

What do you see instead?

90% of runs

293 27p5p7fqtms
294 pn2ij0uxxsq
[...]/sqlite-iterate.js:25
for (const row of iter) {
           ^

Error: statement has been finalized
    at [...]/sqlite-iterate.js:25:12
    at ModuleJob.run (node:internal/modules/esm/module_job:274:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:644:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:98:5) {
  code: 'ERR_INVALID_STATE'
}

Node.js v23.10.0

OR

10% it just stops without an error. It should have printed up to 1000.

279 pjvk19yrl7h
280 xrj1kp6l93
281 e73tq3381ff
282 q93lik5apn
283 6kx6awjjuce
284 3h47eaan34j
285 rkrljlp5qnm
PS [...]\5d9342b0-468e-4b12-b854-28a5775d2fab> 

---

Sometimes the values of the last "successful" row is garbage. Sometimes for name it prints undefined or something like ÙÙ¥Ñ88. The name should always match 0-9a-z so there is some sort of corruption, too.

Additional information

• Github actions log if it helps: https://github.com/melusc/node-bug-sqlite-iterate/actions/runs/13882797889
  You can see on the Ubuntu run, it exits with an error. On the Windows run, it just exits for no apparent reason. That is random, though, not platform-dependent.

[cjihrig]

cc: @tpoisseau

[tpoisseau]

Hello,

Today .iterate() is tested only with .toArray(). I assert if toArrayworks, it's OK for iteration and iterators helpers. And it's tested on a few rows.

I don't know when I will have time to work on this, v8 CPP is really hard for me. It look like, somewhere, the statement is closed / free during the iteration. I have no idea why.

[fuchsia]

I think the problem is the iterator doesn't have a reference to the StatementSync object returned by database.prepare(). So as soon as database.prepare("SELECT * from users;").iterate() has been called, the GC is free to collect the intermediate StatementSync object. And, when it does, the underlying sqlite statement is finalised.

Proof: edit the code to this:

const stmt = database.prepare("SELECT * from users;"); 
const iter = stmt.iterate();

That seems to work till the cows come home, because I'm now hanging on to stmt and so it can't be GC'd.

I imagine the solution is to create a ref to the StatementSync object in the iterator object returned by iterate() (i.e. from inside StatementSync::Iterate(const FunctionCallbackInfo<Value>&)) e.g. a [Symbol.stmt] property that links to it.

[cjihrig]

@fuchsia I believe your assessment is correct. The only thing I'd add is that the iterator can hold a reference to the StatementSync object without the reference being exposed to users in JavaScript.

[fuchsia]

I expect to have some time next week when I could have a go at writing a fix. I've not built node before, and getting my system set up to do that is the most daunting bit! I've plenty of experience of C++ and using sqlite3 from C++, although it's a bit rusty.

[fox1t]

I just encountered this issue while working on https://github.com/ducktors/Valkeyrie

I have this test case that returns this error while I would expect a different one. Something like "Error: the db is closed', but it throws "Error: statement has been finalized" instead.

This is the test case:

await test('Valkeyrie explicit resource management', async () => {
    let db2: Valkeyrie

    {
      await using db = await Valkeyrie.open()
      db2 = db

      const res = await db.get(['a'])
      assert.strictEqual(res.versionstamp, null)
    }

    await assert.rejects(() => db2.get(['a']), Error)
  })

Is this the expected behavior? Do I need to track the status of the database in the user land?

[cjihrig]

@fox1t what you're seeing is not the same as this issue, which is specific to iterate().

Is this the expected behavior?

You're using an additional API on top of node:sqlite, but I think it's expected. In this code, I expect db to close once it goes out of scope even though you've aliased it. Once the database is closed, all of the prepared statements associated with that database are finalized.

Something like "Error: the db is closed', but it throws "Error: statement has been finalized" instead.

If you try to use a database after it's closed, you get a message that the database is closed. If you try to use an individual prepared statement after the database is closed or the statement is otherwise finalized, you get the finalized error.

Do I need to track the status of the database in the user land?

No, you shouldn't need to. There is also going to be a method for this in the next release (#57522).

[fox1t]

@fox1t what you're seeing is not the same as this issue, which is specific to iterate().

Is this the expected behavior?

You're using an additional API on top of node:sqlite, but I think it's expected. In this code, I expect db to close once it goes out of scope even though you've aliased it. Once the database is closed, all of the prepared statements associated with that database are finalized.

Something like "Error: the db is closed', but it throws "Error: statement has been finalized" instead.

If you try to use a database after it's closed, you get a message that the database is closed. If you try to use an individual prepared statement after the database is closed or the statement is otherwise finalized, you get the finalized error.

Do I need to track the status of the database in the user land?

No, you shouldn't need to. There is also going to be a method for this in the next release (#57522).

Oh, @cjihrig, that's awesome. Thanks for sharing this info about the finalization of the statements. I didn't realize it until now. That's exactly what happens since I prepare all queries at the "driver" level. It is a tiny wrapper of the underlying backend (SQLite in this case) that doesn't add any logic. It's cool that isOpen is being added!