fix(protocol): reuse one MID across block retransmissions - #58
Conversation
#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 #56; it predates #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.
|
Deployed this to my two-appliance reference bridge before merging. The happy path is unaffected. Across half an hour the dryer held 93 to 96 reads per 60s window at a p_max of 264 to 1066ms, and the oven 220 to 229 at 1887 to 2550ms, with zero errors, zero timeouts and zero ping failures on both. That matches the pre-deploy baseline window for window. The retransmission path itself is untested on hardware. So the open question from the review stands. If a Samsung stack deduplicates a repeated Message ID without a cached response to replay, a retry that used to be answered under a fresh ID would now be dropped, and a block read that recovered on attempt 2 would instead burn its whole deadline. Nothing here rules that out. One dryer dropout during the container recreate, 44 seconds, self-recovered. Two of its reconnects returned |
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.
1 participant
Summary
Three follow-ups on top of #57, all named in its review. None of them are regressions from that PR.
The read path now retransmits one MID
_exchange_blockregistered a new exchange on every attempt, so an unacknowledged retry went out under a fresh Message ID. RFC 7252 §4.2 defines a retransmission as the same message; a new ID presents each retry to the appliance as a separate request that it may answer more than once. The Block2 NUM check discarded the surplus answer, which masked the effect without removing the cause.Registration and the datagram now happen once, outside the attempt loop, and each attempt sends identical bytes. @mbillow described this in the body of #54 and it predates #57.
Retransmission pacing is untouched, since #54 owns that policy.
Two allocation failures have their own types
A CoAP RST raises
SessionResetError. It refuses one exchange while the transport stays up, which is a different situation from a closed session.Both "no free Message ID" and "this token is already in flight" raise
SessionIdentifierError.Previously all three surfaced as a bare
SessionError(), which #23 added the typed hierarchy to avoid.tests/test_dtls_session_mid_registry.pypinned the old exact type, so that assertion moves with the behaviour.MID allocation stops walking the whole space
_next_available_mid_lockedprobed all 65,536 identifiers before raising. Candidates are consecutive and so distinct, which means one more candidate than there are live exchanges always finds a free one. That is a single probe in normal operation, and it only fails when every identifier is held.Validation
pytest tests/: 325 passed, 5 of them newerrors.pygit diff --checkcleanSequencing
#56 asks everyone to hold their rebases until this is in, so that #36, #54 and #48, #49, #50 each rebase once. Merging this releases them.