src: add support for AbortSignal in backup method #59333

lluisemper · 2025-08-02T18:30:10Z

This adds support for passing an AbortSignal to the database backup function. If the signal is aborted during the backup process, the operation is interrupted and a rejection is returned with an AbortError.

The signal is optionally passed through the options object. Internally, the signal is stored and periodically checked between backup steps to respond quickly to abort requests.

This improves integration with modern web APIs and aligns with how abortable operations are handled elsewhere in the Node.js ecosystem.

Fixes: #58888

nodejs-github-bot · 2025-08-02T18:30:15Z

Review requested:

@nodejs/sqlite

himself65 · 2025-08-03T00:53:48Z

src/node_sqlite.cc

+                     Local<Function> progressFunc,
+                     Local<Object> abort_signal = Local<Object>())


just noticed we have different naming style here. both snake_case and camelcase

All the parameters apart from progressFunc uses snake_case, I will rename this to be more consistent.

However, I'm not really sure what to do here. The naming style seems inconsistent across the file. Just do a search for Local<Function> and you'll find both snake_case and camelCase being used.

himself65 · 2025-08-03T00:59:46Z

src/node_sqlite.cc

+    }
  }

+  void AbortBackup() { is_aborted_.store(true); }


this function is not used

himself65 · 2025-08-03T01:03:17Z

src/node_sqlite.cc

+    }
+
+    if (!signal_v->IsUndefined()) {
+      if (!signal_v->IsObject()) {


should we check if input is abort signal? just realized that the code is the first time brings user's abort signal into C++

node/lib/internal/validators.js

Lines 442 to 450 in 0fd1ecd

/** @type {validateAbortSignal} */

const validateAbortSignal = hideStackFrames((signal, name) => {

if (signal !== undefined &&

(signal === null ||

typeof signal !== 'object' ||

!('aborted' in signal))) {

throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);

}

});

this is a duplicate check if we have JS code

himself65 · 2025-08-03T01:09:49Z

lib/sqlite.js

+    if (typeof options.signal !== 'object' || options.signal === null) {
+      throw new ERR_INVALID_STATE.TypeError('The "options.signal" argument must be an AbortSignal.');
+    }


we already have abort signal check

node/lib/internal/validators.js

Lines 442 to 450 in 0fd1ecd

/** @type {validateAbortSignal} */

const validateAbortSignal = hideStackFrames((signal, name) => {

if (signal !== undefined &&

(signal === null ||

typeof signal !== 'object' ||

!('aborted' in signal))) {

throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);

}

});

himself65 · 2025-08-03T01:10:35Z

lib/sqlite.js

-module.exports = internalBinding('sqlite');
+const binding = internalBinding('sqlite');
+
+function backup(sourceDb, path, options = {}) {


should this function all in C++

I was wondering why we would need to send it to the C++ layer if it is not valid. On the other hand, it might look cleaner to leave this file as it is and add everything in C++ instead.

himself65 · 2025-08-03T01:18:03Z

src/node_sqlite.cc

  }

  void DoThreadPoolWork() override {
+    if (is_aborted_.load()) {


load(std::memory_order_acquire)

himself65 · 2025-08-03T01:18:11Z

src/node_sqlite.cc

    Local<Promise::Resolver> resolver =
        Local<Promise::Resolver>::New(env()->isolate(), resolver_);

+    if (is_aborted_.load() || backup_status_ == SQLITE_INTERRUPT) {


load(std::memory_order_acquire)

himself65 · 2025-08-03T01:20:47Z

src/node_sqlite.cc

+    }
  }

+  void AbortBackup() { is_aborted_.store(true); }


store(true, std::memory_order_release)

himself65 · 2025-08-03T01:25:19Z

src/node_sqlite.cc

+    Local<Value> aborted_value;
+    if (signal->Get(env()->context(), aborted_key).ToLocal(&aborted_value)) {
+      if (aborted_value->BooleanValue(isolate)) {
+        is_aborted_.store(true);


himself65 · 2025-08-03T01:33:12Z

src/node_sqlite.cc

+        ->Set(env()->context(),
+              String::NewFromUtf8(isolate, "name").ToLocalChecked(),
+              name)
+        .ToChecked();


should check the error here

inline MaybeLocal<Object> CreateAbortError(Isolate* isolate, const char* message = "The operation was aborted") { Local<String> js_msg; Local<Object> e; if (!String::NewFromUtf8(isolate, message).ToLocal(&js_msg) || !Exception::Error(js_msg) ->ToObject(isolate->GetCurrentContext()) .ToLocal(&e) || e->Set(isolate->GetCurrentContext(), String::NewFromUtf8(isolate, "name").ToLocalChecked(), String::NewFromUtf8(isolate, "AbortError").ToLocalChecked()) .IsNothing()) { return MaybeLocal<Object>(); } return e; } void HandleAbortError(Local<Promise::Resolver> resolver) { Local<Object> e; if (!CreateAbortError(env()->isolate()).ToLocal(&e)) { Finalize(); return; } Finalize(); resolver->Reject(env()->context(), e).ToChecked(); }

btw, AbortError is in JS side, we should get that class to make consistently

codecov · 2025-08-03T02:26:51Z

Codecov Report

❌ Patch coverage is 35.43307% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (60b9aaa) to head (129f7bd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	18.00%	73 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59333      +/-   ##
==========================================
- Coverage   88.54%   88.50%   -0.05%     
==========================================
  Files         703      703              
  Lines      208077   208195     +118     
  Branches    40082    40108      +26     
==========================================
+ Hits       184252   184265      +13     
- Misses      15848    15961     +113     
+ Partials     7977     7969       -8

Files with missing lines	Coverage Δ
lib/sqlite.js	`100.00% <100.00%> (ø)`
src/node_sqlite.cc	`77.03% <18.00%> (-2.90%)`	⬇️

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lluisemper · 2025-08-03T13:37:55Z

src/node_sqlite.cc

    }                                                                          \
  } while (0)

+class AbortError {


@himself65 I have tried to implement this class as you suggested, I hope I have not misunderstood 😂 I have check its js implementation from here: https://github.com/nodejs/node/blob/main/lib/internal/errors.js#L976-L988

lluisemper · 2025-08-03T13:42:37Z

@himself65 Thank you very much for your review ❤️ I learned a lot from it!
I've made the following changes:

Extended the tests to cover input validation
Implemented an AbortError class to align with the JS side
Used the AbortSignal validator from internal/validators
Addressed the minor suggestions and changes you mentioned

In addition to this, I need to mention that when I ran make test-doc I get an error:

/node/tools/doc/apilinks.mjs:100
            if (property.value.type === 'Identifier') {
                               ^

TypeError: Cannot read properties of undefined (reading 'type')

It is not clear to me what I have done wrong or how to correct this, any pointers will be welcome ☺️

addaleax · 2025-08-04T11:04:04Z

src/node_sqlite.cc

+ public:
+  static MaybeLocal<Object> New(
+      Isolate* isolate,
+      const char* message = "The operation was aborted",


nit: I'd consider using std::string_view here

addaleax · 2025-08-04T11:11:18Z

src/node_sqlite.cc

+
+    if (error_obj
+            ->Set(context,
+                  String::NewFromUtf8(isolate, "name").ToLocalChecked(),


For fixed constants, you'll probably want to use FIXED_ONE_BYTE_STRING()

do we not already have a env->name_string()?

Also, just in general, avoid the use of the ToLocalChecked() in these.

do we not already have a env->name_string()?

Yes, in this specific case we should prefer that 👍 My comment was more generally meant for the rest of the PR.

Also, just in general, avoid the use of the ToLocalChecked() in these.

Yeah, it's a bit odd, for creating fixed-length strings this is generally fine but that's really a special case and it's probably better if we use something like ToV8Value() and add proper error handling, yes, if only it's because then we're sticking to a pattern that makes sense when used everywhere.

src/node_sqlite.cc

doc/api/sqlite.md

lib/sqlite.js

src/node_sqlite.cc

test/parallel/test-sqlite-backup-abort.js

jasnell · 2025-08-04T19:34:29Z

lib/sqlite.js

+  validateString(path, 'options.headers.host');
+  validateAbortSignal(options.signal, 'options.signal');
+  if (options.signal?.aborted) {
+    return PromiseReject(new AbortError(undefined, { cause: options.signal.reason }));


One issue I can see with this, which does not need to be addressed now, is that this will mean we have two AbortError types, one defined in JavaScript another defined in C++. They'll both be named AbortError but won't be the same class so instanceof checks between the two won't work. Not a big deal but it might be good to leave a todo somewhere for someone later to reconcile the two into a single implementation.

Thanks for pointing this out, I wasn't sure if I understood this correctly when I was told in a previous comment! I'd like to follow up on this after landing the current PR. It definitely makes sense to reconcile the two AbortError implementations so instanceof works consistently from js.

Would you happen to have any pointers on the preferred approach for reusing the js native AbortError in C++?

I have put the comment here: https://github.com/nodejs/node/pull/59333/files#diff-dd810db4fe69364c3a67a274e0725f386040c0fd1dcfade7093f23c8514328aeR119-R120

This adds support for passing an AbortSignal to the database backup function. If the signal is aborted during the backup process, the operation is interrupted and a rejection is returned with an AbortError. The signal is optionally passed through the options object. Internally, the signal is stored and periodically checked between backup steps to respond quickly to abort requests. This improves integration with modern web APIs and aligns with how abortable operations are handled elsewhere in the Node.js ecosystem. Fixes: nodejs#58888

lluisemper · 2025-08-15T09:20:05Z

Following up to see what the next steps might be here. I’m happy to continue iterating on the changes, or if it makes more sense for a more experienced C++ contributor to pick it up, I’m fine with that too.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Aug 2, 2025

himself65 reviewed Aug 3, 2025

View reviewed changes

lluisemper force-pushed the 58888 branch from 2f3df21 to a114e04 Compare August 3, 2025 13:36

lluisemper commented Aug 3, 2025

View reviewed changes

lluisemper requested a review from himself65 August 3, 2025 13:38

addaleax reviewed Aug 4, 2025

View reviewed changes