Skip to content

fix: unify client shutdown around access point and dealer lifecycles#305

Merged
devgianlu merged 2 commits intodevgianlu:masterfrom
gjermundgaraba:fix/audio-mercury-connection-lifecycle
May 5, 2026
Merged

fix: unify client shutdown around access point and dealer lifecycles#305
devgianlu merged 2 commits intodevgianlu:masterfrom
gjermundgaraba:fix/audio-mercury-connection-lifecycle

Conversation

@gjermundgaraba
Copy link
Copy Markdown
Contributor

Follow-up to #302 and #303. Those PRs hardened the lifecycle of ap and dealer themselves, but they. This PR is targeting a downstream issue from this, which is that mercury.Client and audio.KeyProvider have no way to observe when pa dies (which can cause some issues, as you might imagine).

The approach I took here is one I can adjust, but essentially the design does two things:

  1. It makes ap the only things that truly closes
  2. It gives ap a single done signal for consumers (like mercury and audio) to rely on

The main benefit of this design is that it simplifies the life cycle a lot (for instance, session close can now just rely on ap closing instead of having to deal with all the consumers too).
The downside is that it is an API breaking change: mercury.Client.Close() and audio.KeyProvider.Close() are removed.

If you want to consider alternative designs, I am happy to do that. I have gone through some iterations on this, and in the end, I thought this was the cleanest.

General summary of the changes (🤖):

  • Unifies shutdown handling in Accesspoint and Dealer by replacing ad-hoc stop/closed flags with a shared done channel lifecycle.
  • Exposes Accesspoint.Done() and updates AP/dealer send/receive/reconnect loops to stop cleanly when shutting down.
  • Makes audio.KeyProvider and mercury.Client follow the access point lifecycle instead of maintaining their own separate close/stop channels.
  • Improves closed-state behavior for request/receive paths by returning ErrAccesspointClosed / ErrDealerClosed consistently and handling closed receiver channels safely.
  • Simplifies Session.Close() by removing redundant explicit closes for audio key / mercury clients and relying on AP shutdown ownership.
  • Adds regression tests covering clean ticker shutdown and request failures after the access point is closed.

@devgianlu
Copy link
Copy Markdown
Owner

I see your point and I like that it reduces the complexity overall. However, I am not fully sold on the fact that once a KeyProvider or MercuryClient is created it cannot be destroyed. This is not necessarely blocking for the merge, but something to take into consideration.

@gjermundgaraba
Copy link
Copy Markdown
Contributor Author

I see your point and I like that it reduces the complexity overall. However, I am not fully sold on the fact that once a KeyProvider or MercuryClient is created it cannot be destroyed. This is not necessarely blocking for the merge, but something to take into consideration.

It is possible, but would be more complexity to support. The question is more if it's needed/wanted to keep IMO. Currently they are there, but not working as expected. If we want proper independent destruction, then we need to make it possible for them to unregister themselves with the AP.

I don't mind adding that, if that is what you prefer. If so, I can do it in this PR or a follow-up.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR unifies shutdown semantics across the access point (AP) and downstream consumers by making AP the single lifecycle owner and exposing an Accesspoint.Done() signal so components like mercury.Client and audio.KeyProvider can stop/return errors reliably when the AP is closed.

Changes:

  • Replaces ad-hoc closed/stop flags with a single done channel in AP and Dealer, and updates loops to stop on <-done.
  • Removes Close() from mercury.Client and audio.KeyProvider and makes their request/recv paths depend on AP shutdown (ap.Done() / ap.ErrAccesspointClosed).
  • Updates session shutdown and adds regression tests for shutdown signaling and post-close request failures.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
session/session.go Simplifies Session.Close() to rely on AP-owned shutdown.
ap/ap.go Introduces done lifecycle + Done() accessor; updates connect/send/receive/loops to observe shutdown.
ap/ap_test.go Updates ticker tests and adds coverage for Done() closing/idempotent Close.
dealer/dealer.go Replaces stop/closed flags with done; updates loops and close/reconnect behavior.
dealer/recv.go Updates receiver registration and request handling to respect dealer shutdown via done.
dealer/dealer_test.go Adjusts ticker tests and closed-state behavior test to use done.
mercury/client.go Removes client-local Close/stop; uses ap.Done() to stop and fail requests on AP close.
mercury/client_test.go Adds regression test ensuring request fails with ErrAccesspointClosed after AP close.
audio/provider.go Removes provider-local Close/stop; uses ap.Done() to stop and fail requests on AP close.
audio/provider_test.go Adds regression test ensuring request fails with ErrAccesspointClosed after AP close.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ap/ap.go Outdated
@devgianlu devgianlu merged commit 6247a08 into devgianlu:master May 5, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants