Skip to content

fix(protocol): pace CoAP request sends - #51

Merged
QuiteYellow merged 1 commit into
QuiteYellow:mainfrom
Moballo-LLC:codex/coap-initial-pacing
Aug 21, 2026
Merged

fix(protocol): pace CoAP request sends#51
QuiteYellow merged 1 commit into
QuiteYellow:mainfrom
Moballo-LLC:codex/coap-initial-pacing

Conversation

@Jason-Morcos

@Jason-Morcos Jason-Morcos commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • pace the first Block2 GET request and every later block
  • pace ordinary POST requests and OBSERVE registrations
  • recheck session liveness after pacing and before a request send
  • keep automatic ACKs, ping(), and teardown deregistration immediate

Why

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

  1. fix(protocol): pace CoAP request sends #51 (this PR) is directly on current main and should merge first.
  2. feat(protocol): validate GET and POST request options #48 adds validated query and extension options.
  3. feat(protocol): add CoAP DELETE requests #49 adds DELETE.
  4. feat(protocol): add bounded Block1 POST uploads #50 adds bounded Block1 POST uploads.

Validation

  • 274 SmartThings-Local tests on current dependencies and the Python 3.11 dependency floor
  • 1,625 LocalThings tests against this exact source tree
  • deterministic eleven-registration pacing, caller-side pacing compatibility, liveness cleanup, immediate-response token ordering, and unpaced control-send regressions
  • wheel and sdist content checks plus isolated install/import smoke tests
  • bytecode compilation and share-safety checks

@Jason-Morcos
Jason-Morcos force-pushed the codex/coap-initial-pacing branch from 75282c2 to 0d9d13b Compare August 18, 2026 21:27
@Jason-Morcos Jason-Morcos changed the title fix(protocol): pace initial CoAP requests fix(protocol): pace CoAP request sends Aug 18, 2026
@Jason-Morcos

Copy link
Copy Markdown
Contributor Author

@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).

@QuiteYellow

Copy link
Copy Markdown
Owner

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:

07:33:34  dryer  DTLS connected — subscribing 11 paths
07:33:38  dryer  seeded → 25 links; sensors live
07:33:35  oven   DTLS connected — subscribing 11 paths
07:33:40  oven   seeded → 16 links; sensors live

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: poll_scheduler logs len(indexed) after a single blockwise GET of /device/0, so the tree is one transfer and your change adds one pace interval at its head rather than one per link. Measured: dryer 834ms to 1260ms, oven 2665ms to 2690ms.

This closes #53. The registration burst there is the genuine per-request case, and test_every_subscribe_in_registration_burst_honors_rate_limit walks exactly it.

One thing I am leaving for a separate change: mqtt_demo still sleeps 50ms between subscribes at bridge.py:389. pace() absorbs that rather than adding to it, which is what test_existing_caller_pacing_before_subscribe_does_not_wait_twice protects, so it is redundant but harmless.

Not proven by this run: a refrigerator. #37 stays open until the reporter there confirms on a release.

@QuiteYellow
QuiteYellow merged commit b3045f5 into QuiteYellow:main Aug 21, 2026
8 checks passed
atc722 pushed a commit to atc722/SmartThings-Local that referenced this pull request Aug 23, 2026
_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.
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
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.
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.

OBSERVE subscribe burst bypasses the rate limit and can wedge a device at connect

2 participants