basecamp / basecamp/basecamp-sdk
Event feed: Connector.Close cancels the pump before dispose can send the close frame, inverting dispose's documented close-before-cancel ordering
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
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.**
```go
func (lc *liveConn) dispose(cancel context.CancelFunc) {
_ = lc.conn.Close(closeCodeNormal, "")
cancel()
...
}
```
But the pump reads on the **attempt** context, a child of `runCtx`:
```go
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.
Contributor guide
Research direction
Start with loop.go:152 and trace Connector.Close through the attempt context, ReadFrame, disposeAttempt, and liveConn.dispose. Read #762 alongside the feedtest.Transport fixtures and their expectClientClose assertions. Done requires an agreed teardown design that preserves close-before-cancel without introducing unbounded shutdown failures, with tests covering the relevant transport behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100