Skip to content

feat(protocol): add shared MID exchange registry - #57

Merged
QuiteYellow merged 1 commit into
QuiteYellow:mainfrom
atc722:agent/shared-mid-registry
Aug 22, 2026
Merged

feat(protocol): add shared MID exchange registry#57
QuiteYellow merged 1 commit into
QuiteYellow:mainfrom
atc722:agent/shared-mid-registry

Conversation

@atc722

@atc722 atc722 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Context

Thank you for clarifying the ownership and landing sequence in #56.

This is the standalone shared MID-registry slice agreed there. Both #36 and
#54 need to correlate tokenless CoAP control messages with pending requests.
Landing that lifecycle once avoids carrying separate read-side and write-side
MID registries before those PRs are rebased.

What changes

  • Adds one private MID-indexed exchange registry shared by GET and POST while
    preserving the existing token-indexed response dispatch.
  • Atomically registers both indices before sending and avoids reusing a MID
    that is still owned by a live exchange.
  • Treats only an exact RFC 7252 four-byte v1 empty ACK or RST as a
    MID-correlated control message.
  • Marks an empty ACK as acknowledged without completing the request; GET stops
    retransmitting that MID and waits for the token-correlated separate response
    under the existing overall deadline.
  • Stores acknowledgement state on the exchange rather than in the response
    container, so clearing a stale Block2 response cannot discard the ACK state.
  • Completes a matching bare RST with SessionError.
  • Unregisters both token and MID indices after success, send failure, timeout,
    or RST.
  • Drains both indices and wakes affected waiters with SessionClosedError from
    both close() and reader-thread teardown.
  • Publishes the closed session state before draining, preventing a request that
    already passed its entry check from registering after the drain.

Scope

This PR intentionally does not include:

The existing Block2 duplicate, SZX, ETag, and accumulation semantics remain
unchanged. Moving acknowledgement state outside the response container also
addresses #36 review point 2 without bringing the rest of that PR into this
change.

Validation

  • python -m pytest -q: 320 passed in a clean Python 3.13 environment
  • focused MID lifecycle, reader-death, pacing, and stale-Block2 tests passed
  • git diff --check
  • share-safety check
  • targeted Ruff check
  • wheel and sdist build
  • distribution-content verification
  • isolated wheel and sdist imports

After this lands, I will rebase #36 and address its remaining review points,
then let @mbillow know that #54 is ready to rebase.

Refs #56

@QuiteYellow QuiteYellow left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approving and merging this.

It matches what #56 asked for in step 1. One structure serving both paths, allocated and indexed under the same lock before the send, and drained from close() and the reader finally through a single function. Leaving ping and Observe unregistered is the right call, since ping() never waits on a reply.

Two details I checked closely and both are right. ev.clear() sits inside the same locked block as the container read, so a response arriving between the check and the wait cannot be dropped. And publishing conn = None before the drain, with the second _check_live() after registration in post() and _exchange_block, closes the register-after-drain race properly.

Moving acknowledged onto the exchange also settles #36 review point 2, so that one is off your list before the rebase.

Three things I am taking on myself in a follow-up:

  1. _exchange_block allocates a fresh MID per attempt, so an unacknowledged retry still goes out as a new request. @mbillow flagged this in the body of #54 and it predates your work. This PR hands same-MID retransmission to #54, which covers the write path, leaving the read half unassigned.
  2. A matching RST, a token already in flight, and MID exhaustion all surface as a bare SessionError(). #23 added the typed hierarchy for exactly this.
  3. _next_available_mid_locked walks all 65,536 MIDs before giving up.

I will land those on top before calling the rebases. Thanks for turning this around so quickly.

@QuiteYellow
QuiteYellow merged commit ed0de9f into QuiteYellow:main Aug 22, 2026
8 checks passed
atc722 pushed a commit to atc722/SmartThings-Local that referenced this pull request Aug 23, 2026
QuiteYellow#57 landed the shared MID registry. Three follow-ups on top of it.

_exchange_block registered a new exchange per attempt, so an
unacknowledged retry went out under a fresh MID. RFC 7252 §4.2 defines a
retransmission as the same message, and a new MID instead presents each
retry to the appliance as a separate request it may answer twice. The
registration and the datagram now happen once, outside the attempt loop,
and every attempt sends the identical bytes. mbillow named this on QuiteYellow#56;
it predates QuiteYellow#57.

Two allocation failures also stopped being indistinguishable from any
other session failure. A CoAP RST now raises SessionResetError, since it
refuses one exchange while the transport stays up, and both "no free
Message ID" and "this token is already in flight" raise
SessionIdentifierError.

_next_available_mid_locked probed all 65,536 identifiers before giving
up. Candidates are consecutive and therefore distinct, so by the
pigeonhole principle one more candidate than there are live exchanges
always suffices, which is normally a single probe.
mbillow added a commit to mbillow/SmartThings-Local that referenced this pull request Aug 23, 2026
post() sent its datagram exactly once and then waited on a bare
ev.wait(timeout), while get() retransmits every block through
_exchange_block. One lost datagram — request or ACK — was therefore an
unrecoverable write, while a read absorbed the identical loss silently.
That is the report in LocalThings#384: reads keep working, three
unrelated resources intermittently do not.

Liveness. The bare wait also skipped the slicing _exchange_block uses,
so a reader thread dying mid-write burned the caller's whole 8s and
reported a device timeout for what was actually a dead session.
_wait_for_block is no longer block-specific — it becomes _wait_live, and
post() waits through it, so a reader death surfaces as SessionClosedError
within one liveness poll.

Retransmission, off by default. post() resends the CON up to
write_max_attempts times inside the caller's deadline, with §4.2 backoff,
pacing every attempt, and the last attempt taking whatever budget is
left. The datagram is built once and resent verbatim; reusing the MID is
the load-bearing part, because a server implementing §4.5 can then
recognise the duplicate and answer from its dedupe cache instead of
re-running the write. A caller-side retry cannot offer that — post()
mints a fresh MID and token per call, so a retry from above is a
genuinely new request the device has no way to dedupe.

It defaults to 1 attempt: on that path the wire behaviour is unchanged,
one datagram sent in the same order as before, per the ordering caution
on #384. Retransmitting into a device already dropping under load turns
one lost write into several, and §4.5 dedupe is unverified on RT-OCF,
which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can
see whether writes are still lost before turning this on, and the flag is
then a one-line change.

Two details that are not carried over unchanged from the single-send
version, both covered by tests:

  * timeout now bounds the whole call rather than the wait after the
    send. Attempts share one budget, so it has to be armed before the
    first pace — and a caller that asked for 8s should not wait 8s plus
    however long the rate limiter withheld the request.
  * a retransmission that fails to send is best-effort. A connected UDP
    socket reports the ICMP error queued by an earlier send on the next
    one, and the reader already treats those errnos as advisory; failing
    the exchange there would make retransmitting less robust than leaving
    it off. Attempt 0 still raises, since it is the caller's only
    datagram.

Rebased onto the shared MID registry: the empty-ACK and RST matching this
originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same
one-datagram-per-exchange shape, so what remains here is the write
attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's
pace-then-check-then-send ordering exactly; the liveness recheck is
skipped only once something has answered, because an answer that beat a
dying reader is a write the device confirmed and must not be discarded.
mbillow added a commit to mbillow/SmartThings-Local that referenced this pull request Aug 23, 2026
post() sent its datagram exactly once and then waited on a bare
ev.wait(timeout), while get() retransmits every block through
_exchange_block. One lost datagram — request or ACK — was therefore an
unrecoverable write, while a read absorbed the identical loss silently.
That is the report in LocalThings#384: reads keep working, three
unrelated resources intermittently do not.

Liveness. The bare wait also skipped the slicing _exchange_block uses,
so a reader thread dying mid-write burned the caller's whole 8s and
reported a device timeout for what was actually a dead session.
_wait_for_block is no longer block-specific — it becomes _wait_live, and
post() waits through it, so a reader death surfaces as SessionClosedError
within one liveness poll.

Retransmission, off by default. post() resends the CON up to
write_max_attempts times inside the caller's deadline, with §4.2 backoff,
pacing every attempt, and the last attempt taking whatever budget is
left. The datagram is built once and resent verbatim; reusing the MID is
the load-bearing part, because a server implementing §4.5 can then
recognise the duplicate and answer from its dedupe cache instead of
re-running the write. A caller-side retry cannot offer that — post()
mints a fresh MID and token per call, so a retry from above is a
genuinely new request the device has no way to dedupe.

It defaults to 1 attempt: on that path the wire behaviour is unchanged,
one datagram sent in the same order as before, per the ordering caution
on #384. Retransmitting into a device already dropping under load turns
one lost write into several, and §4.5 dedupe is unverified on RT-OCF,
which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can
see whether writes are still lost before turning this on, and the flag is
then a one-line change.

Two details that are not carried over unchanged from the single-send
version, both covered by tests:

  * timeout now bounds the whole call rather than the wait after the
    send. Attempts share one budget, so it has to be armed before the
    first pace — and a caller that asked for 8s should not wait 8s plus
    however long the rate limiter withheld the request.
  * a retransmission that fails to send is best-effort. A connected UDP
    socket reports the ICMP error queued by an earlier send on the next
    one, and the reader already treats those errnos as advisory; failing
    the exchange there would make retransmitting less robust than leaving
    it off. Attempt 0 still raises, since it is the caller's only
    datagram.

Rebased onto the shared MID registry: the empty-ACK and RST matching this
originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same
one-datagram-per-exchange shape, so what remains here is the write
attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's
pace-then-check-then-send ordering exactly; the liveness recheck is
skipped only once something has answered, because an answer that beat a
dying reader is a write the device confirmed and must not be discarded.
mbillow added a commit to mbillow/SmartThings-Local that referenced this pull request Aug 23, 2026
post() sent its datagram exactly once and then waited on a bare
ev.wait(timeout), while get() retransmits every block through
_exchange_block. One lost datagram — request or ACK — was therefore an
unrecoverable write, while a read absorbed the identical loss silently.
That is the report in LocalThings#384: reads keep working, three
unrelated resources intermittently do not.

Liveness. The bare wait also skipped the slicing _exchange_block uses,
so a reader thread dying mid-write burned the caller's whole 8s and
reported a device timeout for what was actually a dead session.
_wait_for_block is no longer block-specific — it becomes _wait_live, and
post() waits through it, so a reader death surfaces as SessionClosedError
within one liveness poll.

Retransmission, off by default. post() resends the CON up to
write_max_attempts times inside the caller's deadline, with §4.2 backoff,
pacing every attempt, and the last attempt taking whatever budget is
left. The datagram is built once and resent verbatim; reusing the MID is
the load-bearing part, because a server implementing §4.5 can then
recognise the duplicate and answer from its dedupe cache instead of
re-running the write. A caller-side retry cannot offer that — post()
mints a fresh MID and token per call, so a retry from above is a
genuinely new request the device has no way to dedupe.

It defaults to 1 attempt: on that path the wire behaviour is unchanged,
one datagram sent in the same order as before, per the ordering caution
on #384. Retransmitting into a device already dropping under load turns
one lost write into several, and §4.5 dedupe is unverified on RT-OCF,
which does not reliably emit RST either. With pacing (QuiteYellow#51) landed we can
see whether writes are still lost before turning this on, and the flag is
then a one-line change.

Two details that are not carried over unchanged from the single-send
version, both covered by tests:

  * timeout now bounds the whole call rather than the wait after the
    send. Attempts share one budget, so it has to be armed before the
    first pace — and a caller that asked for 8s should not wait 8s plus
    however long the rate limiter withheld the request.
  * a retransmission that fails to send is best-effort. A connected UDP
    socket reports the ICMP error queued by an earlier send on the next
    one, and the reader already treats those errnos as advisory; failing
    the exchange there would make retransmitting less robust than leaving
    it off. Attempt 0 still raises, since it is the caller's only
    datagram.

Rebased onto the shared MID registry: the empty-ACK and RST matching this
originally carried is QuiteYellow#57's now, and QuiteYellow#58 gave the read path the same
one-datagram-per-exchange shape, so what remains here is the write
attempt loop and the frames that must stop it. Attempt 0 keeps QuiteYellow#51's
pace-then-check-then-send ordering exactly; the liveness recheck is
skipped only once something has answered, because an answer that beat a
dying reader is a write the device confirmed and must not be discarded.
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.

2 participants