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.

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>
@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 77.27273% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.10%. Comparing base (41afbd3) to head (385708b).
⚠️ Report is 244 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 77.27% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65286      +/-   ##
==========================================
- Coverage   90.30%   90.10%   -0.21%     
==========================================
  Files         759      752       -7     
  Lines      247597   252224    +4627     
  Branches    46679    47449     +770     
==========================================
+ Hits       223591   227258    +3667     
- Misses      15469    16277     +808     
- Partials     8537     8689     +152     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.72% <77.27%> (+1.20%) ⬆️

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

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