diff --git a/Cargo.lock b/Cargo.lock index b4b824b..7de6c35 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", @@ -386,6 +389,7 @@ dependencies = [ "figment", "octocrab", "ortho_config", + "rand 0.9.5", "rstest", "serde", "serde_json", @@ -1893,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]] @@ -1904,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]] @@ -1916,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" @@ -3526,7 +3559,7 @@ dependencies = [ "lazy_static", "log", "notify", - "rand", + "rand 0.8.5", "semver", "sysinfo", ] diff --git a/Cargo.toml b/Cargo.toml index b915a19..485ef55 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" @@ -59,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/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/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..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 @@ -42,13 +66,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 candidates = args.socket_candidates(); let request = CommentRequest { owner: args.repo_slug.owner().to_owned(), repo: args.repo_slug.repo().to_owned(), @@ -58,9 +83,7 @@ pub async fn run(args: Args) -> Result<(), ClientError> { let payload = serde_json::to_vec(&request)?; - let mut stream = UnixStream::connect(&args.socket) - .await - .map_err(ClientError::Connect)?; + let mut stream = connect_first(&candidates).await?; stream .write_all(&payload) .await @@ -98,7 +121,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"); @@ -109,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"); @@ -118,7 +174,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..3877e52 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -104,12 +104,54 @@ 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 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 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 + /// + /// ```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_candidates(), + /// vec![std::path::PathBuf::from("/tmp/comenq.sock")] + /// ); + /// ``` + #[must_use] + pub fn socket_candidates(&self) -> Vec { + self.socket + .clone() + .map_or_else(comenq_lib::socket_candidates, |explicit| vec![explicit]) + } } #[cfg(test)] @@ -118,6 +160,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 +217,67 @@ mod tests { assert_eq!(slug.repo(), "hello-world"); } + #[serial_test::serial] + #[test] + 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, None); + assert_eq!( + args.socket_candidates(), + vec![PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH)] + ); + } + + #[serial_test::serial] + #[test] + 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( + "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_candidates(), + vec![ + dir.path().join("comenq/comenq.sock"), + PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH), + ] + ); + } + + #[serial_test::serial] #[test] - fn socket_default_matches_constant() { + 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, PathBuf::from(comenq_lib::DEFAULT_SOCKET_PATH)); + assert_eq!(args.socket, Some(PathBuf::from("/tmp/custom.sock"))); + assert_eq!( + args.socket_candidates(), + vec![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/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 0577af4..4315c09 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; @@ -17,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. @@ -28,8 +29,25 @@ 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 + /// 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. @@ -38,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, @@ -78,9 +103,11 @@ impl From for Config { } = value; Self { github_token, + github_token_file: None, socket_path, queue_path, cooldown_period_seconds, + cooldown_flutter_seconds: DEFAULT_COOLDOWN_FLUTTER, restart_min_delay_ms, github_api_timeout_secs, client_channel_capacity, @@ -116,9 +143,11 @@ 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, + 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, @@ -135,19 +164,25 @@ 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, } fn default_socket_path() -> PathBuf { - PathBuf::from(DEFAULT_SOCKET_PATH) + comenq_lib::default_socket_path() } fn default_queue_path() -> PathBuf { @@ -158,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 } @@ -232,151 +271,80 @@ 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) } -} - -#[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")); + /// 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(()) } +} - #[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 - ); - } +/// 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)] +mod tests; diff --git a/crates/comenqd/src/config/tests.rs b/crates/comenqd/src/config/tests.rs new file mode 100644 index 0000000..74a76c5 --- /dev/null +++ b/crates/comenqd/src/config/tests.rs @@ -0,0 +1,268 @@ +//! 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.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); +} + +#[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, + 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))] +#[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/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:?}" + ); +} 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" + ); +} 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/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/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..1f43db0 100644 --- a/packaging/man/comenq.1 +++ b/packaging/man/comenq.1 @@ -21,10 +21,16 @@ 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 tries +.I $XDG_RUNTIME_DIR/comenq/comenq.sock +(a user-hosted daemon) and then .I /run/comenq/comenq.sock -which matches the packaged systemd service configuration. +(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/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/src/lib.rs b/src/lib.rs index aa08a4c..e355a08 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,12 +4,82 @@ //! 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)) +} + +/// Candidate sockets a client should try, in preference order. +/// +/// 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 +/// for path in comenq_lib::socket_candidates() { +/// println!("would try {}", path.display()); +/// } +/// ``` +#[must_use] +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. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct CommentRequest { @@ -73,4 +143,90 @@ 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, socket_candidates, 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 socket_candidates_prefer_the_user_socket() { + 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!( + socket_candidates(), + vec![ + dir.path().join("comenq/comenq.sock"), + PathBuf::from(DEFAULT_SOCKET_PATH), + ] + ); + } + + #[serial_test::serial] + #[test] + fn socket_candidates_fall_back_to_the_system_path_alone() { + let _guard = EnvVarGuard::remove("XDG_RUNTIME_DIR"); + assert_eq!( + 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); + } + } } 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..62c9612 100644 --- a/tests/features/config.feature +++ b/tests/features/config.feature @@ -27,11 +27,28 @@ 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: 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/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/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..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")); @@ -87,6 +115,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")?; 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(()) +}