Skip to content

LCORE-1438: Organize imports in quota handler#1323

Merged
tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-1438-organize-imports-in-quota-handler
Mar 15, 2026
Merged

LCORE-1438: Organize imports in quota handler#1323
tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-1438-organize-imports-in-quota-handler

Conversation

@tisnik

@tisnik tisnik commented Mar 15, 2026

Copy link
Copy Markdown
Contributor

Description

LCORE-1438: Organize imports in quota handler

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
  • Benchmarks improvement

Tools used to create PR

  • Assisted-by: N/A
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-1438

Summary by CodeRabbit

  • Chores
    • Reorganized imports and formatting across quota management modules for improved code maintainability and consistency. No functional changes or user-facing impact.

@coderabbitai

coderabbitai Bot commented Mar 15, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This PR reorganizes imports across the quota module files, adding datetime and sqlite3 imports, adjusting import ordering for consistency, and introducing INIT_QUOTA_PG and INIT_QUOTA_SQLITE constants alongside existing update quota constants. No public APIs, control flow logic, or method signatures are modified.

Changes

Cohort / File(s) Summary
Import Reorganization
src/quota/cluster_quota_limiter.py, src/quota/user_quota_limiter.py, src/quota/quota_limiter_factory.py
Reordered imports to standardize placement; moved get_logger and QuotaHandlersConfiguration imports to different positions within files.
New and Adjusted Imports
src/quota/quota_limiter.py
Added datetime and sqlite3 imports; reordered models.config imports to list PostgreSQLDatabaseConfiguration before SQLiteDatabaseConfiguration.
Import Source Consolidation
src/quota/token_usage_history.py
Relocated PostgreSQLDatabaseConfiguration, QuotaHandlersConfiguration, and SQLiteDatabaseConfiguration imports from local grouping to models.config source; swapped UTC and datetime import order.
Initialization Constants
src/quota/revokable_quota_limiter.py
Introduced INIT_QUOTA_PG and INIT_QUOTA_SQLITE imports alongside existing UPDATE_AVAILABLE_QUOTA_* constants to support initialization paths.
Formatting
src/quota/connect_pg.py
Added blank line after typing import for spacing consistency.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: organizing imports across quota handler modules. The change is specific, clear, and directly reflects the primary objective of the refactoring work.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/quota/revokable_quota_limiter.py`:
- Around line 12-13: available_quota() races on initial row creation because
INIT_QUOTA_PG and INIT_QUOTA_SQLITE are plain INSERTs; make initialization
idempotent by changing INIT_QUOTA_PG to use a conflict-tolerant INSERT (e.g.
INSERT ... ON CONFLICT DO NOTHING) and INIT_QUOTA_SQLITE to use SQLite's
conflict form (e.g. INSERT OR IGNORE), or alternatively wrap the INSERT in
available_quota() with a duplicate-key handler that swallows the
unique-constraint error and proceeds to return initial_quota; update the SQL
constants INIT_QUOTA_PG and INIT_QUOTA_SQLITE (or add the duplicate-key catch in
available_quota()) so concurrent first requests cannot cause a duplicate-row
failure.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c93d21d4-0170-4f95-817b-04e60dacb002

📥 Commits

Reviewing files that changed from the base of the PR and between 89cac0f and 0291d01.

📒 Files selected for processing (7)
  • src/quota/cluster_quota_limiter.py
  • src/quota/connect_pg.py
  • src/quota/quota_limiter.py
  • src/quota/quota_limiter_factory.py
  • src/quota/revokable_quota_limiter.py
  • src/quota/token_usage_history.py
  • src/quota/user_quota_limiter.py

Comment on lines +12 to +13
INIT_QUOTA_PG,
INIT_QUOTA_SQLITE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Make quota initialization idempotent.

available_quota() follows a read-then-initialize flow. With INIT_QUOTA_PG / INIT_QUOTA_SQLITE defined as plain INSERTs in src/quota/sql.py:60-68, two concurrent first requests for the same subject can both see "missing" and one will fail on the duplicate row. Please make the init SQL conflict-tolerant, or catch duplicate-key violations before returning initial_quota.

Possible fix
-INIT_QUOTA_PG = """
-    INSERT INTO quota_limits (id, subject, quota_limit, available, revoked_at)
-    VALUES (%s, %s, %s, %s, %s)
-    """
+INIT_QUOTA_PG = """
+    INSERT INTO quota_limits (id, subject, quota_limit, available, revoked_at)
+    VALUES (%s, %s, %s, %s, %s)
+    ON CONFLICT DO NOTHING
+    """

-INIT_QUOTA_SQLITE = """
-    INSERT INTO quota_limits (id, subject, quota_limit, available, revoked_at)
-    VALUES (?, ?, ?, ?, ?)
-    """
+INIT_QUOTA_SQLITE = """
+    INSERT OR IGNORE INTO quota_limits (id, subject, quota_limit, available, revoked_at)
+    VALUES (?, ?, ?, ?, ?)
+    """
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/quota/revokable_quota_limiter.py` around lines 12 - 13, available_quota()
races on initial row creation because INIT_QUOTA_PG and INIT_QUOTA_SQLITE are
plain INSERTs; make initialization idempotent by changing INIT_QUOTA_PG to use a
conflict-tolerant INSERT (e.g. INSERT ... ON CONFLICT DO NOTHING) and
INIT_QUOTA_SQLITE to use SQLite's conflict form (e.g. INSERT OR IGNORE), or
alternatively wrap the INSERT in available_quota() with a duplicate-key handler
that swallows the unique-constraint error and proceeds to return initial_quota;
update the SQL constants INIT_QUOTA_PG and INIT_QUOTA_SQLITE (or add the
duplicate-key catch in available_quota()) so concurrent first requests cannot
cause a duplicate-row failure.

@tisnik tisnik merged commit abe551b into lightspeed-core:main Mar 15, 2026
24 checks passed
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