perf: offload blocking SQLite I/O off the event loop - #1327
Conversation
sqlite3.connect(), cursor.execute()/fetchall() and connection.close() all ran directly on the event loop. That froze the loop for the entire duration of every query and silently defeated execute_batch_queries' semaphore + gather concurrency, since nothing underneath ever yielded. Measured on a 400k-row table with 16 batched queries: the batch took 230.0ms against a computed serial floor of 230.5ms, and a 5ms heartbeat task got zero ticks. Offloaded, the same batch takes 64.9ms (3.55x) and the heartbeat reaches 83% of its ideal tick count. check_same_thread=False is required rather than incidental: the connection is now created on a worker thread but is still used from the loop thread by initialize_database_optimization(). The asyncpg path is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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. Comment |
|
@linear-code @coderabbitai review This offloads four blocking SQLite calls off the event loop. The headline is not the 3.55x — it is that Three specific things I would like a second opinion on:
Prove-fail is recorded in Note the |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Diff ==='
git diff --unified=80 HEAD^ HEAD -- '*.py'
echo '=== Candidate implementation and test files ==='
fd -t f -e py . | rg '(database.*optimiz|optimiz.*database|performance_monitor|benchmark)'
echo '=== Symbols relevant to SQLite lifecycle and batching ==='
rg -n -C 5 --glob '*.py' \
'class DatabaseConnectionPool|class QueryOptimizer|async def initialize|async def get_connection|async def release_connection|async def execute_query|async def execute_batch_queries|def initialize_database_optimization|sqlite3\.connect|check_same_thread|asyncio\.to_thread' .Length of output: 192948 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Query execution, batching, and SQLite initialization ==='
sed -n '351,570p' src/youtube_extension/backend/services/database_optimizer.py
sed -n '920,990p' src/youtube_extension/backend/services/database_optimizer.py
echo '=== Existing cancellation-safe worker patterns ==='
sed -n '45,85p' src/youtube_extension/services/cloud/cloud_tasks_queue.py
sed -n '50,90p' src/youtube_extension/backend/code_generator.py
echo '=== Relevant test coverage and direct pool consumers ==='
rg -n -C 4 --glob '*.py' \
'execute_batch_queries\(|get_connection\(|release_connection\(|CancelledError|cancel\(' \
tests/unit/test_database_optimizer.py src/youtube_extension/backend/services/database_optimizer.pyLength of output: 24695
This creates concurrent access to one SQLite connection. The active worker can fail with a closed-connection error. It also invalidates the safety claim for Make the SQLite worker cancellation-safe. Keep a task for the Add a regression test that starts a delayed SQLite query, cancels it after the worker starts, and asserts that For the three design questions:
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Offloads blocking SQLite operations from the event loop, enabling existing batch concurrency and addressing issue #1325.
Changes:
- Moves SQLite setup, connection, query, and close operations to worker threads.
- Adds regression tests for responsiveness, thread usage, and batch overlap.
- Enables cross-thread SQLite access with single-owner semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
database_optimizer.py |
Offloads blocking SQLite operations. |
test_database_optimizer.py |
Adds SQLite offloading and concurrency tests. |
Suppressed comments (1)
tests/unit/test_database_optimizer.py:1552
- As above,
execute_batch_queriesexpects(query, params)tuples; iterating this two-key dict supplies its key names rather thanSELECT {i}andFalse. Build tuples so this regression test runs the queries it claims to run.
queries = [{"query": f"SELECT {i}", "use_cache": False} for i in range(4)]
| cursor.execute(query) | ||
| return cursor.fetchall() | ||
|
|
||
| result = await asyncio.to_thread(_run_sync_query) |
| pool.release_connection = AsyncMock() | ||
| optimizer = QueryOptimizer(pool) | ||
|
|
||
| queries = [{"query": f"SELECT {i}", "use_cache": False} for i in range(count)] |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
Independent review (triggered by the review request) — both open findings verified against 1. Cancellation can close a SQLite connection under an in-flight query — real correctness bug. 2. Batch tests don't exercise the documented I did not push a fix: this PR targets protected Generated by Claude Code |
Independent verification — ⛔ do not merge yet (one real blocker)Second opinion as requested. I traced the diff against Blocker — cancellation can close a SQLite connection mid-query. When a sibling query fails, the Also real (non-blocking, but fix before merge): the two new Answering the three questions:
CI: the Verdict: blocked on the cancellation-safety fix; mergeable once that lands (and the two batch tests use real tuples). Leaving the two review threads unresolved since they're accurate. Generated by Claude Code |
Review — verified locally, change is soundChecked out Verification reproduced
Answers to your reviewer questions
Not merging from here. Protected Generated by Claude Code |
Cancelling a query in execute_batch_queries unwound the await but left the worker thread running cursor.execute(). CancelledError is a BaseException, so `except Exception` did not catch it and `finally` released — and therefore closed — the connection on a second thread while the first was still using it. check_same_thread=False disables sqlite3's check, not the single-owner requirement. Run the offload as a shielded task and drain it in `finally` before release. Also fixes two batch tests that passed dicts where the API takes (query, params) tuples, so they never exercised the real contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Both findings reproduced and fixed in 1. Cancellation could close a connection mid-query — confirmed. I verified the chain independently rather than taking it on trust:
So a cancelled task scheduled Fix: run the offload as an explicit task, if sync_worker is not None:
while not sync_worker.done():
with contextlib.suppress(BaseException):
await asyncio.shield(sync_worker)
if connection:
await self.connection_pool.release_connection(connection)The Prove-fail, measured both ways:
New test 2. Dict-vs-tuple in the batch tests — confirmed, and worse than reported.
Worth flagging: 3. Took your stronger suggestion on the timing assertion. Added a max-concurrent- Real connection pooling stays a follow-up per your agreement; noted in Verification: @linear-code @coderabbitai re-review when you get a chance. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR diff summary ==='
git diff --stat HEAD^ HEAD
git diff --unified=35 HEAD^ HEAD -- \
src/youtube_extension/backend/services/database_optimizer.py \
tests/unit/test_database_optimizer.py
echo '=== Source structure ==='
ast-grep outline src/youtube_extension/backend/services/database_optimizer.py \
--items all --type function,class --match 'DatabaseConnectionPool|QueryOptimizer|execute_query|execute_batch_queries|release_connection|get_connection'
echo '=== Relevant implementation ==='
rg -n -C 12 --glob '*.py' \
'async def execute_query|async def execute_batch_queries|async def release_connection|asyncio\.shield|sync_worker|asyncio\.to_thread|contextlib\.suppress' \
src/youtube_extension/backend/services/database_optimizer.py
echo '=== New and modified optimizer tests ==='
rg -n -C 10 --glob '*.py' \
'TestBatchCancellationDoesNotCloseConnectionMidQuery|TestBatchQueriesActuallyOverlap|max_inflight|inflight|execute_batch_queries\(' \
tests/unit/test_database_optimizer.pyLength of output: 32812 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate optimizer test file ==='
fd -t f -e py 'test_database_optimizer' .
echo '=== Batch implementation ==='
sed -n '476,555p' src/youtube_extension/backend/services/database_optimizer.py
echo '=== Relevant test region at actual paths ==='
fd -t f -e py 'test_database_optimizer' . -x sh -c '
for file do
echo "----- $file -----"
rg -n -C 14 \
"class TestBatchQueriesActuallyOverlap|class TestBatchCancellationDoesNotCloseConnectionMidQuery|def test_batch_queries_are_not_serialised|def test_batch_queries_use_distinct_threads|def test_close_never_overlaps_an_in_flight_query|class _RecordingCursor|class _RecordingConnection" \
"$file"
done
' sh {} +Length of output: 4519 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Test file stored in HEAD ==='
git show HEAD:tests/unit/test_database_optimizer.py | \
nl -ba | sed -n '1470,1655p'
echo '=== Synchronization primitives in committed test file ==='
git show HEAD:tests/unit/test_database_optimizer.py | \
rg -n -C 4 \
'Event|Barrier|Condition|started|start|inflight|close_overlapped_query|TestBatchCancellationDoesNotCloseConnectionMidQuery'
echo '=== Commit identity and changed paths ==='
git show --no-patch --format='%H%n%s' HEAD
git diff-tree --no-commit-id --name-status -r HEADLength of output: 943 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Test file stored in HEAD ==='
git show HEAD:tests/unit/test_database_optimizer.py | \
sed -n '1470,1655p'
echo '=== Synchronization primitives in committed test file ==='
git show HEAD:tests/unit/test_database_optimizer.py | \
rg -n -C 4 \
'threading\.(Event|Barrier|Condition)|asyncio\.(Event|Barrier|Condition)|started|start|inflight|close_overlapped_query|TestBatchCancellationDoesNotCloseConnectionMidQuery'
echo '=== Commit identity and changed paths ==='
git show --no-patch --format='%H%n%s' HEAD
git diff-tree --no-commit-id --name-status -r HEADLength of output: 13223
One non-blocking test concern remains. Add a
This makes the regression test deterministic and preserves its purpose. Also consider removing the remaining 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Canonical issue
Closes #1325
Outcome
The SQLite branch of
DatabaseConnectionPool/QueryOptimizerran every blockingcall directly on the event loop. Four calls are now offloaded with
asyncio.to_thread,matching the idiom already used six times in
performance_monitor.py:initializeos.makedirs(...)get_connectionsqlite3.connect(...)execute_querycursor.execute()+cursor.fetchall()release_connectionconnection.close()The most valuable part of this change is not the raw speedup — it is that
execute_batch_queriesalready ships a semaphore,asyncio.gatherand fail-fastcancellation, and its docstring already claims queries "run concurrently". Because
nothing underneath ever yielded, that concurrency was delivering zero benefit.
This PR does not add concurrency; it activates concurrency that was already merged.
Production evidence
SQLite is the production default, not a test-only fallback —
database_url = os.getenv("DATABASE_URL", "sqlite:////tmp/uvai_data/app.db"), with anexplicit Cloud Run comment.
main.py:506callsinitialize_database_optimization()atstartup, so this is the live path.
Measured on a file-backed 400k-row table, 16 batched queries, mirroring
execute_batch_queries' own semaphore + gather structure:Two numbers matter more than the speedup:
concurrency machinery was inert.
whole batch. Nothing else in the process — health checks, other requests — could
make progress during database work.
Risk
Low, and contained to the SQLite branch.
asyncpgpath is untouched;pool.acquire()/pool.release()/connection.fetch()are already awaited natively.
check_same_thread=Falseis required, not incidental. Oncesqlite3.connectmovesto a worker thread, the returned connection would raise
ProgrammingErroron its firstuse from the loop thread — which
initialize_database_optimization()does directly atstartup. This is covered by a dedicated regression test.
get_connection()andrelease_connection(), so it is never touched by two threadsconcurrently.
asyncioandsqlite3were already imported.Verification
324 passed— 169 intest_database_optimizer.py(161 pre-existing + 8 new) and 155 inthe dependent
test_comprehensive_benchmarking.py.ruff checkclean on both changed files.Prove-fail against pre-change source (stashed the fix, kept the tests):
with diagnostics that reproduce the production finding in miniature:
The 1 pre-existing pass is
test_connection_is_usable_from_the_event_loop_thread, whichis a forward-looking guard: it passes before and after, and exists specifically to fail
if someone later adds the
to_threadoffload withoutcheck_same_thread=False.The tests assert on thread identity and a max-concurrent-
executecounter heldunder a
threading.Lock, not wall-clock ratios, so they stay deterministic on loaded CIhosts — following the review feedback on #1319 about host-dependent timing.
Cancellation safety (added in
7c4dc0931after review)Review caught a defect this PR introduced: cancelling a query unwound the
awaitbut leftthe worker thread inside
cursor.execute().CancelledErroris aBaseException, soexcept Exceptiondid not catch it andfinallyclosed the connection on a secondthread while the first was still using it.
check_same_thread=Falsedisables sqlite3'scheck, not the single-owner requirement. This was unreachable before this PR because
nothing yielded — activating the concurrency is what exposed it.
Fixed by running the offload as a shielded task and draining it in
finallybeforerelease.
TestBatchCancellationDoesNotCloseConnectionMidQueryproves it, per connection:Batch failed (0.59ms)— abandons the running thread,close()overlaps → failsBatch failed (202.26ms)— drains the 200 ms worker first → passesThe same commit fixes two batch tests that passed dicts where the API takes
(query, params)tuples; unpacking a 2-key dict yields its keys, so they had beenexecuting the literal string
"query"— green but vacuous.Scope
Deliberately excluded, to keep this reviewable:
get_connectionstill opens a fresh connection per call andrelease_connectioncloses it —DatabaseConnectionPooldoes not currently pool anythingon the SQLite path. Verified: an in-memory database loses its data across a
release/acquire cycle. That is a larger change, and the offload alone delivers the
measured 3.55x.
get_connection.avg = (avg * len(history) + t) / (len(history) + 1)where
connection_historyis adeque(maxlen=1000). Once saturated,len()pins at 1000forever, so this is neither a cumulative mean nor a well-formed EMA. Not verified as
user-visible, so left alone.
initialize_database_optimization's own synchronouscur.executecalls. Startup-only,before the server accepts traffic.
Agent handoff
mainand unrelated to this change.testandGenerate and Upload Coverageboth fail ontest_gh_aw_workflow_governance.py::test_ci_investigator_requires_dedicated_codex_credential—
07b8a2ec2deleted.github/workflows/eventrelay-ci-investigator.mdbut left the testfrom
c2c23f4fb. PRs fix(ci): drop stale eventrelay-ci-investigator governance checks #1317 and fix(ci): finish removal of eventrelay-ci-investigator workflow #1320 already own that fix; please do not open acompeting one. Expected result here is
1 failed, ~7947 passed.asyncio.to_threaduses the default executor,so this change adds work to the shared thread pool that Blocking I/O offloads share the default executor, so one stalled read can starve the process #1234 describes as a starvation
risk. Executor isolation is PR perf: isolate blocking file I/O from the shared default executor (#1234) #1241's territory and is intentionally not attempted here.
Questions for reviewers
check_same_thread=Falseacceptable given the single-owner invariant betweenget_connection()andrelease_connection(), or would you prefer the connection beconfined to the worker thread instead?
follow-up issue, or folded in here?
be meaningful but loose enough for CI, or should they be thread-identity only?