Conversation
- Fix socket accept busy loop: replace non-blocking select/default with blocking Accept(), checking stopChan only on error to prevent 100% CPU spin when Accept() returns transient errors - Replace go-git worktree.Status() with native git CLI commands (rev-parse, status --porcelain) with 2s timeout, eliminating ~500MB memory from pure-Go tree walking on large repos - Add 5s read deadline on socket connections to prevent goroutine leaks from clients that connect but never send data - Guard fetchRateLimit goroutine with TryLock to prevent unbounded goroutine accumulation when HTTP requests exceed 3s tick interval - Add exponential backoff (100/200/400ms) and max retry (3) to pub/sub Nack handling to prevent CPU-burning retry loops on persistent failures https://claude.ai/code/session_0128SDwXB8JYiSP8kyRjmSgw
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! 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 significantly enhances the daemon's performance and reliability by migrating Git operations to the native Git CLI for improved efficiency. It also introduces a robust exponential backoff retry mechanism for message delivery to ensure resilience against transient failures. Furthermore, concurrency issues in rate limit fetching have been addressed, and socket connection handling has been refined for better resource management and stability. Highlights
Changelog
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the daemon's performance and reliability by replacing go-git with native git CLI calls, adding robust retry logic for message delivery, and fixing a concurrency issue in rate limit fetching. The changes are well-implemented and address the stated goals effectively. My review includes a few suggestions to further enhance maintainability and robustness, such as refactoring duplicated code, replacing magic numbers with constants, and improving error handling in the socket connection loop.
| if err != nil { | ||
| select { | ||
| case <-p.stopChan: | ||
| return | ||
| default: | ||
| } | ||
| go p.handleConnection(conn) | ||
| continue |
There was a problem hiding this comment.
If p.listener.Accept() returns an error that is not due to the listener being closed (e.g., a temporary system error like running out of file descriptors), this loop could become a busy-loop, consuming high CPU. To make this more robust, consider handling specific errors. You could check if the error is net.ErrClosed to return from the function, and for other errors, log them and perhaps add a small delay to prevent spinning.
| go func() { | ||
| if !s.rateLimitFetchMu.TryLock() { | ||
| return | ||
| } | ||
| defer s.rateLimitFetchMu.Unlock() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| s.fetchRateLimit(ctx) | ||
| }() |
There was a problem hiding this comment.
This block of code for fetching the rate limit is a duplicate of the logic at lines 166-174. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a new private method on CCInfoTimerService. You could then call this new method as a goroutine from both locations.
| s.logger.Error("Max retries reached, dropping message", logFields) | ||
| return | ||
| } | ||
| backoff := time.Duration(100<<uint(retryCount-1)) * time.Millisecond |
There was a problem hiding this comment.
The value 100 used for the initial backoff is a magic number. To improve readability and maintainability, it's better to define it as a named constant, for example: const initialBackoffMillis = 100. The code would then be more self-explanatory: backoff := time.Duration(initialBackoffMillis<<uint(retryCount-1)) * time.Millisecond.
|
|
||
| func (p *SocketHandler) handleConnection(conn net.Conn) { | ||
| defer conn.Close() | ||
| conn.SetDeadline(time.Now().Add(5 * time.Second)) |
There was a problem hiding this comment.
The 5-second connection deadline is a magic number. It would be better to define this as a named constant at the package level, like const connectionDeadline = 5 * time.Second, to improve readability and make it easier to configure in the future.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Fix off-by-one in max retries check and add missing error argument to logger. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
Summary
This PR improves daemon performance and reliability through three main changes: replacing the pure-Go git library with native git CLI calls, adding retry logic with exponential backoff for message delivery, and fixing concurrency issues in rate limit fetching.
Key Changes
Replace go-git with native git CLI (
daemon/git.go)go-gitlibrary toos/execfor git operationsGitInfointerfaceAdd retry logic for message delivery (
daemon/chan.go)Fix rate limit fetch concurrency (
daemon/cc_info_timer.go)rateLimitFetchMumutex to guard concurrentfetchRateLimitgoroutinesTryLock()to prevent multiple concurrent fetchesImprove socket connection handling (
daemon/socket.go)acceptConnections()loop to check stop signal only on errorsImplementation Details
100ms * 2^(retryCount-1)https://claude.ai/code/session_0128SDwXB8JYiSP8kyRjmSgw