sqlite: keep database alive while a session is open#63797
Open
3zrv wants to merge 5 commits into
Open
Conversation
Signed-off-by: Mohamed Sayed <k@3zrv.com>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Contributor
|
I think a session is supposed to keep the database alive... |
Signed-off-by: Mohamed Sayed <k@3zrv.com>
3zrv
changed the title
Contributor
Author
|
@louwers totally right, the bug was that the session held the database with a weak ref, so it could get GC'd out from under a live session -> dangling pointer -> crash. sqlite requires sessions to be deleted before the connection closes, so a live session with a dead DB is never valid |
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63797 +/- ##
==========================================
- Coverage 90.14% 90.13% -0.01%
==========================================
Files 741 741
Lines 242133 242135 +2
Branches 45568 45562 -6
==========================================
- Hits 218265 218244 -21
- Misses 15371 15401 +30
+ Partials 8497 8490 -7
π New features to boost your workflow:
|
jasnell
approved these changes
Jul 19, 2026
trivikr
added
request-ci
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
geeksilva97
approved these changes
Jul 21, 2026
Collaborator
geeksilva97
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Collaborator
Commit Queue failed- Loading data for nodejs/node/pull/63797 β Done loading data for nodejs/node/pull/63797 ----------------------------------- PR info ------------------------------------ Title sqlite: keep database alive while a session is open (#63797) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch 3zrv:fix/sqlite-session-crash -> nodejs:main Labels c++, author ready, needs-ci, commit-queue-squash, sqlite Commits 3 - sqlite: fix crash when a session outlives its database - fix: keep database alive while a session is open - Merge branch 'main' into fix/sqlite-session-crash Committers 1 - Mohamed Sayed <k@3zrv.com> PR-URL: https://github.com/nodejs/node/pull/63797 Fixes: https://github.com/nodejs/node/issues/63796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63797 Fixes: https://github.com/nodejs/node/issues/63796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> -------------------------------------------------------------------------------- βΉ This PR was created on Mon, 08 Jun 2026 16:23:46 GMT β Approvals: 2 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/63797#pullrequestreview-4731067200 β - Edy Silva (@geeksilva97): https://github.com/nodejs/node/pull/63797#pullrequestreview-4744533536 β GitHub cannot link the author of 'sqlite: fix crash when a session outlives its database' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β GitHub cannot link the author of 'fix: keep database alive while a session is open' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β GitHub cannot link the author of 'Merge branch 'main' into fix/sqlite-session-crash' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β Last GitHub CI successful βΉ Last Full PR CI on 2026-07-21T12:30:19Z: https://ci.nodejs.org/job/node-test-pull-request/74960/ - Querying data for job/node-test-pull-request/74960/ β Build data downloaded β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 63797 From https://github.com/nodejs/node * branch refs/pull/63797/merge -> FETCH_HEAD β Fetched commits as 69e982f70d31..f6b496025e59 -------------------------------------------------------------------------------- Auto-merging src/node_sqlite.cc Auto-merging test/parallel/test-sqlite-session.js [main ed54edc94a] sqlite: fix crash when a session outlives its database Author: Mohamed Sayed <k@3zrv.com> Date: Mon Jun 8 19:21:31 2026 +0300 2 files changed, 35 insertions(+), 3 deletions(-) Auto-merging src/node_sqlite.cc Auto-merging test/parallel/test-sqlite-session.js error: commit f6b496025e5974385c8adea13857b5fd7d810171 is a merge but no -m option was given. fatal: cherry-pick failed [main aca4f7c47a] fix: keep database alive while a session is open Signed-off-by: Mohamed Sayed <k@3zrv.com> Author: Mohamed Sayed <k@3zrv.com> Date: Mon Jun 8 20:26:15 2026 +0300 3 files changed, 31 insertions(+), 31 deletions(-) β Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/29837964050 |
Contributor
|
Could you rebase this, @3zrv ? |
Contributor
Author
|
should be good now, thank you @geeksilva97 |
geeksilva97
approved these changes
Jul 22, 2026
geeksilva97
added
request-ci
github-actions
Bot
removed
the
request-ci
Collaborator
Collaborator
geeksilva97
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Collaborator
Commit Queue failed- Loading data for nodejs/node/pull/63797 β Done loading data for nodejs/node/pull/63797 ----------------------------------- PR info ------------------------------------ Title sqlite: keep database alive while a session is open (#63797) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch 3zrv:fix/sqlite-session-crash -> nodejs:main Labels c++, author ready, needs-ci, commit-queue-squash, sqlite Commits 4 - sqlite: fix crash when a session outlives its database - fix: keep database alive while a session is open - Merge branch 'main' into fix/sqlite-session-crash - Merge branch 'main' into fix/sqlite-session-crash Committers 1 - Mohamed Sayed <k@3zrv.com> PR-URL: https://github.com/nodejs/node/pull/63797 Fixes: https://github.com/nodejs/node/issues/63796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63797 Fixes: https://github.com/nodejs/node/issues/63796 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> -------------------------------------------------------------------------------- βΉ This PR was created on Mon, 08 Jun 2026 16:23:46 GMT β Approvals: 2 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/63797#pullrequestreview-4731067200 β - Edy Silva (@geeksilva97): https://github.com/nodejs/node/pull/63797#pullrequestreview-4749964087 β GitHub cannot link the author of 'sqlite: fix crash when a session outlives its database' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β GitHub cannot link the author of 'fix: keep database alive while a session is open' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β GitHub cannot link the author of 'Merge branch 'main' into fix/sqlite-session-crash' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β GitHub cannot link the author of 'Merge branch 'main' into fix/sqlite-session-crash' to their GitHub account. β Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork β Last GitHub CI successful βΉ Last Full PR CI on 2026-07-22T03:17:01Z: https://ci.nodejs.org/job/node-test-pull-request/74990/ - Querying data for job/node-test-pull-request/74990/ β Build data downloaded β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 63797 From https://github.com/nodejs/node * branch refs/pull/63797/merge -> FETCH_HEAD β Fetched commits as 15d5b8f7fecc..c0e90048ea8a -------------------------------------------------------------------------------- Auto-merging src/node_sqlite.cc Auto-merging test/parallel/test-sqlite-session.js [main 9755c48553] sqlite: fix crash when a session outlives its database Author: Mohamed Sayed <k@3zrv.com> Date: Mon Jun 8 19:21:31 2026 +0300 2 files changed, 35 insertions(+), 3 deletions(-) Auto-merging src/node_sqlite.cc Auto-merging test/parallel/test-sqlite-session.js error: commit f6b496025e5974385c8adea13857b5fd7d810171 is a merge but no -m option was given. fatal: cherry-pick failed [main 05d775e278] fix: keep database alive while a session is open Signed-off-by: Mohamed Sayed <k@3zrv.com> Author: Mohamed Sayed <k@3zrv.com> Date: Mon Jun 8 20:26:15 2026 +0300 3 files changed, 31 insertions(+), 31 deletions(-) β Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/29926645472 |
Contributor
|
I suspect these merge commits are messing with the bot that merges the PR. Could you squash all your commits into a single one? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes: #63796
Verification