basecamp / basecamp/basecamp-sdk

Event feed: CableConn.Close is documented to unblock reads, and the close-budget timeout path does not

Open
#762 0 comments 0 reactions 0 assignees View on GitHub
bug go spec
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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.