Repository navigation
Conversation
The R2 suite hits a live Cloudflare endpoint with ~130 concurrent
requests. When R2 briefly returns 503 ServiceUnavailable or
InternalError (the error body points at cloudflarestatus.com),
whichever handful of tests are in flight go red and the lane fails.
MinIO (docker) runs the same suite without the external dependency and
stays at retry=0, so a real S3 regression still fails there. For R2
only, give each test {retry: 3} via a scoped `it` shadow so a brief
outage does not fail the lane.
|
Warning Review limit reached
Next review available in: 3 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 6:30 AM PT - Jul 26th, 2026
❌ @robobun, your commit 269d3fd has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35893That installs a local version of the PR into your bun-35893 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Status: closed, not merged. The same idea for the current version of the file is in #46671. The description above and the earlier text of this comment have errors. The corrections are in #35893 (comment). What was checked in July:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only deflake scoped to the R2 suite.
What was reviewed:
itForServicewrapper: number→{timeout, retry:3}conversion,{retry:3, ...opts}spread order, and recursiveskipIfre-wrap all check out.- Confirmed the two shadowed scopes only use plain
it(...)andit.skipIf(...); the soleit.concurrent(line 1502) and the module-tail describes use the unshadowedconst it = bunIt. - MinIO runs the same test bodies with
retry=0, so a real S3 regression still fails deterministically — the retry cannot mask a Bun bug.
Extended reasoning...
Overview
Test-only change to test/js/bun/s3/s3.test.ts. Adds an itForService(service) helper that, for service === "R2" only, returns a shadowed it which injects {retry: 3} into each test's options (preserving any numeric timeout as {timeout, retry: 3} and re-wrapping .skipIf). The wrapper is applied via const it = itForService(...) inside the two R2-backed describe blocks; every other backend and the rest of the file use the original bunIt unchanged. No src/ changes.
Security risks
None. This only alters test-runner retry behaviour for a CI-only suite that talks to a live Cloudflare R2 endpoint. No credentials, parsing, or runtime code paths are touched.
Level of scrutiny
Low. This is a targeted CI deflake for a documented external-service flake (R2 returning 503 ServiceUnavailable pointing at cloudflarestatus.com, seen across seven unrelated builds). REVIEW.md's "never silently weaken a test" concern is addressed head-on: the identical test bodies run against local MinIO with retry=0, so any real S3 regression still fails there on the first attempt. Each retry re-runs the full test body with a fresh randomUUID() key, so attempts are independent.
Other factors
- Verified
TestOptionsinpackages/bun-types/test.d.tsexportstimeout/retry, matching the wrapper's shape. - Audited every
it.*inside the shadowed scopes: onlyit()andit.skipIf()are used (both handled). The oneit.concurrentin the file is in the standaloneminiodescribe, outside the shadow. The wrapper intentionally omits.each/.todo/.onlysince none are used in-scope. {retry: 3, ...opts}spread order lets a future explicitretryoverride the default, and preserves any object-formtimeout.- The robobun status comment confirms 294 tests register both before and after, and that the wrapper's retry behaviour was verified in a standalone harness.
4 x 100_000ms would exceed the 180s per-file CI wall in scripts/runner.node.mjs and SIGTERM the whole file. The 503 flake was observed on the small/fast requests; the slow-transfer timeouts are handled separately.
There was a problem hiding this comment.
LGTM — test-only deflake scoped to R2.
What was reviewed:
itForServicereturns the realitfor MinIO, so the local-docker suite keepsretry=0and would still catch a real S3 regression on first attempt.- Checked all
it.*usages inside the two shadowed blocks — onlyit(...)andit.skipIf(...)are used there; the oneit.concurrentis in the un-shadowed minio block. >30_000guard keeps the 50s/100s large-transfer tests at retry=0 so worst-case wall time stays under the 180s per-file CI cap; the 20s/30s tests that do get retry cap at ~120s.
Extended reasoning...
Overview
Single-file, test-only change to test/js/bun/s3/s3.test.ts. Renames the it import to bunIt, re-exports const it = bunIt at module scope so nothing outside the R2 blocks changes, and adds an itForService(service) helper. Inside the two R2-backed describe blocks, it is shadowed with a wrapper that injects {retry: 3} into the third argument. Numeric timeouts >30s are passed through unchanged (retry=0) to avoid 4×100s exceeding the 180s per-file CI wall; numeric timeouts ≤30s become {timeout, retry: 3}; undefined/object opts get {retry: 3, ...opts}. .skipIf is re-wrapped recursively so the two it.skipIf call sites inside the loop keep working.
Security risks
None. No runtime code touched; the change only affects how many times bun:test re-runs a failed R2 test body.
Level of scrutiny
Low. This is a CI deflake with no src/ changes and no assertion changes. The key REVIEW.md concern for deflakes — "keep asserting the property the original assertion protected" — is satisfied: assertions are untouched, and the identical test bodies still run against local MinIO with retry=0, so a genuine Bun-side regression fails there immediately. Retry re-runs the whole test body with a fresh randomUUID() key, so attempts are independent.
Other factors
- Verified the wrapper's coverage: inside the shadowed scopes only bare
it(...)andit.skipIf(...)(...)are used. The loneit.concurrent(line ~1510) is in the separatedescribe.skipIf(!minioCredentials)("minio", ...)block which uses the un-shadowed module-levelit = bunIt. Otherit.*variants (.todo,.only,.each) are not used in the shadowed scopes, so the partial proxy is sufficient. {retry: 3, ...undefined}correctly yields{retry: 3}for the common no-opts case.- The author already self-caught and fixed the CI-wall-clock interaction in 2d827a2, and confirmed the file registers the same 294 tests before/after.
- Prior build 82252 had
s3.test.tsgreen on every completed lane.
The S3 client reads HTTP_PROXY from the environment without a target host, so NO_PROXY never exempts the localhost endpoint and the 'multipart upload id validation' test fails wherever a proxy is configured. Same fix as in #35057.
There was a problem hiding this comment.
LGTM — test-only deflake that scopes {retry: 3} to the R2-backed it calls while leaving MinIO at retry=0 as the regression backstop.
What was reviewed:
itForServicewrapper: verified onlyit(...)andit.skipIf(...)are used inside the two shadowed describe blocks; the loneit.concurrent(line 1510) is in the un-shadowedminioblock and uses module-scopeit = bunIt.withRetrynumeric handling: 15_000/20_000/30_000 timeouts get{timeout, retry:3}(worst case 4×30s = 120s < 180s CI wall); 50_000/100_000 stay retry=0 as intended.TestOptions/retryexist inpackages/bun-types/test.d.ts; spreadingundefinedin{retry: 3, ...opts}is safe.- HTTP_PROXY strip follows the
{...bunEnv, KEY: undefined}convention andbunEnvdoes not already clear proxy vars.
Extended reasoning...
Overview
Test-only deflake in test/js/bun/s3/s3.test.ts. Imports it as bunIt and TestOptions from bun:test, adds an itForService(service) helper that returns the real it for non-R2 services and a thin wrapper for R2 that injects {retry: 3} into the third argument (preserving numeric timeouts as {timeout, retry: 3}, and passing through numeric timeouts >30s unchanged so 4× a 100s attempt cannot exceed the 180s per-file CI wall). The wrapper re-implements .skipIf recursively so the two it.skipIf call sites inside the wrapped blocks keep working. The two R2-backed describe blocks ("Virtual Hosted-Style" and the per-service loop) shadow it with the wrapped version; every other block uses the module-level const it = bunIt unchanged. Separately, the mock-server subprocess test now strips HTTP_PROXY/HTTPS_PROXY from the spawned env so ambient proxy config cannot redirect the localhost S3 client.
Security risks
None. No src/ changes; the only behavioural change is how many times bun:test re-runs a failed R2 test body and which env keys are passed to a test subprocess.
Level of scrutiny
Low-to-medium. This is CI-reliability plumbing in a test file, not runtime code. The main risk with a blanket retry is masking real regressions — the PR addresses that directly: the identical test bodies run against local MinIO with retry=0, so a Bun-side S3 bug still fails deterministically there. The other risk (retry × long-timeout blowing past the per-file CI wall) was self-caught and fixed with the > 30_000 guard. I checked the numbers: the largest timeout that still gets retry is 30_000, and 4×30s = 120s < 180s.
Other factors
- Verified the wrapper's incomplete surface (
.skipIfonly, no.each/.only/.concurrent) is sufficient: the only non-plainit.*inside the wrapped scopes are the twoit.skipIfcalls; the soleit.concurrentin the file is in the un-wrappedminiodescribe. {retry: 3, ...opts}ordering lets an explicitretryin a futureTestOptionsobject override the default, which is the right precedence.- Retried test bodies mint a fresh
randomUUID()key each attempt, so retries are independent. - The HTTP_PROXY strip uses the repo-standard
{...bunEnv, KEY: undefined}pattern andharness.tsdoes not already clear proxy vars, so it is not redundant. - Author reports the file registers the same 294 tests before/after and that build 82252 passed
s3.test.tson all lanes.
|
Closing this PR because it has been inactive for more than 90 days. |
|
This PR stays closed. The same idea for the current version of I checked the seven July builds again today. The description of this PR has errors. These are the corrections. Why the lanes were red
Other claims
|
Deflakes
test/js/bun/s3/s3.test.tsfor the case where Cloudflare R2 briefly returnsServiceUnavailable/InternalError.What happened
Seen across at least seven unrelated builds: 78962, 78968, 80830, 80971, 81977, 82230, 82231. In each case ~130 R2 tests run concurrently; one to four of them happen to be in flight during a brief R2 blip, fail on
ServiceUnavailable/InternalError/503, and the lane goes red on every retry because each retry hits a fresh blip somewhere in the batch.Bun's S3
retryoption only covers uploads and has no backoff; downloads /stat/deletedo not retry at all. That is documented behaviour and outside the scope of this deflake.Fix
Inside the two R2-backed
describeblocks (Virtual Hosted-Styleand the per-service loop whenservice === "R2"), shadowitwith a wrapper that passes{retry: 3}tobun:test. Any existing timeout number is preserved as{timeout, retry: 3};it.skipIfis re-wrapped so the twoskipIfcall sites keep working.Tests that pass a numeric timeout above 30 000 ms (the 50_000 / 100_000 ms large-transfer tests) keep
retry=0. Withretry: 3a 100 s per-attempt timeout could hold the file open for 400 s against the 180 s per-file wall inscripts/runner.node.mjsand SIGTERM the whole file. Those slow-transfer timeouts are covered by #35057; this PR targets the fast requests where 503 was actually observed.For every other backend (
MinIO),itForServicereturns the realitunchanged, so those tests keepretry=0.Why this is correct
retry. A real S3 regression would still fail there on the first attempt, so the retry cannot mask a Bun bug.randomUUID()object key, so attempts are independent and idempotent.No
src/changes. Related to but distinct from #35057, which addresses R2 large-upload timeouts rather than 503s.[stamp-90s] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file