Backport mavftp fixes from AMC

[amilcarlucas]

I would like at some point to get rid of my mavftp.py fork in AMC.

So here are some fixes from that downstream port

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-28)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_02/devcall_pr_reviews.html#prpymavlink_1267 (also in the AIReview report: https://uav.tridgell.net/DevCallReviews/2026_08_28/devcall_pr_reviews.html#prpymavlink_1267)

Reviewed at head 4d754e2050. Verdict: REQUEST CHANGES.

All five backported fixes are protocol/logic-correct (the num_params fix was verified against ArduPilot's AP_Filesystem_Param.cpp packer — note MAVProxy's own param_ftp.py still has the same latent total_params bug, worth a follow-up there). Two things block merge:

Should fix:

• mavftp.py:849,1679 — Pylint CI failure is caused by this PR: R1705 no-else-return at mavftp.py:849 (the new unconditional return MAVFTPReturn("BurstReadFile", FtpError.Success) at mavftp.py:923 makes the following elif op.opcode == OP_Nack: an elif-after-return), and R0911/R0912/R0915 at mavftp.py:1679 (ftp_param_decode now exceeds return/branch/statement limits after the new bounds checks). Needs pylint: disable= additions (the function already carries disable=too-many-locals) or the "el" dropped from the elif, then a re-run. (link)
• mavftp.py:1845-1852, mavftp.py:2276-2297 — CLI exit-status regression: replacing sys.exit(1) with return MAVFTPReturn("GetParams", FtpError.Fail) in decode_and_save_params is the right call for library users, but the standalone main() never converts ret.error_code into a process exit code — it just calls display_message() and falls off the end. So mavftp.py ... getparams out.param on an unreadable/undecodable param.pck previously exited 1 and now exits 0; scripts checking $? lose the failure. Suggest ending main() with sys.exit(0 if ret.error_code == FtpError.Success else 1) (or similar) in this PR, since it introduces the change. (link)

Notes (non-blocking):

• mavftp.py:1690,1764-1766 — num_params vs total_params swap is protocol-correct: in ArduPilot's AP_Filesystem_Param.cpp the pack loop emits exactly hdr.num_params records (total_params - start, capped by count), while total_params is the vehicle-wide total. Comparing the decoded count against num_params makes subset downloads (?start=N&count=M) decode correctly; full downloads (the only thing cmd_getparams requests) are unchanged since the two are then equal. Side note: MAVProxy's MAVProxy/modules/lib/param_ftp.py still checks total_params and has the same latent bug — worth a follow-up PR there.
• mavftp.py:1016-1020 — cmd_put fh_owned fix closes a real fd leak: master never set fh_owned = True for files cmd_put opens itself, so __release_staging (mavftp.py:456-461) never closed them; the explicit False for caller-supplied handles also protects them from being closed by a stale True. Self-contained, uses infrastructure already on master.

[amilcarlucas]

@peterbarker CI is green and the AI review issues are addressed

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_02/devcall_pr_reviews.html#prpymavlink-1267 (also in the AIReview report)

Re-reviewed at head a06da25c4e; my earlier comment above is superseded. Both blocking findings from my previous review are resolved, CI is fully green (21/21), and nothing new turned up: APPROVE — ready to merge.

• Pylint CI failure — resolved (elif→if at mavftp.py:924, widened disable comment at :1679; the previously failing job is green).
• CLI exit-status regression — resolved exactly as suggested (exit_code plumbing at mavftp.py:2281-2300; the malformed-return branches now also exit 1, an improvement).
• num_params/total_params note — still valid as a MAVProxy param_ftp.py follow-up; the subset decode here re-verified empirically (truncated header/record and bogus common_len all return None instead of crashing; full and subset downloads decode correctly).

For the record, a cold Codex pass claimed the new partial-burst-ack return Success (mavftp.py:921-923) could surface as a spurious operation-level success on idle expiry; I traced it and it cannot — arriving burst replies refresh last_burst_read, a stalled burst triggers a re-send in __idle_task which refreshes last_send_time, and the caller timeout overrides ret with RemoteReplyTimeout. One optional hardening from that pass: also require num_params <= total_params in ftp_param_decode (mavftp.py:1687) so impossible headers are rejected.

[peterbarker]

  ⎿  mavftp.py
       ● 2289 [correctness] The new CLI exit status is taken from `ret`, which for `put`/`list` keeps being overwritten for a full idle_detection_time after the transfer already
                            succeeded, so successful uploads can exit 1.
       ●  923 [correctness] An accepted-but-not-completing burst reply now unconditionally returns Success, removing the only "transfer not finished" signal, so a download that stalls
                            and exits via idle detection reports success with no file written.

[amilcarlucas]

I addressed both of your findings

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/2026_09_02/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head fa8c36a583; my earlier comment above is superseded. REQUEST CHANGES — the header-field fix is right and worth having, but the new completion flag introduces a cross-operation false success.

The headline fix is correct

ftp_param_decode() previously compared its decoded record count against total_params. ArduPilot's writer defines the header as {magic, num_params, total_params} where total_params = AP_Param::count_parameters() (every parameter on the vehicle) and num_params is the count in this file (total_params - start, capped by count) — and ArduPilot's own reader iterates hdr.num_params. So any partial or ranged param fetch reported a spurious "bad count" on a perfectly good transfer. Comparing against num_params is right. Verified against both the writer and the reader.

What's blocking

operation_complete is global and not correlated with the operation being awaited. It's a single instance-level boolean — set in the list handler and in __send_more_writes(), cleared at operation start, read once in process_ftp_reply() — with no tie to which operation is outstanding or to a sequence number. The consequence reproduces as: list success → delayed duplicate list EOF → cmd_rm() returns ListDirectory/Success without ever consuming the RemoveFile ACK. It can equally terminate a get, put or pending reset early. Completion wants to be operation-specific and preferably sequence-correlated, with a delayed-EOF regression test.

CI is red on two Python versions — build (3.14) and build (3.9). On 3.14 pylint reports R0912: Too many branches on process_ftp_reply(), which the new completion logic pushed over the limit.

One unguarded decode remains. Structurally valid parameter data carrying a non-UTF-8 name passes the hardened ftp_param_decode() and then raises an uncaught UnicodeDecodeError in extract_params(), escaping the callback instead of returning MAVFTPReturn(Fail). The decoder also doesn't enforce ArduPilot's 16-byte reconstructed-name limit.

Verified good

All direct indexing and struct.unpack() inside the decoder are now bounds-guarded with no unchecked index remaining. The sys.exit(1) calls are gone from the library path — the remaining ones are in the standalone CLI section, which is fine. TerminateSession and ResetSessions do correlate their own replies by sequence; it's only the generic flag that doesn't. And main() now returns a non-zero exit code on failure where it previously always exited 0 — a good change, though scripts checking exit status will start seeing failures they previously missed.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-02)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_02/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 2cd1d203fe (was fa8c36a583); my earlier comment above is superseded. Three of the four are resolved. The completion bug is narrowed but still reproducible, so REQUEST CHANGES stands.

Still open — narrowed, not closed

The correlation is a real improvement: operation_complete is replaced by completed_reply: Optional[Tuple[int,int]] holding (req_opcode, seq), and reply_complete now requires completed_opcode == last_op.opcode and completed_seq == (last_op.seq + 1) % 256 — the same convention the file already uses for pending_terminate_seq and pending_reset_seq. That closes the immediate case.

But ret = self.__mavlink_packet(m) is assigned before the correlation test, and the correlation only gates reply_complete, not ret. So:

1. List request seq 1 → EOF reply (ListDirectory, seq 2) → success.
2. RemoveFile request seq 2.
3. A duplicate (ListDirectory, seq 2) arrives; it sets ret = ListDirectory/Success.
4. The RemoveFile ACK is delayed past idle_detection_time.
5. The loop breaks on idle and returns that stale ret; the ACK is never consumed.

Same user-visible failure as before, one step further in. The new regression places the ACK immediately after the duplicate, so it does not reach this path.

New — both new parameter-name tests pass for the wrong reason

The decoder reads name_len = ((plen >> 4) & 0x0F) + 1 from the high nibble and common_len = plen & 0x0F from the low one. Both new tests put len(name) - 1 in the low nibble:

• test_rejects_non_utf8_name: b"bad\xff" gives plen = 3 → name_len = 1, common_len = 3. With an empty last_name the record is rejected by common_len > len(last_name) before the UTF-8 check runs.
• test_rejects_name_longer_than_16_bytes: its first record has plen = 15 → common_len = 15, rejected the same way, so the second record is never reached.

Both assert None and get None, so they pass while testing nothing they claim to. For no shared prefix the encoding should be (len(name) - 1) << 4 — e.g. 48, giving name_len = 4, common_len = 0.

Resolved

• The decoder now enforces both name constraints: if len(name) > 16 rejects over-long reconstructed names, and name.decode("utf-8") is wrapped in except UnicodeDecodeError, so a non-UTF-8 name returns MAVFTPReturn(Fail) through the reply loop instead of escaping the callback.
• CI is no longer red — pylint passes on 3.9 and 3.14. Worth knowing that R0912 on process_ftp_reply() was silenced with an inline # pylint: disable=too-many-branches rather than restructured; defensible for a protocol dispatcher and consistent with the dozen other suppressions in this file, but it is a suppression. CI overall is still running (7 passing, 13 pending).
• The headline fix is intact — count validation still compares against num_params rather than total_params, matching ArduPilot's AP_Filesystem_Param header and writer.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_02_2258/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 334c1c091c; my earlier comment above is superseded. Both of my previous findings are properly fixed — but the fix introduces three regressions, two of them serious, so REQUEST CHANGES stands.

I measured all three against the merge-base, so these are things this PR changes rather than pre-existing faults.

1. A failed upload now reports success

During a multi-block put, a NACK for any WriteFile other than the one currently in last_op fails both arms of the new gate: reply_matches_last_op is false because last_op has moved on, and the completed_upload arm then passes the wrong result through — the CreateFile success already latched in completed_reply. The handler logs FTP: Write failed and terminates the session, and the caller is told the upload succeeded.

Two-block upload whose first WriteFile is NACKed FileProtected:

	result
merge-base	WriteFile error_code=9 — reports the failure
334c1c091c	CreateFile error_code=0 — reports success

Silent data loss on upload is about the worst failure mode this file has, and firmware and parameter uploads are exactly the multi-block case. The gate needs to let a NACK through regardless of which write it belongs to.

2. Every 256th operation fails at the sequence wrap

The FTP sequence field is 16-bit (struct.pack("<HBBBBBBI", self.seq, ...)) and the server replies with request.seq + 1 — so a request at seq 255 is answered at seq 256. But the client wraps its own counter with self.seq = (self.seq + 1) % 256 at mavftp.py:449, and the new gate tests op.seq == (self.last_op.seq + 1) % 256, which expects 0. The reply isn't credited and the operation returns Fail although the server acknowledged it.

cmd_rm with the client's counter forced:

client seq	server replies	merge-base	334c1c091c
3	4	SUCCESS	SUCCESS
254	255	SUCCESS	SUCCESS
255	256	SUCCESS	FAIL

It worked before because ret was assigned unconditionally and the % 256 convention only gated the terminate and reset handshakes. Extending that convention to every reply is what exposes it. The narrow fix is to compare against the 16-bit value the server will actually send; the better one is to stop wrapping self.seq at 256 when the wire field is 16 bits.

3. A stale reply still re-sends the current request

__mavlink_packet(m) is deliberately called for every reply so handlers keep their state, and only its return is discarded — but the handlers have side effects on shared state. A delayed ListDirectory ACK arriving while RemoveFile is current runs __handle_list_reply(), which does more = self.last_op — the RemoveFile op — mutates its offset and sends it again. Measured: 2 sends for one cmd_rm where there should be 1. The result stayed correct in my case, but a spurious retransmission also advances the sequence counter, which interacts badly with (2). Gating the result without gating the side effects only solves half of what I raised last round.

Lesser point

reply_matches_last_op compares opcode and sequence but not session, so a reply from a stale session that coincidentally matched both would be credited and could return InvalidSession as the operation's result. I tried to reproduce this and my synthetic packet didn't line the sequence up, so I'm reporting the mechanism from the code rather than from a measurement. Adding op.session == self.session to the test costs nothing.

Both previous findings are genuinely resolved

The stale-reply scenario no longer reproduces. At step 3 the duplicate has op.req_opcode == OP_ListDirectory against last_op.opcode == OP_RemoveFile, so the gate is false and ret stays at its initial Fail. The idle break then returns a failure for the operation that actually timed out. The direction is right; it's the breadth of the gate that's wrong.

The regression test now exercises the idle path it claims to — delayed_remove_ack backdates last_send_time by 2 * idle_detection_time so idle detection fires before the ACK is consumed, then asserts FtpError.Fail and that one reply is left unconsumed. That's exactly the shape I described.

Both parameter-name tests now test what they claim. packed_param() uses (len(name) - 1) << 4 and the 16-byte test's first record uses 15 << 4, so b"bad\xff" gives plen = 48 → name_len = 4, common_len = 0 and reaches the UTF-8 rejection. Wrapping them in assertLogs against the specific error text is stronger than what I suggested.

Test coverage

With the PR's mavftp.py: 9 passed. With mavftp.py reverted to the merge-base and the PR's tests kept: 7 failed, 2 passed — so the new tests do discriminate for what they cover. What they don't cover is any of the three regressions above: nothing drives the sequence past 255, nothing NACKs a non-final write, and nothing asserts the send count when a stale reply arrives. All three are straightforward to add to the existing FakeMaster harness.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_03_0136/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 9b13cbb447; my earlier comment above is superseded. Both regressions I blocked on are fixed — I re-ran the same scripts that demonstrated them. Verdict moves REQUEST CHANGES → COMMENT, held back from APPROVE by one lifecycle gap in the new tracking state.

Resolved — the sequence wrap

You took the better of the two options: % 65536 throughout, removing the mismatch with the 16-bit wire field rather than papering over it. My harness across the boundary:

client seq	server replies	before	now
3, 254	4, 255	SUCCESS	SUCCESS
255	256	FAIL	SUCCESS
256, 65534	257, 65535	—	SUCCESS
65535	0	—	SUCCESS at the new wrap point

Resolved — the swallowed upload error

pending_write_replies: Dict[int, int] maps each expected reply sequence to its requested offset, so a reply is matched to the request that produced it rather than to whatever is current. My two-block upload with the first WriteFile NACKed FileProtected now returns WriteFile error_code=9 — matching the merge-base — where at the previous head it returned CreateFile error_code=0.

Resolved — the stale-reply re-send, and the session check

The delayed ListDirectory ACK during a RemoveFile measured 2 sends for one cmd_rm before and 1 now. And test_wrong_session_reply_is_not_retained covers the session point I could only report from the code last time.

The suite goes 9 → 17 tests plus 2 subtests. Reverting mavftp.py to the previous head while keeping the new tests gives 9 failed, 10 passed, so every new test is tied to a real change rather than passing vacuously. Thank you for adding one per finding — that made this round quick to check.

Open — the new maps are not cleared on timeout or retransmission

pending_read_replies is emptied in exactly one place, __terminate_session() at mavftp.py:518, plus a targeted pop() on a reply (line 989) and a prune on gap completion (line 1002). Neither timeout exit in process_ftp_reply() (lines 1717–1727) calls __terminate_session() — they break and return. And a retried gap send adds a new sequence key at line 464 without removing the one it supersedes.

So entries accumulate one per retry and survive an operation that times out, in a keyspace that now wraps at 65536. pending_burst_offset is scalar but has the same gap. Clearing both on the timeout paths, and replacing rather than adding on a retransmission, closes it and the unbounded-growth question together.

The second pass reports reproducing a concrete consequence — a surviving ReadFile NACK matching during a later unrelated cmd_rm() and terminating that session. I verified the lifecycle gap directly but not that particular outcome, so treat the mechanism as confirmed and the consequence as reported.

Three more from the cross-check, not independently reproduced here

1. An active burst reply can pass the gate while last_op is a gap ReadFile; the burst handler then takes more = self.last_op, changes its offset and resends it — the same shape as the stale-list bug that is fixed, on a path the fix doesn't cover.
2. The dedicated TerminateSession wait bypasses the session and MAVLink-target validation that normal dispatch now performs.
3. A successful out-of-order final gap reply isn't retained unless it matches last_op, so a read can complete with correct data and still return FtpError.Fail.

None of the three is covered by the new suite.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_03_0512/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 8ab6106014; my earlier comment above is superseded. My open finding is fully fixed, and two of the three I passed on are too. Verdict stays COMMENT for the third, which I'd reported without reproducing last time and have now traced myself.

Resolved — both halves of the pending-state leak

The retransmission half is fixed at the source: __send() takes retry: bool = False and, when set, preserves the sequence number rather than allocating a new one — so a retry overwrites its map entry instead of adding one. Neater than clearing up afterwards.

The timeout half is fixed by a new post-loop block in process_ftp_reply() (mavftp.py:1793-1798): on a RemoteReplyTimeout with an active session it calls __terminate_session(), which is where all the maps are cleared. Measured directly — after a timed-out cmd_get: pending_read_requests=0 pending_read_replies=0 pending_write_requests=0 pending_write_replies=0 burst_req=None burst_ofs=None.

Resolved — the burst re-send and the TerminateSession validation

Two of the three I passed on without reproducing. The burst handler now uses pending_burst_request instead of self.last_op, so an active burst continuation arriving while last_op is a gap ReadFile re-sends BurstReadFile at the right offset rather than mutating the gap request. And the dedicated terminate loop now validates MAVLink target system and component, session, request opcode and sequence.

Still open — a complete, correct read can still report failure

The third one. ret is initialised to MAVFTPReturn(operation_name, FtpError.Fail) at mavftp.py:1668, and there are exactly two places it can become anything else: the TerminateSession branch at :1710, and ret = packet_ret at :1747. That second one is gated on:

reply_matches_last_op
or completed_upload
or (reply_matches_active_request and packet_ret.error_code != FtpError.Success)

So a reply matching an active request rather than last_op is retained only when it is a failure. A successful out-of-order final gap reply is discarded; read_complete then sets reply_complete and breaks with no assignment to ret; and the operation returns the initial Fail although the data is complete and correct.

The second pass reproduced the user-visible outcome — two gaps with reordered replies, all 240 bytes correct, gaps empty, read_complete=True, result BurstReadFile/FtpError.Fail. Retaining a success from an active request the same way failures are retained would close it.

Test coverage, measured both ways

The suite goes 17 → 28 tests plus 4 subtests, all passing. Reverting mavftp.py to the previous head while keeping the new tests gives 13 failed, 17 passed, so every new test discriminates. There's one named for each item — test_read_timeout_terminates_active_session, test_process_timeout_terminates_active_session, test_write_retry_reuses_request_sequence (plus gap-read and open variants), test_terminate_ignores_reply_for_wrong_target_or_session — plus extras on session allocation and param-count validation. Thank you; that made this round quick to check.

The gap is the open finding above: nothing asserts the final process_ftp_reply() result for an out-of-order successful gap reply, which is why it survived.

[tridge]

Deprecated — see below for the updated review.

Previous review (head 72261c7)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_03_2258/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 72261c7cae; my earlier comment above is superseded. The finding I have raised four rounds running is fixed — thank you. Two new bugs in the synchronous range read this head also adds move the verdict to REQUEST CHANGES, which is about that new code, not about the fix.

Resolved — and pinned by a test

The gate at mavftp.py:1820-1825 is now just reply_matches_last_op or completed_upload or reply_matches_active_request. The and packet_ret.error_code != FtpError.Success clause that discarded a successful out-of-order final gap reply is gone.

Verified by reverting it. At this head: 37 passed + 11 subtests. Reverting only that gate to its previous form: 1 failed, 36 passed — and the failure is test_out_of_order_final_gap_reply_reports_success with AssertionError: <FtpError.Fail: 1> != <FtpError.Success: 0>, exactly the symptom from each previous round. The regressions an earlier cross-check worried the fix might reintroduce are covered by name and pass: test_stale_write_reply_is_discarded, test_noncurrent_write_nack_fails_upload, test_stale_list_ack_does_not_resend_remove, test_terminate_ignores_reply_for_wrong_target_or_session.

The other two minor items are resolved too — the test now advertises [4, 0, 0, 0] to match b"data", and the line I called dead now sits in a real elif self.read_to_memory: branch, which is better than deleting it.

Two bugs in the new synchronous range read

Both are in the read_to_memory path added by "preserve synchronous read ranges". They matter because read_sector() is not dormant — mavftpfs.py:61 calls it for every FUSE read.

1. A satisfied small range read never stops, and keeps requesting to EOF.

The completion test self.reached_eof or self.read_total >= self.requested_size exists at mavftp.py:878 inside __check_read_finished() — but the normal in-order path never reaches it. In __handle_burst_read() the catch-up branch calls it at line 1009 and the EOF sub-branch at 1045, but a full-sized burst_complete ACK falls straight to lines 1049-1057, which sets more.offset = op.offset + op.size and sends another burst with no size check. __check_read_finished() is called only from the packet handlers (1009, 1045, 1085, 1132), not from __idle_task(), so nothing rescues it later.

Reproduced: a 2-byte request answered by one 80-byte ACK leaves done=False, read_complete=False, get_result=None, and sends a further burst at offset 83. A small FUSE read therefore streams from the requested offset to EOF, or times out on a large file.

2. Memory scales with the read offset rather than the read size — 256 MiB for a 2-byte read.

__write_payload() seeks to the absolute remote offset — that is unchanged from master. What changed is what self.fh is. On master, line 695 uses an in-memory SIO() only for callback or filename == "-"; a range read went to a mkstemp staging file, where seeking past the end makes a sparse hole costing neither RAM nor disk. This head adds or self.read_to_memory to that condition (line 817) and seeks the fresh SIO() to requested_offset at line 820 — turning that free hole into a real zero-filled allocation.

Measured with tracemalloc: BytesIO.seek(n) then a 2-byte write peaks at 1.0 MiB for n = 2^20 and 256 MiB for n = 2^28. So a small FUSE read near the end of a few-hundred-MB log allocates the whole prefix.

The buffer is offset-indexed because line 928 slices result[requested_offset : requested_offset + requested_size]. Writing at op.offset - requested_offset and slicing from 0 would make memory proportional to the request.

Checked and clear

rsplit("\t", 1) is a real improvement — I ran the cases, and a filename containing a tab now parses correctly where split("\t") would have raised on unpacking; a no-tab entry is caught; an empty size still hits the existing int() guard. The read_to_memory flag's lifetime is properly bounded: set in read() (654), cleared in __init__ (403), __terminate_session() (512) and cmd_get() (791), so it cannot leak into a following download. And both logging fixes were real — %u against a list raises TypeError.

An independent pass reached the same two bugs and the same verdict, having run the full suite (95 passed, 8 skipped, 11 subtests). CI here is 16 pending.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_04_0458/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head 48efecda36; my earlier comment above is superseded. Both bugs from the last round are fixed — APPROVE.

The completion check now runs on every payload write

Two lines in exactly the right place: mavftp.py:1026-1027 calls __check_read_finished() straight after __write_payload() and before the if op.burst_complete: block, so the full-sized burst path that previously fell through to sending another burst is caught first.

Verified by reverting it. 40 passed + 11 subtests at this head; reverting only those two lines gives 1 failed, 39 passed, and the failure is test_read_sector_stops_after_requested_range_in_full_burst — named for exactly the case. An independent pass ran the same mutation plus four others (removing the offset subtraction, removing position translation, restoring the open seek, restoring absolute final slicing) and each failed as it should.

Memory is bounded by the request, not the offset

Three coordinated changes, all needed: the fh.seek(self.requested_offset) at open is gone; __write_payload computes write_offset = op.offset - self.requested_offset when read_to_memory; and the extraction becomes result[:self.requested_size] for that path while keeping the absolute slice for the others.

Measured with tracemalloc, a 2-byte read:

offset 2^20   before  1.0 MiB      now  0.0 MiB  (buffer 2 bytes)
offset 2^28   before  256.0 MiB    now  0.0 MiB  (buffer 2 bytes)
offset 2^30   before  1024.0 MiB   now  0.0 MiB  (buffer 2 bytes)

The scoping is right — every translation is guarded by read_to_memory, so the callback and filename == "-" paths keep absolute addressing. The independent pass confirmed that separately and measured an end-to-end peak of 5,939 bytes at offset 2^28 returning the correct two bytes.

Checked

Accounting stays consistent across the two addressing modes, which was worth confirming given one buffer is now offset-relative and others absolute: read_total counts received payload bytes, so an 80-byte ACK against a 2-byte request records 80 while get_result is trimmed to 2; gap tracking stays absolute and produced the correct contiguous result at a non-zero offset. Neither pass found a new accounting bug.

Full suite 98 passed / 8 skipped / 11 subtests. Both reviewers reached APPROVE independently. CI here is 16 pending.

[amilcarlucas]

@peterbarker This is good to go as soon as the tests pass. Regarding API changes, there are mostly behavioral/protocol changes rather than signature changes:

• read()/read_sector() now return exactly the requested range and no longer create a local file named after the remote path (mavftp.py:648).
• Download callbacks can now propagate MAVFTPReturn failures or exceptions; cmd_getparams() returns Fail instead of calling sys.exit(1) on decode/read errors (mavftp.py:869, mavftp.py:2142).
• cmd_set() and cmd_put() now reject invalid settings instead of coercing/accepting them (mavftp.py:704).
• save_params() now expects integer datatype IDs rather than strings (mavftp.py:2110).
• The CLI now exits nonzero when an FTP command fails.
• MAVFTP sequence numbers now wrap at 65,536 instead of 256, retries reuse sequence numbers, and replies are more strictly correlated. This is wire-protocol behavior, not a Python signature change.
• Error results are more specific, preserving server NACK codes in several cases.

No public cmd_*, read, or process_ftp_reply method signatures were added or removed. The targeted local tests passed: 38 tests.

[amilcarlucas]

Each of the 23 commits fixes a different issue.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_04_2335/devcall_pr_reviews.html#prpymavlink-1267

Re-reviewed at head c4a222c2d9; my earlier comment above is superseded. COMMENT, down from APPROVE — not because of anything in the new commits, but because a cold pass (which had none of my findings to anchor on) surfaced something my earlier clearance missed.

The pylint commit is exactly what it claims

Parsing mavftp.py at 48efecda36 and e589f711ab and diffing the ast.dump output gives 16184 AST node lines at both heads, zero differing lines — no behaviour change. And it maps one-to-one onto the CI failure: the old build (3.14) job exited 24 on exactly R0916 at :746, R0911 at :704, R0912 at :869, and C0302 in the test module, and the commit adds a disable for each. The previous round's two fixes were re-verified at the new head and both still hold.

Pushed again mid-run, and one of them fixes a real bug

Three more commits landed at c4a222c2d9 while this review was running; the review was re-based onto them rather than posted against a stale head.

• "reset range-read state for downloads" adds self.requested_offset = 0 / self.requested_size = 0 to cmd_get() with a regression test. That closes the stale-offset defect a cold pass raised against the stacked mavftp: add Python 3.8 annotations and validate transfer settings #1274 and which I'd determined was pre-existing here — fixed in the right place.
• "correct rename destination argument" is a bigger fix than its message suggests. rename declared its second positional with dest="new_remote_path" while every other subcommand uses "arg2" with a per-command metavar. Since the dispatcher reads if "arg2" in args and args.arg2, the destination was never appended — rename silently dropped its second argument. Worth a line in the PR body.
• The third adds *ProfiCNC* and *Matek* to preferred_ports, consistent with the existing list.

What I missed last round

save_params() changes a public signature and nothing says so. On master it's save_params(pdict: Dict[str, Tuple[float, str]], ...) with string type keys; here it's Dict[str, Tuple[float, int]]. It's a @staticmethod on MAVFTP, so it's an entry point external code calls, and pymavlink is embedded very widely. A cold pass called it the old way — {"P": (1.0, "4")} with datatype comments enabled — and got malformed output rather than an error, which is the worst failure mode for a type change. I verified the signatures at both revisions. The PR body is three lines and doesn't mention it.

This is exactly the blind spot a cold review exists for: my previous round returned APPROVE, so a findings-validation pass had nothing to check and would never have looked here.

Burst replies are correlated without their sequence number. In __reply_matches_active_request() the OP_BurstReadFile branch returns true on pending_burst_offset is not None and op.offset >= pending_burst_offset alone, while the OP_ReadFile and OP_WriteFile branches both key on op.seq. Sessions are reusable and PX4 always uses session 0, so the asymmetry deserves a deliberate answer rather than being an accident. The rest of the state machine traced clean on both passes — retries preserve the original sequence, new requests wrap modulo 65536, pipelined reads and writes use sequence-keyed maps, and only a successful open or create may allocate a session.

One thing I'm not carrying across: a cold pass reported that passing a callback to cmd_get() suppresses writing the requested local filename. The behaviour is real but pre-existing — master already has if self.callback is not None or self.filename == "-" selecting an in-memory buffer; this PR only appends or self.read_to_memory. Also pre-existing, mentioned so nobody re-finds it: after cmd_put() opens an internally owned local file, a non-ASCII remote name raises UnicodeEncodeError before any cleanup, leaving the handle open.

Declared Python floor is 3.9, all three changed files compile there, and the suite passes 40 tests plus 11 subtests.

[tridge]

Note

Superseded. This review was for head ca69138. The current review is at
#1267 (comment) (head f8cf52a),
which raises the verdict from REQUEST CHANGES to COMMENT.

Original review (superseded, kept for reference)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Reviewed at head ca69138.

Full report, with what was checked and the carried-over context: https://uav.tridgell.net/DevCallReviews/2026_09_05/devcall_pr_reviews.html#prpymavlink-1267

REQUEST CHANGES

Narrowly, and only about the last commit. Both findings from my previous round are properly fixed and I verified both by measurement. But ca691388a3 makes pending_burst_seq a ratchet that advances on every accepted reply rather than a floor for the outstanding request, and that changes behaviour on exactly the lossy/reordering links the burst+gap machinery exists for. Data integrity survives in both cases (gap repair recovers), so this is a robustness/throughput regression rather than a correctness one — and the fix is deleting six lines.

The ratchet

__send() sets the floor correctly. The problem is __handle_burst_read() at mavftp.py:1001-1006: every accepted reply moves the expectation to its own successor, and __reply_matches_active_request() then drops anything behind it before dispatch.

Measured with your own FakeMaster/ftp_reply harness, driving __mavlink_packet() directly, head vs parent c4a222c2a9:

Case A - reordering inside one burst (replies delivered 2, 4, 3):
  parent : late seq 3 Success -> gap filled, read_gaps=[], read completes
  head   : late seq 3 dropped -> read_gaps=[(80, 80)]

Case B - straggler after a burst retransmission:
  head   : stale seq+40 accepted, pending_burst_seq jumps to 42,
           genuine seq+2 and seq+3 both dropped
           read_total 160   (parent: 320)

Why a straggler can be so far ahead: ArduPilot answers one burst request with up to transfer_size = 2000 replies (GCS_FTP.cpp:608), incrementing reply.seq_number per packet from request.seq_number + 1, while the client's own counter advances by exactly 1. So any in-flight packet from a superseded stream is almost always numerically ahead of the floor, and satisfies the op.offset >= pending_burst_offset half of the gate too. __idle_task() re-arms the floor on the next retry, so this is a repeating stall rather than a deadlock — but each recovery can be re-poisoned while the link queue drains.

Suggested fix, verified: delete mavftp.py:1001-1006 and leave pending_burst_seq as the floor __send() established. I applied exactly that and re-ran: full suite still 42 passed + 13 subtests, including the new test_stale_burst_reply_sequence_is_discarded_for_reused_session (which still discriminates: floor 11, stale reply seq 10 → rejected) and test_out_of_order_burst_reply_is_dispatched. Both cases above return to parent behaviour.

Neither case has a test at this head. If you take the fix, one test each would be worth having — a burst ack pair delivered out of order asserting no gap remains, and a post-retry straggler asserting the restarted stream is still accepted.

Minor, and it disappears with that fix: the pending_burst_seq is None reconstruction at :1001-1004 is unreachable in production. __handle_burst_read() has one production caller and that path already returned earlier when pending_burst_seq is None; the branch exists only to keep three direct-call tests working.

Previous round — triage

1. save_params() changed a public signature — RESOLVED by 4297fcfa19. Measured: with d="4" the parent raised KeyError: '4', this head emits P 1.000000 # 32-bit float. Note master itself was broken here (int ptype against a string-keyed table), so this is a fix rather than a restoration.
2. Burst replies correlated without their sequence — ADDRESSED, which is what I asked for; the ratchet above is the residue.

Carried over so nobody re-finds it

cmd_put with a non-ASCII remote name leaks the handle: enc_fname = bytearray(self.filename, "ascii") at mavftp.py:1246 runs after self.fh is opened, so the UnicodeEncodeError escapes with the file handle open and write_list populated, and the next cmd_put() returns PutAlreadyInProgress. Measured identically at master, at the parent and at this head — pre-existing and out of scope, but cheap to fix here.

The PR body still doesn't record the behaviour changes. Your 2026-09-03 comment lists them well; folding it into the description would make it survive into the merge commit. Two worth adding that aren't in that list: rename never passed its destination before 5cc87a9f86, and main() now ends in sys.exit(exit_code), so scripts checking $? will start seeing failures they previously missed.

I also checked this against ArduPilot #34296 (the server-side stale-session fix, in flight now): it touches only session validation, not reply sequence numbering or the burst loop, so the two do not conflict — and pymavlink still needs the client side for older firmware and for PX4. I verified the sequence semantics against ArduPilot's GCS_FTP.cpp only, so the shape of this on PX4 is unverified.

An independent cold review, given only the PR number, flagged the same burst-correlation change.

[tridge]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head f8cf52ae2f. My earlier comment above is superseded.

Follow-up report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_05_2045/devcall_pr_reviews.html#prpymavlink-1267

COMMENT — up from REQUEST CHANGES

The blocker is deleted exactly as proposed and both regression cases now behave like the parent. But the fix I recommended introduced a new boundary, and that one is on me.

Previous round

The ratchet is gone — RESOLVED, verified by execution. pending_burst_seq is now written only by __send() (mavftp.py:474) and cleared at :539-541, :1071-1073, :1112-1114. Re-running my two measured cases against the real package: replies delivered 2, 4, 3 in one burst now all return Success with the floor unchanged and read_gaps going [] → [(80,80)] → [], 240 bytes assembled; and a straggler at seq 42 followed by a restarted seq 2 both succeed with read_gaps=[], where before the floor jumped to 43 and the genuine replies were dropped.

Both tests I asked for exist and are RED without the fix. test_out_of_order_replies_in_one_burst_fill_the_gap (tests/test_mavftp.py:684) and test_retry_straggler_does_not_block_restarted_burst (:717). Re-injecting the exact ratchet makes both fail (43 passed, 2 failed); unmodified, 45 passed + 13 subtests. Separately, deleting the __seq_is_at_or_after term from :1526 fails test_stale_burst_reply_sequence_is_discarded_for_reused_session and nothing else — so the PR's stated purpose survives the fix.

The unreachable pending_burst_seq is None reconstruction went with it, and save_params() stays fixed.

My "PX4 semantics unverified" caveat — now verified, and it holds. PX4's mavlink_ftp.cpp numbers burst replies identically to ArduPilot: _workBurst() sets stream_seq_number = payload->seq_number + 1 (:618) and the stream loop writes then increments it (:1081, :1087), with the generic seq_number++ at :241 suppressed for a successful burst by the guard at :271. The floor gate is wire-compatible with PX4 too.

New — and it comes from the fix I asked for

With the floor now fixed for the whole burst, a burst longer than 32,767 replies rejects its own tail (mavftp.py:1526, :1546). __seq_is_at_or_after is ((seq - expected) & 0xFFFF) < 0x8000, and expected is armed once per burst request and never advanced. Once a reply is more than half the uint16 space ahead, it reads as older:

pending_burst_seq armed at 5, never advanced:
   reply #32766  seq=32771  accepted
   reply #32767  seq=32772  accepted
   reply #32768  seq=32773  REJECTED
   reply #40000  seq=40005  REJECTED

The previous head advanced the floor per reply, so this could not arise. It is a regression against the parent, introduced by removing the six lines I asked to have removed — an independent cold review found it, and I reproduced the boundary.

Reachability, which is why this is not a blocker. ArduPilot answers one burst request with at most transfer_size = 2000 replies and then sets burst_complete (GCS_FTP.cpp:609, :611, :633), and the client's next burst request re-arms the floor via __send(). 2,000 ≪ 32,768, so against ArduPilot firmware the boundary is unreachable. PX4 is the case to watch: it sets burst_complete only when the TX buffer is nearly full and more than 35,000 bytes have gone out in the chunk (mavlink_ftp.cpp:1133-1141), so on a link fast enough that the buffer never drains low it streams to EOF in one burst — and at the 239-byte maximum payload, 32,768 replies is about 7.8 MB. A large log over USB or UDP is exactly that shape. The failure mode is a stalled download reported as RemoteReplyTimeout, not corruption.

A fix that keeps both properties. The point of removing the ratchet was to tolerate reordering, so the floor should trail rather than track. Advance it to (seq - WINDOW) & 0xFFFF only once a reply is more than WINDOW ahead:

burst length   fixed floor (head)   trailing window (WINDOW=4096)
     2,000            2,000                 2,000
    32,767           32,767                32,767
    32,768           32,767                32,768
   100,000           65,535               100,000
   200,000          101,696               200,000

Anything up to WINDOW behind the newest is still accepted, so the out-of-order tolerance you just gained is untouched. A regression test crossing 32,768 in one burst would pin it.

The OverflowError addition at mavftp.py:726 is correct and minimal. float(10**1000) raises OverflowError, which the old tuple did not catch; removing just that name makes test_cmd_set_rejects_an_integer_too_large_for_float fail. float("1e400") → inf is still caught one line later by math.isfinite. No gap.

Minor: both new burst tests seed pending_burst_seq/pending_burst_request by hand rather than driving __send(), so they pin the gate but not the floor __send() establishes. The existing stale-session test covers that path, so it is a nitpick.

Carried over

cmd_put with a non-ASCII remote name still leaks the handle — enc_fname = bytearray(self.filename, "ascii") is now at mavftp.py:1240, still after self.fh is opened. Pre-existing at master and out of scope; note #1274 fixes exactly this shape for the upload path.

The PR body still records none of the behaviour changes. It is unchanged at three lines. The rename destination fix and the new sys.exit(exit_code) at :2633 — which will make scripts checking $? start seeing failures they previously missed — still aren't written anywhere that survives into the merge commit. 4d9e8976e7 also has an empty body, so the reasoning for the burst fix isn't recorded either.

Thread and CI

@peterbarker's CHANGES_REQUESTED from 2026-08-31 is stale and still blocking. Both of its findings look addressed at this head, with tests: "stalled download reports success" by process_ftp_reply returning RemoteReplyTimeout (:1897-1899, asserted by test_incomplete_burst_read_reports_timeout_on_idle), and "successful put/list exits 1" by the completion scoping at :1872-1888. It needs re-reviewing or dismissing.

CI: 21 of 21 passing, nothing pending. Locally at this head: 45 passed, 13 subtests passed. An independent cold review returned REQUEST CHANGES on the burst boundary above; I adopted the finding and not the severity, for the reachability reason given.