[mikea]

counterpart to capnproto/capnproto#2634

[github-actions]

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR migrates several MembranePolicy subclasses from kj::Refcounted + manual addRef() overrides to kj::Rc<T> / kj::rc<T>().

Issues (ranked by severity):

1. [HIGH] Build is broken — classes are abstract and cannot be instantiated. capnp::MembranePolicy::addRef() is pure virtual (= 0) in the currently pinned capnp-cpp. This PR removes the addRef() overrides, making CompletionMembrane, RevokerMembrane, AttachmentMembrane, and ServerTopLevelMembrane abstract. CI confirms 13 compilation errors. This PR appears to depend on an upstream capnp-cpp change (making addRef() non-pure-virtual or removing it entirely), but the capnp-cpp pin in MODULE.bazel has not been updated.
2. [HIGH] Type mismatch — kj::Rc<T> is not implicitly convertible to kj::Own<T>. capnp::membrane() takes kj::Own<MembranePolicy> for the policy parameter. kj::rc<T>(...) returns kj::Rc<T>, and there is no implicit conversion. Each capnp::membrane(client, kj::rc<...>(...)) call would be a type error even if the abstract-class issue were resolved.
3. [MEDIUM] cm.addRef() on Rc<CompletionMembrane> returns Rc<CompletionMembrane>, not Own<MembranePolicy>. Line 2135 passes the result of cm.addRef() to capnp::membrane(), which again expects kj::Own<MembranePolicy>.

All three issues are symptoms of the same root cause: this PR requires a coordinated capnp-cpp update that adds Rc-based membrane overloads (or changes MembranePolicy to no longer require manual addRef()). That dependency update is missing from this PR.

---

This review was written by an AI assistant (Bonk). Feedback may not be perfect — please use your judgment.

[ask-bonk]

Review posted successfully on PR #6650. Here's a summary of what I found:

3 issues identified, all stemming from the same root cause — this PR depends on an upstream capnp-cpp update that isn't included:

1. [HIGH] Build broken: MembranePolicy::addRef() is pure virtual in the pinned capnp-cpp. Removing the overrides makes the classes abstract and uninstantiable. CI confirms 13 compilation errors.
2. [HIGH] Type mismatch: kj::rc<T>() returns Rc<T>, but capnp::membrane() expects Own<MembranePolicy> — no implicit conversion exists.
3. [MEDIUM] cm.addRef() type: Returns Rc<CompletionMembrane> instead of the Own<MembranePolicy> that membrane() needs.

The PR needs a coordinated capnp-cpp dependency update to land.

github run

[ask-bonk]

@mikea Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

kj::Rc has an implicit converting move constructor: template <typename U, typename = EnableIf<canConvert<U*, T*>()>> inline Rc(Rc<U>&& other) noexcept. So kj::Rc<CompletionMembrane> can implicitly convert to kj::Rc<MembranePolicy> since CompletionMembrane* is convertible to MembranePolicy*. Same for kj::rc<AttachmentMembrane>(...) returning kj::Rc<AttachmentMembrane> being passed where kj::Rc<MembranePolicy> is expected.

Now let me also verify the cm.addRef() call at line 2135. cm is a kj::Rc<CompletionMembrane>&. Calling .addRef() on it: MembranePolicy now has inline kj::Rc<MembranePolicy> addRef() { return addRefToThis(); }. But cm is an Rc<CompletionMembrane>, so cm.addRef() calls Rc<CompletionMembrane>::addRef() which returns kj::Rc<CompletionMembrane>. Wait, let me re-check — cm is an Rc<CompletionMembrane>& reference, not dereferenced...

Looking at line 2133-2135:

auto& cm = JSG_REQUIRE_NONNULL(
    completionMembrane, Error, "getExternalPusher() must be called before call()");
return capnp::membrane(kj::mv(target), cm.addRef());

JSG_REQUIRE_NONNULL on a kj::Maybe<kj::Rc<CompletionMembrane>> returns a kj::Rc<CompletionMembrane>&. So cm is kj::Rc<CompletionMembrane>&, and cm.addRef() calls kj::Rc<CompletionMembrane>::addRef() which returns kj::Rc<CompletionMembrane>. This then implicitly converts to kj::Rc<MembranePolicy> via the converting move constructor. All correct.

The whole change is clean and straightforward. The PR correctly:

1. Updates the capnp-cpp pin to a version that makes PipelineHook and MembranePolicy inherit from kj::Refcounted directly
2. Removes the now-redundant kj::Refcounted base class and addRef() overrides from workerd's membrane classes
3. Switches from kj::refcounted<T>(...) to kj::rc<T>(...) everywhere
4. Changes stored type from kj::Own<CompletionMembrane> to kj::Rc<CompletionMembrane>

There are no type mismatches, no backward compatibility issues (this is all internal), no security concerns. The code compiles cleanly against the new capnp-cpp pin.

LGTM

github run

[clanky-the-clanker]

🛑 Significant concerns — 1 BLOCKING item: PendingPipeline::setPipeline in worker-rpc.c++ still uses kj::refcounted<RevokerMembrane> and membrane->addRef(). This pull request removes that addRef() override, so the build will most likely break.

Findings: 1 blocking

_{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 #6650.

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] `PendingPipeline::setPipeline` still uses `kj::refcounted<RevokerMembrane>`, `membrane->addRef()` and `kj::Own<RevokerMembrane> pipelineMembrane`. This pull request removes the `addRef()` override and `kj::Refcounted` base from `RevokerMembrane`, so this call site will most likely not compile. Move it to `kj::rc` / `kj::Rc<RevokerMembrane>` / `membrane.addRef()`, as the other call sites do.
- **File:** src/workerd/api/worker-rpc.c++:601

---

📋 Code Quality: 🛑 1 blocking

One RevokerMembrane call site in worker-rpc.c++ was not moved to the kj::Rc API and will most likely fail to compile.

Recommendation: 🔴 Serious Concerns

• [BLOCKING] PendingPipeline::setPipeline still uses kj::refcounted<RevokerMembrane>, membrane->addRef() and kj::Own<RevokerMembrane> pipelineMembrane. This pull request removes the addRef() override and kj::Refcounted base from RevokerMembrane, so this call site will most likely not compile. Move it to kj::rc / kj::Rc<RevokerMembrane> / membrane.addRef(), as the other call sites do. (src/workerd/api/worker-rpc.c++:601)

View full review

• Missed call site, src/workerd/api/worker-rpc.c++:598-613: PendingPipeline::setPipeline still does the following:

auto membrane = kj::refcounted<RevokerMembrane>(kj::mv(paf.promise));
pipeline = capnp::membranePipeline(kj::mv(value), membrane->addRef());
pipelineMembrane = kj::mv(membrane);
...
kj::Own<RevokerMembrane> pipelineMembrane;

This pull request removes kj::Refcounted and the kj::Own<MembranePolicy> addRef() override from RevokerMembrane (src/workerd/util/completion-membrane.h). It moves every other capnp::membrane(...) construction to kj::rc<...>. setPipeline was the only call site left behind, and membrane->addRef() no longer matches the API the rest of the pull request targets. The capnp-cpp source at the new pin (a122ef9) is not in this snapshot, so the exact membranePipeline signature was not checked here. Even so, the rest of the diff points to a build break.

• Suggested fix: make the member kj::Rc<RevokerMembrane> pipelineMembrane, build it with kj::rc<RevokerMembrane>(...), and pass membrane.addRef() to capnp::membranePipeline. pipelineMembrane->retainRevoker(...) at line 540 keeps working through the Rc.

---

✅ All other checks passed

📝 Documentation: ✅ LGTM

No documentation concerns. The remaining TODO(cleanup) comments on the membrane classes are still accurate.

---

_{Reviewed commit: 409339e4}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.28%. Comparing base (ad0e6b9) to head (409339e).
⚠️ Report is 53 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/api/restore.c++	33.33%	2 Missing ⚠️
src/workerd/io/trace-stream.c++	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6650      +/-   ##
==========================================
- Coverage   69.44%   69.28%   -0.17%     
==========================================
  Files         695      697       +2     
  Lines      181393   182391     +998     
  Branches    29177    29223      +46     
==========================================
+ Hits       125967   126368     +401     
- Misses      38381    38951     +570     
- Partials    17045    17072      +27     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.