Declare gated thresholds in the v1 report payload

A report has always carried its thresholds as a map of measure to model, and a
map key names a measure and nothing else. Every threshold a report could declare
was therefore the bare one: the conventional `value` name of every grid point. A
pipeline that wanted `p99` watched, or wanted one corner of its grid watched, had
to reach for the thresholds endpoint and then keep it in step with the run by
hand.

At BMF version 1 `thresholds.models` is a list. An entry is `parameters`,
`measure`, `metric`, and `model`: the dimensions a threshold hangs off that the
report does not already state, in their canonical order, and the model to gate
with. `measure` is the same name, slug, or UUID the map key is today, created if
the project has never seen it. An absent `metric` is the conventional `value`
name and an absent `parameters` gates every grid point, so an entry naming only a
measure and a model is the map pair written out longhand. One measure may carry
several entries, and each one creates or updates the threshold with that identity
under the report's branch and testbed, through the same null collapse and
canonical filter storage every other writer goes through.

A threshold the report declares gates the very report that declared it. Nothing
moved to make that true: thresholds are resolved before results are parsed, which
is where they already were. It is worth saying out loud because it is what makes
the list worth having, and it is pinned by a test whose history is five ungated
reports and whose alerts all belong to thresholds that did not exist when the
request arrived.

The declared version says which shape to expect and the shape is checked rather
than guessed at. A list at version 0 and a map at version 1 are both a 400 naming
the version that was declared and the shape it calls for. A payload that declares
a version has stated something exact and gets exact validation, and the project's
version gate still fires first, so a project that does not accept version 1 turns
the payload away for that reason and never reads the list at all.

`reset` reaches exactly as far as the shape can address. A version 0 map can only
name bare thresholds, so it takes a model away from bare thresholds and nothing
else, which is what it already did. A legacy run cannot strip a named or filtered
threshold it has no way to spell, and the pipelines sending `reset` today are
exactly the ones that cannot spell them. A version 1 list can name every identity,
so `reset` reaches every threshold on the branch and testbed that the entries did
not name, including a payload that names none at all.

One identity declared twice in one payload is one threshold: the position is
where it was first written and the model is what it was last told, resolved
without an error. Two spellings of one filter are one identity, because a filter
canonicalizes before it is compared.

Two shapes behind one key is a place where error quality quietly dies. The obvious
spelling, an untagged enum, buffers the input, tries each variant, and on failure
says only that nothing matched, so a misspelled model test in a version 0 map would
come back as "data did not match any variant" rather than as the field and the
variants it could have been. The shape is known from the first token, so it is
decided by looking rather than by trying: every error a version 0 client got before
this layer it gets after it, byte for byte, and a malformed version 1 entry is named
by its position and its field. The one message that moves is the one that has to,
where `models` is neither shape and what it expects now names both.

Deleting a parameter set learns about thresholds. A threshold whose filter names
a set is a reference to that row, so the set cannot go out from under it, and the
refusal says which threshold to delete first. Naming is canonical equality and
nothing looser: a filter of `{"a":1}` matches the grid point `{"a":1,"b":2}`
without naming it, because a filter names only the keys it cares about and is a
predicate over values rather than a pointer at a row. That set can be deleted and
the filter still says what it said. The comparison runs in Rust over the
project's filtered thresholds, because canonical equality is what the canonical
form defines and that form is written in Rust. Nothing caps that read: it is small
because a filtered threshold is a rare thing to write and deleting a parameter set
is a rare thing to ask for, not because a limit says so. The delete and the check
share one transaction with the delete first, so a set that a report still
references is refused for that reason and the client is sent to the results rather
than to a threshold it would have to delete anyway.

The CLI is unchanged: it declares bare thresholds, which is what the version 0
map spells, and naming a metric or a grid point from the command line is a
separate piece of work.

Author: epompeii

lib/api_projects/src/parameters.rs

@@ -4,20 +4,20 @@ use bencher_endpoint::{
 };
 use bencher_json::{
     BenchmarkResourceId, JsonDirection, JsonPagination, JsonParameter, JsonParameters,
-    ParameterUuid, ProjectResourceId,
+    ParameterFilter, ParameterSet, ParameterUuid, ProjectResourceId, ThresholdUuid,
     project::parameter::{JsonNewParameter, JsonUpdateParameter},
 };
 use bencher_rbac::project::Permission;
 use bencher_schema::{
     actor_conn, auth_conn,
-    context::ApiContext,
+    context::{ApiContext, DbConnection},
     error::{
         conflict_error, resource_conflict_err, resource_not_found_err, with_auth_hint,
         with_token_hint,
     },
     model::{
         project::{
-            QueryProject,
+            ProjectId, QueryProject,
             benchmark::QueryBenchmark,
             parameter::{QueryParameter, UpdateParameter},
         },
@@ -26,7 +26,7 @@ use bencher_schema::{
             auth::{AuthUser, BearerToken},
         },
     },
-    schema, write_conn,
+    schema, write_conn, write_transaction,
 };
 use diesel::{BelongingToDsl as _, ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _};
 use dropshot::{HttpError, Path, Query, RequestContext, TypedBody, endpoint};
@@ -407,6 +407,12 @@ pub async fn patch_inner(
 /// Delete a parameter set for a benchmark.
 /// The user must have `delete` permissions for the project.
 /// All reports that use this parameter must be deleted first!
+/// All thresholds that use this parameter must be deleted first!
+///
+/// A threshold uses a parameter set when its `parameters` filter names that exact
+/// set. A filter that merely matches the set, because the set pins every key the
+/// filter names and more besides, is a predicate over values rather than a reference
+/// to this row, and it does not stand in the way.
 ///
 /// A benchmark's empty parameter set cannot be deleted.
 /// The empty set is structural: every benchmark is born with exactly one, and
@@ -465,9 +471,74 @@ async fn delete_inner(
         )));
     }
 
-    diesel::delete(schema::parameter::table.filter(schema::parameter::id.eq(query_parameter.id)))
-        .execute(write_conn!(context))
-        .map_err(resource_conflict_err!(Parameter, &query_parameter))?;
+    // The delete goes first and the threshold check goes second, both inside one
+    // transaction, so a set that a report still references is refused for that
+    // reason: the foreign key fires on the delete itself and the report refusal is
+    // the one the client reads. A set nothing reports is deleted and then put back
+    // if a threshold names it, which is what keeps the two refusals in that order
+    // without either of them growing a query the other already does.
+    let mut blocking_threshold = None;
+    let deleted = write_transaction!(context, |conn| {
+        diesel::delete(
+            schema::parameter::table.filter(schema::parameter::id.eq(query_parameter.id)),
+        )
+        .execute(conn)?;
+
+        blocking_threshold = threshold_naming_set(conn, query_project.id, &query_parameter.set)?;
+        if blocking_threshold.is_some() {
+            return Err(diesel::result::Error::RollbackTransaction);
+        }
+        diesel::QueryResult::Ok(())
+    });
+
+    match (deleted, blocking_threshold) {
+        (Ok(()), _) => Ok(()),
+        (Err(diesel::result::Error::RollbackTransaction), Some(threshold)) => {
+            Err(conflict_error(format!(
+                "All thresholds that use this parameter must be deleted first! Threshold ({threshold}) gates the parameter set ({parameter}) of benchmark ({benchmark}).",
+                parameter = query_parameter.set,
+                benchmark = query_benchmark.uuid,
+            )))
+        },
+        (Err(e), _) => Err(resource_conflict_err!(Parameter, &query_parameter)(e)),
+    }
+}
 
-    Ok(())
+/// The first threshold in the project whose filter names this exact parameter set,
+/// if there is one.
+///
+/// A filter names a set by canonical equality and only by canonical equality. A
+/// filter that merely matches the set, say `{"a":1}` against the grid point
+/// `{"a":1,"b":2}`, is a predicate over values rather than a reference to a row, and
+/// deleting the row it happens to match takes nothing out from under it.
+///
+/// The comparison runs here rather than in SQL because canonical equality is what
+/// the canonical form defines and that form is written in Rust. Only the thresholds
+/// carrying a filter at all are read, which is a small share of a project's
+/// thresholds and empty for every project that has never written one, and deleting a
+/// parameter set is a rare administrative request rather than anything on the ingest
+/// path. Nothing here caps the read, so a project that one day holds a great many
+/// filtered thresholds is what would move this into SQL.
+fn threshold_naming_set(
+    conn: &mut DbConnection,
+    project_id: ProjectId,
+    set: &ParameterSet,
+) -> diesel::QueryResult<Option<ThresholdUuid>> {
+    let canonical = set.canonical();
+    Ok(schema::threshold::table
+        .filter(schema::threshold::project_id.eq(project_id))
+        .filter(schema::threshold::parameters.is_not_null())
+        .order(schema::threshold::id.asc())
+        .select((schema::threshold::uuid, schema::threshold::parameters))
+        .load::<(ThresholdUuid, Option<ParameterFilter>)>(conn)?
+        .into_iter()
+        .find(|(_, parameters)| {
+            parameters.as_ref().is_some_and(|parameters| {
+                parameters
+                    .sets()
+                    .iter()
+                    .any(|set| set.canonical() == canonical)
+            })
+        })
+        .map(|(uuid, _)| uuid))
 }

lib/api_projects/tests/parameters.rs

@@ -3781,3 +3781,258 @@ async fn start_point_clone_carries_what_each_threshold_gates() {
     let source = list_thresholds(&server, &fixture, Some("main")).await;
     assert_eq!(source.len(), 2, "the start point is unchanged: {source:?}");
 }
+
+/// Delete a threshold through the thresholds endpoint.
+async fn delete_threshold(
+    server: &TestServer,
+    fixture: &Fixture,
+    threshold: &str,
+) -> (StatusCode, String) {
+    let resp = server
+        .client
+        .delete(server.api_url(&format!(
+            "/v0/projects/{}/thresholds/{threshold}",
+            fixture.project_slug
+        )))
+        .header(
+            bencher_json::AUTHORIZATION,
+            bencher_json::bearer_header(&fixture.token),
+        )
+        .send()
+        .await
+        .expect("Request failed");
+    let status = resp.status();
+    let body = resp.text().await.expect("Failed to read the response");
+    (status, body)
+}
+
+/// The UUID a threshold response carries.
+fn threshold_uuid(threshold: &serde_json::Value) -> String {
+    threshold
+        .get("uuid")
+        .and_then(serde_json::Value::as_str)
+        .expect("the threshold carries its uuid")
+        .to_owned()
+}
+
+/// One report of one grid point, taken back out again.
+///
+/// A parameter set is only ever minted by a report, and a report that still
+/// references it refuses the delete on its own. Deleting the report leaves the set
+/// behind with nothing pointing at it, which is the state where a threshold's claim
+/// on it is the only thing left to see.
+async fn unreferenced_grid_point(
+    server: &TestServer,
+    fixture: &Fixture,
+    set: &serde_json::Value,
+) -> (String, JsonParameter) {
+    let json_report = report(
+        server,
+        fixture,
+        1,
+        vec![v1(
+            "bench",
+            &[entry(
+                set,
+                &serde_json::json!({ "latency": { "value": 1.0 } }),
+            )],
+        )],
+        None,
+        None,
+    )
+    .await;
+    let report_uuid = json_report
+        .get("uuid")
+        .and_then(serde_json::Value::as_str)
+        .expect("the report carries its uuid")
+        .to_owned();
+
+    let benchmark = only_benchmark(server, fixture).await;
+    let wanted = parameters(&serde_json::to_string(set).expect("the set serializes"));
+    let grid_point = parameter_list(server, fixture, &benchmark, "")
+        .await
+        .into_iter()
+        .find(|parameter| parameter.set == wanted)
+        .expect("the reported grid point");
+
+    let (status, body) = delete_report(server, fixture, &report_uuid).await;
+    assert_eq!(status, StatusCode::NO_CONTENT, "DELETE report: {body}");
+
+    (benchmark, grid_point)
+}
+
+// A threshold that names a parameter set in its filter is a reference to it, so the
+// set cannot be deleted out from under it. Deleting the threshold is what makes the
+// set deletable, exactly as deleting a report is one level up.
+#[tokio::test]
+async fn parameter_delete_refuses_while_a_threshold_names_it() {
+    let server = TestServer::new().await;
+    let fixture = fixture(&server, "delete-threshold").await;
+    let (benchmark, grid_point) =
+        unreferenced_grid_point(&server, &fixture, &serde_json::json!({ "size_mb": 16 })).await;
+
+    let threshold = create_threshold(
+        &server,
+        &fixture,
+        None,
+        Some(serde_json::json!([{ "size_mb": 16 }])),
+    )
+    .await;
+
+    let (status, body) = delete_parameter(
+        &server,
+        &fixture,
+        &benchmark,
+        &fixture.token,
+        &grid_point.uuid,
+    )
+    .await;
+    assert_eq!(
+        status,
+        StatusCode::CONFLICT,
+        "a threshold names the set: {body}"
+    );
+    assert!(
+        body.contains("All thresholds that use this parameter must be deleted first!"),
+        "the refusal says what to delete first: {body}"
+    );
+
+    let mut conn = server.db_conn();
+    assert!(
+        parameter_row_id(&mut conn, &grid_point.uuid).is_some(),
+        "the refused delete put the parameter set back"
+    );
+    drop(conn);
+
+    let (status, body) = delete_threshold(&server, &fixture, &threshold_uuid(&threshold)).await;
+    assert_eq!(status, StatusCode::NO_CONTENT, "DELETE threshold: {body}");
+
+    let (status, body) = delete_parameter(
+        &server,
+        &fixture,
+        &benchmark,
+        &fixture.token,
+        &grid_point.uuid,
+    )
+    .await;
+    assert_eq!(
+        status,
+        StatusCode::NO_CONTENT,
+        "nothing names the set any more: {body}"
+    );
+
+    let mut conn = server.db_conn();
+    assert!(
+        parameter_row_id(&mut conn, &grid_point.uuid).is_none(),
+        "the parameter set is gone"
+    );
+}
+
+// Matching a set is not naming it. A filter of `{"size_mb": 16}` matches the grid
+// point `{"size_mb": 16, "os": "linux"}` because a filter names only the keys it
+// cares about, but it is a predicate over values rather than a reference to that
+// row: the grid point can go and the filter still says what it said.
+#[tokio::test]
+async fn parameter_delete_allows_a_filter_that_only_matches_it() {
+    let server = TestServer::new().await;
+    let fixture = fixture(&server, "delete-subset").await;
+    let (benchmark, grid_point) = unreferenced_grid_point(
+        &server,
+        &fixture,
+        &serde_json::json!({ "os": "linux", "size_mb": 16 }),
+    )
+    .await;
+
+    create_threshold(
+        &server,
+        &fixture,
+        None,
+        Some(serde_json::json!([{ "size_mb": 16 }])),
+    )
+    .await;
+
+    let (status, body) = delete_parameter(
+        &server,
+        &fixture,
+        &benchmark,
+        &fixture.token,
+        &grid_point.uuid,
+    )
+    .await;
+    assert_eq!(
+        status,
+        StatusCode::NO_CONTENT,
+        "a filter that merely matches does not stand in the way: {body}"
+    );
+
+    let mut conn = server.db_conn();
+    assert!(
+        parameter_row_id(&mut conn, &grid_point.uuid).is_none(),
+        "the parameter set is gone"
+    );
+}
+
+// When a report and a threshold both point at a set, the report is the one the
+// refusal names. The results have to go first either way, and telling a client
+// about the threshold while its reports still reference the set would send it to
+// the wrong place.
+#[tokio::test]
+async fn parameter_delete_reports_the_report_reference_first() {
+    let server = TestServer::new().await;
+    let fixture = fixture(&server, "delete-precedence").await;
+
+    report(
+        &server,
+        &fixture,
+        1,
+        vec![v1(
+            "bench",
+            &[entry(
+                &serde_json::json!({ "size_mb": 16 }),
+                &serde_json::json!({ "latency": { "value": 1.0 } }),
+            )],
+        )],
+        None,
+        None,
+    )
+    .await;
+
+    let benchmark = only_benchmark(&server, &fixture).await;
+    let grid_point = parameter_list(&server, &fixture, &benchmark, "")
+        .await
+        .into_iter()
+        .find(|parameter| parameter.set == parameters(r#"{"size_mb":16}"#))
+        .expect("the reported grid point");
+
+    create_threshold(
+        &server,
+        &fixture,
+        None,
+        Some(serde_json::json!([{ "size_mb": 16 }])),
+    )
+    .await;
+
+    let (status, body) = delete_parameter(
+        &server,
+        &fixture,
+        &benchmark,
+        &fixture.token,
+        &grid_point.uuid,
+    )
+    .await;
+    assert_eq!(
+        status,
+        StatusCode::CONFLICT,
+        "both point at the set: {body}"
+    );
+    assert!(
+        !body.contains("All thresholds that use this parameter must be deleted first!"),
+        "the report reference is the one that fires: {body}"
+    );
+
+    let mut conn = server.db_conn();
+    assert!(
+        parameter_row_id(&mut conn, &grid_point.uuid).is_some(),
+        "the parameter set is still there"
+    );
+}