fix(cron): add pending-jobs check to cron#882
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe cron invocation script now supports multiple endpoints via an ChangesMulti-endpoint cron invocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/python/invoke-cron.py`:
- Around line 96-97: Update the log messages that currently read
logger.debug(f"Request URL: {self.base_url}{endpoint}") and
logger.debug(f"Request headers: {headers}") (and the other changed statements at
the same function: lines around 136–137, 147–151) to include the required
function-name prefix in square brackets; replace them with messages like
logger.debug(f"[<enclosing_function_name>] Request URL:
{self.base_url}{endpoint}") and logger.debug(f"[<enclosing_function_name>]
Request headers: {mask_string(headers)}") (substitute <enclosing_function_name>
with the actual name of the function/method that contains these logger calls and
mask any sensitive values using mask_string where appropriate). Ensure all four
affected logger calls use the same pattern and include the function name prefix.
- Around line 149-151: The loop over self.endpoints currently lets a raised
exception from invoke_endpoint abort the whole cycle; modify the for endpoint in
self.endpoints loop to wrap the await self.invoke_endpoint(client, endpoint)
call in a per-endpoint try/except so one failing endpoint does not stop others,
e.g. call invoke_endpoint inside try, on success log via
logger.info(f"[{endpoint}] invoked successfully: {result}"), and on except catch
the exception, log it with logger.error including endpoint and exception
details, then continue to the next endpoint; keep the outer exception handling
for cycle-level failures but ensure per-endpoint errors are handled locally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 733966bd-f710-45eb-b363-4594ffe23dc5
📒 Files selected for processing (1)
scripts/python/invoke-cron.py
vprashrex
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/python/invoke-cron.py (1)
152-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd function-name prefix to per-endpoint run-loop logs.
These messages should include the enclosing function name (e.g.,
[run]) before endpoint context.Suggested patch
- logger.info(f"[{endpoint}] invoked successfully: {result}") + logger.info( + f"[run] [{endpoint}] invoked successfully: {result}" + ) except Exception as endpoint_error: logger.error( - f"[{endpoint}] invocation failed: {endpoint_error}" + f"[run] [{endpoint}] invocation failed: {endpoint_error}" )As per coding guidelines,
Prefix all log messages with the function name in square brackets: logger.info(f"[function_name] Message {mask_string(sensitive_value)}").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/python/invoke-cron.py` around lines 152 - 156, The per-endpoint log lines inside the run loop currently log only the endpoint; update the logger calls in the try/except around the invocation (the logger.info and logger.error shown) to prefix the message with the enclosing function name (e.g., "[run]") before the endpoint context (so messages become "[run][{endpoint}] ..."). Locate the try/except that catches endpoint_error in invoke-cron.py and change both logger.info and logger.error formats to include the function name prefix while keeping the existing endpoint and error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/python/invoke-cron.py`:
- Around line 152-156: The per-endpoint log lines inside the run loop currently
log only the endpoint; update the logger calls in the try/except around the
invocation (the logger.info and logger.error shown) to prefix the message with
the enclosing function name (e.g., "[run]") before the endpoint context (so
messages become "[run][{endpoint}] ..."). Locate the try/except that catches
endpoint_error in invoke-cron.py and change both logger.info and logger.error
formats to include the function name prefix while keeping the existing endpoint
and error text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f492e180-7e9d-46d1-9b53-58bc5b4381b1
📒 Files selected for processing (1)
scripts/python/invoke-cron.py
vprashrex
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/python/invoke-cron.py (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd return annotations to the touched methods.
Line 35 and Line 132 still omit
-> None, which violates the repo-wide typing rule.Proposed fix
- def __init__(self): + def __init__(self) -> None: ... - async def run(self): + async def run(self) -> None:As per coding guidelines,
Always add type hints to all function parameters and return values in Python code.Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/python/invoke-cron.py` at line 35, Add explicit return type annotations "-> None" to the methods missing them in this file: update the constructor "def __init__(self):" to "def __init__(self) -> None:" and also add "-> None" to the other touched method declared at line 132 (the other function in the diff). Ensure every function/method in this file has an explicit return annotation per the repo typing rule.
🧹 Nitpick comments (1)
scripts/python/invoke-cron.py (1)
153-156: ⚡ Quick winKeep the traceback on per-endpoint failures.
Lines 153-156 only log
str(endpoint_error), so unexpected failures lose the stack trace. Addexc_info=Truehere or switch tologger.exception(...).Proposed fix
except Exception as endpoint_error: logger.error( - f"[{endpoint}] invocation failed: {endpoint_error}" + f"[{endpoint}] invocation failed: {endpoint_error}", + exc_info=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/python/invoke-cron.py` around lines 153 - 156, The except block that catches Exception as endpoint_error currently calls logger.error(f"[{endpoint}] invocation failed: {endpoint_error}") which drops the traceback; change this to include the exception info by either using logger.exception(f"[{endpoint}] invocation failed") or calling logger.error(f"[{endpoint}] invocation failed: {endpoint_error}", exc_info=True) so the stack trace for endpoint_error is preserved (references: exception variable endpoint_error, logger.error currently used, and the endpoint variable in the message).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/python/invoke-cron.py`:
- Line 35: Add explicit return type annotations "-> None" to the methods missing
them in this file: update the constructor "def __init__(self):" to "def
__init__(self) -> None:" and also add "-> None" to the other touched method
declared at line 132 (the other function in the diff). Ensure every
function/method in this file has an explicit return annotation per the repo
typing rule.
---
Nitpick comments:
In `@scripts/python/invoke-cron.py`:
- Around line 153-156: The except block that catches Exception as endpoint_error
currently calls logger.error(f"[{endpoint}] invocation failed:
{endpoint_error}") which drops the traceback; change this to include the
exception info by either using logger.exception(f"[{endpoint}] invocation
failed") or calling logger.error(f"[{endpoint}] invocation failed:
{endpoint_error}", exc_info=True) so the stack trace for endpoint_error is
preserved (references: exception variable endpoint_error, logger.error currently
used, and the endpoint variable in the message).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7501130-068b-47ca-8c1e-27599ad8d43d
📒 Files selected for processing (1)
scripts/python/invoke-cron.py
2 participants
Target issue is #881
Summary
In the previous PR that added pending job monitoring, the invoke-cron.py script was modified locally but the changes were never pushed/included in the PR. The server-side implementation existed, but since the updated script wasn't shipped, the cron script was never calling /api/v1/cron/pending-jobs — so pending job monitoring was never actually running. This PR ships that missing change.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
Release Notes
New Features
Improvements