perf(performance-monitor): move blocking sqlite3 I/O off the event loop

[groupthinking]

Canonical issue

Closes #1195

Outcome

PerformanceMonitor used the synchronous sqlite3 driver directly inside six async def methods. Every call ran connect → statement → commit() (which fsyncs) → close() on the event loop, so nothing else could be scheduled for the duration of the disk write.

This is on a live request path — router.py:1165 and :1183 await performance_monitor.record_metric(...), and record_metric calls _store_metric unconditionally (no sampling, no enable flag).

This change does	This change does not
Stop the event loop stalling during sqlite connect/commit/close	Make the queries themselves faster
Keep the API, SQL text and return values byte-identical	Change transaction boundaries or isolation
Let other coroutines run while a metric is being written	Make writes concurrent — SQLite still serialises writers
Preserve the existing swallow-and-log error handling	Add retries, pooling or WAL tuning

The honest claim is "stops stalling the event loop", not "faster". Under concurrent load that is the difference that matters: a request that records a metric no longer freezes every other in-flight request while the disk write completes.

Scope

Six sites, all in performance_monitor.py, all the same defect and the same fix:

_store_metric · _store_alert · _basic_cleanup · get_current_performance_summary · _get_recent_metrics_summary · _store_benchmark_result

Each body moved verbatim into a nested sync function dispatched with await asyncio.to_thread(...). Because the original bodies were already indented inside try:, they move into the nested def without re-indentation — so the diff is genuinely structural, not a rewrite.

Design notes

Why not aiosqlite? It is not a dependency of this project, and adding a driver is a much larger change than this defect warrants. asyncio.to_thread uses the default executor and needs no new dependency.

Why nested functions rather than methods? They close over self and the local variables (cutoff_date, metric, …) that the original code already computed, so no signature or state has to be threaded through.

No use-after-close hazard. Converting a synchronous call to to_thread can introduce a use-after-close race when a caller tears the resource down in a finally — that exact bug was found in #1190. It does not apply here: every connection is opened and closed inside the same call, so there is no shared handle for a caller to close underneath an in-flight statement. PerformanceMonitor has no close(); stop_monitoring() only clears a task flag.

_init_database deliberately untouched. It is a synchronous method called from __init__ — it never runs on the event loop, so wrapping it would add noise and no benefit.

Risk

Low.

• No SQL, schema, transaction boundary or return type changed.
• All 113 existing tests pass with zero test edits — the observable contract is unchanged.
• sqlite3 connections are thread-affine (check_same_thread=True by default). That constraint is satisfied, not violated: each connection is created, used and closed entirely within a single to_thread call, so it never crosses threads.
• Failure mode if the executor is saturated is a delay, not an error; the existing except Exception handlers are unchanged.

Verification

All results below are at head b38b9fdb3.

119 passed

• 113 pre-existing tests pass with zero test edits
• +6 new in TestSqliteDoesNotBlockEventLoop: 3 loop-responsiveness tests and 3 preserved-behaviour guards

Non-vacuity, by behavioural mutation. Deleting the code would raise AttributeErrors, which proves nothing. Instead all six to_thread dispatches were replaced with direct synchronous calls, keeping everything else identical:

MUTATED 6 dispatch sites -> direct sync calls

FAILED TestSqliteDoesNotBlockEventLoop::test_store_metric_does_not_block_event_loop
FAILED TestSqliteDoesNotBlockEventLoop::test_record_metric_does_not_block_event_loop
FAILED TestSqliteDoesNotBlockEventLoop::test_read_path_does_not_block_event_loop
3 failed, 116 passed

Exactly the 3 responsiveness tests fail and nothing else. A uniform "everything failed" would have meant the proof was structural rather than behavioural.

The test measures whether an independent heartbeat coroutine keeps getting scheduled while sqlite3.connect is artificially slowed:

beat = asyncio.create_task(heartbeat())
await asyncio.sleep(0)          # let the heartbeat reach its first await first
result = await coro
assert ticks > 0

Blocking → 0 ticks. Off-loop → > 0.

Honest note: the 3 guards (test_store_metric_still_writes_the_row, test_summary_reflects_stored_metrics, test_store_metric_swallows_database_errors) pass under both old and new code by design. They exist to pin behaviour the change must not regress, not to prove the change. Only the 3 heartbeat tests discriminate.

ruff: All checks passed! on both touched files, exact parity with main.

Production evidence

youtube_extension.backend.services.performance_monitor is in the transitive import closure of the production entrypoint youtube_extension.main:app (root Dockerfile:93).

router.py:69    from ...services.performance_monitor import PerformanceMonitor
router.py:1165  await performance_monitor.record_metric(...)
router.py:1183  await performance_monitor.record_metric(...)
     -> record_metric        (performance_monitor.py:231)
     -> await self._store_metric(metric)   (:261, unconditional)
     -> sqlite3.connect + INSERT + commit  (:274)

Other live callers: memory_manager.py:472/475, load_balancer.py:381, database_optimizer.py:49.

Agent handoff

Reviewers: the interesting questions are in Design notes and the Verification mutation output. Specific challenges are posted as a separate comment.

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8394cb0e-b61a-4815-bf8f-22a2451f3361

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Canceled	v0	Aug 1, 2026 10:30pm

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA b38b9fd.

Ensure 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 Files

None

[groupthinking]

@coderabbitai review

Please be adversarial. Five specific challenges I want pressure-tested:

1. Thread affinity. sqlite3 connections default to check_same_thread=True. I claim this change satisfies that constraint rather than violating it, because each connection is created, used and closed entirely inside one asyncio.to_thread call and never crosses a thread boundary. Is there any path — an exception mid-method, a generator, a closure capturing conn — where a connection could outlive its to_thread call?

2. Write serialisation. I deliberately did not claim this makes writes concurrent; SQLite serialises writers via a database-level lock. But moving six call sites off-loop means multiple executor threads can now contend on that lock simultaneously where previously the event loop serialised them for free. Could that turn a fast implicit queue into SQLITE_BUSY errors under load? The default timeout is 5s — is relying on it acceptable, or should this set it explicitly?

3. Use-after-close. perf(cloud-tasks): move blocking gRPC calls off the event loop #1190 taught me that to_thread is not cancellation-safe when a caller closes the underlying resource in a finally. I argue it does not apply here because connections are per-call and PerformanceMonitor has no close(). Please verify — particularly whether stop_monitoring() or any teardown path could delete the database file while a thread is mid-write.

4. Test honesty. I have labelled 3 of my 6 new tests as preserved-behaviour guards that pass under both old and new code. Is the 3-test discriminating set actually sufficient? It covers _store_metric, record_metric and the read path — but not _store_alert, _basic_cleanup, _get_recent_metrics_summary or _store_benchmark_result. That is a real coverage gap and I would rather you name it than let it slide.

5. Scope. Six sites in one file, one defect class, one fix. Is that coherent scope or should it be split? I would push back on splitting — a partial fix leaves the loop blocked on the remaining paths — but I want the objection on the record if you have one.

Context: this is the 8th in a series of event-loop-blocking fixes. #1190 in the same series introduced a real cancellation regression that a reviewer caught, so I would rather over-scrutinise this one.

[github-actions]

Agent Completion Truth Gate: BLOCKED

Reasons: invalid_payload

Machine-readable verdict

{
  "details": {
    "invalid_fields": [
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

[Copilot]

🟢 Ready to approve

All scoped SQLite operations are safely offloaded with focused regression coverage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Moves synchronous SQLite work off the event loop while preserving existing behavior.

Changes:

• Dispatches six database operations through asyncio.to_thread.
• Adds responsiveness and behavior-regression tests.

File summaries

File	Description
performance_monitor.py	Offloads SQLite reads, writes, and cleanup.
test_performance_monitor.py	Verifies loop responsiveness and persistence behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[groupthinking]

Adversarial review (automated, review-requested trigger)

Reviewed the full diff at head b38b9fdb3 against the five challenges. Independent verdict below — no rubber-stamp.

Code correctness: no blocking defect. All six sites use the identical, correct transformation: the original body moves verbatim into a nested _write/_query/_purge, dispatched with await asyncio.to_thread(...) inside the pre-existing try. Each connection is created, used, and closed entirely inside one to_thread call; none of the moved bodies mutate in-memory self state (they touch only the DB), and record_metric's in-memory append stays on-loop. So there is no data race and no handle that survives its thread. The logger.info(...) relocation in _basic_cleanup preserves semantics.

Answering the five challenges:

1. Thread affinity — satisfied, not violated. No path lets a conn outlive its to_thread call: it is a pure local, not returned, not yielded, not captured by any escaping closure. An exception mid-_write leaks the connection (no finally), but that was true before this PR and is not a regression.
2. Write serialisation — the one real behavioural change. Previously the event loop serialised these writes for free; now multiple executor threads can contend on SQLite's database-level write lock. Under sustained concurrency a writer can exhaust the default 5 s busy timeout and raise OperationalError: database is locked, which the existing except swallows — i.e. a dropped, logged metric rather than a stall. Acceptable for a best-effort metrics path, but I'd note it explicitly and consider a short explicit timeout=/retry if this path ever becomes load-bearing.
3. Use-after-close — does not apply. stop_monitoring() only clears a task flag; there is no close() and no teardown that deletes the DB file mid-write. Connections are per-call, so the perf(cloud-tasks): move blocking gRPC calls off the event loop #1190 cancellation regression cannot recur here.
4. Test honesty — gap confirmed, low risk. The three discriminating tests cover _store_metric, record_metric, and the read path but not _store_alert, _basic_cleanup, _get_recent_metrics_summary, or _store_benchmark_result. Since all six are the same mechanical transform and I verified each in the diff, residual risk is low — but the gap is real and worth a one-line acknowledgement in the test module.
5. Scope — coherent. One defect class, one file, six sites. Splitting would leave the loop blocked on the remaining paths; keep it together.

The actual merge blocker is not this diff. The failing/blocking checks are (a) agent-completion/truth-gate/pr-1196 → BLOCKED: invalid_payload on policy.agent_login and policy.run_id, which is a workflow-tooling bug (the same gate machinery that sibling PRs #1151 and #1154 are reworking), and (b) a Vercel deploy that was manually canceled from the dashboard, not a test failure. There is nothing in performance_monitor.py to fix for those to clear.

Not merging: base is protected main, this carries no automerge label, and the truth-gate is red — publish remains a human decision.

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- PR diff summary ---'
git diff --stat origin/main...HEAD || true
git diff --unified=50 origin/main...HEAD -- '*performance_monitor.py' '*test*performance*' || true

echo '--- candidate files ---'
fd -t f -e py | rg 'performance_monitor|router|memory_manager|load_balancer|database_optimizer|test'

echo '--- implementation outline ---'
pm_file="$(fd -t f -e py 'performance_monitor.py' | head -n 1)"
if [ -n "${pm_file:-}" ]; then
  ast-grep outline "$pm_file" --items all
  echo "--- $pm_file: sqlite/lifecycle/dispatch references ---"
  rg -n -C 8 'sqlite3\.connect|to_thread|def (_store_metric|_store_alert|_basic_cleanup|get_current_performance_summary|_get_recent_metrics_summary|_store_benchmark_result|stop_monitoring)|\b(remove|unlink|rmtree)\b|database.*path|db_path' "$pm_file"
fi

echo '--- project-wide teardown and database deletion references ---'
rg -n -i -C 5 '(stop_monitoring\s*\(|performance_monitor|sqlite.*(remove|unlink)|\b(remove|unlink|rmtree)\s*\(|os\.remove|Path\(.*\)\.unlink)' --glob '*.py' .

Length of output: 41757

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

pm='src/youtube_extension/backend/services/performance_monitor.py'
tests='tests/unit/test_performance_monitor.py'

echo '--- PerformanceMonitor construction and lifecycle ---'
sed -n '1,250p' "$pm"

echo '--- all SQLite dispatch methods, with complete bodies ---'
sed -n '250,370p' "$pm"
sed -n '480,610p' "$pm"
sed -n '760,840p' "$pm"

echo '--- PerformanceMonitor teardown and database-path references ---'
rg -n -C 12 'def stop_monitoring|stop_monitoring\(|self\.db_path|db_path\s*=|os\.(remove|unlink)|Path\(.*\)\.(unlink|rm)|shutil\.rmtree|TemporaryDirectory' \
  "$pm" tests src --glob '*.py'

echo '--- direct callers and their surrounding lifecycle context ---'
rg -n -C 10 'record_metric\s*\(|_cleanup_old_metrics\s*\(|get_current_performance_summary\s*\(|get_performance_dashboard\s*\(' \
  src/youtube_extension/backend/api/v1/router.py \
  src/youtube_extension/backend/services/memory_manager.py \
  src/youtube_extension/backend/services/load_balancer.py \
  src/youtube_extension/backend/services/database_optimizer.py 2>/dev/null || true

echo '--- full new test class ---'
sed -n '1030,1180p' "$tests"

Length of output: 137893

---

@groupthinking I found two items that need action.

1. Close every connection in the worker on failure.
   Each nested closure closes conn only on the success path. For example, _store_metric() creates conn on Line 275, but an exception from cursor(), execute(), or commit() skips conn.close().

   This behavior existed before the PR. The to_thread() conversion makes it more important to fix. The exception traceback can retain conn after the worker returns. Later finalization is not a safe connection-lifecycle policy for a thread-affine resource.

   Add try/finally inside each nested _write, _purge, and _query function. Call conn.close() from finally in that same worker. Add a test that injects a failure after connect() and verifies that close() runs.

2. Extend the discriminating responsiveness tests to all six offloaded methods.
   The current three-test set proves the write path through _store_metric() and record_metric(), plus get_current_performance_summary(). It does not prove that the other four conversions remain off-loop.

   Add heartbeat tests for _store_alert(), _basic_cleanup(), _get_recent_metrics_summary(), and _store_benchmark_result(). The existing slow-sqlite3.connect mechanism is appropriate. This is a real coverage gap because a future direct call at any of these sites would pass the preserved-behavior guards.

My assessment of the five challenges:

1. Thread affinity: Correct on the normal path. No closure returns a connection. No generator captures one. The connection is created, used, and normally closed by one executor worker. The missing finally prevents this claim from being complete on error paths.

2. Write serialization: Moving writes off-loop permits simultaneous writer attempts. SQLite serializes those writes. The default sqlite3.connect() timeout is five seconds, so short contention waits will not immediately become SQLITE_BUSY. Under sustained contention, a write can still time out and the existing handlers will log and drop that metric or alert. Setting timeout=5.0 explicitly does not improve behavior. Set an explicit non-default timeout only if the project defines an acceptable metric-loss latency and failure policy. That policy is outside this focused event-loop fix.

3. Use-after-close and teardown: I found no PerformanceMonitor close path and no database-file deletion path. stop_monitoring() only cancels the background task. Cancellation can detach the awaiting task while the worker continues, but no caller owns or closes that worker connection. Therefore, the #1190 resource-race pattern does not apply. The missing worker-local finally remains the exception-path concern.

4. Test honesty: The labels are accurate. The three behavior guards do not discriminate by design. The three heartbeat tests discriminate, but they leave four converted methods unproven. Add the four tests above.

5. Scope: Keep the six sites together. They are one defect class in one module. Splitting the change would knowingly retain event-loop blocking paths.

• Ensure that every worker-local SQLite connection closes in the same executor thread, including failure paths.
• Add discriminating heartbeat tests for the four currently untested asyncio.to_thread() dispatches.

---

Failed to handle agent chat message. Please try again.

_{You are interacting with an AI system.}

[linear-code]

GRV-228