Release SQLite callbacks and expanded trace SQL safely by 1ndahous3 · Pull Request #9151 · RustPython/RustPython

1ndahous3 · 2026-10-15T11:33:25Z

This PR follows our AI policy

Summary

Release replaced and disabled trace, progress, and authorizer callbacks, and clear their registrations when a connection closes or is reinitialized.
Keep active callbacks alive through invocation and result conversion, releasing Python references after native and Rust locks. Preserve deferred rollback and reject registrations against an already closed native connection.
Dispose of old callbacks before publishing a replacement connection, including databases installed by reentrant finalizers.
Drain deferred cleanup after BLOB operations and native handle disposal. Keep each BLOB attached to its original database across connection reinitialization.
Free the SQL text allocated by SQLite for each trace callback, also addressed in the larger draft Prepare interpreter-local runtime state and thread lifecycle for GC isolation #8944.

Memory usage

Windows 11 x64

Workload	Before	After
Private-memory growth over 20,000 traced queries with a 1 KiB parameter, after warmup	21.8 MiB	64 KiB
Retained callback payload after 256 registrations and close, 64 KiB per callback	16 MiB	0

Known limitations

When __init__() is called from an active native callback, Python finalizers remain deferred until the enclosing native operation returns. This avoids running destructors under SQLite locks, but their timing can differ from CPython and they may observe the replacement connection.

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

Bug Fixes
- Improved SQLite callback reliability when callbacks are replaced, cleared, or the connection is closed.
- Fixed cleanup when callbacks unregister themselves or interact with the connection during release.
- Improved connection reinitialization and cleanup during overlapping operations, including restoring default row and text handling.
- Fixed cases where closing a BLOB could interfere with cursor activity, and improved BLOB operations during connection closure.
Tests
- Added regression coverage for callback lifetimes, concurrent cleanup, BLOB closure during cursor activity, and connection usability after reinitialization.

Assisted-by: Codex:gpt-6

chatgpt-codex-connector · 2026-10-15T11:33:35Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-17T21:46:27.375055Z	`d8b647e`	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-10-15T11:34:11Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Last reviewed commit: a9c7a56
Latest commit, not reviewed: d8b647e

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: f5da7eb0-7e6b-4113-8b10-f926621d9037

📥 Commits

Reviewing files that changed from the base of the PR and between 1bee2b2 and a9c7a56.

📒 Files selected for processing: 2

crates/stdlib/src/_sqlite3.rs
extra_tests/snippets/stdlib_sqlite.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

📝 Walkthrough

SQLite callback registrations now retain callback data through invocation and cleanup. Connection teardown and reinitialization coordinate rollback, callback removal, and deferred drops. BLOB handles retain their originating database and queue native cleanup. Regression tests cover callback lifetimes, reinitialization, and concurrent cleanup.

Changes: SQLite resource lifetimes

Layer / File(s)	Summary
Callback registration and invocation `crates/stdlib/src/_sqlite3.rs`	Authorizer, trace, and progress callbacks use retained references and a shared registration path. Callback entry points acquire strong references. The trace callback frees expanded SQL after use.
Connection cleanup and callback release `crates/stdlib/src/_sqlite3.rs`, `extra_tests/snippets/stdlib_sqlite.py`	Connection calls defer reference drops. Closing and reinitialization coordinate rollback and callback clearing with SQLite mutex access. Tests cover callback release, self-removal, and connection reinitialization.
BLOB ownership and deferred close `crates/stdlib/src/_sqlite3.rs`, `extra_tests/snippets/stdlib_sqlite.py`	BLOB handles retain their originating database for operations and error reporting. Dropping a BLOB queues its native handle for cleanup. Statement-lease cleanup retains the database during reset. Tests cover contended cleanup and closing a BLOB while cursor reset is blocked.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to a9c7a

No actionable issue remains identified for this change; it is ready to merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 13.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the main changes: safe release of SQLite callbacks and cleanup of expanded trace SQL resources.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches: 💡 1

🧪 Generate unit tests (beta)

Create a new PR

🛠️ Fix failing CI checks: 💡

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c999c6003e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

…operations Assisted-by: Codex:gpt-6

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cfdf827fcd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

youknowone · 2026-10-15T13:30:30Z

@ever0de could you please take a look?

Assisted-by: Codex:gpt-6

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36418c4a83

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

fanninpm · 2026-10-15T19:13:26Z

@codspeedbot fix benchmark harness in another PR

codspeed · 2026-10-15T19:15:50Z

@codspeedbot fix benchmark harness in another PR

I opened #9158 for the harness fix and left this PR untouched.

Cause of the failure here: this PR's code didn't cause it. The CodSpeed job was killed in Build the benchmark targets with The runner has received a shutdown signal. 18 of the last ~200 CodSpeed runs failed the same way, mostly on main pushes, including the two most recent main runs. Every one died while building the final rustpython targets on the 2-core, 16 GB runner. The likeliest cause is the runner running short of memory: each of these builds uses about 5 GB, but I couldn't confirm it from the CI logs.

What #9158 changes: cargo codspeed build was building 4 LTO-linked copies of the interpreter with full debug info. One of them was the library's unit-test harness, which no benchmark ever runs. Setting bench = false on [lib] and [[bin]] stops building it. The benchmarks that get measured and cargo test are unchanged.

Measured locally with cargo-codspeed 5.0.3 at -j 2 (the same as CI), with dependencies cached:

	before	after
build wall time	9m 57s	6m 58s (−30%)
LTO interpreter builds	4	3
peak system memory	~10.0 GB	~11.9 GB

The build is shorter, but its memory peak didn't drop, because the two bench binaries still compile and link at the same time. The peak difference in the table is sampling noise. #9158 should make the shutdowns less likely but may not stop them. If they keep happening after it merges, the next options are to stop those two links from overlapping or to move the job to a larger runner.

To get a CodSpeed result for this PR, re-run the failed Run benchmarks job; nothing in this PR needs to change.

If the failures come back after #9158 merges, comment @codspeedbot make the benchmark links run one at a time and I'll work on that.

Summary

Warming up the performance wizard...
Diagnose the failing CodSpeed job — The job is killed during "Build the benchmark targets" (18 such failures in the last ~200 runs, mostly on main). The build compiled 4 full LTO copies of the interpreter, one of them the library unit-test harness, which is never run.
Fix the benchmark build in a separate PR — Opened Skip unit-test harnesses when building benchmarks #9158: bench = false on lib/bin. Build time went from 9m57s to 6m58s locally. Peak memory did not change.
Report back — Report back
Handling comment — Comment handling finished successfully

_{Learnings updated. See what CodSpeed remembers about this repository.}

_{Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.}

Assisted-by: Codex:gpt-6

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bee2b2a75

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

codspeed · 2026-10-15T22:48:24Z

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks¹

_{Comparing 1ndahous3:sqlite_callback_lifetimes (1bee2b2) with main (88f5189)}

4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Assisted-by: Codex:gpt-6

youknowone

👍

Release SQLite callbacks and expanded trace SQL safely

c999c60

Assisted-by: Codex:gpt-6

chatgpt-codex-connector Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread extra_tests/snippets/stdlib_sqlite.py

Comment thread crates/stdlib/src/_sqlite3.rs

Comment thread crates/stdlib/src/_sqlite3.rs

Drain SQLite callback cleanup before reinitialization and after blob …

cfdf827

…operations Assisted-by: Codex:gpt-6

chatgpt-codex-connector Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread crates/stdlib/src/_sqlite3.rs

Comment thread crates/stdlib/src/_sqlite3.rs

Drain SQLite cleanup after statement reset

36418c4

Assisted-by: Codex:gpt-6

chatgpt-codex-connector Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread crates/stdlib/src/_sqlite3.rs Outdated

codspeed Bot mentioned this pull request Oct 15, 2026

Skip unit-test harnesses when building benchmarks #9158

Merged

Fix concurrent SQLite connection reinitialization

1bee2b2

Assisted-by: Codex:gpt-6

chatgpt-codex-connector Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread crates/stdlib/src/_sqlite3.rs

Comment thread crates/stdlib/src/_sqlite3.rs

1ndahous3 added 2 commits October 16, 2026 01:54

Restore SQLite factory defaults after callback cleanup

a9c7a56

Assisted-by: Codex:gpt-6

Merge main into SQLite callback lifetime fixes

d8b647e

Assisted-by: Codex:gpt-6

youknowone approved these changes Oct 18, 2026

View reviewed changes

youknowone merged commit cf3bcee into RustPython:main Oct 18, 2026
29 checks passed

1ndahous3 deleted the sqlite_callback_lifetimes branch October 18, 2026 09:01

Conversation

1ndahous3 commented Oct 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Memory usage

Windows 11 x64

Known limitations

AI assistance

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

youknowone commented Oct 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fanninpm commented Oct 15, 2026

Uh oh!

codspeed Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

codspeed Bot commented Oct 15, 2026

Merging this PR will not alter performance

Footnotes

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1ndahous3 commented Oct 15, 2026 •

edited by coderabbitai Bot

Loading

chatgpt-codex-connector Bot commented Oct 15, 2026 •

edited

Loading

coderabbitai Bot commented Oct 15, 2026 •

edited

Loading

codspeed Bot commented Oct 15, 2026 •

edited

Loading