Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the periodic 60s pull handler with ChangesGit Library Provider Initialization & Pull Refactoring
Sequence Diagram(s)sequenceDiagram
participant PubSub as PubSub (60s tick)
participant Provider as GitLibraryProvider
participant Proactor as ProactorService
participant Git as pygit2 Repo
participant Pub as Library Publisher
PubSub->>Provider: tick / event
Provider->>Provider: _periodic_check()
alt Repo missing
Provider->>Provider: set InitInProgress
Provider->>Proactor: execute _init_task()
Proactor->>Git: clone or open repo & validate
Git-->>Proactor: repo object or error
Proactor->>Provider: init result (fresh_clone?)
Provider->>Provider: _set_ready() on success
end
Provider->>Provider: check PullLock & PullInterval
alt allowed to pull
Provider->>Proactor: execute _periodic_pull()
Proactor->>Provider: _do_pull() results (changed paths)
Provider->>Pub: publish Library.change! for each path
Provider->>Provider: update LastPull and log
else skip (locked/throttled)
Provider-->>PubSub: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 16 minutes and 6 seconds.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: 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 `@asab/library/providers/git.py`:
- Around line 365-389: The docstring for _init_task is inaccurate and calling
self._do_pull() on existing repositories also leaves LastPull unset causing an
immediate duplicate pull; update the _init_task docstring to state that existing
repositories are pulled during init (or remove the self._do_pull() call if you
prefer to defer pulls entirely to _periodic_pull), and if you keep the
_do_pull() call then set LastPull to the current timestamp right after
self._do_pull() (use the same time representation used elsewhere) so the
subsequent _periodic_pull() doesn’t immediately run a second pull; references:
_init_task, _do_pull, _periodic_pull, LastPull, GitRepository, RepoPath.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f82c589a-4efa-4593-8641-a123f3cc89b8
📒 Files selected for processing (1)
asab/library/providers/git.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
asab/library/providers/git.py (1)
468-469:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize
LastPullwhen setup succeeds.After a successful clone or init-time pull,
LastPullis stillNone, so the nextApplication.tick/60!bypassesPullIntervaland fetches again immediately. Seed it here once initialization completes so the throttle applies from startup as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/providers/git.py` around lines 468 - 469, After a successful repo setup (the block that calls await self._set_ready()), initialize the provider's LastPull timestamp so the PullInterval throttle applies immediately after startup; set self.LastPull to the current time (e.g., utcnow/timestamp) right before or after calling _set_ready() in the same initialization path where clone/init pull succeeds. This change should be made in the git provider setup code that currently only calls _set_ready() so subsequent Application.tick/60 checks respect PullInterval from startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/providers/git.py`:
- Around line 419-436: The init flow can leave self.GitRepository set when
_init_task() fails during _do_pull(), which prevents _periodic_check() from
retrying since it only checks for None; modify _init_task() so that any early
returns in the except handlers clear self.GitRepository (or better: only assign
self.GitRepository after the entire init/pull succeeds) — update the KeyError
and pygit2.GitError handlers (and any similar early-return error handlers around
the _do_pull/_init sequence) to unset self.GitRepository before returning or
defer the assignment until after _set_ready() completes.
- Around line 437-440: The log call in L.exception inside
asab/library/providers/git.py leaks credentials by including self.DeployToken in
struct_data; remove self.DeployToken from struct_data (or replace it with a
redacted marker like "deploy_token_redacted": True) so the deploy token is not
logged, keeping other fields (self.Layer, self.URLPath, self.User) unchanged and
preserving the existing error message in the L.exception call.
- Around line 331-333: The periodic checker is scheduling
initialize_git_repository() every tick without deduplication, causing
overlapping inits; add an in-flight boolean (e.g., self.InitInProgress
initialized in the class __init__) and use it in _periodic_check to skip
scheduling when True, and in initialize_git_repository set self.InitInProgress =
True at start and clear it in a finally block (on both success and failure) so
only one init runs or is enqueued for a given RepoPath; reference symbols:
GitRepository, _periodic_check, initialize_git_repository, TaskService.schedule,
and self.InitInProgress.
---
Duplicate comments:
In `@asab/library/providers/git.py`:
- Around line 468-469: After a successful repo setup (the block that calls await
self._set_ready()), initialize the provider's LastPull timestamp so the
PullInterval throttle applies immediately after startup; set self.LastPull to
the current time (e.g., utcnow/timestamp) right before or after calling
_set_ready() in the same initialization path where clone/init pull succeeds.
This change should be made in the git provider setup code that currently only
calls _set_ready() so subsequent Application.tick/60 checks respect PullInterval
from startup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d0967d1-d103-458a-9295-cbd6b9ce9f99
📒 Files selected for processing (1)
asab/library/providers/git.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/providers/git.py`:
- Around line 397-399: The current validation only checks for the existence of
the remotes attribute (hasattr(self.GitRepository, "remotes")) but does not
ensure the 'origin' remote is present, which can lead to a KeyError later in
_do_fetch(); replace that check with an explicit membership check for "origin"
in self.GitRepository.remotes and raise a RuntimeError with a message mentioning
the missing 'origin' remote if it is not found so the repository is validated
correctly before _do_fetch() uses remotes["origin"].
- Around line 435-470: The catch blocks logging KeyError/pygit2.GitError
indicate permanent config problems (missing branch, 404, auth failure) but leave
GitRepository None so _periodic_check keeps retrying; mark these as terminal by
setting a persistent instance flag (e.g. self._permanent_init_failure = True)
inside the specific exception branches where you call L.exception (use the same
branches that inspect self.Branch/self.URLPath/User), and ensure _periodic_check
first skips rescheduling if self._permanent_init_failure is True (and still
allows a successful init to clear that flag or provide a config-change hook to
reset it). This prevents re-enqueuing known-permanent failures until config
changes or a restart.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8072f79f-50b0-429d-b63b-ab6f4eb29045
📒 Files selected for processing (1)
asab/library/providers/git.py
|
If the microservice starts with existing git repo, it reads it as it is and sets library ready. It pulls immediately after. Even if pull is not possible, the library is still ready. Even though it might be old. We don't use this behaviour currently in the deployments (git repo persists only inside the container, is deleted on restart) |
|
Generally, failed pull doesn't flag the library NOT ready. That is convenient when the library is not changing but connection to the server is unreliable. Should I set the Library not ready when pull fails? @mejroslav @ateska ? |
There was a problem hiding this comment.
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 `@asab/library/providers/git.py`:
- Around line 486-491: After a successful fresh clone you must initialize the
provider's LastPull so PullInterval is honored; modify the code in the branch
after await self._set_ready() to set LastPull (the attribute used by
_periodic_pull) to the current time when fresh_clone is True instead of leaving
it None, so subsequent Application.tick uses the configured PullInterval before
triggering a pull. Update the code path that currently only calls _periodic_pull
when not fresh_clone (around the await self._set_ready() / if not fresh_clone
block) to set LastPull appropriately for the fresh-clone case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6895c899-095a-4dae-82f4-2f2b71f54d8a
📒 Files selected for processing (1)
asab/library/providers/git.py
|
I tested on localhost that :
I added a check that the initialition task does not run mutliple times at once. I rearranged the code (parse_url_fragment, error flow in init) to improve readability. I have separated periodic_pull method to periodic_check (with check interval 60s) and periodic_pull (with pull interval 12h if not configured). Only to improve readability. |
1 participant
I tested on localhost that :
I added a check that the initialition task does not run mutliple times at once.
I rearranged the code (parse_url_fragment, error flow in init) to improve readability.
I have separated periodic_pull method to periodic_check (with check interval 60s) and periodic_pull (with pull interval 12h if not configured). Only to improve readability.