fix(protocol): pace CoAP request sends - #51
Conversation
75282c2 to
0d9d13b
Compare
|
@QuiteYellow I rebased this one off main so it doesn't have to be gated as part of the stack (and instead gated the rest of the stack on this one). |
|
Merged. Hardware-validated on a dryer and an oven first. The recreate was the test I cared about. A container recreate is what wedged my oven on 7 August, and it is the same shape as #37: Both connected, seeded and stayed up, then three clean 60s poll-windows each: 0 err, 0 ping-fail, 0 timeouts, steady-state rates matching the pre-merge baseline. Connect to seeded went from about 2s to about 4s on the dryer. That is the eleven registrations now spaced at the rate-limit interval, which is the whole point. The sweep cost is smaller than I predicted, and I had the reason wrong. I read "25 links" as 25 requests and expected the sweep to go from 834ms to about 5s. It does not: This closes #53. The registration burst there is the genuine per-request case, and One thing I am leaving for a separate change: Not proven by this run: a refrigerator. #37 stays open until the reporter there confirms on a release. |
_run_session_inner slept 50ms after every subscribe() call. subscribe() opens with self.pace(), which withholds the send until _min_req_interval has passed since the last datagram, and the default rate limit is 5 req/s. The sleep was always shorter than the wait that followed it, so no registration ever left the host at a different time because of it. Noted on QuiteYellow#51 and QuiteYellow#53.
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.
refresh_observes() dropped every OBSERVE registration in a tight loop. Unlike the teardown dereg in close(), which wants out quickly and leaves a session nobody will use again, this one runs against a session that has to keep working afterwards — and an unpaced OBSERVE burst is what wedges an appliance until something forces a new session (LocalThings#396). The two sleeps that stood in for pacing are gone with it. subscribe() paces its own send since QuiteYellow#51, so the 50ms between registrations was always shorter than the wait that followed it — the same redundancy QuiteYellow#59 removed from the bridge's registration loop, at the sibling call site. The 100ms between the two sweeps is subsumed the same way, by the pace inside the first subscribe(). Note this does not fix the connect-time OBSERVE burst in #396 on its own: that path is the bridge's registration loop, which QuiteYellow#51 already paces.
Summary
ping(), and teardown deregistration immediateWhy
The session already spaces Block2 continuations and Block1 uploads, but the first GET, an ordinary POST, and
subscribe()could still be sent back-to-back with earlier traffic.The hardware report in #53 found the missing case: about eleven OBSERVE registrations were sent immediately after the DTLS handshake. The session stayed healthy for roughly five seconds, then the dryer stopped answering for nearly three minutes. Pacing
subscribe()inside the session protects every caller from that burst instead of relying on each caller to remember it.The Home Assistant integration I use with a locally connected Samsung washer and dryer already calls
pace()before initial and refresh subscriptions. A regression here covers that pattern and confirms the session-owned call does not add a second wait once the interval has already elapsed.Fixes #53.
Stack / merge order
mainand should merge first.Validation