Skip to content

fix(store): apply the configured retry budget to reads only - #45

Merged
yordis merged 3 commits into
mainfrom
yordis/fix-store-apply-retry-budget
Oct 29, 2026
Merged

fix(store): apply the configured retry budget to reads only#45
yordis merged 3 commits into
mainfrom
yordis/fix-store-apply-retry-budget

Conversation

@yordis

@yordis yordis commented Oct 29, 2026

Copy link
Copy Markdown
Member
  • SDK retries on conditional writes could turn a manifest CAS that landed into a 412, so the publisher treated a committed push as a lost race.
  • store.max_retries was ignored.
  • Brings in Apply configured store retry budget tobi/walgit#90 by behinddwalls, cherry-picked with original authorship; the upstream test fails on our main and passes with this change.

behinddwalls and others added 2 commits October 28, 2026 22:38
@behinddwalls @yordis
Apply configured store retry budget
db07568 
## Summary

### Why?

Backend SDK retries covered conditional mutations as well as reads. If a manifest CAS landed but its response was lost, an SDK retry could receive 412 from the already-committed write and make the publisher treat success as a lost race.

### What?

Apply `store.max_retries` only to idempotent GCS and S3 reads and interrupted bulk reads. Configure GCS mutation clients and the S3 mutation client for one attempt, while S3 metadata reads use a separate retry-enabled client and presigned GETs retain explicit bounded retries.

Healthy calls remain one request. Failure paths add at most `store.max_retries` read attempts; conditional writes, uploads, copies, multipart operations, and deletes remain single-attempt.

## Test Plan

✅ `cargo check -p walgit-store --all-features`

✅ `cargo test -p walgit-store --all-features`

✅ `cargo test -p walgit-server --test sim healthy_request_round_trip_budgets -- --exact`

✅ `cargo clippy -p walgit-store --all-targets --all-features -- -D warnings`

✅ `cargo fmt --all -- --check`

✅ `git diff --check`

## Issue

Closes tobi#82

(cherry picked from commit 34f240d)
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis
test(store): adapt retry-budget tests to current S3Store fields
8fd1cb7 
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@cursor

cursor Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes retry behavior on all GCS/S3 read paths and removes SDK retries on writes, which fixes CAS safety but could alter failure/retry timing in production object-store traffic.

Overview
store.max_retries now drives walgit-owned retries on idempotent reads (GCS metadata/list, object body reads, bulk HTTPS resume, S3 head/list and presigned GET), with jittered backoff. Writes, deletes, and compose stay single-attempt: GCS/S3 SDK automatic retries are disabled on mutation clients so a manifest CAS that already landed cannot be retried into a spurious 412.

GCS replaces hard-coded read/resume caps with store.max_retries, extends the deadline wrapper to retry transient RPC/HTTP errors (not only timeouts), and applies read-side retry policy only on read_object. S3 splits client (no SDK retries) from read_client (standard mode capped by config). Config comments, walgit.example.toml, and docs/ROUNDTRIPS.md document that healthy paths stay one attempt and only failure paths may add up to max_retries extra read tries.

Reviewed by Cursor Bugbot for commit cd0ab9d. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b2ad31aa-e213-41c1-8b99-fa6dd4013ece
Commits

Reviewing files that changed from the base of the PR and between 2668870 and cd0ab9d.

Files selected for processing: 5
  • crates/walgit-config/src/lib.rs
  • crates/walgit-store/src/gcs.rs
  • crates/walgit-store/src/s3.rs
  • docs/ROUNDTRIPS.md
  • walgit.example.toml
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

cursor[bot]
cursor Bot reviewed Oct 29, 2026
View reviewed changes

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8fd1cb7. Configure here.

Comment thread crates/walgit-store/src/gcs.rs
@yordis
fix(store): apply the read retry budget to GCS body fetches
cd0ab9d 
A conditional GET that misses fetches the body through a client that never
retries, so a transient error on manifest.pb failed the read instead of
spending store.max_retries.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis closed this Oct 29, 2026
@yordis yordis reopened this Oct 29, 2026
@yordis yordis closed this Oct 29, 2026
@yordis yordis reopened this Oct 29, 2026
@yordis
yordis merged commit 483817f into main Oct 29, 2026
10 of 15 checks passed
@yordis
yordis deleted the yordis/fix-store-apply-retry-budget branch October 29, 2026 05:42
@yordis yordis mentioned this pull request Oct 29, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yordis @behinddwalls