Adds soulseek-rs-lib/tests/e2e.rs, which drives the client's public API
(connect, login, search) against a real Soulseek server. The harness is
server-optional so cargo test stays green everywhere: it uses
SOULSEEK_TEST_SERVER if set, otherwise spawns a soulfind binary (SOULFIND_BIN
or a soulfind/bin/soulfind checkout in a parent dir) on an ephemeral port with
a throwaway database, and skips with a notice when no server is available.
Uses only std and the library's public API — no new dependencies. Documents how
to run it (including the macOS soulfind SQLite workaround) in the README.
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.
MainTui::run only disabled mouse capture and called ratatui::restore() after
the event loop exited normally; any error propagated by ? from draw/poll/read
returned early and left the terminal in raw mode, on the alternate screen, with
mouse capture still enabled. The loop body moved into a helper whose result is
returned only after the terminal is unconditionally restored.
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.
The handler held a read guard on the client-context RwLock while calling
process_failed_uploads, which acquires a write guard on the same lock on the
same thread. std RwLock write() then blocks forever, wedging the entire client
operations loop. The read guard is now scoped to just the registry cleanup and
dropped before the write is taken.
When log buffering and a LOG_FILE were both active, log() wrote each line to
the file immediately and also pushed it to the buffer, which flush_buffered_logs
then wrote to the file a second time. Each line now routes to exactly one sink
(file / buffer / stderr) via a small tested decision function; a configured
file always bypasses buffering, so nothing is written twice.
A job that panicked unwound out of the worker's run loop and permanently
killed that worker; the pool has a fixed worker count and never respawns, so
repeated panics (e.g. from malformed network messages) would exhaust the pool
and wedge the whole actor system — a DoS. The worker now runs each job under
catch_unwind (the shared receiver lock is already released, so a panic cannot
poison it) and survives to process the next job. Drop and execute no longer
unwrap their channel/join results, so shutdown can never itself panic.
Three related TUI correctness bugs:
- download_selector navigation (select_previous/next/last) computed
items.len() - 1, panicking with an arithmetic underflow when the list was
empty (e.g. a filter matched nothing). Index math is now in wrap-safe
helpers that return no selection for an empty list.
- update_search_results overwrote the filtered results view with the full
unfiltered list on every ~100ms poll, so an active filter silently stopped
working as new results streamed in. It now re-derives the filtered view
from the current query.
- The results pane rendered selection checkboxes by testing the display-row
index against a selection set that stores unfiltered indices, so under an
active filter the wrong rows showed as checked. It now maps the display
index back through the filtered-index table.
Filtering and index-mapping logic extracted into pure, unit-tested helpers.
decode_symbol walked the Huffman tree while either child existed, then
unwrapped the child selected by the current bit. A malformed/incomplete code
table (fully attacker-controlled, since search results arrive zlib-compressed
from peers) can leave a node with only one child; reading the opposite bit
unwrapped None and panicked, unwinding out of the actor job. It now returns a
decode error, which propagates as a CompressionError and is handled gracefully.
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.
read_string, read_int8, read_int64, read_bool and the get_message_code
accessors indexed/sliced the buffer using network-controlled offsets and
lengths with no bounds checks. An untrusted server or peer could send a
message with an oversized string length or a truncated/zero-length frame and
panic the parser; the panic unwinds out of the actor job and permanently
kills a thread-pool worker (DoS + worker leak).
All readers now bounds-check and return a safe default (empty/0/false) on
truncation, matching the existing convention already used by read_int32 and
read_raw_byte.
- 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.
- 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.
Move the Vec<Download> and the download lifecycle methods (add, remove,
lookup, status update, pause, resume, remove-if-queued) into a new
download_store module. ClientContext keeps thin delegating shims so
peer/listen, peer/download_peer and the existing client tests carry on
unchanged. Client::pause_download / resume_download / remove_queued_download
now delegate one-liners. Client::process_failed_uploads keeps the
read-then-write lock dance but the inner filtering is the new
collect_failed_tokens helper.
Adds dedicated unit tests for the store's behavior (add/get/remove,
pause+resume, queued-only removal, failed-token collection) so the
state-machine logic is exercised independently of Client.
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