fix: websocket session and callback promise retention leak

[VastBlast]

This PR fixes #153, a client-side memory leak in long-lived RPC sessions.

The session read loop was racing each transport.receive() call against a single session-long abort promise, which caused Promise.race() to accumulate until the entire session was disposed. This changes RpcSession to use a fresh cancellation promise for each pending read.

Edit: I also noticed another issue with callbacks causing a memory leak which isn't specifically reproduced in #153 which has been patched in this PR too.

Essentially if you had something like this where the callback is called regularly:

subscribeToTicks(cb: RpcStub<(ts: number) => void>) {
    const callback = cb.dup();

    let running = true;
    (async () => {
        while (running) {
            callback(Date.now());
            await new Promise(resolve => setTimeout(resolve, 100));
        }
    })()

    function cleanup() {
        running = false;
        callback[Symbol.dispose]?.();
    }

    callback.onRpcBroken(() => { cleanup(); });
}

The client would subscribe and capnweb would keep the callback promises in memory even though the result cannot be used.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4fc13b0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@154

commit: 4fc13b0

[kentonv]

The Promise.race() bug makes sense and the fix seems correct. I honestly didn't know that Promise.race doesn't remove its listeners from all the promises once one of them resolves. That's annoying!

Regarding the second issue, I think this is a bug in your app, not in Cap'n Web:

            callback(Date.now());

You are making a call here, and you are neither awaiting it nor disposing the returned promise. You need to do one or the other.

It is very much an intentional part of the protocol design that if you don't await a call promise, the result is not "pulled", and so never proactively sent from the server. The idea is that you now have a reference to the remote result which you can do promise pipelining on, without actually having to transmit it back. You need to dispose your reference (which is the promise itself) when you are done with it.

You could do something like this to ensure the promise is disposed:

            using promise = callback(Date.now());

Assigning the promise to a using causes it to be disposed at the end of the scope. (Equivalent to calling promise[Symbol.dispose]().)

However, this is still problematic, because in this usage pattern, you are ignoring errors. If you ignore errors, then there are all sorts of ways your callback might not run at all without you realizing it. If there was a network error, or a serialization error, or any number of other bugs, these would all just be silently ignored.

For this reason, you REALLY need to attach a .catch():

            callback(Date.now()).catch(err => console.error(err));

This also has the side effect of pulling the callback result, so resolves your memory leak.

In any case, if you can prune this PR back to just fix the Promise.race issue, I'm happy to accept that.

[kentonv]

I'm sort of losing my mind over the Promise.race memory leak. It's actually leaking the entire messages!

https://x.com/KentonVarda/status/2044935325554823193