Event feed: Connector.Close cancels the pump before dispose can send the close frame, inverting dispose's documented close-before-cancel ordering

[jeremy]

Raised by Copilot as a suppressed comment on #705 (loop.go:152) at commit
93a3d5a457. Suppressed comments never become review threads, so this was never triaged —
it sat through thirteen subsequent review rounds. (The go/README.md:615 item from the
same block was re-reported at head and is fixed in c3ff858a5; this one was not.)

It is real at head (c3ff858a5), and the code's own comment is the proof.

The finding

liveConn.dispose documents its ordering as load-bearing:

The CLOSE COMES FIRST, and the order is load-bearing rather than stylistic. §23 requires
the connector to close the still-open socket explicitly — the rejected subscription
Action Cable leaves open, and every terminal — and a close is only observable to the peer
as a close frame. Cancellation is allowed to kill the connection outright (the seam
contract says a cancelled read returns promptly, and the default transport's library
aborts the socket to do it), so cancelling first races the close handshake and the peer
sees an abrupt teardown instead.

func (lc *liveConn) dispose(cancel context.CancelFunc) {
        _ = lc.conn.Close(closeCodeNormal, "")
        cancel()
        ...
}

But the pump reads on the attempt context, a child of runCtx:

at.lc = newLiveConn(at.ctx, conn, l.cfg.clock, l.cfg.staleAfter, l.hooks)
...
data, err := lc.conn.ReadFrame(ctx)

and Connector.Close cancels runCtx on the caller's goroutine, before returning —
deliberately, so that cancellation is visible before the return. That cancels at.ctx,
which aborts the pump's in-flight ReadFrame; with the default transport,
coder/websocket aborts the underlying connection in order to satisfy the cancelled read.
Only later does the run goroutine reach disposeAttempt → dispose →
conn.Close(closeCodeNormal, ""), by which point there is no socket left to write a close
frame to.

So on the Connector.Close path — the most common teardown there is — the ordering
dispose calls load-bearing is inverted, and the peer sees exactly the abrupt teardown
the comment says it must not.

Why the suite cannot see it

Every deterministic run substitutes feedtest.Transport, whose Close is a scripted
record rather than a socket. The fixtures carrying expectClientClose assert that Close
was called, which it is; they cannot observe that the real socket was already gone. This
is the same reachability boundary #762 and the declined native-close-budget finding sit on.

Severity, and why the obvious remedy is not obviously right

The harm is server-side: Action Cable sees a dropped connection rather than a clean
unsubscribe. No client-side correctness is lost, no events are skipped, nothing hangs. Real
but modest.

Copilot's prescribed remedy — "give the pump a cancellation scope that is cancelled only by
disposal after the socket close" — is where the care is needed. Decoupling the pump from
runCtx means a ReadFrame on a half-open socket is no longer interrupted by Close; it
would be interrupted only by conn.Close unblocking it. The seam contract does require
Close to unblock ReadFrame — but #762 is an open finding that the default transport's
close-budget timeout path does not honour exactly that obligation. Adopting this remedy
while #762 is open trades a cosmetic abrupt disconnect for a teardown that can block, which
is the class #705 spent seven rounds eliminating and the bar ("no unbounded failures") it
has been held to.

So the two want deciding together, in this order: #762 first, this second. A safer
intermediate shape worth costing: keep the pump on a context cancelled by disposal only,
and bound the join, so a transport that violates the unblock obligation degrades to a
delayed exit rather than a permanent one.

[jeremy]

Update: the blocker named above has moved. #777 (event-feed-foundations) was rebased and now carries fae998b16 "Event feed: Close bounds the read, not just itself", which gives the connection its own lifetime context, derives every read and write from it, and cancels it once Close is done waiting — so a parked ReadFrame(context.Background()) is actually released, and the cancel lands after the close budget so §23's close frame stays intact.

That is the obligation #762 says was unmet, and it is what this issue's remedy depends on. Once #777 merges, the ordering I gave — #762 first, this second — is satisfied, and decoupling the pump from runCtx no longer trades an abrupt disconnect for a teardown that can block.

The safer intermediate shape (bound the join) is correspondingly less necessary, though still worth a moment's thought for third-party CableTransport implementations, which are under no obligation to have adopted the same fix.

Noting rather than acting: #777 is another workstream's branch and I have not touched it.