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

Open
#786 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.