Skip to content

Retry transient GitHub REST read failures - #82

Merged
dbachko merged 2 commits into
mainfrom
fix/github-read-retries
Apr 2, 2026
Merged

dbachko merged 2 commits into
mainfrom
fix/github-read-retries

Conversation

@dbachko

@dbachko dbachko commented Apr 1, 2026 •

Copy link
Copy Markdown
Owner

Context

GitHub REST GET calls can fail on transient transport truncation and currently abort otherwise healthy reads instead of retrying the idempotent request.

TL;DR

Retry transient GitHub REST GET failures so truncated responses do not break issue reads.

Summary

  • Retry GitHub REST GET requests on transient send and JSON decode failures.
  • Keep non-GET requests single-shot and preserve existing rate-limit handling.
  • Add a regression test that simulates a truncated response and succeeds on retry.

Alternatives

  • Rely on callers to retry failed reads, which spreads transport recovery across layers.

Test Plan

  • cargo fmt --check
  • cargo test
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test rest_json_retries_truncated_get_responses

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced GitHub API request reliability: automatic retries for transient send/decode errors (up to 3 attempts) with exponential backoff, preserving existing non-2xx and rate-limit responses.
  • Tests
    • Added tests verifying retry behavior against truncated/partial HTTP responses to ensure successful JSON retrieval on subsequent attempts.

Summary:
- retry GitHub REST GET reads when request send or JSON body decode
  fails with transient transport errors
- keep non-GET requests single-shot and preserve existing rate-limit
  handling
- add a regression test that simulates a truncated JSON response and
  verifies the second GET succeeds

Rationale:
- transient connection resets and truncated bodies can fail otherwise
  healthy GitHub reads
- limiting retries to idempotent GET requests reduces recovery noise
  without risking duplicate writes

Tests:
- cargo test rest_json_retries_truncated_get_responses
@coderabbitai

coderabbitai Bot commented Apr 1, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 02e5c4ab-acf8-4652-9732-e12794d08a84

📥 Commits

Reviewing files that changed from the base of the PR and between bdb550b and c3af76e.

📒 Files selected for processing (1)
  • rust/src/github.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • rust/src/github.rs

📝 Walkthrough
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding retry logic for transient GitHub REST read failures.
Description check ✅ Passed The description follows the template structure with all required sections (Context, TL;DR, Summary, Alternatives, Test Plan) completed with substantive details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/github-read-retries

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@rust/src/github.rs`:
- Around line 1967-2016: The server thread currently blocks forever on
listener.accept() calls; add a deadline inside the spawned closure so the second
accept (and thus server.join()) will time out instead of hanging. Inside the
thread closure that currently calls listener.accept().unwrap(), create an
Instant deadline (e.g. now + Duration::from_secs(N)) and replace each direct
accept() with a non-blocking loop (listener.set_nonblocking(true) or try_accept
loop) that repeatedly attempts accept(), sleeps briefly, and returns/errs when
Instant::now() > deadline; ensure the closure returns cleanly on timeout so
server.join() won't block. Update the closure around the two accept points (the
thread spawned into server) so it uses this deadline behavior.
- Around line 1495-1530: should_retry_github_read_error currently only treats
reqwest transport failures and a set of string needles as retryable; extend it
to also treat truncated-JSON decode errors as retryable by inspecting the error
chain for serde_json::Error (or a reqwest::Error that wraps a serde_json::Error)
and checking its classification/message for EOF/truncated JSON (e.g.,
serde_json::error::Category::Eof or messages like "EOF while parsing" /
"unexpected end of file"); update should_retry_github_read_error to return true
when such a serde_json EOF decode is found (in addition to the existing reqwest
checks) and add this check alongside retryable_github_read_error_text so JSON
decode EOFs are retried.
🪄 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: de6bbe4a-c038-44df-899f-ce8fc0206c93

📥 Commits

Reviewing files that changed from the base of the PR and between d9354b3 and bdb550b.

📒 Files selected for processing (1)
  • rust/src/github.rs

Comment thread rust/src/github.rs
Comment thread rust/src/github.rs
Summary:
- retry GitHub REST GET reads when truncated JSON decode errors surface
  as serde_json EOF failures
- harden the raw-socket retry regression test with a bounded accept
  deadline so CI fails cleanly instead of hanging
- add coverage for both transport truncation and JSON EOF decode retry
  paths

Rationale:
- review feedback identified a decode path that could still fail before
  retrying a read
- deadline-bounded test sockets keep retry regressions observable without
  risking stuck test jobs

Tests:
- cargo test rest_json_retries_truncated_get_responses
- cargo test rest_json_retries_truncated_json_decode_errors
- cargo fmt --check
- cargo clippy --all-targets --all-features -- -D warnings
@dbachko
dbachko merged commit 8e86e8f into main Apr 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant