refactor(rate-limiter): remove redundant locks, raise on duplicate id by default

[kevinmessiaen]

Description

• Remove threading.Lock from RateLimiterRegistry (does nothing)
• Remove asyncio.Lock from MinIntervalRateLimiter state (does nothing)
• Raise ValueError on duplicate rate limiter id by default; opt-in to warning via env

Related Issue

• ENG-1430

[linear]

ENG-1430 Clean up "same id different config" handling atm

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the rate limiter implementation by streamlining its concurrency control mechanisms and enhancing the handling of duplicate rate limiter IDs. The changes remove unnecessary locking primitives, simplifying the codebase, and introduce a more robust error-first approach for duplicate ID registration, which can be optionally downgraded to a warning via an environment variable.

Highlights

• Concurrency Simplification: Removed redundant threading.Lock from RateLimiterRegistry and asyncio.Lock from MinIntervalRateLimiter state, as these locks were found to be ineffective or unnecessary.
• Duplicate ID Handling: Changed the default behavior for registering rate limiters with duplicate IDs to raise a ValueError instead of a RuntimeWarning, promoting a fail-fast approach.
• Configurable Error Behavior: Introduced an environment variable (GISKARD_DISABLE_DUPLICATE_RATE_LIMITERS_ERRORS) that allows users to opt into RuntimeWarning behavior for duplicate IDs, overriding the default ValueError.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• libs/giskard-core/src/giskard/core/rate_limiter/base.py
  • Removed the threading module import.
  • Eliminated the _lock attribute and its usage within the RateLimiterRegistry class.
  • Renamed the environment variable GISKARD_DISABLE_DUPLICATE_RATE_LIMITERS_WARNINGS to GISKARD_DISABLE_DUPLICATE_RATE_LIMITERS_ERRORS.
  • Modified the register_instance method to raise a ValueError when a duplicate rate limiter ID is encountered, unless the new environment variable is set, in which case a RuntimeWarning is issued.
• libs/giskard-core/src/giskard/core/rate_limiter/min_interval.py
  • Removed the asyncio.Lock attribute from _MinIntervalRateLimiterState.
  • Removed the async with self._state.lock: context manager from the throttle method.
• libs/giskard-core/tests/test_rate_limiter.py
  • Added ValidationError import from pydantic.
  • Updated test_warns_when_creating_rate_limiter_with_duplicate_id to test_raises_when_creating_rate_limiter_with_duplicate_id, asserting ValidationError is raised.
  • Modified test_does_not_warn_when_disabled_and_creating_rate_limiter_with_duplicate_id to verify that RuntimeWarning is issued when the new environment variable is enabled, instead of asserting no warnings.

Activity

• No human activity (comments, reviews, etc.) has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to refactor the rate limiter by removing what were thought to be redundant locks and changing the behavior for duplicate IDs. However, the removal of threading.Lock from the global RateLimiterRegistry introduces a critical race condition in multi-threaded environments, which could lead to data corruption and incorrect rate-limiting behavior. Furthermore, the new logic for handling duplicate IDs contains a significant logic error regarding environment variable names and provides incorrect instructions in the error message, potentially leading to unintended application crashes (Denial of Service). These issues, along with a misleading variable name, need to be addressed for correctness, clarity, and security.

[Hartorn]

Small nitpick on error message