research: don't advance to reviewing without an outcome + guard-before-write
Two bugs that combine to produce the 409 you get when clicking Approve:
1) notify_run_completed (topology_worker post-hook) was advancing
topic status processing → reviewing whenever the last sibling run
terminated — success OR fail. A failed run with 0 outcomes still
pushed the topic to `reviewing`, the canvas rendered the "Request
publish" affordance, and the reviewer clicked Approve on nothing.
Fixed by adding an EXISTS(research_outcomes …) clause to the
UPDATE. Topic stays in `processing` when no outcome exists; the
loop's next iteration still has a chance to produce one.
2) decide_publish was calling
research_publish_approvals::decide(approve=true)
FIRST (which flips the row to `status='approved'`) and then
running the "no outcome? 409" guard SECOND. On the 409 return,
the DB was left half-flipped: approval says approved, topic still
in reviewing, no outcome exists, and every future click to the
same approval returns 409 on the "already decided" guard —
leaving reviewers with no way forward.
Fixed by moving the outcome-existence check BEFORE the decide()
call. On 409 now nothing was written, so the reviewer can try
again cleanly once an outcome is produced.
Also unstuck the current stuck row out-of-band (SQL UPDATE to reset
the approval to pending + topic to processing) so the user isn't
forced to delete the topic to escape the 409 loop.
sqlx dynamic query — the new EXISTS clause wasn't in the offline
cache so I switched notify_run_completed to plain `sqlx::query`.
This commit is contained in:
@@ -972,6 +972,19 @@ async fn decide_publish(
|
|||||||
if approval.status != "pending" {
|
if approval.status != "pending" {
|
||||||
return Err(ApiError::Conflict);
|
return Err(ApiError::Conflict);
|
||||||
}
|
}
|
||||||
|
// Pre-flight the no-outcome guard BEFORE we flip the approval row.
|
||||||
|
// Previous ordering wrote `status = 'approved'` first, then 409'd on
|
||||||
|
// this check — leaving the DB in a half-flipped state (approval
|
||||||
|
// approved, topic still in reviewing, no outcome existed) which
|
||||||
|
// surfaces to reviewers as a permanent "already decided" 409 the
|
||||||
|
// next click.
|
||||||
|
if approve {
|
||||||
|
let outcome =
|
||||||
|
cm_db::repo::research_outcomes::latest(&state.pool, approval.topic_id).await?;
|
||||||
|
if outcome.is_none() {
|
||||||
|
return Err(ApiError::Conflict);
|
||||||
|
}
|
||||||
|
}
|
||||||
let notes_ref = notes.as_deref().map(str::trim).filter(|s| !s.is_empty());
|
let notes_ref = notes.as_deref().map(str::trim).filter(|s| !s.is_empty());
|
||||||
let landed = cm_db::repo::research_publish_approvals::decide(
|
let landed = cm_db::repo::research_publish_approvals::decide(
|
||||||
&state.pool,
|
&state.pool,
|
||||||
@@ -988,20 +1001,6 @@ async fn decide_publish(
|
|||||||
return Ok(StatusCode::NO_CONTENT);
|
return Ok(StatusCode::NO_CONTENT);
|
||||||
}
|
}
|
||||||
if approve {
|
if approve {
|
||||||
// Guard: you can't approve-to-publish a topic that has no
|
|
||||||
// outcome. Discovered on first prod run — the pipeline can
|
|
||||||
// silently reach `published` state with zero runs surfacing an
|
|
||||||
// outcome (LLM auth failure, network, etc.), leaving the
|
|
||||||
// download endpoint at a 404 with no user-facing warning.
|
|
||||||
//
|
|
||||||
// Refuse with 409 so the frontend can render "no artifact — run
|
|
||||||
// failed, check pipeline diagnostics" and the reviewer isn't
|
|
||||||
// fooled into thinking approval is a no-op.
|
|
||||||
let outcome =
|
|
||||||
cm_db::repo::research_outcomes::latest(&state.pool, approval.topic_id).await?;
|
|
||||||
if outcome.is_none() {
|
|
||||||
return Err(ApiError::Conflict);
|
|
||||||
}
|
|
||||||
// reviewing → publishing → published in one API call.
|
// reviewing → publishing → published in one API call.
|
||||||
//
|
//
|
||||||
// Real async packaging isn't a thing yet — the artifact is the
|
// Real async packaging isn't a thing yet — the artifact is the
|
||||||
|
|||||||
@@ -355,7 +355,18 @@ pub async fn notify_run_completed(pool: &PgPool, id: Uuid) -> Result<bool, DbErr
|
|||||||
// One statement: subquery locates the topic id, subquery counts siblings
|
// One statement: subquery locates the topic id, subquery counts siblings
|
||||||
// still in flight (excluding *this* run — it's about to be flipped to
|
// still in flight (excluding *this* run — it's about to be flipped to
|
||||||
// completed/failed by the caller, but ordering isn't guaranteed here).
|
// completed/failed by the caller, but ordering isn't guaranteed here).
|
||||||
let row = sqlx::query!(
|
//
|
||||||
|
// Only advance the topic when it has AT LEAST ONE outcome — otherwise a
|
||||||
|
// failed run with no synthesis would push the topic into `reviewing`,
|
||||||
|
// the UI would offer "Request publish", the user would click Approve, and
|
||||||
|
// decide_publish would 409 on the "no outcome" guard. Stays in
|
||||||
|
// `processing` when zero outcomes exist so the loop's next iteration
|
||||||
|
// still has a chance to produce one.
|
||||||
|
// Dynamic query — the added EXISTS clause on research_outcomes
|
||||||
|
// doesn't have an entry in the offline sqlx cache, so we bind
|
||||||
|
// values by hand instead of using the `query!` macro.
|
||||||
|
use sqlx::Row;
|
||||||
|
let row: Option<sqlx::postgres::PgRow> = sqlx::query(
|
||||||
"UPDATE research_topics t
|
"UPDATE research_topics t
|
||||||
SET status = 'reviewing', updated_at = now()
|
SET status = 'reviewing', updated_at = now()
|
||||||
WHERE t.id = (
|
WHERE t.id = (
|
||||||
@@ -363,6 +374,10 @@ pub async fn notify_run_completed(pool: &PgPool, id: Uuid) -> Result<bool, DbErr
|
|||||||
WHERE id = $1 AND research_topic_id IS NOT NULL
|
WHERE id = $1 AND research_topic_id IS NOT NULL
|
||||||
)
|
)
|
||||||
AND t.status = 'processing'
|
AND t.status = 'processing'
|
||||||
|
AND EXISTS (
|
||||||
|
SELECT 1 FROM research_outcomes
|
||||||
|
WHERE topic_id = t.id
|
||||||
|
)
|
||||||
AND NOT EXISTS (
|
AND NOT EXISTS (
|
||||||
SELECT 1 FROM topology_runs
|
SELECT 1 FROM topology_runs
|
||||||
WHERE research_topic_id = t.id
|
WHERE research_topic_id = t.id
|
||||||
@@ -370,11 +385,13 @@ pub async fn notify_run_completed(pool: &PgPool, id: Uuid) -> Result<bool, DbErr
|
|||||||
AND status IN ('queued', 'running')
|
AND status IN ('queued', 'running')
|
||||||
)
|
)
|
||||||
RETURNING t.id",
|
RETURNING t.id",
|
||||||
id,
|
|
||||||
)
|
)
|
||||||
|
.bind(id)
|
||||||
.fetch_optional(pool)
|
.fetch_optional(pool)
|
||||||
.await?;
|
.await?;
|
||||||
Ok(row.is_some())
|
Ok(row
|
||||||
|
.map(|r| r.try_get::<Uuid, _>("id").is_ok())
|
||||||
|
.unwrap_or(false))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Mark a job completed and store its final result blob.
|
/// Mark a job completed and store its final result blob.
|
||||||
|
|||||||
Reference in New Issue
Block a user