fix(dr): retry transient R2 5xx on backup S3 client - #1090
Conversation
Nightly exporter ticks were aborting on a single R2 PutObject HTTP 500 while persisting progress.json. Retry 429/5xx and transport blips with backoff so conditional progress writes can complete within the tick. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe DR S3 client now retries transient HTTP and transport failures with configurable exponential backoff. HEAD, GET, and PUT operations use the retry path. Tests cover status classification, retries, exhausted failures, response data, conditional headers, and configuration parsing. ChangesDR S3 retry handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DRBackupS3Client
participant SignedFetch
participant S3R2
DRBackupS3Client->>SignedFetch: Send signed HEAD, GET, or PUT request
SignedFetch->>S3R2: Execute request
S3R2-->>SignedFetch: Return transient response or transport failure
SignedFetch->>SignedFetch: Drain response and wait with exponential backoff
SignedFetch->>S3R2: Retry request
S3R2-->>SignedFetch: Return final response
SignedFetch-->>DRBackupS3Client: Return response or throw after exhaustion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![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
🧹 Nitpick comments (1)
packages/worker/src/dr/backup-s3.node.test.ts (1)
90-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
getBytesretry test.
getBytesnow usessignedFetchWithRetry, but this test covers onlyheadandgetText. Add a transient-response case that asserts the retry count and the returned byte sequence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/dr/backup-s3.node.test.ts` around lines 90 - 118, Add a getBytes retry scenario alongside the existing head and getText cases, using a mocked transient 503 followed by a successful response; assert the fetch mock is called twice and verify getBytes returns the expected byte sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/dr/backup-s3.ts`:
- Around line 105-108: Namespace every backup-s3 read and write operation by a
validated userId before signing requests: update the client or operation
boundary around signedFetch and its HEAD, GET, and PUT callers to derive
user-scoped object keys rather than signing the raw key. In
packages/worker/src/dr/backup-s3.ts lines 105-108, apply this requirement to
signedFetch and all paths using it; in
packages/worker/src/dr/backup-s3.node.test.ts lines 9-14, update the fixture and
assert distinct user IDs generate isolated object keys.
---
Nitpick comments:
In `@packages/worker/src/dr/backup-s3.node.test.ts`:
- Around line 90-118: Add a getBytes retry scenario alongside the existing head
and getText cases, using a mocked transient 503 followed by a successful
response; assert the fetch mock is called twice and verify getBytes returns the
expected byte sequence.
🪄 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 Plus
Run ID: 492a8303-b081-4cf2-8454-6a32a4e72956
📒 Files selected for processing (2)
packages/worker/src/dr/backup-s3.node.test.tspackages/worker/src/dr/backup-s3.ts
| async function signedFetch(key: string, init?: RequestInit) { | ||
| const request = await aws.sign(objectUrl(config, key), init) | ||
| return fetchImpl(request) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Namespace DR backup object access by userId.
The client signs the caller-provided key directly, and its API has no userId. HEAD, GET, and PUT therefore access one shared bucket keyspace without tenant isolation.
packages/worker/src/dr/backup-s3.ts#L105-L108: require a validateduserIdat the client or operation boundary and derive the object key from that namespace before signing.packages/worker/src/dr/backup-s3.node.test.ts#L9-L14: update the fixture and add assertions that distinct user IDs produce isolated object keys.
As per coding guidelines, “every read and write path must be scoped by userId.”
📍 Affects 2 files
packages/worker/src/dr/backup-s3.ts#L105-L108(this comment)packages/worker/src/dr/backup-s3.node.test.ts#L9-L14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/dr/backup-s3.ts` around lines 105 - 108, Namespace every
backup-s3 read and write operation by a validated userId before signing
requests: update the client or operation boundary around signedFetch and its
HEAD, GET, and PUT callers to derive user-scoped object keys rather than signing
the raw key. In packages/worker/src/dr/backup-s3.ts lines 105-108, apply this
requirement to signedFetch and all paths using it; in
packages/worker/src/dr/backup-s3.node.test.ts lines 9-14, update the fixture and
assert distinct user IDs generate isolated object keys.
Source: Coding guidelines
There was a problem hiding this comment.
Not applicable here: this client is the platform disaster-recovery backup path (backup-control-plane), which intentionally writes a shared staging/sealed backup keyspace for the whole deployment. Per-user isolation applies to user-owned storage/MCP paths, not this operator backup bucket. Adding userId namespacing would break the exporter/restore contract.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
|
🔎 Preview deployed: https://kody-pr-1090.kody-a99.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
3 participants
Summary
Sentry KODY-CLOUDFLARE issue 7643617296:
DR backup PUT failed for staging/…/exporter/progress.json: HTTP 500.In the failing production tick, two conditional
progress.jsonPUTs succeeded (HTTP 200), then the next PUT in the sameexportArtifactsPhasegot an R2 S3 API HTTP 500. That aborted the entiredr_exportscheduled lane. Progress already written is durable, and the next cron tick resumes — but the tick wastes its remaining budget and pages Sentry for a platform blip.Fix:
createDrBackupS3Clientnow retries transient HTTP statuses (429,500,502,503,504) and transport errors with exponential backoff (same shape as D1 lock retry).412precondition failures stay non-retryable so overlapping progress ownership still skips cleanly.Merge gate: touches
backup-control-plane/ disaster-recovery surface — leaving open for review; do not auto-merge.Test plan
npx vitest run packages/worker/src/dr/backup-s3.node.test.tsnpm run validateequivalent (Validate aggregate green on prior head; re-running after getBytes test)System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@6d0dc439· Head:66b13836Classification: extends — changes
backup-control-planeR2 S3 client behavior to retry transient platform failures before failing the nightly exporter tick.Primitives touched
backup-control-planeSystem map
Nightly
dr_exportcron persistsprogress.jsonvia the signed R2 S3 client; transient R2 500s are retried inside that client instead of failing the scheduled lane.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context.
Before / after
Before: a single R2 PutObject HTTP 500 during
persistProgressthrew, aborted thedr_exportlane, and opened a Sentry issue even when prior puts in the same tick succeeded.After: the S3 client retries 429/5xx and transport errors with short backoff; exhausted failures still throw (and still alert). Conditional
412conflicts remain immediate non-retries.Risk
Medium — disaster-recovery write path. Retries are idempotent for conditional puts (phantom success → next attempt
412→ existing skip path). Left open for human review; not auto-merged.Test gaps
No live R2 integration test; coverage is unit-level with an injected
fetchstub (HEAD/GET text/GET bytes/PUT, plus non-retry of 412).Summary by CodeRabbit
Bug Fixes
Tests