diff --git a/apps/piquelctl/src/cli.rs b/apps/piquelctl/src/cli.rs index 946527575..e23730da4 100644 --- a/apps/piquelctl/src/cli.rs +++ b/apps/piquelctl/src/cli.rs @@ -153,6 +153,19 @@ pub(crate) struct Cli { #[arg(long, global = true, help_heading = GLOBAL)] pub(crate) noninteractive: bool, + /// Make a change to an application, environment or preview safe to run + /// again: it is made only once. + /// + /// Run the same command with the same ID, such as after losing the + /// connection, and the daemon returns the earlier result instead of + /// making the change again, within 24 hours. Secret, notification, + /// account, token and sign-in commands, and `preview prune`, ignore it, + /// so rerunning one makes its change again. Use a new ID, of up to 128 letters, digits, `-`, + /// `_`, `.` or `:`, for each command you mean to run. Defaults to + /// `PIQUELD_REQUEST_ID`, else a new ID each time. + #[arg(long, global = true, value_name = "ID", value_parser = parse_request_id, help_heading = GLOBAL)] + pub(crate) request_id: Option, + #[command(subcommand)] pub(crate) command: Command, } @@ -674,6 +687,16 @@ pub(crate) struct OperationArgs { pub(crate) no_wait: bool, } +/// Accepts a request ID the daemon accepts: 1 to 128 ASCII letters, digits, +/// `-`, `_`, `.` or `:`. +pub(crate) fn parse_request_id(value: &str) -> std::result::Result { + if piqueld_client::is_request_id(value) { + Ok(value.to_owned()) + } else { + Err("use 1 to 128 letters, digits, `-`, `_`, `.` or `:`".into()) + } +} + /// Parses a positive integer duration with an optional unit (`ms`, `s`, `m`, `h`, /// `d`; bare numbers are seconds). Used for `--timeout`, `PIQUELD_TIMEOUT`, /// profiles, and the `analytics` window. diff --git a/apps/piquelctl/src/commands.rs b/apps/piquelctl/src/commands.rs index ad93d523e..761665e23 100644 --- a/apps/piquelctl/src/commands.rs +++ b/apps/piquelctl/src/commands.rs @@ -204,8 +204,9 @@ async fn build_logs( /// Builds the API client from resolved connection settings: TCP for `--url`, else the /// Unix socket (default path if unset), plus insecure-HTTP opt-in, saved or -/// `PIQUELD_TOKEN` credentials, the per-request timeout, and a fresh request ID. -/// Fails on invalid endpoint input before any request is sent. +/// `PIQUELD_TOKEN` credentials, the per-request timeout, and the request ID: +/// `--request-id`, `PIQUELD_REQUEST_ID`, or a fresh one. Fails on invalid +/// endpoint input or request ID before any request is sent. pub(crate) fn build_client(cli: &Cli) -> Result { let client = match &cli.url { Some(url) => Client::tcp(url)?, @@ -217,9 +218,17 @@ pub(crate) fn build_client(cli: &Cli) -> Result { client }; let client = crate::auth::Credentials::attach(cli, client)?; - Ok(client - .with_timeout(cli.timeout) - .with_request_id(uuid::Uuid::now_v7().to_string())) + let request_id = match (&cli.request_id, std::env::var("PIQUELD_REQUEST_ID")) { + (Some(id), _) => id.clone(), + (None, Ok(id)) => crate::cli::parse_request_id(&id).map_err(|problem| { + CliError::new( + ErrorKind::Input, + format!("invalid PIQUELD_REQUEST_ID: {problem}"), + ) + })?, + (None, Err(_)) => uuid::Uuid::now_v7().to_string(), + }; + Ok(client.with_timeout(cli.timeout).with_request_id(request_id)) } /// Reports daemon status together with the transport that was used. diff --git a/apps/piquelctl/src/snapshot.rs b/apps/piquelctl/src/snapshot.rs index fede4d4f3..ff841a769 100644 --- a/apps/piquelctl/src/snapshot.rs +++ b/apps/piquelctl/src/snapshot.rs @@ -216,13 +216,22 @@ impl Worktree { )); } let message = format!("piquelctl snapshot of {branch}"); + // Dated like `HEAD`, so the same worktree always makes the same + // commit, and rerunning an upload with its `--request-id` replays. + let date = Git::run( + Git::command(&self.root).args(["show", "-s", "--format=%cd", "--date=raw", &head]), + "read HEAD's date", + ) + .await?; let commit = Git::run( snapshot .staged(&["commit-tree", &tree, "-p", &head, "-m", &message]) .env("GIT_AUTHOR_NAME", "piquelctl") .env("GIT_AUTHOR_EMAIL", "snapshot@piquelctl.invalid") + .env("GIT_AUTHOR_DATE", &date) .env("GIT_COMMITTER_NAME", "piquelctl") - .env("GIT_COMMITTER_EMAIL", "snapshot@piquelctl.invalid"), + .env("GIT_COMMITTER_EMAIL", "snapshot@piquelctl.invalid") + .env("GIT_COMMITTER_DATE", &date), "commit the snapshot", ) .await?; @@ -759,6 +768,19 @@ mod tests { root.join("work") } + /// The same worktree snapshots to the same commit, so a rerun can replay. + #[tokio::test] + async fn snapshots_of_an_unchanged_worktree_are_the_same_commit() { + let root = tempfile::tempdir().unwrap(); + let work = fixture(root.path()); + let worktree = Worktree::at(&work).await.unwrap(); + let first = worktree.snapshot("feat/x", None).await.unwrap().commit; + // Commit dates have one-second resolution. + tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + let second = worktree.snapshot("feat/x", None).await.unwrap().commit; + assert_eq!(first, second); + } + #[tokio::test] async fn snapshots_include_untracked_files_and_leave_the_repository_untouched() { let root = tempfile::tempdir().unwrap(); diff --git a/apps/piquelctl/tests/cli.rs b/apps/piquelctl/tests/cli.rs index a7b565f9a..eccd5a2ae 100644 --- a/apps/piquelctl/tests/cli.rs +++ b/apps/piquelctl/tests/cli.rs @@ -876,6 +876,8 @@ fn apply_deploy_retries_a_dropped_request_with_the_same_identity() { let output = run( &server, &[ + "--request-id", + "agent-run-7", "app", "apply", "--deploy", @@ -901,6 +903,8 @@ fn apply_deploy_retries_a_dropped_request_with_the_same_identity() { .headers .get("idempotency-key") .expect("command has a replay identity"); + // Sent as is, so rerunning the command replays it. + assert_eq!(key, "agent-run-7"); assert_eq!(Some(key), second.headers.get("idempotency-key")); } diff --git a/apps/piqueld/src/api/service/mod.rs b/apps/piqueld/src/api/service/mod.rs index 8e83e3266..62ca8d27b 100644 --- a/apps/piqueld/src/api/service/mod.rs +++ b/apps/piqueld/src/api/service/mod.rs @@ -158,14 +158,7 @@ impl ApplicationService { if !force && mutation.lacks_precondition(expected_generation) { return Err(ApplicationError::PreconditionRequired); } - // Request IDs are idempotency keys: 1–128 chars of `[A-Za-z0-9-_.:]`. - if request_id.is_some_and(|id| { - id.is_empty() - || id.len() > 128 - || !id - .bytes() - .all(|b| b.is_ascii_alphanumeric() || b"-_.:".contains(&b)) - }) { + if request_id.is_some_and(|id| !piqueld_core::is_request_id(id)) { return Err(StoreError::InvalidInput.into()); } if let Mutation::Rename { name, .. } = mutation diff --git a/apps/piqueld/src/api/service/promotions.rs b/apps/piqueld/src/api/service/promotions.rs index 3962ea4cc..590fd77ab 100644 --- a/apps/piqueld/src/api/service/promotions.rs +++ b/apps/piqueld/src/api/service/promotions.rs @@ -55,13 +55,7 @@ impl ApplicationService { Self::check_submission(&request(None), expected_generation, force, request_id)?; let replayed = self .store - .replay( - actor, - &request(None), - expected_generation, - force, - request_id, - ) + .replay(actor, &request(None), force, request_id) .await?; if let Some(response) = replayed { return Self::promotion_response(response); diff --git a/apps/piqueld/src/api/service/restores.rs b/apps/piqueld/src/api/service/restores.rs index 995dfac36..2bbbab066 100644 --- a/apps/piqueld/src/api/service/restores.rs +++ b/apps/piqueld/src/api/service/restores.rs @@ -29,7 +29,7 @@ impl ApplicationService { // A repeated request returns what it got, whatever changed since. if let Some(response) = self .store - .replay(actor, &request, None, false, request_id) + .replay(actor, &request, false, request_id) .await? { return Self::restore_response(response); diff --git a/apps/piqueld/tests/api_contract/acceptance.rs b/apps/piqueld/tests/api_contract/acceptance.rs index b42f693d6..b0a46b99c 100644 --- a/apps/piqueld/tests/api_contract/acceptance.rs +++ b/apps/piqueld/tests/api_contract/acceptance.rs @@ -81,6 +81,35 @@ async fn generations_deploy_reconcile_and_event_pagination_share_the_http_contra ); } +/// Rerunning a command with its request ID returns what the first run got, +/// even though the rerun inspected the preconditions again, after the first +/// run changed them, and never applies the change twice. Other content under +/// the same ID still conflicts. +#[tokio::test] +async fn a_rerun_with_the_same_request_id_replays_its_acceptance() { + let temp = tempfile::tempdir().unwrap(); + let api = AcceptanceApi::start(&temp).await; + let request = new_application(); + let keyed = api.client.clone().with_request_id("agent-run-7"); + let accepted = keyed.apply_and_deploy(&request).await.unwrap(); + let mut rerun = request.clone(); + rerun.expected_generation = Some(accepted.generation); + rerun.expected_application_id = Some(accepted.application_id.clone()); + let replayed = keyed.apply_and_deploy(&rerun).await.unwrap(); + assert_eq!(replayed.operation_id, accepted.operation_id); + assert_eq!( + api.client + .application(&accepted.application_id) + .await + .unwrap() + .generation, + accepted.generation + ); + rerun.manifest.spec.services[0].replicas = 2.into(); + let error = keyed.apply_and_deploy(&rerun).await.unwrap_err(); + assert_eq!(api_error(error).code, "request_id_conflict"); +} + #[tokio::test] async fn acceptance_receipts_survive_restart_and_supersession() { let temp = tempfile::tempdir().unwrap(); diff --git a/crates/piqueld-client/src/lib.rs b/crates/piqueld-client/src/lib.rs index 6cd51d2a2..c175e54d8 100644 --- a/crates/piqueld-client/src/lib.rs +++ b/crates/piqueld-client/src/lib.rs @@ -97,7 +97,7 @@ pub use piqueld_core::planner::{ActionReason, ActionRisk}; pub use piqueld_core::{ ApplicationId, ApplicationName, BuildFingerprint, BuildInputs, EnvironmentId, Release, ReleaseAvailability, ReleaseId, ResolvedSource, RouteName, ServiceImage, ServiceName, - TomlDiagnostic, ValidatedApplication, ValidationError, ValidationErrors, + TomlDiagnostic, ValidatedApplication, ValidationError, ValidationErrors, is_request_id, }; pub use piqueld_core::{ ApplicationState, Convergence, Event, EventActor, Operation, OperationKind, OperationState, diff --git a/crates/piqueld-core/src/lib.rs b/crates/piqueld-core/src/lib.rs index a60664a1b..9ab72c707 100644 --- a/crates/piqueld-core/src/lib.rs +++ b/crates/piqueld-core/src/lib.rs @@ -48,7 +48,7 @@ pub use names::{ ApplicationName, ApplicationNameError, EnvironmentName, EnvironmentNameError, GitBranch, GitBranchError, JobName, JobNameError, PreviewSlot, PreviewSlotError, PreviewSlug, PreviewSlugError, RouteName, RouteNameError, ServiceName, ServiceNameError, VolumeName, - VolumeNameError, + VolumeNameError, is_request_id, }; pub use operation::{ApplicationState, Operation, OperationKind, OperationState}; pub use planner::{ diff --git a/crates/piqueld-core/src/names.rs b/crates/piqueld-core/src/names.rs index 6db210ec8..e380e707f 100644 --- a/crates/piqueld-core/src/names.rs +++ b/crates/piqueld-core/src/names.rs @@ -212,6 +212,16 @@ pub fn is_label(value: &str, lengths: RangeInclusive) -> bool { .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-') } +/// Whether `value` can be a request ID, the `Idempotency-Key` of a +/// mutation: 1 to 128 ASCII letters, digits, `-`, `_`, `.` or `:`. +#[must_use] +pub fn is_request_id(value: &str) -> bool { + (1..=128).contains(&value.len()) + && value + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || b"-_.:".contains(&byte)) +} + /// [`is_label`] without uppercase letters: the form of piqueld's IDs, names, /// and hostnames. pub(crate) fn is_lowercase_label(value: &str, lengths: RangeInclusive) -> bool { diff --git a/crates/piqueld-store/src/acceptance.rs b/crates/piqueld-store/src/acceptance.rs index ce70b4f2f..024f70de2 100644 --- a/crates/piqueld-store/src/acceptance.rs +++ b/crates/piqueld-store/src/acceptance.rs @@ -10,6 +10,16 @@ use piqueld_core::{ApplicationId, EnvironmentId, EnvironmentName}; use sha2::{Digest, Sha256}; use sqlx::{Sqlite, SqliteConnection, Transaction}; +/// A save as fingerprinted: its mutation without the inspected application +/// ID, in `Mutation::Save`'s serialized form. +#[derive(serde::Serialize)] +#[serde(tag = "kind", rename = "save")] +struct SaveIdentity<'a> { + application: &'a piqueld_core::manifest::ApplicationTemplate, + expected_application_id: Option, + deploy: bool, +} + /// Application deletion as serialized before environments existed, in its /// original field order, so its request fingerprint still matches. #[derive(serde::Serialize)] @@ -81,14 +91,14 @@ impl Store { .fetch_one(&mut *tx) .await .map_err(StoreError::database)?; - let fingerprint = Self::mutation_fingerprint(&mutation, expected_generation, force)?; + let fingerprint = Self::mutation_fingerprint(&mutation, force)?; let checked = Self::check_request_on( &mut tx, actor, &mutation, [ Some(&fingerprint), - Self::legacy_fingerprint(&mutation, expected_generation, force)?.as_ref(), + Self::legacy_fingerprint(&mutation, force)?.as_ref(), ], request_id, now, @@ -171,12 +181,11 @@ impl Store { &self, actor: Actor<'_>, mutation: &Mutation, - expected_generation: Option, force: bool, request_id: Option<&str>, ) -> Result, StoreError> { let mut tx = self.snapshot().await?; - let fingerprint = Self::mutation_fingerprint(mutation, expected_generation, force)?; + let fingerprint = Self::mutation_fingerprint(mutation, force)?; let checked = Self::check_request_on( &mut tx, actor, @@ -346,18 +355,37 @@ impl Store { Ok(matches!(target, Target::New)) } - /// Hashes the full request (mutation, precondition, and `force`) so a reused - /// request ID can be told apart from an exact retry. - fn mutation_fingerprint( - mutation: &impl serde::Serialize, - expected_generation: Option, - force: bool, - ) -> Result { + /// Hashes the request so a reused request ID can be told apart from a + /// rerun of the same command. It covers the mutation and `force`, but + /// not the preconditions the caller inspected beforehand, the expected + /// generation and application ID: a rerun inspects them again after the + /// first run changed them, and must still replay. + fn mutation_fingerprint(mutation: &Mutation, force: bool) -> Result { + match mutation { + Mutation::Save { + application, + expected_application_id: Some(_), + deploy, + } => Self::fingerprint( + &SaveIdentity { + application: application.as_ref(), + expected_application_id: None, + deploy: *deploy, + }, + force, + ), + mutation => Self::fingerprint(mutation, force), + } + } + + /// Hashes `request` and `force`. `null` stands where the expected + /// generation once was, so receipts of requests sent without one keep + /// their fingerprint. + fn fingerprint(request: &impl serde::Serialize, force: bool) -> Result { Ok(format!( "{:x}", Sha256::digest( - serde_json::to_vec(&(mutation, expected_generation, force)) - .map_err(StoreError::corrupt)? + serde_json::to_vec(&(request, None::, force)).map_err(StoreError::corrupt)? ) )) } @@ -365,11 +393,7 @@ impl Store { /// Fingerprints the form a request had before environments existed, when /// it differs, so receipts accepted by an older daemon still replay. /// Deleting an application was then `{"kind":"delete","id":...}`. - fn legacy_fingerprint( - mutation: &Mutation, - expected_generation: Option, - force: bool, - ) -> Result, StoreError> { + fn legacy_fingerprint(mutation: &Mutation, force: bool) -> Result, StoreError> { let Mutation::DeleteApplication { id, environments } = mutation else { return Ok(None); }; @@ -377,7 +401,7 @@ impl Store { return Ok(None); } let legacy = LegacyDelete { kind: "delete", id }; - Self::mutation_fingerprint(&legacy, expected_generation, force).map(Some) + Self::fingerprint(&legacy, force).map(Some) } /// Returns the stored response for an unexpired receipt of `owner` (the @@ -1106,7 +1130,9 @@ mod tests { async fn application_deletions_accepted_before_environments_replay() { let (_directory, store) = Store::temporary().await; let id = ApplicationId::parse("app-legacy-01").unwrap(); - let original = Sha256::digest(br#"[{"kind":"delete","id":"app-legacy-01"},3,false]"#); + // Fingerprinted without its expected generation, which a rerun + // inspects again. + let original = Sha256::digest(br#"[{"kind":"delete","id":"app-legacy-01"},null,false]"#); let fingerprint = format!("{original:x}"); let response = r#"{"Operation":{"operation_id":"operation-1","application_id":"app-legacy-01","generation":4}}"#; sqlx::query!( @@ -1138,7 +1164,9 @@ mod tests { assert!(!wake); assert_eq!(replayed.operation_id, "operation-1"); assert_eq!(replayed.environment_id, "app-legacy-01"); - assert!(matches!( + // Another inspected generation still replays; forcing is another + // request. + assert!( store .accept( crate::Actor::Daemon, @@ -1147,6 +1175,18 @@ mod tests { false, Some("legacy-delete") ) + .await + .is_ok() + ); + assert!(matches!( + store + .accept( + crate::Actor::Daemon, + delete(), + None, + true, + Some("legacy-delete") + ) .await, Err(StoreError::ReplayConflict) )); @@ -1471,7 +1511,7 @@ mod tests { let deploy = Mutation::deploy(EnvironmentId::parse("input-app").unwrap()); let original = Sha256::digest(br#"[{"kind":"deploy","id":"input-app"},null,false]"#); assert_eq!( - Store::mutation_fingerprint(&deploy, None, false).unwrap(), + Store::mutation_fingerprint(&deploy, false).unwrap(), format!("{original:x}") ); } diff --git a/docs/user/reference/api.md b/docs/user/reference/api.md index a72b2c873..eec5f1de2 100644 --- a/docs/user/reference/api.md +++ b/docs/user/reference/api.md @@ -493,11 +493,14 @@ for each missing image, since promoting it would fail. Plans of an earlier `release` work for any environment. Mutation endpoints accept an optional `Idempotency-Key` (1–128 ASCII letters, -digits, `-`, `_`, `.`, or `:`; 400 `invalid_request` otherwise). The CLI generates one UUID per command and reuses -it for transport retries. The daemon atomically stores the accepted response and a -fingerprint of the normalized request in SQLite for 24 hours. A matching retry -returns the original response before checking the current generation, even after -restart, failure, or supersession. It never restarts the operation. Different input, +digits, `-`, `_`, `.`, or `:`; 400 `invalid_request` otherwise). The CLI generates one UUID per command, or +sends `--request-id`, and reuses it for transport retries. The daemon atomically +stores the accepted response and a fingerprint of the normalized request in +SQLite for 24 hours. The fingerprint leaves out the preconditions a caller +inspects beforehand, `X-Expected-Generation` and the expected application ID, +so a rerun that inspected the application again still matches. A matching +request returns the original response before checking the current generation, +even after restart, failure, or supersession. It never restarts the operation. Different input, or another account, under the same key returns 409 `request_id_conflict`. Expired keys are treated as new requests subject to current preconditions. Receipts contain no manifests or raw request bodies, and do not guarantee exactly-once Docker effects. Force is diff --git a/docs/user/reference/cli-commands.md b/docs/user/reference/cli-commands.md index a2d1abd11..23abf09be 100644 --- a/docs/user/reference/cli-commands.md +++ b/docs/user/reference/cli-commands.md @@ -56,6 +56,9 @@ Global options: - `--noninteractive`: Never prompt, even on a terminal. Commands that ask for confirmation then fail unless you pass `--yes`. +- `--request-id `: Make a change to an application, environment or preview safe to run again: it is made only once. + + Run the same command with the same ID, such as after losing the connection, and the daemon returns the earlier result instead of making the change again, within 24 hours. Secret, notification, account, token and sign-in commands, and `preview prune`, ignore it, so rerunning one makes its change again. Use a new ID, of up to 128 letters, digits, `-`, `_`, `.` or `:`, for each command you mean to run. Defaults to `PIQUELD_REQUEST_ID`, else a new ID each time. Commands: diff --git a/docs/user/reference/cli.md b/docs/user/reference/cli.md index e34a711c1..af6520f6d 100644 --- a/docs/user/reference/cli.md +++ b/docs/user/reference/cli.md @@ -954,11 +954,20 @@ application currently has the name, or creates it if absent. Validation, resourc ownership, and rename busy/name-collision checks still apply. `app show` reports intent and active target generations and separate runtime health. -The CLI retains one automatic transport retry, using the same command UUID in -`Idempotency-Key`. SQLite receipts replay the original acceptance response for -24 hours across daemon restarts. Replays do not restart failed or superseded work; -a separately invoked command gets a new UUID. This includes forced requests: -a transport retry cannot overwrite changes accepted after the original command. +Each command has a request ID, a new UUID unless `--request-id ID` or +`PIQUELD_REQUEST_ID` gives one, sent as `Idempotency-Key` with its change; the +CLI's one automatic transport retry sends the same. Only changes to +applications, environments and previews honor it: secret, notification, +account, token and sign-in commands, and `preview prune`, make their change +again when rerun. SQLite receipts replay the +original acceptance response for 24 hours across daemon restarts, to the same +account. So when a command loses its connection, run it again with the same +`--request-id`: it returns the earlier result and never repeats the change, +even though the rerun inspects the application again and finds it changed. +The same ID with a different change, or with `--force` added, fails with +`request_id_conflict`. Replays do not restart failed or superseded work, and a +command run without the same ID gets a new one. This includes forced requests: +a retry cannot overwrite changes accepted after the original command. Rename checks the inspected generation and name availability. It rejects pending or running operations and deletion intent. It preserves identity, services,