Skip to content

Apply configured store retry budget - #90

Open
behinddwalls wants to merge 1 commit into
tobi:mainfrom
behinddwalls:preetam/issue-82-store-retries
Open

behinddwalls wants to merge 1 commit into
tobi:mainfrom
behinddwalls:preetam/issue-82-store-retries

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Sep 30, 2026 •

Copy link
Copy Markdown

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 #82

Stack

  1. Apply configured store retry budget #90 — Apply configured store retry budget

@0bserver07

Copy link
Copy Markdown
Contributor

Careful with this one. The SDK retry setting applies to every call, including the manifest CAS. If a CAS lands but its reply gets lost, the SDK retries, gets a 412 from our own write, and publish.rs takes that as a lost race and deletes the log segment the new manifest points at. I reproduced it with a fault that applies the write and then answers 412, and main already hits this with the SDK default of 3 attempts, so more retries make it more likely.

Could the retries stay on reads, with conditional writes and deletes at one attempt? I opened #103 for the publisher side.

## 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
@behinddwalls
behinddwalls force-pushed the preetam/issue-82-store-retries branch from a46bb4b to 34f240d Compare October 1, 2026 20:18
@behinddwalls

behinddwalls commented Oct 1, 2026 •

Copy link
Copy Markdown
Author

Restricted configured retries to idempotent reads in 34f240d. GCS mutation clients and the S3 mutation client are single-attempt, while metadata reads and interrupted bulk reads use the configured budget; conditional writes, uploads, multipart operations, and deletes are never retried by the backend SDK. Store tests, backend contracts, healthy round-trip simulation, and clippy pass; #103 remains complementary publisher hardening.

[addressed by agent]

@0bserver07

Copy link
Copy Markdown
Contributor

Thanks, this fixes the risky part, conditional writes aren't retried anymore. I think it went a bit further than it needs to though. On GCS everything is single-attempt now, including normal reads and large uploads, and on S3 the multipart part uploads too. Those retries were safe and worth keeping, it's only the conditional writes that need to be one try. Small one: the comment at s3.rs:313 still says the SDK retries three times.

This branch has not been deployed

No deployments
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.

store.max_retries configuration is ignored

2 participants