Advertise API 1.17: serial proxy ownership and port mode - #34
Merged
Merged
Conversation
The connection that SUBSCRIBEs a serial proxy instance owns it, as on ESPHome 2026.10. CONFIGURE, SET_MODEM_PINS, FLUSH and SET_MODE from any other connection are acknowledged PORT_IN_USE, a non-owner's WRITE is dropped, and GET_MODEM_PINS stays ungated. This applies before anyone has subscribed too, so a client that used to CONFIGURE or WRITE first must now SUBSCRIBE first. Ownership lives in Espex.Server beside BLE address ownership, with one monitor per connection pid shared by both kinds and a DOWN sweep that releases both; a claim against an owner whose process has died takes over. Dispatch gates on the local subscribe intent, which the Connection records only after a successful claim, so the gate stays pure. UNSUBSCRIBE and disconnect close the handle before releasing ownership so a free instance is really free for exclusive-open adapters, and teardown tolerates a Server that is already down. SerialProxySetModeRequest is backed by a new optional set_mode/2 adapter callback. RAW is always OK, PROTOCOL is NOT_SUPPORTED without the callback, and the mode belongs to the session: it is reapplied after a CONFIGURE reopen and reset to raw through the adapter on UNSUBSCRIBE. Version 0.11.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Disconnect paths can expose or re-close serial handles after ownership has been released.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
cleanup/1 ran twice on in-process close paths with the same
opened_ports, so the second pass could close a handle the next owner
had already reopened. It now returns the cleared state, which the
close sites thread into {:close, _} and {:stop, _}.
A connection killed outright never reaches close/1 while its ownership
is still swept, so a handle must not outlive the subscriber pid it was
opened for; documented on open/3 and in the Ownership section.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
SET_MODE validation violates owner-first semantics, and BLE cleanup can skip adapter disconnects when the Server is unavailable.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lib/espex/connection.ex:1148
- When the Server is already down (the
:rest_for_onecase described above),release_all_owners/2catches the exit and returns[]. Cleanup then skipsadapter.disconnect/1even for addresses still present instate.bluetooth_owned, leaking adapter-side BLE connections during Server restarts. Union the locally owned addresses with the Server result so teardown still reaches the adapter while retaining the Server-side drift protection.
lib/espex/dispatch.ex:291 - Validate ownership before the mode value. As written, an unsubscribed client can send mode
7and receiveINVALID_ARGUMENT, even though this PR's single-owner rule says every non-owner SET_MODE isPORT_IN_USE; upstream likewise checksis_subscriber_before validating the mode. This leaks a different externally visible result for the same non-owner operation.
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…E addresses on teardown Upstream set_mode_from_client refuses a non-owner before looking at the mode, so a non-owner is PORT_IN_USE whatever it sent; the enum check now runs after the owner gate, as for CONFIGURE. BLE teardown disconnected only the addresses the Server returned, so a Server that was already down left adapter-side connections open. The locally owned set is unioned in. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-process ownership, monitoring, and adapter teardown semantics warrant final human validation despite comprehensive coverage.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
ThousandIsland routes every close through exactly one handle_* callback, each of which already ran cleanup/1; the pre-close calls made it run twice and the cleared-state threading only papered over that. Teardown now runs once, closes serial handles, releases both ownership kinds in one Server call, then disconnects BLE addresses. Inbound bytes were forwarded to any connection holding a handle, which a non-owner can obtain through the ungated GET_MODEM_PINS; only the subscribed connection receives them now. Closing the handle on UNSUBSCRIBE made the explicit set_mode(:raw) redundant, so the mode dies with the handle and adapters tear down in close/1. UNSUBSCRIBE acks OK once torn down, RAW is OK without a handle, and the docs state the keepalive window before a vanished peer's port is taken over. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Mode-reapply failures can leave the adapter in RAW mode while reporting success and retaining PROTOCOL state, and teardown behavior contradicts the PR description.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/espex/serial_proxy.ex:252
- This says every first operation opens and every operation except GET_MODEM_PINS requires prior ownership, but SUBSCRIBE acquires ownership itself and a non-owner's idempotent UNSUBSCRIBE neither owns nor opens the port. Rewording the lifecycle avoids giving adapter authors and clients an incorrect sequencing contract.
lib/espex/connection.ex:1292
- After a client successfully selects PROTOCOL, CONFIGURE opens a fresh raw handle, but a valid failure from this reapply is only logged. The caller then acknowledges CONFIGURE as
OKand keepsserial_modes[instance] == :protocol, even though the adapter is actually raw, so subsequent traffic violates the advertised session mode. Propagate the reapply failure into the CONFIGURE result and either close the unusable handle or update the recorded mode.
case set_serial_mode({:ok, handle}, adapter, :protocol) do
{:ok, :ok} ->
:ok
other ->
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Balanced
A CONFIGURE reopen that could not re-enter PROTOCOL kept the mode recorded while the fresh handle was raw. The recorded mode is now dropped so state matches the adapter; the client can re-send SET_MODE. The lazy-open paragraph no longer says SUBSCRIBE needs prior ownership. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@api_version_minor 17); package version 0.11.0PORT_IN_USE, a non-owner's WRITE is dropped, GET_MODEM_PINS stays ungatedEspex.Serverbeside BLE address ownership:claim_serial_owner/3,release_serial_owner/3,release_all_serial_owners/2,serial_owner/2; one monitor per connection pid shared by both kinds; DOWN sweep releases both; dead recorded owner is taken over by the next claimSUBSCRIBE/UNSUBSCRIBEbecome{:serial_subscribe, i}/{:serial_unsubscribe, i}actions resolved by the Connection (claim, record intent, lazy open, ack); Dispatch gates on the local intent onlySerialProxySetModeRequest(RAW / PROTOCOL) acknowledged withSET_MODE, backed by new optionalEspex.SerialProxy.set_mode/2; RAW always OK, PROTOCOLNOT_SUPPORTEDwithout the callback, out-of-range modeINVALID_ARGUMENTclose/1Espex.SerialProxymoduledoc: new "Ownership" and "Port mode" sections, updated acknowledgement tabletest/espex/api_1_17_test.exs; existing serial integration tests subscribe first;Espex.Test.TcpClient.subscribe/3andEspex.Test.ModeTrackingSerialProxyadded🤖 Generated with Claude Code