Add retry with backoff and per-blog error reporting#16
Merged
JulienTant merged 3 commits intomainfrom Apr 5, 2026
Merged
Conversation
Transient HTTP failures (timeouts, 503s) now retry up to 3 times with exponential backoff via cenkalti/backoff/v5. ScanAllBlogs collects per-blog errors instead of failing fast, so successful scans are not lost when one blog is temporarily unreachable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds exponential-backoff retries for network operations in the scanner, records per-blog errors in a new ScanResult.Error field, changes scanning to continue on non-fatal per-blog failures, and updates CLI scan reporting and exit semantics to reflect partial vs total failures. New tests and a backoff dependency were added. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI: scan command
participant Scanner as Scanner
participant Retry as retryHTTP
participant HTTP as Remote Feed (HTTP)
participant DB as Repo/Results
CLI->>Scanner: ScanAllBlogs(ctx, blogs)
loop per blog
Scanner->>Retry: retryHTTP(discover/parse/scrape)
Retry->>HTTP: HTTP request
alt transient 5xx
HTTP-->>Retry: 5xx
Retry->>Retry: backoff + retry
Retry->>HTTP: retry request
HTTP-->>Retry: 200 (success)
Retry-->>Scanner: Success (data)
Scanner->>DB: Record result (Error empty, NewArticles...)
else persistent failure or retries exhausted
HTTP-->>Retry: repeated 5xx / failure
Retry-->>Scanner: Error ("failed to fetch feed")
Scanner->>DB: Record result (Error set)
end
end
Scanner-->>CLI: []ScanResult (includes Error fields)
CLI->>CLI: Print per-blog output and summary (X succeeded, Y failed)
alt silent mode and all failed
CLI-->>CLI: exit non-zero
else
CLI-->>CLI: exit 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/scanner/scanner.go (1)
61-69:⚠️ Potential issue | 🟠 MajorFeed auto-discovery still won't retry transient 5xxs.
retryHTTPonly retries whenDiscoverFeedURLreturns an error, butinternal/rss/rss.gocurrently maps every non-2xx discovery response to("", nil). A homepage that briefly returns 500 is therefore treated as “no feed found” on the first attempt, so blogs that rely on auto-discovery can still fall through to scraping orSource == "none"without any retry. Please surface retryable discovery statuses as errors before this wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scanner/scanner.go` around lines 61 - 69, The DiscoverFeedURL call is being wrapped by retryHTTP but non-2xx HTTP discovery responses are currently returned as ("", nil) in internal/rss/rss.go so retryHTTP never retries transient 5xxs; change DiscoverFeedURL (or the response handling in internal/rss/rss.go) to return a retryable error for transient HTTP statuses (e.g., 5xx and other retryable codes) instead of ("", nil) so that retryHTTP and the call site using s.fetcher.DiscoverFeedURL will retry; ensure the returned error type/message makes it clear it’s a transient HTTP status while preserving the existing "no feed found" behavior for true 2xx/404 cases.internal/cli/commands.go (1)
189-223:⚠️ Potential issue | 🟠 Major
--silenthides partial and total scan failures.With the new exit-0 partial-failure flow, this is now the only path that surfaces
result.Error. In--silent, even an all-failed scan ends asscan donewith no error output, so automation has no signal that anything went wrong. Please emit at least a failure summary tostderrin silent mode, and consider returning non-zero whenfailed == len(results).As per coding guidelines, "Never ignore errors. For production errors, log them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands.go` around lines 189 - 223, The silent path currently swallows all scan failures; after calling sc.ScanAllBlogs and iterating results, when silent is true still compute failed and totalNew from results (as done in the non-silent branch) and emit a concise failure summary to stderr using fmt.Fprintln(os.Stderr, ...) (referencing results, result.Error and totalNew), and if failed == len(results) return a non-zero error (e.g. return fmt.Errorf("scan failed: %d/%d blogs failed", failed, len(results))) so automation can detect total failure; keep the existing "scan done" message only for fully-successful or partial-success cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/scanner/scanner.go`:
- Around line 171-175: The code currently converts every error from s.ScanBlog
into a per-blog ScanResult error, but storage failures and context cancellations
from calls like db.UpdateBlog, db.GetExistingArticleURLs, db.AddArticlesBulk,
and db.UpdateBlogLastScanned should abort the overall scan instead of being
folded into result.Error; change the error handling where s.ScanBlog is called
(the blocks around the current result, scanErr handling) to detect and propagate
fatal errors—specifically treat context.Canceled and context.DeadlineExceeded
and any DB/storage-specific error types/markers as non-recoverable by returning
the error (or re-raising it) rather than setting result.Error, while continuing
to fold only fetch/feed/scrape-related errors into result.Error and
result.BlogName.
---
Outside diff comments:
In `@internal/cli/commands.go`:
- Around line 189-223: The silent path currently swallows all scan failures;
after calling sc.ScanAllBlogs and iterating results, when silent is true still
compute failed and totalNew from results (as done in the non-silent branch) and
emit a concise failure summary to stderr using fmt.Fprintln(os.Stderr, ...)
(referencing results, result.Error and totalNew), and if failed == len(results)
return a non-zero error (e.g. return fmt.Errorf("scan failed: %d/%d blogs
failed", failed, len(results))) so automation can detect total failure; keep the
existing "scan done" message only for fully-successful or partial-success cases.
In `@internal/scanner/scanner.go`:
- Around line 61-69: The DiscoverFeedURL call is being wrapped by retryHTTP but
non-2xx HTTP discovery responses are currently returned as ("", nil) in
internal/rss/rss.go so retryHTTP never retries transient 5xxs; change
DiscoverFeedURL (or the response handling in internal/rss/rss.go) to return a
retryable error for transient HTTP statuses (e.g., 5xx and other retryable
codes) instead of ("", nil) so that retryHTTP and the call site using
s.fetcher.DiscoverFeedURL will retry; ensure the returned error type/message
makes it clear it’s a transient HTTP status while preserving the existing "no
feed found" behavior for true 2xx/404 cases.
🪄 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: 67fe71f9-c6ca-4350-9e1f-0c6c4e30f0c4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
e2e/e2e_test.gogo.modinternal/cli/commands.gointernal/scanner/scanner.gointernal/scanner/scanner_test.go
…discovery - DiscoverFeedURL now returns a retryable error for 5xx responses instead of silently treating them as "no feed found" - ScanAllBlogs propagates context cancellation and DB errors as fatal instead of folding them into per-blog result.Error - Silent scan mode reports failures to stderr and exits non-zero on total failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@internal/rss/rss.go`:
- Around line 99-104: DiscoverFeedURL currently only returns FeedParseError for
5xx responses; expand this so transient failures (HTTP 408, 429 and any errors
from client.Do or reading the body) also return a FeedParseError that wraps the
underlying error so callers using rss.IsFeedError see them as fatal; modify
DiscoverFeedURL to return FeedParseError for response.StatusCode >=500 OR
response.StatusCode == 408 || response.StatusCode == 429, and when client.Do or
ioutil.ReadAll (or equivalent) returns an error wrap that error in
FeedParseError (preserving the original error via fmt.Errorf("%w", err) or
similar) so timeouts/cancellations propagate correctly.
🪄 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: 185fcde4-4619-469f-8ea0-214a960a413a
📒 Files selected for processing (6)
e2e/e2e_test.gointernal/cli/commands.gointernal/rss/rss.gointernal/rss/rss_test.gointernal/scanner/scanner.gointernal/scanner/scanner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/scanner/scanner_test.go
- internal/cli/commands.go
Comment on lines
+99
to
104
| if response.StatusCode >= 500 { | ||
| return "", FeedParseError{Message: fmt.Sprintf("discover feed: server error status %d", response.StatusCode)} | ||
| } | ||
| if response.StatusCode < 200 || response.StatusCode >= 300 { | ||
| // Not-found / bad status is not an error — just means no feed at this URL. | ||
| // Client errors (4xx) are not transient — just means no feed at this URL. | ||
| return "", nil |
There was a problem hiding this comment.
Broaden retryable discovery failures beyond 5xx.
Only the 5xx path becomes a typed FeedParseError here. DiscoverFeedURL still returns plain wrapped errors for client.Do / body-read failures elsewhere in this function, and 408/429 still fall through to ("", nil). Because internal/scanner/scanner.go Lines 45-56 only folds rss.IsFeedError into ScanResult.Error, a single blog without a stored FeedURL can still abort scan all or be silently treated as “no feed found.” Please surface transient discovery failures through a typed error that preserves the underlying cause so cancellation/timeouts remain fatal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/rss/rss.go` around lines 99 - 104, DiscoverFeedURL currently only
returns FeedParseError for 5xx responses; expand this so transient failures
(HTTP 408, 429 and any errors from client.Do or reading the body) also return a
FeedParseError that wraps the underlying error so callers using rss.IsFeedError
see them as fatal; modify DiscoverFeedURL to return FeedParseError for
response.StatusCode >=500 OR response.StatusCode == 408 || response.StatusCode
== 429, and when client.Do or ioutil.ReadAll (or equivalent) returns an error
wrap that error in FeedParseError (preserving the original error via
fmt.Errorf("%w", err) or similar) so timeouts/cancellations propagate correctly.
# Conflicts: # internal/cli/commands.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cenkalti/backoff/v5ScanAllBlogscollects per-blog errors intoScanResult.Errorinstead of failing fast — successful scans are no longer lost when one blog is temporarily unreachableTest plan
TestScanPartialFailurewith golden file🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests