basecamp / basecamp/basecamp-sdk
Event feed: CableConn.Close is documented to unblock reads, and the close-budget timeout path does not
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
Raised by Copilot on #705 (`websocket_transport.go`, the close-budget timeout
path). **The finding is correct at the seam-contract level**, and the test
weakness it names is the more useful half.
`seams.go:265` documents the contract: *"Close is idempotent, safe from any
goroutine, and unblocks ReadFrame"* — unqualified. `wsConn.Close` runs
`coder/websocket`'s handshake on a goroutine and returns after a 1s budget
(`errCloseNotAcknowledged`). Against a peer that never answers, the library's
own teardown is still in flight when `Close` returns, so a `ReadFrame` issued
with a context that is not cancelled — `context.Background()`, say — stays
blocked for up to the library's remaining ~9s ceiling. `Close` returned; the
read did not.
And the existing contract test does not catch it, for the reason given: its
peer acknowledges promptly, so the timeout path is never the one under test.
That is a test that passes for the wrong reason on the branch that matters.
### Why the connector itself is unaffected
Worth recording so the fix is not over-scoped. `liveConn.dispose` is the only
caller, and it cancels the attempt on the very next line:
```go
func (lc *liveConn) dispose(cancel context.CancelFunc) {
_ = lc.conn.Close(closeCodeNormal, "")
cancel()
for range lc.frames { } // join
lc.stale.stop()
}
```
The pump's `ReadFrame` takes the attempt context, so the cancel is what
unblocks it, not the `Close`. The close-before-cancel ordering is itself
deliberate (§23 needs the peer to see a close frame, and cancellation is
allowed to abort the socket outright), and it means the connector never
depends on the property being violated here. So this is a contract defect on a
**public seam type**, not a live hang in the reference connector.
### The decision
- **Tighten the implementation.** Make the timeout path force teardown before
returning. The obstacle is the one the budget comment already documents:
`coder/websocket`'s `Conn.Close` gives no exported way to abandon its
handshake, and `CloseNow` is not an escape hatch once `Close` is in flight —
`casClosing` has already flipped, so it just waits too. Any fix here probably
means not using `Conn.Close` for the timeout path at all.
- **Narrow the documented contract.** Say `Close` causes reads and writes to
return *once the transport's own teardown completes*, and that a caller
needing synchronous unblocking must cancel the operation's context — which is
exactly what `dispose` does, and would make the connector's ordering an
explicit part of the contract rather than a coincidence.
I lean to the second: it is honest about what the underlying library can
promise, and it documents the pattern the connector already relies on. But
`CableConn` is implemented by consumers, so weakening its contract is a §23
statement affecting all six SDKs rather than a Go-local doc edit.
Either way the contract test wants a peer that ignores the close handshake, so
the timeout branch is actually exercised.
Follows #606, #614, #645, #696; siblings #753, #758, #759, #760, #761.
Contributor guide
Research direction
Start with the contract at seams.go:265 and the close-budget timeout path in websocket_transport.go, then inspect liveConn.dispose to understand the existing cancellation ordering. Reproduce the behavior with a peer that ignores the close handshake and update the contract test; done requires a decided cross-SDK contract and a test that exercises the timeout branch.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design, networking
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100