fix: transfer outcomes, a stalled peer, and listings that never end - #98
Open
srgg wants to merge 4 commits into
Open
fix: transfer outcomes, a stalled peer, and listings that never end#98srgg wants to merge 4 commits into
srgg wants to merge 4 commits into
Conversation
srgg
force-pushed
the
fix/transfer-robustness
branch
2 times, most recently
from
August 15, 2026 07:07
d869ae4 to
6685cb1
Compare
An app pairs FTP_DOWNLOAD_START/FTP_UPLOAD_START with the terminal callback to release what it acquired for a transfer — a radio boost, a paused advertisement, a progress UI. Several endings never delivered that pairing, and several delivered the wrong one. Missing terminal callback: - closeTransfer() emitted it only inside `deltaT > 0 && bytesTransfered > 0`, a condition written to guard the throughput division. An empty file, or one that finished inside a single millisecond, ended with no notification at all. - dataConnected() answered 426 and closed the stage without any callback. - doStore()'s out-of-space path answered 552 and closed the file by hand: no callback, no dir close, no restart-position reset. Success reported for a failure — the client is told 226 and the app is told FTP_TRANSFER_STOP: - doRetrieve(): a zero-length socket write, and a peer that closed the data connection with bytes still to send (a truncated download). - doStore(): a peer that stayed connected but sent nothing for 5 s. - doRetrieve()'s REST seek failure additionally answered twice, 450 then 226. All of these now end through abortTransfer(), which closes the file and the dir, fires FTP_TRANSFER_ERROR, resets the restart position and replies once. It takes an optional reply so a caller with a more specific code than 426 keeps it — used by the 450 and 552 paths above. Separately, doRetrieve() dropped data on a short write: file.read() had already advanced the cursor by the full block, so bytes the socket did not accept were never sent and the client received a file with a hole in it and no error. The cursor is now rewound to the first unsent byte. (cherry picked from commit 6ca6f9c)
…ransfer A zero-length socket write ended a download. It does not mean the peer is gone: the network client returns zero once its own retry budget expires, which on a memory-constrained board happens whenever the WiFi driver momentarily cannot allocate a transmit buffer. The window reopens seconds later. Downloads were being torn down for a condition that clears by itself — on the board this was first measured on, a 2.9 MB file died after 32 KB. doRetrieve() now treats a zero-length write as no progress rather than an ending: the file cursor is rewound so the same block is retried, the byte count and the download-progress callback stay put, and the transfer continues. Only rounds that actually sent something push the idle deadline out, so a peer that never comes back is ended by that deadline instead of running forever. The deadline could not do that job before. It was the tail of the if/else chain in handleFTP(), and a running transfer always took its own branch first — so it was unreachable during exactly the case that now needs bounding. It is now a check of its own, scoped to an idle command connection or a RETR: the other transfer types never refresh the deadline, and bounding them here would kill them mid-progress. (cherry picked from commit d7c5cf0)
Each entry went out as up to nine separate print() calls. A window that shuts mid-entry left half a line in the stream with the directory cursor already past it, so the rest of the listing was garbage. Each call also spends the socket's whole write budget again — ten seconds on ESP32 Arduino — so a peer that stopped reading held one handleFTP() call for ninety seconds. An entry is now rendered into a buffer sized from the limits this header already declares, and written once, the remainder pending until the peer takes it, as doRetrieve() does for a short write. A line too long keeps its CRLF, or it would merge into the next entry. Both paths write through one accountant, so neither sends without moving the deadline.
The idle deadline covered RETR and the command stage only, so a LIST, NLST or MLSD that stopped being read had nothing to end it: the server retried its failing writes until the client gave up, and the session stayed open holding the queued send buffer. Observed on an ESP32-S3 with internal DRAM exhausted, 415 rounds before the run was stopped by hand. Listings refresh the deadline whenever the peer takes bytes, so covering them ends only the ones that stopped moving. STOR refreshes nothing yet and stays out.
srgg
force-pushed
the
fix/transfer-robustness
branch
from
August 15, 2026 07:36
6685cb1 to
db5a13d
Compare
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
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 Problem
Four defects in the existing transfer paths, all found while serving files from an SD card over WiFi on an ESP32-S3, all reproduced on hardware.
closeTransfer()emits its terminal callback from inside a condition that guards a division. An application holding something for the duration of a transfer never learns it may let go.226for a transfer that failed — a zero-byte socket write, a peer that vanishes mid-file, and an upload that goes silent — and a short write loses data outright, becausefile.read()has already advanced the cursor past the bytes that never left.doRetrieve()ends the transfer when a socket write returns zero. Zero does not mean the peer left:NetworkClient::write()returns it after exhausting its own retry budget, which happens whenever the WiFi driver is momentarily short of a transmit buffer, and clears within seconds. A 2.9 MB file ended at 32 KB, and resuming returned the same block again.LIST,NLSTorMLSDentry goes out as up to nine separateprint()calls, and the idle deadline coversRETRand the command stage only. A window that shuts mid-entry leaves half a line in the stream with the directory cursor already past it, so every entry after it is unparseable — and nothing ends a listing whose peer stopped reading. With internal memory exhausted, 415 rounds of failing writes were captured before the run was stopped by hand.abortTransfer()firesFTP_TRANSFER_ERRORfor whatever stage was running, including a listing. A listing has no file and no byte count of its own, so an application watching transfers receives the name and total of whicheverRETRran before it — reachable today by disconnecting a client mid-LIST, which reachesabortTransfer()throughdisconnectClient().The Fixes
One commit each, in the order they must be read.
Fix 1 and 2 — report every transfer outcome, and report the end at all
Every ending funnels through
abortTransfer(), which takes the reply line so450,552and426 Data connection closedsurvive instead of being replaced by a second226. The terminal callback moves out of the guarded branch, so it fires for an empty transfer too. A short write rewinds the file cursor by the unsent remainder.Fix 3 — ride out a peer whose window shuts
The transfer waits instead of ending: the file cursor rewinds, the idle deadline moves only when bytes actually leave, and the deadline check moves out of the
if/elsechain inhandleFTP()where a running transfer could never reach it.Fix 4a — render a listing entry once, resume it if the peer takes part
The entry is rendered into a member buffer and written once, with the remainder kept pending until the peer takes it — the shape
doRetrieve()uses for a short write. An entry counts toward the closing226 N matches totalonly once all of it is away, and a line too long for the buffer keeps its CRLF rather than merging into the next entry. Both the retrieve path and the listing path write through one accountant, so neither can send bytes without moving the idle deadline, nor move it without sending any. This also removes the nine-calls-per-entry cost: each call spends the socket's full write budget again (WIFI_CLIENT_MAX_WRITE_RETRY×WIFI_CLIENT_SELECT_TIMEOUT_US, ten seconds on ESP32 Arduino), so a peer that stopped reading held onehandleFTP()call for about ninety seconds.Fix 4b — end a listing whose peer stopped reading
The idle deadline covers
FTP_List,FTP_NlstandFTP_Mlsd. That is safe only because of Fix 4a: listings now refresh the deadline whenever the peer takes bytes, so it ends the ones that stopped moving and leaves the ones still delivering.STORrefreshes nothing yet and stays outside rather than risk ending an upload mid-progress.Fix 5 — a listing reports no transfer of its own
abortTransfer()firesFTP_TRANSFER_ERRORonly forFTP_RetrieveandFTP_Store.This is a deliberate behaviour change to an existing path: the callback line predates this branch, and gating it is what stops a listing from reporting a foreign file name and byte count. Routing
dataConnected()throughabortTransfer()in Fix 1 and 2 widened the reach of the defect, which is why it is fixed here rather than left.Nothing automated guards it, and the callback was not observed on hardware — the bench application ignores
FTP_TRANSFER_ERRORunless a transfer is live.Trade-offs
snprintfinto a fixed buffer instead of streamed withprint(). That costsFTP_LIST_LINE_SIZEbytes of RAM per server instance — derived fromFTP_FIL_SIZEandFTP_CRED_SIZE, the limits this header already declares, plus the date field and the widest alongprints — and it is what makes a partial write resumable, which streaming cannot be once the directory cursor has moved.FTP_TIME_OUTgets426where it previously got a server that waited forever; that is the intent, and progress of any size defers it.How to verify
Serve a directory over FTP and pull a listing and a file while the client reads normally: the listing and the file are unchanged, byte for byte. Then stop reading mid-transfer and watch the session end with
426afterFTP_TIME_OUTinstead of retrying indefinitely.Verified on an ESP32-S3 with an SD card:
NLSTandLISTboth return all 108 entries in the same format as before, and a 41.7 MB file retrieves byte-identical to the same file fetched by name. Not verified: the SPIFFS, LittleFS, FFAT, FatFs and SdFat backends compile-only paths — this hardware exercisesSTORAGE_SD_MMCwithNETWORK_ESP32, and the other backends were changed mechanically, in the same shape.