- remove 34 trivial restating comments (auto-fix)
- replace all 20 production .unwrap() calls with graceful handling:
?-propagation via LockPoisoned, unwrap_or_default on infallible
conversions, let _ = on actor-channel sends, and a downloads_sender()
helper in the TUI that also dedupes three channel-init blocks
- collapse nested if into match guard (clippy::collapsible_match)
- split client.rs (2275 lines) into a client/ module: connection,
search, downloads, uploads, rooms, operations
- split ui/main_tui.rs (2027 lines) into a main_tui/ module: render,
input, browse, rooms, downloads, search
- extract oversized functions: peer_actor/server_actor handle_message
into per-message helpers, main() into init_logging/run_default_tui,
download_selector and main_tui render into per-section helpers
Mechanical only, zero behavior change. Verified: clippy clean
(pedantic+nursery), fmt clean, 143 unit tests + 16 e2e tests against
soulfind pass, lint clean.
Carry a human-readable reason on DownloadStatus::Failed so it flows
through the existing status channel to the TUI. The download Info pane
now shows an Error line for a failed download (connect failure, peer
disconnect, truncated transfer, peer-reported upload failure, or the
underlying transfer error). Failures without a known reason render as
before.
Implement the upload path so a peer can download our shared files:
- fix the mis-coded PlaceInQueueResponse (43 -> 44) and add a real QueueUpload
handler (code 43) -> PeerMessage::IncomingQueueUpload;
- the peer actor forwards it as ClientOperation::QueueUpload; the client looks
the file up in its shares, mints an upload token (high half of the token
space, disjoint from md5 download tokens), and offers it via ServeUpload ->
build_upload_transfer_request (peer code 40, direction=upload);
- a TransferResponse for one of our serving tokens is handled by a surgical
early-return that emits StartUpload, leaving the download path untouched;
- StartUpload resolves the downloader's address (GetPeerAddress, cached) and
streams the file via a new peer::upload_peer::serve_file over an F connection.
Tested: build_upload_transfer_request decodes via the production Transfer
parser; serve_file streams a file correctly over a loopback F connection.
GetPeerAddressResponse narrowed obfuscated_port (u16) into a u8 via
try_into().unwrap(), panicking for any real obfuscated port (>255) and
unwinding the client's single ops-loop thread — hanging all downloads,
searches and peer events. Since download() now auto-resolves peer addresses
via GetPeerAddress, a download to any obfuscation-enabled peer could crash the
whole client. Widen Peer::obfuscated_port to u16 and stop truncating.
Implement the FIREWALLED connection mode: when a direct outbound connection to
a peer fails (peer unreachable), fall back to asking the server to broker it.
- Add MessageFactory::build_connect_to_peer (server code 18).
- A pre-established outbound failure now emits ClientOperation::PeerConnectFailed
instead of a normal disconnect; the client registers a correlation token and
sends ConnectToPeer so the peer connects back.
- The listener accepts the peer's PierceFirewall (peer code 0) connect-back,
matches the pending token, registers it as the P control connection, and
fires PeerConnected to flush the queued download.
Fix a latent aliasing bug this surfaced: a peer actor could emit two terminal
notifications (a connect-failure, then a disconnect when later replaced and
stopped), and the second removed the *newer* actor registered under the same
username, killing the brokered connection mid-transfer. An actor now reports a
terminal outcome at most once (disconnect_reported guard).
Verified end to end against soulfind: the mock advertises an unlistened port so
the direct connect is refused, the server brokers the connection, the mock
pierces back, and the file transfers — bytes match. A companion test
(soulfind_brokers_connect_to_peer_between_users) pins the server behaviour the
mode relies on.
A peer actor multiplexes socket reads with its mailbox on one thread: tick()
reads the socket while outgoing messages (e.g. a queued QueueUpload that starts
a download) are delivered through the mailbox between ticks. Inbound
connections accepted by the listener were left in blocking mode, so once such a
peer went idle tick() parked inside read() and never drained the mailbox — a
download queued against that peer never sent its QueueUpload and hung. Outbound
connections already set non-blocking; do the same for inbound P connections.
Covered by a new server-optional e2e test that downloads a file end to end from
an in-process mock peer over real P (control) and F (transfer) connections,
asserting the bytes written match the source. The mock speaks the library's own
public Message wire format, so no dependencies are added.
Enable the full clippy all + pedantic + nursery groups as workspace warnings,
with a small documented allow-list for the pedantic/nursery lints this project
intentionally does not follow (rustdoc-style requirements, deliberate numeric
casts in the protocol/UI layers, and a few noisy nursery lints). Also enable
the rust unused_lifetimes and unsafe_op_in_unsafe_fn lints.
Fixed every resulting warning (527 -> 0) mechanically per clippy's suggestions:
inline format args, const fns, item ordering, let-else, redundant clones,
map_or, cast_lossless, Eq derives, associated-fn conversions for methods that
never used self, and removing unnecessary Result wrappers whose call sites could
never propagate an error. No runtime behavior changed; cargo fmt applied.
cargo clippy --workspace --all-targets: 0 warnings. All tests pass.
read_download_stream broke out of its read loop on peer EOF (Ok(0)) without
checking whether the promised number of bytes had actually arrived, then saved
the partial buffer and the caller reported DownloadStatus::Completed. A peer
that aborts mid-transfer thus produced a truncated file marked complete. The
buffer was also never trimmed, so trailing coalesced bytes were written past
the file's real size.
finalize_download_buffer now errors with IncompleteDownload when fewer bytes
than expected arrived (caller maps this to Failed) and trims any overshoot to
the exact expected size. The listener's incoming-file path now also marks the
download Failed on error rather than only logging, so it can no longer stay
stuck Queued/InProgress forever.
Peer::new_from_message, NewPeer::new_from_message and the listener's
parse_peer_init_message parsed an untrusted connection-type string with
.parse().unwrap(), panicking on anything other than P/F/D. Reachable from:
- a malicious server via ConnectToPeer (kills a thread-pool worker), and
- any peer connecting to our listener (kills the accept loop).
new_from_message now returns Option and callers drop malformed messages;
the listener rejects the connection. Added a regression test for truncated
TransferRequest parsing, now safe via the bounds-checked readers.
- Add pub const BYTES_PER_MB to ui/utils.rs and replace 7 uses of the
bare 1_048_576.0 literal across the UI modules. Also pick up
uninlined-format-args cleanup in the same file while we're here.
- Replace Vec<i32> IP-byte accumulator in Peer::new_from_message with a
[u8; 4]; the .abs() calls on u8.into() were no-ops on always-positive
values. Network byte order ([3].[2].[1].[0]) preserved; tests cover
the wire format.
Port numbers are 16-bit; the codebase had listen_port/listener_port
typed as u32, inconsistent with PeerAddress.port (already u16).
Migrate the type all the way through the CLI, config, ClientSettings,
Client, ServerActor, and the listener entry point. The wire format is
preserved: build_set_wait_port_message now takes u16 and widens to u32
via .into() before write_int32. Also rename DEFALT_LISTEN_PORT to
DEFAULT_LISTEN_PORT.
Splits the downloads area 60/40 with a new info pane that adapts to the
selected download's status: header (filename, user, size, status, save
path, bitrate, length) plus queued-specific fields (queue position, peer
slots, peer upload speed) or live progress/speed/ETA when transferring.
Plumbs queue position via a new ClientOperation::PlaceInQueueUpdate so
PlaceInQueueResponse messages from peers reach DownloadStore, and threads
search-time metadata (bitrate, length, peer slots/speed) onto Download
through a new download_with_metadata entry point.
Add SoulseekRs::LockPoisoned variant and a small RwLockExt/MutexExt
helper module exposing read_safe/write_safe/lock_safe that map
PoisonError to that variant. Walk every non-test lock acquisition
across client, peer/server actors, peer registry, listener and the
thread pool: propagate via ? where the function returns Result, log
and degrade to a sensible default (return early, false, None, empty)
where it does not. Client::connect now returns Result so the CLI can
surface the failure instead of panicking.
Michel de Graaf