dumbgoy/soulseek-rs-modded
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor: cut what a whole-repo audit proved dead, padded, or hand-rolled

Browse Source 
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.
This commit is contained in:
Michel de Graaf
2026-07-28 12:52:18 +02:00
parent cd56fb3b39
commit f1b667f7f9
62 changed files with 1367 additions and 2319 deletions
Download Patch File Download Diff File Expand all files Collapse all files
soulseek-rs/tests/cli_e2e.rs
+21 -87
View File
@@ -9,8 +9,9 @@
//! with a notice. `SOULSEEK_E2E_REQUIRED=1` turns a missing server into a
//! failure, which is how CI guarantees these actually run.
#![allow(clippy::doc_markdown)]
mod common;
use common::{CLI_ENV_VARS, free_port, soulfind_binary};
use soulseek_rs::{Client, ClientSettings, PeerAddress};
use std::io::Write;
use std::net::{TcpStream, ToSocketAddrs};
@@ -28,26 +29,6 @@ const EXIT_CONNECTION: i32 = 3;
const EXIT_NO_RESULTS: i32 = 4;
const EXIT_SESSION_LOST: i32 = 7;
/// Every environment variable the CLI reads, cleared for each run so a
/// developer's shell (or a stray `.env`) cannot change what a test observes.
const CLI_ENV_VARS: [&str; 15] = [
"SOULSEEK_CONFIG_DIR",
"SOULSEEK_STATE_DIR",
"SOULSEEK_DAEMON",
"SOULSEEK_DAEMON_TOKEN",
"SOULSEEK_USERNAME",
"SOULSEEK_PASSWORD",
"SOULSEEK_PASSWORD_CMD",
"SOULSEEK_SERVER",
"SOULSEEK_NO_LISTENER",
"SOULSEEK_LISTENER_PORT",
"SOULSEEK_DOWNLOAD_DIR",
"SOULSEEK_SHARED_DIR",
"SOULSEEK_MAX_CONCURRENT_DOWNLOADS",
"SOULSEEK_SEARCH_TIMEOUT",
"SOULSEEK_CONFIG",
];
/// Build a command for the binary, isolated from the developer's environment
/// and from the repository's own working directory.
///
@@ -281,9 +262,7 @@ fn without_a_terminal_the_tui_reports_instead_of_panicking() {
#[test]
fn portmap_emits_a_record_whose_verdict_matches_the_exit_code() {
let output = run(&["--no-config", "--quiet", "--json", "portmap"]);
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("portmap should emit valid JSON");
let record = json_record(&output);
assert!(record["port"].is_number());
let ok = record["ok"].as_bool().expect("ok should be a boolean");
@@ -316,10 +295,7 @@ fn quiet_keeps_progress_off_stderr() {
/// What the first record says happened, parsed rather than matched as text.
fn action(output: &Output) -> String {
let record: serde_json::Value =
serde_json::from_str(records(output).first().expect("one record"))
.expect("valid JSON");
record["action"]
json_record(output)["action"]
.as_str()
.expect("a record carries an action")
.to_string()
@@ -828,25 +804,6 @@ impl Drop for TestServer {
}
}
fn soulfind_binary() -> Option<PathBuf> {
if let Ok(path) = std::env::var("SOULFIND_BIN") {
let path = PathBuf::from(path);
return path.exists().then_some(path);
}
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.ancestors()
.map(|dir| dir.join("soulfind/bin/soulfind"))
.find(|candidate| candidate.exists())
}
fn free_port() -> Option<u16> {
std::net::TcpListener::bind("127.0.0.1:0")
.ok()?
.local_addr()
.ok()
.map(|addr| addr.port())
}
fn wait_until_listening(
host: &str,
port: u16,
@@ -1993,9 +1950,7 @@ fn whoami_confirms_the_login_and_reports_what_we_offer() {
);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&output);
assert_eq!(record["user"], "cli_e2e_whoami");
assert_eq!(record["shared_files"], 1, "the shared file is indexed");
assert!(record["server"].as_str().is_some_and(|s| s.contains(':')));
@@ -2009,8 +1964,7 @@ fn whoami_reports_how_much_privilege_time_the_account_has() {
let output = cli(&server, "cli_e2e_priv_owner", &["--json", "whoami"]);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(&records(&output)[0]).expect("valid JSON");
let record = json_record(&output);
// Zero, not null: the server answered, and the answer is "no privileges".
// Null would mean it never said, which is a different fact.
assert_eq!(
@@ -2156,9 +2110,7 @@ fn user_reports_a_peers_status_and_share_counts() {
);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&output);
assert_eq!(record["user"], "cli_e2e_subject");
assert_ne!(record["status"], "offline", "the subject is logged in");
assert_eq!(record["shared_files"], 2);
@@ -2292,9 +2244,7 @@ fn shares_status_reports_what_the_network_will_see() {
);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&output);
assert_eq!(record["files"], 3);
assert!(record["folders"].as_u64().is_some_and(|n| n >= 1));
}
@@ -2321,9 +2271,7 @@ fn shares_reindex_picks_up_a_file_added_after_startup() {
);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&output);
assert_eq!(record["files"], 2, "re-indexing should see both files");
}
@@ -2569,9 +2517,7 @@ fn asking_about_an_unknown_user_succeeds_with_an_offline_record() {
);
assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output));
let record: serde_json::Value =
serde_json::from_str(records(&output).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&output);
assert_eq!(record["status"], "offline");
assert_eq!(record["shared_files"], 0);
}
@@ -2585,10 +2531,7 @@ fn assert_record_keys(output: &Output, label: &str, keys: &[&str]) {
"`{label}` should succeed, stderr: {}",
stderr(output)
);
let record: serde_json::Value = serde_json::from_str(
records(output).first().expect("at least one record"),
)
.expect("valid JSON");
let record = json_record(output);
for key in keys {
assert!(
record.get(*key).is_some(),
@@ -3071,9 +3014,7 @@ fn json_records_carry_every_extra_field_the_readme_names() {
// `portmap` adds the port it probed; the verdict lives in the exit code,
// which is `EXIT_NO_RESULTS` on a network with no cooperating router.
let portmap = cli(&server, "cli_e2e_keys", &["--json", "portmap"]);
let record: serde_json::Value =
serde_json::from_str(records(&portmap).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&portmap);
for key in ["ok", "backend", "external", "port"] {
assert!(record.get(key).is_some(), "portmap should carry `{key}`");
}
@@ -3347,9 +3288,7 @@ fn a_daemon_reports_its_session_and_stops_on_request() {
let status =
run(&["--no-config", "--json", "--daemon", &at, "daemon", "status"]);
assert_eq!(code(&status), EXIT_OK, "stderr: {}", stderr(&status));
let record: serde_json::Value =
serde_json::from_str(records(&status).first().expect("one record"))
.expect("valid JSON");
let record = json_record(&status);
assert_eq!(record["user"], user);
assert_eq!(record["server"], server.address());
assert!(
@@ -3443,19 +3382,14 @@ fn adding_a_share_takes_effect_on_a_running_daemon() {
// The daemon re-indexes on the way through, so the folder is visible to
// the network now rather than after a restart.
let after: serde_json::Value = serde_json::from_str(
records(&run(&[
"--no-config",
"--json",
"--daemon",
&at,
"shares",
"status",
]))
.first()
.expect("one record"),
)
.expect("valid JSON");
let after = json_record(&run(&[
"--no-config",
"--json",
"--daemon",
&at,
"shares",
"status",
]));
let listed = after["directories"]
.as_array()
.expect("directories is an array");
soulseek-rs/tests/common/mod.rs
+46
View File
@@ -0,0 +1,46 @@
//! Scaffolding both CLI end-to-end binaries need: the environment they scrub,
//! the soulfind they run against, and a port nobody else holds.
#![allow(dead_code)]
use std::path::{Path, PathBuf};
/// Every environment variable the binary reads, cleared for each run so a
/// developer's shell (or a stray `.env`) cannot change what a test observes —
/// and so no run finds a real daemon.
pub const CLI_ENV_VARS: [&str; 15] = [
"SOULSEEK_CONFIG_DIR",
"SOULSEEK_STATE_DIR",
"SOULSEEK_DAEMON",
"SOULSEEK_DAEMON_TOKEN",
"SOULSEEK_USERNAME",
"SOULSEEK_PASSWORD",
"SOULSEEK_PASSWORD_CMD",
"SOULSEEK_SERVER",
"SOULSEEK_NO_LISTENER",
"SOULSEEK_LISTENER_PORT",
"SOULSEEK_DOWNLOAD_DIR",
"SOULSEEK_SHARED_DIR",
"SOULSEEK_MAX_CONCURRENT_DOWNLOADS",
"SOULSEEK_SEARCH_TIMEOUT",
"SOULSEEK_CONFIG",
];
pub fn soulfind_binary() -> Option<PathBuf> {
if let Ok(path) = std::env::var("SOULFIND_BIN") {
let path = PathBuf::from(path);
return path.exists().then_some(path);
}
Path::new(env!("CARGO_MANIFEST_DIR"))
.ancestors()
.map(|dir| dir.join("soulfind/bin/soulfind"))
.find(|candidate| candidate.exists())
}
pub fn free_port() -> Option<u16> {
std::net::TcpListener::bind("127.0.0.1:0")
.ok()?
.local_addr()
.ok()
.map(|addr| addr.port())
}
soulseek-rs/tests/daemon_protocol_e2e.rs
+24 -81
View File
@@ -11,39 +11,23 @@
//! Like the rest of the e2e suite this needs a soulfind server and skips
//! without one; `SOULSEEK_E2E_REQUIRED=1` turns that skip into a failure.
mod common;
use common::{CLI_ENV_VARS, free_port, soulfind_binary};
use std::io::{BufRead, BufReader, Write};
use std::net::TcpStream;
use std::path::{Path, PathBuf};
use std::process::{Child, Command, Stdio};
use std::time::{Duration, Instant};
use tempfile::TempDir;
const BIN: &str = env!("CARGO_BIN_EXE_soulseek-rs");
/// Every environment variable the binary reads, cleared so a developer's shell
/// cannot change what a test observes — and so no run finds a real daemon.
const CLI_ENV_VARS: [&str; 15] = [
"SOULSEEK_CONFIG_DIR",
"SOULSEEK_STATE_DIR",
"SOULSEEK_DAEMON",
"SOULSEEK_DAEMON_TOKEN",
"SOULSEEK_USERNAME",
"SOULSEEK_PASSWORD",
"SOULSEEK_PASSWORD_CMD",
"SOULSEEK_SERVER",
"SOULSEEK_NO_LISTENER",
"SOULSEEK_LISTENER_PORT",
"SOULSEEK_DOWNLOAD_DIR",
"SOULSEEK_SHARED_DIR",
"SOULSEEK_MAX_CONCURRENT_DOWNLOADS",
"SOULSEEK_SEARCH_TIMEOUT",
"SOULSEEK_CONFIG",
];
/// A daemon bound to loopback TCP as well as its socket, so both transports
/// can be exercised against one session.
struct Daemon {
child: Child,
state: PathBuf,
state: TempDir,
port: u16,
_server: Soulfind,
}
@@ -63,10 +47,10 @@ fn cli_command(state: &Path) -> Command {
impl Daemon {
fn start() -> Option<Self> {
let server = Soulfind::start()?;
let state = scratch("daemon-proto");
let state = tempfile::tempdir().ok()?;
let port = free_port()?;
let mut command = cli_command(&state);
let mut command = cli_command(state.path());
command.args([
"--no-config",
"--server",
@@ -78,7 +62,7 @@ impl Daemon {
"--listener-port",
&free_port()?.to_string(),
"--download-dir",
state.join("downloads").to_str()?,
state.path().join("downloads").to_str()?,
"daemon",
"--bind",
&format!("127.0.0.1:{port}"),
@@ -121,13 +105,13 @@ impl Daemon {
}
fn token(&self) -> String {
std::fs::read_to_string(self.state.join("daemon.token"))
std::fs::read_to_string(self.state.path().join("daemon.token"))
.map(|token| token.trim().to_string())
.unwrap_or_default()
}
fn socket_path(&self) -> PathBuf {
self.state.join("daemon.sock")
self.state.path().join("daemon.sock")
}
fn connect_tcp(&self) -> Option<Connection> {
@@ -152,7 +136,6 @@ impl Drop for Daemon {
fn drop(&mut self) {
let _ = self.child.kill();
let _ = self.child.wait();
let _ = std::fs::remove_dir_all(&self.state);
}
}
@@ -426,7 +409,7 @@ fn discover_serves_a_contract_whose_parameters_match_what_is_accepted() {
// ---------------------------------------------------------------------------
struct Soulfind {
child: Option<Child>,
child: Child,
db: PathBuf,
port: u16,
}
@@ -438,7 +421,7 @@ impl Soulfind {
let db = std::env::temp_dir().join(format!("soulfind-proto-{port}.db"));
let _ = std::fs::remove_file(&db);
let child = Command::new(bin)
let mut child = Command::new(bin)
.args(["-d", db.to_str()?, "-p", &port.to_string()])
.stdout(Stdio::null())
.stderr(Stdio::null())
@@ -448,15 +431,10 @@ impl Soulfind {
let deadline = Instant::now() + Duration::from_secs(10);
while Instant::now() < deadline {
if TcpStream::connect(("127.0.0.1", port)).is_ok() {
return Some(Self {
child: Some(child),
db,
port,
});
return Some(Self { child, db, port });
}
std::thread::sleep(Duration::from_millis(100));
}
let mut child = child;
let _ = child.kill();
let _ = child.wait();
None
@@ -469,43 +447,12 @@ impl Soulfind {
impl Drop for Soulfind {
fn drop(&mut self) {
if let Some(child) = self.child.as_mut() {
let _ = child.kill();
let _ = child.wait();
}
let _ = self.child.kill();
let _ = self.child.wait();
let _ = std::fs::remove_file(&self.db);
}
}
fn soulfind_binary() -> Option<PathBuf> {
if let Ok(path) = std::env::var("SOULFIND_BIN") {
let path = PathBuf::from(path);
return path.exists().then_some(path);
}
Path::new(env!("CARGO_MANIFEST_DIR"))
.ancestors()
.map(|dir| dir.join("soulfind/bin/soulfind"))
.find(|candidate| candidate.exists())
}
fn free_port() -> Option<u16> {
std::net::TcpListener::bind("127.0.0.1:0")
.ok()?
.local_addr()
.ok()
.map(|addr| addr.port())
}
fn scratch(label: &str) -> PathBuf {
static COUNTER: std::sync::atomic::AtomicU32 =
std::sync::atomic::AtomicU32::new(0);
let n = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
let path = std::env::temp_dir()
.join(format!("soulseek-{label}-{}-{n}", std::process::id()));
std::fs::create_dir_all(&path).expect("scratch directory");
path
}
/// A client configured for a remote daemon reports the *daemon's* world.
///
/// This is the download-box workflow: the token lives in the client's config
@@ -517,13 +464,13 @@ fn scratch(label: &str) -> PathBuf {
#[test]
fn a_config_driven_remote_client_reports_the_daemons_world() {
let daemon = daemon_or_skip!();
let config = scratch("remote-client-config");
let state = scratch("remote-client-state");
let config = tempfile::tempdir().expect("config directory");
let state = tempfile::tempdir().expect("state directory");
// A laptop that knows only where the daemon is. No username, no password,
// no server — exactly what the documented setup produces.
std::fs::write(
config.join("config.toml"),
config.path().join("config.toml"),
format!(
"daemon = \"127.0.0.1:{}\"\ndaemon_token = \"{}\"\n",
daemon.port,
@@ -532,8 +479,8 @@ fn a_config_driven_remote_client_reports_the_daemons_world() {
)
.expect("config file");
let mut command = cli_command(&state);
command.env("SOULSEEK_CONFIG_DIR", &config);
let mut command = cli_command(state.path());
command.env("SOULSEEK_CONFIG_DIR", config.path());
command.args(["--json", "whoami"]);
let output = command.output().expect("the binary should run");
@@ -567,9 +514,6 @@ fn a_config_driven_remote_client_reports_the_daemons_world() {
"the directory the bytes actually land in, on the daemon's host, not \
this machine's Downloads folder: {record}"
);
let _ = std::fs::remove_dir_all(&config);
let _ = std::fs::remove_dir_all(&state);
}
/// Several clients at once, all seeing the same session.
@@ -813,7 +757,8 @@ fn the_download_folder_moves_when_asked_and_status_says_so() {
let mut connection = daemon.connect_socket().expect("socket");
connection.authenticate(None).expect("an answer");
let moved = scratch("moved-downloads").join("deeper");
let downloads = tempfile::tempdir().expect("downloads directory");
let moved = downloads.path().join("deeper");
let reply = connection
.call(&format!(
r#"{{"jsonrpc":"2.0","id":2,"method":"download.set_dir","params":{{"directory":{}}}}}"#,
@@ -844,7 +789,6 @@ fn the_download_folder_moves_when_asked_and_status_says_so() {
)
.expect("an answer");
assert!(refused.contains("-32602"), "got {refused}");
let _ = std::fs::remove_dir_all(moved.parent().expect("parent"));
}
/// A wishlist search must read as *running* to the other windows.
@@ -906,9 +850,9 @@ fn the_upload_slot_limit_can_be_retuned_live() {
#[test]
fn the_cli_reports_a_bad_token_as_a_connection_error() {
let daemon = daemon_or_skip!();
let state = scratch("cli-bad-token");
let state = tempfile::tempdir().expect("state directory");
let run = |token: Option<&str>| {
let mut command = cli_command(&state);
let mut command = cli_command(state.path());
command.args([
"--no-config",
"--daemon",
@@ -951,5 +895,4 @@ fn the_cli_reports_a_bad_token_as_a_connection_error() {
"the real token must keep working: {}",
String::from_utf8_lossy(&right.stderr)
);
let _ = std::fs::remove_dir_all(&state);
}