Correctness:
- A child that stopped reading could freeze the whole client: the relay
writes under the context lock and the socket was blocking, so one stalled
peer took every download, upload and API call with it. Child writes now
have a deadline, and a child that hits it loses its slot.
- Relaying searches to children was gated by the leaf's answer budget, so a
parent forwarded a fraction of the stream while advertising itself as
healthy. The budget bounds our own share scan, which is what it was for;
the tree's stream goes down whole.
- A dropped client kept its listener port for the life of the process, so a
retried login advertised an ephemeral port nobody was told about. The
listener is now stopped and the operations thread let go.
- Excluded phrases were compared against a lowercased path without being
lowercased themselves, so a capitalised phrase never matched. They are
normalised where they arrive.
- Losing our own operatorship wiped the room's whole operator roster,
though we are still a member who can see it.
- A brokered ConnectToPeer of type D was logged and dropped; the peer is
now told, since we never dial out to adopt a child.
- A peer request made with no server connection returned Ok and parked
forever; it says NotConnected, as its docs always claimed.
- An empty interest was refused locally but still sent, leaving the
server's list and ours permanently apart.
Simplification:
- Deleted the excluded-phrase helper nothing called, and the field doc that
promised behaviour it never had.
- The four roster handlers and four standing handlers were eight
constructors carrying constants their own message code already implies:
one newtype each, 100 lines less.
- One place registers a search; the phrase list is borrowed, not cloned per
relayed search.
Reading Nicotine+ and slskd against what was just written turned up three
places where this client had the message right and the behaviour wrong.
Excluded search phrases (code 160) are not a filter on the searches we
send — the server already drops those. Nicotine+ applies them to the files
it offers, leaving out any whose path carries an excluded phrase, and a
reply with nothing left is not sent. Ours now does the same, and the
local refusal of outgoing searches is gone.
Serving children now follows the same rules as Nicotine+: the limit comes
from the server's ParentMinSpeed and ParentSpeedRatio against the upload
speed the server records for us (capped at ten, none at all below the
minimum), children are refused until something feeds us the search stream
and let go when it stops, a second link from a child we already carry is
refused, and AcceptChildren is re-sent whenever that answer changes. Where
the server has never reported our speed there is nothing to apply the
formula to, so the configured cap stands, which is how slskd runs.
Queued downloads now ask where they sit every five minutes, as Nicotine+
does, instead of only when a caller asks; a brokered file connection we
cannot complete reports CantConnectToPeer like a control connection does;
and a peer's shares can be asked for one folder at a time (peer code 36
and its reply, 37), which is what "download folder" is built on.
Two bugs an e2e test found the moment one existed for this path.
A private message the server held while we were offline arrives with
new_message false, and the handler only acknowledged the ones flagged
new — so exactly the messages that need an ack never got one and the
server re-delivered them at every login, forever. Every delivery is now
acknowledged.
A dropped Client also left its server connection open, so the session
lived on: mail addressed to us went to a socket nobody was reading and a
later login collided with our own ghost. Client::disconnect closes the
server and peer connections, and Drop calls it.
Also covers two features that were never driven end to end: an offline
message delivered once and not again, and a wishlist search answered like
any other.
The server hands every client a list of possible parents and expects it to
hang from one so searches also travel peer to peer. Until now we told the
server we were a lone root and left it at that, which the network tolerates
but does not favour.
A leaf now dials each PossibleParents candidate over a D connection, adopts
the first that passes a search down after stating its branch, announces the
new branch root and level to the server, and answers those searches the same
way as ones from the server. Losing the parent or a ResetDistributed puts it
back to a parentless stance. The leaf takes no children.
The Leaf state machine is pure so its rules are unit-tested without sockets;
the e2e test drives a real client from a mock parent, since soulfind never
hands out parents.
The registry stored a registration Instant only to pick the eviction
victim, but the actor id is already monotonic per registration and
refreshed at the same moments, so it orders identically. The
register-refuse-fail sequence was also copy-pasted between the ops
loop and the listener; both now call one register_peer_or_fail.
Review of the capacity work surfaced three real defects and two gaps:
- A peer dribbling bytes re-armed the 10s handshake read timeout on
every read and held its accept permit forever; 128 such sockets
wedged the accept loop for good. The handshake now has one deadline
across all reads, plus a 4 KiB cap on buffered handshake bytes.
- A capacity refusal was logged and swallowed, leaving the refused
peer's queued downloads stuck until the caller's own timeout. Both
registration paths now fail those downloads so the caller gets a
verdict, and the listener only announces PeerConnected when the
registration actually happened.
- The transfer-claim was undone by the worker thread writing Queued
back over it; that write is gone, so a replayed TransferResponse
can no longer start a second writer on the same .part file.
- Offered uploads and queue waiters joined the eviction-protected
set, and the browse protection window matches the minutes a real
share listing takes rather than one.
Regression tests: the broker-timeout sweep failing a queued download
end to end, the replay guard observed over a real socket, and the
handshake deadline against a dribbling and an oversized peer.
One popular search produced a ConnectToPeer for every responder, and
each one cost a 2 MiB wrapper thread, an unbounded 256 KiB actor, a
listener thread per inbound socket, and on failure a 2 MiB thread that
slept for 20 seconds. A container with a pids limit died on one album
search (issues #32, #33).
Four bounds, no behavior change for a quiet session:
- P-type registrations run inline on the ops loop; only F transfers
keep a worker thread. The dial already ran on the actor thread.
- The broker timeout is a deadline swept by the ops loop instead of a
sleeping thread per failed dial.
- The registry caps live actors (default 512, Client::set_max_peers).
At capacity the oldest peer outside the caller's protected set
(live downloads, uploads, pending serves, queued messages, browses
in flight) is evicted; if none qualify the registration is refused
before any thread is spawned.
- Concurrent listener handshakes are bounded by a 128-permit
semaphore released once the peer-init arrives, so slow handshakes
cannot multiply threads while transfers stay unthrottled.
A replayed TransferResponse for an InProgress or Completed token is
dropped instead of starting a second transfer thread over the same
.part file.
Fixes#32Fixes#33
The peer listener died with "Too many open files (os error 24)" on any
long-lived non-daemon session: only the daemon raised RLIMIT_NOFILE, and
peer sockets were unbounded — no idle reaping, no deadlines on accepted
transfer streams, and one peer could hold two connections under split
registry identities.
- raise the open-file limit at the top of run(), for every entry point,
and warn from Client::connect when the soft limit is below 1024
- reap peer control connections idle for five minutes; a clean
disconnect no longer discards the peer's queued uploads
- give listener-supplied download streams and upload serving the same
read/write deadlines dialled sockets already had
- register inbound peers under their plain username so the registry
holds one connection per peer, and announce them like pierced ones
- hold the registry lock across actor spawn + insert and surface
thread-spawn failure, so an instantly-dead peer cannot leave a
zombie entry
Seven region audits, every finding adversarially verified against the
whole workspace before being applied; 80 landed and the two that failed
in application were refuted honestly (ParseError has a from_str_radix
producer the audit missed; NatPmpError's Display is the only reader of
its diagnostic payloads).
The bigger cuts:
- The NewPeer/PierceFireWall cluster: an operation variant nothing ever
sent, its 77-line arm, a protocol-wrong Client::pierce_firewall that
wrote a peer-init frame down the server socket, and the struct only
they used. The live paths in peer/listen.rs are untouched.
- The ops loop reads `for operation in reader` instead of hand-rolling
recv/break, de-indenting the whole dispatch body.
- Dead lib surface: Message::print_hex and four unused accessors,
UploadFailed, registry and actor getters, build_watch_user,
strip_ansi_codes, delegating shims on ClientContext. Removing public
items makes this a semver major, which the release pipeline enforces.
- Never-used flexibility: the state-migration framework that never held
a migration, border-type helpers that all answered Rounded, the TUI's
max_concurrent_downloads field nothing read.
- Convergence: six poll loops into poll_until, three wrap-around
navigation blocks into cycle, hit tests into Rect::contains, char
walkers into floor/ceil_char_boundary, five Download literals into
one helper, the suites' shared scaffolding into tests/common.
Deliberately not taken: swapping the hand-rolled zlib inflate and MD5
for crates (they replace wire-facing machinery whose tolerance of
real-world peer quirks no local test exercises) and pulling chrono into
the lib for a log timestamp. Recorded for their own decision.
Net -1002 lines. Both e2e modes stay green: 94 CLI tests each way, 16
protocol, 33 lib, unit suites, clippy pedantic clean, aislop 100/100
with the four scanner false positives suppressed inline where they
fire.
Running several one-shot commands at once returned "no results" for queries that
had plenty. Four defects, each of which made a dead session look like an empty
network:
- The peer listener bound its port inside a detached thread and panicked on
EADDRINUSE, unwinding that thread alone. The process carried on with no
listener, so no peer could deliver a search response. `Listen::bind` now takes
the socket before login and falls back to an ephemeral port when the
configured one is held.
- SetWaitPort advertised the configured port whether or not it was bound, so
peers dialled a sibling process that discarded responses to tickets it never
issued. The port the listener really holds is what gets advertised, mapped
through UPnP, and reported by `whoami` and `serve`.
- A zero-byte read is EOF on a TCP socket, but MessageReader reported it as
success, so a server that closed cleanly left the actor polling a dead socket.
It is now an error, and Relogged (code 41) has a handler. The peer actor
classifies that error: a control connection that was established and then
closed is a clean disconnect, because peers drop idle sockets while the bytes
move over their own connection, and reporting an error there fails every
download queued with that peer including the one streaming. A connection that
never established keeps the error path, which is what the server-brokered
fallback reads to know a peer is unreachable.
- A session the server cut off - usually because another process logged in under
the same name - exited 4, documented as "it worked and found nothing". Callers
concluded the file was unavailable. Sessions now record why they ended and
those verdicts become exit 7.
The server allows one session per account, so concurrency still needs a distinct
--username per run; SKILL.md documents that. The listener port needs no such
care any more.
Six e2e tests against soulfind cover it, each of which fails without the fix: a
taken listen port, a displaced session, three clients searching at once under
one configured port, the two CLI equivalents, and a peer that hangs up its
control connection while a transfer is still to come.
BREAKING CHANGE: `Listen::start` is replaced by `Listen::bind` and
`Listen::serve`, so the caller owns the socket and can read the port it actually
bound. `Client::listen_port` and `Client::session_loss` are new.
The Login message hardcoded SoulseekQt's major version 157, so the
server could not tell soulseek-rs apart from the official client. The
protocol document now reserves a unique major version per project, so
claim 176 and send it via a new ClientVersion on ClientSettings.
Clients and bots built on the library keep major 176 and pick their own
minor version (default 1); the reference CLI/TUI logs in with minor 100.
Refs #12
The one-shot half of the binary could not finish its headline task without a
human at the keyboard: `search` opened a full-screen picker, panicked with
exit 101 when there was no terminal, and no command could fetch a known file
at all. Everything exited 0 whatever happened, wrote emoji prose to stdout,
and ignored --download-dir, SOULSEEK_DOWNLOAD_DIR and config.toml.
The commands are now built for scripts and agents:
search download get browse room list|say|listen message send|read
stdout carries records only (tab-separated, or NDJSON under --json), stderr
carries progress, and the exit code carries the verdict: 2 usage, 3 connect,
4 no results, 5 timed out, 6 transfer failed. `get` does search, rank and
fetch in one step, and `search --json | jq ... | download --stdin` composes.
Global flags parse in any position and every one has a SOULSEEK_ variable and
a config.toml key resolved through a single precedence chain.
Defects found and fixed along the way, each covered by a test:
- a download with an unknown size was finalized against zero bytes and saved
as an empty file; the size now comes from the peer's share listing
- transfers are keyed by a hash of the remote path, so the same path from two
peers shadowed itself and two files sharing a basename overwrote each
other; both collisions are now dropped with a warning
- `message send` and `room say` slept 500ms and claimed success; they now
wait for a server reply queued behind the message before reporting
- Client::login waited on an unbounded channel and never returned when the
server went quiet
Deleted the TUI picker the old `search` drove (download_selector,
MultiDownloadProgress, SearchConfig): 1,573 lines out, 926 in. The
interactive TUI is otherwise untouched.
BREAKING CHANGE: `rooms` and `chat` become `room list|say|listen` and
`message send|read`; `--disable-listener` becomes `--no-listener`; the
unprefixed DISABLE_LISTENER, LISTENER_PORT, LOG_FILE and
MAX_CONCURRENT_DOWNLOADS variables are now SOULSEEK_-prefixed; `search` no
longer opens a picker or downloads anything.
Client::set_shared_directories rescans the given roots into a fresh
in-memory index, swaps it for peers, and re-announces the folder/file
counts to the server. The current directory list moves into the client
context so the running set is queryable. e2e-verified: a browser sees a
directory that was added after login.
Shares::scan_many merges several roots into one index, suffixing
duplicate root names so virtual paths cannot collide, and skipping
unreadable roots instead of failing. ClientSettings now takes
shared_directories: Vec<String>; config gains a shared_dirs list
(combined with the single shared_dir and --shared-dir, deduped), with an
explicitly empty value still disabling sharing and the
share-what-you-download default unchanged.
- 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.
Michel de Graaf