Make the client's lifecycle observable and its teardown safe

[monorkin]

hey-cli ended up working around a handful of things the client kept to itself. This makes the client say them instead.

• A failed Connect stops the client. Before, a Connect whose context ran out left the retry loop running with nothing to stop it. Now it shuts the client down, and the error wraps whatever the last attempt failed on, so a header that couldn't be built shows up rather than hiding behind a deadline. Retry semantics for header errors are unchanged.
• Client.Done() and Client.Err(). A stopped client can be noticed and asked why, instead of probing with Connect and reading the tea leaves.
• WithMaxAttempts(n). Stops with ErrGaveUp after n failures in a row. A welcome resets the count, so it bounds an outage, not the client's lifetime.
• Messages closes after the last callback returns. The dispatcher used to drain its queue after being stopped, so OnConnected could fire after Messages closed. Now the channel closes from the callback goroutine as its final act.
• Subscription.Err(). Says why Messages closed: ErrUnsubscribed, ErrRejected, or whatever stopped the client. That includes a rejection after a reconnect, which was only observable through the callback.
• Unsubscribe() takes no context. The command goes out on the client's own connection context, so it works during a teardown whose context has already ended. This is the one breaking change.

Independent of #6, which fixes the duplicate-subscribe hang.

[Copilot]

🟡 Changes recommended

Two moderate concurrency races can violate connection and callback-ordering guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Makes client lifecycle and subscription teardown observable while adding bounded retries and safer shutdown behavior.

Changes:

• Adds Client.Done(), Client.Err(), Subscription.Err(), and lifecycle errors.
• Adds WithMaxAttempts and preserves connection failure details.
• Closes Messages after callbacks finish and removes Unsubscribe’s context parameter.

File summaries

File	Review
subscription.go	Tracks subscription termination reasons and updates unsubscribe behavior.
README.md	Documents lifecycle, retry, teardown, and breaking API changes.
options.go	Adds maximum-attempt configuration.
errors.go	Defines new lifecycle and subscription errors.
dispatcher.go	Adds callback draining, but has a moderate enqueue-after-stop race; also contains a grammatical nit.
client.go	Implements lifecycle and retry handling, but has a moderate race between welcome and shutdown.
client_test.go	Tests lifecycle, retries, callback ordering, and unsubscribe behavior.

Review details

Suppressed comments (1)

dispatcher.go:14

• This sentence is grammatically incomplete; use “then stops.”

// Once stopped it runs what it still holds, then stopped. That is how a
// subscription closes Messages only after its last callback has returned.

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The terminal disconnect callback behavior and race-prone retry test require correction before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

README.md:91

• Connect can also return ErrAlreadyConnected, which leaves the existing client running rather than stopped. This advice should call out that exception so users do not throw away a healthy client after a duplicate call.

A `Connect` that returns an error leaves the client stopped, with nothing running
behind it. Throw it away and make a new one.

client.go:149

• This guarantee has an exception: a second Connect returns ErrAlreadyConnected at line 159 while the original client remains healthy and running. As written, callers may discard a live client based on the documentation; explicitly exclude that error from the teardown guarantee.

// ctx bounds the wait, not a connection that got through: that lives until Close.
// A Connect that returns an error leaves the client stopped, with nothing running
// behind it, so a client that failed to connect is one to throw away.

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The verdict callback race can cause observable callbacks to be lost.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

dispatcher.go:45

• This can discard a callback for an event that already happened. Subscription.confirm and Subscription.reject close their verdict channel before calling dispatch; the awakened Subscribe path can therefore unsubscribe/forget the subscription and stop this dispatcher before the corresponding OnConnected/OnRejected is queued. In the rejection path Subscribe itself calls forget, so TestSubscribeRejected can race and report that OnRejected was never called. Queue each verdict callback before publishing the verdict, or make publishing and dispatch atomic with stopping.
  client.go:330
• This says shutdown waits until nothing is running, but awaitStopped only waits for the client run loop. Subscription dispatchers drain asynchronously after closeSubscriptions; TestMessagesCloseAfterTheLastCallbackReturns explicitly has Close return while a callback is still blocked. Please narrow this comment so callers do not assume callback teardown is complete.

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced