Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 9 additions & 23 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,33 +39,19 @@ jobs:

steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4

# Initializes the CodeQL tools for scanning.
- name: Initialize CodeQL
uses: github/codeql-action/init@v1
uses: github/codeql-action/init@v4
env:
CODEQL_ACTION_FILE_COVERAGE_ON_PRS: true
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
with:
languages: ${{ matrix.language }}
# If you wish to specify custom queries, you can do so here or in a config file.
# By default, queries listed here will override any specified in a config file.
# Prefix the list here with "+" to use these queries and those in the config file.
# queries: ./path/to/local/query, your-org/your-repo/queries@main

# Autobuild attempts to build any compiled languages (C/C++, C#, or Java).
# If this step fails, then you should remove it and run the build manually (see below)
- name: Autobuild
uses: github/codeql-action/autobuild@v1

# ℹ️ Command-line programs to run using the OS shell.
# πŸ“š https://git.io/JvXDl

# ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
# and modify them (or add more) to build your code if your project
# uses a compiled language

#- run: |
# make bootstrap
# make release
build-mode: none

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v1
uses: github/codeql-action/analyze@v4
with:
category: "/language:${{ matrix.language }}"
3 changes: 3 additions & 0 deletions .jules/bolt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
## 2025-03-20 - Database Connection Pooling
**Learning:** `streamrip/db.py` creates a new `sqlite3.connect` for *every single database operation* (`contains`, `add`, `all`). In bulk operations like resolving a playlist with hundreds of tracks, this introduces significant overhead.
**Action:** Use a single persistent connection per database instance instead of creating a new one on every method call.
Comment on lines +1 to +3
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟑 Minor

Fix the note's date and terminology.

This PR was opened on March 19, 2026, but the note is dated March 20, 2025, and the implementation caches one persistent connection rather than introducing a pool. Please rename/date this entry so the documentation matches the change.

πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.jules/bolt.md around lines 1 - 3, Update the journal entry header and
wording to reflect the actual change: change the date from "2025-03-20" to
"2026-03-19" and replace the term "Database Connection Pooling" with something
like "Database Connection Caching" (or "Persistent DB Connection") and update
the action text to state that streamrip/db.py now caches a single persistent
sqlite3 connection per database instance (affecting methods contains, add, all)
rather than introducing a connection pool; keep the learning line consistent
with the implementation (no new pooling semantics).

71 changes: 46 additions & 25 deletions streamrip/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,39 @@ def __init__(self, path: str):

self.path = path

if not os.path.exists(self.path):
# sqlite3.connect creates the file if it does not exist, so we check
# existence beforehand to know if we need to call create() later
exists = os.path.exists(self.path)

# ⚑ Bolt: Cache persistent SQLite connection to avoid recreating it
# on every db check/add. This gives ~10x speedup for database operations
# like downloading a playlist where it does hundreds of ID checks sequentially.
self.conn = sqlite3.connect(self.path, check_same_thread=False)
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_same_thread=False disables sqlite3's thread-safety guard. In this repo, DB usage appears to be synchronous and called from the main asyncio thread (no asyncio.to_thread/executor usage around database access), so keeping the default check_same_thread=True would preserve safety. If cross-thread access is intended, it should be paired with a lock (or per-thread connections) to prevent concurrent use of the same connection.

Suggested change
self.conn = sqlite3.connect(self.path, check_same_thread=False)
self.conn = sqlite3.connect(self.path)

Copilot uses AI. Check for mistakes.

if not exists or not self._table_exists():
Comment on lines +75 to +80
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd streamrip && find . -name "db.py" -o -name "main.py" -o -name "playlist.py" -o -name "track.py" | head -20

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 175


🏁 Script executed:

wc -l streamrip/db.py streamrip/rip/main.py streamrip/media/playlist.py streamrip/media/track.py 2>/dev/null | head -20

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 200


🏁 Script executed:

sed -n '1,100p' streamrip/db.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 3287


🏁 Script executed:

sed -n '70,85p' streamrip/db.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 898


🏁 Script executed:

sed -n '50,75p' streamrip/rip/main.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 1053


🏁 Script executed:

# Check how Database is imported and used in playlist.py
head -50 streamrip/media/playlist.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 1610


🏁 Script executed:

# Check the async resolve method in playlist.py
sed -n '30,65p' streamrip/media/playlist.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 1512


🏁 Script executed:

# Check track.py async methods
sed -n '110,220p' streamrip/media/track.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 4480


🏁 Script executed:

# Check the Database class implementation and its methods
sed -n '100,220p' streamrip/db.py | cat -n

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 4424


🏁 Script executed:

# Search for the 'downloaded' method definition
rg "def downloaded" streamrip/db.py -A 3

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 217


🏁 Script executed:

# Search for the 'set_failed' method definition
rg "def set_failed" streamrip/db.py -A 3

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 188


🏁 Script executed:

# Check the contains method which is likely called by downloaded
rg "def contains" streamrip/db.py -A 5

Repository: davidjuarezdev/streamrip_RipDL

Length of output: 477


Move database operations behind an async/await wrapper to prevent blocking the event loop.

Lines 75-80 create a synchronous SQLite connection that is used from async contexts. The check_same_thread=False parameter only disables SQLite's thread guardβ€”it does not make the connection async-safe or prevent blocking I/O. The Database.downloaded() and Database.set_failed() methods call DatabaseBase.contains() and DatabaseBase.add() synchronously (lines 108-109, 117-118), which execute blocking self.conn.execute() calls. These methods are invoked from async def resolve() in streamrip/media/playlist.py:45-50, streamrip/media/track.py:15-38, and streamrip/media/track.py:78-102, causing the event loop to stall on I/O operations. Wrap database access in an async interface or use a dedicated worker thread/executor to comply with the coding guideline: "Use async/await for asynchronous operations instead of blocking I/O".

πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@streamrip/db.py` around lines 75 - 80, The code opens a synchronous SQLite
connection (sqlite3.connect(...)) and uses it directly in async contexts causing
blocking I/O; update the Database API (methods like Database.downloaded(),
Database.set_failed()) to perform DB calls via an async wrapper or executor and
move synchronous calls in DatabaseBase.contains() and DatabaseBase.add() off the
event loop (e.g., run the self.conn.execute()/commit() operations in
asyncio.get_running_loop().run_in_executor or a dedicated DB worker thread);
ensure the Database constructor no longer exposes direct synchronous usage and
that callers in streamrip/media/playlist.py and streamrip/media/track.py await
the new async methods so all DB access is non-blocking.

self.create()

def __del__(self):
"""Ensure connection is closed on exit."""
if hasattr(self, 'conn') and self.conn:
self.conn.close()

Comment on lines +83 to +87
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on __del__ for closing the SQLite connection is unreliable (not guaranteed to run promptly, may be skipped on interpreter shutdown, and exceptions in __del__ are ignored). Since this PR introduces long-lived connections, consider adding an explicit close() API (and/or __enter__/__exit__) and calling it from the owning lifecycle (e.g., CLI/main shutdown), possibly with an atexit fallback.

Copilot uses AI. Check for mistakes.
def _table_exists(self) -> bool:
command = f"SELECT count(name) FROM sqlite_master WHERE type='table' AND name='{self.name}'"
return bool(self.conn.execute(command).fetchone()[0])

def create(self):
"""Create a database."""
with sqlite3.connect(self.path) as conn:
params = ", ".join(
f"{key} {' '.join(map(str.upper, props))} NOT NULL"
for key, props in self.structure.items()
)
command = f"CREATE TABLE {self.name} ({params})"
params = ", ".join(
f"{key} {' '.join(map(str.upper, props))} NOT NULL"
for key, props in self.structure.items()
)
command = f"CREATE TABLE IF NOT EXISTS {self.name} ({params})"

logger.debug("executing %s", command)
logger.debug("executing %s", command)

conn.execute(command)
self.conn.execute(command)
self.conn.commit()

def keys(self):
"""Get the column names of the table."""
Expand All @@ -101,13 +119,12 @@ def contains(self, **items) -> bool:

items = {k: str(v) for k, v in items.items()}

with sqlite3.connect(self.path) as conn:
conditions = " AND ".join(f"{key}=?" for key in items.keys())
command = f"SELECT EXISTS(SELECT 1 FROM {self.name} WHERE {conditions})"
conditions = " AND ".join(f"{key}=?" for key in items.keys())
command = f"SELECT EXISTS(SELECT 1 FROM {self.name} WHERE {conditions})"

logger.debug("Executing %s", command)
logger.debug("Executing %s", command)

return bool(conn.execute(command, tuple(items.values())).fetchone()[0])
return bool(self.conn.execute(command, tuple(items.values())).fetchone()[0])

def add(self, items: tuple[str]):
"""Add a row to the table.
Expand All @@ -124,12 +141,12 @@ def add(self, items: tuple[str]):
logger.debug("Executing %s", command)
logger.debug("Items to add: %s", items)

with sqlite3.connect(self.path) as conn:
try:
conn.execute(command, tuple(items))
except sqlite3.IntegrityError as e:
# tried to insert an item that was already there
logger.debug(e)
try:
self.conn.execute(command, tuple(items))
self.conn.commit()
except sqlite3.IntegrityError as e:
# tried to insert an item that was already there
logger.debug(e)
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a persistent connection means exceptions can leave the connection in an open transaction/locked state. add() currently commits on success but does not rollback() on failures (including IntegrityError), whereas the previous per-call context manager would cleanly end the transaction when an exception escaped. Consider wrapping the write in with self.conn: (commit/rollback) or adding an explicit rollback() in exception paths before returning/raising.

Suggested change
logger.debug(e)
logger.debug(e)
self.conn.rollback()
except Exception:
# rollback on any unexpected database error before propagating
self.conn.rollback()
raise

Copilot uses AI. Check for mistakes.

def remove(self, **items):
"""Remove items from a table.
Expand All @@ -141,21 +158,25 @@ def remove(self, **items):
conditions = " AND ".join(f"{key}=?" for key in items.keys())
command = f"DELETE FROM {self.name} WHERE {conditions}"

with sqlite3.connect(self.path) as conn:
logger.debug(command)
conn.execute(command, tuple(items.values()))
logger.debug(command)
self.conn.execute(command, tuple(items.values()))
self.conn.commit()
Comment on lines +162 to +163
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove() executes a DELETE then commits, but if execute() raises (e.g., OperationalError/database locked), the connection can be left mid-transaction since there is no rollback/transaction context. To keep the persistent connection usable after errors, wrap the operation in with self.conn: or ensure you rollback() in an exception handler.

Suggested change
self.conn.execute(command, tuple(items.values()))
self.conn.commit()
try:
self.conn.execute(command, tuple(items.values()))
self.conn.commit()
except sqlite3.Error:
# Ensure the connection is usable after a failed DELETE
self.conn.rollback()
raise

Copilot uses AI. Check for mistakes.

def all(self):
"""Iterate through the rows of the table."""
with sqlite3.connect(self.path) as conn:
return list(conn.execute(f"SELECT * FROM {self.name}"))
return list(self.conn.execute(f"SELECT * FROM {self.name}"))

def reset(self):
"""Delete the database file."""
if hasattr(self, 'conn') and self.conn:
self.conn.close()
self.conn = None
try:
os.remove(self.path)
except FileNotFoundError:
pass
self.conn = sqlite3.connect(self.path, check_same_thread=False)
self.create()


class Downloads(DatabaseBase):
Expand Down
5 changes: 3 additions & 2 deletions tests/test_deezer.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import os
import pytest
from unittest.mock import Mock, patch

import pytest
from util import arun

from streamrip.client.downloadable import DeezerDownloadable
from streamrip.client.deezer import DeezerClient
from streamrip.config import Config
from streamrip.exceptions import NonStreamableError


@pytest.fixture(scope="session")
def deezer_client():
"""Integration test fixture - requires DEEZER_ARL environment variable"""
Expand Down
Loading