protocol/ymodem: keep the TX drain inside the retry, not around it - #138
Conversation
`_send_frame_with_retry` called `flush_input()` and `flush_output()` outside its own try block, so anything they raised escaped the retry loop and propagated out of `RecoverySession.run()`, which wraps the protocol call in `try/finally` with no handler. That was harmless while `flush_output()` was `reset_output_buffer()`, which discards queued bytes and effectively never raises. 52582f0 changed it to wait for the TX queue to drain against a 5 s deadline and raise `TransportTimeout` when it does not — exactly the hung-adapter case the retry loop exists to absorb. On a stalled PL2303/FT232R or with flow control asserted, a burn that previously purged and retried now aborts before writing a single frame. This is the same defect `TestWriteTimeoutRetry` already documents for `transport.write()`, which was moved inside the try for the identical reason. Move both flushes in with it. Verification: uv run pytest tests/test_protocol_standard.py -k "drain or write_timeout" -q The added test fails against 52582f0 (raw TransportTimeout escapes `_send_head`) and passes with the flushes inside the try. Not hardware verified: reproduced against a pyserial-shaped port whose `out_waiting` never reaches zero. On real hardware the divergence needs a link where the frame is still draining when the per-attempt ACK timeout fires.
`_send_packet` called `write()` and `flush_output()` outside its own try block, and the retry loop only caught `YModemError`. A `TransportTimeout` from either therefore escaped the sender, past `_chainload`'s `except YModemError`, and aborted the stock-U-Boot chainload mid-transfer. This is the same defect as the preceding commit: `flush_output()` waits for the TX queue to drain and raises when it does not, so it is an I/O call that belongs inside the retry, not setup around it. `write()` has the same property through its 5 s write_timeout. Move both inside the try and treat a transport stall like an unacknowledged packet, which is what it is — a queue that never drained means the receiver never saw the whole frame. Verification: uv run pytest tests/test_ymodem.py -q The added test drives the real sender against a receiver that leaves stalled packets unacknowledged. It fails on 52582f0 with a raw TransportTimeout and passes with the flush inside the try. `_finish()` still lets a transport error escape, but it has no retry semantics to restore and the installer reports it cleanly through the orchestrator's TransportError handler.
PR Summary by QodoRetry stalled TX drains inside protocol retry loops
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Retrying a stalled drain retransmits a packet whose first copy is still queued, so when the link recovers the receiver sees it twice and answers twice. `_send_packet` consumed one response and left the spare behind, where the next packet's read window picked it up — a NAK the receiver really sent was read as an ACK, and the data it asked to have resent was never retransmitted. Silent truncation of the chainloaded image, not a failed transfer. Flush the receive buffer before every attempt, the way `HiSiliconStandard._send_frame_with_retry` already does, so a response can only ever belong to the attempt that just wrote. `_finish()` gets the same treatment because the last data packet can leave a spare response that would otherwise be read as the EOT answer. Found by review of the previous commit, which introduced the retry that makes duplicate transmission possible. Verification: uv run pytest tests/test_ymodem.py -q The added test NAKs packet 2 after packet 1 stalls and duplicates, then asserts packet 2 is actually retransmitted. It fails against the previous commit and passes here.
|
@openipc-ai , Hardware-tested this on a physical HiSilicon board. Hardware
Tested against current uv run python -m defib burn `
-c hi3516ev200 `
-p COM15 `
-bResult: PASS. Relevant output: There was one initial attempt that failed at: An immediate retry completed the full DDR -> SPL -> U-Boot upload and reached the U-Boot console normally. So the normal HiSilicon This does not specifically reproduce the stalled- |
|
Thank you — that is exactly the check that was missing, and on the right kind of link. The FT232R is a nice accident: that adapter is already on record here for dropping off USB when TX is driven across a PoE energize, which is one of the real routes into the stalled-queue state this PR is about. So the run exercised the normal path on hardware that can actually produce the abnormal one. You are right that it does not reproduce the stalled On the first attempt failing with
|
2 participants
52582f0 (#137) changed
Transport.flush_output()from "discard queued TX" to"wait for queued TX to drain". That is the right semantics — it matches the
documented contract, and
reset_output_buffer()did the opposite of what thedocstring promised. But it also turned a call that effectively never raised
into one that raises
TransportTimeoutafter 5 s, and two retry loops call itfrom outside their own
tryblock.The regression
HiSiliconStandard._send_frame_with_retry:RecoverySession.run()calls the protocol insidetry/finallywith nohandler, and so does
burn, so a raise here propagates out of the command.Measured against a pyserial-shaped port whose
out_waitingnever reacheszero:
On a stalled PL2303 or FT232R, or with hardware flow control asserted, a burn
that previously purged the queue and retried now aborts before writing a
single frame. This is the primary recovery path for all 112 UART chips.
YModemSender._send_packethas the same shape, with a retry loop that onlycatches
YModemError, so a stalled queue aborts a stock-U-Boot chainloadmid-transfer instead of costing one retry.
This is a re-introduction, not a new class of bug
tests/test_protocol_standard.py::TestWriteTimeoutRetryalready existsbecause
transport.write()used to sit outside that sametry. Its docstringnames the same trigger:
That test pins the invariant for
write()only, which is why moving theflushes out of the try went unnoticed.
The fix
Move
flush_input(),flush_output()andwrite()inside the existing tryin both loops. A flush that waits on hardware is an I/O operation, not
bookkeeping, so it belongs where transient failures are already absorbed. The
bounded drain from #137 is kept exactly as merged.
Two regression tests, each in the style of the existing one it sits beside.
Both fail against 52582f0 with a raw
TransportTimeoutand pass with theflushes inside the try.
Follow-up from review (c943b37)
Review caught a real bug in the second commit, and it is worth spelling out
because the fix creates the window itself. Retrying a stalled drain
retransmits a packet whose first copy is still queued, so when the link
recovers the receiver sees it twice and answers twice.
_send_packetconsumedone response and left the spare behind, where the next packet's read window
picked it up: a NAK the receiver really sent was read as an ACK, and the data
it asked to have resent was never retransmitted. That is silent truncation of
the chainloaded image, not a failed transfer.
Reproduced first, then fixed.
_send_packetnow flushes the receive bufferbefore every attempt — which is exactly what
_send_frame_with_retryalreadydoes, and why the same duplicate-transmission window never bites the HiSilicon
path.
_finish()gets the same flush, because the last data packet can alsoleave a spare response that would be read as the EOT answer.
The third test NAKs packet 2 after packet 1 stalls and duplicates, then asserts
packet 2 really is retransmitted. It fails against 9d90a3c and passes on
c943b37.
Testing
Not hardware verified. Reproduced against a port whose
out_waitingneverreaches zero. On real hardware the divergence needs a link where the frame is
still draining when the per-attempt ACK timeout fires, so the natural check is
a
defib burnon any HiSilicon board — which should behave exactly as it doestoday, since the fix only restores the previous behaviour on the stalled path.
Not addressed here
YModemSender._finish()still lets a transport error escape. It has no retrysemantics to restore, and the installer reports it cleanly through the
orchestrator's
TransportErrorhandler, so it is left alone.The remaining
flush_output()call sites invendors/hikvision.pyare allinside deadline-driven loops whose failures reach
install'sexcept (TimeoutError, TransportError)and surface as a normal CLI error.