Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Oct 21, 2025

Description

LCORE-741: quota limiter implementation for SQLite

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #LCORE-741

Summary by CodeRabbit

  • New Features

    • Added SQLite support for quota management (initialization, querying, revocation, and adjustments) alongside existing PostgreSQL support.
  • Bug Fixes / Reliability

    • Improved database connection and cursor handling and expanded error coverage to reduce resource leaks and make quota operations more robust.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Adds SQLite support alongside PostgreSQL for quota management, introduces SQLite-specific SQL constants, and refactors cursor handling to use explicit cursor creation/closure with expanded exception handling in quota-related modules.

Changes

Cohort / File(s) Summary
Cursor management refactor
src/quota/quota_limiter.py
Replace context-managed cursor usage with explicit cursor creation and null-initialization; add sqlite3 import, catch sqlite3.Error alongside psycopg2.OperationalError, and guard cursor.close() in a finally block with error handling.
Dual DB quota logic
src/quota/revokable_quota_limiter.py
Add branching for sqlite_connection_config vs postgres_connection_config across available_quota, revoke_quota, increase_quota, consume_tokens, and _init_quota; introduce SQLite-specific helpers (_read_available_quota, _revoke_quota, _increase_quota, _consume_tokens) that use SQLite SQL constants, manual cursor lifecycle, and explicit commits.
SQLite SQL constants
src/quota/sql.py
Add INIT_QUOTA_SQLITE, SELECT_QUOTA_SQLITE, SET_AVAILABLE_QUOTA_SQLITE, and UPDATE_AVAILABLE_QUOTA_SQLITE mirroring PostgreSQL variants and using SQLite parameter placeholders/format.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Limiter as RevokableQuotaLimiter
    participant Config
    participant SQLite_DB as SQLite
    participant PG_DB as PostgreSQL

    Client->>Limiter: available_quota()
    Limiter->>Config: inspect sqlite_connection_config / postgres_connection_config
    alt sqlite_connection_config present
        Limiter->>SQLite_DB: open connection, create cursor (explicit)
        SQLite_DB-->>Limiter: return rows
        Limiter->>SQLite_DB: commit (when mutating)
        Note right of Limiter: finally -> cursor.close() (guarded)
    else postgres_connection_config present
        Limiter->>PG_DB: with connection.cursor() as cur (context-managed)
        PG_DB-->>Limiter: return rows
        Note right of Limiter: context manager closes cursor
    else
        Limiter-->>Client: return 0
    end
    Limiter-->>Client: available quota value
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 I nibble rows in two DB beds,
SQLite carrots, Postgres threads.
Cursors closed with tidy care,
Tokens counted everywhere.
Hop — quota balanced in my lair.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "LCORE-741: quota limiter implementation for SQLite" is directly related to the main changes in the pull request. The code changes across all three modified files implement SQLite support for the quota limiter system: quota_limiter.py adds SQLite error handling and cursor management, revokable_quota_limiter.py routes quota operations to SQLite-specific SQL paths alongside PostgreSQL paths, and sql.py introduces four new SQLite-specific quota command constants. The title is concise, specific, includes the ticket reference, and clearly conveys to a teammate that this PR adds SQLite functionality to the quota limiter without vague language or misleading information.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19ff7e8 and c8d5c85.

📒 Files selected for processing (3)
  • src/quota/quota_limiter.py (2 hunks)
  • src/quota/revokable_quota_limiter.py (5 hunks)
  • src/quota/sql.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/quota/quota_limiter.py
  • src/quota/revokable_quota_limiter.py
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

  • src/quota/sql.py
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

  • src/quota/sql.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
🔇 Additional comments (2)
src/quota/sql.py (2)

52-91: LGTM! SQLite support correctly implemented.

The new SQLite-specific constants properly use ? placeholders and mirror the PostgreSQL variants. The implementation correctly handles parameter binding differences between the two databases.


60-61: The WHERE clause modifications are stylistic changes, not functional fixes.

The git diff shows the only change is converting lowercase and to uppercase AND in three PostgreSQL queries. Since SQL keywords are case-insensitive, and and AND are functionally identical—both execute exactly the same. This is a code style normalization, not a syntax correction or functional modification. The parameter binding remains unchanged and continues to work correctly with all three queries.

Additionally, new SQLite variants of these queries were added, not modifications to existing PostgreSQL queries. No backward compatibility concerns exist since the WHERE logic is identical.

Likely an incorrect or invalid review comment.


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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/quota/revokable_quota_limiter.py (2)

103-108: Critical: Missing SQLite implementation.

The increase_quota method only implements the PostgreSQL path. This breaks SQLite support, which is the primary goal of this PR.

Add the SQLite path:

         # timestamp to be used
         updated_at = datetime.now()
 
-        with self.connection.cursor() as cursor:
-            cursor.execute(
-                UPDATE_AVAILABLE_QUOTA_PG,
-                (self.increase_by, updated_at, subject_id, self.subject_type),
-            )
-            self.connection.commit()
+        if self.sqlite_connection_config is not None:
+            with self.connection.cursor() as cursor:
+                cursor.execute(
+                    UPDATE_AVAILABLE_QUOTA_SQLITE,
+                    (self.increase_by, updated_at, subject_id, self.subject_type),
+                )
+                self.connection.commit()
+        if self.postgres_connection_config is not None:
+            with self.connection.cursor() as cursor:
+                cursor.execute(
+                    UPDATE_AVAILABLE_QUOTA_PG,
+                    (self.increase_by, updated_at, subject_id, self.subject_type),
+                )
+                self.connection.commit()

175-201: Inconsistent cursor management patterns.

The _init_quota method also suffers from the same cursor management inconsistency.

Apply the context manager pattern to the SQLite path:

         if self.sqlite_connection_config is not None:
-            cursor = self.connection.cursor()
-            cursor.execute(
-                INIT_QUOTA_SQLITE,
-                (
-                    subject_id,
-                    self.subject_type,
-                    self.initial_quota,
-                    self.initial_quota,
-                    revoked_at,
-                ),
-            )
-            cursor.close()
-            self.connection.commit()
+            with self.connection.cursor() as cursor:
+                cursor.execute(
+                    INIT_QUOTA_SQLITE,
+                    (
+                        subject_id,
+                        self.subject_type,
+                        self.initial_quota,
+                        self.initial_quota,
+                        revoked_at,
+                    ),
+                )
+                self.connection.commit()
🧹 Nitpick comments (2)
src/quota/quota_limiter.py (1)

78-92: Consider retaining the context manager pattern.

The change from a context manager to manual cursor handling adds complexity without apparent benefit. Context managers automatically handle cleanup even on exceptions, making the code more robust and concise.

If you must use manual cursor management, the current implementation with the finally block is acceptable. However, consider reverting to the context manager pattern:

-        cursor = None
         try:
-            cursor = self.connection.cursor()
-            cursor.execute("SELECT 1")
+            with self.connection.cursor() as cursor:
+                cursor.execute("SELECT 1")
             logger.info("Connection to storage is ok")
             return True
         except (psycopg2.OperationalError, sqlite3.Error) as e:
             logger.error("Disconnected from storage: %s", e)
             return False
-        finally:
-            if cursor is not None:
-                try:
-                    cursor.close()
-                except Exception:  # pylint: disable=broad-exception-caught
-                    logger.warning("Unable to close cursor")
src/quota/revokable_quota_limiter.py (1)

43-201: Consider extracting common database operation pattern.

Each method duplicates the pattern of checking config type, creating cursor, executing SQL, and committing. This duplication increases maintenance burden.

Consider extracting a helper method:

def _execute_query(
    self, 
    sqlite_sql: str, 
    pg_sql: str, 
    params: tuple,
    fetch: bool = False
) -> Any:
    """Execute query on the appropriate database backend."""
    if self.sqlite_connection_config is not None:
        with self.connection.cursor() as cursor:
            cursor.execute(sqlite_sql, params)
            if fetch:
                return cursor.fetchone()
            self.connection.commit()
    elif self.postgres_connection_config is not None:
        with self.connection.cursor() as cursor:
            cursor.execute(pg_sql, params)
            if fetch:
                return cursor.fetchone()
            self.connection.commit()
    else:
        raise RuntimeError("No database configuration available")

This would simplify all methods and ensure consistent patterns.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a761489 and d385cdc.

📒 Files selected for processing (3)
  • src/quota/quota_limiter.py (1 hunks)
  • src/quota/revokable_quota_limiter.py (6 hunks)
  • src/quota/sql.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

  • src/quota/sql.py
  • src/quota/revokable_quota_limiter.py
  • src/quota/quota_limiter.py
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

  • src/quota/sql.py
  • src/quota/revokable_quota_limiter.py
  • src/quota/quota_limiter.py
🧬 Code graph analysis (2)
src/quota/revokable_quota_limiter.py (1)
src/utils/connection_decorator.py (1)
  • connection (6-22)
src/quota/quota_limiter.py (1)
src/utils/connection_decorator.py (1)
  • connection (6-22)
🪛 GitHub Actions: Pyright
src/quota/revokable_quota_limiter.py

[error] 43-43: Function with declared return type "int" must return value on all code paths. None is not assignable to "int" (reportReturnType)

src/quota/quota_limiter.py

[error] 84-84: "sqlite3" is not defined (reportUndefinedVariable)

🪛 GitHub Actions: Python linter
src/quota/revokable_quota_limiter.py

[error] 43-43: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

src/quota/quota_limiter.py

[error] 84-84: Undefined variable 'sqlite3' (undefined-variable). Likely missing an import for sqlite3.

🪛 GitHub Actions: Ruff
src/quota/quota_limiter.py

[error] 84-84: F821 Undefined name 'sqlite3' referenced in exception handling. Ensure sqlite3 is imported or referenced correctly.

🪛 GitHub Actions: Type checks
src/quota/revokable_quota_limiter.py

[error] 43-43: Mypy error: Missing return statement. [return]

src/quota/quota_limiter.py

[error] 84-84: Mypy error: Name 'sqlite3' is not defined. [name-defined]

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
🔇 Additional comments (5)
src/quota/sql.py (4)

52-55: LGTM!

The SQLite initialization query correctly mirrors the PostgreSQL variant with proper SQLite parameter placeholders.


60-67: LGTM!

The SELECT query formatting is improved for consistency, and the SQLite variant correctly implements the same logic with appropriate parameter placeholders.


72-79: LGTM!

Both PostgreSQL and SQLite UPDATE queries for setting available quota are syntactically correct with proper parameter placeholders.


84-91: LGTM!

The UPDATE queries for incrementing available quota are correctly implemented for both database backends.

src/quota/revokable_quota_limiter.py (1)

13-19: LGTM!

The SQLite SQL constants are correctly imported alongside their PostgreSQL counterparts.

Comment on lines 47 to 69
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SELECT_QUOTA_PG,
SELECT_QUOTA_SQLITE,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
cursor.close()
return value[0]
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_PG,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Inconsistent cursor management patterns.

The SQLite path uses manual cursor handling (create, close) while the PostgreSQL path uses a context manager. This inconsistency makes the code harder to maintain and more error-prone.

Consider unifying to context managers for both paths:

         if self.sqlite_connection_config is not None:
-            cursor = self.connection.cursor()
-            cursor.execute(
-                SELECT_QUOTA_SQLITE,
-                (subject_id, self.subject_type),
-            )
-            value = cursor.fetchone()
-            if value is None:
-                self._init_quota(subject_id)
-                return self.initial_quota
-            cursor.close()
-            return value[0]
+            with self.connection.cursor() as cursor:
+                cursor.execute(
+                    SELECT_QUOTA_SQLITE,
+                    (subject_id, self.subject_type),
+                )
+                value = cursor.fetchone()
+                if value is None:
+                    self._init_quota(subject_id)
+                    return self.initial_quota
+                return value[0]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SELECT_QUOTA_PG,
SELECT_QUOTA_SQLITE,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
cursor.close()
return value[0]
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_PG,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
if self.sqlite_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_SQLITE,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_PG,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
🤖 Prompt for AI Agents
In src/quota/revokable_quota_limiter.py around lines 47 to 69, the SQLite branch
manually creates and closes a cursor while the Postgres branch uses a context
manager; unify the patterns by using a context manager for the SQLite branch as
well (i.e., replace manual cursor creation/close with "with
self.connection.cursor() as cursor:"), and if the sqlite cursor implementation
in our environment does not support the context-manager protocol, replace the
manual approach with a try/finally that ensures cursor.close() in the finally
block so both branches guarantee deterministic cursor cleanup and match the
Postgres pattern.

⚠️ Potential issue | 🔴 Critical

Critical: Missing return statement for edge case.

The function must return a value on all code paths. If both sqlite_connection_config and postgres_connection_config are None, the function returns None implicitly, violating the return type annotation.

Add a fallback return or raise an exception:

                 if value is None:
                     self._init_quota(subject_id)
                     return self.initial_quota
                 return value[0]
+        
+        raise RuntimeError("No database configuration available")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SELECT_QUOTA_PG,
SELECT_QUOTA_SQLITE,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
cursor.close()
return value[0]
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_PG,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SELECT_QUOTA_SQLITE,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
cursor.close()
return value[0]
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SELECT_QUOTA_PG,
(subject_id, self.subject_type),
)
value = cursor.fetchone()
if value is None:
self._init_quota(subject_id)
return self.initial_quota
return value[0]
raise RuntimeError("No database configuration available")
🤖 Prompt for AI Agents
In src/quota/revokable_quota_limiter.py around lines 47 to 69, the method can
fall through with both sqlite_connection_config and postgres_connection_config
being None and return None implicitly; add an explicit fallback to satisfy the
return type by raising a clear exception (e.g., RuntimeError or ValueError)
indicating no database backend is configured, or alternatively return a sensible
default (like self.initial_quota) depending on intended semantics; implement the
chosen fallback at the end of the function so every code path returns or raises.

Comment on lines 79 to 95
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SET_AVAILABLE_QUOTA_PG,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SET_AVAILABLE_QUOTA_PG,
SET_AVAILABLE_QUOTA_SQLITE,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
cursor.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Inconsistent cursor management patterns.

Same issue as in available_quota: SQLite uses manual cursor management while PostgreSQL uses a context manager.

Apply the same fix - use context managers for both paths:

         if self.sqlite_connection_config is not None:
-            cursor = self.connection.cursor()
-            cursor.execute(
-                SET_AVAILABLE_QUOTA_SQLITE,
-                (self.initial_quota, revoked_at, subject_id, self.subject_type),
-            )
-            self.connection.commit()
-            cursor.close()
+            with self.connection.cursor() as cursor:
+                cursor.execute(
+                    SET_AVAILABLE_QUOTA_SQLITE,
+                    (self.initial_quota, revoked_at, subject_id, self.subject_type),
+                )
+                self.connection.commit()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SET_AVAILABLE_QUOTA_PG,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
cursor.execute(
SET_AVAILABLE_QUOTA_PG,
SET_AVAILABLE_QUOTA_SQLITE,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
cursor.close()
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SET_AVAILABLE_QUOTA_PG,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
if self.sqlite_connection_config is not None:
with self.connection.cursor() as cursor:
cursor.execute(
SET_AVAILABLE_QUOTA_SQLITE,
(self.initial_quota, revoked_at, subject_id, self.subject_type),
)
self.connection.commit()
🤖 Prompt for AI Agents
In src/quota/revokable_quota_limiter.py around lines 79 to 93, the SQLite branch
manually opens and closes a cursor while the PostgreSQL branch uses a context
manager; change the SQLite path to use a with self.connection.cursor() as cursor
context manager (like the Postgres branch), perform the execute inside that
context, call self.connection.commit() after the with block, and remove the
explicit cursor.close() to ensure consistent, safe cursor management.

Comment on lines 140 to 162
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
# timestamp to be used
updated_at = datetime.now()

cursor.execute(
UPDATE_AVAILABLE_QUOTA_PG,
UPDATE_AVAILABLE_QUOTA_SQLITE,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
cursor.close()
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
# timestamp to be used
updated_at = datetime.now()

cursor.execute(
UPDATE_AVAILABLE_QUOTA_PG,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Inconsistent cursor management patterns.

Same cursor management inconsistency: manual cursor handling for SQLite vs. context manager for PostgreSQL.

Unify to context managers:

         if self.sqlite_connection_config is not None:
-            cursor = self.connection.cursor()
-            # timestamp to be used
-            updated_at = datetime.now()
-
-            cursor.execute(
-                UPDATE_AVAILABLE_QUOTA_SQLITE,
-                (-to_be_consumed, updated_at, subject_id, self.subject_type),
-            )
-            self.connection.commit()
-            cursor.close()
+            with self.connection.cursor() as cursor:
+                # timestamp to be used
+                updated_at = datetime.now()
+
+                cursor.execute(
+                    UPDATE_AVAILABLE_QUOTA_SQLITE,
+                    (-to_be_consumed, updated_at, subject_id, self.subject_type),
+                )
+                self.connection.commit()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.sqlite_connection_config is not None:
cursor = self.connection.cursor()
# timestamp to be used
updated_at = datetime.now()
cursor.execute(
UPDATE_AVAILABLE_QUOTA_PG,
UPDATE_AVAILABLE_QUOTA_SQLITE,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
cursor.close()
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
# timestamp to be used
updated_at = datetime.now()
cursor.execute(
UPDATE_AVAILABLE_QUOTA_PG,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
if self.sqlite_connection_config is not None:
with self.connection.cursor() as cursor:
# timestamp to be used
updated_at = datetime.now()
cursor.execute(
UPDATE_AVAILABLE_QUOTA_SQLITE,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
if self.postgres_connection_config is not None:
with self.connection.cursor() as cursor:
# timestamp to be used
updated_at = datetime.now()
cursor.execute(
UPDATE_AVAILABLE_QUOTA_PG,
(-to_be_consumed, updated_at, subject_id, self.subject_type),
)
self.connection.commit()
🤖 Prompt for AI Agents
In src/quota/revokable_quota_limiter.py around lines 140 to 160, you currently
open and close the SQLite cursor manually but use a context manager for the
PostgreSQL cursor; unify both to context-manager style. Replace the manual
SQLite cursor block with a context-managed cursor (use
contextlib.closing(self.connection.cursor()) as cursor or use the connection as
a context manager and acquire the cursor inside) set updated_at inside the
block, execute UPDATE_AVAILABLE_QUOTA_SQLITE with the same parameters, call
self.connection.commit() after the with-block, and remove the explicit
cursor.close(); keep the PostgreSQL block as-is so both backends use
context-managed cursors and consistent timestamp/commit semantics.

@tisnik tisnik force-pushed the lcore-741-quota-limiter-implementation-for-sqlite branch from d385cdc to 19ff7e8 Compare October 21, 2025 11:07
@tisnik tisnik force-pushed the lcore-741-quota-limiter-implementation-for-sqlite branch from 19ff7e8 to c8d5c85 Compare October 21, 2025 11:36
@tisnik
Copy link
Contributor Author

tisnik commented Oct 21, 2025

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@tisnik tisnik merged commit bd3d836 into lightspeed-core:main Oct 21, 2025
18 of 20 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Oct 27, 2025
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant