fix(mavftp): harden FTP reply handling and validate transfer settings by amilcarlucas · Pull Request #1274 · ArduPilot/pymavlink

amilcarlucas · 2026-09-03T23:09:42Z

requires #1267

Capture a MAVFTPReturn failure from a download callback and return it from\nthe reply loop.\n\nThis lets callers of cmd_getparams detect malformed packed parameter data\ninstead of reporting a successful transfer after the callback has rejected\nthe payload. The callback result is cleared for every new download so a\nprior failure cannot affect a later operation. finish termination before returning callback errors return packed-parameter decode failures consume reported callback failures fix(mavftp): skip rejected download output

A successful BurstReadFile reply can contain only part of a download.\n\nReturn success after processing every ACK, including replies that request\nthe next burst. This prevents a valid partial reply from being reported as\na transfer failure on slow telemetry links, while preserving EOF and gap\nrecovery handling. fix(mavftp): safely decode malformed burst nacks

@PARAM

Validate each packed-parameter record before slicing or unpacking it.\n\nMalformed @PARAM responses can end in a partial header or value, or claim a\nshared name prefix longer than the prior parameter name. Report these cases\nas decode failures rather than raising struct errors or producing corrupted\nparameter names.

Validate decoded packed-parameter records against the transmitted num_params\nheader field.\n\ntotal_params describes the controller-wide parameter count and can be larger\nthan a valid subset response. Using num_params accepts those subset downloads\nwhile still rejecting incomplete or overlong payloads.

Mark file handles opened internally by cmd_put as MAVFTP-owned.\n\nThe existing staging-resource cleanup then closes those handles when the FTP\nsession ends, preventing descriptor leaks and file-lock problems on repeated\nuploads. Handles supplied through cmd_put's fh argument remain caller-owned\nand are left open.

Replace the instance-wide operation_complete flag with a completion record containing the request opcode and reply sequence. This prevents delayed or duplicated replies from a previous operation from completing the command currently being awaited. Add a regression test covering a delayed ListDirectory EOF arriving before a RemoveFile acknowledgement.

Adopt the session IDs returned by OpenFileRO and CreateFile before issuing follow-up requests. Accept those allocation ACKs even though their session differs from the request session. Retain in-flight ReadFile, BurstReadFile, and WriteFile requests so timeout retries resend the original request with its original sequence number. Return decoded NACK errors for gap reads and writes instead of reporting success or FileProtected. Terminate active remote file sessions when either download loop times out. Add regression coverage for session allocation, NACK propagation, retransmission sequence reuse, and both timeout cleanup paths.

The parameter decoder imported typing.Tuple and typing.Dict for annotations but called them as constructors at runtime. This broke getparams on supported Python versions after a successful FTP transfer. Use the built-in tuple and dict constructors for sorting and rebuilding decoded parameter mappings.

Treat callback-owned downloads as fully consumed by the callback and skip the generic publish step. This prevents virtual MAVFTP paths such as @PARAM/param.pck?withdefaults=1 from being treated as local files. Add regression coverage for successful callbacks.

Keep a reply result when it matches an in-flight request, including successful replies. A final out-of-order gap ReadFile ACK can complete a download after a later BurstReadFile becomes last_op; previously the data was correct but process_ftp_reply() returned Fail. Add regression coverage for this completion path, stabilize the mocked read-timeout clock, and log the number of remaining gaps correctly.

The callback path already clears publish_result before invoking the callback, making the failure-branch assignment redundant. Remove the dead assignment while preserving callback failure propagation and the guarantee that callback-owned downloads are not published as files. Document the callback success and failure regression coverage, and make the successful callback fixture advertise the exact four-byte payload it consumes.

Keep read_sector() downloads in memory, retain the caller's requested size, and start BurstReadFile at the requested offset. This prevents FUSE reads from downloading the whole remote file, returning an oversized range, or publishing a local file named after the remote path. Add regression coverage for offset reads, returned range length, and absence of local output.

Use integer parameter type IDs in save_params(), matching the values returned by ftp_param_decode() and extract_params(). This makes the getparams datatype-comment option usable for valid parameter files. Add regression coverage for the emitted float datatype comment.

Return a ReadFile failure after terminating a session for a short acknowledgement that does not satisfy an outstanding gap. This prevents the reply loop from reporting a completed download after a file-size race or malformed reply. Add regression coverage for the unexpected short gap-ACK path.

Catch download callback exceptions, retain an FTP failure result, and continue through the normal session cleanup path. This also makes getparams output failures fail the command rather than leaking the active FTP session. Add regression coverage for callback exceptions during download completion.

Reject unsafe command settings before they can violate retry invariants, stall transfer queues, divide by zero, or exceed the MAVFTP payload limit. Defensively validate upload write sizing for callers that set settings directly. Add regressions for invalid command settings and direct invalid upload block sizes.

Validate file listing entries before splitting their name and size fields. Malformed server data now returns InvalidDataSize instead of raising ValueError from the reply-processing loop. Add regression coverage for a file entry without a size separator.

Add an executable MAVFTP integration script for replaying operations against a connected flight controller. The test verifies heartbeat communication and exercises status, configuration, cancellation, listing, upload, CRC, download, rename, removal, directory creation/removal, and parameter retrieval. Temporary remote paths are unique per run and cleaned up on completion or failure.

Complete synchronous range reads as soon as the requested bytes and any gaps are satisfied, even when the reply is a full-sized burst.\n\nAdd a regression covering a small request fulfilled by a full burst so the client terminates instead of requesting data through EOF.

Store synchronous range-read payloads relative to the requested offset so a small read does not allocate a buffer proportional to the remote file offset.\n\nReturn the in-memory range directly from the compact buffer and add a regression covering a one-megabyte offset with a two-byte read.

Store synchronous range-read payloads relative to the requested offset so small reads do not allocate memory proportional to the remote offset. Keep remote and buffer positions distinct while handling burst gaps, retries, and completion, and add a regression for a two-byte read at a one-megabyte offset.

Exercise directory creation/removal and synchronous range reads using the crafted uploaded file, including a small full-burst request and a high-offset request. Reinitialize the MAVFTP connection before rename/delete to avoid late range-read termination replies interfering with subsequent mutations. Document the controller firmware limitation when that handshake remains incomplete.

tridge · 2026-09-04T10:09:44Z

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_04/devcall_pr_reviews.html#prpymavlink-1274

Reviewed at head f0e7942918. REQUEST CHANGES. CI is deterministically red for three reasons this PR introduces, and separately I'd ask you to retitle it.

The title is the main finding

It's called "Ruff lint annotations" with a one-line body, and a reviewer skimming it would reasonably treat it as cosmetic. I measured rather than assumed: parsing mavftp.py at base and head, stripping every annotation (AnnAssign, argument, return) and all docstrings, and diffing the ast.dump output still gives −801 / +3070 node lines, 13268 → 15537 — a 17% growth in code structure with typing removed from consideration. A second pass reproduced the measurement.

Six methods added, none removed: __has_active_session, __read_position, __reply_matches_active_request, __seek_read_position, get_setting, has_setting. __reply_matches_active_request is FTP protocol reply-matching logic. Alongside it: new math.isfinite() and range validation of settings (retry_time uses <= against the minimum where every other setting uses <), a new read_to_memory flag that widens the download-completion condition, a new publish_result gate, and a new broad except Exception around a callback.

None of that is necessarily wrong — but it's a MAVFTP correctness rewrite and deserves to be reviewed as one. Please retitle and describe it, or split the behavioural work out.

CI: three deterministic pylint messages, all introduced here

build (3.9) and build (3.14) both exit 16 (pylint's convention-only bit), score 9.99/10, on exactly:

mavftp.py:53:0 C0413 wrong-import-position (from pymavlink import mavutil)
mavftp.py:57:0 C0413 wrong-import-position (the pymavlink.mavftp_op import)
tests/test_mavftp.py:55:0 C0302 too-many-lines (1150/1000)

The two C0413s come from putting the MavlinkObject/Callback/FileHandle aliases above those imports; the # noqa: E402 you added silences ruff but pylint has no suppression. The C0302 is the test module going 229 → 1150 lines (+930/−9). Moving the aliases below the import block fixes the first two; splitting the regression suite into focused modules is nicer than a file-level disable for the third.

Annotations that are wrong

Since typing is the stated purpose, these matter on their own terms:

MAVFTPSettings.__setattr__(self, name, value: float) is false on a real path — the class does self._vars = {} through it, so value is demonstrably a dict. The old Union[int, float] was already insufficient; narrowing to float made it less true.
ptype is annotated type but holds an integer protocol ID after ptype &= 0x0F.
ParamData.defaults is Optional, but extract_params() takes a non-optional list and is called with pdata.defaults.
extract_params() claims Tuple[float, type] while save_params() expects Tuple[float, int].

And the three headline aliases are all Any, so the newly annotated master/message/file/callback interfaces get essentially no static checking — the broad ones are vacuous while the specific ones are wrong.

Related: the new settings validation lives only in cmd_set(). The CLI builds MAVFTPSettings directly from argparse in main(), and library callers can too, so retry_time=0.1 is accepted and then trips AssertionError: recv_timeout must be < settings.retry_time at use. Validation belongs in the settings object or its constructor, shared by cmd_set, CLI startup and library callers.

One thing I am not blaming on this PR

A cross-check pass reported that cmd_get() leaves requested_offset set after a range read, so a subsequent normal download seeks its staging file to that offset. The defect is real — read() sets it, __handle_open_ro_reply() does self.fh.seek(self.requested_offset), and cmd_get() resets read_to_memory but not the offset. But it's pre-existing: I checked the base revision and it has the same shape. Worth fixing while you're in here; not something you introduced.

Compatibility is fine. pymavlink declares no python_requires; setup.py's lowest classifier is 3.9. All three changed files compile under 3.9 (and 3.7/3.8) — only typing.List/Optional/Union, no X | Y unions or match statements. The 3.9 runtime job passes; only pylint fails. No generated source or generator template is touched, and the PR's own suite passes locally (40 tests plus 11 subtests).

Keep the rename destination stored as arg2 so main() forwards both paths correctly, while displaying new_remote_path in help output.

Add explicit annotations for MAVFTP state, operations, parameter decoding, and command interfaces while retaining compatibility with the project's supported Python versions. Apply the accompanying Ruff-driven cleanup to simplify formatting, resource cleanup, and type-safe optional values without changing the FTP protocol contract. Keep generated sources untouched so later behavioral fixes can be reviewed independently.

tridge · 2026-09-04T13:40:09Z

Automated review note — AI-generated (Claude). A correction to my review above — please read this one first.

I measured that review against the wrong base, and most of what I attributed to this PR belongs to #1267. GitHub's diff is against master, so for a PR stacked on another it includes the whole base series. I read it as this PR's work. Sorry — that's the second-worst kind of review error after a wrong finding.

Measured against this PR's actual stacked base (#1267's head 48efecda36) rather than master:

	vs master (what I reported)	vs #1267 (correct)
AST node lines, annotations and docstrings stripped	−801 / +3070	−98 / +174
growth	+17.1%	+0.5%
new methods	6	2 (`get_setting`, `has_setting`)

So my headline was wrong. This is not "a MAVFTP correctness rewrite mislabelled as a lint change" — it is close to what the title says. __has_active_session, __read_position, __reply_matches_active_request and __seek_read_position are #1267's, as are the math.isfinite validation, the read_to_memory flag and the widened completion condition, the publish_result gate, and the cmd_set settings-validation bypass. I withdraw the retitle request and the behaviour-change list.

The C0302 too-many-lines CI failure is also #1267's, not yours. tests/test_mavftp.py is 229 lines on master and 1150 at both #1267's head and yours — git diff --numstat 48efecda36 f0e7942918 -- tests/test_mavftp.py produces no output at all, and the blobs are identical. #1267 has since added a too-many-lines disable, so that message should clear once you rebase. My suggestion to split the test module doesn't apply to you.

What does stand, verified as yours:

The two C0413 wrong-import-position errors. The MavlinkObject / Callback / FileHandle aliases at lines 49–51 exist only at your head, and they sit above from pymavlink import mavutil and the mavftp_op import. The # noqa: E402 silences ruff but pylint has no suppression. Moving the aliases below the import block fixes both.
MAVFTPSettings.__setattr__(self, name, value: float) is wrong, and this one is sharper than I first put it: the base had Union[int, float], which was already insufficient because the class assigns self._vars = {} through it — a dict on a real execution path. This PR narrows it to float, making it less true rather than more. It needs Any/object or overloads separating internal attributes from setting values.
The observation that the three headline aliases are all Any, so the newly annotated master/message/file/callback interfaces get little static checking, also stands.

The ptype/ParamData.defaults/extract_params annotation inconsistencies I listed are #1267's, not yours.

Net: with the base corrected, the remaining ask on this PR is small — move three lines above the imports to below them, and widen one annotation. Details in the follow-up report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_04_2335/devcall_pr_reviews.html

tridge · 2026-09-04T13:41:39Z

Deprecated — see below for the updated review.

Previous note

Automated review note — AI-generated (Claude). Short addendum to the correction directly above, which was already stale when I posted it.

You pushed 4509d5c2c2 about three minutes before that comment landed, and it already fixes both of the two items I said still stood. I re-checked heads before posting the other comments in this run but not before that one, which is my mistake.

At 4509d5c2c2:

The MavlinkObject / Callback / FileHandle aliases have moved to lines 76–77, below the pymavlink imports at 49 and 53 — so both C0413 wrong-import-position errors are gone.
__setattr__ is now (self, name: str, value: Any), which is exactly the right widening: the base's Union[int, float] was already false because the class assigns self._vars = {} through it.

So nothing from my review is outstanding. CI on the new head is 31 queued; worth watching that the pylint job comes back clean now that too-many-lines is disabled in #1267 and the import positions are fixed here.

I'll pick this head up properly on the next follow-up pass rather than review it inside an addendum.

Move MAVFTP transfer-setting constraints into MAVFTPSettings so constructor inputs, direct library assignments, CLI options, and cmd_set() share one validation path. Reject non-finite and out-of-range values, enforce the idle-detection/read-retry relationship, and restore the prior value when an assignment fails validation. CLI validation now happens before opening a MAVLink transport. Add regression coverage for command, constructor, and direct-library entry points to prevent invalid settings from reaching the transfer state machine.

tridge · 2026-09-04T15:21:31Z

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_05/devcall_pr_reviews.html#prpymavlink-1274

Re-reviewed at head 8794d467c8; my earlier comments above are superseded. APPROVE, up from REQUEST CHANGES — all three findings that survived this morning's correction are fixed, and the retitle is right.

Measured against the real stacked base (#1267's head c4a222c2d9) this time, not master.

Resolved

Both C0413 wrong-import-position errors are gone. The MavlinkObject/Callback/FileHandle aliases moved to lines 76–77, below the pymavlink imports at 49 and 53. That was the whole of the CI failure genuinely attributable to this PR.
__setattr__ is honest now: (self, name: str, value: Any). The base had Union[int, float], already false because the class assigns self._vars = {} through it, and the previous head had narrowed it to float. Any is the right answer.
Settings validation is centralised. __validate_vars() is called from __init__, append() and __setattr__, so construction, appending and attribute assignment all validate — not just cmd_set(). That closes the CLI and library-caller bypass. One qualification from the cross-check worth keeping: _vars is still directly mutable, so settings._vars["write_size"].value = 0 escapes until validate() is called explicitly. A protected-internal escape hatch rather than a public-API hole.

The refactor is behaviour-preserving, and my numbers needed correcting

Against the real base the AST delta with annotations and docstrings stripped is −497/+818 node lines, +2.07%, adding five methods (__copy_setting, __validate_vars, get_setting, has_setting, validate) and removing none. The cross-check pointed out that the "449 lines removed" figure is the two-commit total — bcbdc52 alone is +192/−397 — and confirmed that after stripping typing the only semantic changes are the aliases, the two accessors, a runtime-identity cast(), an equivalent OSError suppression and an equivalent boolean simplification. No FTP operation, branch, state update or cleanup was deleted. It compiled and passed all 43 MAVFTP tests on Python 3.8, 3.9 and 3.14.

One small new thing

math.isfinite(value) raises OverflowError for an arbitrarily large integer such as 10**1000, and the CLI catches only ValueError, so an extreme integer argument produces a traceback instead of a clean parser.error(). Normalising OverflowError to ValueError, or treating integers as inherently finite, fixes it. Non-blocking.

amilcarlucas added 23 commits September 2, 2026 14:38

fix(mavftp): fix reporting of target component

ab0c39e

amilcarlucas added the AIReview label Sep 3, 2026

chore(mavftp): Add pylint disable statements to some hard-to-fix issues

e589f71

amilcarlucas changed the title ~~Ruff lint annotations~~ fix(mavftp): harden FTP reply handling and validate transfer settings Sep 4, 2026

amilcarlucas added 3 commits September 4, 2026 15:35

fix(mavftp): correct rename destination argument

5cc87a9

Keep the rename destination stored as arg2 so main() forwards both paths correctly, while displaying new_remote_path in help output.

feat(mavftp): add ProfiCNC and Matek port detection

c4c66e5

fix(mavftp): reset range-read state for downloads

c4a222c

amilcarlucas force-pushed the ruff_lint_annotations branch from f0e7942 to 4509d5c Compare September 4, 2026 13:36

tridge mentioned this pull request Sep 4, 2026

Backport mavftp fixes from AMC #1267

Open

amilcarlucas force-pushed the ruff_lint_annotations branch from 4509d5c to 8794d46 Compare September 4, 2026 13:59

Navigation Menu

Uh oh!

fix(mavftp): harden FTP reply handling and validate transfer settings - #1274

fix(mavftp): harden FTP reply handling and validate transfer settings#1274
amilcarlucas wants to merge 29 commits into
masterfrom
ruff_lint_annotations

amilcarlucas commented Sep 3, 2026 •

edited

Loading

Uh oh!

tridge commented Sep 4, 2026

Uh oh!

tridge commented Sep 4, 2026

Uh oh!

tridge commented Sep 4, 2026 •

edited

Loading

Uh oh!

tridge commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

amilcarlucas commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tridge commented Sep 4, 2026

The title is the main finding

CI: three deterministic pylint messages, all introduced here

Annotations that are wrong

One thing I am not blaming on this PR

Uh oh!

tridge commented Sep 4, 2026

Uh oh!

tridge commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tridge commented Sep 4, 2026

Resolved

The refactor is behaviour-preserving, and my numbers needed correcting

One small new thing

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amilcarlucas commented Sep 3, 2026 •

edited

Loading

tridge commented Sep 4, 2026 •

edited

Loading