The brokered third-party browse e2e failed deterministically on CI. The test
bound its listener to 127.0.0.1 only, but the server brokers back the address
it observed for that client - the host's routable IP (172.17.0.2 under Docker,
10.x on a CI runner). The sharer's dial was refused, it fell back to
re-brokering, and the accept never arrived. The sibling third-party browse
tests already bind 0.0.0.0.
disconnect_with_error logged only "disconnect" and dropped the io::Error on the
floor, which is what made this invisible in the logs; it now logs the cause.
Peer layer:
- Stream peer messages with a partial-write queue instead of write_all, so a
large SharedFileListResponse over the non-blocking socket is no longer
truncated (third-party clients like SoulseekQt can now browse our shares).
- Bound the peer-init handshake with a read timeout and handle each accepted
connection on its own thread, so one stalled peer can't wedge the listener.
- e2e: third-party direct/brokered/large-share browse + stalled-peer coverage.
TUI:
- Uploads share the downloads table columns (real Speed) and the info pane now
renders details for a selected upload.
- Download progress bar counts only in-progress transfers and hides when idle.
- Always-visible unread badge (private messages + rooms) in the shortcuts bar,
self-drawn blink so it works under tmux.
- Persist the unread private-message count across restarts.
- 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.
Implement the Soulseek chat-room server messages against soulfind's
wire format:
- MessageFactory builders: room-list request (64), join (14),
leave (15), say (13).
- Handlers parsing the server side: RoomList (64) names+counts,
JoinRoom (14) membership, SayChatroom (13), UserJoinedRoom (16),
UserLeftRoom (17), LeaveRoom (15).
- Surface everything to the client as a drained RoomEvent stream plus
a room-list snapshot; new Client API: request_room_list, join_room,
leave_room, say_in_room, room_list, take_room_events.
Unit tests cover each builder and handler; server-gated e2e tests
exercise message delivery and room listing against soulfind.
Add Client::browse_user (sends GetShareFileList, code 4, connecting first if
needed via the same queue-and-flush path as search replies) and
take_browse_result. A new SharedFileListResponseHandler parses the peer's
code-5 reply into SharedDirectory entries, forwarded as
ClientOperation::BrowseResult and stored per user. Verified end to end: a
second real client browses a sharer and receives its file listing.
GetShareFileList (peer code 4) previously replied with a malformed message (a
server SharedFolders stats message over a peer connection). It now emits
ShareListRequested, and the client builds a real SharedFileListResponse
(code 5) from its shares grouped by directory. Adds a std-only code-5
encoder/decoder (build_shared_file_list / parse_shared_file_list) with a
roundtrip test, and Shares::directories() to group files. The decoder is the
foundation for the Browse feature.
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.
Wire the incoming server FileSearch (code 26): the handler now forwards
ServerMessage::FileSearchRequest -> ClientOperation::IncomingSearch instead of
just logging. The client matches the query against its shares and, if anything
matches, sends a FileSearchResponse to the searcher — connecting to them first
(GetPeerAddress -> outbound P) and flushing the response on PeerConnected when
not already connected. GetPeerAddressResponse now also caches peer addresses
for the serve path. Adds build_search_response + a unit test.
The post-login handshake (SharedFolders / HaveNoParent / SetStatus /
SetWaitPort) lived only in ServerActor::login(), which nothing ever called —
so a live client never advertised its listen port or share counts. Move it to
a testable post_login_messages() fired from the LoginStatus(true) arm (once, on
success, on the live path), and delete the dead login().
Client::connect now scans ClientSettings.shared_directory once into
ClientContext.shares and passes the real folder/file counts to the server.
This is the prerequisite for two live clients to resolve each other's
addresses (GetPeerAddress needs the peer to have sent SetWaitPort).
Unit-tested post_login_messages; the login/listener e2e still passes, proving
soulfind accepts the handshake.
When a healthy peer actor was replaced under the same username (both sides
connecting at once, or two overlapping downloads to the same not-yet-connected
peer), its graceful shutdown still emitted PeerDisconnected, and the client
evicted by username — removing and stopping the NEWER actor now occupying the
slot, tearing down the live connection and stalling that peer's downloads.
Give each peer actor a unique id (assigned by the registry) and echo it in
PeerDisconnected/PeerConnectFailed. The client now evicts via
remove_peer_if(username, id), which removes the entry only when the reporting
actor still owns the slot, so a stale actor's shutdown is a no-op. The brokered
timeout no longer removes an actor by username either, avoiding the same
aliasing. Add a registry unit test for the identity guard.
When the server asks us to connect to a peer (a ConnectToPeer we receive,
carrying the server's correlation token), we dial that peer and must announce
ourselves with a PierceFirewall quoting the token — not a PeerInit — so the
remote peer can correlate its own indirect request (search-result / upload
delivery). The outbound handshake now sends PierceFirewall when the peer
carries a server token and PeerInit only for direct GetPeerAddress dials
(token None). Direct and firewalled e2e paths are unchanged.
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.
Implement the DIRECT connection-initiation mode so a download can target a
peer the client is not already connected to:
- Fix the outbound peer handshake. on_connection_established sent
build_watch_user() (a server WatchUser message, wrongly framed with
get_data()) for outbound connections; it now sends a proper length-prefixed
PeerInit (peer code 1) with our own username. own_username is plumbed in
via PeerRegistry -> PeerActor; inbound peers (outbound=false) are unchanged.
- download() now auto-connects: if there is no control connection to the peer
it sends GetPeerAddress and defers the QueueUpload until the outbound stream
handshakes, signalled by a new ClientOperation::PeerConnected. A never-
connected client still resolves the download to Failed (unit test).
- Add Client::connect_peer(username) to open/pre-warm a control connection.
Verified end to end against soulfind: a_file_downloads_from_a_peer_via_direct_connection
logs a mock peer into the server, advertises its port via SetWaitPort, and the
client resolves the address, dials it, and downloads the file — bytes match.
The F-serve leg is shared with the passive-download mock.
Wire private messaging through the client:
- MessageFactory::build_message_user (code 22) and build_message_acked
(code 23);
- the incoming MessageUser handler now acknowledges freshly delivered
messages and forwards them to the client instead of only logging them;
- ClientContext buffers received UserMessages;
- Client::send_private_message and Client::take_private_messages expose the
feature, and UserMessage (with accessors) is re-exported from the crate.
Verified end to end against soulfind: a_private_message_is_delivered_between_users
logs in two clients and asserts one receives the other's message.
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.
register_peer inserted the new handle and silently dropped the old one, leaving
the previous actor running as an orphan that pins a thread-pool worker forever;
its later shutdown could also remove the replacement connection from the
registry. The displaced actor is now told to stop.
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.
- Drop dead Rooms/Room infrastructure and the rooms field on Context.
- Remove unused ActorMessage::Tick variant and the now-empty handler arm.
- Strip dead active-thread tracking machinery from ThreadPool (unused
getters, _id/_active_threads/_total_threads parameters).
- Flatten ServerActor::initiate_connection: the duplicated retry block
was a copy-paste retry against the same address with the same args
and could never succeed where the first attempt failed.
- Delete commented-out print_hex2/decode debug stubs, dead test
fixtures in login.rs and message_user.rs, the 47-line commented body
of RoomListHandler, and assorted // TODO theater.
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