Skip to content

release the stale reply binding of a buffered push - #6788

Merged
SteffenDE merged 1 commit into
phoenixframework:mainfrom
patric-vinicios:fix-buffered-push-binding-leak
Aug 10, 2026
Merged

release the stale reply binding of a buffered push#6788
SteffenDE merged 1 commit into
phoenixframework:mainfrom
patric-vinicios:fix-buffered-push-binding-leak

Conversation

@patric-vinicios

Copy link
Copy Markdown
Contributor

startTimeout can run twice for the same push, and the second run leaves the first one's binding behind.

It happens whenever a push goes out on a channel that isn't joined yet. Channel.push calls startTimeout() and parks the push in pushBuffer;
once the join is acked the buffer gets flushed through Push.send, which calls startTimeout() again. Each call mints a fresh ref and registers a new chan_reply_* binding, but nothing releases the one from the first call — cancelRefEvent only knows about this.refEvent, and that's already been overwritten by then.

So the first binding just sits there. The push never went out under that ref, so the server never replies to it, so it never fires and never cleans itself up. It stays on channel.bindings for as long as the channel lives, and since the callback closes over the Push, the payload stays reachable with it. Channel.trigger walks that array on every message that comes in.

The window is narrow but it gets hit constantly — everything between join() and the join ack, plus the entire time a socket is down. Which is exactly when pushes get buffered in the first place.

Against a real server, 25 pushes during the join window plus another 25 across a reconnect leave 50 dead bindings, and all 50 payloads are still reachable after a forced GC. Both go to zero with this change. A push sent while the channel is joined gets collected either way, which acts as the control.

Dropping the old binding can't lose a real reply. A buffered push never hit the wire under the old ref, since Push.send is the only thing that calls socket.push and it hasn't run yet. Every other path into startTimeout (resend, the join push, leave, an ordinary push) has refEvent at null already, so the extra call does nothing.

One thing about the test: it restores the real makeRef. The existing describe("push") block stubs it to a constant, which lands every reply binding on chan_reply_1 — and since channel.off(event) clears all bindings for an event, the leaked one gets swept up along with the live one. That's why the suite never caught it.

@xh4 worked out the mechanism back in #4948. That issue got closed asking whether the regenerated ref actually caused a problem, and the numbers above are the answer to that.

@patric-vinicios
patric-vinicios force-pushed the fix-buffered-push-binding-leak branch 2 times, most recently from 3ef782f to 1a85e2c Compare August 9, 2026 16:05
Push.startTimeout mints a new ref and registers a new chan_reply_*
binding on the channel, but never releases the binding for the previous
ref. A buffered push runs it twice, once from Channel.push when canPush
is false and again from Push.send once the join is acked, so every push
made while the channel is not joined leaves a binding behind. The server
only ever replies to the second ref, so that binding is never triggered:
it stays on channel.bindings for the lifetime of the channel, keeps the
push payload reachable through its callback closure, and is walked by
Channel.trigger on every incoming message.

Cancelling the previous binding cannot drop a real reply, because a
buffered push never reached the wire under the old ref: Push.send is the
only path that calls socket.push. On every other path into startTimeout
refEvent is already null and the call is a no-op.

The test restores the real makeRef. The existing describe("push") block
stubs it to a constant, which collides every reply binding on
chan_reply_1 so channel.off clears the leaked one along with the live
one.

Mechanism originally diagnosed by @xh4 in phoenixframework#4948.
@patric-vinicios
patric-vinicios force-pushed the fix-buffered-push-binding-leak branch from 1a85e2c to facbcba Compare August 9, 2026 16:08
@SteffenDE
SteffenDE merged commit 4c11840 into phoenixframework:main Aug 10, 2026
8 checks passed
@SteffenDE

Copy link
Copy Markdown
Member

Thank you! 🙌🏻

SteffenDE pushed a commit that referenced this pull request Aug 10, 2026
Push.startTimeout mints a new ref and registers a new chan_reply_*
binding on the channel, but never releases the binding for the previous
ref. A buffered push runs it twice, once from Channel.push when canPush
is false and again from Push.send once the join is acked, so every push
made while the channel is not joined leaves a binding behind. The server
only ever replies to the second ref, so that binding is never triggered:
it stays on channel.bindings for the lifetime of the channel, keeps the
push payload reachable through its callback closure, and is walked by
Channel.trigger on every incoming message.

Cancelling the previous binding cannot drop a real reply, because a
buffered push never reached the wire under the old ref: Push.send is the
only path that calls socket.push. On every other path into startTimeout
refEvent is already null and the call is a no-op.

The test restores the real makeRef. The existing describe("push") block
stubs it to a constant, which collides every reply binding on
chan_reply_1 so channel.off clears the leaked one along with the live
one.

Mechanism originally diagnosed by @xh4 in #4948.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants