refactor(store): own removal mark SQL in mark.rs

.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.

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);
+    }
+}

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() {

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<Vec<Included>, 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());

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<Vec<OpenPullRequest>, 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<Vec<PullRequestRef>, 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<Vec<(EnvironmentId, crate::id::ProjectId)>, 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<Option<i64>, 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<Option<i64>, 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<Option<i6
 /// Move every removal on, then close each Branch idle past its deadline.
 pub(crate) fn sweep(tx: &mut dyn Tx, who: &Actor, sweep: &Sweep) -> Result<Automated, RpcError> {
     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<Vec<PullRequestNumber>, 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();

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<Option<Mark>, RpcError> {
     let (column, id) = match target {
@@ -140,7 +170,7 @@ pub(crate) fn of(tx: &mut dyn Tx, target: &Target) -> Result<Option<Mark>, 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<Option<Mark>, 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<Vec<Mark>, 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<Vec<Residual>, 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<Finished, RpcError> {
     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")
 }