Running the e2e suite against a real soulfind showed the per-member slots
field was implemented backwards. The wire field is `slotsfull` -- a boolean
that is true when the member's upload slots are all occupied -- not a count
of free ones. `slots_free: u32` named it, documented it and printed it as
the opposite of what the server means, so a caller reading it would pick the
busiest peer to download from.
- types: `slots_full: bool`, with the wire name in the doc comment
- the DTO, the `room users` output and SKILL.md follow
- a new e2e test joins a room with a member sharing a known three files and
asserts the counts, the roster alignment and the slot flag against
soulfind, so the parsing is checked against a real server rather than only
against frames this repo wrote itself
The statistics parsed from the JoinRoom reply were reachable only from the
library. They answer the question `room users` already exists to ask -- who
is in here, and are they worth downloading from -- so they belong on its
output rather than behind a second call.
- proto: room.member_stats, with a RoomUserStatsDto carrying status, share
counts, free slots and country
- SessionApi::room_member_stats, so local and remote sessions stay
interchangeable; the status word is decoded through one shared helper
rather than a third copy of the match
- `room users` now emits status, average_speed, shared_files,
shared_folders, slots_free and country, each null when the server said
nothing, documented in SKILL.md
- docs/openrpc.json regenerated from the wire types
The watch subscription was reachable only from the library. It is most
useful against a daemon -- that is the session that stays connected long
enough to receive the server's status pushes -- so it needs the wire.
- proto: user.watch / user.unwatch / user.watched, with a Watched response
- SessionApi gains watch_user / unwatch_user / watched_users, so a local and
a remote session are still interchangeable above the seam
- CLI: `watch add`, `watch remove`, `watch list`, documented in SKILL.md
including the caveat that a one-shot run's watch ends with the run
- docs/openrpc.json regenerated from the wire types
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
docs/openrpc.json lived at the repository root, outside the soulseek-rs
package, so the include_str! in proto compiled from a checkout but not
from the published tarball - cargo publish failed verification and the
v13.0.0 release stopped after publishing the lib. The document moves
into the crate's own docs/, where packaging can see it; the generator
test and every link follow.
info.version carried the crate version, so release-plz bumping Cargo.toml
left docs/openrpc.json a version behind and failed document_is_current on
every platform. With the CI gate in front of publishing, that made every
release PR unmergeable. The protocol version is what auth negotiates and
what the contract actually describes.
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.
The full-surface audit found the settings story half-true: `config set
download_dir` and the TUI's settings form edited a file a running daemon
never rereads, while the attached TUI displayed the laptop's path and
discarded the edit entirely.
- `download.set_dir`: a session-wide move, published in the contract;
the per-transfer destination stays refused. `daemon.status` reports
it and the next transfer lands there.
- `config set download_dir` and `config set shared_dirs` push to a
live daemon the way `shares add` does, through one shared helper;
the socket probe is paid only by the keys a daemon owns.
- The attached TUI seeds its settings form from the daemon's status,
pushes both folders over the socket as typed, reports a partial
refusal as exactly that, and creates no client-side directories. It
reads the daemon token from the resolved config — with the token only
in config.toml it could not attach over TCP — and settings
persistence honours --config instead of hard-coding the default path.
- `search.wishlist` records a start time, so an attached window
renders it as running rather than instantly finished.
The rest is the coverage the audit demanded: CLI-level token auth
(wrong, missing, and valid over TCP), `daemon status|stop` with
nothing running, --sort, --limit, --pick first, --password-cmd,
--log-file, --upload-slots, the config-named download folder in both
modes, and headless settings-form tests attached and local. Every
guard was checked by disabling what it protects. Both e2e modes run
green: 94 CLI tests each way, 16 protocol, 285 unit, 34 lib.
Two TUIs on one daemon behaved as two separate windows that happened to share
a login: each showed only the transfers it had queued and the searches it had
run, and neither said it was attached to anything.
The status bar now names the daemon, and both lists are views of the session:
transfers and searches appear in every window, dismissing one dismisses it
everywhere, and a window opened later starts from what is already running
rather than blank. message read likewise starts from the conversation the
daemon collected instead of only what arrives while it runs.
Search state needed the daemon to say when a query went out. The window a
search collects for belongs to whichever client started it, so without that
every other window showed a running search as finished the moment it appeared
— which is what it did. The summary carries a file count too, so a window has
the right number before it has pulled the results across.
Adds Client::forget_search, which the library had no way to do: a resident
client otherwise holds every result set it has ever seen, and a window that
dismissed a search had no way to say so.
A review of the first cut found five defects, all fixed here and all covered:
syncing ran locally too, where it cost a full copy of every result set per
frame and deleted the completed-download history restored from disk; a
transfer could be listed twice by racing its own registration; clearing all
searches did not reach the session, so every row came back; a search dismissed
elsewhere never left; and a wishlist search reported as finished the instant
it went out.
A search fans out to hundreds of peers, each holding a socket. A shell hands
its children a soft limit of 256 open files on macOS, which one busy search
blows straight through; a one-shot command never noticed because it exited
before it mattered, but a resident daemon hits it and then cannot accept peers
or control connections. From a client that looks like 'cannot reach the
daemon: Connection closed', intermittently, for no visible reason.
The daemon now raises its own soft limit to 8192 at startup, before anything
opens a socket, and says so plainly when the hard limit will not allow it.
macOS rejects an unlimited hard limit for open files, so both fields are named.
Both accept loops also slept zero on failure. Out of descriptors does not clear
by the next instruction, so one exhausted moment became a spinning core and
thousands of identical log lines — which is what the report looked like. They
now back off.
spawn_connection used thread::spawn, which panics when a thread cannot be
created. That panic unwinds through the accept loop, so on a client already
running a thread per peer one exhausted moment would end control for the life
of the process — every later connection accepted and immediately closed, which
is exactly what 'cannot reach the daemon: Connection closed' looks like from
the other side.
Builder::spawn instead: refusing the one connection, with a reason in the log.
Starting the daemon printed 'ready' and nothing else, so the token and the
next step were both somewhere else. It now prints what other commands can do
from here, and when --bind is given, the token itself with the two config
commands a remote client needs. All on stderr, so stdout stays empty and
daemon still composes in a pipeline.
Adds the guarantee daemon mode is for as a test: eight clients connected at
once are each answered correctly, none has its replies crossed with another's,
and the daemon still counts all eight. Plus 250 connect-and-drop cycles, well
past the connection cap, to catch a per-connection resource that is not
released — the shape a daemon that works for a while and then refuses
everybody would take.
A connection the daemon drops before it can be served now logs why at warning
level. A client only ever sees 'the connection closed', so that line is the
only account of the reason.
Only 25 of the 83 CLI e2e tests actually reached a daemon: the helpers that
build a command bypassed the one place that routed. Routing at
TestServer::args instead brings that to 39 and, more importantly, puts the
streaming tests through a daemon — event.room and event.message delivery to a
remote client's mirror had no end-to-end coverage at all.
Adds what the command surface structurally cannot reach:
- Two clients against one daemon at once, which is the reason the hub exists
and was proven nowhere in CI.
- tests/daemon_protocol_e2e.rs, speaking the socket by hand: TCP token
required and socket token not, pre-auth refusal, error ids, notification
silence, the line cap, the handshake timeout, and that a call shaped by the
published contract is accepted.
- daemon status/stop/token as commands.
- A TUI unit test for conversation seeding, the part of the attach path that
had actually been broken.
The protocol suite caught a regression in yesterday's hardening on its first
run: the socket was shut down before the writer flushed a failed handshake's
refusal, so a client with a wrong token saw the connection vanish instead of
being told why.
A review pass over the diff found seventeen confirmed defects. The worst was
the contract itself: every method's parameters were published as a single
descriptor named `params`, so a client generated from openrpc.json sent
{"params":{"params":{…}}} and every call failed -32602. Parameters are now
published per field, with a test asserting the published names are the keys
the daemon actually deserializes.
Security and lifecycle:
- download.start no longer lets a caller name a directory, which was an
arbitrary write on the daemon's host.
- The Unix socket is bound under a temporary name and renamed into place; it
was briefly connectable by any local user between bind and chmod, and on
that transport connecting is authenticating.
- Pre-auth reads are capped at 1 MiB with a 30s handshake timeout, and
connections are capped, so an unauthenticated peer cannot exhaust memory or
threads.
- A read error no longer returns past the hub unsubscribe, which leaked a
subscriber and two threads per occurrence.
- A client whose event queue fills is now actually disconnected. It used to be
dropped from the fan-out and left connected, working on but blind to every
event after — the documentation already promised otherwise.
- A client whose daemon vanishes mid-transfer is told at once instead of
waiting out its full timeout and reporting a misleading "gave up".
Contract accuracy: errors carry an explicit null id (a missing one reads as a
notification and gets dropped), a second auth answers with the published
shape, and nullable fields are sent rather than omitted so a generated client
sees one shape.
Also splits proto.rs and server.rs, whose concerns had diverged, and applies
the review's simplifications.
SOULSEEK_E2E_MODE=daemon makes the existing e2e harness serve every session
from a daemon instead of the command's own login, so the same 83 tests assert
the same records and exit codes both ways. CI runs the suite twice.
Two defects it found:
serve judged whether anything was shared from this run's configuration
rather than from the session, so routed to a daemon it would start with
nothing to serve and report success.
The TUI showed an empty conversation when attached, because the daemon's
message history was never asked for. It is now a SessionApi method, empty
locally, which puts the difference behind the seam.
main.rs is now a thin entry point over the library rather than a second copy
of every module, so the program compiles once instead of twice.
The daemon drains the library's destructive take_* accessors once and fans
events out per connection, which is what lets more than one client share a
session. A Unix socket at 0600 is the local transport and its permissions
are the authentication; TCP is opt-in and needs the generated token. Rooms,
transfers, and the conversation are daemon-owned and restored on restart.
Methods and events are enums built by one macro, so every consumer matches
exhaustively: adding a member is a compile error until the dispatcher, the
published schema, and the remote client all handle it. docs/openrpc.json is
derived from the DTOs rather than hand-written, and a test fails the build
when the checked-in copy drifts.
Michel de Graaf