Repository navigation
fix: harden sequence matching and prevent resolver leaks - #710
Open
atwright147 wants to merge 2 commits into
Open
atwright147 wants to merge 2 commits into
atwright147 wants to merge 2 commits into
Conversation
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.
- record request command byte on resolver so replies can be matched when sequence numbers drift - match DP_QUERY/DP_QUERY_NEW replies by command byte (newest outstanding) to tolerate off-by-one/number collisions - drain resolvers on CONTROL/CONTROL_NEW ack (prevents set() leak) - guard exact-seq matching so a query resolver isn't mis-resolved by an unrelated packet - add _newestResolverFor helper These fix the case where the device doesn't echo our sequence numbers (replies use its own counter) and prevent stale resolvers from being triggered.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The existing PR #709 fixes the forward-sync from received packets. However, on v3.4 devices the device numbers its replies from its own counter (does not echo ours), so replies can arrive on a different sequence number than registered or collide with other pending requests. This causes:
This PR hardens _packetHandler/_send:
These are backwards compatible and only affect matching behaviour when sequence numbers drift/collide. They fix the rapid-fire SET->GET case we reproduced on real ECOSO Torr heaters.