Repository: denoland/deno
Pull request: fix(ext/node): `sqlite:backup` compatibility

Notable changes:
- Respect progress callback function option.
- Set default backup rate to 100 to follow Node's implementation.
- Various validation and error handling.

Note that the op implementation in this PR is still sync.

Given this code:
```js
import { backup, DatabaseSync } from "node:sqlite";

const populate = (database, rows) => {
  database.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT);");
  let values = "";
  for (let i = 0; i < rows; i++) {
    values += `(${i}, 'Name ${i}'),`;
  }
  values = values.slice(0, -1);
  database.exec(`INSERT INTO test (id, name) VALUES ${values}`);
};

const backupPath = "backup.sqlite";
const database = new DatabaseSync(":memory:");
populate(database, 1000);

const t0 = performance.now();
await backup(database, backupPath, { rate: 1 });
const t1 = performance.now();
database.close();
console.log(`Backup completed in ${(t1 - t0).toFixed(2)} ms`);
```

Results:
```bash
# Node.js v25.2.1
➜ node ./backup-benchmark.js
Backup completed in 0.71 ms

# This PR (debug build mode)
➜ ddeno -A ./backup-benchmark.js
Backup completed in 0.71 ms

# Deno v2.6.0
➜ deno -A ./backup-benchmark.js
Backup completed in 1272.16 ms
```

The current implementation is slow because it sleeps 250ms after each `sqlite3_backup_step` call.

Changed files:
filename: ext/node/ops/sqlite/backup.rs
patch: @@ -1,51 +1,217 @@
 // Copyright 2018-2025 the Deno authors. MIT license.
 
 use std::borrow::Cow;
+use std::cell::RefCell;
 use std::ffi::c_int;
-use std::time;
+use std::rc::Rc;
 
 use deno_core::OpState;
 use deno_core::op2;
+use deno_core::v8;
+use deno_core::v8_static_strings;
 use deno_permissions::OpenAccessKind;
 use deno_permissions::PermissionsContainer;
 use rusqlite::Connection;
 use rusqlite::backup;
-use serde::Deserialize;
-use serde::Serialize;
 
 use super::DatabaseSync;
 use super::SqliteError;
+use super::validators;
 
-const DEFAULT_BACKUP_RATE: c_int = 5;
+const DEFAULT_BACKUP_RATE: c_int = 100;
 
-#[derive(Serialize, Deserialize)]
-struct BackupOptions {
-  source: Option<String>,
-  target: Option<String>,
-  rate: Option<c_int>,
+struct BackupOptions<'a> {
+  source: String,
+  target: String,
+  rate: i32,
+  progress: Option<v8::Local<'a, v8::Function>>,
 }
 
-#[op2(stack_trace)]
+impl<'a> Default for BackupOptions<'a> {
+  fn default() -> Self {
+    BackupOptions {
+      source: "main".to_string(),
+      target: "main".to_string(),
+      rate: DEFAULT_BACKUP_RATE,
+      progress: None,
+    }
+  }
+}
+
+impl<'a> BackupOptions<'a> {
+  fn from_value(
+    scope: &mut v8::PinScope<'a, '_>,
+    value: v8::Local<'a, v8::Value>,
+  ) -> Result<Self, validators::Error> {
+    let mut options = BackupOptions::default();
+
+    if value.is_undefined() {
+      return Ok(options);
+    }
+
+    let Ok(obj) = v8::Local::<v8::Object>::try_from(value) else {
+      return Err(validators::Error::InvalidArgType(
+        "The \"options\" argument must be an object.",
+      ));
+    };
+
+    v8_static_strings! {
+      SOURCE_STRING = "source",
+      TARGET_STRING = "target",
+      RATE_STRING = "rate",
+      PROGRESS_STRING = "progress",
+    }
+
+    let source_string = SOURCE_STRING.v8_string(scope).unwrap();
+    if let Some(source_val) = obj.get(scope, source_string.into())
+      && !source_val.is_undefined()
+    {
+      let source_str =
+        v8::Local::<v8::String>::try_from(source_val).map_err(|_| {
+          validators::Error::InvalidArgType(
+            "The \"options.source\" argument must be a string.",
+          )
+        })?;
+      options.source = source_str.to_rust_string_lossy(scope);
+    }
+
+    let target_string = TARGET_STRING.v8_string(scope).unwrap();
+    if let Some(target_val) = obj.get(scope, target_string.into())
+      && !target_val.is_undefined()
+    {
+      let target_str =
+        v8::Local::<v8::String>::try_from(target_val).map_err(|_| {
+          validators::Error::InvalidArgType(
+            "The \"options.target\" argument must be a string.",
+          )
+        })?;
+      options.target = target_str.to_rust_string_lossy(scope);
+    }
+
+    let rate_string = RATE_STRING.v8_string(scope).unwrap();
+    if let Some(rate_val) = obj.get(scope, rate_string.into())
+      && !rate_val.is_undefined()
+    {
+      let rate_int = v8::Local::<v8::Integer>::try_from(rate_val)
+        .map_err(|_| {
+          validators::Error::InvalidArgType(
+            "The \"options.rate\" argument must be an integer.",
+          )
+        })?
+        .value();
+
+      options.rate = i32::try_from(rate_int).map_err(|_| {
+        validators::Error::InvalidArgType(
+          "The \"options.rate\" argument must be an integer.",
+        )
+      })?;
+    }
+
+    let progress_string = PROGRESS_STRING.v8_string(scope).unwrap();
+    if let Some(progress_val) = obj.get(scope, progress_string.into())
+      && !progress_val.is_undefined()
+    {
+      let progress_fn = v8::Local::<v8::Function>::try_from(progress_val)
+        .map_err(|_| {
+          validators::Error::InvalidArgType(
+            "The \"options.progress\" argument must be a function.",
+          )
+        })?;
+      options.progress = Some(progress_fn);
+    }
+
+    Ok(options)
+  }
+}
+
+#[op2(reentrant, stack_trace)]
 #[smi]
-pub fn op_node_database_backup(
-  state: &mut OpState,
+pub fn op_node_database_backup<'a>(
+  state: Rc<RefCell<OpState>>,
+  scope: &mut v8::PinScope<'a, '_>,
   #[cppgc] source_db: &DatabaseSync,
   #[string] path: &str,
-  #[serde] options: Option<BackupOptions>,
-) -> Result<i32, SqliteError> {
+  options: v8::Local<'a, v8::Value>,
+) -> Result<Option<i32>, SqliteError> {
+  use rusqlite::backup::StepResult::Busy;
+  use rusqlite::backup::StepResult::Done;
+  use rusqlite::backup::StepResult::Locked;
+  use rusqlite::backup::StepResult::More;
+
+  let options = BackupOptions::from_value(scope, options)?;
   let src_conn_ref = source_db.conn.borrow();
-  let src_conn = src_conn_ref.as_ref().ok_or(SqliteError::SessionClosed)?;
-  let path = std::path::Path::new(path);
-  let checked_path = state.borrow_mut::<PermissionsContainer>().check_open(
-    Cow::Borrowed(path),
-    OpenAccessKind::Write,
-    Some("node:sqlite.backup"),
+  let src_conn = src_conn_ref.as_ref().ok_or(SqliteError::AlreadyClosed)?;
+  let checked_path = {
+    let mut state = state.borrow_mut();
+    let permissions = state.borrow_mut::<PermissionsContainer>();
+    permissions.check_open(
+      Cow::Borrowed(std::path::Path::new(path)),
+      OpenAccessKind::Write,
+      Some("node:sqlite.backup"),
+    )?
+  };
+
+  let mut dst_conn = Connection::open(checked_path).map_err(|e| match e {
+    rusqlite::Error::SqliteFailure(err, Some(msg)) => {
+      let message = if err.extended_code == rusqlite::ffi::SQLITE_CANTOPEN {
+        "unable to open database file".to_string()
+      } else {
+        msg
+      };
+      SqliteError::SqliteSysError {
+        message: message.clone(),
+        errstr: message,
+        errcode: err.extended_code as _,
+      }
+    }
+    other_err => SqliteError::from(other_err),
+  })?;
+  let backup = backup::Backup::new_with_names(
+    src_conn,
+    options.source.as_str(),
+    &mut dst_conn,
+    options.target.as_str(),
   )?;
-  let mut dst_conn = Connection::open(checked_path)?;
-  let rate = options
-    .and_then(|opts| opts.rate)
-    .unwrap_or(DEFAULT_BACKUP_RATE);
-  let backup = backup::Backup::new(src_conn, &mut dst_conn)?;
-  backup.run_to_completion(rate, time::Duration::from_millis(250), None)?;
-  Ok(backup.progress().pagecount)
+
+  v8_static_strings! {
+    TOTAL_PAGES_STRING = "totalPages",
+    REMAINING_PAGES_STRING = "remainingPages",
+  }
+
+  loop {
+    let step_result = backup.step(options.rate)?;
+    if let Some(progress_fn) = options.progress {
+      let progress = backup.progress();
+      if progress.remaining != 0 {
+        let js_progress_obj = v8::Object::new(scope);
+        let total_pages_string = TOTAL_PAGES_STRING.v8_string(scope).unwrap();
+        let remaining_pages_string =
+          REMAINING_PAGES_STRING.v8_string(scope).unwrap();
+
+        let total_pages_js = v8::Integer::new(scope, progress.pagecount);
+        let remaining_pages_js = v8::Integer::new(scope, progress.remaining);
+        js_progress_obj.set(
+          scope,
+          total_pages_string.into(),
+          total_pages_js.into(),
+        );
+        js_progress_obj.set(
+          scope,
+          remaining_pages_string.into(),
+          remaining_pages_js.into(),
+        );
+
+        let recv = v8::null(scope).into();
+        let res = progress_fn.call(scope, recv, &[js_progress_obj.into()]);
+        if res.is_none() {
+          // JS exception occurred in progress callback
+          return Ok(None);
+        }
+      }
+    }
+    match step_result {
+      Done => return Ok(Some(backup.progress().pagecount)),
+      More | Busy | Locked | _ => continue,
+    }
+  }
 }

filename: ext/node/polyfills/sqlite.ts
patch: @@ -12,6 +12,7 @@ import { URLPrototype } from "ext:deno_web/00_url.js";
 import type { URL } from "node:url";
 
 const {
+  ObjectDefineProperty,
   ObjectDefineProperties,
   ObjectPrototypeIsPrototypeOf,
   ObjectSetPrototypeOf,
@@ -30,12 +31,10 @@ class ConstructCallRequiredError extends TypeError {
   }
 }
 
-class InvalidPathError extends TypeError {
+class InvalidArgTypeError extends TypeError {
   code: string;
-  constructor() {
-    super(
-      'The "path" argument must be a string, Uint8Array, or URL without null bytes.',
-    );
+  constructor(message: string) {
+    super(message);
     this.code = "ERR_INVALID_ARG_TYPE";
   }
 }
@@ -67,7 +66,9 @@ const parsePath = (path: unknown): string => {
     typeof parsedPath === "undefined" ||
     StringPrototypeIncludes(parsedPath, "\0")
   ) {
-    throw new InvalidPathError();
+    throw new InvalidArgTypeError(
+      'The "path" argument must be a string, Uint8Array, or URL without null bytes.',
+    );
   }
 
   return parsedPath;
@@ -151,17 +152,32 @@ interface BackupProgressInfo {
  * following properties are supported:
  * @returns A promise that resolves when the backup is completed and rejects if an error occurs.
  */
+// deno-lint-ignore require-await
 async function backup(
   sourceDb: DatabaseSync,
   path: string | Buffer | URL,
   options?: BackupOptions,
 ): Promise<number> {
-  return await op_node_database_backup(
+  if (!ObjectPrototypeIsPrototypeOf(DatabaseSync.prototype, sourceDb)) {
+    throw new InvalidArgTypeError(
+      'The "sourceDb" argument must be an object.',
+    );
+  }
+
+  // TODO(Tango992): Implement async op
+  return op_node_database_backup(
     sourceDb,
     parsePath(path),
     options,
   );
 }
+ObjectDefineProperty(backup, "length", {
+  __proto__: null,
+  value: 2,
+  enumerable: false,
+  configurable: true,
+  writable: false,
+});
 
 export const constants = {
   SQLITE_CHANGESET_OMIT: 0,

filename: tests/unit_node/sqlite_test.ts
patch: @@ -3,9 +3,20 @@ import sqlite, { backup, DatabaseSync } from "node:sqlite";
 import { assert, assertEquals, assertThrows } from "@std/assert";
 import * as nodeAssert from "node:assert";
 import { Buffer } from "node:buffer";
+import { writeFileSync } from "node:fs";
 
 const tempDir = Deno.makeTempDirSync();
 
+const populate = (db: DatabaseSync, rows: number) => {
+  db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)");
+  let values = "";
+  for (let i = 0; i < rows; i++) {
+    values += `(${i}, 'Name ${i}'),`;
+  }
+  values = values.slice(0, -1); // Remove trailing comma
+  db.exec(`INSERT INTO test (id, name) VALUES ${values}`);
+};
+
 Deno.test("[node/sqlite] sqlite-type symbol", () => {
   const db = new DatabaseSync(":memory:");
   const sqliteTypeSymbol = Symbol.for("sqlite-type");
@@ -872,3 +883,90 @@ Deno.test("[node/sqlite] accept URL paths", () => {
   Deno.removeSync(dbPathUrl);
   Deno.removeSync(backupPathUrl);
 });
+
+Deno.test("[node/sqlite] database backup fails when dest file is not writable", async () => {
+  const readonlyDestDb = `${tempDir}/readonly_backup.db`;
+  using db = new DatabaseSync(":memory:");
+  writeFileSync(readonlyDestDb, "", { mode: 0o444 });
+
+  await nodeAssert.rejects(async () => {
+    await backup(db, readonlyDestDb);
+  }, {
+    code: "ERR_SQLITE_ERROR",
+    message: "attempt to write a readonly database",
+  });
+
+  Deno.removeSync(readonlyDestDb);
+});
+
+Deno.test("[node/sqlite] progress function to have been called at least once", async () => {
+  const destDb = `${tempDir}/backup_progress.db`;
+  using db = new DatabaseSync(":memory:");
+  populate(db, 100);
+
+  let totalPages: number | undefined;
+  let remainingPages: number | undefined;
+  await backup(db, destDb, {
+    rate: 1,
+    progress: (progress) => {
+      totalPages ??= progress.totalPages;
+      remainingPages ??= progress.remainingPages;
+    },
+  });
+
+  assertEquals(typeof totalPages, "number");
+  assertEquals(typeof remainingPages, "number");
+
+  Deno.removeSync(destDb);
+});
+
+Deno.test("[node/sqlite] backup fails when progress function throws", async () => {
+  const destDb = `${tempDir}/backup_progress.db`;
+  using db = new DatabaseSync(":memory:");
+  populate(db, 100);
+
+  await nodeAssert.rejects(async () => {
+    await backup(db, destDb, {
+      rate: 1,
+      progress: () => {
+        throw new Error("progress error");
+      },
+    });
+  }, {
+    message: "progress error",
+  });
+
+  Deno.removeSync(destDb);
+});
+
+Deno.test("[node/sqlite] backup fails when source db is invalid", async () => {
+  using database = new DatabaseSync(":memory:");
+  const destDb = `${tempDir}/other_backup_progress.db`;
+
+  await nodeAssert.rejects(async () => {
+    await backup(database, destDb, {
+      rate: 1,
+      source: "invalid",
+    });
+  }, {
+    message: "unknown database invalid",
+  });
+
+  Deno.removeSync(destDb);
+});
+
+Deno.test("[node/sqlite] backup fails when path cannot be opened", async () => {
+  using db = new DatabaseSync(":memory:");
+
+  await nodeAssert.rejects(async () => {
+    await backup(db, `${tempDir}/invalid/backup.db`);
+  }, {
+    message: "unable to open database file",
+  });
+});
+
+// https://github.com/nodejs/node/blob/591ba692bfe30408e6a67397e7d18bfa1b9c3561/test/parallel/test-sqlite-backup.mjs#L311-L314
+Deno.test("[node/sqlite] backup has correct name and length", () => {
+  assertEquals(backup.name, "backup");
+  assertEquals(backup.length, 2);
+});