diff --git a/.claude/skills/verify-removal-marks/SKILL.md b/.claude/skills/verify-removal-marks/SKILL.md index f9b50eabe7..2ab7fd99da 100644 --- a/.claude/skills/verify-removal-marks/SKILL.md +++ b/.claude/skills/verify-removal-marks/SKILL.md @@ -74,14 +74,15 @@ Out of scope: a Project-target mark and its member Environments, two sweeps side Each model step is a copy of code at the sync commit. Reread these when a diff touches them: - `core/crates/ployz-store/src/removal/due.rs`: `sweep`, `settle`, `finish`, `close` and its `Cancelled` early return. -- `core/crates/ployz-store/src/removal/mark.rs`: `live`, `of`, `save` and its `ON CONFLICT` update, `finish` and its `WHERE state = 'removing'`. +- `core/crates/ployz-store/src/removal/mark.rs`: `live`, `of`, `save` and its `ON CONFLICT` update, `finish` and its `WHERE state = 'removing'`, and the helpers other modules call: `unmarked` and `marks_holding`, which return SQL they splice in, and `drop_live`, which deletes the live mark. - `core/crates/ployz-store/src/removal/mod.rs`: `cancel_removal`, `stop`, `lock_project`, `on_servers`, `ran`, `purge`. - `core/crates/ployz-store/src/scope.rs`: `environment`, `lock`, `lock_all`, `lock_id`, `owned`, `load_by_id`. - `core/crates/ployz-store/src/deployment/lifecycle.rs`: `cancel`, `record` and its `Applied`/`Failed` choice, `end` and its `(Cancelling, Failed)` to `Cancelled` mapping. - `core/crates/ployz-store/src/deployment/history.rs`: `in_flight`, `last_ran`. - `core/crates/ployz-store/src/storage/pg.rs`: `begin` and its READ COMMITTED level, `TIMEOUT`. - `core/crates/ployz-store/src/storage/sqlite.rs`: `transaction` and `BEGIN IMMEDIATE`. -- `core/crates/ployz-store/src/pull_request/mod.rs`: the calls to `removal::close` and `removal::sweep`. +- `core/crates/ployz-store/src/pull_request/mod.rs`: the calls to `removal::close` and `removal::sweep`, `mark::unmarked`, `mark::drop_live` at the merge undo, and `idle_facts`. +- `core/crates/ployz-store/src/branch/follow.rs` and `branch/proposal.rs`: `mark::unmarked`. - `dashboard/src/modules/config-store/store-github.inngest.ts` and `store-deployment.inngest.ts`: when Cloud sweeps. Sync commit: `379d927a88e00ad82aafa355f828f3b4a0bf5451`, the commit that made `finish` conditional, checked against it. Code in the list above that changed after it has not been rechecked. diff --git a/core/crates/ployz-core/tests/environment_intent.rs b/core/crates/ployz-core/tests/environment_intent.rs index cc93d25dbb..445ab69b53 100644 --- a/core/crates/ployz-core/tests/environment_intent.rs +++ b/core/crates/ployz-core/tests/environment_intent.rs @@ -236,3 +236,13 @@ fn configs_refuse_bare_references_and_colliding_mounts() { assert!(parse_environment_intent(value).is_err(), "{mode}"); } } + +#[test] +fn a_stored_service_slug_that_is_not_a_service_name_still_decodes() { + for slug in ["Web", "web_app", "web-", &"a".repeat(64)] { + let mut value = intent(); + *value.pointer_mut("/services/0/slug").unwrap() = json!(slug); + let parsed = parse_environment_intent(value).unwrap(); + assert_eq!(parsed.services.first().unwrap().slug, slug); + } +} diff --git a/core/crates/ployz-store/src/branch/follow.rs b/core/crates/ployz-store/src/branch/follow.rs index 338136869c..6facdd6d1c 100644 --- a/core/crates/ployz-store/src/branch/follow.rs +++ b/core/crates/ployz-store/src/branch/follow.rs @@ -5,6 +5,7 @@ use super::*; use crate::Trusted; +use crate::removal::mark; use ployz_core::RpcErrorCode; /// A Parent's deployed value that followed into its Branch but isn't staged there: @@ -52,9 +53,11 @@ pub(crate) fn follow( ) -> Result<(), RpcError> { let trusted = &Trusted::default(); let branches = tx.query( - "SELECT environment_id, organization_id FROM config_environment_branch b \ - WHERE parent_id = ?1 AND NOT EXISTS (SELECT 1 FROM config_removal_mark m \ - WHERE m.environment_id = b.environment_id AND m.state = 'removing') ORDER BY environment_id", + &format!( + "SELECT environment_id, organization_id FROM config_environment_branch b \ + WHERE parent_id = ?1 AND {} ORDER BY environment_id", + mark::unmarked("b.environment_id") + ), &[parent.as_str().into()], )?; if branches.is_empty() || lineages.is_empty() { diff --git a/core/crates/ployz-store/src/branch/proposal.rs b/core/crates/ployz-store/src/branch/proposal.rs index 100b4e3f09..651ccef984 100644 --- a/core/crates/ployz-store/src/branch/proposal.rs +++ b/core/crates/ployz-store/src/branch/proposal.rs @@ -10,6 +10,7 @@ use super::*; use crate::Trusted; use crate::id::{ProposalId, RepositoryId}; use crate::pull_request::PullRequestRef; +use crate::removal::mark; use crate::storage::Param; /// Remove a proposal from a draft: what it still owns there goes back to what the @@ -903,23 +904,25 @@ pub(crate) fn included( environment: &EnvironmentId, ) -> Result, RpcError> { let rows = tx.query( - "SELECT p.id, p.source_environment_id, p.source_name, p.source_revision, p.last_sync, \ - p.repository_id, p.number, \ - (SELECT COUNT(*) FROM config_sync_arrival a \ - WHERE a.environment_id = p.environment_id AND a.proposal_id = p.id), \ - COALESCE(r.name, e.name), COALESCE(r.working_revision, e.working_revision), \ - r.id \ - FROM config_proposal p JOIN config_environment d ON d.id = p.environment_id \ - LEFT JOIN config_environment e ON e.id = p.source_environment_id \ - LEFT JOIN (SELECT q.environment_id AS id, q.repository_id, q.number, \ - v.project_id, v.name, v.working_revision \ - FROM config_pr_environment q JOIN config_environment v ON v.id = q.environment_id \ - JOIN config_environment_branch b ON b.environment_id = q.environment_id \ - WHERE NOT EXISTS (SELECT 1 FROM config_removal_mark m \ - WHERE m.environment_id = b.environment_id AND m.state = 'removing')) r \ - ON r.id <> p.source_environment_id AND r.repository_id = p.repository_id \ - AND r.number = p.number AND r.project_id = d.project_id \ - WHERE p.environment_id = ?1 ORDER BY p.source_name, p.id", + &format!( + "SELECT p.id, p.source_environment_id, p.source_name, p.source_revision, p.last_sync, \ + p.repository_id, p.number, \ + (SELECT COUNT(*) FROM config_sync_arrival a \ + WHERE a.environment_id = p.environment_id AND a.proposal_id = p.id), \ + COALESCE(r.name, e.name), COALESCE(r.working_revision, e.working_revision), \ + r.id \ + FROM config_proposal p JOIN config_environment d ON d.id = p.environment_id \ + LEFT JOIN config_environment e ON e.id = p.source_environment_id \ + LEFT JOIN (SELECT q.environment_id AS id, q.repository_id, q.number, \ + v.project_id, v.name, v.working_revision \ + FROM config_pr_environment q JOIN config_environment v ON v.id = q.environment_id \ + JOIN config_environment_branch b ON b.environment_id = q.environment_id \ + WHERE {}) r \ + ON r.id <> p.source_environment_id AND r.repository_id = p.repository_id \ + AND r.number = p.number AND r.project_id = d.project_id \ + WHERE p.environment_id = ?1 ORDER BY p.source_name, p.id", + mark::unmarked("b.environment_id") + ), &[environment.as_str().into()], )?; let mut included = Vec::with_capacity(rows.len()); diff --git a/core/crates/ployz-store/src/pull_request/mod.rs b/core/crates/ployz-store/src/pull_request/mod.rs index 5d22685871..c2bca3a09f 100644 --- a/core/crates/ployz-store/src/pull_request/mod.rs +++ b/core/crates/ployz-store/src/pull_request/mod.rs @@ -46,6 +46,7 @@ use crate::deployment::{self, DeploymentSummary}; use crate::error; use crate::id::{EnvironmentId, EnvironmentName, ProjectName}; use crate::project::ProjectSummary; +use crate::removal::mark; use crate::review::history::NotSetVariable; use crate::scope::{self, Environment, EnvironmentRef, EnvironmentSummary}; use crate::settings::NodeName; @@ -503,10 +504,13 @@ fn open_in( repository_id: RepositoryId, ) -> Result, RpcError> { let rows = tx.query( - "SELECT p.number, e.name FROM config_pr_environment p \ - JOIN config_environment e ON e.id = p.environment_id \ - JOIN config_environment_branch b ON b.environment_id = p.environment_id \ - WHERE e.project_id = ?1 AND p.repository_id = ?2 AND NOT EXISTS (SELECT 1 FROM config_removal_mark m WHERE m.environment_id = b.environment_id AND m.state = 'removing') ORDER BY p.number", + &format!( + "SELECT p.number, e.name FROM config_pr_environment p \ + JOIN config_environment e ON e.id = p.environment_id \ + JOIN config_environment_branch b ON b.environment_id = p.environment_id \ + WHERE e.project_id = ?1 AND p.repository_id = ?2 AND {} ORDER BY p.number", + mark::unmarked("b.environment_id") + ), &[project.as_str().into(), repository_id.into()], )?; let mut open = Vec::new(); @@ -804,11 +808,14 @@ pub(crate) fn project_checks( environment: &EnvironmentId, ) -> Result, RpcError> { let rows = tx.query( - "SELECT DISTINCT p.repository_id, p.number FROM config_pr_environment p \ - JOIN config_environment e ON e.id = p.environment_id \ - JOIN config_environment s ON s.project_id = e.project_id \ - JOIN config_environment_branch b ON b.environment_id = p.environment_id \ - WHERE s.id = ?1 AND NOT EXISTS (SELECT 1 FROM config_removal_mark m WHERE m.environment_id = b.environment_id AND m.state = 'removing') ORDER BY p.repository_id, p.number", + &format!( + "SELECT DISTINCT p.repository_id, p.number FROM config_pr_environment p \ + JOIN config_environment e ON e.id = p.environment_id \ + JOIN config_environment s ON s.project_id = e.project_id \ + JOIN config_environment_branch b ON b.environment_id = p.environment_id \ + WHERE s.id = ?1 AND {} ORDER BY p.repository_id, p.number", + mark::unmarked("b.environment_id") + ), &[environment.as_str().into()], )?; rows.iter().map(pull_request_ref).collect() @@ -1021,10 +1028,7 @@ pub(crate) fn pull_request( if !merged.all { kept.insert(environment.clone()); // A close that came before the merge is undone: it stays open. - tx.execute( - "DELETE FROM config_removal_mark WHERE environment_id = ?1 AND state = 'removing'", - &[environment.as_str().into()], - )?; + mark::drop_live(tx, environment)?; } } on_merge::record_unlanded(tx, who, &reported, &unlanded)?; @@ -1210,10 +1214,9 @@ fn current( closing: bool, ) -> Result, RpcError> { let open = if closing { - "" + String::new() } else { - "AND NOT EXISTS (SELECT 1 FROM config_removal_mark m \ - WHERE m.environment_id = b.environment_id AND m.state = 'removing')" + format!("AND {}", mark::unmarked("b.environment_id")) }; let rows = tx.query( &format!( @@ -1548,15 +1551,18 @@ fn shut_down( /// What the idle rule reads of each Branch: `environment_id`, kept, whether a /// removal names it (or a cancelled one did), its latest admission (-1 for none), /// its Branches and whether it is a Default. -const IDLE_FACTS: &str = "SELECT b.environment_id, b.kept, \ - (SELECT COUNT(*) FROM config_removal_mark m WHERE (m.environment_id = b.environment_id AND m.state <> 'removed') \ - OR (m.state = 'removing' AND m.project_id = (SELECT e.project_id FROM config_environment e WHERE e.id = b.environment_id))), \ - (SELECT COALESCE(MAX(d.admitted), -1) FROM config_deployment d WHERE d.environment_id = b.environment_id), \ - (SELECT COUNT(*) FROM config_environment_branch c WHERE c.parent_id = b.environment_id), \ - (SELECT COUNT(*) FROM config_project p WHERE p.default_environment_id = b.environment_id) \ - FROM config_environment_branch b"; - -/// When the sweep closes a Branch for sitting idle, from an [`IDLE_FACTS`] row: a +fn idle_facts() -> String { + format!( + "SELECT b.environment_id, b.kept, {}, \ + (SELECT COALESCE(MAX(d.admitted), -1) FROM config_deployment d WHERE d.environment_id = b.environment_id), \ + (SELECT COUNT(*) FROM config_environment_branch c WHERE c.parent_id = b.environment_id), \ + (SELECT COUNT(*) FROM config_project p WHERE p.default_environment_id = b.environment_id) \ + FROM config_environment_branch b", + mark::marks_holding("b.environment_id") + ) +} + +/// When the sweep closes a Branch for sitting idle, from an [`idle_facts`] row: a /// week after its latest Deployment. Never when it is kept, being removed or its /// removal was cancelled, never /// deployed, a Parent or the Default Environment. @@ -1569,7 +1575,7 @@ fn idle_close(row: &crate::storage::Row) -> Result, RpcError> { /// When a Branch closes for sitting idle, in seconds since the Unix epoch. pub(crate) fn closes_at(tx: &mut dyn Tx, id: &EnvironmentId) -> Result, RpcError> { let rows = tx.query( - &format!("{IDLE_FACTS} WHERE b.environment_id = ?1"), + &format!("{} WHERE b.environment_id = ?1", idle_facts()), &[id.as_str().into()], )?; rows.first().map_or(Ok(None), idle_close) @@ -1578,7 +1584,10 @@ pub(crate) fn closes_at(tx: &mut dyn Tx, id: &EnvironmentId) -> Result Result { let rows = tx.query( - &format!("{IDLE_FACTS} WHERE b.organization_id = ?1 ORDER BY b.environment_id"), + &format!( + "{} WHERE b.organization_id = ?1 ORDER BY b.environment_id", + idle_facts() + ), &[who.organization.as_str().into()], )?; // Closing would lose what it holds for its merge: an idle PR Environment that @@ -1636,15 +1645,16 @@ pub(crate) fn awaiting_merge( branch: &BranchName, ) -> Result, RpcError> { let rows = tx.query( - "SELECT DISTINCT p.number, r.facts, r.merged, r.unlanded, r.standing, r.phase \ - FROM config_pr_environment p \ - JOIN config_environment_branch b ON b.environment_id = p.environment_id \ - JOIN config_pull_request r ON r.organization_id = p.organization_id \ - AND r.repository_id = p.repository_id AND r.number = p.number \ - WHERE p.organization_id = ?1 AND p.repository_id = ?2 \ - AND NOT EXISTS (SELECT 1 FROM config_removal_mark m \ - WHERE m.environment_id = b.environment_id AND m.state = 'removing') \ - AND r.merged IS NULL ORDER BY p.number", + &format!( + "SELECT DISTINCT p.number, r.facts, r.merged, r.unlanded, r.standing, r.phase \ + FROM config_pr_environment p \ + JOIN config_environment_branch b ON b.environment_id = p.environment_id \ + JOIN config_pull_request r ON r.organization_id = p.organization_id \ + AND r.repository_id = p.repository_id AND r.number = p.number \ + WHERE p.organization_id = ?1 AND p.repository_id = ?2 \ + AND {} AND r.merged IS NULL ORDER BY p.number", + mark::unmarked("b.environment_id") + ), &[who.organization.as_str().into(), repository_id.into()], )?; let mut awaiting = Vec::new(); diff --git a/core/crates/ployz-store/src/removal/mark.rs b/core/crates/ployz-store/src/removal/mark.rs index 3c5e9751f0..ba5145ddb8 100644 --- a/core/crates/ployz-store/src/removal/mark.rs +++ b/core/crates/ployz-store/src/removal/mark.rs @@ -132,6 +132,36 @@ fn state_text(state: State) -> &'static str { } } +/// What [`finish`] writes. It is no [`State`]: no query loads it back as a [`Mark`]. +const REMOVED: &str = "removed"; + +/// A SQL condition that holds when no live removal mark names the Environment whose ID +/// `environment_column` reads, as its own target. A Project's mark does not count: the +/// callers that splice this in leave Branches of a removing Project be. Splice it into +/// a `WHERE` and keep the alias `m` free. +pub(crate) fn unmarked(environment_column: &str) -> String { + format!( + "NOT EXISTS (SELECT 1 FROM config_removal_mark m \ + WHERE m.environment_id = {environment_column} AND m.state = '{}')", + state_text(State::Removing) + ) +} + +/// A SQL scalar counting the marks that hold the Environment whose ID +/// `environment_column` reads: its own mark in any state but `removed`, a tombstone +/// included, and the live mark of its Project. The idle rule reads it where +/// [`unmarked`] would not do, because a tombstone and a Project removal also keep a +/// Branch from closing for sitting idle. Keep the aliases `m` and `e` free. +pub(crate) fn marks_holding(environment_column: &str) -> String { + format!( + "(SELECT COUNT(*) FROM config_removal_mark m \ + WHERE (m.environment_id = {environment_column} AND m.state <> '{REMOVED}') \ + OR (m.state = '{removing}' AND m.project_id = \ + (SELECT e.project_id FROM config_environment e WHERE e.id = {environment_column})))", + removing = state_text(State::Removing) + ) +} + /// `target`'s live mark or its tombstone. pub(crate) fn of(tx: &mut dyn Tx, target: &Target) -> Result, RpcError> { let (column, id) = match target { @@ -140,7 +170,7 @@ pub(crate) fn of(tx: &mut dyn Tx, target: &Target) -> Result, RpcEr }; tx.query( &format!( - "SELECT {COLUMNS} FROM config_removal_mark WHERE {column} = ?1 AND state <> 'removed'" + "SELECT {COLUMNS} FROM config_removal_mark WHERE {column} = ?1 AND state <> '{REMOVED}'" ), &[id.into()], )? @@ -156,9 +186,10 @@ pub(crate) fn naming( ) -> Result, RpcError> { tx.query( &format!( - "SELECT {COLUMNS} FROM config_removal_mark WHERE state = 'removing' AND \ + "SELECT {COLUMNS} FROM config_removal_mark WHERE state = '{}' AND \ (environment_id = ?1 OR project_id = \ - (SELECT project_id FROM config_environment WHERE id = ?1))" + (SELECT project_id FROM config_environment WHERE id = ?1))", + state_text(State::Removing) ), &[environment.as_str().into()], )? @@ -172,7 +203,8 @@ pub(crate) fn live(tx: &mut dyn Tx, who: &Actor) -> Result, RpcError> tx.query( &format!( "SELECT {COLUMNS} FROM config_removal_mark \ - WHERE organization_id = ?1 AND state = 'removing' ORDER BY at, id" + WHERE organization_id = ?1 AND state = '{}' ORDER BY at, id", + state_text(State::Removing) ), &[who.organization.as_str().into()], )? @@ -188,8 +220,10 @@ pub(crate) fn residuals( id: &RemovalId, ) -> Result, RpcError> { tx.query( - "SELECT residuals FROM config_removal_mark \ - WHERE organization_id = ?1 AND id = ?2 AND state = 'removed'", + &format!( + "SELECT residuals FROM config_removal_mark \ + WHERE organization_id = ?1 AND id = ?2 AND state = '{REMOVED}'" + ), &[who.organization.as_str().into(), id.as_str().into()], )? .first() @@ -247,8 +281,11 @@ pub(crate) fn finish( residuals: &[Residual], ) -> Result { let moved = tx.execute( - "UPDATE config_removal_mark SET state = 'removed', residuals = ?2 \ - WHERE id = ?1 AND state = 'removing'", + &format!( + "UPDATE config_removal_mark SET state = '{REMOVED}', residuals = ?2 \ + WHERE id = ?1 AND state = '{}'", + state_text(State::Removing) + ), &[id.as_str().into(), text(&residuals).as_str().into()], )?; Ok(if moved == 0 { @@ -265,12 +302,28 @@ pub(crate) fn clear_tombstone(tx: &mut dyn Tx, target: &Target) -> Result<(), Rp Target::Project(id) => ("project_id", id.as_str()), }; tx.execute( - &format!("DELETE FROM config_removal_mark WHERE {column} = ?1 AND state = 'cancelled'"), + &format!( + "DELETE FROM config_removal_mark WHERE {column} = ?1 AND state = '{}'", + state_text(State::Cancelled) + ), &[id.into()], )?; Ok(()) } +/// Delete `environment`'s live mark, leaving no tombstone, so a close that came before a +/// merge is undone. +pub(crate) fn drop_live(tx: &mut dyn Tx, environment: &EnvironmentId) -> Result<(), RpcError> { + tx.execute( + &format!( + "DELETE FROM config_removal_mark WHERE environment_id = ?1 AND state = '{}'", + state_text(State::Removing) + ), + &[environment.as_str().into()], + )?; + Ok(()) +} + fn text(value: &impl Serialize) -> String { serde_json::to_string(value).expect("stored documents are JSON") }