Close and retry the longpoll transport when a batch POST times out - #6769
Merged
SteffenDE merged 2 commits intoJul 29, 2026
Merged
Conversation
`batchSend` reset `awaitingBatchAck` in its response callback and on a non-200 response, but the caller timeout path only called `onerror`. The transport was left waiting for an ack that could never arrive, so every subsequent `send` appended to `batchBuffer` and nothing was ever sent again, while the poll side kept the transport looking alive. Reset the ack and reuse `ontimeout`, so a timed out batch closes and retries the transport the same way a failed one does. Closes phoenixframework#6767 Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6767.
The bug
LongPoll.batchSendresetsawaitingBatchAckin its response callback and again on the non-200 branch, but its caller-timeout argument is only() => this.onerror("timeout"). Nothing resets the ack, nothing retries, and the transport is not closed. From then on everysend()takes theelse if(this.awaitingBatchAck)branch and appends tobatchBuffer, which is never flushed again. The GET poll keeps working, so the transport still looks open, but no client message — including the channel join — ever leaves the browser on that instance.The fix
Make the timeout path do what the non-200 path in the same function already does: reset
awaitingBatchAckand close and retry, reusing the existingontimeoutthat the poll side already uses for its own caller timeout.On "if it holds POST requests, how would reconnecting help?"
@SteffenDE — fair challenge. A few parts to the answer:
A network that blackholes every POST is unrecoverable, and nothing here claims otherwise. If the appliance holds every POST forever, longpoll is dead and the user needs a different network. And yes — a middlebox that breaks both transports puts you in a bad spot no matter what the client does.
But it does work sometimes. The case we traced is a dual-WAN branch office whose two uplinks are not equal: one path runs through the inspecting appliance, the other doesn't, and the router picks per connection. A fresh connection is therefore a fresh draw. In one production session the POSTs began flowing 8.5 minutes in, on the same page, with nothing else changed. A transport that closes and retries gets those draws; the zombie instance never does, by construction — it holds one bad connection forever.
Where nothing helps, the fix buys honest failure instead of a silent freeze. Today the failure is invisible: the transport reports itself open, the poll loop keeps ticking, and sends pile into
batchBufferwith no error, no backoff, no fallback memory, and a join that stays in flight forever. Nothing above the transport can react to a failure it is never told about. With the fix the socket layer learns the transport failed and does its usual work — channel error, reconnect backoff, and the application's own failure handling finally gets a chance to run.And the small one: the non-200 branch of this same function already does exactly this, so the timeout path diverging reads more like an oversight than a decision.
Tests
One test added to
assets/test/longpoll_test.js: a batch POST that times out on the caller side now firesonerror("timeout"), closes the transport (readyStateback toconnecting), clearsawaitingBatchAck, and a subsequentsend()goes out as a new POST rather than disappearing intobatchBuffer. It fails onmain(the transport is never closed).npm testis green (179 passing, 3 skipped).Reproduction
We have a harness — a node middlebox proxy that forwards
GET /longpollnormally and holdsPOST /longpollopen forever, plus a Playwright driver — and are glad to share it or run a patch through it.Submitted on behalf of Rupert Deese by Claude, the AI engineering assistant at Gearflow.