From df298422b3c1951baeab55f073b51b2b8e9748da Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 14:51:37 +0200 Subject: [PATCH 1/6] Discover the daemon socket via XDG_RUNTIME_DIR Add socket path resolution helpers to `comenq-lib` so both binaries agree on where a user-hosted daemon lives: - `user_socket_path` derives `$XDG_RUNTIME_DIR/comenq/comenq.sock` when a user runtime directory is available. - `default_socket_path` prefers the user runtime path and falls back to the system-wide `/run/comenq/comenq.sock`. - `discover_socket_path` returns the first existing socket among the user and system paths so clients find whichever daemon is running. The daemon's default `socket_path` now uses the context-aware resolution, and the listener creates the socket's parent directory when absent so a user-hosted daemon works without systemd's `RuntimeDirectory=` support. The client's `--socket` flag becomes optional, resolves through discovery at connect time, and gains a `COMENQ_SOCKET` environment override. Resolution happens in `Args::socket_path` rather than clap's `default_value_os_t` because clap caches computed defaults in a process-wide static and would ignore environment changes between parses. Exclude `comenq_lib` from the `no_std_fs_operations` Whitaker lint: its new discovery tests create placeholder sockets in ambient tempdirs, the same sanctioned test-support category as the cucumber crate. Move the daemon config tests to `config/tests.rs` to respect the 400-line module cap. --- Cargo.lock | 3 + Cargo.toml | 3 +- crates/comenq/Cargo.toml | 2 + crates/comenq/src/client.rs | 9 +- crates/comenq/src/lib.rs | 99 ++++++++++++++++-- crates/comenqd/src/config.rs | 142 ++------------------------ crates/comenqd/src/config/tests.rs | 155 +++++++++++++++++++++++++++++ crates/comenqd/src/listener.rs | 17 ++++ dylint.toml | 4 + src/lib.rs | 141 ++++++++++++++++++++++++++ tests/features/cli.feature | 2 +- tests/features/config.feature | 9 +- tests/steps/cli_steps.rs | 13 ++- tests/steps/client_main_steps.rs | 2 +- tests/steps/config_steps.rs | 9 ++ 15 files changed, 457 insertions(+), 153 deletions(-) create mode 100644 crates/comenqd/src/config/tests.rs diff --git a/Cargo.lock b/Cargo.lock index b4b824b..250986a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -346,7 +346,9 @@ dependencies = [ "rstest", "serde", "serde_json", + "serial_test", "tempfile", + "test-support", "thiserror 1.0.69", "tokio", "tracing", @@ -367,6 +369,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "serial_test", "tempfile", "test-support", "tokio", diff --git a/Cargo.toml b/Cargo.toml index b915a19..84bece1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ clap = { workspace = true } comenq = { path = "crates/comenq" } comenqd = { path = "crates/comenqd", features = ["test-support"] } ortho_config = { git = "https://github.com/leynos/ortho-config.git", tag = "v0.4.0" } +serial_test = "2" tempfile = { workspace = true } # latest 3.x at time of writing; update as new patch versions release yaque = { workspace = true } wiremock = { workspace = true } @@ -41,7 +42,7 @@ resolver = "2" [workspace.dependencies] tokio = { version = "1.35", features = ["full"] } -clap = { version = "4.4", features = ["derive"] } +clap = { version = "4.4", features = ["derive", "env"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" octocrab = "0.38" diff --git a/crates/comenq/Cargo.toml b/crates/comenq/Cargo.toml index 5dd25f5..845f16b 100644 --- a/crates/comenq/Cargo.toml +++ b/crates/comenq/Cargo.toml @@ -21,4 +21,6 @@ tracing = { workspace = true } [dev-dependencies] rstest = { workspace = true } +serial_test = "2" tempfile = { workspace = true } +test-support = { workspace = true } diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index 0f5bcac..db5504e 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -42,13 +42,14 @@ pub enum ClientError { /// repo_slug: "owner/repo".parse().expect("slug"), /// pr_number: 1, /// comment_body: String::from("Hi"), -/// socket: PathBuf::from(DEFAULT_SOCKET_PATH), +/// socket: Some(PathBuf::from(DEFAULT_SOCKET_PATH)), /// }; /// run(args).await?; /// # Ok(()) /// # } /// ``` pub async fn run(args: Args) -> Result<(), ClientError> { + let socket = args.socket_path(); let request = CommentRequest { owner: args.repo_slug.owner().to_owned(), repo: args.repo_slug.repo().to_owned(), @@ -58,7 +59,7 @@ pub async fn run(args: Args) -> Result<(), ClientError> { let payload = serde_json::to_vec(&request)?; - let mut stream = UnixStream::connect(&args.socket) + let mut stream = UnixStream::connect(socket) .await .map_err(ClientError::Connect)?; stream @@ -98,7 +99,7 @@ mod tests { repo_slug: "octocat/hello-world".parse().expect("slug"), pr_number: 1, comment_body: "Hi".into(), - socket: socket.clone(), + socket: Some(socket.clone()), }; run(args).await.expect("run succeeds"); @@ -118,7 +119,7 @@ mod tests { repo_slug: "octocat/hello-world".parse().expect("slug"), pr_number: 1, comment_body: "Hi".into(), - socket: socket.clone(), + socket: Some(socket.clone()), }; let err = run(args).await.expect_err("should error"); diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 10b4e9d..588e3ad 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -104,12 +104,46 @@ pub struct Args { pub comment_body: String, /// Path to the daemon's Unix Domain Socket. - #[arg( - long, - value_hint = ValueHint::FilePath, - default_value_os_t = PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH), - )] - pub socket: PathBuf, + /// + /// When omitted, the client discovers the socket: the first existing + /// path among the per-user runtime path + /// (`$XDG_RUNTIME_DIR/comenq/comenq.sock`) and the system path is used, + /// so a user-hosted daemon is found automatically. May be overridden + /// with the `COMENQ_SOCKET` environment variable or this flag. + // The default is resolved in `socket_path` rather than through clap's + // `default_value_os_t`, which caches the computed value in a + // process-wide static and would ignore later environment changes. + #[arg(long, value_hint = ValueHint::FilePath, env = "COMENQ_SOCKET")] + pub socket: Option, +} + +impl Args { + /// Socket path to connect to, discovering a running daemon when the + /// user did not specify one. + /// + /// # Examples + /// + /// ```rust + /// use clap::Parser; + /// use comenq::Args; + /// + /// let args = Args::try_parse_from([ + /// "comenq", + /// "octocat/hello-world", + /// "1", + /// "Hi", + /// "--socket", + /// "/tmp/comenq.sock", + /// ]) + /// .expect("arguments parse"); + /// assert_eq!(args.socket_path(), std::path::PathBuf::from("/tmp/comenq.sock")); + /// ``` + #[must_use] + pub fn socket_path(&self) -> PathBuf { + self.socket + .clone() + .unwrap_or_else(comenq_lib::discover_socket_path) + } } #[cfg(test)] @@ -118,6 +152,7 @@ mod tests { use clap::Parser; use rstest::rstest; use std::path::PathBuf; + use test_support::EnvVarGuard; #[rstest] #[case("octocat/hello-world", 1, "Hi")] @@ -174,10 +209,58 @@ mod tests { assert_eq!(slug.repo(), "hello-world"); } + #[serial_test::serial] #[test] - fn socket_default_matches_constant() { + fn socket_defaults_to_system_path_without_runtime_dir() { + let _socket_guard = EnvVarGuard::remove("COMENQ_SOCKET"); + let _xdg_guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); let args = Args::try_parse_from(["comenq", "octocat/hello-world", "1", "Hi"]) .expect("valid arguments should parse"); - assert_eq!(args.socket, PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH)); + assert_eq!(args.socket, None); + assert_eq!( + args.socket_path(), + PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH) + ); + } + + #[serial_test::serial] + #[test] + fn socket_defaults_to_user_runtime_path() { + let dir = tempfile::tempdir().expect("create tempdir"); + let _socket_guard = EnvVarGuard::remove("COMENQ_SOCKET"); + let _xdg_guard = EnvVarGuard::set( + "XDG_RUNTIME_DIR", + dir.path().to_str().expect("tempdir path is UTF-8"), + ); + let args = Args::try_parse_from(["comenq", "octocat/hello-world", "1", "Hi"]) + .expect("valid arguments should parse"); + assert_eq!(args.socket, None); + assert_eq!(args.socket_path(), dir.path().join("comenq/comenq.sock")); + } + + #[serial_test::serial] + #[test] + fn socket_env_var_overrides_default() { + let _socket_guard = EnvVarGuard::set("COMENQ_SOCKET", "/tmp/custom.sock"); + let args = Args::try_parse_from(["comenq", "octocat/hello-world", "1", "Hi"]) + .expect("valid arguments should parse"); + assert_eq!(args.socket, Some(PathBuf::from("/tmp/custom.sock"))); + assert_eq!(args.socket_path(), PathBuf::from("/tmp/custom.sock")); + } + + #[serial_test::serial] + #[test] + fn socket_flag_overrides_env_var() { + let _socket_guard = EnvVarGuard::set("COMENQ_SOCKET", "/tmp/env.sock"); + let args = Args::try_parse_from([ + "comenq", + "octocat/hello-world", + "1", + "Hi", + "--socket", + "/tmp/flag.sock", + ]) + .expect("valid arguments should parse"); + assert_eq!(args.socket, Some(PathBuf::from("/tmp/flag.sock"))); } } diff --git a/crates/comenqd/src/config.rs b/crates/comenqd/src/config.rs index 0577af4..d35a949 100644 --- a/crates/comenqd/src/config.rs +++ b/crates/comenqd/src/config.rs @@ -4,7 +4,6 @@ //! overridden by environment variables using the `COMENQD_` prefix. use clap::Parser; -use comenq_lib::DEFAULT_SOCKET_PATH; use figment::providers::Env; use serde::{Deserialize, Serialize}; use std::io; @@ -30,6 +29,10 @@ pub struct Config { /// GitHub Personal Access Token. pub github_token: String, /// Path to the Unix Domain Socket. + /// + /// Defaults to `$XDG_RUNTIME_DIR/comenq/comenq.sock` when a user runtime + /// directory is available, falling back to the system-wide path + /// otherwise. See [`comenq_lib::default_socket_path`]. #[serde(default = "default_socket_path")] pub socket_path: PathBuf, /// Directory for the persistent queue. @@ -147,7 +150,7 @@ struct CliArgs { } fn default_socket_path() -> PathBuf { - PathBuf::from(DEFAULT_SOCKET_PATH) + comenq_lib::default_socket_path() } fn default_queue_path() -> PathBuf { @@ -246,137 +249,4 @@ impl Config { } #[cfg(test)] -mod tests { - use super::*; - use rstest::rstest; - use std::fs; - use tempfile::tempdir; - - use test_support::EnvVarGuard; - - #[rstest] - #[serial_test::serial] - fn loads_from_file() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write( - &path, - "github_token='abc'\nsocket_path='/tmp/s.sock'\nqueue_path='/tmp/q'", - ) - .expect("write config fixture"); - let _guard = EnvVarGuard::remove("COMENQD_SOCKET_PATH"); - let cfg = Config::from_file(&path).expect("load config"); - assert_eq!(cfg.github_token, "abc"); - assert_eq!(cfg.socket_path, PathBuf::from("/tmp/s.sock")); - assert_eq!(cfg.queue_path, PathBuf::from("/tmp/q")); - } - - #[rstest] - #[serial_test::serial] - fn error_when_missing_file() { - let path = PathBuf::from("/nonexistent/file.toml"); - let res = Config::from_file(&path); - assert!(res.is_err()); - } - - #[rstest] - #[serial_test::serial] - fn env_vars_override_file() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write(&path, "github_token='abc'\nsocket_path='/tmp/s.sock'") - .expect("write config fixture"); - let _guard = EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/override.sock"); - let cfg = Config::from_file(&path).expect("load config"); - assert_eq!(cfg.socket_path, PathBuf::from("/tmp/override.sock")); - } - - #[rstest] - #[serial_test::serial] - fn error_with_invalid_toml() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write(&path, "github_token='abc' this is not toml").expect("write invalid toml"); - let res = Config::from_file(&path); - assert!(res.is_err()); - } - - #[rstest] - #[serial_test::serial] - fn error_when_missing_token() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write(&path, "socket_path='/tmp/s.sock'").expect("write config without token"); - let res = Config::from_file(&path); - assert!(res.is_err()); - } - - #[rstest] - #[serial_test::serial] - fn defaults_are_applied() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write(&path, "github_token='abc'").expect("write config fixture"); - let cfg = Config::from_file(&path).expect("load config"); - assert_eq!( - cfg.socket_path, - PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH) - ); - assert_eq!(cfg.queue_path, PathBuf::from("/var/lib/comenq/queue")); - assert_eq!(cfg.cooldown_period_seconds, DEFAULT_COOLDOWN); - assert_eq!(cfg.restart_min_delay_ms, DEFAULT_RESTART_MIN_DELAY_MS); - assert_eq!(cfg.github_api_timeout_secs, DEFAULT_GITHUB_API_TIMEOUT_SECS); - assert_eq!(cfg.client_channel_capacity, DEFAULT_CLIENT_CHANNEL_CAPACITY); - } - - /// CLI arguments should take precedence over environment variables - /// and configuration file values when building the daemon `Config`. - #[rstest] - #[serial_test::serial] - fn cli_overrides_env_and_file() { - let dir = tempdir().expect("create tempdir"); - let path = dir.path().join("config.toml"); - fs::write(&path, "github_token='abc'\nsocket_path='/tmp/file.sock'") - .expect("write config fixture"); - let _guard = EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/env.sock"); - let cli = CliArgs { - config: path.clone(), - github_token: None, - socket_path: Some(PathBuf::from("/tmp/cli.sock")), - queue_path: None, - github_api_timeout_secs: None, - }; - let cfg = Config::from_file_with_cli(&path, &cli).expect("load config"); - assert_eq!(cfg.socket_path, PathBuf::from("/tmp/cli.sock")); - } - - #[cfg(feature = "test-support")] - #[rstest] - #[case(|cfg: &test_support::daemon::TestConfig| Config::from(cfg))] - #[case(|cfg: &test_support::daemon::TestConfig| Config::from(cfg.clone()))] - #[serial_test::serial] - fn converts_from_test_config(#[case] conv: fn(&test_support::daemon::TestConfig) -> Config) { - use test_support::temp_config; - - let tmp = tempdir().expect("create tempdir"); - let test_cfg = temp_config(&tmp).with_cooldown(42); - let cfg = conv(&test_cfg); - - assert_eq!(cfg.github_token, test_cfg.github_token); - assert_eq!(cfg.socket_path, test_cfg.socket_path); - assert_eq!(cfg.queue_path, test_cfg.queue_path); - assert_eq!( - cfg.cooldown_period_seconds, - test_cfg.cooldown_period_seconds - ); - assert_eq!(cfg.restart_min_delay_ms, test_cfg.restart_min_delay_ms); - assert_eq!( - cfg.github_api_timeout_secs, - test_cfg.github_api_timeout_secs - ); - assert_eq!( - cfg.client_channel_capacity, - test_cfg.client_channel_capacity - ); - } -} +mod tests; diff --git a/crates/comenqd/src/config/tests.rs b/crates/comenqd/src/config/tests.rs new file mode 100644 index 0000000..38279f2 --- /dev/null +++ b/crates/comenqd/src/config/tests.rs @@ -0,0 +1,155 @@ +//! Tests for daemon configuration loading, overrides, and defaults. + +use super::{ + CliArgs, Config, DEFAULT_CLIENT_CHANNEL_CAPACITY, DEFAULT_COOLDOWN, + DEFAULT_GITHUB_API_TIMEOUT_SECS, DEFAULT_RESTART_MIN_DELAY_MS, +}; +use rstest::rstest; +use std::fs; +use std::path::PathBuf; +use tempfile::tempdir; + +use test_support::EnvVarGuard; + +#[rstest] +#[serial_test::serial] +fn loads_from_file() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write( + &path, + "github_token='abc'\nsocket_path='/tmp/s.sock'\nqueue_path='/tmp/q'", + ) + .expect("write config fixture"); + let _guard = EnvVarGuard::remove("COMENQD_SOCKET_PATH"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!(cfg.github_token, "abc"); + assert_eq!(cfg.socket_path, PathBuf::from("/tmp/s.sock")); + assert_eq!(cfg.queue_path, PathBuf::from("/tmp/q")); +} + +#[rstest] +#[serial_test::serial] +fn error_when_missing_file() { + let path = PathBuf::from("/nonexistent/file.toml"); + let res = Config::from_file(&path); + assert!(res.is_err()); +} + +#[rstest] +#[serial_test::serial] +fn env_vars_override_file() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc'\nsocket_path='/tmp/s.sock'") + .expect("write config fixture"); + let _guard = EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/override.sock"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!(cfg.socket_path, PathBuf::from("/tmp/override.sock")); +} + +#[rstest] +#[serial_test::serial] +fn error_with_invalid_toml() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc' this is not toml").expect("write invalid toml"); + let res = Config::from_file(&path); + assert!(res.is_err()); +} + +#[rstest] +#[serial_test::serial] +fn error_when_missing_token() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "socket_path='/tmp/s.sock'").expect("write config without token"); + let res = Config::from_file(&path); + assert!(res.is_err()); +} + +#[rstest] +#[serial_test::serial] +fn defaults_are_applied() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc'").expect("write config fixture"); + let _socket_guard = EnvVarGuard::remove("COMENQD_SOCKET_PATH"); + let _xdg_guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!( + cfg.socket_path, + PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH) + ); + assert_eq!(cfg.queue_path, PathBuf::from("/var/lib/comenq/queue")); + assert_eq!(cfg.cooldown_period_seconds, DEFAULT_COOLDOWN); + assert_eq!(cfg.restart_min_delay_ms, DEFAULT_RESTART_MIN_DELAY_MS); + assert_eq!(cfg.github_api_timeout_secs, DEFAULT_GITHUB_API_TIMEOUT_SECS); + assert_eq!(cfg.client_channel_capacity, DEFAULT_CLIENT_CHANNEL_CAPACITY); +} + +#[rstest] +#[serial_test::serial] +fn default_socket_path_uses_runtime_dir() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc'").expect("write config fixture"); + let _socket_guard = EnvVarGuard::remove("COMENQD_SOCKET_PATH"); + let _xdg_guard = EnvVarGuard::set("XDG_RUNTIME_DIR", "/run/user/1000"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!( + cfg.socket_path, + PathBuf::from("/run/user/1000/comenq/comenq.sock") + ); +} + +/// CLI arguments should take precedence over environment variables +/// and configuration file values when building the daemon `Config`. +#[rstest] +#[serial_test::serial] +fn cli_overrides_env_and_file() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc'\nsocket_path='/tmp/file.sock'") + .expect("write config fixture"); + let _guard = EnvVarGuard::set("COMENQD_SOCKET_PATH", "/tmp/env.sock"); + let cli = CliArgs { + config: path.clone(), + github_token: None, + socket_path: Some(PathBuf::from("/tmp/cli.sock")), + queue_path: None, + github_api_timeout_secs: None, + }; + let cfg = Config::from_file_with_cli(&path, &cli).expect("load config"); + assert_eq!(cfg.socket_path, PathBuf::from("/tmp/cli.sock")); +} + +#[cfg(feature = "test-support")] +#[rstest] +#[case(|cfg: &test_support::daemon::TestConfig| Config::from(cfg))] +#[case(|cfg: &test_support::daemon::TestConfig| Config::from(cfg.clone()))] +#[serial_test::serial] +fn converts_from_test_config(#[case] conv: fn(&test_support::daemon::TestConfig) -> Config) { + use test_support::temp_config; + + let tmp = tempdir().expect("create tempdir"); + let test_cfg = temp_config(&tmp).with_cooldown(42); + let cfg = conv(&test_cfg); + + assert_eq!(cfg.github_token, test_cfg.github_token); + assert_eq!(cfg.socket_path, test_cfg.socket_path); + assert_eq!(cfg.queue_path, test_cfg.queue_path); + assert_eq!( + cfg.cooldown_period_seconds, + test_cfg.cooldown_period_seconds + ); + assert_eq!(cfg.restart_min_delay_ms, test_cfg.restart_min_delay_ms); + assert_eq!( + cfg.github_api_timeout_secs, + test_cfg.github_api_timeout_secs + ); + assert_eq!( + cfg.client_channel_capacity, + test_cfg.client_channel_capacity + ); +} diff --git a/crates/comenqd/src/listener.rs b/crates/comenqd/src/listener.rs index b6f298b..d74e850 100644 --- a/crates/comenqd/src/listener.rs +++ b/crates/comenqd/src/listener.rs @@ -36,6 +36,13 @@ use crate::supervisor::backoff; /// ``` pub fn prepare_listener(path: &Path) -> Result { let parent = path.parent().context("socket path missing parent")?; + // Create the socket directory when absent so a user-hosted daemon works + // without systemd's RuntimeDirectory= support. An empty parent means the + // path is relative to the working directory, which already exists. + if !parent.as_os_str().is_empty() { + stdfs::create_dir_all(parent) + .with_context(|| format!("creating socket directory {}", parent.display()))?; + } let file_name = path .file_name() .ok_or_else(|| anyhow::anyhow!("socket path missing file name"))?; @@ -169,6 +176,16 @@ mod tests { use std::thread; use tempfile::tempdir; + #[tokio::test] + async fn prepare_listener_creates_missing_parent_directory() { + let dir = tempdir().expect("create tempdir"); + let sock = dir.path().join("missing/nested/comenq.sock"); + let listener = prepare_listener(&sock).expect("prepare listener"); + let meta = std::fs::symlink_metadata(&sock).expect("metadata"); + assert!(meta.file_type().is_socket()); + drop(listener); + } + #[tokio::test] async fn prepare_listener_prevents_pre_bind_race() { let dir = tempdir().expect("create tempdir"); diff --git a/dylint.toml b/dylint.toml index 8bd2210..83df05c 100644 --- a/dylint.toml +++ b/dylint.toml @@ -13,9 +13,13 @@ # files and packaging manifests inside ambient tempdirs, which the Whitaker # user's guide lists as a sanctioned exclusion category (test support # utilities). +# - comenq_lib: socket discovery unit tests create placeholder sockets inside +# ambient tempdirs to exercise filesystem probing; the same sanctioned test +# support category as the cucumber crate. [no_std_fs_operations] excluded_crates = [ "build_script_build", "comenqd", + "comenq_lib", "cucumber", ] \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index aa08a4c..a7a3540 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,12 +4,79 @@ //! and daemon. use serde::{Deserialize, Serialize}; +use std::env; +use std::path::PathBuf; /// Default Unix Domain Socket path for the Comenq daemon. /// /// Shared by the daemon and CLI to avoid configuration drift. pub const DEFAULT_SOCKET_PATH: &str = "/run/comenq/comenq.sock"; +/// Environment variable naming the per-user runtime directory. +const XDG_RUNTIME_DIR: &str = "XDG_RUNTIME_DIR"; + +/// Socket location relative to a runtime directory. +const SOCKET_RELATIVE_PATH: &str = "comenq/comenq.sock"; + +/// Socket path within the per-user runtime directory, when one is available. +/// +/// Returns `None` when `XDG_RUNTIME_DIR` is unset or empty, which is the +/// case for system services and non-session processes. +/// +/// # Examples +/// +/// ```rust,no_run +/// if let Some(path) = comenq_lib::user_socket_path() { +/// println!("user socket would live at {}", path.display()); +/// } +/// ``` +#[must_use] +pub fn user_socket_path() -> Option { + env::var_os(XDG_RUNTIME_DIR) + .filter(|dir| !dir.is_empty()) + .map(|dir| PathBuf::from(dir).join(SOCKET_RELATIVE_PATH)) +} + +/// Default socket path for the current execution context. +/// +/// Prefers the per-user runtime directory so a user-hosted daemon needs no +/// configuration, falling back to [`DEFAULT_SOCKET_PATH`] when no user +/// runtime directory is available (for example under a system service). +/// +/// # Examples +/// +/// ```rust,no_run +/// let path = comenq_lib::default_socket_path(); +/// println!("daemon will listen on {}", path.display()); +/// ``` +#[must_use] +pub fn default_socket_path() -> PathBuf { + user_socket_path().unwrap_or_else(|| PathBuf::from(DEFAULT_SOCKET_PATH)) +} + +/// Discover the socket a client should connect to. +/// +/// Returns the first existing socket among the per-user runtime path and the +/// system path, so a client reaches a user-hosted daemon when one is running +/// and otherwise falls back to a system-hosted daemon. When neither socket +/// exists the context-appropriate default is returned so connection errors +/// mention the most likely intended path. +/// +/// # Examples +/// +/// ```rust,no_run +/// let path = comenq_lib::discover_socket_path(); +/// println!("connecting to {}", path.display()); +/// ``` +#[must_use] +pub fn discover_socket_path() -> PathBuf { + [user_socket_path(), Some(PathBuf::from(DEFAULT_SOCKET_PATH))] + .into_iter() + .flatten() + .find(|candidate| candidate.exists()) + .unwrap_or_else(default_socket_path) +} + /// Request sent from the client to the daemon. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct CommentRequest { @@ -73,4 +140,78 @@ mod tests { let result: Result = serde_json::from_str(data); assert!(result.is_err()); } + + #[expect( + clippy::expect_used, + reason = "tests should fail loudly when fixture setup fails" + )] + mod socket_path { + + //! Unit tests for socket path resolution and discovery. + use crate::{ + DEFAULT_SOCKET_PATH, default_socket_path, discover_socket_path, user_socket_path, + }; + use std::path::PathBuf; + use test_support::EnvVarGuard; + + #[serial_test::serial] + #[test] + fn user_socket_path_requires_runtime_dir() { + let _guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); + assert_eq!(user_socket_path(), None); + } + + #[serial_test::serial] + #[test] + fn user_socket_path_ignores_empty_runtime_dir() { + let _guard = EnvVarGuard::set("XDG_RUNTIME_DIR", ""); + assert_eq!(user_socket_path(), None); + } + + #[serial_test::serial] + #[test] + fn default_socket_path_prefers_runtime_dir() { + let _guard = EnvVarGuard::set("XDG_RUNTIME_DIR", "/run/user/1000"); + assert_eq!( + default_socket_path(), + PathBuf::from("/run/user/1000/comenq/comenq.sock") + ); + } + + #[serial_test::serial] + #[test] + fn default_socket_path_falls_back_to_system_path() { + let _guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); + assert_eq!(default_socket_path(), PathBuf::from(DEFAULT_SOCKET_PATH)); + } + + #[serial_test::serial] + #[test] + fn discover_socket_path_prefers_existing_user_socket() { + let dir = tempfile::tempdir().expect("create tempdir"); + let socket_dir = dir.path().join("comenq"); + std::fs::create_dir_all(&socket_dir).expect("create socket dir"); + let socket = socket_dir.join("comenq.sock"); + std::fs::write(&socket, b"").expect("create placeholder socket"); + let _guard = EnvVarGuard::set( + "XDG_RUNTIME_DIR", + dir.path().to_str().expect("tempdir path is UTF-8"), + ); + assert_eq!(discover_socket_path(), socket); + } + + #[serial_test::serial] + #[test] + fn discover_socket_path_defaults_when_nothing_exists() { + let dir = tempfile::tempdir().expect("create tempdir"); + let _guard = EnvVarGuard::set( + "XDG_RUNTIME_DIR", + dir.path().to_str().expect("tempdir path is UTF-8"), + ); + assert_eq!( + discover_socket_path(), + dir.path().join("comenq/comenq.sock") + ); + } + } } diff --git a/tests/features/cli.feature b/tests/features/cli.feature index 57b1e1b..58678eb 100644 --- a/tests/features/cli.feature +++ b/tests/features/cli.feature @@ -4,7 +4,7 @@ Feature: CLI argument parsing Given valid CLI arguments When they are parsed Then parsing succeeds - And the socket path is "/run/comenq/comenq.sock" + And no socket path override is present Scenario: overriding the socket path Given valid CLI arguments diff --git a/tests/features/config.feature b/tests/features/config.feature index af05f7a..ff5c3bf 100644 --- a/tests/features/config.feature +++ b/tests/features/config.feature @@ -27,11 +27,18 @@ Feature: Daemon configuration When the config is loaded Then config loading fails - Scenario: uses default socket path + Scenario: uses default socket path without a user runtime directory Given a configuration file with token "abc" and no socket_path + And environment variable "XDG_RUNTIME_DIR" is unset When the config is loaded Then socket path is "/run/comenq/comenq.sock" + Scenario: derives default socket path from the user runtime directory + Given a configuration file with token "abc" and no socket_path + And environment variable "XDG_RUNTIME_DIR" is "/run/user/1000" + When the config is loaded + Then socket path is "/run/user/1000/comenq/comenq.sock" + Scenario: uses default cooldown period Given a configuration file with token "abc" and no cooldown_period_seconds When the config is loaded diff --git a/tests/steps/cli_steps.rs b/tests/steps/cli_steps.rs index 3f778e9..076693e 100644 --- a/tests/steps/cli_steps.rs +++ b/tests/steps/cli_steps.rs @@ -84,5 +84,16 @@ fn the_socket_path_is(world: &mut CliWorld, expected: String) { Some(Ok(a)) => a, other => panic!("expected parsed args, got {other:?}"), }; - assert_eq!(args.socket, PathBuf::from(expected)); + assert_eq!(args.socket, Some(PathBuf::from(expected))); +} + +#[then("no socket path override is present")] +fn no_socket_override(world: &mut CliWorld) { + let args = match world.result.take() { + Some(Ok(a)) => a, + other => panic!("expected parsed args, got {other:?}"), + }; + // Discovery of the actual path is covered by unit tests; the parser + // must simply leave the override unset. + assert_eq!(args.socket, None); } diff --git a/tests/steps/client_main_steps.rs b/tests/steps/client_main_steps.rs index 61032e9..0f34032 100644 --- a/tests/steps/client_main_steps.rs +++ b/tests/steps/client_main_steps.rs @@ -22,7 +22,7 @@ fn base_args(socket: std::path::PathBuf) -> anyhow::Result { repo_slug: "octocat/hello-world".parse().context("slug")?, pr_number: 1, comment_body: "Hi".into(), - socket, + socket: Some(socket), }) } diff --git a/tests/steps/config_steps.rs b/tests/steps/config_steps.rs index 4632fe6..ad4cada 100644 --- a/tests/steps/config_steps.rs +++ b/tests/steps/config_steps.rs @@ -87,6 +87,15 @@ fn set_env_var(world: &mut ConfigWorld, key: String, value: String) { world.env_guard = Some(EnvVarGuard::set(&key, &value)); } +#[expect( + clippy::needless_pass_by_value, + reason = "cucumber requires owned values" +)] +#[given(regex = r#"^environment variable \"(.+)\" is unset$"#)] +fn unset_env_var(world: &mut ConfigWorld, key: String) { + world.env_guard = Some(EnvVarGuard::remove(&key)); +} + #[when("the config is loaded")] fn load_config(world: &mut ConfigWorld) -> anyhow::Result<()> { let path = world.path.as_ref().context("path set")?; From 0098998b404b1ca47f2e0eedd9c77d0d467fe909 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 14:53:51 +0200 Subject: [PATCH 2/6] Support token files and a cooldown CLI override Add an optional `github_token_file` key to the daemon configuration. When set, the file is read at startup and its trimmed contents override `github_token`; an empty or unreadable file fails loudly. A leading `${VAR}` placeholder in the path is expanded from the environment, so a systemd unit can pass `LoadCredential=token:%h/pandalump-token` and the configuration can reference `github_token_file = "${CREDENTIALS_DIRECTORY}/token"` without hard-coding the credentials directory. `github_token` is no longer required by deserialization; instead a validation step demands that one of the two sources yields a non-empty token, preserving the previous failure mode for configs with no token at all. Also add `--github-token-file` and `--cooldown-period-seconds` to the daemon's command-line overrides, so the posting interval can be set without editing the configuration file. Both keys remain settable through `COMENQD_*` environment variables as before. --- crates/comenqd/src/config.rs | 83 +++++++++++++++++++++ crates/comenqd/src/config/tests.rs | 112 +++++++++++++++++++++++++++++ tests/features/config.feature | 10 +++ tests/steps/config_steps.rs | 28 ++++++++ 4 files changed, 233 insertions(+) diff --git a/crates/comenqd/src/config.rs b/crates/comenqd/src/config.rs index d35a949..e616940 100644 --- a/crates/comenqd/src/config.rs +++ b/crates/comenqd/src/config.rs @@ -27,7 +27,20 @@ const DEFAULT_CLIENT_CHANNEL_CAPACITY: usize = 1024; #[derive(Debug, Deserialize, Serialize, PartialEq, Eq, Clone)] pub struct Config { /// GitHub Personal Access Token. + /// + /// May be supplied directly or via [`Config::github_token_file`]; one of + /// the two must be provided. + #[serde(default)] pub github_token: String, + /// Optional path to a file containing the GitHub Personal Access Token. + /// + /// When set, the file is read at startup and its trimmed contents + /// override [`Config::github_token`]. A leading `${VAR}` placeholder is + /// expanded from the environment, so systemd credentials work with + /// `LoadCredential=token:...` and + /// `github_token_file = "${CREDENTIALS_DIRECTORY}/token"`. + #[serde(default)] + pub github_token_file: Option, /// Path to the Unix Domain Socket. /// /// Defaults to `$XDG_RUNTIME_DIR/comenq/comenq.sock` when a user runtime @@ -81,6 +94,7 @@ impl From for Config { } = value; Self { github_token, + github_token_file: None, socket_path, queue_path, cooldown_period_seconds, @@ -119,6 +133,7 @@ impl From<&test_support::daemon::TestConfig> for Config { fn from(value: &test_support::daemon::TestConfig) -> Self { Self { github_token: value.github_token.clone(), + github_token_file: None, socket_path: value.socket_path.clone(), queue_path: value.queue_path.clone(), cooldown_period_seconds: value.cooldown_period_seconds, @@ -138,12 +153,18 @@ struct CliArgs { /// GitHub Personal Access Token. #[arg(long)] github_token: Option, + /// Path to a file containing the GitHub Personal Access Token. + #[arg(long, value_name = "FILE")] + github_token_file: Option, /// Override the Unix Domain Socket path. #[arg(long)] socket_path: Option, /// Override the queue directory. #[arg(long)] queue_path: Option, + /// Override the cooldown between comment posts (seconds). + #[arg(long)] + cooldown_period_seconds: Option, /// Override GitHub API timeout (seconds). #[arg(long)] github_api_timeout_secs: Option, @@ -235,17 +256,79 @@ impl Config { if let Some(token) = &cli.github_token { cfg.github_token = token.clone(); } + if let Some(file) = &cli.github_token_file { + cfg.github_token_file = Some(file.clone()); + } if let Some(socket) = &cli.socket_path { cfg.socket_path = socket.clone(); } if let Some(queue) = &cli.queue_path { cfg.queue_path = queue.clone(); } + if let Some(secs) = cli.cooldown_period_seconds { + cfg.cooldown_period_seconds = secs; + } if let Some(secs) = cli.github_api_timeout_secs { cfg.github_api_timeout_secs = secs; } + cfg.resolve_github_token()?; Ok(cfg) } + + /// Resolve the effective GitHub token, reading `github_token_file` when + /// set and validating that a non-empty token is available. + #[expect(clippy::result_large_err, reason = "propagate ortho_config errors")] + fn resolve_github_token(&mut self) -> Result<(), ortho_config::OrthoError> { + if let Some(file) = &self.github_token_file { + let path = expand_env_prefix(file)?; + let token = + std::fs::read_to_string(&path).map_err(|e| ortho_config::OrthoError::File { + path: path.clone(), + source: Box::new(e), + })?; + let token = token.trim(); + if token.is_empty() { + return Err(ortho_config::OrthoError::Validation { + key: "github_token_file".into(), + message: format!("token file {} is empty", path.display()), + }); + } + self.github_token = token.to_owned(); + } + if self.github_token.is_empty() { + return Err(ortho_config::OrthoError::Validation { + key: "github_token".into(), + message: "provide github_token or github_token_file".into(), + }); + } + Ok(()) + } +} + +/// Expand a leading `${VAR}` placeholder in `path` from the environment. +/// +/// Only a placeholder at the very start of the path is expanded; paths +/// without one are returned unchanged. An unset variable or a malformed +/// placeholder is an error so misconfiguration fails loudly at startup. +#[expect(clippy::result_large_err, reason = "propagate ortho_config errors")] +fn expand_env_prefix(path: &Path) -> Result { + let Some(text) = path.to_str() else { + return Ok(path.to_path_buf()); + }; + let Some(rest) = text.strip_prefix("${") else { + return Ok(path.to_path_buf()); + }; + let Some((var, tail)) = rest.split_once('}') else { + return Err(ortho_config::OrthoError::Validation { + key: "github_token_file".into(), + message: format!("unterminated environment placeholder in '{text}'"), + }); + }; + let value = std::env::var(var).map_err(|_| ortho_config::OrthoError::Validation { + key: "github_token_file".into(), + message: format!("environment variable '{var}' is not set"), + })?; + Ok(PathBuf::from(format!("{value}{tail}"))) } #[cfg(test)] diff --git a/crates/comenqd/src/config/tests.rs b/crates/comenqd/src/config/tests.rs index 38279f2..301a0c4 100644 --- a/crates/comenqd/src/config/tests.rs +++ b/crates/comenqd/src/config/tests.rs @@ -116,14 +116,126 @@ fn cli_overrides_env_and_file() { let cli = CliArgs { config: path.clone(), github_token: None, + github_token_file: None, socket_path: Some(PathBuf::from("/tmp/cli.sock")), queue_path: None, + cooldown_period_seconds: None, github_api_timeout_secs: None, }; let cfg = Config::from_file_with_cli(&path, &cli).expect("load config"); assert_eq!(cfg.socket_path, PathBuf::from("/tmp/cli.sock")); } +#[rstest] +#[serial_test::serial] +fn cli_overrides_cooldown() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token='abc'\ncooldown_period_seconds=10") + .expect("write config fixture"); + let cli = CliArgs { + config: path.clone(), + github_token: None, + github_token_file: None, + socket_path: None, + queue_path: None, + cooldown_period_seconds: Some(30), + github_api_timeout_secs: None, + }; + let cfg = Config::from_file_with_cli(&path, &cli).expect("load config"); + assert_eq!(cfg.cooldown_period_seconds, 30); +} + +#[rstest] +#[serial_test::serial] +fn token_file_overrides_inline_token() { + let dir = tempdir().expect("create tempdir"); + let token_path = dir.path().join("token"); + fs::write(&token_path, "s3cret\n").expect("write token file"); + let path = dir.path().join("config.toml"); + fs::write( + &path, + format!( + "github_token='inline'\ngithub_token_file='{}'", + token_path.display() + ), + ) + .expect("write config fixture"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!(cfg.github_token, "s3cret"); +} + +#[rstest] +#[serial_test::serial] +fn token_file_alone_suffices() { + let dir = tempdir().expect("create tempdir"); + let token_path = dir.path().join("token"); + fs::write(&token_path, "s3cret").expect("write token file"); + let path = dir.path().join("config.toml"); + fs::write( + &path, + format!("github_token_file='{}'", token_path.display()), + ) + .expect("write config fixture"); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!(cfg.github_token, "s3cret"); +} + +#[rstest] +#[serial_test::serial] +fn missing_token_file_errors() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token_file='/nonexistent/token'").expect("write config fixture"); + assert!(Config::from_file(&path).is_err()); +} + +#[rstest] +#[serial_test::serial] +fn empty_token_file_errors() { + let dir = tempdir().expect("create tempdir"); + let token_path = dir.path().join("token"); + fs::write(&token_path, "\n").expect("write empty token file"); + let path = dir.path().join("config.toml"); + fs::write( + &path, + format!("github_token_file='{}'", token_path.display()), + ) + .expect("write config fixture"); + assert!(Config::from_file(&path).is_err()); +} + +#[rstest] +#[serial_test::serial] +fn token_file_expands_credentials_directory() { + let dir = tempdir().expect("create tempdir"); + let token_path = dir.path().join("token"); + fs::write(&token_path, "cred-token").expect("write token file"); + let path = dir.path().join("config.toml"); + fs::write(&path, "github_token_file='${CREDENTIALS_DIRECTORY}/token'") + .expect("write config fixture"); + let _guard = EnvVarGuard::set( + "CREDENTIALS_DIRECTORY", + dir.path().to_str().expect("tempdir path is UTF-8"), + ); + let cfg = Config::from_file(&path).expect("load config"); + assert_eq!(cfg.github_token, "cred-token"); +} + +#[rstest] +#[serial_test::serial] +fn token_file_with_unset_placeholder_errors() { + let dir = tempdir().expect("create tempdir"); + let path = dir.path().join("config.toml"); + fs::write( + &path, + "github_token_file='${COMENQD_TEST_UNSET_VARIABLE}/token'", + ) + .expect("write config fixture"); + let _guard = EnvVarGuard::remove("COMENQD_TEST_UNSET_VARIABLE"); + assert!(Config::from_file(&path).is_err()); +} + #[cfg(feature = "test-support")] #[rstest] #[case(|cfg: &test_support::daemon::TestConfig| Config::from(cfg))] diff --git a/tests/features/config.feature b/tests/features/config.feature index ff5c3bf..62c9612 100644 --- a/tests/features/config.feature +++ b/tests/features/config.feature @@ -39,6 +39,16 @@ Feature: Daemon configuration When the config is loaded Then socket path is "/run/user/1000/comenq/comenq.sock" + Scenario: reading the token from a file + Given a configuration file referencing a token file containing "s3cret" + When the config is loaded + Then github token is "s3cret" + + Scenario: token file missing on disk + Given a configuration file referencing a missing token file + When the config is loaded + Then config loading fails + Scenario: uses default cooldown period Given a configuration file with token "abc" and no cooldown_period_seconds When the config is loaded diff --git a/tests/steps/config_steps.rs b/tests/steps/config_steps.rs index ad4cada..71adb04 100644 --- a/tests/steps/config_steps.rs +++ b/tests/steps/config_steps.rs @@ -73,6 +73,34 @@ fn config_without_cooldown(world: &mut ConfigWorld, token: String) -> anyhow::Re config_file_with_token(world, token) } +#[given(regex = r#"^a configuration file referencing a token file containing \"(.+)\"$"#)] +#[expect( + clippy::needless_pass_by_value, + reason = "cucumber requires owned values" +)] +fn config_with_token_file(world: &mut ConfigWorld, token: String) -> anyhow::Result<()> { + let dir = TempDir::new().context("create temp dir")?; + let token_path = dir.path().join("token"); + fs::write(&token_path, format!("{token}\n")).context("write token file")?; + let path = dir.path().join("config.toml"); + fs::write( + &path, + format!("github_token_file='{}'", token_path.display()), + ) + .context("write config")?; + world.dir = Some(dir); + world.path = Some(path); + Ok(()) +} + +#[given("a configuration file referencing a missing token file")] +fn config_with_missing_token_file(world: &mut ConfigWorld) -> anyhow::Result<()> { + let (dir, path) = write_temp_config("github_token_file='/nonexistent/token'")?; + world.dir = Some(dir); + world.path = Some(path); + Ok(()) +} + #[given("a missing configuration file")] fn missing_configuration_file(world: &mut ConfigWorld) { world.path = Some(PathBuf::from("/nonexistent/nowhere.toml")); From 1f24ca4e9afad8e5728155d2ca348823edfa5028 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 14:56:11 +0200 Subject: [PATCH 3/6] Add optional random flutter to the posting cooldown Introduce a `cooldown_flutter_seconds` configuration key (default 0, disabled). After each processed request the worker now waits the full configured cooldown plus a fresh uniformly random duration between zero and the flutter ceiling. Flutter only ever lengthens the wait, so the guaranteed minimum spacing between posts is preserved while the cadence stops being perfectly regular. Use the `rand` crate for sampling and `saturating_add` so extreme configurations cannot overflow. Move the worker unit tests to `worker/tests.rs` to stay within the 400-line module cap. --- Cargo.lock | 38 +++++++- Cargo.toml | 1 + crates/comenqd/Cargo.toml | 1 + crates/comenqd/src/config.rs | 15 +++ crates/comenqd/src/config/tests.rs | 1 + crates/comenqd/src/worker.rs | 132 +++++---------------------- crates/comenqd/src/worker/tests.rs | 141 +++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 114 deletions(-) create mode 100644 crates/comenqd/src/worker/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 250986a..7de6c35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -389,6 +389,7 @@ dependencies = [ "figment", "octocrab", "ortho_config", + "rand 0.9.5", "rstest", "serde", "serde_json", @@ -1896,8 +1897,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" dependencies = [ "libc", - "rand_chacha", - "rand_core", + "rand_chacha 0.3.1", + "rand_core 0.6.4", +] + +[[package]] +name = "rand" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +dependencies = [ + "rand_chacha 0.9.0", + "rand_core 0.9.5", ] [[package]] @@ -1907,7 +1918,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" dependencies = [ "ppv-lite86", - "rand_core", + "rand_core 0.6.4", +] + +[[package]] +name = "rand_chacha" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" +dependencies = [ + "ppv-lite86", + "rand_core 0.9.5", ] [[package]] @@ -1919,6 +1940,15 @@ dependencies = [ "getrandom 0.2.16", ] +[[package]] +name = "rand_core" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76afc826de14238e6e8c374ddcc1fa19e374fd8dd986b0d2af0d02377261d83c" +dependencies = [ + "getrandom 0.3.3", +] + [[package]] name = "redox_syscall" version = "0.5.15" @@ -3529,7 +3559,7 @@ dependencies = [ "lazy_static", "log", "notify", - "rand", + "rand 0.8.5", "semver", "sysinfo", ] diff --git a/Cargo.toml b/Cargo.toml index 84bece1..485ef55 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,7 @@ rstest = "0.18.0" wiremock = "0.6" test-support = { path = "test-support" } backon = "1.5.2" +rand = "0.9" uuid = { version = "1", features = ["v4"] } [lints.clippy] diff --git a/crates/comenqd/Cargo.toml b/crates/comenqd/Cargo.toml index 5138ed5..b35a194 100644 --- a/crates/comenqd/Cargo.toml +++ b/crates/comenqd/Cargo.toml @@ -27,6 +27,7 @@ ortho_config = { workspace = true } figment = { version = "0.10", default-features = false, features = ["env", "toml"] } test-support = { workspace = true, optional = true } backon = { workspace = true } +rand = { workspace = true } uuid = { workspace = true, features = ["v4"] } [dev-dependencies] diff --git a/crates/comenqd/src/config.rs b/crates/comenqd/src/config.rs index e616940..4315c09 100644 --- a/crates/comenqd/src/config.rs +++ b/crates/comenqd/src/config.rs @@ -16,6 +16,8 @@ const DEFAULT_QUEUE_PATH: &str = "/var/lib/comenq/queue"; /// The period was increased from 15 to 16 minutes to provide a larger /// buffer against GitHub's secondary rate limits. const DEFAULT_COOLDOWN: u64 = 960; +/// Default random flutter added to the cooldown, in seconds (disabled). +const DEFAULT_COOLDOWN_FLUTTER: u64 = 0; /// Default minimum delay between task restarts in milliseconds. const DEFAULT_RESTART_MIN_DELAY_MS: u64 = 100; /// Default timeout in seconds for GitHub API calls. @@ -54,6 +56,13 @@ pub struct Config { /// Cooldown between comment posts in seconds. #[serde(default = "default_cooldown")] pub cooldown_period_seconds: u64, + /// Maximum random flutter added to each cooldown, in seconds. + /// + /// After every post the worker waits the full cooldown plus a fresh + /// uniformly random duration between zero and this value. Flutter only + /// ever lengthens the wait; zero (the default) disables it. + #[serde(default = "default_cooldown_flutter")] + pub cooldown_flutter_seconds: u64, /// Minimum delay in milliseconds applied between task restarts. #[serde(default = "default_restart_min_delay_ms")] pub restart_min_delay_ms: u64, @@ -98,6 +107,7 @@ impl From for Config { socket_path, queue_path, cooldown_period_seconds, + cooldown_flutter_seconds: DEFAULT_COOLDOWN_FLUTTER, restart_min_delay_ms, github_api_timeout_secs, client_channel_capacity, @@ -137,6 +147,7 @@ impl From<&test_support::daemon::TestConfig> for Config { socket_path: value.socket_path.clone(), queue_path: value.queue_path.clone(), cooldown_period_seconds: value.cooldown_period_seconds, + cooldown_flutter_seconds: DEFAULT_COOLDOWN_FLUTTER, restart_min_delay_ms: value.restart_min_delay_ms, github_api_timeout_secs: value.github_api_timeout_secs, client_channel_capacity: value.client_channel_capacity, @@ -182,6 +193,10 @@ fn default_cooldown() -> u64 { DEFAULT_COOLDOWN } +fn default_cooldown_flutter() -> u64 { + DEFAULT_COOLDOWN_FLUTTER +} + fn default_restart_min_delay_ms() -> u64 { DEFAULT_RESTART_MIN_DELAY_MS } diff --git a/crates/comenqd/src/config/tests.rs b/crates/comenqd/src/config/tests.rs index 301a0c4..74a76c5 100644 --- a/crates/comenqd/src/config/tests.rs +++ b/crates/comenqd/src/config/tests.rs @@ -83,6 +83,7 @@ fn defaults_are_applied() { ); assert_eq!(cfg.queue_path, PathBuf::from("/var/lib/comenq/queue")); assert_eq!(cfg.cooldown_period_seconds, DEFAULT_COOLDOWN); + assert_eq!(cfg.cooldown_flutter_seconds, 0); assert_eq!(cfg.restart_min_delay_ms, DEFAULT_RESTART_MIN_DELAY_MS); assert_eq!(cfg.github_api_timeout_secs, DEFAULT_GITHUB_API_TIMEOUT_SECS); assert_eq!(cfg.client_channel_capacity, DEFAULT_CLIENT_CHANNEL_CAPACITY); diff --git a/crates/comenqd/src/worker.rs b/crates/comenqd/src/worker.rs index 6a5fd9e..a38ba9a 100644 --- a/crates/comenqd/src/worker.rs +++ b/crates/comenqd/src/worker.rs @@ -1,12 +1,15 @@ //! Queue worker for comenqd. //! //! Dequeues requests from the persistent queue and posts comments to GitHub -//! while enforcing a fixed cooldown between attempts. +//! while enforcing a cooldown between attempts. The cooldown always runs in +//! full; an optional random flutter lengthens each wait to avoid a perfectly +//! regular posting cadence. use crate::config::Config; use anyhow::Result; use comenq_lib::CommentRequest; use octocrab::Octocrab; +use rand::Rng; use std::sync::Arc; use std::time::Duration; use thiserror::Error; @@ -31,6 +34,21 @@ enum PostCommentError { Timeout, } +/// Seconds to wait after processing a request: the configured cooldown plus +/// a fresh random flutter of up to `cooldown_flutter_seconds`. +/// +/// Flutter only ever lengthens the wait — the full cooldown always elapses — +/// so the posting cadence stays below GitHub's secondary rate limits while +/// avoiding a perfectly regular interval. +fn cooldown_with_flutter(config: &Config) -> u64 { + let flutter = config.cooldown_flutter_seconds; + if flutter == 0 { + return config.cooldown_period_seconds; + } + let jitter = rand::rng().random_range(0..=flutter); + config.cooldown_period_seconds.saturating_add(jitter) +} + /// Constructs an authenticated Octocrab GitHub client using a personal access token. #[expect(clippy::result_large_err, reason = "propagate Octocrab errors")] pub(crate) fn build_octocrab(token: &str) -> octocrab::Result { @@ -201,7 +219,7 @@ pub async fn run_worker( if let Err(check_err) = hooks.notify_drained_if_empty(&config.queue_path) { tracing::warn!(error = %check_err, "Queue emptiness check failed after drop"); } - if WorkerHooks::wait_or_shutdown(config.cooldown_period_seconds, shutdown).await { + if WorkerHooks::wait_or_shutdown(cooldown_with_flutter(&config), shutdown).await { break; } continue; @@ -234,7 +252,7 @@ pub async fn run_worker( hooks.notify_idle(); #[cfg(any(test, feature = "test-support"))] hooks.notify_drained_if_empty(&config.queue_path)?; - if WorkerHooks::wait_or_shutdown(config.cooldown_period_seconds, shutdown).await { + if WorkerHooks::wait_or_shutdown(cooldown_with_flutter(&config), shutdown).await { break; } } @@ -244,110 +262,4 @@ pub async fn run_worker( } #[cfg(test)] -mod tests { - use super::*; - use std::time::Instant; - use tokio::sync::watch; - - #[tokio::test] - async fn wait_or_shutdown_returns_false_on_timeout() { - let (_tx, mut rx) = watch::channel(()); - let start = Instant::now(); - let result = WorkerHooks::wait_or_shutdown(0, &mut rx).await; - assert!(!result, "should return false when timeout expires"); - assert!( - start.elapsed().as_millis() < 500, - "zero-second wait should return immediately" - ); - } - - #[tokio::test] - async fn wait_or_shutdown_returns_true_on_shutdown() { - let (tx, mut rx) = watch::channel(()); - // Signal shutdown before waiting - tx.send(()).expect("send shutdown signal"); - let result = WorkerHooks::wait_or_shutdown(60, &mut rx).await; - assert!(result, "should return true when shutdown is signalled"); - } - - #[tokio::test] - async fn wait_or_shutdown_prioritises_shutdown_over_timeout() { - let (tx, mut rx) = watch::channel(()); - // Send shutdown signal - tx.send(()).expect("send shutdown signal"); - // Even with zero timeout, shutdown should be detected due to biased select - let result = WorkerHooks::wait_or_shutdown(0, &mut rx).await; - assert!(result, "biased select should prioritize shutdown signal"); - } - - /// Tests that notify_one wakes exactly one waiter when multiple tasks are waiting. - /// - /// This validates the single-waiter semantics documented on WorkerHooks. - #[tokio::test] - async fn notify_one_wakes_exactly_one_waiter() { - use std::sync::atomic::{AtomicUsize, Ordering}; - use std::time::Duration; - - let notify = Arc::new(Notify::new()); - let wake_count = Arc::new(AtomicUsize::new(0)); - - // Spawn three waiters - let mut handles = Vec::new(); - for _ in 0..3 { - let n = notify.clone(); - let count = wake_count.clone(); - handles.push(tokio::spawn(async move { - // Wait with a timeout to avoid hanging the test - if tokio::time::timeout(Duration::from_millis(100), n.notified()) - .await - .is_ok() - { - count.fetch_add(1, Ordering::SeqCst); - } - })); - } - - // Give waiters time to register - tokio::time::sleep(Duration::from_millis(10)).await; - - // Send exactly one notification - notify.notify_one(); - - // Wait for all tasks to complete (they'll timeout after 100ms) - for h in handles { - let _ = h.await; - } - - // Only one waiter should have been woken - assert_eq!( - wake_count.load(Ordering::SeqCst), - 1, - "notify_one should wake exactly one waiter" - ); - } - - /// Tests that notify_one buffers a permit when no waiters exist. - /// - /// This validates that the notification is not lost if sent before waiting. - #[tokio::test] - async fn notify_one_buffers_permit_when_no_waiters() { - let notify = Arc::new(Notify::new()); - - // Send notification before anyone is waiting - notify.notify_one(); - - // The first waiter should receive the buffered permit immediately - let result = tokio::time::timeout(Duration::from_millis(50), notify.notified()).await; - assert!( - result.is_ok(), - "buffered permit should wake first waiter immediately" - ); - - // Second waiter should NOT receive a permit (it was consumed) - let result = tokio::time::timeout(Duration::from_millis(50), notify.notified()).await; - assert!( - result.is_err(), - "second waiter should timeout with no remaining permit" - ); - } -} +mod tests; diff --git a/crates/comenqd/src/worker/tests.rs b/crates/comenqd/src/worker/tests.rs new file mode 100644 index 0000000..8298929 --- /dev/null +++ b/crates/comenqd/src/worker/tests.rs @@ -0,0 +1,141 @@ +//! Tests for the queue worker's cooldown, flutter, and notification hooks. + +use super::{Config, Notify, WorkerHooks, cooldown_with_flutter}; +use std::sync::Arc; +use std::time::Duration; +use std::time::Instant; +use tokio::sync::watch; + +/// Build a minimal config with the given cooldown and flutter. +fn config_with_flutter(cooldown: u64, flutter: u64) -> Config { + let dir = tempfile::tempdir().unwrap_or_else(|e| panic!("create tempdir: {e}")); + let mut cfg = Config::from(test_support::temp_config(&dir).with_cooldown(cooldown)); + cfg.cooldown_flutter_seconds = flutter; + cfg +} + +#[test] +fn zero_flutter_leaves_cooldown_unchanged() { + let cfg = config_with_flutter(960, 0); + assert_eq!(cooldown_with_flutter(&cfg), 960); +} + +#[test] +fn flutter_only_lengthens_the_cooldown() { + let cfg = config_with_flutter(60, 240); + for _ in 0..200 { + let wait = cooldown_with_flutter(&cfg); + assert!( + (60..=300).contains(&wait), + "wait {wait} outside [cooldown, cooldown + flutter]" + ); + } +} + +#[test] +fn flutter_saturates_instead_of_overflowing() { + let cfg = config_with_flutter(u64::MAX, 1); + assert_eq!(cooldown_with_flutter(&cfg), u64::MAX); +} + +#[tokio::test] +async fn wait_or_shutdown_returns_false_on_timeout() { + let (_tx, mut rx) = watch::channel(()); + let start = Instant::now(); + let result = WorkerHooks::wait_or_shutdown(0, &mut rx).await; + assert!(!result, "should return false when timeout expires"); + assert!( + start.elapsed().as_millis() < 500, + "zero-second wait should return immediately" + ); +} + +#[tokio::test] +async fn wait_or_shutdown_returns_true_on_shutdown() { + let (tx, mut rx) = watch::channel(()); + // Signal shutdown before waiting + tx.send(()).expect("send shutdown signal"); + let result = WorkerHooks::wait_or_shutdown(60, &mut rx).await; + assert!(result, "should return true when shutdown is signalled"); +} + +#[tokio::test] +async fn wait_or_shutdown_prioritises_shutdown_over_timeout() { + let (tx, mut rx) = watch::channel(()); + // Send shutdown signal + tx.send(()).expect("send shutdown signal"); + // Even with zero timeout, shutdown should be detected due to biased select + let result = WorkerHooks::wait_or_shutdown(0, &mut rx).await; + assert!(result, "biased select should prioritize shutdown signal"); +} + +/// Tests that notify_one wakes exactly one waiter when multiple tasks are waiting. +/// +/// This validates the single-waiter semantics documented on WorkerHooks. +#[tokio::test] +async fn notify_one_wakes_exactly_one_waiter() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::time::Duration; + + let notify = Arc::new(Notify::new()); + let wake_count = Arc::new(AtomicUsize::new(0)); + + // Spawn three waiters + let mut handles = Vec::new(); + for _ in 0..3 { + let n = notify.clone(); + let count = wake_count.clone(); + handles.push(tokio::spawn(async move { + // Wait with a timeout to avoid hanging the test + if tokio::time::timeout(Duration::from_millis(100), n.notified()) + .await + .is_ok() + { + count.fetch_add(1, Ordering::SeqCst); + } + })); + } + + // Give waiters time to register + tokio::time::sleep(Duration::from_millis(10)).await; + + // Send exactly one notification + notify.notify_one(); + + // Wait for all tasks to complete (they'll timeout after 100ms) + for h in handles { + let _ = h.await; + } + + // Only one waiter should have been woken + assert_eq!( + wake_count.load(Ordering::SeqCst), + 1, + "notify_one should wake exactly one waiter" + ); +} + +/// Tests that notify_one buffers a permit when no waiters exist. +/// +/// This validates that the notification is not lost if sent before waiting. +#[tokio::test] +async fn notify_one_buffers_permit_when_no_waiters() { + let notify = Arc::new(Notify::new()); + + // Send notification before anyone is waiting + notify.notify_one(); + + // The first waiter should receive the buffered permit immediately + let result = tokio::time::timeout(Duration::from_millis(50), notify.notified()).await; + assert!( + result.is_ok(), + "buffered permit should wake first waiter immediately" + ); + + // Second waiter should NOT receive a permit (it was consumed) + let result = tokio::time::timeout(Duration::from_millis(50), notify.notified()).await; + assert!( + result.is_err(), + "second waiter should timeout with no remaining permit" + ); +} From 562b091cc954c4ec10639b5de30066f9b0bda047 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 14:58:48 +0200 Subject: [PATCH 4/6] Package a per-user systemd unit and example config Ship `packaging/linux/comenqd-user.service` for hosting the daemon under `systemd --user`: - `%h`-based paths for the binary and configuration file - `RuntimeDirectory=comenq` for the socket under `$XDG_RUNTIME_DIR` - `StateDirectory=comenq` plus `COMENQD_QUEUE_PATH` for a queue in `~/.local/state/comenq/queue` - `LoadCredential=token:%h/pandalump-token` feeding the PAT to the daemon via `github_token_file = "${CREDENTIALS_DIRECTORY}/token"` - `WantedBy=default.target` Add the matching example configuration `packaging/config/comenqd-user.toml`, document the new `github_token_file` and `cooldown_flutter_seconds` keys in the system example config, and update the man pages, README, and design document for socket discovery, token files, flutter, and user-mode deployment. Cover the user unit with a packaging scenario. --- README.md | 21 +++++++++++++++ docs/comenq-design.md | 40 ++++++++++++++++++++++------ packaging/config/comenqd-user.toml | 28 +++++++++++++++++++ packaging/config/comenqd.toml | 12 ++++++++- packaging/linux/comenqd-user.service | 35 ++++++++++++++++++++++++ packaging/man/comenq.1 | 11 +++++--- packaging/man/comenqd.1 | 32 ++++++++++++++++++++-- tests/features/packaging.feature | 4 +++ tests/steps/packaging_steps.rs | 18 +++++++++++++ 9 files changed, 187 insertions(+), 14 deletions(-) create mode 100644 packaging/config/comenqd-user.toml create mode 100644 packaging/linux/comenqd-user.service diff --git a/README.md b/README.md index 0292166..23cac91 100644 --- a/README.md +++ b/README.md @@ -32,3 +32,24 @@ make build ``` Queued requests persist on disk and are posted sequentially by the daemon. + +## Running as a user service + +The daemon also runs unprivileged under `systemd --user`. Install the binaries +to `~/.local/bin`, copy +[`packaging/linux/comenqd-user.service`](packaging/linux/comenqd-user.service) +to `~/.config/systemd/user/comenqd.service` and +[`packaging/config/comenqd-user.toml`](packaging/config/comenqd-user.toml) to +`~/.config/comenqd/config.toml`, then enable it: + +```bash +systemctl --user daemon-reload +systemctl --user enable --now comenqd.service +``` + +The socket defaults to `$XDG_RUNTIME_DIR/comenq/comenq.sock` and the client +discovers whichever daemon (user or system) is running. The GitHub token is +supplied through systemd's credential system +(`LoadCredential=token:%h/pandalump-token` with +`github_token_file = "${CREDENTIALS_DIRECTORY}/token"`), keeping the secret out +of the unit file and process environment. diff --git a/docs/comenq-design.md b/docs/comenq-design.md index 59acb65..e7e8762 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -588,14 +588,16 @@ For operational flexibility and security, the daemon's behaviour must be controlled via a configuration file, not hard-coded values. A TOML file located at `/etc/comenqd/config.toml` is the conventional choice. -| Parameter | Type | Description | Default Value | -| ----------------------- | ------- | ------------------------------------------------------------------------------------------ | ----------------------- | -| github_token | String | The GitHub Personal Access Token (PAT) used for authentication. This is a required field. | (none) | -| socket_path | PathBuf | The filesystem path for the Unix Domain Socket. | /run/comenq/comenq.sock | -| queue_path | PathBuf | The directory path for the persistent yaque queue data. | /var/lib/comenq/queue | -| log_level | String | The minimum log level to record (e.g., "info", "debug", "trace"). | info | -| cooldown_period_seconds | u64 | The cooling-off period in seconds after each comment post. | 960 | -| restart_min_delay_ms | u64 | The minimum delay (milliseconds) applied between supervised task restarts (backoff floor). | 100 | +| Parameter | Type | Description | Default Value | +| ------------------------ | ------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- | +| github_token | String | The GitHub Personal Access Token (PAT) used for authentication. Required unless `github_token_file` is set. | (none) | +| github_token_file | PathBuf | Optional path to a file containing the PAT. Read at startup; its trimmed contents override `github_token`. A leading `${VAR}` placeholder is expanded from the environment, enabling systemd `LoadCredential` integration. | (none) | +| socket_path | PathBuf | The filesystem path for the Unix Domain Socket. | `$XDG_RUNTIME_DIR/comenq/comenq.sock` when a user runtime directory is available, else /run/comenq/comenq.sock | +| queue_path | PathBuf | The directory path for the persistent yaque queue data. | /var/lib/comenq/queue | +| log_level | String | The minimum log level to record (e.g., "info", "debug", "trace"). | info | +| cooldown_period_seconds | u64 | The cooling-off period in seconds after each comment post. | 960 | +| cooldown_flutter_seconds | u64 | Maximum random flutter in seconds added to each cooldown. The full cooldown always elapses; a fresh random duration up to this value is added on top. Zero disables flutter. | 0 | +| restart_min_delay_ms | u64 | The minimum delay (milliseconds) applied between supervised task restarts (backoff floor). | 100 | Configuration is loaded using the `ortho_config` crate. The daemon calls `Config::load()` which merges values from `/etc/comenqd/config.toml`, @@ -759,6 +761,28 @@ sudo systemctl start comenq.service sudo systemctl status comenq.service ``` +#### 4.2.1. Running as a per-user service + +The daemon also runs unprivileged under `systemd --user`, using the +`packaging/linux/comenqd-user.service` unit and the matching example +configuration `packaging/config/comenqd-user.toml`: + +- The socket defaults to `$XDG_RUNTIME_DIR/comenq/comenq.sock`; + `RuntimeDirectory=comenq` provisions the directory, and the client discovers + whichever socket (user or system) exists. +- The queue lives in `~/.local/state/comenq/queue`, provided through + `StateDirectory=comenq` and the `COMENQD_QUEUE_PATH` environment variable in + the unit. +- The PAT is supplied through the systemd credential system: + `LoadCredential=token:%h/pandalump-token` exposes the token file to the + service, and the configuration references it as + `github_token_file = "${CREDENTIALS_DIRECTORY}/token"`, keeping the secret + out of the unit, the environment, and `ps` output. + +Install the unit as `~/.config/systemd/user/comenqd.service`, the configuration +as `~/.config/comenqd/config.toml`, then enable it with +`systemctl --user enable --now comenqd.service`. + ### 4.3. Security Posture and Best Practices Security is a primary consideration in the design and deployment of this diff --git a/packaging/config/comenqd-user.toml b/packaging/config/comenqd-user.toml new file mode 100644 index 0000000..e55a279 --- /dev/null +++ b/packaging/config/comenqd-user.toml @@ -0,0 +1,28 @@ +# Example configuration for a user-hosted comenqd (systemd --user). +# Install as ~/.config/comenqd/config.toml and pair with the +# packaging/linux/comenqd-user.service unit. + +# Read the GitHub Personal Access Token from the systemd credential +# provided by the unit's LoadCredential=token:... directive. The leading +# ${CREDENTIALS_DIRECTORY} placeholder is expanded from the environment at +# startup. Alternatively set an absolute path to any readable token file, +# or provide the token inline via github_token. +github_token_file = "${CREDENTIALS_DIRECTORY}/token" + +# Path to the Unix domain socket for client connections. Defaults to +# $XDG_RUNTIME_DIR/comenq/comenq.sock for user sessions, which the client +# discovers automatically. +# socket_path = "/run/user/1000/comenq/comenq.sock" + +# Location for the persistent yaque queue data. The packaged user unit +# sets COMENQD_QUEUE_PATH to ~/.local/state/comenq/queue, which overrides +# this file; uncomment to pin a different location. +# queue_path = "/home/user/.local/state/comenq/queue" + +# Cooldown period between posting comments in seconds. +# cooldown_period_seconds = 960 + +# Maximum random flutter added to each cooldown in seconds. The full +# cooldown always elapses; a fresh random duration up to this value is +# added on top. Zero disables flutter. +# cooldown_flutter_seconds = 0 diff --git a/packaging/config/comenqd.toml b/packaging/config/comenqd.toml index eb0826d..e57af6a 100644 --- a/packaging/config/comenqd.toml +++ b/packaging/config/comenqd.toml @@ -1,9 +1,14 @@ # Default configuration for comenqd # GitHub Personal Access Token used for authentication. -# If left empty, GitHub integration is disabled. +# Provide either github_token or github_token_file. # github_token = "" +# Path to a file containing the token. Read at startup; overrides +# github_token. A leading ${VAR} placeholder is expanded from the +# environment, enabling systemd LoadCredential integration: +# github_token_file = "${CREDENTIALS_DIRECTORY}/token" + # Minimum log level to output # log_level = "info" @@ -15,3 +20,8 @@ # Cooldown period between posting comments in seconds # cooldown_period_seconds = 960 + +# Maximum random flutter added to each cooldown in seconds. The full +# cooldown always elapses; a fresh random duration up to this value is +# added on top. Zero (the default) disables flutter. +# cooldown_flutter_seconds = 0 diff --git a/packaging/linux/comenqd-user.service b/packaging/linux/comenqd-user.service new file mode 100644 index 0000000..418035e --- /dev/null +++ b/packaging/linux/comenqd-user.service @@ -0,0 +1,35 @@ +# Comenq daemon as a per-user service (systemd --user). +# +# Install to ~/.config/systemd/user/comenqd.service alongside a +# configuration file at ~/.config/comenqd/config.toml (see +# packaging/config/comenqd-user.toml for an example), then run: +# +# systemctl --user daemon-reload +# systemctl --user enable --now comenqd.service +# +# The GitHub token is supplied through the systemd credential system: +# LoadCredential exposes ~/pandalump-token to the daemon as +# ${CREDENTIALS_DIRECTORY}/token, which the example configuration +# references via github_token_file. Adjust the source path to taste. + +[Unit] +Description=Comenq Daemon (user service) +Documentation=https://github.com/leynos/comenq +After=network.target + +[Service] +ExecStart=%h/.local/bin/comenqd --config %h/.config/comenqd/config.toml +# Socket lives in $XDG_RUNTIME_DIR/comenq; RuntimeDirectory creates and +# cleans it with the service. +RuntimeDirectory=comenq +# Persistent queue under ~/.local/state/comenq/queue (%S is the user state +# directory for user units). +StateDirectory=comenq +Environment=COMENQD_QUEUE_PATH=%S/comenq/queue +LoadCredential=token:%h/pandalump-token +NoNewPrivileges=true +Restart=on-failure +RestartSec=5s + +[Install] +WantedBy=default.target diff --git a/packaging/man/comenq.1 b/packaging/man/comenq.1 index 1e63947..4de2d51 100644 --- a/packaging/man/comenq.1 +++ b/packaging/man/comenq.1 @@ -21,10 +21,15 @@ format, the pull request number, and the comment body to submit. .PP Use .B --socket -to override the Unix Domain Socket path used to contact the daemon. The -socket defaults to +(or the +.B COMENQ_SOCKET +environment variable) to override the Unix Domain Socket path used to +contact the daemon. By default the client connects to the first existing +socket among +.I $XDG_RUNTIME_DIR/comenq/comenq.sock +(a user-hosted daemon) and .I /run/comenq/comenq.sock -which matches the packaged systemd service configuration. +(the packaged system service). .SH EXAMPLES Send a comment to pull request 42 of example/repo: .PP diff --git a/packaging/man/comenqd.1 b/packaging/man/comenqd.1 index 3d3193c..5332d9c 100644 --- a/packaging/man/comenqd.1 +++ b/packaging/man/comenqd.1 @@ -12,6 +12,10 @@ comenqd \- daemon that posts queued GitHub pull request comments .I token ] [ +.B --github-token-file +.I file +] +[ .B --socket-path .I path ] @@ -19,6 +23,10 @@ comenqd \- daemon that posts queued GitHub pull request comments .B --queue-path .I path ] +[ +.B --cooldown-period-seconds +.I seconds +] .SH DESCRIPTION The .B comenqd @@ -30,8 +38,28 @@ and may be overridden through command line options or environment variables prefixed with .BR COMENQD_. .PP -The daemon is designed to run as a system service using the packaged -systemd unit. When launched manually it logs its effective socket and +The GitHub token may be given inline via +.B github_token +or read from a file named by +.BR github_token_file . +The file is read at startup and its trimmed contents take precedence. A +leading \fB${VAR}\fR placeholder in the path is expanded from the +environment, so a systemd unit using +.B LoadCredential=token:... +can reference \fB${CREDENTIALS_DIRECTORY}/token\fR. +.PP +The cooldown between comment posts is set by +.B cooldown_period_seconds +and may be lengthened by a random flutter of up to +.B cooldown_flutter_seconds +after each post; the full cooldown always elapses. +.PP +The daemon may run as a system service using the packaged systemd unit, +or as a per-user service (\fBsystemd --user\fR) using the +.I comenqd-user.service +example. For user sessions the socket defaults to +.IR $XDG_RUNTIME_DIR/comenq/comenq.sock . +When launched manually it logs its effective socket and queue locations to the terminal. .SH FILES .TP diff --git a/tests/features/packaging.feature b/tests/features/packaging.feature index 25aca1e..36a39f9 100644 --- a/tests/features/packaging.feature +++ b/tests/features/packaging.feature @@ -8,3 +8,7 @@ Feature: Packaging configuration Scenario: service unit hardening Given the systemd unit file Then it includes hardening directives + + Scenario: user service unit + Given the user systemd unit file + Then it targets the user session diff --git a/tests/steps/packaging_steps.rs b/tests/steps/packaging_steps.rs index ffce21c..4104e43 100644 --- a/tests/steps/packaging_steps.rs +++ b/tests/steps/packaging_steps.rs @@ -47,3 +47,21 @@ fn includes_hardening(world: &mut PackagingWorld) -> anyhow::Result<()> { assert!(text.contains("NoNewPrivileges=true")); Ok(()) } + +#[given("the user systemd unit file")] +fn the_user_systemd_file(world: &mut PackagingWorld) -> anyhow::Result<()> { + let text = + fs::read_to_string("packaging/linux/comenqd-user.service").context("read user service")?; + world.content = Some(text); + Ok(()) +} + +#[then("it targets the user session")] +fn targets_user_session(world: &mut PackagingWorld) -> anyhow::Result<()> { + let text = world.content.take().context("service loaded")?; + assert!(text.contains("WantedBy=default.target")); + assert!(text.contains("RuntimeDirectory=comenq")); + assert!(text.contains("LoadCredential=token:")); + assert!(text.contains("%h/.config/comenqd/config.toml")); + Ok(()) +} From 10389af8c0114b5d873d2a2dbe8b3326bbfbd06f Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 15:01:55 +0200 Subject: [PATCH 5/6] Open one yaque handle per queue side in the supervisor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The daemon opened a full `yaque::channel()` in three places: at startup for the writer's sender, in the worker spawn for its receiver, and again on writer respawn. Each `channel()` call acquires both of yaque's per-side lock files, so the worker could never start while the writer held the sender — it failed with "sender side already in use" and sat in a permanent restart loop, and a writer respawn would likewise have contended with the worker's receiver. Open exactly the required side instead: `Sender::open` for the writer (startup and respawn) and `Receiver::open` for the worker. Add a regression test that spawns the worker while a sender is held and asserts it starts and shuts down cleanly. --- crates/comenqd/src/supervisor.rs | 16 +++++++++---- crates/comenqd/src/supervisor/tests.rs | 33 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/crates/comenqd/src/supervisor.rs b/crates/comenqd/src/supervisor.rs index bd18e4f..e2ca4b0 100644 --- a/crates/comenqd/src/supervisor.rs +++ b/crates/comenqd/src/supervisor.rs @@ -14,7 +14,7 @@ use tokio::fs; #[cfg(unix)] use tokio::signal::unix::{SignalKind, signal}; use tokio::sync::{mpsc, watch}; -use yaque::{Sender, channel}; +use yaque::{Receiver, Sender}; use crate::listener::run_listener; use crate::worker::{WorkerControl, WorkerHooks, build_octocrab, run_worker}; @@ -140,8 +140,10 @@ async fn supervise_writer( break; } backoff = backoff_builder(); - match channel(&cfg.queue_path) { - Ok((queue_tx, _)) => { + // Open only the sender: the worker holds the queue's receiver, + // and yaque permits one live handle per side. + match Sender::open(&cfg.queue_path) { + Ok(queue_tx) => { handle = tokio::spawn(queue_writer(queue_tx, rx)); } Err(e) => { @@ -198,7 +200,10 @@ pub async fn run(config: Config) -> Result<()> { ensure_queue_dir(&config.queue_path).await?; tracing::info!(queue = %config.queue_path.display(), "Queue directory prepared"); let octocrab = Arc::new(build_octocrab(&config.github_token)?); - let (queue_tx, _) = channel(&config.queue_path)?; // drop unused receiver + // Open only the sender here; the worker opens the matching receiver. + // Opening a full channel() in both places would contend for yaque's + // per-side lock files and leave the worker in a permanent restart loop. + let queue_tx = Sender::open(&config.queue_path)?; let (client_tx_initial, client_rx) = mpsc::channel(config.client_channel_capacity); let client_tx = Arc::new(tokio::sync::Mutex::new(client_tx_initial)); let cfg = Arc::new(config); @@ -305,7 +310,8 @@ fn spawn_worker( ) -> tokio::task::JoinHandle> { let cfg_clone = cfg.clone(); tokio::spawn(async move { - let (_tx, rx) = channel(&cfg_clone.queue_path)?; + // Open only the receiver; the queue writer owns the sender side. + let rx = Receiver::open(&cfg_clone.queue_path)?; let control = WorkerControl::new(shutdown, WorkerHooks::default()); run_worker(cfg_clone, rx, octocrab, control).await }) diff --git a/crates/comenqd/src/supervisor/tests.rs b/crates/comenqd/src/supervisor/tests.rs index d3611ab..c0c92b1 100644 --- a/crates/comenqd/src/supervisor/tests.rs +++ b/crates/comenqd/src/supervisor/tests.rs @@ -78,3 +78,36 @@ fn logs_failures( } } } + +/// The worker must start while the queue writer holds the sender. +/// +/// Regression test for the daemon's startup topology: the writer owns the +/// queue's `yaque::Sender` and the worker must open only the `Receiver`. +/// Opening a full `channel()` on both sides contends for yaque's per-side +/// lock files and left the worker in a permanent restart loop. +#[rstest] +#[tokio::test] +async fn worker_starts_while_writer_holds_the_sender() { + let dir = tempfile::tempdir().expect("create tempdir"); + let cfg: std::sync::Arc = + std::sync::Arc::new(test_support::temp_config(&dir).into()); + super::ensure_queue_dir(&cfg.queue_path) + .await + .expect("create queue dir"); + let _sender = yaque::Sender::open(&cfg.queue_path).expect("open queue sender"); + + let octocrab = + std::sync::Arc::new(crate::worker::build_octocrab("token").expect("build octocrab")); + let (shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(()); + let handle = super::spawn_worker(cfg, octocrab, shutdown_rx); + shutdown_tx.send(()).expect("signal shutdown"); + + let res = tokio::time::timeout(std::time::Duration::from_secs(5), handle) + .await + .expect("worker should exit promptly") + .expect("worker task should not panic"); + assert!( + res.is_ok(), + "worker must open the queue receiver while the sender is held: {res:?}" + ); +} From 0603c3ccef69f2a68276c2a3dc1649973a4c8cb5 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 18:26:59 +0200 Subject: [PATCH 6/6] Probe socket candidates by connecting instead of existence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A daemon that exits outside the packaged unit leaves its socket file behind, and the existence-based discovery always selected that stale user socket, so `UnixStream::connect` failed without ever trying the healthy system daemon — contradicting the advertised fallback. Replace `discover_socket_path` with `socket_candidates`, which returns the user runtime path (when available) followed by the system path, and have the client connect to each candidate in order, using the first socket that accepts. A stale file fails to connect (typically `ECONNREFUSED`) and is skipped with a warning; the last connection error is surfaced when every candidate fails. An explicit `--socket` or `COMENQ_SOCKET` still yields exactly that path with no fallback. Add regression tests binding and dropping a listener so the socket file persists: the client must fall past it to a live socket, and report the connection error when all candidates fail. --- crates/comenq/src/client.rs | 63 ++++++++++++++++++++++++++++++-- crates/comenq/src/lib.rs | 51 +++++++++++++++++--------- packaging/man/comenq.1 | 9 +++-- src/lib.rs | 73 ++++++++++++++++++++++--------------- 4 files changed, 142 insertions(+), 54 deletions(-) diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index db5504e..9774432 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -6,6 +6,7 @@ //! testable. use comenq_lib::CommentRequest; +use std::path::PathBuf; use thiserror::Error; use tokio::{io::AsyncWriteExt, net::UnixStream}; use tracing::warn; @@ -29,6 +30,29 @@ pub enum ClientError { Shutdown(#[source] std::io::Error), } +/// Connect to the first candidate socket that accepts a connection. +/// +/// A daemon that exits without unlinking its socket leaves a stale file +/// behind; connecting to it fails (typically `ECONNREFUSED`), and the next +/// candidate is tried, so a stale user socket never shadows a healthy +/// system daemon. The last connection error is returned when every +/// candidate fails. +async fn connect_first(candidates: &[PathBuf]) -> Result { + let mut last_error: Option = None; + for candidate in candidates { + match UnixStream::connect(candidate).await { + Ok(stream) => return Ok(stream), + Err(e) => { + warn!(socket = %candidate.display(), error = %e, "socket candidate refused"); + last_error = Some(e); + } + } + } + Err(ClientError::Connect(last_error.unwrap_or_else(|| { + std::io::Error::new(std::io::ErrorKind::NotFound, "no socket candidates to try") + }))) +} + /// Send a `CommentRequest` to the daemon. /// /// # Examples @@ -49,7 +73,7 @@ pub enum ClientError { /// # } /// ``` pub async fn run(args: Args) -> Result<(), ClientError> { - let socket = args.socket_path(); + let candidates = args.socket_candidates(); let request = CommentRequest { owner: args.repo_slug.owner().to_owned(), repo: args.repo_slug.repo().to_owned(), @@ -59,9 +83,7 @@ pub async fn run(args: Args) -> Result<(), ClientError> { let payload = serde_json::to_vec(&request)?; - let mut stream = UnixStream::connect(socket) - .await - .map_err(ClientError::Connect)?; + let mut stream = connect_first(&candidates).await?; stream .write_all(&payload) .await @@ -110,6 +132,39 @@ mod tests { assert_eq!(req.body, "Hi"); } + /// A stale socket file (bound once, listener dropped, file left behind) + /// must not shadow a live daemon later in the candidate list. + #[tokio::test] + async fn connect_first_skips_stale_sockets() { + let dir = tempdir().expect("temp dir"); + let stale = dir.path().join("stale.sock"); + drop(UnixListener::bind(&stale).expect("bind stale socket")); + assert!(stale.exists(), "stale socket file should remain on disk"); + + let live = dir.path().join("live.sock"); + let listener = UnixListener::bind(&live).expect("bind live socket"); + + let stream = super::connect_first(&[stale.clone(), live.clone()]) + .await + .expect("should fall back to the live socket"); + drop(stream); + drop(listener); + } + + /// Every candidate failing yields the connection error. + #[tokio::test] + async fn connect_first_reports_failure_when_all_candidates_fail() { + let dir = tempdir().expect("temp dir"); + let stale = dir.path().join("stale.sock"); + drop(UnixListener::bind(&stale).expect("bind stale socket")); + let missing = dir.path().join("missing.sock"); + + let err = super::connect_first(&[stale, missing]) + .await + .expect_err("all candidates should fail"); + assert!(matches!(err, ClientError::Connect(_))); + } + #[tokio::test] async fn run_errors_when_socket_missing() { let dir = tempdir().expect("temp dir"); diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 588e3ad..3877e52 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -105,21 +105,26 @@ pub struct Args { /// Path to the daemon's Unix Domain Socket. /// - /// When omitted, the client discovers the socket: the first existing - /// path among the per-user runtime path - /// (`$XDG_RUNTIME_DIR/comenq/comenq.sock`) and the system path is used, - /// so a user-hosted daemon is found automatically. May be overridden - /// with the `COMENQ_SOCKET` environment variable or this flag. - // The default is resolved in `socket_path` rather than through clap's - // `default_value_os_t`, which caches the computed value in a + /// When omitted, the client tries the per-user runtime path + /// (`$XDG_RUNTIME_DIR/comenq/comenq.sock`) and then the system path, + /// connecting to the first socket that accepts, so a user-hosted daemon + /// is found automatically and a stale socket file never shadows a + /// healthy daemon. May be overridden with the `COMENQ_SOCKET` + /// environment variable or this flag. + // The candidates are resolved at connect time rather than through + // clap's `default_value_os_t`, which caches the computed value in a // process-wide static and would ignore later environment changes. #[arg(long, value_hint = ValueHint::FilePath, env = "COMENQ_SOCKET")] pub socket: Option, } impl Args { - /// Socket path to connect to, discovering a running daemon when the - /// user did not specify one. + /// Socket paths to try in order, honouring an explicit override. + /// + /// An explicit `--socket` (or `COMENQ_SOCKET`) yields exactly that + /// path; otherwise the discovery candidates from + /// [`comenq_lib::socket_candidates`] are returned. Callers connect to + /// each in turn so a stale socket file cannot shadow a live daemon. /// /// # Examples /// @@ -136,13 +141,16 @@ impl Args { /// "/tmp/comenq.sock", /// ]) /// .expect("arguments parse"); - /// assert_eq!(args.socket_path(), std::path::PathBuf::from("/tmp/comenq.sock")); + /// assert_eq!( + /// args.socket_candidates(), + /// vec![std::path::PathBuf::from("/tmp/comenq.sock")] + /// ); /// ``` #[must_use] - pub fn socket_path(&self) -> PathBuf { + pub fn socket_candidates(&self) -> Vec { self.socket .clone() - .unwrap_or_else(comenq_lib::discover_socket_path) + .map_or_else(comenq_lib::socket_candidates, |explicit| vec![explicit]) } } @@ -218,14 +226,14 @@ mod tests { .expect("valid arguments should parse"); assert_eq!(args.socket, None); assert_eq!( - args.socket_path(), - PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH) + args.socket_candidates(), + vec![PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH)] ); } #[serial_test::serial] #[test] - fn socket_defaults_to_user_runtime_path() { + fn socket_candidates_prefer_the_user_runtime_path() { let dir = tempfile::tempdir().expect("create tempdir"); let _socket_guard = EnvVarGuard::remove("COMENQ_SOCKET"); let _xdg_guard = EnvVarGuard::set( @@ -235,7 +243,13 @@ mod tests { let args = Args::try_parse_from(["comenq", "octocat/hello-world", "1", "Hi"]) .expect("valid arguments should parse"); assert_eq!(args.socket, None); - assert_eq!(args.socket_path(), dir.path().join("comenq/comenq.sock")); + assert_eq!( + args.socket_candidates(), + vec![ + dir.path().join("comenq/comenq.sock"), + PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH), + ] + ); } #[serial_test::serial] @@ -245,7 +259,10 @@ mod tests { let args = Args::try_parse_from(["comenq", "octocat/hello-world", "1", "Hi"]) .expect("valid arguments should parse"); assert_eq!(args.socket, Some(PathBuf::from("/tmp/custom.sock"))); - assert_eq!(args.socket_path(), PathBuf::from("/tmp/custom.sock")); + assert_eq!( + args.socket_candidates(), + vec![PathBuf::from("/tmp/custom.sock")] + ); } #[serial_test::serial] diff --git a/packaging/man/comenq.1 b/packaging/man/comenq.1 index 4de2d51..1f43db0 100644 --- a/packaging/man/comenq.1 +++ b/packaging/man/comenq.1 @@ -24,12 +24,13 @@ Use (or the .B COMENQ_SOCKET environment variable) to override the Unix Domain Socket path used to -contact the daemon. By default the client connects to the first existing -socket among +contact the daemon. By default the client tries .I $XDG_RUNTIME_DIR/comenq/comenq.sock -(a user-hosted daemon) and +(a user-hosted daemon) and then .I /run/comenq/comenq.sock -(the packaged system service). +(the packaged system service), connecting to the first socket that +accepts. A stale socket file left behind by a daemon that exited without +unlinking it is skipped rather than shadowing a healthy daemon. .SH EXAMPLES Send a comment to pull request 42 of example/repo: .PP diff --git a/src/lib.rs b/src/lib.rs index a7a3540..e355a08 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -54,27 +54,30 @@ pub fn default_socket_path() -> PathBuf { user_socket_path().unwrap_or_else(|| PathBuf::from(DEFAULT_SOCKET_PATH)) } -/// Discover the socket a client should connect to. +/// Candidate sockets a client should try, in preference order. /// -/// Returns the first existing socket among the per-user runtime path and the -/// system path, so a client reaches a user-hosted daemon when one is running -/// and otherwise falls back to a system-hosted daemon. When neither socket -/// exists the context-appropriate default is returned so connection errors -/// mention the most likely intended path. +/// Returns the per-user runtime path (when a user runtime directory is +/// available) followed by the system path. Callers must probe candidates by +/// actually connecting, in order, rather than checking for file existence: a +/// daemon that exits without unlinking its socket leaves a stale file +/// behind, and an existence check would select it even though nothing is +/// listening, shadowing a healthy daemon at the next candidate. /// /// # Examples /// /// ```rust,no_run -/// let path = comenq_lib::discover_socket_path(); -/// println!("connecting to {}", path.display()); +/// for path in comenq_lib::socket_candidates() { +/// println!("would try {}", path.display()); +/// } /// ``` #[must_use] -pub fn discover_socket_path() -> PathBuf { - [user_socket_path(), Some(PathBuf::from(DEFAULT_SOCKET_PATH))] - .into_iter() - .flatten() - .find(|candidate| candidate.exists()) - .unwrap_or_else(default_socket_path) +pub fn socket_candidates() -> Vec { + let mut candidates: Vec = user_socket_path().into_iter().collect(); + let system = PathBuf::from(DEFAULT_SOCKET_PATH); + if !candidates.contains(&system) { + candidates.push(system); + } + candidates } /// Request sent from the client to the daemon. @@ -149,7 +152,7 @@ mod tests { //! Unit tests for socket path resolution and discovery. use crate::{ - DEFAULT_SOCKET_PATH, default_socket_path, discover_socket_path, user_socket_path, + DEFAULT_SOCKET_PATH, default_socket_path, socket_candidates, user_socket_path, }; use std::path::PathBuf; use test_support::EnvVarGuard; @@ -187,31 +190,43 @@ mod tests { #[serial_test::serial] #[test] - fn discover_socket_path_prefers_existing_user_socket() { + fn socket_candidates_prefer_the_user_socket() { let dir = tempfile::tempdir().expect("create tempdir"); - let socket_dir = dir.path().join("comenq"); - std::fs::create_dir_all(&socket_dir).expect("create socket dir"); - let socket = socket_dir.join("comenq.sock"); - std::fs::write(&socket, b"").expect("create placeholder socket"); let _guard = EnvVarGuard::set( "XDG_RUNTIME_DIR", dir.path().to_str().expect("tempdir path is UTF-8"), ); - assert_eq!(discover_socket_path(), socket); + assert_eq!( + socket_candidates(), + vec![ + dir.path().join("comenq/comenq.sock"), + PathBuf::from(DEFAULT_SOCKET_PATH), + ] + ); } #[serial_test::serial] #[test] - fn discover_socket_path_defaults_when_nothing_exists() { - let dir = tempfile::tempdir().expect("create tempdir"); - let _guard = EnvVarGuard::set( - "XDG_RUNTIME_DIR", - dir.path().to_str().expect("tempdir path is UTF-8"), - ); + fn socket_candidates_fall_back_to_the_system_path_alone() { + let _guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); assert_eq!( - discover_socket_path(), - dir.path().join("comenq/comenq.sock") + socket_candidates(), + vec![PathBuf::from(DEFAULT_SOCKET_PATH)] ); } + + #[serial_test::serial] + #[test] + fn socket_candidates_deduplicate_identical_paths() { + let _guard = EnvVarGuard::set("XDG_RUNTIME_DIR", "/run/comenq"); + // Contrived: the user path resolves inside /run/comenq, but the + // system path must not be listed twice if they ever coincide. + let candidates = socket_candidates(); + let system_count = candidates + .iter() + .filter(|p| **p == PathBuf::from(DEFAULT_SOCKET_PATH)) + .count(); + assert!(system_count <= 1); + } } }