Skip to content

cold-store-reaper: retry S3 requests on transport failures - #972

Merged
dezren39 merged 2 commits into
mainfrom
cold-store-s3-retry
Oct 26, 2026
Merged

dezren39 merged 2 commits into
mainfrom
cold-store-s3-retry

Conversation

@dezren39

@dezren39 dezren39 commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Problem

Live-diagnosed 2026-10-26: through this sandbox's egress proxy, urllib responses from R2 S3 are intermittently truncated mid-body (~1 in 3 ListObjectsV2 trials failed with http.client.IncompleteRead at exactly 1191 of 3710 bytes; occasionally RemoteDisconnected). The traceback escaped http_request's resp.read() through _s3_request, killing backup-supabase and evict-scan at the LIST step; GET objects could also be hit before their byte-count retry ever saw a body.

Fix (S3 layer only)

_s3_request now retries the whole request up to 4 total attempts on:

  • http.client.IncompleteRead
  • http.client.RemoteDisconnected
  • http.client.BadStatusLine
  • ConnectionResetError
  • ColdStoreError — the form a reset takes when urllib wraps a send-phase failure in URLError and http_request converts it (the retry's try covers only the http_request call, so no other ColdStoreError can be caught)

Backoff: self.s3_backoff_seconds × attempt (default 0.5s → 0.5/1.0/1.5s), capped at 5s; the value is an injectable instance attribute and the test base zeroes it. Each attempt is re-signed (SigV4 timestamps go stale). LIST/GET/DELETE are idempotent, so blind retry is safe; no retries were added to http_request or any Drive path. If all attempts fail, a ColdStoreError names the operation and bucket (S3 GET supabase-backups failed after 4 attempts: transport error (IncompleteRead: ...)) instead of a raw traceback. s3_get_object's byte-count verification and its own retries are unchanged and compose on top.

Tests

44/44 pass (1 pre-existing skip), including 4 new regression tests in TestS3TransportRetry against an extended fake S3 (list-failure injection: truncate-mid-body / drop-connection, plus an attempt counter):

  • LIST truncated once → retries, returns all objects across pagination (4 attempts total: 1 failed + 3 pages)
  • LIST dropped once (RemoteDisconnected) → retries and succeeds (2 attempts)
  • LIST truncated permanently → named ColdStoreError after exactly 4 attempts
  • DELETE transport failure → retries, lands exactly once

Existing backup-supabase and evict-scan happy paths pass unchanged.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Summary by CodeRabbit

  • Bug Fixes
    • S3 operations now retry after temporary connection failures or incomplete responses, helping transient network issues resolve without immediately interrupting the operation.
    • Retries use increasing delays and are limited to four attempts. If an operation still fails, the resulting error identifies the affected S3 operation and bucket.

Muse Sweep added 2 commits October 26, 2026 14:22
Through the sandbox egress proxy, R2 S3 responses are intermittently
truncated mid-body (IncompleteRead at a fixed offset) or dropped
(RemoteDisconnected). http_request surfaces those raw, and they
escaped _s3_request as tracebacks that killed backup-supabase and
evict-scan at the LIST step.

_s3_request now retries the whole request up to S3_TRANSPORT_ATTEMPTS
(4) times on IncompleteRead, RemoteDisconnected, BadStatusLine,
ConnectionResetError, and ColdStoreError (the URLError form a reset
takes during the send phase), with a capped linear backoff
(self.s3_backoff_seconds x attempt, injectable, zeroed in tests).
Requests are re-signed per attempt. LIST/GET/DELETE are idempotent;
the retry lives only in the S3 layer, http_request callers elsewhere
are untouched. Exhaustion raises a ColdStoreError naming the
operation and bucket instead of a raw traceback. s3_get_object's
byte-count verification + retries compose on top unchanged.
FakeS3 gains list-failure injection (truncate = full Content-Length,
half a body, connection closed; drop = no response at all) plus a
list_attempts counter. New TestS3TransportRetry: truncated listing
retries and returns all objects across pagination, dropped listing
retries, permanently-truncating listing raises the named
ColdStoreError after exactly 4 attempts, DELETE retries a transport
failure and lands exactly once. S3TestBase zeroes the retry backoff.
@dezren39
dezren39 requested a review from a team as a code owner October 26, 2026 14:22
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T14:24:01.302781Z 500b345 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Walkthrough

The S3 request path now retries selected transport failures up to four times, with per-attempt signing and configurable backoff. Tests cover retries for LIST and DELETE requests and error reporting after repeated failures.

Changes: S3 Transport Retries

Layer / File(s) Summary
Configure and apply S3 request retries
pkgs/cold-store-reaper/cold_store.py
_s3_request retries configured transport exceptions and ColdStoreError up to four times. It re-signs each attempt, applies linearly increasing backoff capped at five seconds, and raises a final ColdStoreError with operation and bucket details.
Exercise transport retries
pkgs/cold-store-reaper/tests/test_cold_store.py
The fake S3 server can simulate dropped and truncated LIST responses. Tests verify LIST and DELETE retries and confirm that repeated truncation raises an error after four attempts.

Priority: Not assessed

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Merge Risk: Low · up to 500b3

S3 transport recovery improves, but persistent GET failures can now delay backups substantially longer. Mergeable with explicit acceptance of that bounded delay; consolidating GET retries and extending DELETE response-loss coverage are recommended.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • 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.

@dezren39
dezren39 merged commit 036a6dd into main Oct 26, 2026
3 of 4 checks passed
@dezren39
dezren39 deleted the cold-store-s3-retry branch October 26, 2026 14:29
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