Skip to content

sqlite: copy changeset before applying it - #65286

Open
mcollina wants to merge 2 commits into
nodejs:mainfrom
mcollina:fix-sqlite-apply-changeset-buffer
Open

sqlite: copy changeset before applying it#65286
mcollina wants to merge 2 commits into
nodejs:mainfrom
mcollina:fix-sqlite-apply-changeset-buffer

Conversation

@mcollina

Copy link
Copy Markdown
Member

SQLite can invoke JavaScript conflict and filter callbacks while applying a changeset. Copy the input first so detaching or modifying its backing buffer during a callback does not affect the active operation.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@mcollina
mcollina requested review from Renegade334 and trivikr and removed request for trivikr August 14, 2026 14:41
@nodejs-github-bot 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 14, 2026
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cc57cb7) to head (994c5d3).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 58.82% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65286      +/-   ##
==========================================
- Coverage   90.12%   90.12%   -0.01%     
==========================================
  Files         751      751              
  Lines      252536   252558      +22     
  Branches    47519    47518       -1     
==========================================
+ Hits       227598   227612      +14     
  Misses      16250    16250              
- Partials     8688     8696       +8     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.89% <58.82%> (-0.15%) ⬇️

... and 33 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.

Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

SQLite can invoke JavaScript conflict and filter callbacks while
applying a changeset. Copy the input first so detaching or modifying
its backing buffer during a callback does not affect the active
operation.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Refactor: only copy the changeset before applying it when a filter or
onConflict callback is present, since SQLite can only invoke JavaScript
mid-apply in that case. Without callbacks, the input buffer cannot be
detached or modified during sqlite3changeset_apply(), so no copy is
needed and the buffer is passed through directly.

Address review comment from geeksilva97.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
mcollina force-pushed the fix-sqlite-apply-changeset-buffer branch from 385708b to 994c5d3 Compare August 23, 2026 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants