From 4b7867b2dca32d32e015c0ecd1db59c0a108f607 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 17:24:48 +0200 Subject: [PATCH 1/6] Define the client-daemon queue protocol Add a `protocol` module to `comenq-lib` with tagged `Request` and `Response` enums covering the forthcoming queue operations: `put`, `list`, `bump`, `bust`, and `del`. `PendingEntry` carries the deterministic identifier, approximate ETA in seconds, target pull request, and full comment body; display truncation is left to consumers. Each connection will carry one JSON request and one JSON reply. Derive `Clone` on `CommentRequest` so queue entries can embed it. --- src/lib.rs | 4 +- src/protocol.rs | 188 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 src/protocol.rs diff --git a/src/lib.rs b/src/lib.rs index a7a3540..8cdb246 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,8 @@ use serde::{Deserialize, Serialize}; use std::env; use std::path::PathBuf; +pub mod protocol; + /// Default Unix Domain Socket path for the Comenq daemon. /// /// Shared by the daemon and CLI to avoid configuration drift. @@ -78,7 +80,7 @@ pub fn discover_socket_path() -> PathBuf { } /// Request sent from the client to the daemon. -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct CommentRequest { /// Repository owner. pub owner: String, diff --git a/src/protocol.rs b/src/protocol.rs new file mode 100644 index 0000000..5523076 --- /dev/null +++ b/src/protocol.rs @@ -0,0 +1,188 @@ +//! Request and response types exchanged between the `comenq` client and the +//! `comenqd` daemon over the Unix domain socket. +//! +//! Every connection carries exactly one JSON-encoded [`Request`]; the daemon +//! replies with one JSON-encoded [`Response`] and closes the connection. + +use serde::{Deserialize, Serialize}; + +use crate::CommentRequest; + +/// Operation requested by the client. +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +#[serde(tag = "op", rename_all = "snake_case")] +pub enum Request { + /// Enqueue a new comment. + Put { + /// The comment to enqueue. + request: CommentRequest, + }, + /// List pending comments in posting order. + List, + /// Move the identified entry to the head of the queue. + Bump { + /// Identifier printed by `list` and `put`. + id: String, + }, + /// Move the identified entry to the tail of the queue. + Bust { + /// Identifier printed by `list` and `put`. + id: String, + }, + /// Remove the identified entry from the queue. + Del { + /// Identifier printed by `list` and `put`. + id: String, + }, +} + +/// A pending queue entry as reported by the daemon. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct PendingEntry { + /// Deterministic eight-character identifier. + pub id: String, + /// Approximate seconds until the comment is posted. + pub eta_seconds: u64, + /// Repository owner. + pub owner: String, + /// Repository name. + pub repo: String, + /// Pull request number. + pub pr_number: u64, + /// Full comment body; consumers truncate for display. + pub body: String, +} + +/// Daemon reply to a [`Request`]. +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +#[serde(tag = "outcome", rename_all = "snake_case")] +pub enum Response { + /// The request succeeded. + Ok { + /// Entry affected by `put`, when applicable. + #[serde(default, skip_serializing_if = "Option::is_none")] + entry: Option, + /// Pending entries, returned by `list`. + #[serde(default, skip_serializing_if = "Option::is_none")] + entries: Option>, + }, + /// The request failed; `message` explains why. + Error { + /// Human-readable failure description. + message: String, + }, +} + +impl Response { + /// Successful reply carrying no payload. + #[must_use] + pub fn ok() -> Self { + Self::Ok { + entry: None, + entries: None, + } + } + + /// Successful reply for `put`, echoing the enqueued entry. + #[must_use] + pub fn entry(entry: PendingEntry) -> Self { + Self::Ok { + entry: Some(entry), + entries: None, + } + } + + /// Successful reply for `list`. + #[must_use] + pub fn entries(entries: Vec) -> Self { + Self::Ok { + entry: None, + entries: Some(entries), + } + } + + /// Failed reply with a description. + #[must_use] + pub fn error(message: impl Into) -> Self { + Self::Error { + message: message.into(), + } + } +} + +#[cfg(test)] +mod tests { + //! Serialization round-trip tests for the client-daemon protocol. + use super::{PendingEntry, Request, Response}; + use crate::CommentRequest; + + fn sample_entry() -> PendingEntry { + PendingEntry { + id: "0011aabb".into(), + eta_seconds: 120, + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 7, + body: "Hi".into(), + } + } + + #[test] + fn put_round_trips_through_json() { + let req = Request::Put { + request: CommentRequest { + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 7, + body: "Hi".into(), + }, + }; + let json = serde_json::to_string(&req).unwrap_or_else(|e| panic!("serialize: {e}")); + assert!(json.contains(r#""op":"put""#), "missing op tag: {json}"); + let back: Request = + serde_json::from_str(&json).unwrap_or_else(|e| panic!("deserialize: {e}")); + assert_eq!(back, req); + } + + #[test] + fn id_operations_round_trip_through_json() { + for req in [ + Request::List, + Request::Bump { + id: "0011aabb".into(), + }, + Request::Bust { + id: "0011aabb".into(), + }, + Request::Del { + id: "0011aabb".into(), + }, + ] { + let json = serde_json::to_string(&req).unwrap_or_else(|e| panic!("serialize: {e}")); + let back: Request = + serde_json::from_str(&json).unwrap_or_else(|e| panic!("deserialize: {e}")); + assert_eq!(back, req); + } + } + + #[test] + fn responses_round_trip_through_json() { + for resp in [ + Response::ok(), + Response::entry(sample_entry()), + Response::entries(vec![sample_entry()]), + Response::error("nope"), + ] { + let json = serde_json::to_string(&resp).unwrap_or_else(|e| panic!("serialize: {e}")); + let back: Response = + serde_json::from_str(&json).unwrap_or_else(|e| panic!("deserialize: {e}")); + assert_eq!(back, resp); + } + } + + #[test] + fn unknown_operation_fails_to_parse() { + let result: Result = serde_json::from_str(r#"{"op":"zap"}"#); + assert!(result.is_err()); + } +} From f03a464718c8dcbd6687e2ad921bd9863561380e Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 17:26:45 +0200 Subject: [PATCH 2/6] Add a reorderable persistent queue store Introduce `comenqd::store::QueueStore`, a filesystem-backed queue holding one JSON file per pending comment under `/entries`. Each entry carries: - a deterministic eight-character identifier (64-bit FNV-1a over the request fields and enqueue time, first eight hex digits), stable across restarts; - an explicit ordering key so entries can be moved to the head (`bump`), moved to the tail (`bust`), or removed (`del`); and - a flutter sampled at enqueue time, fixing the entry's estimated posting time from the moment it is reported. The Unix time of the most recent successful post persists in `/last_post`, letting `schedule` project an estimated posting time for every pending entry: the head is due one full cooldown plus its own flutter after the last post (immediately when nothing has been posted), and each successor follows its predecessor's projected time by a further cooldown plus flutter. Files are written to a temporary sibling and renamed into place so entries are never observed half-written. The append-only `yaque` queue cannot express reordering or deletion, so subsequent commits move the daemon onto this store. --- crates/comenqd/src/lib.rs | 1 + crates/comenqd/src/store.rs | 281 ++++++++++++++++++++++++++++++ crates/comenqd/src/store/tests.rs | 179 +++++++++++++++++++ 3 files changed, 461 insertions(+) create mode 100644 crates/comenqd/src/store.rs create mode 100644 crates/comenqd/src/store/tests.rs diff --git a/crates/comenqd/src/lib.rs b/crates/comenqd/src/lib.rs index b9cd8c3..d59fb41 100644 --- a/crates/comenqd/src/lib.rs +++ b/crates/comenqd/src/lib.rs @@ -18,6 +18,7 @@ pub mod config; mod listener; +pub mod store; mod supervisor; mod util; mod worker; diff --git a/crates/comenqd/src/store.rs b/crates/comenqd/src/store.rs new file mode 100644 index 0000000..e463b9b --- /dev/null +++ b/crates/comenqd/src/store.rs @@ -0,0 +1,281 @@ +//! Persistent, reorderable comment queue. +//! +//! Each pending comment lives in its own JSON file under +//! `/entries`, carrying an explicit ordering key so entries can +//! be moved to the head (`bump`) or tail (`bust`) of the queue, or removed +//! (`del`). The Unix timestamp of the most recent successful post is kept in +//! `/last_post` so estimated posting times survive restarts. +//! +//! The store itself holds no in-memory state; callers serialize mutations +//! (the daemon wraps it in a `tokio::sync::Mutex`). Files are written to a +//! temporary sibling and renamed into place so entries are never observed +//! half-written. + +use comenq_lib::CommentRequest; +use comenq_lib::protocol::PendingEntry; +use rand::Rng; +use serde::{Deserialize, Serialize}; +use std::fs; +use std::io; +use std::path::{Path, PathBuf}; + +/// Sub-directory of the queue path holding one JSON file per entry. +const ENTRIES_DIR: &str = "entries"; +/// File recording the Unix time of the most recent successful post. +const LAST_POST_FILE: &str = "last_post"; + +/// A queued comment with its scheduling metadata. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct StoredEntry { + /// Deterministic eight-character identifier. + pub id: String, + /// Explicit queue position; lower posts first. May go negative after + /// repeated bumps. + pub order: i64, + /// Flutter sampled when the entry was enqueued, in seconds. + pub flutter_seconds: u64, + /// Unix time the entry was enqueued, in seconds. + pub enqueued_at: u64, + /// The comment to post. + pub request: CommentRequest, +} + +impl StoredEntry { + /// Convert to the wire representation with the given ETA. + #[must_use] + pub fn to_pending(&self, eta_seconds: u64) -> PendingEntry { + PendingEntry { + id: self.id.clone(), + eta_seconds, + owner: self.request.owner.clone(), + repo: self.request.repo.clone(), + pr_number: self.request.pr_number, + body: self.request.body.clone(), + } + } +} + +/// Errors raised by queue store operations. +#[derive(Debug, thiserror::Error)] +pub enum StoreError { + /// Underlying filesystem failure. + #[error(transparent)] + Io(#[from] io::Error), + /// Entry serialization failed. + #[error("entry serialization failed: {0}")] + Serde(#[from] serde_json::Error), + /// No entry carries the requested identifier. + #[error("no queued comment has id '{0}'")] + UnknownId(String), +} + +/// Result alias for store operations. +pub type Result = std::result::Result; + +/// Filesystem-backed queue of pending comments. +#[derive(Debug, Clone)] +pub struct QueueStore { + entries_dir: PathBuf, + last_post_path: PathBuf, +} + +/// Compute the deterministic eight-character identifier for an entry. +/// +/// Uses the 64-bit FNV-1a hash of the request fields and enqueue time, +/// rendered as the first eight lowercase hex digits. The hash is content +/// derived, so an entry keeps the same identifier for its whole life and +/// across daemon restarts. +fn entry_id(request: &CommentRequest, enqueued_at: u64) -> String { + const FNV_OFFSET: u64 = 0xcbf2_9ce4_8422_2325; + const FNV_PRIME: u64 = 0x0000_0100_0000_01b3; + let mut hash = FNV_OFFSET; + let mut eat = |bytes: &[u8]| { + for b in bytes { + hash ^= u64::from(*b); + hash = hash.wrapping_mul(FNV_PRIME); + } + }; + eat(request.owner.as_bytes()); + eat(&[0]); + eat(request.repo.as_bytes()); + eat(&[0]); + eat(&request.pr_number.to_le_bytes()); + eat(request.body.as_bytes()); + eat(&enqueued_at.to_le_bytes()); + let hex = format!("{hash:016x}"); + hex.chars().take(8).collect() +} + +impl QueueStore { + /// Open (creating if necessary) the store rooted at `queue_path`. + pub fn open(queue_path: &Path) -> Result { + let entries_dir = queue_path.join(ENTRIES_DIR); + fs::create_dir_all(&entries_dir)?; + Ok(Self { + entries_dir, + last_post_path: queue_path.join(LAST_POST_FILE), + }) + } + + /// Enqueue `request` at the tail, sampling its flutter now. + /// + /// The flutter is fixed at enqueue time so the entry's estimated posting + /// time is stable from the moment it is reported to the client. + pub fn put(&self, request: CommentRequest, flutter_max: u64, now: u64) -> Result { + let flutter_seconds = if flutter_max == 0 { + 0 + } else { + rand::rng().random_range(0..=flutter_max) + }; + let order = self + .entries()? + .last() + .map_or(0, |e| e.order.saturating_add(1)); + let entry = StoredEntry { + id: entry_id(&request, now), + order, + flutter_seconds, + enqueued_at: now, + request, + }; + self.write_entry(&entry)?; + Ok(entry) + } + + /// All pending entries in posting order. + pub fn entries(&self) -> Result> { + let mut entries = Vec::new(); + for dirent in fs::read_dir(&self.entries_dir)? { + let path = dirent?.path(); + if path.extension().is_some_and(|e| e == "json") { + let text = fs::read_to_string(&path)?; + match serde_json::from_str::(&text) { + Ok(entry) => entries.push(entry), + Err(e) => { + tracing::error!( + path = %path.display(), + error = %e, + "Skipping unreadable queue entry" + ); + } + } + } + } + entries + .sort_by(|a, b| (a.order, a.enqueued_at, &a.id).cmp(&(b.order, b.enqueued_at, &b.id))); + Ok(entries) + } + + /// Move the identified entry to the head of the queue. + pub fn bump(&self, id: &str) -> Result<()> { + self.reorder(id, |entry, all| { + all.first().map_or(entry.order, |head| { + if head.id == entry.id { + entry.order + } else { + head.order.saturating_sub(1) + } + }) + }) + } + + /// Move the identified entry to the tail of the queue. + pub fn bust(&self, id: &str) -> Result<()> { + self.reorder(id, |entry, all| { + all.last().map_or(entry.order, |tail| { + if tail.id == entry.id { + entry.order + } else { + tail.order.saturating_add(1) + } + }) + }) + } + + /// Remove the identified entry from the queue. + pub fn del(&self, id: &str) -> Result<()> { + let entry = self.find(id)?; + fs::remove_file(self.entry_path(&entry.id))?; + Ok(()) + } + + /// Unix time of the most recent successful post, when any. + pub fn last_post(&self) -> Option { + let text = fs::read_to_string(&self.last_post_path).ok()?; + text.trim().parse().ok() + } + + /// Remove the posted head entry and record the posting time. + pub fn complete(&self, id: &str, now: u64) -> Result<()> { + self.del(id)?; + self.write_atomic(&self.last_post_path, now.to_string().as_bytes())?; + Ok(()) + } + + /// Pending entries paired with their estimated seconds-until-post. + /// + /// The head is due one full cooldown plus its own flutter after the most + /// recent post (immediately when nothing has been posted yet); each + /// subsequent entry follows a further cooldown plus its own flutter after + /// the projected posting time of its predecessor. + pub fn schedule(&self, cooldown: u64, now: u64) -> Result> { + let mut previous_post = self.last_post(); + let mut scheduled = Vec::new(); + for entry in self.entries()? { + let due = previous_post.map_or(now, |prev| { + prev.saturating_add(cooldown) + .saturating_add(entry.flutter_seconds) + }); + let post_at = due.max(now); + previous_post = Some(post_at); + scheduled.push((entry, post_at.saturating_sub(now))); + } + Ok(scheduled) + } + + /// The head entry and its estimated seconds-until-post, when any. + pub fn next_due(&self, cooldown: u64, now: u64) -> Result> { + Ok(self.schedule(cooldown, now)?.into_iter().next()) + } + + fn entry_path(&self, id: &str) -> PathBuf { + self.entries_dir.join(format!("{id}.json")) + } + + fn find(&self, id: &str) -> Result { + self.entries()? + .into_iter() + .find(|entry| entry.id == id) + .ok_or_else(|| StoreError::UnknownId(id.to_owned())) + } + + fn reorder( + &self, + id: &str, + new_order: impl Fn(&StoredEntry, &[StoredEntry]) -> i64, + ) -> Result<()> { + let all = self.entries()?; + let mut entry = all + .iter() + .find(|entry| entry.id == id) + .cloned() + .ok_or_else(|| StoreError::UnknownId(id.to_owned()))?; + entry.order = new_order(&entry, &all); + self.write_entry(&entry) + } + + fn write_entry(&self, entry: &StoredEntry) -> Result<()> { + let bytes = serde_json::to_vec_pretty(entry)?; + self.write_atomic(&self.entry_path(&entry.id), &bytes) + } + + fn write_atomic(&self, path: &Path, bytes: &[u8]) -> Result<()> { + let tmp = path.with_extension("tmp"); + fs::write(&tmp, bytes)?; + fs::rename(&tmp, path)?; + Ok(()) + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/comenqd/src/store/tests.rs b/crates/comenqd/src/store/tests.rs new file mode 100644 index 0000000..a25abec --- /dev/null +++ b/crates/comenqd/src/store/tests.rs @@ -0,0 +1,179 @@ +//! Tests for the reorderable persistent queue store. + +use super::{QueueStore, StoreError, entry_id}; +use comenq_lib::CommentRequest; +use rstest::rstest; +use tempfile::TempDir; + +fn request(body: &str) -> CommentRequest { + CommentRequest { + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 7, + body: body.into(), + } +} + +fn open_store(dir: &TempDir) -> QueueStore { + QueueStore::open(dir.path()).expect("open store") +} + +fn ids(store: &QueueStore) -> Vec { + store + .entries() + .expect("list entries") + .into_iter() + .map(|entry| entry.id) + .collect() +} + +#[rstest] +fn identifiers_are_deterministic_and_eight_characters() { + let a = entry_id(&request("Hi"), 1000); + let b = entry_id(&request("Hi"), 1000); + assert_eq!(a, b); + assert_eq!(a.len(), 8); + assert!(a.chars().all(|c| c.is_ascii_hexdigit())); + assert_ne!(a, entry_id(&request("Hi"), 1001), "time must vary the id"); + assert_ne!(a, entry_id(&request("Yo"), 1000), "body must vary the id"); +} + +#[rstest] +fn put_preserves_arrival_order() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let first = store.put(request("first"), 0, 1000).expect("put first"); + let second = store.put(request("second"), 0, 1001).expect("put second"); + let third = store.put(request("third"), 0, 1002).expect("put third"); + assert_eq!(ids(&store), vec![first.id, second.id, third.id]); +} + +#[rstest] +fn put_samples_flutter_within_bounds() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + for i in 0..50 { + let entry = store + .put(request(&format!("body {i}")), 240, 1000 + i) + .expect("put entry"); + assert!(entry.flutter_seconds <= 240); + } + let zero = store.put(request("plain"), 0, 2000).expect("put plain"); + assert_eq!(zero.flutter_seconds, 0); +} + +#[rstest] +fn bump_moves_entry_to_head() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + let b = store.put(request("b"), 0, 1001).expect("put b"); + let c = store.put(request("c"), 0, 1002).expect("put c"); + store.bump(&c.id).expect("bump c"); + assert_eq!(ids(&store), vec![c.id, a.id, b.id]); +} + +#[rstest] +fn bump_of_head_is_a_no_op() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + let b = store.put(request("b"), 0, 1001).expect("put b"); + store.bump(&a.id).expect("bump head"); + assert_eq!(ids(&store), vec![a.id, b.id]); +} + +#[rstest] +fn bust_moves_entry_to_tail() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + let b = store.put(request("b"), 0, 1001).expect("put b"); + let c = store.put(request("c"), 0, 1002).expect("put c"); + store.bust(&a.id).expect("bust a"); + assert_eq!(ids(&store), vec![b.id, c.id, a.id]); +} + +#[rstest] +fn del_removes_entry() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + let b = store.put(request("b"), 0, 1001).expect("put b"); + store.del(&a.id).expect("del a"); + assert_eq!(ids(&store), vec![b.id]); +} + +#[rstest] +#[case::bump(|store: &QueueStore| store.bump("deadbeef"))] +#[case::bust(|store: &QueueStore| store.bust("deadbeef"))] +#[case::del(|store: &QueueStore| store.del("deadbeef"))] +fn unknown_ids_are_rejected(#[case] op: fn(&QueueStore) -> super::Result<()>) { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + store.put(request("a"), 0, 1000).expect("put a"); + let err = op(&store).expect_err("unknown id must fail"); + assert!(matches!(err, StoreError::UnknownId(id) if id == "deadbeef")); +} + +#[rstest] +fn schedule_starts_immediately_when_never_posted() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + store.put(request("a"), 0, 1000).expect("put a"); + store.put(request("b"), 0, 1001).expect("put b"); + let schedule = store.schedule(600, 2000).expect("schedule"); + let etas: Vec = schedule.iter().map(|(_, eta)| *eta).collect(); + // Head posts immediately; the next entry follows one full cooldown + // (flutter is zero here). + assert_eq!(etas, vec![0, 600]); +} + +#[rstest] +fn schedule_respects_last_post_and_flutter() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + store.put(request("b"), 0, 1001).expect("put b"); + // Pretend `a` was posted at t=1000 by another entry's completion. + store.complete(&a.id, 1000).expect("complete a"); + let schedule = store.schedule(600, 1100).expect("schedule"); + assert_eq!(schedule.len(), 1); + let (_, eta) = &schedule[0]; + // Due at 1000 + 600 = 1600; now is 1100, so 500 seconds remain. + assert_eq!(*eta, 500); +} + +#[rstest] +fn complete_records_last_post_and_removes_entry() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + assert_eq!(store.last_post(), None); + store.complete(&a.id, 4242).expect("complete a"); + assert_eq!(store.last_post(), Some(4242)); + assert!(ids(&store).is_empty()); +} + +#[rstest] +fn next_due_returns_the_head() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let a = store.put(request("a"), 0, 1000).expect("put a"); + store.put(request("b"), 0, 1001).expect("put b"); + let (head, _) = store + .next_due(600, 2000) + .expect("next_due") + .expect("head entry"); + assert_eq!(head.id, a.id); +} + +#[rstest] +fn entries_survive_reopen() { + let dir = TempDir::new().expect("tempdir"); + let first = open_store(&dir); + let a = first.put(request("a"), 0, 1000).expect("put a"); + drop(first); + let reopened = open_store(&dir); + assert_eq!(ids(&reopened), vec![a.id]); +} From 5f40fbf7f8367fc502385cc9d8f1c1d170a59944 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 17:41:20 +0200 Subject: [PATCH 3/6] Rewire the daemon around the reorderable queue store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the yaque-based pipeline (listener → mpsc channel → queue writer → worker) with a `SharedQueue` that bundles the persistent store, the daemon configuration, and a change notifier: - The listener now parses protocol `Request`s, executes them against the shared queue, and writes the JSON `Response` back over the connection. Malformed requests receive an error reply instead of a silently dropped connection. - The worker recomputes the head entry's due time on every iteration and sleeps until it falls due; queue mutations and shutdown interrupt the wait, so bump, bust, and del take effect immediately. Successful posts record `last_post`; API failures keep the entry and retry after a full cooldown. - The supervisor sheds the queue-writer task, its respawn logic, and the client channel; it now supervises just the listener and worker over the shared queue. Remove the `yaque` dependency and its metadata-file helper. Rewrite the daemon integration tests and the listener and worker cucumber steps against the protocol, strengthening the success scenario to assert the posted entry leaves the queue — which also caught the success mock returning an unparseable empty body. --- Cargo.lock | 315 +------------------ Cargo.toml | 2 - crates/comenqd/Cargo.toml | 1 - crates/comenqd/src/daemon.rs | 9 +- crates/comenqd/src/lib.rs | 2 +- crates/comenqd/src/listener.rs | 68 ++-- crates/comenqd/src/queue.rs | 120 ++++++++ crates/comenqd/src/store.rs | 11 +- crates/comenqd/src/store/tests.rs | 10 + crates/comenqd/src/supervisor.rs | 162 ++-------- crates/comenqd/src/supervisor/tests.rs | 19 +- crates/comenqd/src/util.rs | 53 ---- crates/comenqd/src/worker.rs | 141 +++------ crates/comenqd/src/worker/tests.rs | 40 +-- crates/comenqd/tests/daemon.rs | 411 ++++++++++--------------- crates/comenqd/tests/listener.rs | 116 ++++--- tests/steps/listener_steps.rs | 109 ++++--- tests/steps/worker_steps.rs | 93 +++--- 18 files changed, 632 insertions(+), 1050 deletions(-) create mode 100644 crates/comenqd/src/queue.rs delete mode 100644 crates/comenqd/src/util.rs diff --git a/Cargo.lock b/Cargo.lock index 7de6c35..5bf9a92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -374,7 +374,6 @@ dependencies = [ "test-support", "tokio", "wiremock", - "yaque", ] [[package]] @@ -389,7 +388,7 @@ dependencies = [ "figment", "octocrab", "ortho_config", - "rand 0.9.5", + "rand", "rstest", "serde", "serde_json", @@ -402,7 +401,6 @@ dependencies = [ "tracing-subscriber", "uuid", "wiremock", - "yaque", ] [[package]] @@ -434,15 +432,6 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" -[[package]] -name = "crossbeam-channel" -version = "0.5.15" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82b8f8f868b36967f9606790d1903570de9ceaf870a7bf9fbbd3016d636a2cb2" -dependencies = [ - "crossbeam-utils", -] - [[package]] name = "crossbeam-deque" version = "0.8.6" @@ -677,16 +666,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "filetime" -version = "0.2.25" -dependencies = [ - "cfg-if", - "libc", - "libredox", - "windows-sys 0.59.0", -] - [[package]] name = "fnv" version = "1.0.7" @@ -702,15 +681,6 @@ dependencies = [ "percent-encoding", ] -[[package]] -name = "fsevent-sys" -version = "4.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "76ee7a02da4d231650c7cea31349b889be2f45ddb3ef3032d2ec8185f6313fd2" -dependencies = [ - "libc", -] - [[package]] name = "futures" version = "0.3.31" @@ -1246,26 +1216,6 @@ version = "0.1.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c8fae54786f62fb2918dcfae3d568594e50eb9b5c25bf04371af6fe7516452fb" -[[package]] -name = "inotify" -version = "0.9.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f8069d3ec154eb856955c1c0fbffefbf5f3c40a104ec912d4797314c1801abff" -dependencies = [ - "bitflags 1.3.2", - "inotify-sys", - "libc", -] - -[[package]] -name = "inotify-sys" -version = "0.1.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e05c02b5e89bff3b946cedeca278abc628fe811e604f027c45a8aa3cf793d0eb" -dependencies = [ - "libc", -] - [[package]] name = "inventory" version = "0.3.20" @@ -1342,26 +1292,6 @@ dependencies = [ "simple_asn1", ] -[[package]] -name = "kqueue" -version = "1.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eac30106d7dce88daf4a3fcb4879ea939476d5074a9b7ddd0fb97fa4bed5596a" -dependencies = [ - "kqueue-sys", - "libc", -] - -[[package]] -name = "kqueue-sys" -version = "1.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed9625ffda8729b85e45cf04090035ac368927b8cebc34898e7c120f52e4838b" -dependencies = [ - "bitflags 1.3.2", - "libc", -] - [[package]] name = "lazy-regex" version = "3.4.1" @@ -1405,7 +1335,6 @@ checksum = "4488594b9328dee448adb906d8b126d9b7deb7cf5c22161ee591610bb1be83c0" dependencies = [ "bitflags 2.9.1", "libc", - "redox_syscall", ] [[package]] @@ -1472,18 +1401,6 @@ dependencies = [ "adler2", ] -[[package]] -name = "mio" -version = "0.8.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4a650543ca06a924e8b371db273b2756685faae30f8487da1b56505a8f78b0c" -dependencies = [ - "libc", - "log", - "wasi 0.11.1+wasi-snapshot-preview1", - "windows-sys 0.48.0", -] - [[package]] name = "mio" version = "1.0.4" @@ -1516,33 +1433,6 @@ dependencies = [ "nom", ] -[[package]] -name = "notify" -version = "5.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "729f63e1ca555a43fe3efa4f3efdf4801c479da85b432242a7b726f353c88486" -dependencies = [ - "bitflags 1.3.2", - "crossbeam-channel", - "filetime", - "fsevent-sys", - "inotify", - "kqueue", - "libc", - "mio 0.8.11", - "walkdir", - "windows-sys 0.45.0", -] - -[[package]] -name = "ntapi" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8a3895c6391c39d7fe7ebc444a87eb2991b2a0bc718fdabd071eec617fc68e4" -dependencies = [ - "winapi", -] - [[package]] name = "nu-ansi-term" version = "0.46.0" @@ -1890,35 +1780,14 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" -[[package]] -name = "rand" -version = "0.8.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" -dependencies = [ - "libc", - "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]] -name = "rand_chacha" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" -dependencies = [ - "ppv-lite86", - "rand_core 0.6.4", + "rand_chacha", + "rand_core", ] [[package]] @@ -1928,16 +1797,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" dependencies = [ "ppv-lite86", - "rand_core 0.9.5", -] - -[[package]] -name = "rand_core" -version = "0.6.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" -dependencies = [ - "getrandom 0.2.16", + "rand_core", ] [[package]] @@ -2518,20 +2378,6 @@ dependencies = [ "syn", ] -[[package]] -name = "sysinfo" -version = "0.28.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4c2f3ca6693feb29a89724516f016488e9aafc7f37264f898593ee4b942f31b" -dependencies = [ - "cfg-if", - "core-foundation-sys", - "libc", - "ntapi", - "once_cell", - "winapi", -] - [[package]] name = "tempfile" version = "3.20.0" @@ -2688,7 +2534,7 @@ dependencies = [ "bytes", "io-uring", "libc", - "mio 1.0.4", + "mio", "parking_lot", "pin-project-lite", "signal-hook-registry", @@ -3202,24 +3048,6 @@ dependencies = [ "windows-link", ] -[[package]] -name = "windows-sys" -version = "0.45.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" -dependencies = [ - "windows-targets 0.42.2", -] - -[[package]] -name = "windows-sys" -version = "0.48.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "677d2418bec65e3338edb076e806bc1ec15693c5d0104683f2efe857f61056a9" -dependencies = [ - "windows-targets 0.48.5", -] - [[package]] name = "windows-sys" version = "0.52.0" @@ -3247,36 +3075,6 @@ dependencies = [ "windows-targets 0.53.2", ] -[[package]] -name = "windows-targets" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e5180c00cd44c9b1c88adb3693291f1cd93605ded80c250a75d472756b4d071" -dependencies = [ - "windows_aarch64_gnullvm 0.42.2", - "windows_aarch64_msvc 0.42.2", - "windows_i686_gnu 0.42.2", - "windows_i686_msvc 0.42.2", - "windows_x86_64_gnu 0.42.2", - "windows_x86_64_gnullvm 0.42.2", - "windows_x86_64_msvc 0.42.2", -] - -[[package]] -name = "windows-targets" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a2fa6e2155d7247be68c096456083145c183cbbbc2764150dda45a87197940c" -dependencies = [ - "windows_aarch64_gnullvm 0.48.5", - "windows_aarch64_msvc 0.48.5", - "windows_i686_gnu 0.48.5", - "windows_i686_msvc 0.48.5", - "windows_x86_64_gnu 0.48.5", - "windows_x86_64_gnullvm 0.48.5", - "windows_x86_64_msvc 0.48.5", -] - [[package]] name = "windows-targets" version = "0.52.6" @@ -3309,18 +3107,6 @@ dependencies = [ "windows_x86_64_msvc 0.53.0", ] -[[package]] -name = "windows_aarch64_gnullvm" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "597a5118570b68bc08d8d59125332c54f1ba9d9adeedeef5b99b02ba2b0698f8" - -[[package]] -name = "windows_aarch64_gnullvm" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2b38e32f0abccf9987a4e3079dfb67dcd799fb61361e53e2882c3cbaf0d905d8" - [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" @@ -3333,18 +3119,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "86b8d5f90ddd19cb4a147a5fa63ca848db3df085e25fee3cc10b39b6eebae764" -[[package]] -name = "windows_aarch64_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e08e8864a60f06ef0d0ff4ba04124db8b0fb3be5776a5cd47641e942e58c4d43" - -[[package]] -name = "windows_aarch64_msvc" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc35310971f3b2dbbf3f0690a219f40e2d9afcf64f9ab7cc1be722937c26b4bc" - [[package]] name = "windows_aarch64_msvc" version = "0.52.6" @@ -3357,18 +3131,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c7651a1f62a11b8cbd5e0d42526e55f2c99886c77e007179efff86c2b137e66c" -[[package]] -name = "windows_i686_gnu" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c61d927d8da41da96a81f029489353e68739737d3beca43145c8afec9a31a84f" - -[[package]] -name = "windows_i686_gnu" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a75915e7def60c94dcef72200b9a8e58e5091744960da64ec734a6c6e9b3743e" - [[package]] name = "windows_i686_gnu" version = "0.52.6" @@ -3393,18 +3155,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ce6ccbdedbf6d6354471319e781c0dfef054c81fbc7cf83f338a4296c0cae11" -[[package]] -name = "windows_i686_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "44d840b6ec649f480a41c8d80f9c65108b92d89345dd94027bfe06ac444d1060" - -[[package]] -name = "windows_i686_msvc" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f55c233f70c4b27f66c523580f78f1004e8b5a8b659e05a4eb49d4166cca406" - [[package]] name = "windows_i686_msvc" version = "0.52.6" @@ -3417,18 +3167,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "581fee95406bb13382d2f65cd4a908ca7b1e4c2f1917f143ba16efe98a589b5d" -[[package]] -name = "windows_x86_64_gnu" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8de912b8b8feb55c064867cf047dda097f92d51efad5b491dfb98f6bbb70cb36" - -[[package]] -name = "windows_x86_64_gnu" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53d40abd2583d23e4718fddf1ebec84dbff8381c07cae67ff7768bbf19c6718e" - [[package]] name = "windows_x86_64_gnu" version = "0.52.6" @@ -3441,18 +3179,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2e55b5ac9ea33f2fc1716d1742db15574fd6fc8dadc51caab1c16a3d3b4190ba" -[[package]] -name = "windows_x86_64_gnullvm" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26d41b46a36d453748aedef1486d5c7a85db22e56aff34643984ea85514e94a3" - -[[package]] -name = "windows_x86_64_gnullvm" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b7b52767868a23d5bab768e390dc5f5c55825b6d30b86c844ff2dc7414044cc" - [[package]] name = "windows_x86_64_gnullvm" version = "0.52.6" @@ -3465,18 +3191,6 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0a6e035dd0599267ce1ee132e51c27dd29437f63325753051e71dd9e42406c57" -[[package]] -name = "windows_x86_64_msvc" -version = "0.42.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9aec5da331524158c6d1a4ac0ab1541149c0b9505fde06423b02f5ef0106b9f0" - -[[package]] -name = "windows_x86_64_msvc" -version = "0.48.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" - [[package]] name = "windows_x86_64_msvc" version = "0.52.6" @@ -3549,21 +3263,6 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cfe53a6657fd280eaa890a3bc59152892ffa3e30101319d168b781ed6529b049" -[[package]] -name = "yaque" -version = "0.6.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "487f1a92dacd945cc5bc78a8193cc00b9a2cce3c07746ca51533f513843f40d2" -dependencies = [ - "futures", - "lazy_static", - "log", - "notify", - "rand 0.8.5", - "semver", - "sysinfo", -] - [[package]] name = "yoke" version = "0.8.0" @@ -3667,3 +3366,7 @@ dependencies = [ "quote", "syn", ] + +[[patch.unused]] +name = "filetime" +version = "0.2.25" diff --git a/Cargo.toml b/Cargo.toml index 485ef55..1e136be 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,6 @@ 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 } octocrab = { workspace = true } test-support = { workspace = true } @@ -46,7 +45,6 @@ clap = { version = "4.4", features = ["derive", "env"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" octocrab = "0.38" - yaque = "0.6" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } anyhow = "1.0" diff --git a/crates/comenqd/Cargo.toml b/crates/comenqd/Cargo.toml index b35a194..21ec12e 100644 --- a/crates/comenqd/Cargo.toml +++ b/crates/comenqd/Cargo.toml @@ -16,7 +16,6 @@ clap = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } octocrab = { workspace = true } -yaque = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } anyhow = { workspace = true } diff --git a/crates/comenqd/src/daemon.rs b/crates/comenqd/src/daemon.rs index 40a9921..9fec288 100644 --- a/crates/comenqd/src/daemon.rs +++ b/crates/comenqd/src/daemon.rs @@ -8,20 +8,17 @@ pub use crate::supervisor::SupervisorError as DaemonError; /// Create the queue directory (idempotent). pub use crate::supervisor::ensure_queue_dir; -/// Spawn the background writer that persists enqueued payloads. -pub use crate::supervisor::queue_writer; /// Run the daemon orchestration loop. pub use crate::supervisor::run; +/// Shared queue state used by the listener and worker. +pub use crate::queue::SharedQueue; + /// Run the worker that drains the queue and talks to the GitHub API. pub use crate::worker::run_worker; /// Control handle and lifecycle hooks for the worker task. pub use crate::worker::{WorkerControl, WorkerHooks}; -#[cfg(feature = "test-support")] -#[doc(hidden)] -pub use crate::util::is_metadata_file; - pub mod listener { //! Listener utilities for accepting client connections. //! diff --git a/crates/comenqd/src/lib.rs b/crates/comenqd/src/lib.rs index d59fb41..36d1505 100644 --- a/crates/comenqd/src/lib.rs +++ b/crates/comenqd/src/lib.rs @@ -18,9 +18,9 @@ pub mod config; mod listener; +pub mod queue; pub mod store; mod supervisor; -mod util; mod worker; pub mod daemon; diff --git a/crates/comenqd/src/listener.rs b/crates/comenqd/src/listener.rs index d74e850..b9db9e6 100644 --- a/crates/comenqd/src/listener.rs +++ b/crates/comenqd/src/listener.rs @@ -1,21 +1,21 @@ //! Unix socket listener for comenqd. //! -//! Accepts client connections, deserializes requests, and forwards them to the -//! persistent queue for processing by the worker. +//! Accepts client connections, deserializes protocol requests, executes them +//! against the shared queue, and writes the JSON reply back to the client. -use crate::config::Config; use anyhow::{Context, Result}; -use comenq_lib::CommentRequest; +use comenq_lib::protocol::{Request, Response}; use std::fs as stdfs; use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::sync::Arc; use std::time::Duration; -use tokio::io::AsyncReadExt; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::{UnixListener, UnixStream}; -use tokio::sync::{mpsc, watch}; +use tokio::sync::watch; use uuid::Uuid; +use crate::queue::SharedQueue; use crate::supervisor::backoff; /// Prepare a Unix domain socket for the listener. @@ -76,22 +76,22 @@ pub fn prepare_listener(path: &Path) -> Result { /// Listen on the Unix socket and spawn a handler for each client. /// -/// The listener accepts connections on the path configured in [`Config`]. Each -/// connection is handled concurrently by [`handle_client`], forwarding valid -/// requests to the queue writer. The function exits when the `shutdown` watch -/// channel is triggered. +/// The listener accepts connections on the path configured in the shared +/// queue's configuration. Each connection is handled concurrently by +/// [`handle_client`], which executes the request and replies. The function +/// exits when the `shutdown` watch channel is triggered. /// /// # Errors /// Returns an error if the socket cannot be created or if accepting a /// connection fails after retries. Exiting due to a shutdown signal is normal /// and not treated as an error. pub async fn run_listener( - config: Arc, - tx: mpsc::Sender>, + queue: Arc, mut shutdown: watch::Receiver<()>, ) -> Result<()> { - let listener = prepare_listener(&config.socket_path)?; - let min_delay = Duration::from_millis(config.restart_min_delay_ms); + let cfg = queue.config(); + let listener = prepare_listener(&cfg.socket_path)?; + let min_delay = Duration::from_millis(cfg.restart_min_delay_ms); let mut accept_backoff = backoff(min_delay); loop { @@ -102,9 +102,9 @@ pub async fn run_listener( let cred = stream.peer_cred().ok(); let pid = cred.as_ref().map(|c| c.pid()); let uid = cred.as_ref().map(|c| c.uid()); - let tx_clone = tx.clone(); + let queue = queue.clone(); tokio::spawn(async move { - if let Err(e) = handle_client(stream, tx_clone).await { + if let Err(e) = handle_client(stream, queue).await { match (pid, uid) { (Some(pid), Some(uid)) => { tracing::warn!(pid, uid, error = %e, "Client handling failed"); @@ -133,19 +133,22 @@ pub async fn run_listener( Ok(()) } -/// Read a single request from `stream` and forward it to the queue. -/// -/// Expects the client to send a JSON encoded [`CommentRequest`] and then close -/// the connection. The request is re-encoded to bytes and sent over `tx` for the -/// queue writer to persist. -/// -/// # Errors -/// Fails if reading from the socket or parsing JSON fails, or if the queue -/// writer has shut down. +/// Maximum accepted request payload, in bytes. pub const MAX_REQUEST_BYTES: usize = 1024 * 1024; // 1 MiB +/// Seconds a client has to transmit its request. pub const CLIENT_READ_TIMEOUT_SECS: u64 = 5; -pub async fn handle_client(stream: UnixStream, tx: mpsc::Sender>) -> Result<()> { +/// Read a single request from `stream`, execute it, and reply. +/// +/// Expects the client to send one JSON encoded [`Request`] and then close its +/// write side. The reply is a JSON encoded [`Response`] written back over the +/// same connection. A malformed request receives an error reply rather than +/// silently closing the connection. +/// +/// # Errors +/// Fails if reading from or writing to the socket fails, or if the payload +/// exceeds [`MAX_REQUEST_BYTES`]. +pub async fn handle_client(stream: UnixStream, queue: Arc) -> Result<()> { let mut buffer = Vec::with_capacity(8 * 1024); // Read up to LIMIT+1 to detect oversize payloads without relying on client EOF. let mut limited = stream.take((MAX_REQUEST_BYTES as u64) + 1); @@ -158,11 +161,14 @@ pub async fn handle_client(stream: UnixStream, tx: mpsc::Sender>) -> Res if buffer.len() > MAX_REQUEST_BYTES { anyhow::bail!("client payload exceeds {} bytes", MAX_REQUEST_BYTES); } - let request: CommentRequest = serde_json::from_slice(&buffer)?; - let bytes = serde_json::to_vec(&request)?; - tx.send(bytes) - .await - .map_err(|_| anyhow::anyhow!("queue writer dropped"))?; + let response = match serde_json::from_slice::(&buffer) { + Ok(request) => queue.execute(request).await, + Err(e) => Response::error(format!("invalid request: {e}")), + }; + let bytes = serde_json::to_vec(&response)?; + let mut stream = limited.into_inner(); + stream.write_all(&bytes).await.context("write response")?; + stream.shutdown().await.context("close connection")?; Ok(()) } diff --git a/crates/comenqd/src/queue.rs b/crates/comenqd/src/queue.rs new file mode 100644 index 0000000..7d94001 --- /dev/null +++ b/crates/comenqd/src/queue.rs @@ -0,0 +1,120 @@ +//! Shared queue state and protocol operation dispatch. +//! +//! [`SharedQueue`] bundles the persistent [`QueueStore`] with the daemon +//! configuration and a change signal. The listener executes protocol +//! requests against it, and the worker waits on the change signal so queue +//! mutations (put, bump, bust, del) are observed promptly. + +use std::sync::Arc; +use std::time::{SystemTime, UNIX_EPOCH}; + +use comenq_lib::protocol::{Request, Response}; +use tokio::sync::{Mutex, Notify}; + +use crate::config::Config; +use crate::store::{QueueStore, Result as StoreResult, StoredEntry}; + +/// Current Unix time in whole seconds. +/// +/// Clamps to zero should the system clock report a time before the epoch. +#[must_use] +pub fn unix_now() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_or(0, |d| d.as_secs()) +} + +/// Queue state shared between the listener and the worker. +#[derive(Debug)] +pub struct SharedQueue { + cfg: Arc, + store: Mutex, + changed: Notify, +} + +impl SharedQueue { + /// Open the queue store described by `cfg`. + pub fn open(cfg: Arc) -> StoreResult> { + let store = QueueStore::open(&cfg.queue_path)?; + Ok(Arc::new(Self { + cfg, + store: Mutex::new(store), + changed: Notify::new(), + })) + } + + /// The daemon configuration this queue was opened with. + #[must_use] + pub fn config(&self) -> &Arc { + &self.cfg + } + + /// Wait until the queue contents change. + pub async fn changed(&self) { + self.changed.notified().await; + } + + /// The head entry and its estimated seconds-until-post, when any. + pub async fn next_due(&self) -> StoreResult> { + self.store + .lock() + .await + .next_due(self.cfg.cooldown_period_seconds, unix_now()) + } + + /// Remove the posted entry and record the posting time. + pub async fn complete(&self, id: &str) -> StoreResult<()> { + self.store.lock().await.complete(id, unix_now()) + } + + /// Execute a protocol request and produce the reply. + /// + /// Mutations signal the worker through the change notifier. Failures are + /// reported to the client as [`Response::Error`]; they never propagate. + pub async fn execute(&self, request: Request) -> Response { + let store = self.store.lock().await; + let cooldown = self.cfg.cooldown_period_seconds; + let now = unix_now(); + let (response, mutated) = match request { + Request::Put { request } => { + let outcome = store + .put(request, self.cfg.cooldown_flutter_seconds, now) + .and_then(|entry| { + let eta = store + .schedule(cooldown, now)? + .into_iter() + .find(|(scheduled, _)| scheduled.id == entry.id) + .map_or(0, |(_, eta)| eta); + Ok(Response::entry(entry.to_pending(eta))) + }); + (outcome, true) + } + Request::List => { + let outcome = store.schedule(cooldown, now).map(|schedule| { + Response::entries( + schedule + .into_iter() + .map(|(entry, eta)| entry.to_pending(eta)) + .collect(), + ) + }); + (outcome, false) + } + Request::Bump { id } => (store.bump(&id).map(|()| Response::ok()), true), + Request::Bust { id } => (store.bust(&id).map(|()| Response::ok()), true), + Request::Del { id } => (store.del(&id).map(|()| Response::ok()), true), + }; + drop(store); + match response { + Ok(reply) => { + if mutated { + // notify_one buffers a permit, so a worker that is busy + // computing rather than parked still observes the change. + self.changed.notify_one(); + } + reply + } + Err(e) => Response::error(e.to_string()), + } + } +} diff --git a/crates/comenqd/src/store.rs b/crates/comenqd/src/store.rs index e463b9b..dbb73bf 100644 --- a/crates/comenqd/src/store.rs +++ b/crates/comenqd/src/store.rs @@ -121,7 +121,16 @@ impl QueueStore { /// /// The flutter is fixed at enqueue time so the entry's estimated posting /// time is stable from the moment it is reported to the client. + /// + /// Identifiers derive from the request content and enqueue second, so an + /// identical request repeated within the same second maps to the same + /// identifier; the operation is idempotent and returns the existing + /// entry unchanged. pub fn put(&self, request: CommentRequest, flutter_max: u64, now: u64) -> Result { + let id = entry_id(&request, now); + if let Ok(existing) = self.find(&id) { + return Ok(existing); + } let flutter_seconds = if flutter_max == 0 { 0 } else { @@ -132,7 +141,7 @@ impl QueueStore { .last() .map_or(0, |e| e.order.saturating_add(1)); let entry = StoredEntry { - id: entry_id(&request, now), + id, order, flutter_seconds, enqueued_at: now, diff --git a/crates/comenqd/src/store/tests.rs b/crates/comenqd/src/store/tests.rs index a25abec..b0eb444 100644 --- a/crates/comenqd/src/store/tests.rs +++ b/crates/comenqd/src/store/tests.rs @@ -177,3 +177,13 @@ fn entries_survive_reopen() { let reopened = open_store(&dir); assert_eq!(ids(&reopened), vec![a.id]); } + +#[rstest] +fn identical_put_within_the_same_second_is_idempotent() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let first = store.put(request("dup"), 240, 1000).expect("first put"); + let second = store.put(request("dup"), 240, 1000).expect("second put"); + assert_eq!(first, second, "repeat put must return the existing entry"); + assert_eq!(ids(&store).len(), 1); +} diff --git a/crates/comenqd/src/supervisor.rs b/crates/comenqd/src/supervisor.rs index e2ca4b0..65abf3b 100644 --- a/crates/comenqd/src/supervisor.rs +++ b/crates/comenqd/src/supervisor.rs @@ -1,7 +1,7 @@ //! Task orchestration for comenqd. //! -//! Coordinates the listener, queue writer, and worker tasks, applying -//! exponential backoff on failure and handling graceful shutdown. +//! Coordinates the listener and worker tasks, applying exponential backoff on +//! failure and handling graceful shutdown. use crate::config::Config; use backon::{ExponentialBackoff, ExponentialBuilder}; @@ -13,10 +13,11 @@ use thiserror::Error; use tokio::fs; #[cfg(unix)] use tokio::signal::unix::{SignalKind, signal}; -use tokio::sync::{mpsc, watch}; -use yaque::{Receiver, Sender}; +use tokio::sync::watch; use crate::listener::run_listener; +use crate::queue::SharedQueue; +use crate::store::StoreError; use crate::worker::{WorkerControl, WorkerHooks, build_octocrab, run_worker}; #[derive(Debug, Error)] @@ -25,6 +26,8 @@ pub enum SupervisorError { Io(#[from] std::io::Error), #[error(transparent)] Octocrab(#[from] octocrab::Error), + #[error(transparent)] + Store(#[from] StoreError), } pub type Result = std::result::Result; @@ -103,125 +106,21 @@ async fn supervise_task( } } -async fn supervise_writer( - mut handle: tokio::task::JoinHandle>>, - mut backoff: ExponentialBackoff, - mut backoff_builder: B, - cfg: Arc, - client_tx: Arc>>>, - shutdown_tx: watch::Sender<()>, - mut shutdown: watch::Receiver<()>, -) where - B: FnMut() -> ExponentialBackoff, -{ - loop { - tokio::select! { - _ = shutdown.changed() => { - let grace = tokio::time::sleep(Duration::from_millis(100)); - tokio::select! { - _ = &mut handle => {} - _ = grace => handle.abort(), - } - break; - } - res = &mut handle => { - let rx = match res { - Ok(r) => r, - Err(e) => { - // Only log join failures here; queue_writer logs enqueue errors. - log_task_failure::<(), _>("writer", &Err(e)); - let pair = mpsc::channel(cfg.client_channel_capacity); - *client_tx.lock().await = pair.0; - pair.1 - } - }; - let delay = backoff.next().unwrap_or(BACKOFF_FALLBACK_DELAY); - if sleep_or_shutdown(&mut shutdown, delay).await { - break; - } - backoff = backoff_builder(); - // 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) => { - tracing::error!(error = %e, "Queue sender creation failed"); - let _ = shutdown_tx.send(()); - break; - } - } - } - } - } -} - -/// Forward bytes from a channel into the persistent queue. -/// -/// The queue writer decouples the listener from the queue, ensuring a -/// single writer for the `yaque` queue. It reads raw JSON payloads from the -/// provided [`mpsc::Receiver`] and attempts to enqueue each item -/// using the [`yaque::Sender`]. On enqueue failure the error is logged and the -/// loop terminates so a supervising task can recreate the sender. -/// -/// When the loop terminates the receiver is returned so a supervising task can -/// resume consumption without losing any buffered requests. -/// -/// # Examples -/// ```rust,no_run -/// use yaque::channel; -/// use tokio::sync::mpsc; -/// # async fn docs() -> anyhow::Result<()> { -/// let (queue_tx, _rx) = channel("/tmp/q")?; -/// let (tx, rx) = mpsc::channel(1); -/// tokio::spawn(async move { comenqd::daemon::queue_writer(queue_tx, rx).await }); -/// tx.send(Vec::new()) -/// .await -/// .expect("send on docs channel failed"); -/// # Ok(()) -/// # } -/// ``` -pub async fn queue_writer( - mut sender: Sender, - mut rx: mpsc::Receiver>, -) -> mpsc::Receiver> { - while let Some(bytes) = rx.recv().await { - if let Err(e) = sender.send(bytes).await { - tracing::error!(error = %e, "Queue enqueue failed"); - break; - } - } - rx -} - /// Start the daemon with the provided configuration. 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)?); - // 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); + let queue = SharedQueue::open(cfg.clone())?; let (shutdown_tx, shutdown_rx) = watch::channel(()); // Initial task spawns and backoff builders. - let writer = tokio::spawn(queue_writer(queue_tx, client_rx)); - let listener_tx = client_tx.clone(); - let listener = spawn_listener( - cfg.clone(), - listener_tx.lock().await.clone(), - shutdown_rx.clone(), - ); - let worker = spawn_worker(cfg.clone(), octocrab.clone(), shutdown_rx.clone()); + let listener = spawn_listener(queue.clone(), shutdown_rx.clone()); + let worker = spawn_worker(queue.clone(), octocrab.clone(), shutdown_rx.clone()); let min_delay = Duration::from_millis(cfg.restart_min_delay_ms); let listener_backoff = backoff(min_delay); let worker_backoff = backoff(min_delay); - let writer_backoff = backoff(min_delay); // Convert SIGINT and SIGTERM into a shutdown signal. #[cfg(unix)] @@ -253,23 +152,15 @@ pub async fn run(config: Config) -> Result<()> { } // Supervise tasks concurrently. - let client_tx_clone = client_tx.clone(); let shutdown_listener = shutdown_rx.clone(); - let shutdown_worker = shutdown_rx.clone(); + let shutdown_worker = shutdown_rx; + let listener_queue = queue.clone(); tokio::join!( supervise_task( "listener", listener, listener_backoff, - || { - let cfg = cfg.clone(); - let client_tx = client_tx_clone.clone(); - let shutdown_listener = shutdown_listener.clone(); - tokio::spawn(async move { - let tx = client_tx.lock().await.clone(); - run_listener(cfg, tx, shutdown_listener).await - }) - }, + || spawn_listener(listener_queue.clone(), shutdown_listener.clone()), shutdown_listener.clone(), || backoff(min_delay), ), @@ -277,44 +168,29 @@ pub async fn run(config: Config) -> Result<()> { "worker", worker, worker_backoff, - || spawn_worker(cfg.clone(), octocrab.clone(), shutdown_worker.clone()), + || spawn_worker(queue.clone(), octocrab.clone(), shutdown_worker.clone()), shutdown_worker.clone(), || backoff(min_delay), ), - supervise_writer( - writer, - writer_backoff, - || backoff(min_delay), - cfg.clone(), - client_tx, - shutdown_tx.clone(), - shutdown_rx, - ), ); Ok(()) } fn spawn_listener( - cfg: Arc, - tx: mpsc::Sender>, + queue: Arc, shutdown: watch::Receiver<()>, ) -> tokio::task::JoinHandle> { - tokio::spawn(run_listener(cfg, tx, shutdown)) + tokio::spawn(run_listener(queue, shutdown)) } fn spawn_worker( - cfg: Arc, + queue: Arc, octocrab: Arc, shutdown: watch::Receiver<()>, ) -> tokio::task::JoinHandle> { - let cfg_clone = cfg.clone(); - tokio::spawn(async move { - // 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 - }) + let control = WorkerControl::new(shutdown, WorkerHooks::default()); + tokio::spawn(run_worker(queue, octocrab, control)) } /// Log any failure from a supervised task. diff --git a/crates/comenqd/src/supervisor/tests.rs b/crates/comenqd/src/supervisor/tests.rs index c0c92b1..6cd261b 100644 --- a/crates/comenqd/src/supervisor/tests.rs +++ b/crates/comenqd/src/supervisor/tests.rs @@ -79,35 +79,26 @@ 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. +/// The worker starts against the shared queue and shuts down cleanly. #[rstest] #[tokio::test] -async fn worker_starts_while_writer_holds_the_sender() { +async fn worker_starts_and_stops_cleanly() { 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 queue = crate::queue::SharedQueue::open(cfg).expect("open shared queue"); 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); + let handle = super::spawn_worker(queue, 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:?}" - ); + assert!(res.is_ok(), "worker must exit cleanly on shutdown: {res:?}"); } diff --git a/crates/comenqd/src/util.rs b/crates/comenqd/src/util.rs deleted file mode 100644 index 26d29f4..0000000 --- a/crates/comenqd/src/util.rs +++ /dev/null @@ -1,53 +0,0 @@ -//! Internal utilities shared by daemon components. -//! -//! Provides helpers used across production code and tests. - -#[cfg(any(test, feature = "test-support"))] -use std::ffi::OsStr; - -/// Names of files storing queue metadata. -/// -/// Extend this list when new metadata files are introduced. -#[cfg(any(test, feature = "test-support"))] -pub(crate) const METADATA_FILE_NAMES: [&str; 3] = ["version", "recv.lock", "send.lock"]; - -/// Returns whether a file name represents queue metadata. -/// -/// # Examples -/// -/// ``` -/// use comenqd::daemon::is_metadata_file; -/// use std::ffi::OsStr; -/// assert!(is_metadata_file(OsStr::new("version"))); -/// assert!(!is_metadata_file(OsStr::new("0001"))); -/// ``` -#[cfg(any(test, feature = "test-support"))] -pub fn is_metadata_file(name: impl AsRef) -> bool { - let name = name.as_ref(); - METADATA_FILE_NAMES.iter().any(|m| OsStr::new(m) == name) -} - -#[cfg(test)] -mod tests { - use super::*; - use rstest::rstest; - - #[rstest] - #[case::version("version")] - #[case::recv_lock("recv.lock")] - #[case::send_lock("send.lock")] - fn is_metadata_file_recognises_metadata(#[case] name: &str) { - assert!(is_metadata_file(name)); - } - - #[rstest] - #[case::segment_0000("0000")] - #[case::segment_0001("0001")] - #[case::segment_9999("9999")] - #[case::data_json("data.json")] - #[case::lock("lock")] - #[case::empty("")] - fn is_metadata_file_rejects_non_metadata(#[case] name: &str) { - assert!(!is_metadata_file(name)); - } -} diff --git a/crates/comenqd/src/worker.rs b/crates/comenqd/src/worker.rs index a38ba9a..36081ca 100644 --- a/crates/comenqd/src/worker.rs +++ b/crates/comenqd/src/worker.rs @@ -1,27 +1,19 @@ //! Queue worker for comenqd. //! -//! Dequeues requests from the persistent queue and posts comments to GitHub -//! 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. +//! Watches the shared queue and posts the head comment once its estimated +//! posting time arrives. Cooldowns always run in full; each entry's random +//! flutter was fixed when it was enqueued, so the projected schedule reported +//! to clients matches what the worker executes. use crate::config::Config; +use crate::queue::SharedQueue; use anyhow::Result; use comenq_lib::CommentRequest; use octocrab::Octocrab; -use rand::Rng; use std::sync::Arc; use std::time::Duration; use thiserror::Error; use tokio::sync::{Notify, watch}; -use yaque::Receiver; - -#[cfg(any(test, feature = "test-support"))] -use crate::util::is_metadata_file; -#[cfg(any(test, feature = "test-support"))] -use std::fs as stdfs; -#[cfg(any(test, feature = "test-support"))] -use std::path::Path; /// Errors returned when posting a comment to GitHub. #[derive(Debug, Error)] @@ -34,21 +26,6 @@ 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 { @@ -77,11 +54,11 @@ async fn post_comment( /// multiple tasks await the same hook, only one will be woken per notification. #[derive(Default)] pub struct WorkerHooks { - /// Signalled when a request is retrieved from the queue. + /// Signalled when the worker picks up a due entry for posting. /// /// Only one waiter is supported; additional waiters will not be notified. pub enqueued: Option>, - /// Signalled after the worker completes processing of a request. + /// Signalled after the worker completes processing of an entry. /// /// Only one waiter is supported; additional waiters will not be notified. pub idle: Option>, @@ -104,19 +81,10 @@ impl WorkerHooks { } } - #[cfg(any(test, feature = "test-support"))] - fn notify_drained_if_empty(&self, queue_path: &Path) -> std::io::Result<()> { + fn notify_drained(&self) { if let Some(n) = &self.drained { - // Ignore sentinel files left by the queue implementation and - // consider the directory empty when no other files remain. - let empty = !stdfs::read_dir(queue_path)? - .filter_map(Result::ok) - .any(|e| !is_metadata_file(e.file_name())); - if empty { - n.notify_one(); - } + n.notify_one(); } - Ok(()) } /// Waits for the specified number of seconds or until a shutdown is signalled. @@ -187,77 +155,62 @@ impl WorkerControl { } } -/// Processes queued comment requests and posts them to GitHub, enforcing a cooldown between attempts. +/// Posts queued comments as they fall due, enforcing the scheduled cooldowns. +/// +/// The worker recomputes the head entry's due time on every iteration, so +/// queue mutations (put, bump, bust, del) take effect immediately: the shared +/// queue's change signal interrupts any wait. pub async fn run_worker( - config: Arc, - mut rx: Receiver, + queue: Arc, octocrab: Arc, mut control: WorkerControl, ) -> Result<()> { - let hooks = &mut control.hooks; + let hooks = &control.hooks; let shutdown = &mut control.shutdown; + let config = queue.config().clone(); loop { - let guard = tokio::select! { - biased; - _ = shutdown.changed() => { - break; - } - res = rx.recv() => { - res? + let due = queue.next_due().await?; + let Some((entry, wait_seconds)) = due else { + hooks.notify_drained(); + tokio::select! { + biased; + _ = shutdown.changed() => break, + () = queue.changed() => continue, } }; + if wait_seconds > 0 { + tokio::select! { + biased; + _ = shutdown.changed() => break, + () = queue.changed() => {} + _ = tokio::time::sleep(Duration::from_secs(wait_seconds)) => {} + } + continue; + } hooks.notify_enqueued(); - let request: CommentRequest = match serde_json::from_slice::(&guard) { - Ok(req) => req, + match post_comment(&octocrab, &entry.request, &config).await { + Ok(()) => { + queue.complete(&entry.id).await?; + } Err(e) => { - tracing::error!(error = %e, "Failed to deserialize queued request; dropping"); - if let Err(commit_err) = guard.commit() { - tracing::error!(error = %commit_err, "Failed to commit malformed queue entry"); - } + tracing::error!( + error = %e, + id = %entry.id, + owner = %entry.request.owner, + repo = %entry.request.repo, + pr = entry.request.pr_number, + "GitHub API call failed; will retry after cooldown", + ); hooks.notify_idle(); - #[cfg(any(test, feature = "test-support"))] - 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(cooldown_with_flutter(&config), shutdown).await { + // Pace retries so a persistently failing API is not hammered. + if WorkerHooks::wait_or_shutdown(config.cooldown_period_seconds, shutdown).await { break; } continue; } - }; - - match post_comment(&octocrab, &request, &config).await { - Ok(_) => { - guard.commit()?; - } - Err(PostCommentError::Api(e)) => { - tracing::error!( - error = %e, - owner = %request.owner, - repo = %request.repo, - pr = request.pr_number, - "GitHub API call failed", - ); - } - Err(PostCommentError::Timeout) => { - tracing::error!( - owner = %request.owner, - repo = %request.repo, - pr = request.pr_number, - "GitHub API call timed out", - ); - } } - hooks.notify_idle(); - #[cfg(any(test, feature = "test-support"))] - hooks.notify_drained_if_empty(&config.queue_path)?; - if WorkerHooks::wait_or_shutdown(cooldown_with_flutter(&config), shutdown).await { - break; - } } - #[cfg(any(test, feature = "test-support"))] - hooks.notify_drained_if_empty(&config.queue_path)?; Ok(()) } diff --git a/crates/comenqd/src/worker/tests.rs b/crates/comenqd/src/worker/tests.rs index 8298929..750e299 100644 --- a/crates/comenqd/src/worker/tests.rs +++ b/crates/comenqd/src/worker/tests.rs @@ -1,43 +1,10 @@ -//! Tests for the queue worker's cooldown, flutter, and notification hooks. +//! Tests for the worker's shutdown waits and notification hooks. -use super::{Config, Notify, WorkerHooks, cooldown_with_flutter}; +use super::{Notify, WorkerHooks}; use std::sync::Arc; -use std::time::Duration; -use std::time::Instant; +use std::time::{Duration, 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(()); @@ -75,7 +42,6 @@ async fn wait_or_shutdown_prioritises_shutdown_over_timeout() { #[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)); diff --git a/crates/comenqd/tests/daemon.rs b/crates/comenqd/tests/daemon.rs index e951aa5..47606ed 100644 --- a/crates/comenqd/tests/daemon.rs +++ b/crates/comenqd/tests/daemon.rs @@ -2,13 +2,13 @@ mod util; +use comenq_lib::protocol::{PendingEntry, Request, Response}; use comenqd::config::Config; use comenqd::daemon::{ - WorkerControl, WorkerHooks, + SharedQueue, WorkerControl, WorkerHooks, listener::{handle_client, prepare_listener, run_listener}, - queue_writer, run, run_worker, + run, run_worker, }; -use octocrab::Octocrab; use rstest::{fixture, rstest}; use std::fs as stdfs; use std::os::unix::fs::PermissionsExt; @@ -17,13 +17,12 @@ use std::sync::Arc; use std::time::Duration; use tempfile::{TempDir, tempdir}; use test_support::{octocrab_for, temp_config}; -use tokio::io::AsyncWriteExt; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::UnixStream; -use tokio::sync::{Notify, mpsc, watch}; +use tokio::sync::{Notify, watch}; use tokio::time::sleep; use wiremock::matchers::{method, path}; -use wiremock::{Mock, ResponseTemplate}; -use yaque::{Receiver, channel}; +use wiremock::{Mock, MockServer, ResponseTemplate}; use util::{TestComplexity, TimeoutConfig, join_err, timeout_with_retries}; @@ -43,15 +42,65 @@ fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { Config { github_token: cfg.github_token, + github_token_file: None, socket_path: cfg.socket_path, queue_path: cfg.queue_path, cooldown_period_seconds: cfg.cooldown_period_seconds, + cooldown_flutter_seconds: 0, restart_min_delay_ms: cfg.restart_min_delay_ms, github_api_timeout_secs: cfg.github_api_timeout_secs, client_channel_capacity: cfg.client_channel_capacity, } } +fn sample_request() -> comenq_lib::CommentRequest { + comenq_lib::CommentRequest { + owner: "o".into(), + repo: "r".into(), + pr_number: 1, + body: "b".into(), + } +} + +/// Enqueue the sample request and return its reported entry. +async fn seed_queue(queue: &Arc) -> PendingEntry { + match queue + .execute(Request::Put { + request: sample_request(), + }) + .await + { + Response::Ok { + entry: Some(entry), .. + } => entry, + other => panic!("expected put to succeed, got {other:?}"), + } +} + +/// Pending entries as reported by the daemon. +async fn list_entries(queue: &Arc) -> Vec { + match queue.execute(Request::List).await { + Response::Ok { + entries: Some(entries), + .. + } => entries, + other => panic!("expected list to succeed, got {other:?}"), + } +} + +/// Write `request` over `stream`, close the write side, and parse the reply. +async fn roundtrip(mut stream: UnixStream, request: &Request) -> Response { + let payload = serde_json::to_vec(request).expect("serialize request"); + stream.write_all(&payload).await.expect("write request"); + stream + .shutdown() + .await + .expect("close write side of connection"); + let mut reply = Vec::new(); + stream.read_to_end(&mut reply).await.expect("read reply"); + serde_json::from_slice(&reply).expect("parse reply") +} + async fn wait_for_file(path: &Path, tries: u32, delay: Duration) -> bool { for _ in 0..tries { if path.exists() { @@ -97,54 +146,69 @@ async fn prepare_listener_sets_permissions() { #[tokio::test] async fn handle_client_enqueues_request() { let dir = tempdir().expect("tempdir"); - let queue_path = dir.path().join("q"); - let (sender, mut receiver) = channel(&queue_path).expect("channel"); - let (client_tx, writer_rx) = mpsc::channel(4); - let writer = tokio::spawn(queue_writer(sender, writer_rx)); + let cfg = Arc::new(cfg_from(temp_config(&dir))); + let queue = SharedQueue::open(cfg).expect("open queue"); + + let (client, server) = UnixStream::pair().expect("pair"); + let handle = tokio::spawn(handle_client(server, queue.clone())); + let response = roundtrip( + client, + &Request::Put { + request: sample_request(), + }, + ) + .await; + handle.await.expect("join").expect("client"); + let Response::Ok { + entry: Some(entry), .. + } = response + else { + panic!("expected put reply with entry, got {response:?}"); + }; + assert_eq!(entry.id.len(), 8); + let entries = list_entries(&queue).await; + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].id, entry.id); +} + +#[tokio::test] +async fn handle_client_rejects_invalid_request() { + let dir = tempdir().expect("tempdir"); + let cfg = Arc::new(cfg_from(temp_config(&dir))); + let queue = SharedQueue::open(cfg).expect("open queue"); let (mut client, server) = UnixStream::pair().expect("pair"); - let handle = tokio::spawn(handle_client(server, client_tx)); - let req = comenq_lib::CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - let payload = serde_json::to_vec(&req).expect("serialize"); - client.write_all(&payload).await.expect("write"); + let handle = tokio::spawn(handle_client(server, queue.clone())); + client.write_all(b"not json").await.expect("write"); client.shutdown().await.expect("shutdown"); + let mut reply = Vec::new(); + client.read_to_end(&mut reply).await.expect("read reply"); handle.await.expect("join").expect("client"); - drop(writer); // stop queue writer - let guard = receiver.recv().await.expect("recv"); - let stored: comenq_lib::CommentRequest = serde_json::from_slice(&guard).expect("parse"); - assert_eq!(stored, req); + let response: Response = serde_json::from_slice(&reply).expect("parse reply"); + assert!(matches!(response, Response::Error { .. })); + assert!(list_entries(&queue).await.is_empty()); } #[tokio::test] async fn run_listener_accepts_connections() -> Result<(), String> { let dir = tempdir().expect("tempdir"); let cfg = Arc::new(cfg_from(temp_config(&dir).with_cooldown(1))); - let (sender, mut receiver) = channel(&cfg.queue_path).expect("channel"); - let (client_tx, writer_rx) = mpsc::channel(4); + let queue = SharedQueue::open(cfg.clone()).expect("open queue"); let (shutdown_tx, shutdown_rx) = watch::channel(()); - let writer_handle = tokio::spawn(queue_writer(sender, writer_rx)); - let listener_task = tokio::spawn(run_listener(cfg.clone(), client_tx, shutdown_rx)); + let listener_task = tokio::spawn(run_listener(queue.clone(), shutdown_rx)); wait_for_file(&cfg.socket_path, 10, Duration::from_millis(10)).await; - let mut stream = UnixStream::connect(&cfg.socket_path) + let stream = UnixStream::connect(&cfg.socket_path) .await .expect("connect"); - let req = comenq_lib::CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - let payload = serde_json::to_vec(&req).expect("serialize"); - stream.write_all(&payload).await.expect("write"); - stream.shutdown().await.expect("shutdown"); - let guard = receiver.recv().await.expect("recv"); - let stored: comenq_lib::CommentRequest = serde_json::from_slice(&guard).expect("parse"); - assert_eq!(stored, req); + let response = roundtrip( + stream, + &Request::Put { + request: sample_request(), + }, + ) + .await; + assert!(matches!(response, Response::Ok { .. })); + assert_eq!(list_entries(&queue).await.len(), 1); let _ = shutdown_tx.send(()); let timeout = TimeoutConfig::new(10, TestComplexity::Moderate) .with_ci(false) @@ -175,10 +239,6 @@ async fn run_listener_accepts_connections() -> Result<(), String> { } Err(e) => return Err(join_err("listener", e)), } - match writer_handle.await { - Ok(_) => {} - Err(e) => return Err(join_err("writer", e)), - } Ok(()) } @@ -188,13 +248,11 @@ mod worker_tests { const DRAINED_NOTIFICATION: TimeoutConfig = TimeoutConfig::new(15, TestComplexity::Moderate); const WORKER_SUCCESS: TimeoutConfig = TimeoutConfig::new(10, TestComplexity::Moderate); const WORKER_ERROR: TimeoutConfig = TimeoutConfig::new(15, TestComplexity::Complex); - use wiremock::MockServer; struct WorkerTestContext { server: MockServer, - cfg: Arc, - rx: Receiver, - octo: Arc, + queue: Arc, + octo: Arc, _dir: TempDir, } @@ -204,17 +262,8 @@ mod worker_tests { let cfg = Arc::new(cfg_from( temp_config(&dir).with_cooldown(TEST_COOLDOWN_SECONDS), )); - let (mut sender, rx) = channel(&cfg.queue_path).expect("channel"); - let req = comenq_lib::CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - sender - .send(serde_json::to_vec(&req).expect("serialize")) - .await - .expect("send"); + let queue = SharedQueue::open(cfg).expect("open queue"); + seed_queue(&queue).await; let server = MockServer::start().await; // Build response body based on status - success returns Comment, error returns GitHub error let response_body: serde_json::Value = if (200..300).contains(&status) { @@ -233,59 +282,20 @@ mod worker_tests { let octo = octocrab_for(&server).expect("octocrab client should build for the mock server"); WorkerTestContext { server, - cfg, - rx, + queue, octo, _dir: dir, } } - async fn diagnose_queue_state( - cfg: &Config, - server: &MockServer, - expected_files: usize, - ) -> String { - let queue_files = stdfs::read_dir(&cfg.queue_path) - .map(|entries| entries.count()) - .unwrap_or(0); - let server_requests = server.received_requests().await.unwrap_or_default().len(); - let mut output = - format!("Queue directory contains {queue_files} files (expected {expected_files})\n"); - output.push_str(&format!( - "Mock server received {server_requests} requests\n" - )); - if let Ok(entries) = stdfs::read_dir(&cfg.queue_path) { - output.push_str("Remaining queue files:\n"); - for (i, entry) in entries.enumerate() { - if let Ok(entry) = entry { - let name = entry.file_name(); - let file_num = i + 1; - output.push_str(&format!(" {file_num}. {}\n", name.to_string_lossy())); - if let Ok(metadata) = entry.metadata() { - let size = metadata.len(); - output.push_str(&format!(" Size: {size} bytes\n")); - if let Ok(modified) = metadata.modified() - && let Ok(elapsed) = modified.elapsed() - { - let age = elapsed.as_secs_f32(); - output.push_str(&format!(" Age: {age:.1}s ago\n")); - } - } - } - } - } - output - } - #[rstest] #[tokio::test] - async fn run_worker_commits_on_success( + async fn run_worker_completes_entry_on_success( #[future] #[from(worker_test_context)] ctx: WorkerTestContext, ) { let ctx = ctx.await; - let server = Arc::new(ctx.server); let idle = Arc::new(Notify::new()); let (shutdown_tx, shutdown_rx) = watch::channel(()); let control = WorkerControl { @@ -296,27 +306,20 @@ mod worker_tests { drained: None, }, }; - let h = tokio::spawn(run_worker(ctx.cfg.clone(), ctx.rx, ctx.octo, control)); - - // Wait for idle notification which fires after processing completes - if let Err(e) = - timeout_with_retries(DRAINED_NOTIFICATION, "worker idle notification", || { - let idle = idle.clone(); - async move { - idle.notified().await; - Ok(()) - } - }) - .await - { - let diagnostics = diagnose_queue_state(&ctx.cfg, &server, 0).await; - tracing::error!("Timeout waiting for worker idle notification: {e}"); - tracing::error!("{diagnostics}"); - panic!("worker idle: PROCESSING FAILURE"); - } + let h = tokio::spawn(run_worker(ctx.queue.clone(), ctx.octo, control)); + + timeout_with_retries(DRAINED_NOTIFICATION, "worker idle notification", || { + let idle = idle.clone(); + async move { + idle.notified().await; + Ok(()) + } + }) + .await + .expect("worker processed the entry"); shutdown_tx.send(()).expect("send shutdown"); let mut join_handle = Some(h); - if let Err(e) = timeout_with_retries(WORKER_SUCCESS, "worker join", || { + timeout_with_retries(WORKER_SUCCESS, "worker join", || { let handle = join_handle.take(); async move { let handle = handle.ok_or_else(|| "join handle consumed".to_string())?; @@ -328,31 +331,31 @@ mod worker_tests { } }) .await - { - let diagnostics = diagnose_queue_state(&ctx.cfg, &server, 0).await; - tracing::error!("\u{274C} Worker join timeout: {e}"); - tracing::error!("{diagnostics}"); - panic!("join worker: timeout in success test"); - } - // Verify exactly one request was made - this proves the item was processed - // and committed (otherwise the worker would retry and make multiple requests) - assert_eq!(server.received_requests().await.expect("requests").len(), 1); - // Note: We don't assert on queue data files because yaque cleans up segment - // files lazily during the next recv() call. Since the worker exited after - // shutdown, the segment file may still exist on disk even though the item - // was logically committed. + .expect("worker exited cleanly"); + // Exactly one request proves the entry was posted once and removed. + assert_eq!( + ctx.server + .received_requests() + .await + .expect("requests") + .len(), + 1 + ); + assert!( + list_entries(&ctx.queue).await.is_empty(), + "posted entry should leave the queue" + ); } #[rstest] #[tokio::test] - async fn run_worker_requeues_on_error( + async fn run_worker_retains_entry_on_error( #[future] #[with(500)] #[from(worker_test_context)] ctx: WorkerTestContext, ) { let ctx = ctx.await; - let server = Arc::new(ctx.server); let (shutdown_tx, shutdown_rx) = watch::channel(()); let enqueued = Arc::new(Notify::new()); let control = WorkerControl { @@ -363,7 +366,7 @@ mod worker_tests { drained: None, }, }; - let h = tokio::spawn(run_worker(ctx.cfg.clone(), ctx.rx, ctx.octo, control)); + let h = tokio::spawn(run_worker(ctx.queue.clone(), ctx.octo, control)); timeout_with_retries(WORKER_SUCCESS, "worker enqueued", || { let enqueued = enqueued.clone(); @@ -376,7 +379,7 @@ mod worker_tests { .expect("worker picked up job"); shutdown_tx.send(()).expect("send shutdown"); let mut join_handle = Some(h); - if let Err(e) = timeout_with_retries(WORKER_ERROR, "worker join", || { + timeout_with_retries(WORKER_ERROR, "worker join", || { let handle = join_handle.take(); async move { let handle = handle.ok_or_else(|| "join handle consumed".to_string())?; @@ -388,28 +391,20 @@ mod worker_tests { } }) .await - { - let diagnostics = diagnose_queue_state(&ctx.cfg, &server, 1).await; - tracing::error!("\u{274C} Worker join timeout: {e}"); - tracing::error!("{diagnostics}"); - panic!("join worker: timeout in error test"); - } + .expect("worker exited cleanly"); // At least one request should have been made (proving worker attempted processing) assert!( - !server + !ctx.server .received_requests() .await .expect("requests") .is_empty(), "worker should attempt the request at least once", ); - // Queue should still have data files (job was NOT committed due to error) - assert!( - stdfs::read_dir(&ctx.cfg.queue_path) - .expect("read queue directory") - .count() - > 0, - "Queue should retain job after API failure", + assert_eq!( + list_entries(&ctx.queue).await.len(), + 1, + "queue should retain the entry after an API failure", ); } @@ -419,8 +414,7 @@ mod worker_tests { async fn worker_terminates_on_shutdown_while_idle() { let dir = tempdir().expect("tempdir"); let cfg = Arc::new(cfg_from(temp_config(&dir).with_cooldown(60))); - // Create empty queue - worker will block on recv() - let (_sender, rx) = channel(&cfg.queue_path).expect("channel"); + let queue = SharedQueue::open(cfg).expect("open queue"); let server = MockServer::start().await; let octo = octocrab_for(&server).expect("octocrab client should build for the mock server"); @@ -430,9 +424,9 @@ mod worker_tests { hooks: WorkerHooks::default(), }; - let h = tokio::spawn(run_worker(cfg.clone(), rx, octo, control)); + let h = tokio::spawn(run_worker(queue, octo, control)); - // Give worker time to enter recv() wait + // Give worker time to park on the change signal sleep(Duration::from_millis(50)).await; // Signal shutdown @@ -448,26 +442,26 @@ mod worker_tests { } } - /// Tests that shutdown during cooldown (between processing iterations) - /// causes immediate termination. + /// Tests that shutdown during a cooldown wait (between entries) causes + /// immediate termination. #[tokio::test] async fn worker_terminates_on_shutdown_during_cooldown() { let dir = tempdir().expect("tempdir"); - // Long cooldown to ensure we're testing shutdown during wait + // Long cooldown to ensure we're testing shutdown during the wait let cfg = Arc::new(cfg_from(temp_config(&dir).with_cooldown(60))); - let (mut sender, rx) = channel(&cfg.queue_path).expect("channel"); - - // Enqueue a valid request - let req = comenq_lib::CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - sender - .send(serde_json::to_vec(&req).expect("serialize")) - .await - .expect("send"); + let queue = SharedQueue::open(cfg).expect("open queue"); + // Two entries: the second is due one full cooldown after the first. + seed_queue(&queue).await; + queue + .execute(Request::Put { + request: comenq_lib::CommentRequest { + owner: "o".into(), + repo: "r".into(), + pr_number: 1, + body: "second".into(), + }, + }) + .await; let server = MockServer::start().await; let response_body: serde_json::Value = @@ -492,9 +486,10 @@ mod worker_tests { }, }; - let h = tokio::spawn(run_worker(cfg.clone(), rx, octo, control)); + let h = tokio::spawn(run_worker(queue, octo, control)); - // Wait for idle notification (worker finished processing, now in cooldown) + // Wait for idle notification (first entry posted; worker now waits a + // full cooldown before the second). let wait_timeout = Duration::from_secs(10); if tokio::time::timeout(wait_timeout, idle.notified()) .await @@ -503,7 +498,7 @@ mod worker_tests { panic!("worker did not reach idle state within {wait_timeout:?}"); } - // Worker is now in cooldown (sleeping for 60 seconds) + // Worker is now waiting out the 60 second cooldown. // Signal shutdown - it should terminate immediately shutdown_tx.send(()).expect("send shutdown"); @@ -517,72 +512,4 @@ mod worker_tests { } } } - - /// Tests that shutdown during cooldown after a malformed message - /// causes immediate termination. - #[tokio::test] - async fn worker_terminates_on_shutdown_during_cooldown_after_malformed() { - let dir = tempdir().expect("tempdir"); - // Long cooldown to ensure we're testing shutdown during wait - let cfg = Arc::new(cfg_from(temp_config(&dir).with_cooldown(60))); - let (mut sender, rx) = channel(&cfg.queue_path).expect("channel"); - - // Enqueue malformed data (not valid JSON) - sender - .send(b"this is not valid json".to_vec()) - .await - .expect("send"); - - let server = MockServer::start().await; - // No mock needed - malformed message won't reach GitHub API - let octo = octocrab_for(&server).expect("octocrab client should build for the mock server"); - - let (shutdown_tx, shutdown_rx) = watch::channel(()); - let idle = Arc::new(Notify::new()); - let control = WorkerControl { - shutdown: shutdown_rx, - hooks: WorkerHooks { - enqueued: None, - idle: Some(idle.clone()), - drained: None, - }, - }; - - let h = tokio::spawn(run_worker(cfg.clone(), rx, octo, control)); - - // Wait for idle notification (worker dropped malformed message, now in cooldown) - let wait_timeout = Duration::from_secs(10); - if tokio::time::timeout(wait_timeout, idle.notified()) - .await - .is_err() - { - panic!( - "worker did not reach idle state after malformed message within {wait_timeout:?}" - ); - } - - // Worker is now in cooldown after dropping the malformed message - // Signal shutdown - it should terminate immediately - shutdown_tx.send(()).expect("send shutdown"); - - let shutdown_timeout = Duration::from_secs(2); - match tokio::time::timeout(shutdown_timeout, h).await { - Ok(Ok(Ok(()))) => {} // Success - Ok(Ok(Err(e))) => panic!("worker returned error: {e}"), - Ok(Err(e)) => panic!("worker task panicked: {e}"), - Err(_) => panic!( - "worker did not terminate within {shutdown_timeout:?} during cooldown after malformed message" - ), - } - - // Verify no requests were made to GitHub (malformed message was dropped) - assert!( - server - .received_requests() - .await - .expect("requests") - .is_empty(), - "no GitHub API requests should be made for malformed messages" - ); - } } diff --git a/crates/comenqd/tests/listener.rs b/crates/comenqd/tests/listener.rs index 9d84039..2a203ac 100644 --- a/crates/comenqd/tests/listener.rs +++ b/crates/comenqd/tests/listener.rs @@ -1,21 +1,82 @@ //! Tests for listener behaviour. Freezes Tokio time, advances beyond the client -//! read timeout, and asserts idle connections time out. +//! read timeout, and asserts idle connections time out. Also exercises the +//! request size limit at and just past the boundary. +use std::sync::Arc; use std::time::Duration; -use tokio::io::AsyncWriteExt; +use tempfile::tempdir; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::UnixStream; -use tokio::sync::mpsc; use tokio::task::yield_now; use tokio::time::advance; use comenq_lib::CommentRequest; +use comenq_lib::protocol::{Request, Response}; +use comenqd::config::Config; +use comenqd::daemon::SharedQueue; use comenqd::daemon::listener::{CLIENT_READ_TIMEOUT_SECS, MAX_REQUEST_BYTES, handle_client}; +use test_support::temp_config; + +/// Convert a test configuration into the runtime `Config`. +/// +/// Mirrors the helper in `daemon.rs`: the `From` conversion needs the +/// `test-support` feature, which coverage builds disable. +#[cfg(feature = "test-support")] +fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { + Config::from(cfg) +} + +#[cfg(not(feature = "test-support"))] +fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { + Config { + github_token: cfg.github_token, + github_token_file: None, + socket_path: cfg.socket_path, + queue_path: cfg.queue_path, + cooldown_period_seconds: cfg.cooldown_period_seconds, + cooldown_flutter_seconds: 0, + restart_min_delay_ms: cfg.restart_min_delay_ms, + github_api_timeout_secs: cfg.github_api_timeout_secs, + client_channel_capacity: cfg.client_channel_capacity, + } +} + +fn open_queue(dir: &tempfile::TempDir) -> Arc { + let cfg: Arc = Arc::new(cfg_from(temp_config(dir))); + SharedQueue::open(cfg).expect("open queue") +} + +/// Build a `put` payload whose serialized size is exactly `target` bytes. +fn payload_of_size(target: usize) -> Vec { + let base = Request::Put { + request: CommentRequest { + owner: String::new(), + repo: String::new(), + pr_number: 0, + body: String::new(), + }, + }; + let base_len = serde_json::to_vec(&base).expect("serialize").len(); + let body_len = target - base_len; + let request = Request::Put { + request: CommentRequest { + owner: String::new(), + repo: String::new(), + pr_number: 0, + body: "a".repeat(body_len), + }, + }; + let payload = serde_json::to_vec(&request).expect("serialize"); + assert_eq!(payload.len(), target); + payload +} #[tokio::test(start_paused = true)] async fn handle_client_times_out_if_client_does_not_close() { - let (client_tx, _rx) = mpsc::channel(1); + let dir = tempdir().expect("tempdir"); + let queue = open_queue(&dir); let (client, server) = UnixStream::pair().expect("pair"); - let handle = tokio::spawn(handle_client(server, client_tx)); + let handle = tokio::spawn(handle_client(server, queue)); // Ensure the handler registers its timeout before advancing time. yield_now().await; advance(Duration::from_secs(CLIENT_READ_TIMEOUT_SECS + 1)).await; @@ -26,48 +87,33 @@ async fn handle_client_times_out_if_client_does_not_close() { #[tokio::test] async fn handle_client_accepts_exact_max_request_bytes() { - let (tx, mut rx) = mpsc::channel(1); + let dir = tempdir().expect("tempdir"); + let queue = open_queue(&dir); let (mut client, server) = UnixStream::pair().expect("pair"); - let handle = tokio::spawn(handle_client(server, tx)); - - let base = CommentRequest { - owner: String::new(), - repo: String::new(), - pr_number: 0, - body: String::new(), - }; - let base_len = serde_json::to_vec(&base).expect("serialize").len(); - let body_len = MAX_REQUEST_BYTES - base_len; - let mut request = base; - request.body = "a".repeat(body_len); - let payload = serde_json::to_vec(&request).expect("serialize"); + let handle = tokio::spawn(handle_client(server, queue)); + let payload = payload_of_size(MAX_REQUEST_BYTES); client.write_all(&payload).await.expect("write"); client.shutdown().await.expect("shutdown"); + let mut reply = Vec::new(); + client.read_to_end(&mut reply).await.expect("read reply"); handle.await.expect("join").expect("handle"); - let msg = rx.recv().await.expect("recv"); - assert_eq!(msg, payload); + let response: Response = serde_json::from_slice(&reply).expect("parse reply"); + assert!( + matches!(response, Response::Ok { .. }), + "boundary-sized request should be accepted: {response:?}" + ); } #[tokio::test] async fn handle_client_rejects_request_exceeding_limit() { - let (tx, _rx) = mpsc::channel(1); + let dir = tempdir().expect("tempdir"); + let queue = open_queue(&dir); let (mut client, server) = UnixStream::pair().expect("pair"); - let handle = tokio::spawn(handle_client(server, tx)); - - let base = CommentRequest { - owner: String::new(), - repo: String::new(), - pr_number: 0, - body: String::new(), - }; - let base_len = serde_json::to_vec(&base).expect("serialize").len(); - let body_len = MAX_REQUEST_BYTES - base_len + 1; - let mut request = base; - request.body = "a".repeat(body_len); - let payload = serde_json::to_vec(&request).expect("serialize"); + let handle = tokio::spawn(handle_client(server, queue)); + let payload = payload_of_size(MAX_REQUEST_BYTES + 1); client.write_all(&payload).await.expect("write"); client.shutdown().await.expect("shutdown"); diff --git a/tests/steps/listener_steps.rs b/tests/steps/listener_steps.rs index 7fe102c..ea7f2b2 100644 --- a/tests/steps/listener_steps.rs +++ b/tests/steps/listener_steps.rs @@ -1,30 +1,29 @@ //! Behavioural test steps for the listener task. use std::sync::Arc; -use std::time::Duration; use anyhow::Context as _; use cucumber::World; use cucumber::{given, then, when}; use tempfile::TempDir; use test_support::{SOCKET_RETRY_COUNT, SOCKET_RETRY_DELAY, temp_config, wait_for_file}; -use tokio::io::AsyncWriteExt; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::UnixStream; -use tokio::sync::{mpsc, watch}; +use tokio::sync::watch; use comenq_lib::CommentRequest; +use comenq_lib::protocol::{PendingEntry, Request, Response}; use comenqd::config::Config; -use comenqd::daemon::{listener::run_listener, queue_writer}; -use yaque::channel; +use comenqd::daemon::{SharedQueue, listener::run_listener}; #[derive(Default, World)] pub struct ListenerWorld { dir: Option, cfg: Option>, - receiver: Option, + queue: Option>, shutdown: Option>, - writer: Option>, handle: Option>, + response: Option, } impl std::fmt::Debug for ListenerWorld { @@ -33,29 +32,33 @@ impl std::fmt::Debug for ListenerWorld { } } +/// Pending entries as reported through the protocol. +async fn listed_entries(queue: &Arc) -> anyhow::Result> { + match queue.execute(Request::List).await { + Response::Ok { + entries: Some(entries), + .. + } => Ok(entries), + other => anyhow::bail!("expected list reply, got {other:?}"), + } +} + #[given("a running listener task")] async fn running_listener(world: &mut ListenerWorld) -> anyhow::Result<()> { let dir = TempDir::new().context("tempdir")?; let cfg = Arc::new(Config::from(temp_config(&dir))); - let (sender, receiver) = channel(&cfg.queue_path).context("channel")?; - let (client_tx, writer_rx) = mpsc::channel(4); + let queue = SharedQueue::open(cfg.clone()).context("open queue")?; let (shutdown_tx, shutdown_rx) = watch::channel(()); - let cfg_clone = cfg.clone(); - let writer = tokio::spawn(async move { - // Intentionally ignore the returned receiver: the writer remains - // alive for the test's duration and the channel is not reused. - let _ = queue_writer(sender, writer_rx).await; - }); + let listener_queue = queue.clone(); let handle = tokio::spawn(async move { - if let Err(error) = run_listener(cfg_clone, client_tx, shutdown_rx).await { + if let Err(error) = run_listener(listener_queue, shutdown_rx).await { panic!("listener task failed: {error}"); } }); world.dir = Some(dir); world.cfg = Some(cfg); + world.queue = Some(queue); world.shutdown = Some(shutdown_tx); - world.writer = Some(writer); - world.receiver = Some(receiver); world.handle = Some(handle); let socket_path = &world @@ -71,52 +74,67 @@ async fn running_listener(world: &mut ListenerWorld) -> anyhow::Result<()> { Ok(()) } -#[when("a client sends a valid request")] -async fn client_sends_valid(world: &mut ListenerWorld) -> anyhow::Result<()> { +/// Send raw `bytes` to the daemon socket and parse the JSON reply. +async fn send_raw(world: &mut ListenerWorld, bytes: &[u8]) -> anyhow::Result { let cfg = world.cfg.as_ref().context("config initialized")?; let mut stream = UnixStream::connect(&cfg.socket_path) .await .context("connect")?; - let req = CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - let data = serde_json::to_vec(&req).context("serialize request")?; - stream.write_all(&data).await.context("write request")?; + stream.write_all(bytes).await.context("write request")?; stream.shutdown().await.context("shutdown")?; + let mut reply = Vec::new(); + stream.read_to_end(&mut reply).await.context("read reply")?; + serde_json::from_slice(&reply).context("parse reply") +} + +#[when("a client sends a valid request")] +async fn client_sends_valid(world: &mut ListenerWorld) -> anyhow::Result<()> { + let request = Request::Put { + request: CommentRequest { + owner: "o".into(), + repo: "r".into(), + pr_number: 1, + body: "b".into(), + }, + }; + let data = serde_json::to_vec(&request).context("serialize request")?; + let response = send_raw(world, &data).await?; + world.response = Some(response); Ok(()) } #[when("a client sends invalid JSON")] async fn client_sends_invalid(world: &mut ListenerWorld) -> anyhow::Result<()> { - let cfg = world.cfg.as_ref().context("config initialized")?; - let mut stream = UnixStream::connect(&cfg.socket_path) - .await - .context("connect")?; - stream - .write_all(b"not json") - .await - .context("write request")?; - stream.shutdown().await.context("shutdown")?; + let response = send_raw(world, b"not json").await?; + world.response = Some(response); Ok(()) } #[then("the request is enqueued")] async fn request_enqueued(world: &mut ListenerWorld) -> anyhow::Result<()> { - let receiver = world.receiver.as_mut().context("receiver initialized")?; - let guard = receiver.recv().await.context("recv")?; - let req: CommentRequest = serde_json::from_slice(&guard).context("parse request")?; - assert_eq!(req.owner, "o"); + match world.response.take() { + Some(Response::Ok { + entry: Some(entry), .. + }) => { + assert_eq!(entry.owner, "o"); + assert_eq!(entry.id.len(), 8); + } + other => anyhow::bail!("expected put reply with entry, got {other:?}"), + } + let queue = world.queue.as_ref().context("queue initialized")?; + let entries = listed_entries(queue).await?; + assert_eq!(entries.len(), 1); Ok(()) } #[then("the request is rejected")] async fn request_rejected(world: &mut ListenerWorld) -> anyhow::Result<()> { - let receiver = world.receiver.as_mut().context("receiver initialized")?; - let res = tokio::time::timeout(Duration::from_millis(100), receiver.recv()).await; - assert!(res.is_err()); + match world.response.take() { + Some(Response::Error { .. }) => {} + other => anyhow::bail!("expected error reply, got {other:?}"), + } + let queue = world.queue.as_ref().context("queue initialized")?; + assert!(listed_entries(queue).await?.is_empty()); Ok(()) } @@ -125,9 +143,6 @@ impl Drop for ListenerWorld { if let Some(shutdown) = self.shutdown.take() { let _ = shutdown.send(()); } - if let Some(writer) = self.writer.take() { - writer.abort(); - } if let Some(handle) = self.handle.take() { handle.abort(); } diff --git a/tests/steps/worker_steps.rs b/tests/steps/worker_steps.rs index bdee024..c6cdfc8 100644 --- a/tests/steps/worker_steps.rs +++ b/tests/steps/worker_steps.rs @@ -3,16 +3,17 @@ //! These steps drive the Cucumber scenarios that verify the worker posts //! queued comments and handles failures gracefully. //! -//! Uses Wiremock to stub the GitHub Issues API and yaque for the on-disk queue. -//! See also: `test-support::octocrab_for()` and `yaque::channel()`. +//! Uses Wiremock to stub the GitHub Issues API and the daemon's shared queue +//! for on-disk persistence. use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; use comenq_lib::CommentRequest; +use comenq_lib::protocol::{PendingEntry, Request, Response}; use comenqd::config::Config; -use comenqd::daemon::{WorkerControl, WorkerHooks, is_metadata_file, run_worker}; +use comenqd::daemon::{SharedQueue, WorkerControl, WorkerHooks, run_worker}; use cucumber::{World, given, then, when}; use tempfile::TempDir; use test_support::{octocrab_for, temp_config}; @@ -20,7 +21,6 @@ use tokio::sync::{Notify, watch}; use tokio::time::timeout; use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; -use yaque::{self, channel}; fn coverage_timeout_multiplier() -> u32 { if std::env::var("CARGO_LLVM_COV_TARGET_DIR").is_ok() @@ -35,8 +35,7 @@ fn coverage_timeout_multiplier() -> u32 { #[derive(World, Default)] pub struct WorkerWorld { dir: Option, - cfg: Option>, - receiver: Option, + queue: Option>, server: Option, shutdown: Option>, handle: Option>, @@ -70,34 +69,57 @@ impl WorkerWorld { } } +/// Pending entries as reported through the protocol. +async fn listed_entries(queue: &Arc) -> anyhow::Result> { + match queue.execute(Request::List).await { + Response::Ok { + entries: Some(entries), + .. + } => Ok(entries), + other => anyhow::bail!("expected list reply, got {other:?}"), + } +} + #[given("a queued comment request")] async fn queued_request(world: &mut WorkerWorld) -> anyhow::Result<()> { let dir = TempDir::new().context("tempdir")?; let cfg = Arc::new(Config::from(temp_config(&dir).with_cooldown(0))); - let (mut sender, receiver) = channel(&cfg.queue_path).context("channel")?; - let req = CommentRequest { - owner: "o".into(), - repo: "r".into(), - pr_number: 1, - body: "b".into(), - }; - let data = serde_json::to_vec(&req).context("serialize")?; - sender.send(data).await.context("send")?; + let queue = SharedQueue::open(cfg).context("open queue")?; + let response = queue + .execute(Request::Put { + request: CommentRequest { + owner: "o".into(), + repo: "r".into(), + pr_number: 1, + body: "b".into(), + }, + }) + .await; + anyhow::ensure!( + matches!(response, Response::Ok { .. }), + "put should succeed, got {response:?}" + ); world.dir = Some(dir); - world.cfg = Some(cfg); - world.receiver = Some(receiver); + world.queue = Some(queue); Ok(()) } #[given("GitHub returns success")] -async fn github_success(world: &mut WorkerWorld) { +async fn github_success(world: &mut WorkerWorld) -> anyhow::Result<()> { let server = MockServer::start().await; + // A structurally valid comment body: an empty object fails octocrab's + // response parsing and would masquerade as an API error. + let body: serde_json::Value = serde_json::from_str(include_str!( + "../../crates/comenqd/tests/fixtures/github_comment_response.json" + )) + .context("parse comment fixture")?; Mock::given(method("POST")) .and(path("/repos/o/r/issues/1/comments")) - .respond_with(ResponseTemplate::new(201).set_body_raw("{}", "application/json")) + .respond_with(ResponseTemplate::new(201).set_body_json(body)) .mount(&server) .await; world.server = Some(server); + Ok(()) } #[given("GitHub returns an error")] @@ -113,15 +135,11 @@ async fn github_error(world: &mut WorkerWorld) { #[when("the worker runs briefly")] async fn worker_runs(world: &mut WorkerWorld) -> anyhow::Result<()> { - let cfg = world - .cfg + let queue = world + .queue .as_ref() - .context("configuration should be initialized")? + .context("queue should be initialized")? .clone(); - let rx = world - .receiver - .take() - .context("receiver should be initialized")?; let server = world .server .as_ref() @@ -141,7 +159,7 @@ async fn worker_runs(world: &mut WorkerWorld) -> anyhow::Result<()> { }, ); let handle = tokio::spawn(async move { - let _ = run_worker(cfg, rx, octocrab, control).await; + let _ = run_worker(queue, octocrab, control).await; }); timeout( Duration::from_secs(30 * u64::from(coverage_timeout_multiplier())), @@ -169,22 +187,23 @@ async fn comment_posted(world: &mut WorkerWorld) -> anyhow::Result<()> { .context("inbound requests should be recorded")? .is_empty(), ); + let queue = world.queue.as_ref().context("queue initialized")?.clone(); + assert!( + listed_entries(&queue).await?.is_empty(), + "posted entry should leave the queue" + ); world.shutdown_and_join().await; Ok(()) } #[then("the queue retains the job")] async fn queue_retains(world: &mut WorkerWorld) -> anyhow::Result<()> { - let cfg = world - .cfg - .as_ref() - .context("configuration should be initialized")?; - let job_count = std::fs::read_dir(&cfg.queue_path) - .context("queue directory should be readable")? - .filter_map(Result::ok) - .filter(|e| !is_metadata_file(e.file_name())) - .count(); - assert!(job_count > 0, "queue should retain at least one job file"); + let queue = world.queue.as_ref().context("queue initialized")?.clone(); + assert_eq!( + listed_entries(&queue).await?.len(), + 1, + "queue should retain the entry after an API failure" + ); world.shutdown_and_join().await; Ok(()) } From 71d5a4ad8139637306db16154c227079652090e2 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 17:45:06 +0200 Subject: [PATCH 4/6] Split the comenq client into queue subcommands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single-purpose argument list with five subcommands: - `put owner/repo ` enqueues a comment and prints its deterministic identifier with the daemon-reported approximate ETA (`Queued 1a2b3c4d for owner/repo#7 — posts in ~1h 01m`). - `list` prints one line per pending comment: identifier, ETA, target pull request, and the body collapsed to a single line and truncated to sixty characters with an ellipsis. - `bump `, `bust `, and `del ` move an entry to the head or tail of the queue, or remove it. The `--socket` flag becomes global (still discoverable via `$XDG_RUNTIME_DIR` and overridable with `COMENQ_SOCKET`). The client now completes a full transaction: it writes one protocol request, half-closes, reads the JSON reply, and surfaces daemon-reported errors distinctly from transport failures. Rendering lives in a new `output` module with unit-tested ETA and summary formatting. --- crates/comenq/src/client.rs | 210 ++++++++++++++++++++----------- crates/comenq/src/lib.rs | 132 +++++++++++++++---- crates/comenq/src/output.rs | 167 ++++++++++++++++++++++++ tests/steps/cli_steps.rs | 2 + tests/steps/client_main_steps.rs | 30 +++-- 5 files changed, 440 insertions(+), 101 deletions(-) create mode 100644 crates/comenq/src/output.rs diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index db5504e..6cf0987 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -1,16 +1,20 @@ //! Client-side communication with the `comenqd` daemon. //! -//! This module contains the logic to serialize a comment request and send it to -//! the daemon over its Unix Domain Socket. It is separated from `lib.rs` so -//! that argument parsing remains focused and the network logic is easily -//! testable. +//! This module serializes a protocol request, sends it to the daemon over +//! its Unix Domain Socket, and renders the reply. It is separated from +//! `lib.rs` so that argument parsing remains focused and the network logic +//! is easily testable. -use comenq_lib::CommentRequest; +use comenq_lib::protocol::{Request, Response}; +use std::path::Path; use thiserror::Error; -use tokio::{io::AsyncWriteExt, net::UnixStream}; -use tracing::warn; +use tokio::{ + io::{AsyncReadExt, AsyncWriteExt}, + net::UnixStream, +}; -use crate::Args; +use crate::output::{render_entry, render_put}; +use crate::{Args, Command}; /// Errors that can occur when interacting with the daemon. #[derive(Debug, Error)] @@ -18,8 +22,8 @@ pub enum ClientError { /// Connecting to the daemon failed. #[error("failed to connect to daemon: {0}")] Connect(#[from] std::io::Error), - /// Serializing the request failed. - #[error("failed to serialize request: {0}")] + /// Serializing the request or parsing the reply failed. + #[error("failed to encode or decode a daemon message: {0}")] Serialize(#[from] serde_json::Error), /// Writing the request to the socket failed. #[error("failed to write to daemon: {0}")] @@ -27,22 +31,47 @@ pub enum ClientError { /// Shutting down the socket failed. #[error("failed to close connection: {0}")] Shutdown(#[source] std::io::Error), + /// Reading the daemon's reply failed. + #[error("failed to read daemon reply: {0}")] + Read(#[source] std::io::Error), + /// The daemon reported a failure. + #[error("daemon refused the request: {0}")] + Daemon(String), + /// The daemon's reply did not match the request. + #[error("unexpected reply from daemon")] + UnexpectedResponse, } -/// Send a `CommentRequest` to the daemon. +/// Send `request` to the daemon at `socket` and parse the reply. +async fn transact(socket: &Path, request: &Request) -> Result { + let payload = serde_json::to_vec(request)?; + let mut stream = UnixStream::connect(socket) + .await + .map_err(ClientError::Connect)?; + stream + .write_all(&payload) + .await + .map_err(ClientError::Write)?; + stream.shutdown().await.map_err(ClientError::Shutdown)?; + let mut reply = Vec::new(); + stream + .read_to_end(&mut reply) + .await + .map_err(ClientError::Read)?; + Ok(serde_json::from_slice(&reply)?) +} + +/// Execute the parsed command against the daemon and print the outcome. /// /// # Examples /// /// ```no_run -/// # use comenq::{Args, run}; +/// # use comenq::{Args, Command, run}; /// # use std::path::PathBuf; -/// # use comenq_lib::DEFAULT_SOCKET_PATH; /// # async fn try_run() -> Result<(), comenq::ClientError> { /// let args = Args { -/// repo_slug: "owner/repo".parse().expect("slug"), -/// pr_number: 1, -/// comment_body: String::from("Hi"), -/// socket: Some(PathBuf::from(DEFAULT_SOCKET_PATH)), +/// socket: Some(PathBuf::from("/tmp/comenq.sock")), +/// command: Command::List, /// }; /// run(args).await?; /// # Ok(()) @@ -50,64 +79,117 @@ pub enum ClientError { /// ``` 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(), - pr_number: args.pr_number, - body: args.comment_body, + let request = args.command.to_request(); + let response = transact(&socket, &request).await?; + let (entry, entries) = match response { + Response::Error { message } => return Err(ClientError::Daemon(message)), + Response::Ok { entry, entries } => (entry, entries), }; - - let payload = serde_json::to_vec(&request)?; - - let mut stream = UnixStream::connect(socket) - .await - .map_err(ClientError::Connect)?; - stream - .write_all(&payload) - .await - .map_err(ClientError::Write)?; - if let Err(e) = stream.shutdown().await { - warn!("failed to close connection: {e}"); - return Err(ClientError::Shutdown(e)); + match &args.command { + Command::Put { .. } => { + let entry = entry.ok_or(ClientError::UnexpectedResponse)?; + println!("{}", render_put(&entry)); + } + Command::List => { + let entries = entries.ok_or(ClientError::UnexpectedResponse)?; + if entries.is_empty() { + println!("No comments queued."); + } else { + for entry in &entries { + println!("{}", render_entry(entry)); + } + } + } + Command::Bump { id } => println!("Moved {id} to the head of the queue."), + Command::Bust { id } => println!("Moved {id} to the tail of the queue."), + Command::Del { id } => println!("Removed {id} from the queue."), } Ok(()) } #[cfg(test)] mod tests { + //! Round-trip tests for the client transport. use super::{ClientError, run}; - use crate::{Args, RepoSlug}; - use comenq_lib::CommentRequest; + use crate::{Args, Command}; + use comenq_lib::protocol::{PendingEntry, Request, Response}; use tempfile::tempdir; - use tokio::io::AsyncReadExt; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::UnixListener; - #[tokio::test] - async fn run_sends_request() { - let dir = tempdir().expect("temp dir"); - let socket = dir.path().join("sock"); - let listener = UnixListener::bind(&socket).expect("bind socket"); + fn put_args(socket: std::path::PathBuf) -> Args { + Args { + socket: Some(socket), + command: Command::Put { + repo_slug: "octocat/hello-world".parse().expect("slug"), + pr_number: 1, + comment_body: "Hi".into(), + }, + } + } - let accept = tokio::spawn(async move { + /// Accept one connection, capture the request, and reply. + fn spawn_daemon(listener: UnixListener, reply: Response) -> tokio::task::JoinHandle { + tokio::spawn(async move { let (mut stream, _) = listener.accept().await.expect("accept"); let mut buf = Vec::new(); stream.read_to_end(&mut buf).await.expect("read"); - serde_json::from_slice::(&buf).expect("deserialize") - }); + let request = serde_json::from_slice::(&buf).expect("deserialize"); + let bytes = serde_json::to_vec(&reply).expect("serialize reply"); + stream.write_all(&bytes).await.expect("write reply"); + request + }) + } - let args = Args { - repo_slug: "octocat/hello-world".parse().expect("slug"), + #[tokio::test] + async fn run_sends_put_request_and_accepts_reply() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind socket"); + let reply = Response::entry(PendingEntry { + id: "1a2b3c4d".into(), + eta_seconds: 0, + owner: "octocat".into(), + repo: "hello-world".into(), pr_number: 1, - comment_body: "Hi".into(), - socket: Some(socket.clone()), + body: "Hi".into(), + }); + let accept = spawn_daemon(listener, reply); + + run(put_args(socket)).await.expect("run succeeds"); + let request = accept.await.expect("join"); + let Request::Put { request } = request else { + panic!("expected put request, got {request:?}"); }; + assert_eq!(request.owner, "octocat"); + assert_eq!(request.repo, "hello-world"); + assert_eq!(request.pr_number, 1); + assert_eq!(request.body, "Hi"); + } + + #[tokio::test] + async fn run_surfaces_daemon_errors() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind socket"); + let accept = spawn_daemon(listener, Response::error("queue unavailable")); - run(args).await.expect("run succeeds"); - let req = accept.await.expect("join"); - assert_eq!(req.owner, "octocat"); - assert_eq!(req.repo, "hello-world"); - assert_eq!(req.pr_number, 1); - assert_eq!(req.body, "Hi"); + let err = run(put_args(socket)).await.expect_err("should error"); + assert!(matches!(err, ClientError::Daemon(m) if m == "queue unavailable")); + accept.await.expect("join"); + } + + #[tokio::test] + async fn run_rejects_mismatched_reply() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind socket"); + // A bare Ok reply lacks the entry a put expects. + let accept = spawn_daemon(listener, Response::ok()); + + let err = run(put_args(socket)).await.expect_err("should error"); + assert!(matches!(err, ClientError::UnexpectedResponse)); + accept.await.expect("join"); } #[tokio::test] @@ -115,21 +197,7 @@ mod tests { let dir = tempdir().expect("temp dir"); let socket = dir.path().join("nosock"); - let args = Args { - repo_slug: "octocat/hello-world".parse().expect("slug"), - pr_number: 1, - comment_body: "Hi".into(), - socket: Some(socket.clone()), - }; - - let err = run(args).await.expect_err("should error"); + let err = run(put_args(socket)).await.expect_err("should error"); assert!(matches!(err, ClientError::Connect(_))); } - - #[test] - fn slug_parses() { - let slug: RepoSlug = "octocat/hello-world".parse().expect("slug"); - assert_eq!(slug.owner(), "octocat"); - assert_eq!(slug.repo(), "hello-world"); - } } diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 588e3ad..69d0568 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -1,12 +1,14 @@ //! Library utilities for the `comenq` CLI. -use clap::{Parser, builder::ValueHint}; +use clap::{Parser, Subcommand, builder::ValueHint}; use std::{fmt, path::PathBuf, str::FromStr}; use thiserror::Error; mod client; +mod output; pub use client::{ClientError, run}; +pub use output::{format_eta, one_line_summary}; /// A GitHub repository slug in `owner/repo` format. #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -92,17 +94,8 @@ impl fmt::Display for RepoSlug { /// Command line arguments for the `comenq` client. #[derive(Debug, Clone, Parser)] -#[command(name = "comenq", about = "Enqueue a GitHub PR comment")] +#[command(name = "comenq", about = "Queue and manage GitHub PR comments")] pub struct Args { - /// The repository in 'owner/repo' format (e.g., "rust-lang/rust"). - pub repo_slug: RepoSlug, - - /// The pull request number to comment on. - pub pr_number: u64, - - /// The body of the comment. It is recommended to quote this argument. - pub comment_body: String, - /// Path to the daemon's Unix Domain Socket. /// /// When omitted, the client discovers the socket: the first existing @@ -113,8 +106,71 @@ pub struct Args { // 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")] + #[arg(long, global = true, value_hint = ValueHint::FilePath, env = "COMENQ_SOCKET")] pub socket: Option, + + /// Queue operation to perform. + #[command(subcommand)] + pub command: Command, +} + +/// Queue operations offered by the client. +#[derive(Debug, Clone, Subcommand)] +pub enum Command { + /// Enqueue a comment and print its identifier and approximate ETA. + Put { + /// The repository in 'owner/repo' format (e.g., "rust-lang/rust"). + repo_slug: RepoSlug, + + /// The pull request number to comment on. + pr_number: u64, + + /// The body of the comment. It is recommended to quote this argument. + comment_body: String, + }, + /// List pending comments with identifiers and ETAs. + List, + /// Move the identified comment to the head of the queue. + Bump { + /// Identifier printed by `put` and `list`. + id: String, + }, + /// Move the identified comment to the tail of the queue. + Bust { + /// Identifier printed by `put` and `list`. + id: String, + }, + /// Remove the identified comment from the queue. + Del { + /// Identifier printed by `put` and `list`. + id: String, + }, +} + +impl Command { + /// The protocol request this command performs. + #[must_use] + pub fn to_request(&self) -> comenq_lib::protocol::Request { + use comenq_lib::protocol::Request; + match self { + Self::Put { + repo_slug, + pr_number, + comment_body, + } => Request::Put { + request: comenq_lib::CommentRequest { + owner: repo_slug.owner().to_owned(), + repo: repo_slug.repo().to_owned(), + pr_number: *pr_number, + body: comment_body.clone(), + }, + }, + Self::List => Request::List, + Self::Bump { id } => Request::Bump { id: id.clone() }, + Self::Bust { id } => Request::Bust { id: id.clone() }, + Self::Del { id } => Request::Del { id: id.clone() }, + } + } } impl Args { @@ -129,6 +185,7 @@ impl Args { /// /// let args = Args::try_parse_from([ /// "comenq", + /// "put", /// "octocat/hello-world", /// "1", /// "Hi", @@ -148,7 +205,7 @@ impl Args { #[cfg(test)] mod tests { - use super::{Args, RepoSlug, RepoSlugParseError}; + use super::{Args, Command, RepoSlug, RepoSlugParseError}; use clap::Parser; use rstest::rstest; use std::path::PathBuf; @@ -156,14 +213,43 @@ mod tests { #[rstest] #[case("octocat/hello-world", 1, "Hi")] - fn parses_valid_arguments(#[case] slug: &str, #[case] pr: u64, #[case] body: &str) { + fn parses_valid_put_arguments(#[case] slug: &str, #[case] pr: u64, #[case] body: &str) { let pr_str = pr.to_string(); - let args = Args::try_parse_from(["comenq", slug, &pr_str, body]); + let args = Args::try_parse_from(["comenq", "put", slug, &pr_str, body]); let args = args.expect("valid arguments should parse"); let expected: RepoSlug = slug.parse().expect("slug parses"); - assert_eq!(args.repo_slug, expected); - assert_eq!(args.pr_number, pr); - assert_eq!(args.comment_body, body); + let Command::Put { + repo_slug, + pr_number, + comment_body, + } = args.command + else { + panic!("expected put command"); + }; + assert_eq!(repo_slug, expected); + assert_eq!(pr_number, pr); + assert_eq!(comment_body, body); + } + + #[rstest] + #[case::list(&["comenq", "list"])] + #[case::bump(&["comenq", "bump", "1a2b3c4d"])] + #[case::bust(&["comenq", "bust", "1a2b3c4d"])] + #[case::del(&["comenq", "del", "1a2b3c4d"])] + fn parses_queue_management_subcommands(#[case] argv: &[&str]) { + let args = Args::try_parse_from(argv).expect("valid arguments should parse"); + match (argv[1], args.command) { + ("list", Command::List) => {} + ("bump", Command::Bump { id }) + | ("bust", Command::Bust { id }) + | ("del", Command::Del { id }) => assert_eq!(id, "1a2b3c4d"), + (name, other) => panic!("unexpected parse for {name}: {other:?}"), + } + } + + #[test] + fn missing_subcommand_is_rejected() { + assert!(Args::try_parse_from(["comenq"]).is_err()); } #[rstest] @@ -172,7 +258,7 @@ mod tests { #[case("owner/")] #[case("owner/repo/extra")] fn rejects_invalid_slug(#[case] slug: &str) { - let result = Args::try_parse_from(["comenq", slug, "1", "Hi"]); + let result = Args::try_parse_from(["comenq", "put", slug, "1", "Hi"]); // Ensure the CLI surfaces the canonical repo format error. // This guards regressions in the Display of the parse error // as rendered through clap's error handling. @@ -214,7 +300,7 @@ mod tests { 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"]) + let args = Args::try_parse_from(["comenq", "put", "octocat/hello-world", "1", "Hi"]) .expect("valid arguments should parse"); assert_eq!(args.socket, None); assert_eq!( @@ -232,7 +318,7 @@ mod tests { "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"]) + let args = Args::try_parse_from(["comenq", "put", "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")); @@ -242,8 +328,7 @@ mod tests { #[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"); + let args = Args::try_parse_from(["comenq", "list"]).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")); } @@ -254,6 +339,7 @@ mod tests { let _socket_guard = EnvVarGuard::set("COMENQ_SOCKET", "/tmp/env.sock"); let args = Args::try_parse_from([ "comenq", + "put", "octocat/hello-world", "1", "Hi", diff --git a/crates/comenq/src/output.rs b/crates/comenq/src/output.rs new file mode 100644 index 0000000..a21be89 --- /dev/null +++ b/crates/comenq/src/output.rs @@ -0,0 +1,167 @@ +//! Human-readable rendering of daemon replies. +//! +//! Formats estimated posting times and one-line comment summaries for the +//! `put` and `list` subcommands. + +use comenq_lib::protocol::PendingEntry; + +/// Maximum characters of comment text shown by `list`. +const SUMMARY_LIMIT: usize = 60; + +/// Render an ETA in seconds as a compact human duration. +/// +/// # Examples +/// +/// ```rust +/// assert_eq!(comenq::format_eta(0), "now"); +/// assert_eq!(comenq::format_eta(45), "45s"); +/// assert_eq!(comenq::format_eta(150), "2m 30s"); +/// assert_eq!(comenq::format_eta(3_720), "1h 02m"); +/// ``` +#[must_use] +pub fn format_eta(seconds: u64) -> String { + const MINUTE: u64 = 60; + const HOUR: u64 = 60 * MINUTE; + if seconds == 0 { + return "now".to_owned(); + } + if seconds < MINUTE { + return format!("{seconds}s"); + } + if seconds < HOUR { + let minutes = seconds / MINUTE; + let rest = seconds % MINUTE; + return format!("{minutes}m {rest:02}s"); + } + let hours = seconds / HOUR; + let minutes = (seconds % HOUR) / MINUTE; + format!("{hours}h {minutes:02}m") +} + +/// Collapse a comment body to a single line of at most 60 characters. +/// +/// Control characters (including newlines and tabs) become spaces so the +/// summary never spans lines; longer bodies are truncated with an ellipsis. +/// +/// # Examples +/// +/// ```rust +/// assert_eq!(comenq::one_line_summary("Hi there"), "Hi there"); +/// assert_eq!(comenq::one_line_summary("a\nb\tc"), "a b c"); +/// let long = "x".repeat(80); +/// let summary = comenq::one_line_summary(&long); +/// assert_eq!(summary.chars().count(), 60); +/// assert!(summary.ends_with('…')); +/// ``` +#[must_use] +pub fn one_line_summary(body: &str) -> String { + let flat: String = body + .chars() + .map(|c| if c.is_control() { ' ' } else { c }) + .collect(); + if flat.chars().count() <= SUMMARY_LIMIT { + return flat; + } + let mut truncated: String = flat.chars().take(SUMMARY_LIMIT - 1).collect(); + truncated.push('…'); + truncated +} + +/// Render the `put` confirmation line. +pub(crate) fn render_put(entry: &PendingEntry) -> String { + format!( + "Queued {} for {}/{}#{} — posts in ~{}", + entry.id, + entry.owner, + entry.repo, + entry.pr_number, + format_eta(entry.eta_seconds) + ) +} + +/// Render one `list` line for a pending entry. +pub(crate) fn render_entry(entry: &PendingEntry) -> String { + format!( + "{} {:>7} {}/{}#{} {}", + entry.id, + format_eta(entry.eta_seconds), + entry.owner, + entry.repo, + entry.pr_number, + one_line_summary(&entry.body) + ) +} + +#[cfg(test)] +mod tests { + //! Unit tests for ETA and summary rendering. + use super::{format_eta, one_line_summary, render_entry, render_put}; + use comenq_lib::protocol::PendingEntry; + use rstest::rstest; + + fn entry(body: &str, eta: u64) -> PendingEntry { + PendingEntry { + id: "1a2b3c4d".into(), + eta_seconds: eta, + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 7, + body: body.into(), + } + } + + #[rstest] + #[case(0, "now")] + #[case(1, "1s")] + #[case(59, "59s")] + #[case(60, "1m 00s")] + #[case(150, "2m 30s")] + #[case(3_599, "59m 59s")] + #[case(3_600, "1h 00m")] + #[case(3_720, "1h 02m")] + #[case(90_000, "25h 00m")] + fn formats_eta(#[case] seconds: u64, #[case] expected: &str) { + assert_eq!(format_eta(seconds), expected); + } + + #[rstest] + #[case("short", "short")] + #[case("line\nbreaks\tand\rreturns", "line breaks and returns")] + fn summarises_one_line(#[case] body: &str, #[case] expected: &str) { + assert_eq!(one_line_summary(body), expected); + } + + #[rstest] + fn truncates_long_bodies_to_sixty_characters() { + let body = "a".repeat(100); + let summary = one_line_summary(&body); + assert_eq!(summary.chars().count(), 60); + assert!(summary.ends_with('…')); + } + + #[rstest] + fn sixty_character_bodies_are_untouched() { + let body = "a".repeat(60); + assert_eq!(one_line_summary(&body), body); + } + + #[rstest] + fn renders_put_confirmation() { + let line = render_put(&entry("Hi", 3_660)); + assert_eq!( + line, + "Queued 1a2b3c4d for octocat/hello-world#7 — posts in ~1h 01m" + ); + } + + #[rstest] + fn renders_list_line_with_truncated_body() { + let body = "b".repeat(100); + let line = render_entry(&entry(&body, 90)); + assert!(line.starts_with("1a2b3c4d")); + assert!(line.contains("1m 30s")); + assert!(line.contains("octocat/hello-world#7")); + assert!(line.ends_with('…')); + assert!(!line.contains('\n')); + } +} diff --git a/tests/steps/cli_steps.rs b/tests/steps/cli_steps.rs index 076693e..10eae3f 100644 --- a/tests/steps/cli_steps.rs +++ b/tests/steps/cli_steps.rs @@ -22,6 +22,7 @@ pub struct CliWorld { fn valid_cli_arguments(world: &mut CliWorld) { world.args = Some(vec![ OsString::from("comenq"), + OsString::from("put"), OsString::from("octocat/hello-world"), OsString::from("1"), OsString::from("Hi"), @@ -32,6 +33,7 @@ fn valid_cli_arguments(world: &mut CliWorld) { fn cli_args_with_repo_slug(world: &mut CliWorld, slug: String) { world.args = Some(vec![ OsString::from("comenq"), + OsString::from("put"), OsString::from(slug), OsString::from("1"), OsString::from("Hi"), diff --git a/tests/steps/client_main_steps.rs b/tests/steps/client_main_steps.rs index 0f34032..6f5d412 100644 --- a/tests/steps/client_main_steps.rs +++ b/tests/steps/client_main_steps.rs @@ -1,11 +1,11 @@ //! Behavioural test steps for the client binary's interaction with the daemon. use anyhow::Context as _; -use comenq::{Args, ClientError, run}; -use comenq_lib::CommentRequest; +use comenq::{Args, ClientError, Command, run}; +use comenq_lib::protocol::{PendingEntry, Request, Response}; use cucumber::{World, given, then, when}; use tempfile::TempDir; -use tokio::io::AsyncReadExt; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::UnixListener; #[derive(Debug, Default, World)] @@ -19,10 +19,12 @@ pub struct ClientWorld { /// Build the default client arguments targeting `socket`. fn base_args(socket: std::path::PathBuf) -> anyhow::Result { Ok(Args { - repo_slug: "octocat/hello-world".parse().context("slug")?, - pr_number: 1, - comment_body: "Hi".into(), socket: Some(socket), + command: Command::Put { + repo_slug: "octocat/hello-world".parse().context("slug")?, + pr_number: 1, + comment_body: "Hi".into(), + }, }) } @@ -36,6 +38,17 @@ fn dummy_daemon(world: &mut ClientWorld) -> anyhow::Result<()> { let (mut stream, _) = listener.accept().await.context("accept")?; let mut buf = Vec::new(); stream.read_to_end(&mut buf).await.context("read")?; + // Reply so the client's transaction completes. + let reply = Response::entry(PendingEntry { + id: "1a2b3c4d".into(), + eta_seconds: 0, + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 1, + body: "Hi".into(), + }); + let bytes = serde_json::to_vec(&reply).context("serialize reply")?; + stream.write_all(&bytes).await.context("write reply")?; Ok(buf) }); @@ -65,7 +78,10 @@ async fn send_request(world: &mut ClientWorld) -> anyhow::Result<()> { async fn daemon_receives(world: &mut ClientWorld) -> anyhow::Result<()> { let handle = world.server.take().context("server handle")?; let data = handle.await.context("join")??; - let req: CommentRequest = serde_json::from_slice(&data).context("parse")?; + let request: Request = serde_json::from_slice(&data).context("parse")?; + let Request::Put { request: req } = request else { + anyhow::bail!("expected put request, got {request:?}"); + }; assert_eq!(req.owner, "octocat"); assert_eq!(req.repo, "hello-world"); assert_eq!(req.pr_number, 1); From 507204e6cc3ea315b460f7ccee064770bd3f5b76 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 17:54:08 +0200 Subject: [PATCH 5/6] Add queue management BDD coverage and refresh documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `tests/features/queue.feature` with scenarios exercising the daemon's protocol surface end to end: `put` replies carry an eight-character identifier and an ETA, identifiers reappear in listings, listings report strictly increasing ETAs in posting order, long bodies collapse to a one-line sixty-character summary, `bump` moves an entry to the head, `bust` to the tail, `del` removes it, and unknown identifiers are rejected with a named error. Refresh the documentation for the new architecture: the design document now describes the bespoke `QueueStore` (entry format, identifier scheme, ordering keys, atomic writes, `last_post`, ETA projection), the request/response protocol, the two-task supervisor topology, and the client subcommands; the `comenq` man page and README document `put`, `list`, `bump`, `bust`, and `del`. Remove the vestigial `client_channel_capacity` configuration field — nothing has buffered client requests since the queue-writer task was removed. --- README.md | 11 +- crates/comenqd/src/config.rs | 12 - crates/comenqd/src/config/tests.rs | 9 +- crates/comenqd/tests/daemon.rs | 1 - crates/comenqd/tests/listener.rs | 1 - docs/comenq-design.md | 376 ++++++++++++++++++----------- packaging/man/comenq.1 | 47 +++- test-support/src/daemon.rs | 11 - tests/cucumber.rs | 5 +- tests/features/queue.feature | 46 ++++ tests/steps/mod.rs | 2 + tests/steps/queue_steps.rs | 251 +++++++++++++++++++ 12 files changed, 586 insertions(+), 186 deletions(-) create mode 100644 tests/features/queue.feature create mode 100644 tests/steps/queue_steps.rs diff --git a/README.md b/README.md index 23cac91..61812d6 100644 --- a/README.md +++ b/README.md @@ -28,10 +28,19 @@ After building, launch the daemon and queue comments with the client: ```bash make build ./target/debug/comenqd & -./target/debug/comenq owner/repo 123 "Comment body" +./target/debug/comenq put owner/repo 123 "Comment body" ``` Queued requests persist on disk and are posted sequentially by the daemon. +`put` prints the comment's deterministic eight-character identifier and an +approximate ETA. The queue can be inspected and reordered by identifier: + +```bash +comenq list # schedule of pending comments with IDs and ETAs +comenq bump 1a2b3c4d # move to the head of the queue +comenq bust 1a2b3c4d # move to the tail of the queue +comenq del 1a2b3c4d # remove from the queue +``` ## Running as a user service diff --git a/crates/comenqd/src/config.rs b/crates/comenqd/src/config.rs index 4315c09..11d05bd 100644 --- a/crates/comenqd/src/config.rs +++ b/crates/comenqd/src/config.rs @@ -22,8 +22,6 @@ const DEFAULT_COOLDOWN_FLUTTER: u64 = 0; const DEFAULT_RESTART_MIN_DELAY_MS: u64 = 100; /// Default timeout in seconds for GitHub API calls. const DEFAULT_GITHUB_API_TIMEOUT_SECS: u64 = 30; -/// Default capacity for the listener channel buffering client requests. -const DEFAULT_CLIENT_CHANNEL_CAPACITY: usize = 1024; /// Runtime configuration for the daemon. #[derive(Debug, Deserialize, Serialize, PartialEq, Eq, Clone)] @@ -69,9 +67,6 @@ pub struct Config { /// Timeout applied to GitHub API requests in seconds. #[serde(default = "default_github_api_timeout_secs")] pub github_api_timeout_secs: u64, - /// Capacity of the channel buffering client requests. - #[serde(default = "default_client_channel_capacity")] - pub client_channel_capacity: usize, } /// Convert a [`test_support::daemon::TestConfig`] into a [`Config`]. @@ -99,7 +94,6 @@ impl From for Config { cooldown_period_seconds, restart_min_delay_ms, github_api_timeout_secs, - client_channel_capacity, } = value; Self { github_token, @@ -110,7 +104,6 @@ impl From for Config { cooldown_flutter_seconds: DEFAULT_COOLDOWN_FLUTTER, restart_min_delay_ms, github_api_timeout_secs, - client_channel_capacity, } } } @@ -150,7 +143,6 @@ impl From<&test_support::daemon::TestConfig> for Config { 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, } } } @@ -205,10 +197,6 @@ fn default_github_api_timeout_secs() -> u64 { DEFAULT_GITHUB_API_TIMEOUT_SECS } -fn default_client_channel_capacity() -> usize { - DEFAULT_CLIENT_CHANNEL_CAPACITY -} - impl Config { /// Default location of the daemon configuration file. pub const DEFAULT_PATH: &'static str = "/etc/comenqd/config.toml"; diff --git a/crates/comenqd/src/config/tests.rs b/crates/comenqd/src/config/tests.rs index 74a76c5..8322b58 100644 --- a/crates/comenqd/src/config/tests.rs +++ b/crates/comenqd/src/config/tests.rs @@ -1,8 +1,8 @@ //! 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, + CliArgs, Config, DEFAULT_COOLDOWN, DEFAULT_GITHUB_API_TIMEOUT_SECS, + DEFAULT_RESTART_MIN_DELAY_MS, }; use rstest::rstest; use std::fs; @@ -86,7 +86,6 @@ fn defaults_are_applied() { 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] @@ -261,8 +260,4 @@ fn converts_from_test_config(#[case] conv: fn(&test_support::daemon::TestConfig) 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/tests/daemon.rs b/crates/comenqd/tests/daemon.rs index 47606ed..fefd4d6 100644 --- a/crates/comenqd/tests/daemon.rs +++ b/crates/comenqd/tests/daemon.rs @@ -49,7 +49,6 @@ fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { cooldown_flutter_seconds: 0, restart_min_delay_ms: cfg.restart_min_delay_ms, github_api_timeout_secs: cfg.github_api_timeout_secs, - client_channel_capacity: cfg.client_channel_capacity, } } diff --git a/crates/comenqd/tests/listener.rs b/crates/comenqd/tests/listener.rs index 2a203ac..549da71 100644 --- a/crates/comenqd/tests/listener.rs +++ b/crates/comenqd/tests/listener.rs @@ -37,7 +37,6 @@ fn cfg_from(cfg: test_support::daemon::TestConfig) -> Config { cooldown_flutter_seconds: 0, restart_min_delay_ms: cfg.restart_min_delay_ms, github_api_timeout_secs: cfg.github_api_timeout_secs, - client_channel_capacity: cfg.client_channel_capacity, } } diff --git a/docs/comenq-design.md b/docs/comenq-design.md index e7e8762..a943e30 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -84,20 +84,26 @@ The complete lifecycle of a request is illustrated in the following sequence: This architecture ensures that comment posting is strictly serialized and paced, directly addressing the primary goal of avoiding API rate limits. -### Channels and buffering +### Shared queue and concurrency -The daemon sends client requests via an unbounded channel. The channel sender -is recreated whenever the writer task restarts, preserving the existing -receiver when possible. +The listener and worker tasks do not communicate over a channel. Instead, both +hold an `Arc` wrapping the on-disk `QueueStore`, the daemon +configuration, and a `tokio::sync::Notify` used to wake the worker promptly +when the queue changes. -- Operational warning: the channel is unbounded. During writer downtime, the - in-memory backlog can grow until the supervisor respawns the writer. Deploy - with external backpressure (e.g., rate-limiting clients) if this risk is - unacceptable. +- The listener executes each client request directly against the shared + queue (via `SharedQueue::execute`) and writes the reply before the + connection closes. A request is durably persisted to disk before the client + receives its acknowledgement, so there is no in-memory backlog that could be + lost if the daemon exits unexpectedly. -- Lossy path: If the writer task panics, the old receiver is dropped and any - buffered items in that channel are lost. This is the only scenario where - pending requests may be discarded. +- Mutating operations (`put`, `bump`, `bust`, `del`) call `notify_one` on the + shared `Notify` after the change is written, so the worker interrupts any + wait and re-evaluates the head of the queue immediately. + +- Because every mutation goes straight to disk, restarting either the + listener or the worker after a panic loses no pending requests: the store + itself is the single source of truth, not an in-memory buffer. ### 1.2. Core Technology Stack: Crate Selection and Justification @@ -113,7 +119,7 @@ project requirements. | Asynchronous Runtime | tokio | The de-facto standard for asynchronous programming in Rust. It provides a high-performance, multithreaded scheduler and a comprehensive suite of utilities for I/O, networking, and timers, including the essential UnixListener, UnixStream, and time::sleep components.[^2] Its maturity and extensive ecosystem make it the definitive choice for the daemon's core. | async-std | | CLI Argument Parsing | clap | The most popular and feature-rich CLI argument parsing library for Rust.[^3] The | derive feature offers an exceptionally ergonomic and declarative way to define the CLI's structure, automatically generating argument parsing, validation, and help text from a simple struct definition.[^3] | argh, pico-args 4 | | GitHub API Client | octocrab | A modern, actively maintained, and extensible GitHub API client.[^5] It provides strongly typed models for API responses and a builder pattern for requests, simplifying interaction with the GitHub REST API. Its static API and support for custom middleware are valuable for building robust clients.[^3] | roctokit 12, manual | reqwest 13 | -| Persistent Queue | yaque | A disk-backed, persistent queue designed for asynchronous environments.[^7] Its most critical feature is | transactional reads via its RecvGuard mechanism. This ensures that a dequeued item is automatically returned to the queue if the program panics or fails before the item is explicitly committed, providing an "at-least-once" delivery guarantee essential for reliability.[^7] | queue-file 16, | v_queue 18 | +| Persistent Queue | bespoke `QueueStore` (`comenqd::store`) | A disk-backed store using one JSON file per pending entry plus a `last_post` marker, with atomic write-then-rename updates. Unlike an append-only queue, it supports reordering (`bump`, `bust`) and removal (`del`) of arbitrary entries, and only deletes an entry after a confirmed post, giving an "at-least-once" delivery guarantee. | yaque (rejected: append-only, cannot reorder or delete arbitrary entries) | | IPC Serialization | serde / serde_json | serde is the universal framework for serialization and deserialization in Rust. serde_json provides a straightforward implementation for the JSON data format, which is chosen for its human-readability (aiding in debugging) and widespread support. | bincode, prost | | Systemd Integration | systemd (crate) | Provides native Rust bindings for interacting with the systemd journal and daemon notification APIs.[^8] While the primary deployment mechanism is a | .service file, this crate can be used for more advanced integration, such as sending readiness notifications. | systemctl (crate) 20 | | Logging | tracing / tracing-subscriber | A modern, structured, and asynchronous-aware logging and diagnostics framework. It is the standard choice for tokio-based applications, providing contextual information that is superior to traditional line-based logging. | log / env_logger 22 | @@ -351,6 +357,50 @@ communicate with the daemon are surfaced via a small `ClientError` enumeration. The repository slug is converted into a structured `RepoSlug` during argument handling, so `run` operates on validated data without rechecking it. +### 2.4. Client Subcommands and ETA Semantics + +The illustrative blueprint above shows a client that only enqueues a comment. +The shipped client instead exposes five subcommands, one per protocol +operation, all sharing a global `--socket` flag (also settable via the +`COMENQ_SOCKET` environment variable): + +- `comenq put `: enqueues a comment and + prints its identifier and an approximate ETA, e.g. `Queued 1a2b3c4d for + octocat/hello-world#7 — posts in ~1h 01m`. + +- `comenq list`: prints one line per pending comment, in posting order, each + showing the identifier, the ETA, the target `owner/repo#pr`, and the + comment body collapsed to a single line of at most 60 characters (control + characters, including newlines, become spaces; longer bodies are truncated + with an ellipsis). + +- `comenq bump `: moves the identified comment to the head of the queue. + +- `comenq bust `: moves the identified comment to the tail of the queue. + +- `comenq del `: removes the identified comment from the queue. + +Each subcommand maps directly to one variant of the `Request` enum (see +§3.2 for the store and §5 for the wire protocol) and prints a short +confirmation, or the daemon's error message, once the single reply is +received. + +**ETA semantics.** The estimated time until posting shown by `put` and `list` +reflects two rules enforced by the daemon: + +- **Flutter is fixed at enqueue time.** When a comment is enqueued, a random + flutter duration (up to `cooldown_flutter_seconds`) is sampled once and + stored with the entry. It does not change on subsequent `list` calls, so the + reported ETA for a given entry only decreases as time passes; it never + jumps around. + +- **The cooldown always runs in full.** The daemon never shortens + `cooldown_period_seconds` to catch up; each entry's projected posting time + is its predecessor's projected posting time plus a full cooldown plus its + own flutter (or, for the head entry, the last successful post plus a full + cooldown plus its own flutter). This keeps the reported ETA consistent with + what the worker will actually do. + ## Section 3: Design of the `comenqd` Daemon The `comenqd` daemon is the heart of the system. It is a stateful, @@ -365,33 +415,36 @@ Upon startup, the `main` function will initialize necessary resources (configuration, logger, queue) and then spawn two primary, independent asynchronous tasks that run concurrently for the lifetime of the daemon: -1. `task_listen_for_requests`: This task is the daemon's public-facing +1. **Listener** (`run_listener`): This task is the daemon's public-facing interface. It binds to the UDS and listens for incoming connections from - `comenq` clients. Its sole job is to accept requests and place them into the - queue as quickly as possible. - -2. `task_process_queue`: This is the main worker task. It operates in a - serialized loop, pulling one job at a time from the queue, processing it - (i.e., posting the comment to GitHub), and then observing the mandatory - 16-minute cooldown period. - -This concurrent design ensures that the daemon remains responsive to new client -requests even while the worker task is in its long sleep phase. A request can -be accepted and enqueued in milliseconds, while the worker task independently -processes the queue at its own deliberate pace. - -All daemon tasks—the listener, worker, and queue writer—are supervised. If any -task exits unexpectedly, the daemon logs the failure, waits using an -exponential backoff with jitter (via the `backon` crate) to avoid a tight -restart loop, and then respawns the task. The minimum delay between restarts is -configurable via `restart_min_delay_ms`. This keeps the service available -without relying on an external process supervisor. Restarting the writer -recreates the listener→writer channel and restarts the listener to attach a -fresh sender, preserving single-writer semantics. When the writer exits -cleanly, the supervisor reuses the existing receiver, so buffered bytes are -preserved. If the writer panics and the receiver is lost, a new channel is -created, and any bytes buffered in the discarded channel that were not -persisted to the queue are lost. + `comenq` clients. For each connection it reads one JSON `Request`, + executes it directly against the shared queue, and writes back one JSON + `Response` before closing the connection. + +2. **Worker** (`run_worker`): This is the main worker task. It operates in a + loop, recomputing the head entry's due time on every iteration, waiting + until that entry is due (or until the queue changes, or shutdown is + signalled), posting the comment to GitHub, and recording the post before + moving on to the next entry. + +This concurrent design ensures that the daemon remains responsive to new +client requests even while the worker task is in its long sleep phase. A +request can be accepted, persisted, and acknowledged in milliseconds, while +the worker task independently processes the queue at its own deliberate pace. + +Both tasks share the queue through an `Arc` rather than a +channel: there is no intermediate queue-writer task. The listener performs +each mutation directly, and the worker is woken (via a `tokio::sync::Notify`) +whenever a mutation occurs. + +Both the listener and the worker are supervised. If either task exits +unexpectedly, the daemon logs the failure, waits using an exponential backoff +with jitter (via the `backon` crate) to avoid a tight restart loop, and then +respawns the task. The minimum delay between restarts is configurable via +`restart_min_delay_ms`. This keeps the service available without relying on +an external process supervisor. Because both tasks operate on the same +on-disk store rather than an in-memory buffer, restarting either task after a +panic does not discard any pending request. The supervision and restart behaviour is illustrated in the sequence diagram below. @@ -403,26 +456,22 @@ sequenceDiagram participant Sup as Supervisor::run participant L as Listener participant W as Worker - participant QW as QueueWriter - participant YQ as YaQue + participant Q as SharedQueue Sup->>Sup: ensure_queue_dir() - Sup->>YQ: init Sender - Sup->>QW: spawn queue_writer(rx) - Sup->>L: spawn run_listener(tx, shutdown) - Sup->>W: spawn run_worker(yaque_rx, octocrab, control) + Sup->>Q: SharedQueue::open(config) + Sup->>L: spawn run_listener(queue, shutdown) + Sup->>W: spawn run_worker(queue, octocrab, control) par Normal flow - L->>QW: tx.send(bytes) - QW->>YQ: enqueue(bytes) - YQ-->>W: deliver entry - W->>W: deserialize & post to GitHub - W->>YQ: commit() + L->>Q: execute(Request) [persist & reply] + Q-->>W: notify_one() on mutation + W->>Q: next_due() + W->>W: post to GitHub + W->>Q: complete(id) and Error/backoff L--x Sup: accept error Sup->>L: restart after backoff - QW--x Sup: enqueue error - Sup->>QW: restart after backoff W--x Sup: fatal error Sup->>W: restart after backoff end @@ -430,43 +479,64 @@ sequenceDiagram OS-->>Sup: SIGINT/SIGTERM Sup->>L: signal shutdown Sup->>W: signal shutdown - Sup->>QW: abort/await Sup-->>OS: exit ``` -### 3.2. The Persistent Job Queue with `yaque` +### 3.2. The Persistent Job Queue: `comenqd::store::QueueStore` A core requirement for the daemon is fault tolerance. If the daemon or the entire server restarts, pending comments must not be lost. This rules out -simple in-memory queues like `std::collections::VecDeque` 26 and necessitates a +simple in-memory queues like `std::collections::VecDeque` and necessitates a disk-backed, persistent solution. -The `yaque` crate is selected as the ideal queue implementation for this -project.[^7] While other file-based queues exist 17, - -`yaque` offers a unique combination of features perfectly suited to this -daemon's needs: - -- **Natively Asynchronous:** It is built on `mio` and integrates seamlessly - with the `tokio` runtime without requiring blocking operations.[^7] - -- **Persistence:** It stores queue data on the filesystem, ensuring durability - across process restarts.[^7] - -- **Transactional Reads:** This is the most compelling feature. When an item is - dequeued using `receiver.recv().await`, `yaque` returns a `RecvGuard`. The - item is not permanently removed from the queue at this point. It is only - removed when `guard.commit()` is explicitly called. If the `RecvGuard` is - dropped without being committed (e.g., due to a program panic or an API - error), the item is automatically and safely returned to the head of the - queue. This "dead man's switch" mechanism provides a powerful "at-least-once" - delivery guarantee, which is the cornerstone of the daemon's reliability.[^7] - -The queue will be initialized at a configurable path (e.g., -`/var/lib/comenq/queue`) and will store the `CommentRequest` struct defined in -the shared library. - -### 3.3. The UDS Listener and Request Ingestion (`task_listen_for_requests`) +An earlier design used the `yaque` crate, an append-only, transactional, +disk-backed queue. `yaque` was rejected once the client gained `bump`, `bust`, +and `del` operations: an append-only queue has no way to reorder or delete an +arbitrary entry, only to dequeue from the head. The daemon therefore uses a +bespoke store, `comenqd::store::QueueStore`, built directly on plain files. + +`QueueStore` keeps one JSON file per pending comment under +`/entries`, plus a `/last_post` file recording the Unix +time of the most recent successful post. Each entry (a `StoredEntry`) carries: + +- **A deterministic eight-character identifier.** This is the first eight hex + digits of a 64-bit FNV-1a hash computed over the request's owner, repo, PR + number, and body, together with the second at which it was enqueued. The + identifier is therefore stable across daemon restarts, and an identical + request repeated within the same second is idempotent: `put` returns the + existing entry unchanged rather than creating a duplicate. + +- **An explicit integer ordering key.** New entries are appended after the + current tail. `bump` sets the key to one less than the current head's key + (or leaves it unchanged if the entry is already the head); `bust` sets it to + one more than the current tail's key. Repeated bumps or busts can drive the + key negative or arbitrarily large; entries are always read back sorted by + `(order, enqueued_at, id)`. + +- **The flutter sampled at enqueue time.** A random duration up to + `cooldown_flutter_seconds` is chosen once, when the entry is created, and + stored with it. Fixing the flutter at enqueue time keeps the ETA reported to + the client stable for the entry's whole life, rather than jittering on every + `list`. + +- **The enqueue time**, used both as a tiebreaker for ordering and as an input + to the identifier hash. + +Entries are written to a temporary sibling file and then renamed into place, +so a reader never observes a half-written entry. `bump`, `bust`, and `del` +mutate or remove a single entry file the same way. `complete` removes the +posted entry and atomically rewrites `last_post` with the current Unix time. + +Because an entry is only removed from disk after a successful post +(`complete`), and a failed post simply leaves the entry in place for the next +attempt, the store preserves the same "at-least-once" delivery guarantee that +motivated the original choice of `yaque`, without its append-only limitation. + +The queue is initialized at a configurable path (e.g., `/var/lib/comenq/queue`) +and stores the `CommentRequest` struct defined in the shared library as part of +each entry. + +### 3.3. The UDS Listener and Request Ingestion (`run_listener`) This task is responsible for handling all client communication. It will be implemented as an asynchronous function spawned by the main `tokio` runtime. @@ -486,21 +556,25 @@ Its workflow is as follows: 4. **Spawn Connection Handler:** To ensure the listener is never blocked, upon accepting a new connection, it immediately spawns a new, short-lived `tokio` - task to handle that specific client. + task (`handle_client`) to handle that specific client. 5. **Handle Client:** This per-client task reads at most 1 MiB within `CLIENT_READ_TIMEOUT_SECS` (default: 5 s); larger or slower requests are rejected with a timeout or size error. The `MAX_REQUEST_BYTES` and `CLIENT_READ_TIMEOUT_SECS` limits are compile-time constants that can be - adjusted. It deserializes the received JSON into a `CommentRequest` and uses - the sender half of the `yaque` channel to enqueue the request. After - enqueuing, the task terminates. + adjusted. It deserializes the received JSON into a `Request`, executes it + directly against the shared queue (`SharedQueue::execute`), and writes the + resulting `Response` back to the client before closing the connection. A + request that fails to deserialize receives an error `Response` rather than + a silently dropped connection. This design makes the request ingestion process highly concurrent and robust, capable of handling multiple simultaneous client connections without impacting -the main worker loop. +the worker loop. Because each request is executed and persisted before the +reply is sent, there is no intermediate queue-writer stage to lose data if the +listener is later restarted. -The interaction between the client, listener, and queue writer is shown in the +The interaction between the client, listener, and shared queue is shown in the sequence diagram below. ```mermaid @@ -509,23 +583,22 @@ sequenceDiagram participant Client as Client participant L as Listener participant H as handle_client - participant TX as mpsc::UnboundedSender - participant QW as QueueWriter + participant Q as SharedQueue Client->>L: connect(socket) L->>H: spawn handler(stream) - Client->>H: write JSON CommentRequest + Client->>H: write JSON Request H->>H: read & deserialize - H->>TX: send JSON bytes - TX-->>QW: deliver payload - H-->>Client: close + H->>Q: execute(Request) + Q-->>H: Response + H-->>Client: write JSON Response, close ``` -### 3.4. The GitHub Comment-Posting Worker (`task_process_queue`) +### 3.4. The GitHub Comment-Posting Worker (`run_worker`) This task implements the core business logic of the service. It runs in a -simple, infinite loop, ensuring that comments are processed one by one with the -required delay. +loop, ensuring that comments are processed one by one with the required +delay. #### 3.4.1. `octocrab` Initialization and API Usage @@ -551,36 +624,52 @@ octocrab.issues("owner", "repo").create_comment(pr_number, "body").await?; The worker task's loop consists of the following steps: -1. **Dequeue Job:** It calls `receiver.recv().await?` to receive the next - `CommentRequest` wrapped in a `yaque::RecvGuard`. This operation will block - asynchronously until a job is available in the queue. +1. **Compute the due entry:** It calls `queue.next_due()`, which asks the + store to recompute the head entry and its estimated seconds-until-post on + every iteration. This means a `bump`, `bust`, `del`, or new `put` performed + by a client takes effect on the very next iteration, not just at the start + of the loop. + +2. **Wait until due:** If nothing is queued, the worker waits for the shared + queue's change signal or a shutdown signal. If the head entry is not yet + due, the worker sleeps for the remaining time, but the sleep is + interruptible: a queue change (e.g. a `bump` promoting a different entry) + or shutdown wakes it early, and it loops back to recompute the due entry + rather than blindly posting whichever entry it started waiting for. -2. **Post Comment:** It constructs and sends the API request to GitHub using - the `octocrab` client and the data from the dequeued job. +3. **Post Comment:** Once an entry is due, the worker constructs and sends the + API request to GitHub using the `octocrab` client and the data from the + entry. -3. **Handle Result:** +4. **Handle Result:** - - **On API Success:** The task immediately calls `guard.commit()` to - finalize the transaction and permanently remove the job from the queue. It - then logs the successful post. + - **On API Success:** The task calls `queue.complete(&entry.id)`, which + removes the entry from disk and atomically records the current time in + `last_post`. It then logs the successful post. - **On API Failure:** The task logs the error from the GitHub API. The - `guard` is simply dropped. `yaque`'s transactional guarantee ensures the - job is automatically returned to the queue, ready to be retried on the - next iteration of the loop. For more advanced error handling, a retry + entry is left in place (`complete` is never called for it), so it is + retried on a later iteration. The worker waits a full + `cooldown_period_seconds` before retrying, to avoid hammering a + persistently failing API. For more advanced error handling, a retry counter could be added to the `CommentRequest` to prevent infinite loops for unfixable errors, eventually moving the job to a "dead-letter" queue. -4. **Cooldown:** After successfully processing a job (or after a failed - attempt), the task calls - `tokio::time::sleep(Duration::from_secs(960)).await` to enforce the - 16-minute cooling-off period. - 5. The loop then repeats. -This workflow, built upon `yaque`'s transactional foundation, creates a highly -resilient system that can tolerate both network failures and process crashes -without losing data. +**ETA projection.** The store computes, for each pending entry, an estimated +number of seconds until it is posted (`schedule` in `QueueStore`). The head +entry is due one full `cooldown_period_seconds` plus its own sampled flutter +after the last successful post, or immediately if nothing has been posted +yet. Each subsequent entry is due a further cooldown plus its own flutter +after its predecessor's projected posting time. Because flutter is sampled +once, at enqueue time, and the cooldown always runs in full, the ETA reported +to a client by `put` or `list` matches what the worker will actually do, and +does not drift as time passes. + +This workflow gives a highly resilient system that can tolerate both network +failures and process crashes without losing data: an entry is only ever +removed from the store once GitHub has confirmed the post. ### 3.5. Daemon Configuration and Logging @@ -593,7 +682,7 @@ at `/etc/comenqd/config.toml` is the conventional choice. | 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 | +| queue_path | PathBuf | The directory path for the persistent queue data (`entries/` and `last_post`, managed by `QueueStore`). | /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 | @@ -885,7 +974,9 @@ clap = { version = "4.4", features = ["derive"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" octocrab = "0.38" -yaque = "0.6" +rand = "0.9" +uuid = { version = "1", features = ["v4"] } +backon = "1.5" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } anyhow = "1.0" @@ -1019,20 +1110,22 @@ sequenceDiagram participant WatchChannel participant WorkerHooks loop Process queue - Worker->>Queue: rx.recv() - alt Shutdown signal - WatchChannel-->>Worker: shutdown.changed() - Worker->>WorkerHooks: (optional) drained.notify_waiters() - Worker-->>Worker: break - else Got request - Worker->>WorkerHooks: (optional) enqueued.notify_waiters() - Worker->>Worker: process and commit - alt Queue empty - Worker->>WorkerHooks: (optional) drained.notify_waiters() - Worker->>WorkerHooks: (optional) idle.notify_waiters() - else Queue not empty - Worker->>Worker: sleep or shutdown + Worker->>Queue: next_due() + alt Nothing queued + Worker->>WorkerHooks: (optional) drained.notify_one() + Worker->>WatchChannel: select: shutdown.changed() or queue.changed() + else Head not yet due + Worker->>WatchChannel: select: shutdown, queue.changed(), or sleep(wait_seconds) + else Head due now + Worker->>WorkerHooks: (optional) enqueued.notify_one() + Worker->>Worker: post to GitHub + alt Success + Worker->>Queue: complete(id) + else Failure + Worker->>WorkerHooks: (optional) idle.notify_one() + Worker->>WatchChannel: wait_or_shutdown(cooldown_period_seconds) end + Worker->>WorkerHooks: (optional) idle.notify_one() end end ``` @@ -1046,16 +1139,18 @@ because the notifier may fire before the waiter starts listening. ### 5.6. Implementation Notes The repository initializes the workspace with `comenq-lib` at the root and two -binary crates under `crates/`. `CommentRequest` resides in the library and -derives both `Serialize` and `Deserialize`. The daemon now spawns a Unix socket -listener and queue worker as described above. Structured logging is initialized -using `tracing_subscriber` with JSON output controlled by the `RUST_LOG` -environment variable. The queue directory is created asynchronously on start if -it does not already exist before `yaque` opens it. Incoming requests are -forwarded from the listener to a dedicated queue writer task over a bounded -Tokio `mpsc` channel sized by `client_channel_capacity`. This task serializes -writes to the `yaque::Sender`, preserving single-writer semantics without -per-connection locking. +binary crates under `crates/`. `CommentRequest` and the `protocol` module +(`Request`/`Response`) reside in the library and derive both `Serialize` and +`Deserialize`. The daemon now spawns a Unix socket listener and queue worker as +described above. Structured logging is initialized using `tracing_subscriber` +with JSON output controlled by the `RUST_LOG` environment variable. The queue +directory is created asynchronously on start if it does not already exist, +before `QueueStore::open` reads or creates the `entries` sub-directory within +it. There is no dedicated queue-writer task or inter-task channel: the listener +and worker both hold an `Arc`, which wraps the store in a +`tokio::sync::Mutex`. Serializing access through this mutex, rather than a +per-connection lock or a writer task, gives the same single-writer semantics +for on-disk mutations while keeping the concurrency model simple. The worker's cooling-off period is configured via `cooldown_period_seconds` and defaults to 960 seconds (16 minutes) to provide ample headroom against GitHub's @@ -1086,7 +1181,6 @@ flag. [^8]: octocrab/examples/custom_client.rs at main - GitHub. Accessed on July 24, 2025. - July 24, 2025. [^10]: codyps/rust-systemd: Rust interface to systemd c apis - GitHub. Accessed on July 24, 2025. diff --git a/packaging/man/comenq.1 b/packaging/man/comenq.1 index 4de2d51..bb4dc60 100644 --- a/packaging/man/comenq.1 +++ b/packaging/man/comenq.1 @@ -2,22 +2,48 @@ .SH NAME comenq \- enqueue a GitHub pull request comment for the comenqd daemon .SH SYNOPSIS -.B comenq +.B comenq put .I owner/repo .I pr-number .I comment -[ -.B --socket -.I path -] +.br +.B comenq list +.br +.B comenq bump +.I id +.br +.B comenq bust +.I id +.br +.B comenq del +.I id .SH DESCRIPTION The .B comenq -command enqueues a comment request for the +command manages the comment queue of the .BR comenqd (8) -daemon. It takes the target repository in +daemon. +.TP +.B put +Enqueue a comment for the given repository (in .I owner/repo -format, the pull request number, and the comment body to submit. +format) and pull request. Prints the comment's deterministic +eight-character identifier and an approximate ETA. The ETA includes the +random flutter, which is fixed when the comment is enqueued. +.TP +.B list +Print the schedule of pending comments: identifier, ETA, target pull +request, and the comment text collapsed to a single line of at most 60 +characters. +.TP +.B bump +Move the identified comment to the head of the queue. +.TP +.B bust +Move the identified comment to the tail of the queue. +.TP +.B del +Remove the identified comment from the queue. .PP Use .B --socket @@ -31,11 +57,12 @@ socket among .I /run/comenq/comenq.sock (the packaged system service). .SH EXAMPLES -Send a comment to pull request 42 of example/repo: +Send a comment to pull request 42 of example/repo, then inspect the queue: .PP .RS .nf -comenq example/repo 42 "Queued for review" +comenq put example/repo 42 "Queued for review" +comenq list .fi .RE .SH SEE ALSO diff --git a/test-support/src/daemon.rs b/test-support/src/daemon.rs index 59bd742..48b753c 100644 --- a/test-support/src/daemon.rs +++ b/test-support/src/daemon.rs @@ -25,8 +25,6 @@ pub struct TestConfig { pub restart_min_delay_ms: u64, /// Timeout for GitHub API requests in seconds. pub github_api_timeout_secs: u64, - /// Capacity of the channel buffering client requests. - pub client_channel_capacity: usize, } impl Default for TestConfig { @@ -38,7 +36,6 @@ impl Default for TestConfig { cooldown_period_seconds: 1, restart_min_delay_ms: 1, github_api_timeout_secs: 30, - client_channel_capacity: 1024, } } } @@ -56,7 +53,6 @@ pub fn temp_config(tmp: &TempDir) -> TestConfig { cooldown_period_seconds: 1, restart_min_delay_ms: 1, github_api_timeout_secs: 30, - client_channel_capacity: 1024, } } @@ -97,13 +93,6 @@ impl TestConfig { self.github_api_timeout_secs = d.as_secs(); self } - - /// Override the client channel capacity. - #[must_use] - pub fn with_client_channel_capacity(mut self, cap: usize) -> Self { - self.client_channel_capacity = cap; - self - } } /// Construct an [`Octocrab`] client for a [`MockServer`]. diff --git a/tests/cucumber.rs b/tests/cucumber.rs index ffaef36..265554f 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -6,8 +6,8 @@ mod steps; use cucumber::World as _; use steps::{ - CliWorld, ClientWorld, CommentWorld, ConfigWorld, ListenerWorld, PackagingWorld, ReleaseWorld, - WorkerWorld, + CliWorld, ClientWorld, CommentWorld, ConfigWorld, ListenerWorld, PackagingWorld, QueueWorld, + ReleaseWorld, WorkerWorld, }; fn main() -> anyhow::Result<()> { @@ -28,6 +28,7 @@ fn main() -> anyhow::Result<()> { ConfigWorld::run("tests/features/config.feature"), ListenerWorld::run("tests/features/listener.feature"), PackagingWorld::run("tests/features/packaging.feature"), + QueueWorld::run("tests/features/queue.feature"), WorkerWorld::run("tests/features/worker.feature"), ); }); diff --git a/tests/features/queue.feature b/tests/features/queue.feature new file mode 100644 index 0000000..a2bf55b --- /dev/null +++ b/tests/features/queue.feature @@ -0,0 +1,46 @@ +Feature: Queue management + + Scenario: putting a comment reports its identifier and ETA + Given an empty comment queue + When the comment "First comment" is put + Then the reply carries an eight character identifier + And the reply reports an immediate ETA + + Scenario: put identifiers reappear in listings + Given an empty comment queue + When the comment "First comment" is put + Then listing shows the same identifier as the put reply + + Scenario: listing shows the pending schedule in posting order + Given an empty comment queue + And the comments "First", "Second" and "Third" are queued + Then listing shows 3 entries with strictly increasing ETAs + + Scenario: listings collapse long comments to one line + Given an empty comment queue + When a comment of 100 "x" characters is put + Then the listed body summarizes to one line of 60 characters + + Scenario: bumping moves a comment to the head of the queue + Given an empty comment queue + And the comments "First", "Second" and "Third" are queued + When the comment "Third" is bumped + Then the queue order is "Third", "First", "Second" + + Scenario: busting moves a comment to the tail of the queue + Given an empty comment queue + And the comments "First", "Second" and "Third" are queued + When the comment "First" is busted + Then the queue order is "Second", "Third", "First" + + Scenario: deleting removes a comment from the queue + Given an empty comment queue + And the comments "First", "Second" and "Third" are queued + When the comment "Second" is deleted + Then the queue order is "First", "Third" + + Scenario: operations on unknown identifiers are rejected + Given an empty comment queue + And the comments "First", "Second" and "Third" are queued + When the unknown identifier "deadbeef" is bumped + Then the daemon reports an unknown identifier error diff --git a/tests/steps/mod.rs b/tests/steps/mod.rs index dfb1509..2d5ab05 100644 --- a/tests/steps/mod.rs +++ b/tests/steps/mod.rs @@ -11,6 +11,8 @@ pub mod listener_steps; pub use listener_steps::ListenerWorld; pub mod packaging_steps; pub use packaging_steps::PackagingWorld; +pub mod queue_steps; +pub use queue_steps::QueueWorld; pub mod release_steps; pub use release_steps::ReleaseWorld; pub mod worker_steps; diff --git a/tests/steps/queue_steps.rs b/tests/steps/queue_steps.rs new file mode 100644 index 0000000..0d74e55 --- /dev/null +++ b/tests/steps/queue_steps.rs @@ -0,0 +1,251 @@ +//! Behavioural test steps for queue management operations. +//! +//! Exercises the daemon's protocol surface — put, list, bump, bust, and +//! del — against a real `SharedQueue`, plus the client's one-line summary +//! rendering for listings. + +use std::collections::HashMap; +use std::sync::Arc; + +use anyhow::Context as _; +use comenq_lib::CommentRequest; +use comenq_lib::protocol::{PendingEntry, Request, Response}; +use comenqd::config::Config; +use comenqd::daemon::SharedQueue; +use cucumber::{World, given, then, when}; +use tempfile::TempDir; +use test_support::temp_config; + +/// Cooldown used by these scenarios, in seconds. +/// +/// Long enough that nothing is ever due during a test run, so listings are +/// stable, and distinct entries have strictly increasing estimated times. +const COOLDOWN_SECONDS: u64 = 600; + +#[derive(Default, World)] +pub struct QueueWorld { + dir: Option, + queue: Option>, + /// Identifier of each queued comment, keyed by its body text. + ids: HashMap, + last_put: Option, + last_response: Option, +} + +impl std::fmt::Debug for QueueWorld { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("QueueWorld").finish() + } +} + +impl QueueWorld { + fn queue(&self) -> anyhow::Result<&Arc> { + self.queue.as_ref().context("queue not initialized") + } + + fn id_for(&self, body: &str) -> anyhow::Result<&str> { + self.ids + .get(body) + .map(String::as_str) + .with_context(|| format!("no queued comment with body '{body}'")) + } + + async fn put(&mut self, body: &str) -> anyhow::Result { + let queue = self.queue()?.clone(); + let response = queue + .execute(Request::Put { + request: CommentRequest { + owner: "octocat".into(), + repo: "hello-world".into(), + pr_number: 7, + body: body.into(), + }, + }) + .await; + let Response::Ok { + entry: Some(entry), .. + } = response + else { + anyhow::bail!("expected put reply with entry, got {response:?}"); + }; + self.ids.insert(body.to_owned(), entry.id.clone()); + Ok(entry) + } + + async fn entries(&self) -> anyhow::Result> { + let response = self.queue()?.execute(Request::List).await; + match response { + Response::Ok { + entries: Some(entries), + .. + } => Ok(entries), + other => anyhow::bail!("expected list reply, got {other:?}"), + } + } +} + +#[given("an empty comment queue")] +fn empty_queue(world: &mut QueueWorld) -> anyhow::Result<()> { + let dir = TempDir::new().context("tempdir")?; + let cfg = Arc::new(Config::from( + temp_config(&dir).with_cooldown(COOLDOWN_SECONDS), + )); + world.queue = Some(SharedQueue::open(cfg).context("open queue")?); + world.dir = Some(dir); + Ok(()) +} + +#[given(regex = r#"^the comments \"(.+)\", \"(.+)\" and \"(.+)\" are queued$"#)] +async fn comments_are_queued( + world: &mut QueueWorld, + first: String, + second: String, + third: String, +) -> anyhow::Result<()> { + for body in [first, second, third] { + world.put(&body).await?; + } + Ok(()) +} + +#[when(regex = r#"^the comment \"(.+)\" is put$"#)] +async fn comment_is_put(world: &mut QueueWorld, body: String) -> anyhow::Result<()> { + let entry = world.put(&body).await?; + world.last_put = Some(entry); + Ok(()) +} + +#[when(regex = r#"^a comment of ([0-9]+) \"(.)\" characters is put$"#)] +async fn long_comment_is_put( + world: &mut QueueWorld, + count: usize, + character: String, +) -> anyhow::Result<()> { + let body = character.repeat(count); + let entry = world.put(&body).await?; + world.last_put = Some(entry); + Ok(()) +} + +#[when(regex = r#"^the comment \"(.+)\" is (bumped|busted|deleted)$"#)] +async fn comment_is_moved( + world: &mut QueueWorld, + body: String, + operation: String, +) -> anyhow::Result<()> { + let id = world.id_for(&body)?.to_owned(); + let request = match operation.as_str() { + "bumped" => Request::Bump { id }, + "busted" => Request::Bust { id }, + "deleted" => Request::Del { id }, + other => anyhow::bail!("unsupported operation '{other}'"), + }; + let response = world.queue()?.execute(request).await; + anyhow::ensure!( + matches!(response, Response::Ok { .. }), + "operation should succeed, got {response:?}" + ); + Ok(()) +} + +#[when(regex = r#"^the unknown identifier \"(.+)\" is bumped$"#)] +async fn unknown_id_is_bumped(world: &mut QueueWorld, id: String) -> anyhow::Result<()> { + let response = world.queue()?.execute(Request::Bump { id }).await; + world.last_response = Some(response); + Ok(()) +} + +#[then("the reply carries an eight character identifier")] +fn reply_has_identifier(world: &mut QueueWorld) -> anyhow::Result<()> { + let entry = world.last_put.as_ref().context("no put reply recorded")?; + assert_eq!(entry.id.len(), 8, "identifier '{}' length", entry.id); + assert!(entry.id.chars().all(|c| c.is_ascii_hexdigit())); + Ok(()) +} + +#[then("the reply reports an immediate ETA")] +fn reply_reports_eta(world: &mut QueueWorld) -> anyhow::Result<()> { + let entry = world.last_put.as_ref().context("no put reply recorded")?; + // Nothing has ever been posted, so the queue head is due immediately. + assert_eq!(entry.eta_seconds, 0); + Ok(()) +} + +#[then("listing shows the same identifier as the put reply")] +async fn listing_matches_put(world: &mut QueueWorld) -> anyhow::Result<()> { + let entry = world.last_put.clone().context("no put reply recorded")?; + let entries = world.entries().await?; + assert_eq!(entries.len(), 1); + let listed = entries.first().context("listing should hold the comment")?; + assert_eq!(listed.id, entry.id); + Ok(()) +} + +#[then(regex = r"^listing shows ([0-9]+) entries with strictly increasing ETAs$")] +async fn listing_shows_schedule(world: &mut QueueWorld, expected: usize) -> anyhow::Result<()> { + let entries = world.entries().await?; + assert_eq!(entries.len(), expected); + for (earlier, later) in entries.iter().zip(entries.iter().skip(1)) { + assert!( + earlier.eta_seconds < later.eta_seconds, + "ETAs should increase: {} then {}", + earlier.eta_seconds, + later.eta_seconds, + ); + } + Ok(()) +} + +#[then(regex = r"^the listed body summarizes to one line of ([0-9]+) characters$")] +async fn listed_body_truncates(world: &mut QueueWorld, limit: usize) -> anyhow::Result<()> { + let entries = world.entries().await?; + let entry = entries.first().context("queue should hold the comment")?; + let summary = comenq::one_line_summary(&entry.body); + assert_eq!(summary.chars().count(), limit); + assert!(summary.ends_with('…')); + assert!(!summary.contains('\n')); + Ok(()) +} + +#[then(regex = r#"^the queue order is \"([^\"]+)\", \"([^\"]+)\", \"([^\"]+)\"$"#)] +async fn queue_order_is_three( + world: &mut QueueWorld, + first: String, + second: String, + third: String, +) -> anyhow::Result<()> { + assert_queue_order(world, &[first, second, third]).await +} + +#[then(regex = r#"^the queue order is \"([^\"]+)\", \"([^\"]+)\"$"#)] +async fn queue_order_is_two( + world: &mut QueueWorld, + first: String, + second: String, +) -> anyhow::Result<()> { + assert_queue_order(world, &[first, second]).await +} + +async fn assert_queue_order(world: &mut QueueWorld, bodies: &[String]) -> anyhow::Result<()> { + let entries = world.entries().await?; + let listed: Vec<&str> = entries.iter().map(|entry| entry.body.as_str()).collect(); + assert_eq!( + listed, + bodies.iter().map(String::as_str).collect::>() + ); + Ok(()) +} + +#[then("the daemon reports an unknown identifier error")] +fn daemon_reports_error(world: &mut QueueWorld) -> anyhow::Result<()> { + match world.last_response.take() { + Some(Response::Error { message }) => { + assert!( + message.contains("deadbeef"), + "error should name the identifier: {message}" + ); + } + other => anyhow::bail!("expected error reply, got {other:?}"), + } + Ok(()) +} From 804f68331f9100de7871970cb64bafbbce2460ce Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 18:20:08 +0200 Subject: [PATCH 6/6] Defer fresh puts by one cooldown unless --now is passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A default `put` previously posted immediately when the queue was idle. Each entry now records a `not_before` floor — its enqueue time plus one full cooldown plus its own sampled flutter — and the schedule takes the later of that floor and the chain-projected due time, so even an idle queue paces a fresh comment and `bump` cannot circumvent the floor. `comenq put --now` (protocol field `immediate`) lifts the floor, restoring the previous post-as-soon-as-possible behaviour. The field defaults off on the wire and in the store, so entries persisted before this change behave like immediate puts. Group the store's put inputs in `PutOptions` (cooldown, flutter ceiling, immediacy), cover the floor with store unit tests and new queue scenarios, and document the behaviour in the design document, man page, and README. --- README.md | 5 +- crates/comenq/src/client.rs | 4 +- crates/comenq/src/lib.rs | 28 ++++++ crates/comenqd/src/queue.rs | 27 +++--- crates/comenqd/src/store.rs | 48 ++++++++-- crates/comenqd/src/store/tests.rs | 147 ++++++++++++++++++++++++------ crates/comenqd/tests/daemon.rs | 4 + crates/comenqd/tests/listener.rs | 2 + docs/comenq-design.md | 13 ++- packaging/man/comenq.1 | 9 +- src/protocol.rs | 21 +++++ tests/features/queue.feature | 6 ++ tests/steps/client_main_steps.rs | 8 +- tests/steps/listener_steps.rs | 1 + tests/steps/queue_steps.rs | 28 +++++- tests/steps/worker_steps.rs | 1 + 16 files changed, 300 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 61812d6..fe1b8f2 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,10 @@ make build Queued requests persist on disk and are posted sequentially by the daemon. `put` prints the comment's deterministic eight-character identifier and an -approximate ETA. The queue can be inspected and reordered by identifier: +approximate ETA. By default a fresh comment waits one full cooldown (plus +its flutter) from enqueue even when the queue is idle; pass `--now` to +post as soon as the queue allows. The queue can be inspected and +reordered by identifier: ```bash comenq list # schedule of pending comments with IDs and ETAs diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index 6cf0987..3e7beb2 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -124,6 +124,7 @@ mod tests { repo_slug: "octocat/hello-world".parse().expect("slug"), pr_number: 1, comment_body: "Hi".into(), + now: false, }, } } @@ -158,13 +159,14 @@ mod tests { run(put_args(socket)).await.expect("run succeeds"); let request = accept.await.expect("join"); - let Request::Put { request } = request else { + let Request::Put { request, immediate } = request else { panic!("expected put request, got {request:?}"); }; assert_eq!(request.owner, "octocat"); assert_eq!(request.repo, "hello-world"); assert_eq!(request.pr_number, 1); assert_eq!(request.body, "Hi"); + assert!(!immediate, "put must default to deferred posting"); } #[tokio::test] diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 69d0568..0ad9de1 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -118,6 +118,10 @@ pub struct Args { #[derive(Debug, Clone, Subcommand)] pub enum Command { /// Enqueue a comment and print its identifier and approximate ETA. + /// + /// By default the comment waits one full cooldown (plus its flutter) + /// from enqueue even when the queue is idle; pass `--now` to post as + /// soon as the queue allows. Put { /// The repository in 'owner/repo' format (e.g., "rust-lang/rust"). repo_slug: RepoSlug, @@ -127,6 +131,11 @@ pub enum Command { /// The body of the comment. It is recommended to quote this argument. comment_body: String, + + /// Post as soon as the queue allows instead of waiting a full + /// cooldown from enqueue. + #[arg(long)] + now: bool, }, /// List pending comments with identifiers and ETAs. List, @@ -157,6 +166,7 @@ impl Command { repo_slug, pr_number, comment_body, + now, } => Request::Put { request: comenq_lib::CommentRequest { owner: repo_slug.owner().to_owned(), @@ -164,6 +174,7 @@ impl Command { pr_number: *pr_number, body: comment_body.clone(), }, + immediate: *now, }, Self::List => Request::List, Self::Bump { id } => Request::Bump { id: id.clone() }, @@ -222,6 +233,7 @@ mod tests { repo_slug, pr_number, comment_body, + now, } = args.command else { panic!("expected put command"); @@ -229,6 +241,22 @@ mod tests { assert_eq!(repo_slug, expected); assert_eq!(pr_number, pr); assert_eq!(comment_body, body); + assert!(!now, "put must default to deferred posting"); + } + + #[test] + fn put_accepts_the_now_flag() { + let args = + Args::try_parse_from(["comenq", "put", "--now", "octocat/hello-world", "1", "Hi"]) + .expect("valid arguments should parse"); + let Command::Put { now, .. } = args.command else { + panic!("expected put command"); + }; + assert!(now); + let comenq_lib::protocol::Request::Put { immediate, .. } = args.command.to_request() else { + panic!("expected put request"); + }; + assert!(immediate); } #[rstest] diff --git a/crates/comenqd/src/queue.rs b/crates/comenqd/src/queue.rs index 7d94001..22764d5 100644 --- a/crates/comenqd/src/queue.rs +++ b/crates/comenqd/src/queue.rs @@ -12,7 +12,7 @@ use comenq_lib::protocol::{Request, Response}; use tokio::sync::{Mutex, Notify}; use crate::config::Config; -use crate::store::{QueueStore, Result as StoreResult, StoredEntry}; +use crate::store::{PutOptions, QueueStore, Result as StoreResult, StoredEntry}; /// Current Unix time in whole seconds. /// @@ -76,17 +76,20 @@ impl SharedQueue { let cooldown = self.cfg.cooldown_period_seconds; let now = unix_now(); let (response, mutated) = match request { - Request::Put { request } => { - let outcome = store - .put(request, self.cfg.cooldown_flutter_seconds, now) - .and_then(|entry| { - let eta = store - .schedule(cooldown, now)? - .into_iter() - .find(|(scheduled, _)| scheduled.id == entry.id) - .map_or(0, |(_, eta)| eta); - Ok(Response::entry(entry.to_pending(eta))) - }); + Request::Put { request, immediate } => { + let options = PutOptions { + cooldown, + flutter_max: self.cfg.cooldown_flutter_seconds, + immediate, + }; + let outcome = store.put(request, &options, now).and_then(|entry| { + let eta = store + .schedule(cooldown, now)? + .into_iter() + .find(|(scheduled, _)| scheduled.id == entry.id) + .map_or(0, |(_, eta)| eta); + Ok(Response::entry(entry.to_pending(eta))) + }); (outcome, true) } Request::List => { diff --git a/crates/comenqd/src/store.rs b/crates/comenqd/src/store.rs index dbb73bf..5362b15 100644 --- a/crates/comenqd/src/store.rs +++ b/crates/comenqd/src/store.rs @@ -36,6 +36,14 @@ pub struct StoredEntry { pub flutter_seconds: u64, /// Unix time the entry was enqueued, in seconds. pub enqueued_at: u64, + /// Earliest Unix time the entry may post. + /// + /// A default `put` sets this one cooldown plus the entry's flutter + /// after enqueue, so even an idle queue paces a fresh comment; an + /// immediate put (and entries persisted before this field existed) + /// leave it at zero. + #[serde(default)] + pub not_before: u64, /// The comment to post. pub request: CommentRequest, } @@ -72,6 +80,18 @@ pub enum StoreError { /// Result alias for store operations. pub type Result = std::result::Result; +/// Scheduling inputs for [`QueueStore::put`]. +#[derive(Debug, Clone, Copy)] +pub struct PutOptions { + /// Cooldown between posts, in seconds. + pub cooldown: u64, + /// Flutter ceiling to sample from, in seconds. + pub flutter_max: u64, + /// Post as soon as the queue allows instead of waiting a full cooldown + /// from enqueue. + pub immediate: bool, +} + /// Filesystem-backed queue of pending comments. #[derive(Debug, Clone)] pub struct QueueStore { @@ -120,21 +140,35 @@ impl QueueStore { /// Enqueue `request` at the tail, sampling its flutter now. /// /// The flutter is fixed at enqueue time so the entry's estimated posting - /// time is stable from the moment it is reported to the client. + /// time is stable from the moment it is reported to the client. By + /// default the entry may not post until one full cooldown plus its + /// flutter after enqueue; `options.immediate` lifts that floor so the + /// entry posts as soon as the queue allows. /// /// Identifiers derive from the request content and enqueue second, so an /// identical request repeated within the same second maps to the same /// identifier; the operation is idempotent and returns the existing /// entry unchanged. - pub fn put(&self, request: CommentRequest, flutter_max: u64, now: u64) -> Result { + pub fn put( + &self, + request: CommentRequest, + options: &PutOptions, + now: u64, + ) -> Result { let id = entry_id(&request, now); if let Ok(existing) = self.find(&id) { return Ok(existing); } - let flutter_seconds = if flutter_max == 0 { + let flutter_seconds = if options.flutter_max == 0 { + 0 + } else { + rand::rng().random_range(0..=options.flutter_max) + }; + let not_before = if options.immediate { 0 } else { - rand::rng().random_range(0..=flutter_max) + now.saturating_add(options.cooldown) + .saturating_add(flutter_seconds) }; let order = self .entries()? @@ -145,6 +179,7 @@ impl QueueStore { order, flutter_seconds, enqueued_at: now, + not_before, request, }; self.write_entry(&entry)?; @@ -226,7 +261,8 @@ impl QueueStore { /// The head is due one full cooldown plus its own flutter after the most /// recent post (immediately when nothing has been posted yet); each /// subsequent entry follows a further cooldown plus its own flutter after - /// the projected posting time of its predecessor. + /// the projected posting time of its predecessor. An entry additionally + /// never posts before its own `not_before` floor. pub fn schedule(&self, cooldown: u64, now: u64) -> Result> { let mut previous_post = self.last_post(); let mut scheduled = Vec::new(); @@ -235,7 +271,7 @@ impl QueueStore { prev.saturating_add(cooldown) .saturating_add(entry.flutter_seconds) }); - let post_at = due.max(now); + let post_at = due.max(entry.not_before).max(now); previous_post = Some(post_at); scheduled.push((entry, post_at.saturating_sub(now))); } diff --git a/crates/comenqd/src/store/tests.rs b/crates/comenqd/src/store/tests.rs index b0eb444..c6ac13f 100644 --- a/crates/comenqd/src/store/tests.rs +++ b/crates/comenqd/src/store/tests.rs @@ -1,6 +1,6 @@ //! Tests for the reorderable persistent queue store. -use super::{QueueStore, StoreError, entry_id}; +use super::{PutOptions, QueueStore, StoreError, entry_id}; use comenq_lib::CommentRequest; use rstest::rstest; use tempfile::TempDir; @@ -18,6 +18,24 @@ fn open_store(dir: &TempDir) -> QueueStore { QueueStore::open(dir.path()).expect("open store") } +/// Options for an immediate put with the given flutter ceiling. +fn immediate(flutter_max: u64) -> PutOptions { + PutOptions { + cooldown: 600, + flutter_max, + immediate: true, + } +} + +/// Options for a default (deferred) put with no flutter. +fn deferred() -> PutOptions { + PutOptions { + cooldown: 600, + flutter_max: 0, + immediate: false, + } +} + fn ids(store: &QueueStore) -> Vec { store .entries() @@ -42,9 +60,15 @@ fn identifiers_are_deterministic_and_eight_characters() { fn put_preserves_arrival_order() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let first = store.put(request("first"), 0, 1000).expect("put first"); - let second = store.put(request("second"), 0, 1001).expect("put second"); - let third = store.put(request("third"), 0, 1002).expect("put third"); + let first = store + .put(request("first"), &immediate(0), 1000) + .expect("put first"); + let second = store + .put(request("second"), &immediate(0), 1001) + .expect("put second"); + let third = store + .put(request("third"), &immediate(0), 1002) + .expect("put third"); assert_eq!(ids(&store), vec![first.id, second.id, third.id]); } @@ -54,11 +78,13 @@ fn put_samples_flutter_within_bounds() { let store = open_store(&dir); for i in 0..50 { let entry = store - .put(request(&format!("body {i}")), 240, 1000 + i) + .put(request(&format!("body {i}")), &immediate(240), 1000 + i) .expect("put entry"); assert!(entry.flutter_seconds <= 240); } - let zero = store.put(request("plain"), 0, 2000).expect("put plain"); + let zero = store + .put(request("plain"), &immediate(0), 2000) + .expect("put plain"); assert_eq!(zero.flutter_seconds, 0); } @@ -66,9 +92,9 @@ fn put_samples_flutter_within_bounds() { fn bump_moves_entry_to_head() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - let b = store.put(request("b"), 0, 1001).expect("put b"); - let c = store.put(request("c"), 0, 1002).expect("put c"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + let b = store.put(request("b"), &immediate(0), 1001).expect("put b"); + let c = store.put(request("c"), &immediate(0), 1002).expect("put c"); store.bump(&c.id).expect("bump c"); assert_eq!(ids(&store), vec![c.id, a.id, b.id]); } @@ -77,8 +103,8 @@ fn bump_moves_entry_to_head() { fn bump_of_head_is_a_no_op() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - let b = store.put(request("b"), 0, 1001).expect("put b"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + let b = store.put(request("b"), &immediate(0), 1001).expect("put b"); store.bump(&a.id).expect("bump head"); assert_eq!(ids(&store), vec![a.id, b.id]); } @@ -87,9 +113,9 @@ fn bump_of_head_is_a_no_op() { fn bust_moves_entry_to_tail() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - let b = store.put(request("b"), 0, 1001).expect("put b"); - let c = store.put(request("c"), 0, 1002).expect("put c"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + let b = store.put(request("b"), &immediate(0), 1001).expect("put b"); + let c = store.put(request("c"), &immediate(0), 1002).expect("put c"); store.bust(&a.id).expect("bust a"); assert_eq!(ids(&store), vec![b.id, c.id, a.id]); } @@ -98,8 +124,8 @@ fn bust_moves_entry_to_tail() { fn del_removes_entry() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - let b = store.put(request("b"), 0, 1001).expect("put b"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + let b = store.put(request("b"), &immediate(0), 1001).expect("put b"); store.del(&a.id).expect("del a"); assert_eq!(ids(&store), vec![b.id]); } @@ -111,7 +137,7 @@ fn del_removes_entry() { fn unknown_ids_are_rejected(#[case] op: fn(&QueueStore) -> super::Result<()>) { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - store.put(request("a"), 0, 1000).expect("put a"); + store.put(request("a"), &immediate(0), 1000).expect("put a"); let err = op(&store).expect_err("unknown id must fail"); assert!(matches!(err, StoreError::UnknownId(id) if id == "deadbeef")); } @@ -120,8 +146,8 @@ fn unknown_ids_are_rejected(#[case] op: fn(&QueueStore) -> super::Result<()>) { fn schedule_starts_immediately_when_never_posted() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - store.put(request("a"), 0, 1000).expect("put a"); - store.put(request("b"), 0, 1001).expect("put b"); + store.put(request("a"), &immediate(0), 1000).expect("put a"); + store.put(request("b"), &immediate(0), 1001).expect("put b"); let schedule = store.schedule(600, 2000).expect("schedule"); let etas: Vec = schedule.iter().map(|(_, eta)| *eta).collect(); // Head posts immediately; the next entry follows one full cooldown @@ -133,8 +159,8 @@ fn schedule_starts_immediately_when_never_posted() { fn schedule_respects_last_post_and_flutter() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - store.put(request("b"), 0, 1001).expect("put b"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + store.put(request("b"), &immediate(0), 1001).expect("put b"); // Pretend `a` was posted at t=1000 by another entry's completion. store.complete(&a.id, 1000).expect("complete a"); let schedule = store.schedule(600, 1100).expect("schedule"); @@ -148,7 +174,7 @@ fn schedule_respects_last_post_and_flutter() { fn complete_records_last_post_and_removes_entry() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); assert_eq!(store.last_post(), None); store.complete(&a.id, 4242).expect("complete a"); assert_eq!(store.last_post(), Some(4242)); @@ -159,8 +185,8 @@ fn complete_records_last_post_and_removes_entry() { fn next_due_returns_the_head() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let a = store.put(request("a"), 0, 1000).expect("put a"); - store.put(request("b"), 0, 1001).expect("put b"); + let a = store.put(request("a"), &immediate(0), 1000).expect("put a"); + store.put(request("b"), &immediate(0), 1001).expect("put b"); let (head, _) = store .next_due(600, 2000) .expect("next_due") @@ -172,7 +198,7 @@ fn next_due_returns_the_head() { fn entries_survive_reopen() { let dir = TempDir::new().expect("tempdir"); let first = open_store(&dir); - let a = first.put(request("a"), 0, 1000).expect("put a"); + let a = first.put(request("a"), &immediate(0), 1000).expect("put a"); drop(first); let reopened = open_store(&dir); assert_eq!(ids(&reopened), vec![a.id]); @@ -182,8 +208,75 @@ fn entries_survive_reopen() { fn identical_put_within_the_same_second_is_idempotent() { let dir = TempDir::new().expect("tempdir"); let store = open_store(&dir); - let first = store.put(request("dup"), 240, 1000).expect("first put"); - let second = store.put(request("dup"), 240, 1000).expect("second put"); + let first = store + .put(request("dup"), &immediate(240), 1000) + .expect("first put"); + let second = store + .put(request("dup"), &immediate(240), 1000) + .expect("second put"); assert_eq!(first, second, "repeat put must return the existing entry"); assert_eq!(ids(&store).len(), 1); } + +#[rstest] +fn deferred_put_waits_a_full_cooldown_when_idle() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let entry = store + .put(request("a"), &deferred(), 1000) + .expect("put deferred"); + assert_eq!(entry.not_before, 1600); + let schedule = store.schedule(600, 1000).expect("schedule"); + let etas: Vec = schedule.iter().map(|(_, eta)| *eta).collect(); + assert_eq!(etas, vec![600], "idle queue must still wait one cooldown"); +} + +#[rstest] +fn immediate_put_bypasses_the_enqueue_floor() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let entry = store + .put(request("a"), &immediate(0), 1000) + .expect("put immediate"); + assert_eq!(entry.not_before, 0); + let schedule = store.schedule(600, 1000).expect("schedule"); + let etas: Vec = schedule.iter().map(|(_, eta)| *eta).collect(); + assert_eq!(etas, vec![0]); +} + +#[rstest] +fn deferred_floor_never_shortens_the_chain_schedule() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let head = store + .put(request("head"), &immediate(0), 1000) + .expect("put head"); + store + .put(request("tail"), &deferred(), 1001) + .expect("put tail"); + // Head posted at t=1000; the tail's chain due time (1000 + 600) is + // later than its own floor (1001 + 600 = 1601)... choose the maximum. + store.complete(&head.id, 1000).expect("complete head"); + let schedule = store.schedule(600, 1002).expect("schedule"); + assert_eq!(schedule.len(), 1); + let (_, eta) = &schedule[0]; + // max(chain 1600, floor 1601) = 1601; now is 1002 → 599 seconds. + assert_eq!(*eta, 599); +} + +#[rstest] +fn deferred_floor_includes_the_entry_flutter() { + let dir = TempDir::new().expect("tempdir"); + let store = open_store(&dir); + let options = PutOptions { + cooldown: 600, + flutter_max: 240, + immediate: false, + }; + let entry = store.put(request("a"), &options, 1000).expect("put"); + assert_eq!( + entry.not_before, + 1600 + entry.flutter_seconds, + "floor must be enqueue + cooldown + sampled flutter" + ); +} diff --git a/crates/comenqd/tests/daemon.rs b/crates/comenqd/tests/daemon.rs index fefd4d6..436bffa 100644 --- a/crates/comenqd/tests/daemon.rs +++ b/crates/comenqd/tests/daemon.rs @@ -66,6 +66,7 @@ async fn seed_queue(queue: &Arc) -> PendingEntry { match queue .execute(Request::Put { request: sample_request(), + immediate: true, }) .await { @@ -154,6 +155,7 @@ async fn handle_client_enqueues_request() { client, &Request::Put { request: sample_request(), + immediate: true, }, ) .await; @@ -203,6 +205,7 @@ async fn run_listener_accepts_connections() -> Result<(), String> { stream, &Request::Put { request: sample_request(), + immediate: true, }, ) .await; @@ -459,6 +462,7 @@ mod worker_tests { pr_number: 1, body: "second".into(), }, + immediate: true, }) .await; diff --git a/crates/comenqd/tests/listener.rs b/crates/comenqd/tests/listener.rs index 549da71..80c6f55 100644 --- a/crates/comenqd/tests/listener.rs +++ b/crates/comenqd/tests/listener.rs @@ -54,6 +54,7 @@ fn payload_of_size(target: usize) -> Vec { pr_number: 0, body: String::new(), }, + immediate: false, }; let base_len = serde_json::to_vec(&base).expect("serialize").len(); let body_len = target - base_len; @@ -64,6 +65,7 @@ fn payload_of_size(target: usize) -> Vec { pr_number: 0, body: "a".repeat(body_len), }, + immediate: false, }; let payload = serde_json::to_vec(&request).expect("serialize"); assert_eq!(payload.len(), target); diff --git a/docs/comenq-design.md b/docs/comenq-design.md index a943e30..506f4ea 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -366,7 +366,9 @@ operation, all sharing a global `--socket` flag (also settable via the - `comenq put `: enqueues a comment and prints its identifier and an approximate ETA, e.g. `Queued 1a2b3c4d for - octocat/hello-world#7 — posts in ~1h 01m`. + octocat/hello-world#7 — posts in ~1h 01m`. By default the comment waits one + full cooldown (plus its flutter) from enqueue even when the queue is idle; + `--now` lifts that floor so the comment posts as soon as the queue allows. - `comenq list`: prints one line per pending comment, in posting order, each showing the identifier, the ETA, the target `owner/repo#pr`, and the @@ -386,7 +388,7 @@ confirmation, or the daemon's error message, once the single reply is received. **ETA semantics.** The estimated time until posting shown by `put` and `list` -reflects two rules enforced by the daemon: +reflects three rules enforced by the daemon: - **Flutter is fixed at enqueue time.** When a comment is enqueued, a random flutter duration (up to `cooldown_flutter_seconds`) is sampled once and @@ -401,6 +403,13 @@ reflects two rules enforced by the daemon: cooldown plus its own flutter). This keeps the reported ETA consistent with what the worker will actually do. +- **A fresh comment waits a cooldown by default.** A default `put` records a + `not_before` floor of its enqueue time plus one full cooldown plus its own + flutter, so even an idle queue paces a new comment. `put --now` sets no + floor, restoring the previous behaviour of posting as soon as the queue + allows. An entry never posts before the later of its floor and its + chain-projected due time, so `bump` cannot circumvent the floor. + ## Section 3: Design of the `comenqd` Daemon The `comenqd` daemon is the heart of the system. It is a stateful, diff --git a/packaging/man/comenq.1 b/packaging/man/comenq.1 index bb4dc60..e896348 100644 --- a/packaging/man/comenq.1 +++ b/packaging/man/comenq.1 @@ -3,6 +3,9 @@ comenq \- enqueue a GitHub pull request comment for the comenqd daemon .SH SYNOPSIS .B comenq put +[ +.B --now +] .I owner/repo .I pr-number .I comment @@ -29,7 +32,11 @@ Enqueue a comment for the given repository (in .I owner/repo format) and pull request. Prints the comment's deterministic eight-character identifier and an approximate ETA. The ETA includes the -random flutter, which is fixed when the comment is enqueued. +random flutter, which is fixed when the comment is enqueued. By default +the comment waits one full cooldown (plus its flutter) from enqueue even +when the queue is idle; pass +.B --now +to post as soon as the queue allows. .TP .B list Print the schedule of pending comments: identifier, ETA, target pull diff --git a/src/protocol.rs b/src/protocol.rs index 5523076..a116fe8 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -16,6 +16,10 @@ pub enum Request { Put { /// The comment to enqueue. request: CommentRequest, + /// Post as soon as the queue allows instead of waiting a full + /// cooldown from enqueue. + #[serde(default)] + immediate: bool, }, /// List pending comments in posting order. List, @@ -136,6 +140,7 @@ mod tests { pr_number: 7, body: "Hi".into(), }, + immediate: false, }; let json = serde_json::to_string(&req).unwrap_or_else(|e| panic!("serialize: {e}")); assert!(json.contains(r#""op":"put""#), "missing op tag: {json}"); @@ -180,6 +185,22 @@ mod tests { } } + #[test] + fn put_defaults_to_deferred_posting() { + let json = concat!( + r#"{"op":"put","request":{"owner":"o","repo":"r","#, + r#""pr_number":1,"body":"b"}}"# + ); + let req: Request = serde_json::from_str(json).unwrap_or_else(|e| panic!("parse: {e}")); + assert!(matches!( + req, + Request::Put { + immediate: false, + .. + } + )); + } + #[test] fn unknown_operation_fails_to_parse() { let result: Result = serde_json::from_str(r#"{"op":"zap"}"#); diff --git a/tests/features/queue.feature b/tests/features/queue.feature index a2bf55b..3cfdca7 100644 --- a/tests/features/queue.feature +++ b/tests/features/queue.feature @@ -4,6 +4,12 @@ Feature: Queue management Given an empty comment queue When the comment "First comment" is put Then the reply carries an eight character identifier + And the reply reports an ETA of one cooldown + + Scenario: an immediate put bypasses the enqueue cooldown + Given an empty comment queue + When the comment "First comment" is put immediately + Then the reply carries an eight character identifier And the reply reports an immediate ETA Scenario: put identifiers reappear in listings diff --git a/tests/steps/client_main_steps.rs b/tests/steps/client_main_steps.rs index 6f5d412..d8a020e 100644 --- a/tests/steps/client_main_steps.rs +++ b/tests/steps/client_main_steps.rs @@ -24,6 +24,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(), + now: false, }, }) } @@ -79,9 +80,14 @@ async fn daemon_receives(world: &mut ClientWorld) -> anyhow::Result<()> { let handle = world.server.take().context("server handle")?; let data = handle.await.context("join")??; let request: Request = serde_json::from_slice(&data).context("parse")?; - let Request::Put { request: req } = request else { + let Request::Put { + request: req, + immediate, + } = request + else { anyhow::bail!("expected put request, got {request:?}"); }; + anyhow::ensure!(!immediate, "plain put must not request immediate posting"); assert_eq!(req.owner, "octocat"); assert_eq!(req.repo, "hello-world"); assert_eq!(req.pr_number, 1); diff --git a/tests/steps/listener_steps.rs b/tests/steps/listener_steps.rs index ea7f2b2..a515c2c 100644 --- a/tests/steps/listener_steps.rs +++ b/tests/steps/listener_steps.rs @@ -96,6 +96,7 @@ async fn client_sends_valid(world: &mut ListenerWorld) -> anyhow::Result<()> { pr_number: 1, body: "b".into(), }, + immediate: false, }; let data = serde_json::to_vec(&request).context("serialize request")?; let response = send_raw(world, &data).await?; diff --git a/tests/steps/queue_steps.rs b/tests/steps/queue_steps.rs index 0d74e55..b85bda9 100644 --- a/tests/steps/queue_steps.rs +++ b/tests/steps/queue_steps.rs @@ -28,6 +28,8 @@ pub struct QueueWorld { queue: Option>, /// Identifier of each queued comment, keyed by its body text. ids: HashMap, + /// Whether the next put bypasses the enqueue cooldown floor. + immediate_puts: bool, last_put: Option, last_response: Option, } @@ -60,6 +62,7 @@ impl QueueWorld { pr_number: 7, body: body.into(), }, + immediate: self.immediate_puts, }) .await; let Response::Ok { @@ -102,9 +105,13 @@ async fn comments_are_queued( second: String, third: String, ) -> anyhow::Result<()> { + // Queue with immediate puts so ordering scenarios are not skewed by + // per-entry enqueue floors. + world.immediate_puts = true; for body in [first, second, third] { world.put(&body).await?; } + world.immediate_puts = false; Ok(()) } @@ -115,6 +122,15 @@ async fn comment_is_put(world: &mut QueueWorld, body: String) -> anyhow::Result< Ok(()) } +#[when(regex = r#"^the comment \"(.+)\" is put immediately$"#)] +async fn comment_is_put_immediately(world: &mut QueueWorld, body: String) -> anyhow::Result<()> { + world.immediate_puts = true; + let entry = world.put(&body).await?; + world.immediate_puts = false; + world.last_put = Some(entry); + Ok(()) +} + #[when(regex = r#"^a comment of ([0-9]+) \"(.)\" characters is put$"#)] async fn long_comment_is_put( world: &mut QueueWorld, @@ -166,11 +182,21 @@ fn reply_has_identifier(world: &mut QueueWorld) -> anyhow::Result<()> { #[then("the reply reports an immediate ETA")] fn reply_reports_eta(world: &mut QueueWorld) -> anyhow::Result<()> { let entry = world.last_put.as_ref().context("no put reply recorded")?; - // Nothing has ever been posted, so the queue head is due immediately. + // Nothing has ever been posted and the floor was bypassed, so the + // queue head is due immediately. assert_eq!(entry.eta_seconds, 0); Ok(()) } +#[then("the reply reports an ETA of one cooldown")] +fn reply_reports_cooldown_eta(world: &mut QueueWorld) -> anyhow::Result<()> { + let entry = world.last_put.as_ref().context("no put reply recorded")?; + // A default put waits one full cooldown from enqueue even though the + // queue is idle (flutter is zero in these scenarios). + assert_eq!(entry.eta_seconds, COOLDOWN_SECONDS); + Ok(()) +} + #[then("listing shows the same identifier as the put reply")] async fn listing_matches_put(world: &mut QueueWorld) -> anyhow::Result<()> { let entry = world.last_put.clone().context("no put reply recorded")?; diff --git a/tests/steps/worker_steps.rs b/tests/steps/worker_steps.rs index c6cdfc8..636cf3e 100644 --- a/tests/steps/worker_steps.rs +++ b/tests/steps/worker_steps.rs @@ -93,6 +93,7 @@ async fn queued_request(world: &mut WorkerWorld) -> anyhow::Result<()> { pr_number: 1, body: "b".into(), }, + immediate: true, }) .await; anyhow::ensure!(