fix(ocale): stop losing place uploads to keep-alive connection reuse - #531
Merged
Conversation
Roblox's edge gateway discards idle keep-alive connections faster than a pooling fetch expects. A publish written into a discarded connection never reaches Open Cloud: it surfaces as a gateway error page or a socket reset and creates no version. Reproduced on both Node and Bun; disabling reuse for the upload removes it. Extracts the body validation so the builder stays within the function-length limit. Refs #530 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A transport kill or an edge-gateway response cannot have created a place version, so neither carries the duplicate-write risk that keeps create methods off the retry path. Classify a gateway-origin ApiError by a synthetic GATEWAY_REJECTED transport code rather than its status, since that status belongs to the gateway and says nothing about the request, and give publish and save their own upload retry policy that recovers both. Refs #530 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Amends ADR-010 with the keep-alive reuse diagnosis and the two-part decision, and updates the package retry table with the "never reached Open Cloud" axis. Refs #530 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The keep-alive reuse race is a property of upload-shaped requests, not of place publishing, and the transport already classifies that population with isUploadRequest to drop the default timeout. Move the directive there so icon, thumbnail, and binary-input uploads get the same protection instead of place publish carrying a lone special case, and set it after request headers so a caller cannot silently re-enable pooling. Also derives UPLOAD_METHOD_DEFAULTS from CREATE_METHOD_DEFAULTS rather than restating its status list, corrects the retryableTransportCodes doc now that the list spans a synthetic non-errno code, and tightens two tests that could not fail for the reason they named. Refs #530 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughPlace publish and save uploads now disable HTTP connection reuse and retry transient failures that are classified as not reaching Open Cloud. Retry policy, request validation, client wiring, tests, and documentation were updated accordingly. ChangesPlace upload reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlacesClient
participant FetchClient
participant RobloxGateway
participant OpenCloud
PlacesClient->>FetchClient: Send publish/save upload
FetchClient->>RobloxGateway: Request with connection: close
RobloxGateway->>OpenCloud: Forward accepted request
OpenCloud-->>PlacesClient: Publish/save response
RobloxGateway-->>PlacesClient: Gateway rejection when request is not forwarded
PlacesClient->>FetchClient: Retry gateway or transport failure
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
claude
Bot
added
pkg:ocale
|
Claude finished @christopher-buss's task in 5m 16s —— View job Code ReviewOverall: looks correct and well-tested. One recurring policy violation (em-dashes) across four locations; everything else clears the rubric. Architecture (FCIS)The layering is correct throughout.
TestingTest coverage is thorough and idiomatic. All new tests use The integration tests cover:
The One observation (not blocking): there is no test confirming that Types
Commit / PR FormatPR title and all four commit subjects are valid scope-enum, lowercase, consumer-facing. The Em-Dashes (policy violation — 4 locations)CLAUDE.md: "No em-dashes ( All four locations are in content introduced by this PR: 1. 2. 3. 4. Note: the prior amendments in this file also contain em-dashes, but those are pre-existing and out of scope for this PR. SummaryThe fix is correct, well-evidenced (probe data in the ADR, integration tests exercising the retry paths), and the retry safety reasoning (killed publish creates no version; Roblox dedupes identical content) is sound. The only required change is replacing the em-dashes in four locations. |
coderabbitai
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
Fixes intermittent Open Cloud place upload failures caused by keep-alive connection reuse, by forcing upload requests to close connections and by retrying only failures that provably never reached Open Cloud.
Changes:
- Add
connection: closefor upload-shaped requests viaisUploadRequest, preventing reuse of stale pooled connections. - Introduce
UPLOAD_METHOD_DEFAULTSandGATEWAY_REJECTEDto retry gateway-servedApiErrors and transient transport failures for place uploads without enabling 5xx retries. - Add integration/unit tests plus documentation updates (CLAUDE.md + ADR-010 amendment) and a changeset.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/open-cloud/tests/integration/resources/places/client.spec.ts | Adds integration coverage for retrying gateway rejection and ECONNRESET on publish/save. |
| packages/open-cloud/src/resources/places/client.ts | Switches publish/save to UPLOAD_METHOD_DEFAULTS and documents the refined retry behavior. |
| packages/open-cloud/src/internal/http/retry.ts | Adds GATEWAY_REJECTED and UPLOAD_METHOD_DEFAULTS, and updates shouldRetry to treat gateway responses as transport-class retry. |
| packages/open-cloud/src/internal/http/retry.spec.ts | Adds unit tests for gateway retry classification and upload defaults. |
| packages/open-cloud/src/internal/http/fetch-client.ts | Applies connection: close for upload requests and refactors request header merging. |
| packages/open-cloud/src/internal/http/fetch-client.spec.ts | Tests the new upload connection behavior and header precedence. |
| packages/open-cloud/src/domains/universes/places/builders.ts | Refactors publish body validation into a helper for clarity while preserving behavior. |
| packages/open-cloud/CLAUDE.md | Documents the new "never reached Open Cloud" retry axis and the upload connection behavior. |
| docs/adr/010-sdk-managed-rate-limiting-and-retry.md | Adds an amendment describing the diagnosis and the new retry/connection tradeoff for uploads. |
| .changeset/hungry-donkeys-shake.md | Adds a patch changeset describing the fix for ocale consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 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/open-cloud/src/internal/http/fetch-client.ts`:
- Around line 12-26: Remove the internal implementation comments requested at
all affected sites: in packages/open-cloud/src/internal/http/fetch-client.ts
lines 12-26 remove the connection-reuse rationale, and lines 257-265 remove the
private helper JSDoc; in
packages/open-cloud/src/domains/universes/places/builders.ts lines 55-64 remove
the private helper JSDoc; in packages/open-cloud/src/internal/http/retry.spec.ts
lines 151-152 and 295-296 remove the explanatory test comments while leaving the
self-evident test setup and behavior unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6628fdc4-f6a1-4e37-84ef-e2854e025f93
📒 Files selected for processing (10)
.changeset/hungry-donkeys-shake.mddocs/adr/010-sdk-managed-rate-limiting-and-retry.mdpackages/open-cloud/CLAUDE.mdpackages/open-cloud/src/domains/universes/places/builders.tspackages/open-cloud/src/internal/http/fetch-client.spec.tspackages/open-cloud/src/internal/http/fetch-client.tspackages/open-cloud/src/internal/http/retry.spec.tspackages/open-cloud/src/internal/http/retry.tspackages/open-cloud/src/resources/places/client.tspackages/open-cloud/tests/integration/resources/places/client.spec.ts
Repo review standard forbids em-dashes in code comments, docstrings, commit messages, and PR bodies. Refs #530 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
christopher-buss
added a commit
that referenced
this pull request
Jul 31, 2026
## Summary A consumer's deploy lost all three of its places at once, two of them in the same millisecond. The codes were new: `ERR_HTTP2_STREAM_ERROR` and `UND_ERR_INFO`, where this failure has always surfaced as a socket error before. Node 26 bundles undici 8, which [enables HTTP/2 by default](nodejs/undici#4828) (taken into Node in [nodejs/node#62384](nodejs/node#62384)). That breaks the mitigation from #531 in two ways. `Connection` is a connection-specific header, forbidden in HTTP/2, so an h2 transport drops the `connection: close` uploads set — the directive was doing nothing. And h2 multiplexes, so every upload shares one session where HTTP/1.1 gave each its own: one session death now fails every place in flight, instead of one. Measured against a local server offering ALPN `["h2", "http/1.1"]`: | Client | ALPN | TCP connections for 2 uploads | `connection` on the wire | | --- | --- | --- | --- | | Node 26.5.1 `fetch` | h2 | 1 | absent (stripped) | | Node 24.18 `fetch` | http/1.1 | 2 | `close` | | Bun 1.3.14 `fetch` | http/1.1 | 2 | `close` | Bun implements `node:http2`, but its `fetch` does not offer h2 in ALPN, so Bun is unaffected. The exposure is Node 26 and later. The fix has two parts: 1. **Uploads pin HTTP/1.1.** There is no standard `fetch` option for this, and the documented route — `setGlobalDispatcher(new Agent({ allowH2: false }))` — means depending on undici, which ADR-008 rules out, and mutating a process-wide default, which a library should not do. Instead the transport reconstructs the class of the runtime's own global dispatcher with `allowH2: false`. No dependency, and it re-arms the directive #531 relies on. The symbol is undici's internal contract and moved from `.1` to `.2` in undici 8, so an absent symbol, a non-constructible value, or a throwing constructor each fall back to the runtime's default transport rather than failing the deploy. 2. **The h2 codes join `TRANSIENT_TRANSPORT_CODES`.** `ERR_HTTP2_STREAM_ERROR`, `ERR_HTTP2_SESSION_ERROR`, and `UND_ERR_INFO` are the h2 spellings of socket deaths already in that set. This is the safety net for any runtime where part 1's probe finds nothing. Part 2 corrects a claim #531 made. The transient set was described as failures that never reached Open Cloud. That is not true of the h2 codes — `UND_ERR_INFO` covers both a `GOAWAY` declaring a stream was never started and a stream that was fully sent — nor of `UND_ERR_SOCKET`, which can fire once a response is already streaming. For idempotent methods this changes nothing. For uploads, retry safety rests where #531 actually measured it: Roblox dedupes identical place content, so a retry that races a publish which did land returns that same version. The doc comments now say so. ## Verification A local ALPN server driving the real transport, asserting the symptom rather than the header: two uploads must reach it over two connections with `connection: close` on the wire. ``` before node 26.5.1 FAIL alpn=h2 tcpConnections=1 connectionHeader=[null,null] after node 26.5.1 PASS alpn=http/1.1 tcpConnections=2 connectionHeader=["close","close"] after node 24.18 PASS alpn=http/1.1 tcpConnections=2 connectionHeader=["close","close"] after bun 1.3.14 PASS alpn=http/1.1 connectionHeader=["close"] ``` The CI error was also reproduced end to end: a server that sends `RST_STREAM` mid-upload surfaces `ERR_HTTP2_STREAM_ERROR` through `fetch`, byte-identical to the deploy log. Full suite green, 100% mutation score on every touched file. ## Notes for reviewers - **`http1-dispatcher.spec.ts` has one test that makes a real network call** (`http://127.0.0.1:1/`, refused in ~20ms). It is deliberate. Every other test injects a fake scope, so nothing else would notice the day undici's symbol stops being reachable — which is the day this outage silently returns with a green suite. - **A consumer who calls `setGlobalDispatcher(new MockAgent())` in their own tests would see uploads bypass their interceptors**, because the fresh agent carries none of them. Uploads only; every other request still uses the global. I did not guard it: there is no reliable signal distinguishing a mock from a real agent, and `OpenCloudClientOptions.httpClient` is the documented test seam. - **`TRANSIENT_TRANSPORT_CODES` is publicly exported and its meaning widened** from "proved unprocessed" to "transport-level failure", under a `patch`. No signature changes. ## Follow-ups, deliberately not in this PR - **No consumer-reachable override.** If undici's symbol moves again, a consumer on that runtime loses the mitigation and has no supported workaround short of an SDK release. A `dispatcher` or `fetchFunc` option on `OpenCloudClientOptions` would fix that, but it is a public API change needing its own tests and changeset. - **A gateway `RST_STREAM(CANCEL)` hangs `fetch` forever.** undici emits no error event and dequeues the request without rejecting it, and uploads carry no timeout by design, so the hang is unbounded and retries cannot reach it — nothing ever fails. Needs its own decision plus an upstream fix; recorded in the ADR amendment. ## References Follows #531, which fixed the HTTP/1.1 half of this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Overall Summary The PR aligns with Bedrock’s architecture: HTTP dispatcher discovery is isolated to an adapter, while fetch-client orchestration remains separate. Transport failures are handled with typed error codes, and the retry-safety documentation is appropriately narrowed. Tests cover dispatcher construction, fallback behavior, upload integration, and retry decisions, including malformed and unavailable runtime dispatcher shapes. No blocking architecture, security, or implementation issues were identified. ## Recommendations - Confirm CI enforces 100% coverage for the new dispatcher branches. - Verify all new tests use the required `it("should ...")` naming convention. - Retain Node 24, Node 26, and Bun verification because dispatcher internals are runtime-dependent. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Place publishes intermittently failed with either an
ApiErrorcarrying agateway summary (
HTTP 400 from gateway ("400 Bad request")) or aNetworkError(ECONNRESET/fetch failed), leaving a deploy with someplaces published and others not. The cause is HTTP keep-alive connection reuse:
Roblox's edge gateway discards idle pooled connections faster than a pooling
fetchexpects, and a request written into a discarded connection neverreaches Open Cloud.
The transport now sends
connection: closeon every requestisUploadRequestclassifies, the same predicate that already drops the default timeout, for the
same reason: uploads hold a connection far longer than a JSON call, so they are
the shape that loses this race. Place publishes are the observed victim, but
icon, thumbnail, and binary-input uploads share the exposure.
publishandsaveadditionally gainUPLOAD_METHOD_DEFAULTS, which retriesfailures that provably never reached Open Cloud: transient transport codes plus
a synthetic
GATEWAY_REJECTEDfor gateway-served responses.shouldRetryclassifies a gateway-origin
ApiErrorby that code rather than its status,since the status came from the gateway and says nothing about the request. They
still do not retry 5xx, where the duplicate-write risk actually lives.
ADR-010 gains a dated amendment recording the diagnosis and the trade.
Manual verification
A probe publishing to three places in one universe on the rate-limit queue's own
2s cadence, against the live API:
fetchfetchcurl, one process per requestRuled out by the same probe: it is not concurrency (a strictly sequential run
reproduces it), not content processing (it reproduces on a payload the server
rejects in 0.9s), and not payload size or runtime.
Retry safety was measured, not assumed. A killed publish creates no version:
after one, the place's next distinct upload took the version number the killed
one would have used, not the one after it. And Roblox dedupes identical place
content, returning the existing version number rather than minting a new one, so
a retry that races a publish which did land cannot duplicate it.
Real 7 MB publishes still succeed through the new path.
References
Closes #530
🤖 Generated with Claude Code