fix!: make concurrent invocations work instead of silently seeing nothing (#22)
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.
This commit is contained in:
Michel de Graaf
@@ -50,7 +50,8 @@ soulseek-rs search "rjd2" --json \
|
||||
```
|
||||
|
||||
Exit codes: `0` success, `2` usage or config, `3` connect or login, `4` nothing
|
||||
found, `5` timed out, `6` transfer failed, `1` unexpected error.
|
||||
found, `5` timed out, `6` transfer failed, `7` session lost mid-command, `1`
|
||||
unexpected error.
|
||||
|
||||
The [full README](https://github.com/michel/soulseek-rs#readme) documents every
|
||||
command, record shape, flag, environment variable and `config.toml` key.
|
||||
|
||||
@@ -38,6 +38,7 @@ that form; the columns are a subset of the JSON keys.
|
||||
| 4 | It worked and found nothing | Widen or correct the query — retrying it verbatim will not help |
|
||||
| 5 | Gave up waiting | Raise `--timeout`, or `--search-timeout` for searches |
|
||||
| 6 | A transfer started and did not finish | The peer stalled; try another result |
|
||||
| 7 | The session ended mid-command (usually another login took the username) | Nothing this run saw is reliable; retry, and give each concurrent run its own `--username` |
|
||||
|
||||
## Commands
|
||||
|
||||
@@ -148,6 +149,22 @@ soulseek-rs user someuser --json
|
||||
`download --stdin` reads whole records, so filtering with `jq` and piping back
|
||||
is the normal way to be selective.
|
||||
|
||||
## Running several at once
|
||||
|
||||
The server allows one session per account, and the later login wins: two runs
|
||||
sharing a username silently kill each other's session. Give every concurrent
|
||||
run its own name.
|
||||
|
||||
```bash
|
||||
for q in 'gary beck fold' 'marcal steady'; do
|
||||
soulseek-rs search "$q" --username "$SOULSEEK_USERNAME-$RANDOM" --json &
|
||||
done
|
||||
wait
|
||||
```
|
||||
|
||||
The listener port needs no such care: a run that finds the configured port
|
||||
taken binds a free one and tells the server about that one instead.
|
||||
|
||||
## Rules
|
||||
|
||||
- **Remote paths belong to the peer.** They are backslash-separated and may
|
||||
@@ -159,6 +176,8 @@ is the normal way to be selective.
|
||||
searches. There is no unbounded form.
|
||||
- **Exit 4 is not a failure.** A search that finds nothing means the query was
|
||||
too narrow or misspelled. Widen it; use `whoami` to rule out the connection.
|
||||
- **Exit 7 is not an answer.** The session ended mid-command, so nothing it saw
|
||||
counts. Retry under a name no other run is using.
|
||||
- **Never put a password in argv** — `ps` shows it to every user on the box.
|
||||
Use `--password-stdin`, the OS keychain, or `SOULSEEK_PASSWORD_CMD`.
|
||||
- **Downloading pulls a file off a stranger's machine.** Confirm with the user
|
||||
|
||||
@@ -31,7 +31,7 @@ Examples:
|
||||
|
||||
Exit codes:
|
||||
0 success 2 usage/config 3 connect or login 4 no results
|
||||
5 timed out 6 transfer failed 1 unexpected error";
|
||||
5 timed out 6 transfer failed 7 session lost 1 unexpected error";
|
||||
|
||||
#[derive(Parser, Debug, Default)]
|
||||
#[command(
|
||||
|
||||
@@ -17,9 +17,9 @@ use crate::cli::{Commands, MessageCommand, RoomCommand};
|
||||
use crate::output::{CliError, CliResult, Exit, Out, PortmapRecord};
|
||||
use crate::port_mapping::{self, PortMapper};
|
||||
use soulseek_rs::types::{RoomEvent, RoomInfo};
|
||||
use soulseek_rs::{Client, ClientSettings};
|
||||
use soulseek_rs::{Client, ClientSettings, SessionLoss};
|
||||
use std::net::{SocketAddr, TcpStream, ToSocketAddrs};
|
||||
use std::sync::Arc;
|
||||
use std::sync::{Arc, OnceLock};
|
||||
use std::time::{Duration, Instant};
|
||||
|
||||
/// Everything a one-shot command needs that does not come from its own flags.
|
||||
@@ -29,6 +29,47 @@ pub struct Ctx {
|
||||
pub download_dir: String,
|
||||
pub max_concurrent_downloads: usize,
|
||||
pub search_timeout: Duration,
|
||||
/// The client of the session this command opened, once it has one, so the
|
||||
/// verdict can be checked against the session that produced it.
|
||||
pub session: OnceLock<Arc<Client>>,
|
||||
}
|
||||
|
||||
impl Ctx {
|
||||
/// Re-read a "found nothing", "timed out" or "transfer failed" verdict
|
||||
/// against the session that produced it.
|
||||
///
|
||||
/// A session the server cut off — most often because another process
|
||||
/// logged in under the same name — sees nothing at all from that moment
|
||||
/// on. Reported as exit 4 it reads as "this file is not on the network",
|
||||
/// and a caller acting on that gives up on files that were there.
|
||||
fn checked(&self, result: CliResult) -> CliResult {
|
||||
let Err(error) = result else {
|
||||
return Ok(());
|
||||
};
|
||||
if !matches!(
|
||||
error.exit,
|
||||
Exit::NoResults | Exit::Timeout | Exit::Transfer
|
||||
) {
|
||||
return Err(error);
|
||||
}
|
||||
match self.session.get().and_then(|client| client.session_loss()) {
|
||||
Some(loss) => Err(session_lost(loss)),
|
||||
None => Err(error),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// What to tell a caller whose session ended under it.
|
||||
fn session_lost(loss: SessionLoss) -> CliError {
|
||||
let fix = match loss {
|
||||
SessionLoss::Displaced => {
|
||||
"; give each concurrent run its own --username"
|
||||
}
|
||||
SessionLoss::Disconnected => "",
|
||||
};
|
||||
CliError::session_lost(format!(
|
||||
"the session ended before this command finished: {loss}{fix}"
|
||||
))
|
||||
}
|
||||
|
||||
/// A logged-in client plus, while the listener is enabled, the port mapping
|
||||
@@ -45,21 +86,33 @@ const LOGIN_TIMEOUT: Duration = Duration::from_secs(30);
|
||||
|
||||
impl Session {
|
||||
pub fn open(ctx: &Ctx) -> CliResult<Self> {
|
||||
let mapper = ctx
|
||||
.settings
|
||||
.enable_listen
|
||||
.then(|| PortMapper::spawn(ctx.settings.listen_port));
|
||||
|
||||
let address = ctx.settings.server_address.to_string();
|
||||
ctx.out.status(&format!("connecting to {address}…"));
|
||||
reachable(&address, REACH_TIMEOUT)?;
|
||||
|
||||
let client = login(ctx.settings.clone(), &address)?;
|
||||
let client = Arc::new(login(ctx.settings.clone(), &address)?);
|
||||
ctx.out
|
||||
.status(&format!("logged in as {}", ctx.settings.username));
|
||||
let _ = ctx.session.set(Arc::clone(&client));
|
||||
|
||||
let bound = client.listen_port();
|
||||
if let Some(bound) = bound
|
||||
&& ctx.settings.listen_port != 0
|
||||
&& bound != ctx.settings.listen_port
|
||||
{
|
||||
ctx.out.warn(&format!(
|
||||
"listener port {} is in use; listening on {bound} instead",
|
||||
ctx.settings.listen_port
|
||||
));
|
||||
}
|
||||
|
||||
// Ask the router for the port the listener actually holds: when the
|
||||
// configured one was taken, mapping it would open a hole to whoever
|
||||
// did take it.
|
||||
let mapper = bound.map(PortMapper::spawn);
|
||||
|
||||
Ok(Self {
|
||||
client: Arc::new(client),
|
||||
client,
|
||||
_mapper: mapper,
|
||||
})
|
||||
}
|
||||
@@ -167,6 +220,10 @@ pub fn confirm_flush(client: &Client, timeout: Duration) -> bool {
|
||||
}
|
||||
|
||||
pub fn run(ctx: &Ctx, command: Commands) -> CliResult {
|
||||
ctx.checked(dispatch(ctx, command))
|
||||
}
|
||||
|
||||
fn dispatch(ctx: &Ctx, command: Commands) -> CliResult {
|
||||
match command {
|
||||
Commands::Search(args) => transfer::search(ctx, &args),
|
||||
Commands::Download(args) => transfer::download(ctx, &args),
|
||||
|
||||
@@ -20,11 +20,14 @@ const PRIVILEGE_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
pub fn whoami(ctx: &Ctx) -> CliResult {
|
||||
let session = Session::open(ctx)?;
|
||||
let (folders, files) = session.client.shared_counts();
|
||||
// The bound port, not the configured one: a caller checking whether it is
|
||||
// reachable needs what is true, not what was asked for.
|
||||
let bound = session.client.listen_port();
|
||||
ctx.out.emit(&WhoamiRecord {
|
||||
user: ctx.settings.username.clone(),
|
||||
server: ctx.settings.server_address.to_string(),
|
||||
listening: ctx.settings.enable_listen,
|
||||
listen_port: ctx.settings.listen_port,
|
||||
listening: bound.is_some(),
|
||||
listen_port: bound.unwrap_or(0),
|
||||
shared_folders: folders,
|
||||
shared_files: files,
|
||||
download_dir: ctx.download_dir.clone(),
|
||||
@@ -119,7 +122,7 @@ pub fn serve(
|
||||
let (folders, files) = session.client.shared_counts();
|
||||
ctx.out.status(&format!(
|
||||
"serving {files} files in {folders} folders on port {}",
|
||||
ctx.settings.listen_port
|
||||
session.client.listen_port().unwrap_or_default()
|
||||
));
|
||||
if files == 0 {
|
||||
ctx.out
|
||||
|
||||
@@ -182,6 +182,7 @@ fn context(
|
||||
download_dir: resolved.download_dir.clone(),
|
||||
max_concurrent_downloads: resolved.max_concurrent_downloads,
|
||||
search_timeout: Duration::from_secs(resolved.search_timeout),
|
||||
session: std::sync::OnceLock::new(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -298,11 +299,6 @@ fn run_default_tui(
|
||||
// Enable logger buffering BEFORE connection to prevent log artifacts
|
||||
soulseek_rs::utils::logger::enable_buffering();
|
||||
|
||||
// Best-effort: make ourselves reachable behind a home router so
|
||||
// firewalled peers can connect back. Kept alive for the session.
|
||||
let _port_mapper = (!resolved.disable_listener)
|
||||
.then(|| port_mapping::PortMapper::spawn(resolved.listener_port));
|
||||
|
||||
let enable_listen = !resolved.disable_listener;
|
||||
let listen_port = resolved.listener_port;
|
||||
let make_settings =
|
||||
@@ -345,6 +341,14 @@ fn run_default_tui(
|
||||
|
||||
persist_credentials(&outcome, config_path, file_config, &secret_store);
|
||||
|
||||
// Best-effort: make ourselves reachable behind a home router so firewalled
|
||||
// peers can connect back. Mapped only once the listener is up, and for the
|
||||
// port it really bound. Kept alive for the session.
|
||||
let _port_mapper = outcome
|
||||
.client
|
||||
.listen_port()
|
||||
.map(port_mapping::PortMapper::spawn);
|
||||
|
||||
let store =
|
||||
persist::paths::state_dir().map(persist::state::StateStore::new);
|
||||
|
||||
|
||||
@@ -26,6 +26,9 @@ pub enum Exit {
|
||||
Timeout = 5,
|
||||
/// A transfer started but did not complete.
|
||||
Transfer = 6,
|
||||
/// The server session ended mid-command, so nothing this run saw (or did
|
||||
/// not see) means anything. Retry rather than believe it.
|
||||
SessionLost = 7,
|
||||
}
|
||||
|
||||
impl Exit {
|
||||
@@ -75,6 +78,10 @@ impl CliError {
|
||||
pub fn no_results(message: impl Into<String>) -> Self {
|
||||
Self::new(Exit::NoResults, message)
|
||||
}
|
||||
|
||||
pub fn session_lost(message: impl Into<String>) -> Self {
|
||||
Self::new(Exit::SessionLost, message)
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for CliError {
|
||||
@@ -528,6 +535,7 @@ mod tests {
|
||||
assert_eq!(Exit::NoResults.code(), 4);
|
||||
assert_eq!(Exit::Timeout.code(), 5);
|
||||
assert_eq!(Exit::Transfer.code(), 6);
|
||||
assert_eq!(Exit::SessionLost.code(), 7);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -26,6 +26,7 @@ const EXIT_OK: i32 = 0;
|
||||
const EXIT_USAGE: i32 = 2;
|
||||
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.
|
||||
@@ -114,7 +115,7 @@ fn help_lists_every_scriptable_command() {
|
||||
fn help_documents_the_exit_codes_a_script_branches_on() {
|
||||
let help = stdout(&run(&["--help"]));
|
||||
assert!(help.contains("Exit codes"), "help should list exit codes");
|
||||
for code in ["2", "3", "4", "5", "6"] {
|
||||
for code in ["2", "3", "4", "5", "6", "7"] {
|
||||
assert!(help.contains(code), "exit code {code} should be documented");
|
||||
}
|
||||
}
|
||||
@@ -552,6 +553,12 @@ impl TestServer {
|
||||
/// connection the *uploader* opens back to us, so a downloader without a
|
||||
/// listener never receives its file.
|
||||
fn args(&self, user: &str) -> Vec<String> {
|
||||
self.args_on(user, free_port().expect("listener port"))
|
||||
}
|
||||
|
||||
/// The same arguments on a chosen listener port, for the tests about what
|
||||
/// happens when two runs want one port.
|
||||
fn args_on(&self, user: &str, port: u16) -> Vec<String> {
|
||||
vec![
|
||||
"--no-config".to_string(),
|
||||
"--quiet".to_string(),
|
||||
@@ -562,7 +569,7 @@ impl TestServer {
|
||||
"--password".to_string(),
|
||||
"pw".to_string(),
|
||||
"--listener-port".to_string(),
|
||||
free_port().expect("listener port").to_string(),
|
||||
port.to_string(),
|
||||
]
|
||||
}
|
||||
|
||||
@@ -2800,3 +2807,183 @@ fn json_records_carry_every_extra_field_the_readme_names() {
|
||||
assert!(record.get(key).is_some(), "portmap should carry `{key}`");
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Several invocations at once: what an agent driving this CLI actually does.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Spawn the binary as `user` with an explicit listener port, without waiting.
|
||||
fn spawn_cli(
|
||||
server: &TestServer,
|
||||
user: &str,
|
||||
port: u16,
|
||||
args: &[&str],
|
||||
) -> std::process::Child {
|
||||
let mut all = server.args_on(user, port);
|
||||
all.extend(args.iter().map(|a| (*a).to_string()));
|
||||
let refs: Vec<&str> = all.iter().map(String::as_str).collect();
|
||||
command(&refs)
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.spawn()
|
||||
.expect("the binary should start")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn concurrent_searches_sharing_a_listener_port_all_find_the_file() {
|
||||
let server = server_or_skip!();
|
||||
let share = Scratch::new("share");
|
||||
std::fs::write(share.path().join("cli_probe_multi.bin"), probe_bytes())
|
||||
.expect("share file");
|
||||
let _sharer = server.client("cli_e2e_sharer_multi", vec![share.display()]);
|
||||
settle();
|
||||
|
||||
// One configured port between them, which is what a config file gives
|
||||
// three agents started at the same moment. Losing the race for it used to
|
||||
// cost that invocation its listener, and with it every search result.
|
||||
let port = free_port().expect("shared listener port");
|
||||
// Collected on purpose: all three have to be running before any is waited
|
||||
// on, which is the whole point of the test.
|
||||
#[allow(clippy::needless_collect)]
|
||||
let running: Vec<_> = (0..3)
|
||||
.map(|i| {
|
||||
spawn_cli(
|
||||
&server,
|
||||
&format!("cli_e2e_par_{i}"),
|
||||
port,
|
||||
&["--search-timeout", "10", "search", "multi"],
|
||||
)
|
||||
})
|
||||
.collect();
|
||||
|
||||
for (i, child) in running.into_iter().enumerate() {
|
||||
let output = child.wait_with_output().expect("wait");
|
||||
assert_eq!(
|
||||
code(&output),
|
||||
EXIT_OK,
|
||||
"run {i} failed: {}",
|
||||
stderr(&output)
|
||||
);
|
||||
assert!(
|
||||
records(&output)
|
||||
.iter()
|
||||
.any(|line| line.contains("cli_probe_multi")),
|
||||
"run {i} listed nothing"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_session_taken_over_mid_search_is_not_reported_as_no_results() {
|
||||
let server = server_or_skip!();
|
||||
let name = "cli_e2e_evicted";
|
||||
|
||||
// Run it loud, so its own progress line says when it is logged in: the
|
||||
// eviction has to land after that, or this test would race the login it is
|
||||
// trying to interrupt.
|
||||
let mut args = server.args_on(name, free_port().expect("listener port"));
|
||||
args.retain(|arg| arg != "--quiet");
|
||||
args.extend(
|
||||
["--search-timeout", "20", "search", "nothingmatchesthisqzx"]
|
||||
.map(String::from),
|
||||
);
|
||||
let refs: Vec<&str> = args.iter().map(String::as_str).collect();
|
||||
let mut searching = command(&refs)
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.spawn()
|
||||
.expect("the binary should start");
|
||||
|
||||
let (logged_in, wait_for_login) = std::sync::mpsc::channel();
|
||||
let stderr = searching.stderr.take().expect("piped stderr");
|
||||
let progress = std::thread::spawn(move || {
|
||||
let mut seen = String::new();
|
||||
for line in std::io::BufRead::lines(std::io::BufReader::new(stderr))
|
||||
.map_while(Result::ok)
|
||||
{
|
||||
if line.contains("logged in as") {
|
||||
let _ = logged_in.send(());
|
||||
}
|
||||
seen.push_str(&line);
|
||||
seen.push('\n');
|
||||
}
|
||||
seen
|
||||
});
|
||||
wait_for_login
|
||||
.recv_timeout(Duration::from_mins(1))
|
||||
.expect("the search should log in");
|
||||
|
||||
// Now take the username away, the way a second invocation under one
|
||||
// account does.
|
||||
let _thief = server.client(name, Vec::new());
|
||||
|
||||
let status = searching.wait().expect("wait");
|
||||
let seen = progress.join().expect("progress reader");
|
||||
assert_eq!(
|
||||
status.code(),
|
||||
Some(EXIT_SESSION_LOST),
|
||||
"a killed session must not look like an answered query: {seen}"
|
||||
);
|
||||
|
||||
let mut written = String::new();
|
||||
std::io::Read::read_to_string(
|
||||
&mut searching.stdout.take().expect("piped stdout"),
|
||||
&mut written,
|
||||
)
|
||||
.expect("read stdout");
|
||||
assert!(written.trim().is_empty(), "stdout stays data-only");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn concurrent_downloads_sharing_a_listener_port_all_land() {
|
||||
let server = server_or_skip!();
|
||||
let share = Scratch::new("share");
|
||||
std::fs::write(share.path().join("cli_probe_dual.bin"), probe_bytes())
|
||||
.expect("share file");
|
||||
let _sharer = server.client("cli_e2e_sharer_dual", vec![share.display()]);
|
||||
settle();
|
||||
|
||||
// A transfer arrives over a connection the *uploader* opens back to us, so
|
||||
// a run without a listener never receives its file however well its search
|
||||
// went. Two `get` runs, one configured port between them.
|
||||
let port = free_port().expect("shared listener port");
|
||||
let targets: Vec<Scratch> =
|
||||
(0..2).map(|i| Scratch::new(&format!("dl{i}"))).collect();
|
||||
// Collected on purpose: both have to be running before either is waited
|
||||
// on, which is the whole point of the test.
|
||||
#[allow(clippy::needless_collect)]
|
||||
let running: Vec<_> = targets
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(i, target)| {
|
||||
spawn_cli(
|
||||
&server,
|
||||
&format!("cli_e2e_dual_{i}"),
|
||||
port,
|
||||
&[
|
||||
"--download-dir",
|
||||
&target.display(),
|
||||
"--search-timeout",
|
||||
"10",
|
||||
"get",
|
||||
"dual",
|
||||
"--timeout",
|
||||
"40",
|
||||
],
|
||||
)
|
||||
})
|
||||
.collect();
|
||||
|
||||
for (i, (child, target)) in running.into_iter().zip(&targets).enumerate() {
|
||||
let output = child.wait_with_output().expect("wait");
|
||||
assert_eq!(
|
||||
code(&output),
|
||||
EXIT_OK,
|
||||
"run {i} failed: {}",
|
||||
stderr(&output)
|
||||
);
|
||||
let written = std::fs::read(target.path().join("cli_probe_dual.bin"))
|
||||
.unwrap_or_else(|e| panic!("run {i} wrote no file: {e}"));
|
||||
assert_eq!(written, probe_bytes(), "run {i} wrote the wrong bytes");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user