fix(placement): a drained previous node re-places the phase instead of failing it

`drain-midmission` found this. `choose` treated its `want` argument as a hard
requirement, and the only caller passes `missions.target_node_id` — which is
not an operator's choice, only where the PREVIOUS phase happened to run. Two
consequences, both wrong:

  - A node drained or filled between phases produced `TargetUnfit`, which
    `is_transient()` says false to, so `phase_runner` FAILED the phase rather
    than queueing or moving it. The queue silently did not apply to the second
    phase of any mission.
  - While the node stayed fit, every later phase went straight back to it
    regardless of ranking — accidental mission-to-node affinity, which this
    module's own header says must not exist.

Mission state lives on the gateway (inject -> run -> collect -> destroy), so
re-placing costs nothing. The pin is now advisory: preferred while it fits,
and when it does not, the reason is logged and ranking proceeds. `TargetUnfit`
is deleted rather than left unconstructed, so it cannot come back as a
non-transient failure by accident.

The scenario had its own race: it waited for phase 0 to COMPLETE before
draining, but warm phases finish in ~80s against a 10s placement sweep, so
phase 1 was often already placed — and the run then blamed the platform for
running on a node that was not yet drained. It now drains while phase 0 is
still running, which does not disturb a live VM and is the more faithful test.

Co-Authored-By: Claude Opus 5 <[email protected]>

crates/cm-api/src/vm_placement.rs

@@ -119,8 +119,6 @@ pub enum PlacementError {
     FleetAtCapacity { report: String },
     /// We could not READ capacity. Must never be reported as "full".
     FleetUnreadable { report: String },
-    /// An explicit target exists but cannot take the work.
-    TargetUnfit { node: NodeId, why: Unfit },
 }
 
 impl PlacementError {
@@ -143,11 +141,6 @@ impl PlacementError {
             PlacementError::FleetUnreadable { report } => format!(
                 "cannot read node capacity — this is NOT a full fleet; check the node daemons.\n{report}"
             ),
-            PlacementError::TargetUnfit { node, why } => format!(
-                "node {} cannot take this phase: {}",
-                node.as_uuid(),
-                why.reason()
-            ),
         }
     }
 }
@@ -449,15 +442,30 @@ pub async fn choose(
     }
     let report = report(&fit, &unfit);
 
+    // `want` is ADVISORY, always. The only caller passes `missions.target_node_id`,
+    // which is simply where the PREVIOUS phase ran — not an operator's choice.
+    // Treating it as a requirement had two consequences, both wrong:
+    //
+    //   - a drained or full previous node failed the phase outright, because
+    //     the resulting error was not transient and so never reached the queue.
+    //     `drain-midmission` was written to exercise exactly that path.
+    //   - and while the node stayed fit, every later phase went back to it
+    //     regardless of ranking — accidental mission-to-node affinity, which
+    //     this module's own header says must not exist.
+    //
+    // Mission state lives on the gateway (inject -> run -> collect -> destroy),
+    // so re-placing costs nothing. Prefer the pin when it still fits; say out
+    // loud why it did not when it does not, and rank as usual.
     if let Some(want) = want {
         if let Some(c) = fit.iter().find(|c| c.node_id.as_uuid() == want) {
             return Ok(c.node_id);
         }
-        if let Some((n, _, why)) = unfit.iter().find(|(n, _, _)| n.as_uuid() == want) {
+        if let Some((_, name, why)) = unfit.iter().find(|(n, _, _)| n.as_uuid() == want) {
-            return Err(PlacementError::TargetUnfit { node: *n, why: why.clone() });
+            eprintln!(
+                "vm_placement: the previous phase's node {name} is {} — re-placing this phase",
+                why.reason()
+            );
         }
-        // Target is not even a candidate — fall through and place it somewhere
-        // capable rather than failing, since the target was advisory.
     }
 
     if let Some(best) = fit.into_iter().next() {
@@ -500,6 +508,41 @@ mod tests {
     /// holding roughly 1 GiB of its 8 GiB, observed usage is ~6314 MiB;
     /// inferring the baseline as 6314 - 16384 goes negative, clamps to the
     /// 2048 floor, and invents 2266 MiB — exactly one more VM than exists.
+    /// A drained previous node re-places the next phase; it does not fail it.
+    ///
+    /// `drain-midmission` found this: `choose` treated `missions.target_node_id`
+    /// — which is only ever "where the last phase ran" — as a hard requirement,
+    /// so a node an operator cordoned mid-mission produced a non-transient
+    /// error that failed the phase instead of moving it. The same path also
+    /// gave every later phase silent affinity back to the first node.
+    #[test]
+    fn an_unfit_previous_node_is_re_placed_not_refused() {
+        let drained = uuid::Uuid::from_u128(1);
+        let healthy = capacity_of(nid(2), "tank", 61440, 6144, 800, 0, None, 90.0).unwrap();
+
+        // Stand in for `choose`'s decision: the pin is consulted, then dropped.
+        let fit = vec![healthy.clone()];
+        let picked = fit
+            .iter()
+            .find(|c| c.node_id.as_uuid() == drained)
+            .or_else(|| fit.first())
+            .expect("a fit node exists");
+        assert_eq!(
+            picked.node_id,
+            nid(2),
+            "with the pinned node absent from `fit`, ranking must still yield a node"
+        );
+
+        // And the error that used to be produced here no longer exists, so it
+        // cannot be reintroduced as a non-transient failure by accident.
+        for e in [
+            PlacementError::FleetAtCapacity { report: String::new() },
+            PlacementError::FleetUnreadable { report: String::new() },
+        ] {
+            assert!(e.is_transient(), "both no-node outcomes must QUEUE, not fail");
+        }
+    }
+
     #[test]
     fn a_young_vms_unconsumed_memory_is_not_handed_out_twice() {
         // Idle: the reading that gets remembered, and the slot count it implies.

scripts/verify-mission-delivery.sh

@@ -1283,13 +1283,22 @@ except Exception: print(0)')
   info "drain-midmission: mission=$mission"
   api "$token" PATCH "/api/missions/$mission/status" '{"status":"running"}' >/dev/null
 
-  # Wait for phase 0 to finish, then drain the node it used — before phase 1
+  # Drain the node while phase 0 is still RUNNING on it.
-  # is placed. Polling is on the PHASE, not the mission: by the time the
+  #
-  # mission is terminal there is nothing left to re-place.
+  # The first version waited for phase 0 to COMPLETE and lost the race: warm
+  # phases finish in ~80s and `start_pending_phases` sweeps every 10s, so
+  # phase 1 was routinely already placed by the time the drain landed — and the
+  # run then reported "phase 1 ran on the DRAINED node" for a drain that had not
+  # yet happened. Draining does not touch a VM already running, only future
+  # placement, so doing it mid-phase is both safe and the more faithful test.
+  #
+  # Polling is on the PHASE, not the mission: by the time the mission is
+  # terminal there is nothing left to re-place.
   while [ "$waited" -lt "$MISSION_TIMEOUT" ]; do
     first_node=$(ssh "$HOST" "docker exec clawmates_postgres_1 psql -U postgres -d clawmates -tAc \
       \"select m.target_node_id from mission_phases p join missions m on m.id = p.mission_id
-         where p.mission_id = '$mission' and p.order_idx = 0 and p.status = 'completed';\"" \
+         where p.mission_id = '$mission' and p.order_idx = 0
+           and p.status in ('running','completed') and m.target_node_id is not null;\"" \
       | head -1 | tr -d '[:space:]')
     [ -n "$first_node" ] && break
     # A mission that died before phase 0 completed has nothing to test.
@@ -1297,16 +1306,16 @@ except Exception: print(0)')
 try: print(json.load(sys.stdin).get("status",""))
 except Exception: pass' 2>/dev/null)
     case "$status" in failed|cancelled)
-      norun "drain-midmission: mission ended $status before phase 0 completed"; return 1 ;;
+      norun "drain-midmission: mission ended $status before phase 0 started"; return 1 ;;
     esac
-    sleep 10
+    sleep 3
-    waited=$((waited + 10))
+    waited=$((waited + 3))
   done
   if [ -z "$first_node" ]; then
-    norun "drain-midmission: phase 0 never completed in ${MISSION_TIMEOUT}s"
+    norun "drain-midmission: phase 0 never started in ${MISSION_TIMEOUT}s"
     return 1
   fi
-  info "drain-midmission: phase 0 ran on $first_node — draining it"
+  info "drain-midmission: phase 0 is on $first_node — draining it now"
   ssh "$HOST" "docker exec clawmates_postgres_1 psql -U postgres -d clawmates -tAc \
     \"update nodes set status='draining' where id='$first_node';\"" >/dev/null
 
@@ -1332,7 +1341,7 @@ except Exception: pass' 2>/dev/null)
     || { fail "drain-midmission: could not read DRAIN.md from the pushed branch"; return 1; }
   case "$delivered" in
     *PHASE-ONE-OK*PHASE-TWO-OK*)
-      pass "drain-midmission: phase 1 read phase 0's work from a DIFFERENT node and appended" ;;
+      pass "drain-midmission: phase 1 read phase 0's work and appended to it" ;;
     *PRIOR-PHASE-WORK-WAS-LOST*)
       fail "drain-midmission: re-placement lost the previous phase's work" ;;
     *)