Rc<PipelineHook> and Rc<MembranePolicy>

[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