refactor(users): align regenerate_token with documented behavior#309
refactor(users): align regenerate_token with documented behavior#309Matteovanypersele wants to merge 1 commit intolinagora:devfrom
Conversation
The endpoint description states that token regeneration is allowed for admins and for the user themselves, but the implementation had no dependency enforcing this. Add the admin-or-self check. Cover the allowed (self) and denied (cross-user) paths with Robot tests.
📝 WalkthroughWalkthroughThis PR adds timeout and exponential backoff retry configurations for three data loaders (Whisper, Marker, Docling). New configuration parameters define timeout values and retry behavior, wired through Pydantic models and applied via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Loader
participant RetryHandler
participant TimeoutWrapper
participant RayActor
Caller->>Loader: transcribe/process(path)
Loader->>RetryHandler: retry_with_backoff(attempt_fn, max_retries, base_delay)
loop Attempt (up to max_retries + 1)
RetryHandler->>Loader: attempt_fn(attempt_index)
Loader->>Loader: select_worker / acquire_actor
Loader->>TimeoutWrapper: call_ray_actor_with_timeout(...)
TimeoutWrapper->>RayActor: .remote(path)
alt Success
RayActor-->>TimeoutWrapper: result
TimeoutWrapper-->>Loader: return result
Loader-->>RetryHandler: return result
RetryHandler-->>Caller: return result
else Timeout / Exception
RayActor-->>TimeoutWrapper: timeout/error
TimeoutWrapper-->>Loader: raise exception
Loader-->>Loader: cleanup (return_worker)
Loader-->>RetryHandler: exception
alt Final Attempt
RetryHandler-->>Caller: re-raise
else More Retries Available
RetryHandler->>RetryHandler: await delay (base_delay * 2^attempt)
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Matteovanypersele
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openrag/components/ray_utils.py (1)
61-66: Nit: type hint doesn't reflect thatattempt_fnmust be async.
attempt_fnis awaited at line 76, so the callable must return an awaitable. The current annotationCallable[[int], Any]would type-check a plain sync callable returning a non-awaitable, defeating the purpose of the hint.♻️ Suggested change
-from collections.abc import Callable +from collections.abc import Awaitable, Callableasync def retry_with_backoff( - attempt_fn: Callable[[int], Any], + attempt_fn: Callable[[int], Awaitable[Any]], max_retries: int, base_delay: float, task_description: str = "task", ) -> Any:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/ray_utils.py` around lines 61 - 66, The type hint for retry_with_backoff's attempt_fn is incorrect: it must be an async function returning an awaitable. Update the annotation for attempt_fn in retry_with_backoff to reflect an awaitable return type (e.g., use Callable[[int], Awaitable[Any]] or Coroutine[Any, Any, Any]) so static type checkers know the callable is awaitable; keep the rest of the signature and behavior unchanged.openrag/config/models.py (1)
250-252: LGTM — field names, types, and defaults matchconf/config.yaml.
whisper_timeout=1800,whisper_max_task_retry=1,whisper_retry_base_delay=2.0,marker_max_task_retry=3,marker_retry_base_delay=2.0,docling_timeout=3600,docling_max_task_retry=3,docling_retry_base_delay=2.0all align with the YAML keys and defaults.Optional: for consistency with the adjacent
docling_*fields that useField(default=..., ge=...)validators (Lines 346-348), you could add non-negative validators on the new timeout/retry fields (e.g.ge=0for timeout/delay,ge=0for retries). Non-blocking.Also applies to: 344-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/models.py` around lines 250 - 252, Add non-negative validators to the new whisper_* fields to match the pattern used for docling_*: update the whisper_timeout, whisper_max_task_retry, and whisper_retry_base_delay declarations to use Field(default=..., ge=0) (or ge=0 for retries and delays, ge=0 for timeout) so they enforce non-negative values; apply the same change to the other mentioned fields at lines 344-351 (marker_max_task_retry, marker_retry_base_delay, docling_timeout, docling_max_task_retry, docling_retry_base_delay) to keep validation consistent with the existing docling_* Field(…ge=…) pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openrag/components/ray_utils.py`:
- Around line 61-66: The type hint for retry_with_backoff's attempt_fn is
incorrect: it must be an async function returning an awaitable. Update the
annotation for attempt_fn in retry_with_backoff to reflect an awaitable return
type (e.g., use Callable[[int], Awaitable[Any]] or Coroutine[Any, Any, Any]) so
static type checkers know the callable is awaitable; keep the rest of the
signature and behavior unchanged.
In `@openrag/config/models.py`:
- Around line 250-252: Add non-negative validators to the new whisper_* fields
to match the pattern used for docling_*: update the whisper_timeout,
whisper_max_task_retry, and whisper_retry_base_delay declarations to use
Field(default=..., ge=0) (or ge=0 for retries and delays, ge=0 for timeout) so
they enforce non-negative values; apply the same change to the other mentioned
fields at lines 344-351 (marker_max_task_retry, marker_retry_base_delay,
docling_timeout, docling_max_task_retry, docling_retry_base_delay) to keep
validation consistent with the existing docling_* Field(…ge=…) pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1c03a97-d0de-45f2-a79c-16935fd5509c
📒 Files selected for processing (8)
conf/config.yamlopenrag/components/indexer/loaders/audio/local_whisper.pyopenrag/components/indexer/loaders/pdf_loaders/docling2.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/ray_utils.pyopenrag/config/models.pyopenrag/routers/users.pytests/api/users.robot
|
Thanks a lot for this PR. It turns out another contributor was working on the same feature in parallel, enabling both users and admins to regenerate user tokens. Additionally, IndexerUI has been updated to include a button for this action. As a result, this PR will be closed. |
2 participants
We align
regenerate_tokenwith its documented behavior.We add the admin-or-self check described in the endpoint documentation, following the same
is_adminpattern already used in the other functions of this file.Two Robot cases added in
tests/api/users.robotto cover the two expected behaviors (self → 200, cross-user → 403).Summary by CodeRabbit
New Features
Improvements
Tests