Skip to content

fix: keep sequence counter in sync with device-initiated packets - #709

Open
atwright147 wants to merge 1 commit into
codetheweb:masterfrom
atwright147:fix/sequence-counter-desync
Open

atwright147 wants to merge 1 commit into
codetheweb:masterfrom
atwright147:fix/sequence-counter-desync

Conversation

@atwright147

Copy link
Copy Markdown

Problem

A Tuya device advances its own sequence number for every packet it sends, including unsolicited ones — most notably the STATUS/DP_REFRESH push a v3.4 device emits immediately after a SET.

TuyAPI only advances _currentSequenceN when it sends a packet. After an unsolicited push the two counters drift, so the client's next request reuses a sequence number the device has already consumed. The device answers with a different number, no resolver matches, and the pending get()/set() never resolves — it hangs until the socket is recycled.

This makes get() immediately after set() hang deterministically on a v3.4 device, even though the SET itself succeeded.

Reproduction (Tuya v3.4 panel heater)

Packet trace from the real device:

connect -> _currentSequenceN = 53896
SET     -> request seq 53897 ; device acks (CONTROL_NEW) seq 53897
(push)  -> device sends STATUS seq 53898 ; client counter stays 53897
GET     -> request seq 53898 ; device replies (DP_QUERY_NEW) seq 53899
           -> `53899 in this._resolvers` is false -> get() hangs

connect -> get() on its own works, because the counters happen to stay aligned. connect -> set -> get() always hangs. Every subsequent get() in that session hangs too, because the offset persists.

The same is visible with shouldWaitForResponse: false: set() returns immediately (fire-and-forget), but the asynchronous push still leaves the counter behind.

Fix

Re-sync _currentSequenceN from each received packet in _packetHandler():

if (packet.sequenceN > this._currentSequenceN) {
  this._currentSequenceN = packet.sequenceN;
}

Only moving it forward keeps it monotonic (an out-of-order or older packet never rewinds it), so the next request uses the sequence number the device will answer with.

This is intentionally placed at the top of _packetHandler() so it also applies to packets handled by the early-return branches (the SET ack, the empty DP_REFRESH response). It's safe for the session-key negotiation because the SESS_KEY_NEG_RES branch that follows rebases the counter with sequenceN - 1 regardless.

Tests

Adds test/sequence.js:

  • re-syncs the counter from an unsolicited device packet
  • does not rewind the counter on an older packet
  • a pending request still resolves after an unsolicited push

Without the fix the first and third fail; with it, the full suite passes (42 tests passed).

Verification

Checked against two ECOSO Torr panel heaters (Tuya v3.4) via a Homey app: get() now succeeds immediately after set(), and repeatedly (previously it timed out after every write).

Notes

  • Related to Fix memory leak: time out unanswered get()/refresh() requests #707, which cleans up resolvers for get()/refresh() requests that are never answered. That mitigates the leak; this change prevents the hang at its source, so the two are complementary.
  • The counter unwind is limited to packets that actually arrive; nothing else changes in the response routing.

The device advances its own sequence number for every packet it sends,
including unsolicited ones — e.g. the STATUS/DP_REFRESH push a v3.4 device
emits immediately after a SET. `_currentSequenceN` was only advanced when *we*
sent a packet, so after such a push the client's next request reused a sequence
number the device had already consumed. The device then answered with a
different number and the resolver registered for the request was never called,
leaving the pending get()/set() unresolved until the socket was recycled.

Reproduced against a Tuya v3.4 panel heater:

  connect -> _currentSequenceN = 53896
  SET     -> request seq 53897 ; device acks seq 53897
  (push)  -> device sends STATUS seq 53898 ; client counter stays 53897
  GET     -> request seq 53898 ; device replies seq 53899
             -> no resolver for 53899 -> get() hangs

Re-sync `_currentSequenceN` from every received packet in `_packetHandler()`.
Only moving it forward keeps it monotonic. This is safe for the session-key
negotiation: the SESS_KEY_NEG_RES branch that follows still rebases the counter
with `sequenceN - 1`.

Adds test/sequence.js covering the re-sync, a no-rewind guard, and the
end-to-end case where a request must still resolve after an unsolicited push.

This branch has not been deployed

No deployments
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.

1 participant