R2 jsrpc bindings by bthwaites · Pull Request #7913 · cloudflare/workerd

bthwaites · 2026-10-23T14:05:42Z

Picking up from @Carolx715 's PR: #7092

clanky-the-clanker · 2026-10-23T14:06:47Z

⚠️ Out of date: this review is of fe2abc06; the pull request is now at fcbdd084. Someone with write access can comment clanky review (or clanker review or @clanky-the-clanker review) to review the new commits.

🛑 Significant concerns — 1 BLOCKING item: on the JSRPC path, complete() ignores the create-time metadata it stores, so it either returns the wrong metadata or fails the gateway's missing-metadata assertion. There is also 1 WARNING: the RPC methods emit no r2_* trace spans.

Findings: 1 blocking · 1 warning · 2 info · 1 suggestion

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7913.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## BLOCKING (1)

### [Code Quality] `completeRpc` ignores the metadata `R2MultipartUpload` stores at create time, even though the header comment says the gateway omits it. `headResultFromBackend` (default EMPTY policy) asserts that metadata is present, so over JSRPC `complete()` either fails with an internal assertion or returns the wrong metadata.
- **File:** src/workerd/api/r2-multipart.c++:246

## WARNING (1)

### [Code Quality] The new `*Rpc` methods emit no `r2_*` trace spans or `cloudflare.r2.*` tags, so workers that enable `r2_binding_jsrpc` lose the R2 tracing the HTTP path provides. No test covers this.
- **File:** src/workerd/api/r2-bucket.c++

## INFO (2)

### [Code Quality] Unused `#include <workerd/util/autogate.h>`.
- **File:** src/workerd/api/r2-bucket.h:12

### [Code Quality] The instrumentation test now compares only a prefix of the received spans, so unexpected extra spans no longer fail it.
- **File:** src/workerd/api/tests/r2-instrumentation-test.js

## SUGGESTION (1)

### [Documentation] Document the backend entrypoint contract for the experimental `r2_binding_jsrpc` flag before it gets an enable date.
- **File:** src/workerd/io/compatibility-date.capnp:1729

📋 Code Quality: 🛑 1 blocking · 1 warning · 2 info

The multipart RPC complete() ignores the metadata saved at create time, the RPC methods emit no trace spans, and there are two smaller nits.

Recommendation: 🔴 Serious Concerns

[BLOCKING] completeRpc ignores the metadata R2MultipartUpload stores at create time, even though the header comment says the gateway omits it. headResultFromBackend (default EMPTY policy) asserts that metadata is present, so over JSRPC complete() either fails with an internal assertion or returns the wrong metadata. (src/workerd/api/r2-multipart.c++:246)
[WARNING] The new *Rpc methods emit no r2_* trace spans or cloudflare.r2.* tags, so workers that enable r2_binding_jsrpc lose the R2 tracing the HTTP path provides. No test covers this. (src/workerd/api/r2-bucket.c++)
[INFO] Unused #include <workerd/util/autogate.h>. (src/workerd/api/r2-bucket.h:12)
[INFO] The instrumentation test now compares only a prefix of the received spans, so unexpected extra spans no longer fail it. (src/workerd/api/tests/r2-instrumentation-test.js)

View full review

Scope: This review could not read PR #7092 or its review thread, so it does not check point by point how that feedback was addressed. Everything below comes from this pull request's code.

[BLOCKING] Create-time metadata is stored but completeRpc never uses it (src/workerd/api/r2-multipart.h:29-30,115, src/workerd/api/r2-multipart.c++:230-250, src/workerd/api/r2-bucket.c++:679-693)
- The header comment says the gateway's completion response omits create-time metadata, and that uploads created over RPC keep those values locally.
- createMultipartUploadRpc normalizes and clones httpMetadata and customMetadata into R2MultipartUpload::metadata.
- completeRpc never reads that field. It passes the backend result straight to headResultFromBackend(js, backend), which uses the default MissingMetadataPolicy::EMPTY.
- That policy KJ_ASSERTs that both HTTP metadata and custom metadata are present.
- If the gateway really omits metadata, as the comment says, every complete() call over JSRPC fails with an internal assertion. If the gateway sends empty metadata instead, complete() returns an object without the user's metadata. In both cases the result differs from the HTTP path.
- Fix: merge the stored metadata into the completion result when the backend omits it, or remove the field and correct the comment.
- Test: add one that passes httpMetadata and customMetadata to createMultipartUpload and checks the object complete() returns. Today every test backend forwards to a real binding, so this case is never exercised.
[WARNING] No trace spans on the RPC path (src/workerd/api/r2-bucket.c++ *Rpc methods, src/workerd/api/r2-multipart.c++ *Rpc methods)
- The HTTP methods create r2_* spans through the new makeR2TraceContext helper. None of the RPC methods create a TraceContext or set cloudflare.r2.* tags.
- A worker that enables r2_binding_jsrpc therefore loses the R2 spans it gets today.
- r2-instrumentation-test.js only runs with the flag off, so nothing covers this.
- Fix: emit equivalent spans, or document that the omission is intended and add a test that pins the behaviour.
[INFO] Unused include: #include <workerd/util/autogate.h> in src/workerd/api/r2-bucket.h:12. Nothing in the R2 files references an autogate.
[INFO] Weaker instrumentation assertion (src/workerd/api/tests/r2-instrumentation-test.js)
- deepStrictEqual(received, expected) now compares only received.slice(0, expected.length).
- Extra spans that nobody expects no longer fail the test.
- Fix: filter received to the relevant tests, or also assert the total count.

📝 Documentation: 💬 1 suggestion

The experimental flag has no user-facing documentation.

Recommendation: 💬 Optional Nits

[SUGGESTION] Document the backend entrypoint contract for the experimental r2_binding_jsrpc flag before it gets an enable date. (src/workerd/io/compatibility-date.capnp:1729)

View full review

[SUGGESTION] src/workerd/io/compatibility-date.capnp adds the experimental r2_binding_jsrpc flag but does not describe the entrypoint methods the backend must implement.
The flag has no enable date, so the rule that flags are documented before their enable date does not apply yet.
Documenting the backend contract before the flag becomes non-experimental would help anyone configuring this transport.

✅ All other checks passed

🔒 Security: ✅ LGTM

Serialization: RPC result types require an RPC serializer or deserializer, so a script cannot structured-clone them.
Untrusted RPC results: headResultFromSerializable rejects non-finite, negative and non-integer sizes and range values. Body lengths are capped at the safe-integer limit.
Request inputs: SSE-C keys, checksums, onlyIf and ranges go through the same validation as the HTTP path. SSE-C keys are not added to trace tags.
Inline body limits: inline bodies have explicit size limits, and larger bodies are sent as streams.

📖 AGENTS.md: ✅ LGTM

No AGENTS.md concerns.

⚡ Performance: ✅ LGTM

Large bodies: bodies over 16 MiB, and strings over 8 MiB, are sent as streams. Large arrays and blobs are copied once into native memory, which the isolate lock requires.
Get bodies: ensureBodyLength adds one constant-cost pipe per get.
List results: conversion is linear in the number of objects.

_{Reviewed commit: fe2abc06}

jp4a50 · 2026-10-25T13:17:54Z

clanky review with a focus on how comprehensively the feedback on the predecessor PR (#7092) has been addressed

jp4a50 · 2026-10-25T13:26:44Z

[WARNING] The new *Rpc methods emit no r2_* trace spans or cloudflare.r2.* tags, so workers that enable r2_binding_jsrpc lose the R2 tracing the HTTP path provides. No test covers this. (src/workerd/api/r2-bucket.c++)

Unlike the HTTP binding implementation, it is not the JSRPC client's responsibility to emit trace spans - that is the JSRPC server's responsibility.

jp4a50

The RPC methods should forward binding arguments without validating or normalizing them on the client (marshalling to an appropriate wire representation should be able to be handled by serialize/deserialize methods on the R2 binding API types).

The only exceptions I'm aware of are:

Adapting oversized put/uploadPart bodies for JSRPC’s message-size limit
Applying caller-specific compatibility behavior, including r2_list_honor_include as described in the previous review (#7092 (comment))
(Possibly) handling upload-stream cancellation - it's worth testing this to see what happens if, for example, the JSRPC server rejects the RPC without reading the body or after partially reading the body, then see if any changes (e.g. cancellation guard) are required to achieve the desired behaviour.

jp4a50 · 2026-10-25T13:19:21Z

+  KJ_UNREACHABLE;
+}
+
+/*****************************


[nit] Remove comment. Doesn't add meaningful value.

jp4a50 · 2026-10-25T13:39:48Z

+        }
+        KJ_IF_SOME(requestedIncludes, o.include) {
+          for (auto& requested: requestedIncludes) {
+            JSG_REQUIRE(requested.value == "httpMetadata" || requested.value == "customMetadata",


This comment from the previous PR is still unaddressed. We need all client-side input validation to go.

The JSRPC API contract is that all binding calls from the user must reach the server (assuming the server at least holds up its side of the contract/exposes the correct API methods etc), even if their input is malformed

jp4a50 · 2026-10-25T13:47:16Z

+          KJ_CASE_ONEOF(value, Range) {
+            kj::Vector<jsg::Dict<double>::Field> fields;
+            KJ_IF_SOME(offset, value.offset) {
+              JSG_REQUIRE(offset >= 0, RangeError, "Invalid range. Starting offset (", offset,


Remove input validation.

In general, all of the preamble before the call to callR2RpcMethod should be able to be removed. JSG_STRUCTs are generally JSG-serializable by default so don't need to be transformed before being passed to JSRPC as long as their fields are also JSG-serializable (and I believe Carol's now made all non-trivially-serializable types like Checksums serializable now (via JSG_SERIALIZABLE).

LMK if there is any reason any of this preamble can't be removed.

jp4a50 · 2026-10-25T13:50:35Z

+      return kj::mv(checksum);
+    };
+
+    jsg::Optional<PutOptions> rpcOptions;


Remove all input validation and argument (PutOptions) transformation before RPC call - should no longer be needed.

jp4a50 · 2026-10-26T12:58:16Z

+        kj::String, kj::Maybe<R2PutValue>, jsg::Optional<PutOptions>)>>& putFnHandler,
+    const jsg::TypeHandler<jsg::Promise<kj::Maybe<HeadBackendResult>>>& putResultHandler) {
+  return js.evalNow([&] {
+    auto cancelReader = kj::defer([&] {


This cancellation guard seems to be dropped before the RPC call is made so once input validation is removed it will become redundant.

jp4a50 · 2026-10-26T13:07:48Z

  };

-  R2MultipartUpload(kj::String key, kj::String uploadId, jsg::Ref<R2Bucket> bucket)
+  // The gateway's completion response omits create-time metadata. Created RPC uploads retain


This comment has not been addressed from the predecessor PR. Please address or respond to all comments from the previous PR.

Yes, I missed that you added another round of comments on Oct 21. Doing another pass through for those.

jp4a50 · 2026-10-26T13:11:28Z

+  // as the key) by mistake. NonCoercible so that a non-string uploadId is rejected rather than
+  // stringified. A Dict rather than a JSG_STRUCT because structs must be registered in the
+  // isolate's type list, which is at the compiler's fold-expression nesting limit.
+  using CreateMultipartUploadBackendResult = jsg::Dict<jsg::NonCoercible<kj::String>>;


I discussed with @Carolx715 that this should be made it into a JSG_STRUCT. You should be able to work around the compiler limitation by updating -fbracket-depth in the compile options (grep for it as it's set in multiple places in the codebase) from 512 to e.g. 1024.

jp4a50 · 2026-10-26T13:13:09Z

+    return uploadPromise.then(js,
+        [bucket = JSG_THIS, key = kj::mv(key), metadata = kj::mv(metadata)](
+            jsg::Lock& js, CreateMultipartUploadBackendResult result) mutable {
+      KJ_ASSERT(result.fields.size() == 1 && result.fields[0].name == "uploadId"_kj,


This assertion seems unnecessarily restrictive. I think assertions on JSRPC call return values should be limited only to those that would otherwise prevent a valid local result from being constructed.

jp4a50 · 2026-10-26T13:15:08Z

+        uploadPartFnHandler,
+    const jsg::TypeHandler<jsg::Promise<UploadedPart>>& uploadPartResultHandler) {
+  return js.evalNow([&] {
+    JSG_REQUIRE(partNumber >= 1 && partNumber <= 10000, TypeError,


Remove all input validation. Leave this to the server.

jp4a50 · 2026-10-26T13:15:21Z

+    const jsg::TypeHandler<jsg::Promise<R2Bucket::HeadBackendResult>>& completeResultHandler) {
+  return js.evalNow([&] {
+    for (auto& part: uploadedParts) {
+      JSG_REQUIRE(part.partNumber >= 1 && part.partNumber <= 10000, TypeError,


Remove input validation.

jp4a50 · 2026-10-27T23:04:08Z

3. it's worth testing this to see what happens if, for example, the JSRPC server rejects the RPC without reading the body or after partially reading the body, then see if any changes (e.g. cancellation guard) are required to achieve the desired behaviour.

@bthwaites could you add some test coverage for this. It may be that no cancellation guard or other code changes are required to make them pass but I think testing backend rejection before the body is read or after it's only partially read would be worthwhile.

jp4a50

Nice!

Thanks @Carolx715 and @bthwaites . It's been a journey. Appreciate the patience shown to work towards a nice clean API.

As per internal chat, we could still do with explicit test coverage clarifying the behaviour when:

JSRPC server explicitly cancels stream (codifies the cancellation behaviour we get with the gate either on or off)
JSRPC server throws before (or during?) consumption of the stream

But I don't think that needs to block merging this PR.

jp4a50 · 2026-10-27T22:59:20Z

 class R2MultipartUpload: public jsg::Object {
 public:
  struct UploadedPart {
    int partNumber;


I set an agent on the task of assessing how transparently the latest changes forward the arguments passed to the R2 binding API calls up to the JSRPC server.

One quirk is that a couple user-facing fields (UploadedPart.partNumber and ListOptions.limit) are implicitly narrowed from a JS number to an int when they enter C++, meaning that if someone sets them to a non-integer they will get rounded (e.g. caller passes 1.5, JSRPC server sees 1).

I don't think that's a massive deal but it's a quirky little exception to the API contract of "pass all arguments to the JSRPC server without validation or transformation".

Not blocking for this PR but perhaps we should think about either documenting this or fixing it in a follow up.

jp4a50 · 2026-10-27T23:14:57Z

 # overriden in config=opt
 build --@capnp-cpp//src/kj:debug_memory=True
-build --cxxopt="-fbracket-depth=512" --host_cxxopt="-fbracket-depth=512"
+build --cxxopt="-fbracket-depth=1024" --host_cxxopt="-fbracket-depth=1024"


[nit] Could you also update -fbracket-depth=512 to 1024 in compile_flags.txt and doxyfile?

bthwaites · 2026-10-28T18:00:36Z

Added the stream cancellation test cases and updated fbracket-depth to 1024

…ag test

The JSRPC methods on R2Bucket and R2MultipartUpload no longer create R2 user trace spans or set request/response tags. The HTTP-backed methods keep their spans unchanged. The JSRPC spans could not report cloudflare.r2.response.success or the R2 error tags, because the gateway's RPC results do not carry the backend status. Rather than emit spans that diverge from the HTTP transport, drop them until the RPC protocol can support parity. Drop addHttpMetadataRequestSpanTags and the TraceContext parameter of makeSerializableConditional, which only the JSRPC methods used. r2-jsrpc-test no longer attaches the instrumentation tail worker, and r2-instrumentation-test compares the HTTP transport's spans exactly.

…ments

- Preserve list.include compatibility - Replace multipart result dict with JSG struct - TS updates - Split tests

…ancel-propagation

Carolx715 · 2026-10-27T23:02:03Z

-    // reenable the type
+    // Keep the existing generic error behavior for R2 bindings. Changing this throw path to
+    // throw a structured R2Error would be a public compatibility change, and R2Error would also
+    // need to be made serializable so it can cross JSRPC.


Nit: Maybe we should revert this comment since it's slightly outdated now? It implies that we decided to keep it as is going forward. I added the comment initially to elaborate on how R2 users expect a plain error string to be returned and we should keep that behavior for back compat. But that depends on whether or not we ever plan on supporting throwing R2Error structs, which is a conversation for a later time and not really relevant to this PR

Carolx715 · 2026-10-28T17:31:58Z

INFO: Moving a discussion from DMs to here - Since the contract isn't fully settled yet & this is only used for testing right now we can keep the contract in workerd/api/tests/r2-binding-service for lack of a better place. On public release we probably want to move this to a different directory

bthwaites requested review from a team as code owners October 23, 2026 14:05

bthwaites requested a review from edmundhung October 23, 2026 14:05

bthwaites force-pushed the r2-jsrpc-bindings branch 2 times, most recently from 4e07a46 to fe2abc0 Compare October 23, 2026 17:30

jp4a50 requested changes Oct 26, 2026

View reviewed changes

jp4a50 approved these changes Oct 27, 2026

View reviewed changes

Carolx715 and others added 13 commits October 28, 2026 14:02

Add JSRPC passthroughs on R2Bucket and R2MultipartUpload

5e1c5a5

refactor tests to use ObservedR2Binding

c44df4a

remove unneeded fast struct tests, group them under general compat fl…

9c69cac

…ag test

Remove R2BindingEntrypoint mock and related tests

cec2d55

fixing various edge cases

df90b32

Refactoring redundant serializable types out

11d8cae

add R2BindingService and return uploadId

6402055

Narrow named R2 JSRPC methods to accept only native R2 API result types

47fffff

Remove R2 client-side stream length validation and valueSize RPC argu…

7426e20

…ments

remove unusued multipart upload metadata snapshot

f199cef

Remove R2 RPC input validation

dee191e

- Preserve list.include compatibility - Replace multipart result dict with JSG struct - TS updates - Split tests

Add test cases for r2 jsrpc stream cancellation with jsrpc-readable-c…

fcbdd08

…ancel-propagation

bthwaites force-pushed the r2-jsrpc-bindings branch from c75b57c to fcbdd08 Compare October 28, 2026 18:02

Carolx715 reviewed Oct 28, 2026

View reviewed changes

Conversation

bthwaites commented Oct 23, 2026

Uh oh!

clanky-the-clanker Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📋 Code Quality: 🛑 1 blocking · 1 warning · 2 info

📝 Documentation: 💬 1 suggestion

🔒 Security: ✅ LGTM

📖 AGENTS.md: ✅ LGTM

⚡ Performance: ✅ LGTM

Uh oh!

jp4a50 commented Oct 25, 2026

Uh oh!

jp4a50 commented Oct 25, 2026

Uh oh!

jp4a50 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jp4a50 commented Oct 27, 2026

Uh oh!

jp4a50 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bthwaites commented Oct 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clanky-the-clanker Bot commented Oct 23, 2026 •

edited

Loading