fix(dev): a cancel names the turn it stops

Cancel carried no id and stopped whatever turn was in flight when it landed.
Under at-least-once delivery that is the wrong turn: Turn 1, Cancel, Turn 2,
and the first Cancel delivered again answered Done{turn: 2, Cancelled} for a
turn nobody cancelled. Every other event was already safe to replay — a turn by
the high-water mark, a result by the outstanding ledger and its family, a timer
by the saved counter — so "for every event this protocol has" was false for
exactly this one.

Event::Cancel carries the turn id. The core acts on it only while that turn is
in flight or queued, and a turn id is accepted once, so a redelivered cancel
finds nothing under its id. In flight, the turn ends and takes the queue behind
it along, as before: a stop that started the next prompt would not be one.
Queued, the prompt goes alone, with a Save for the queue and its own Done. Any
other id changes nothing, and a cancel that arrives ahead of its turn does not
spend the id.

One event's bytes change: "Cancel" becomes {"Cancel":{"turn":1}}, spelled in
the C fixture. A host still sending the old form is refused with DEV_MALFORMED
rather than misread. ABI stays 1, as it did when Turn gained its id: nothing
has shipped under it, and a snapshot holds no event.

Three tests pin it. With the id check taken out they all fail, the first on
the redelivery's [Save, Done{turn: 2, Cancelled}].

Co-authored-by: Hanzo Dev <[email protected]>

crates/core/src/lib.rs

@@ -24,7 +24,10 @@
 //! - A **[`Event::Timer`]** carries no id and needs none: it asks only for
 //!   [`Op::Save`], and only when the sequence has moved since the last one, so
 //!   a redelivered timer asks for nothing.
-//! - A **[`Event::Cancel`]** is idempotent: there is one turn to stop.
+//! - A **[`Event::Cancel`]** names the turn it stops, by the id that turn
+//!   arrived under. It is acted on only while that turn is in flight or queued,
+//!   and an id is accepted once, so a redelivered cancel finds nothing under
+//!   its id — it cannot reach the turn that came after.
 //!
 //! So at-least-once delivery of an event is at-most-once execution of an
 //! effect, for every event this protocol has.
@@ -140,7 +143,7 @@ impl Session {
             Event::Git(output) => self.observed(Wants::Git, output),
             Event::Browse(output) => self.observed(Wants::Browse, output),
             Event::Timer => self.tick(),
-            Event::Cancel => self.cancel(),
+            Event::Cancel(cancel) => self.cancel(cancel.turn),
         }
     }
 
@@ -242,15 +245,25 @@ impl Session {
         }
     }
 
-    fn cancel(&mut self) -> Vec<Action> {
-        let Some(turn) = self.turn.take() else {
+    /// Stop the turn `turn` names. The one in flight takes the queue behind it
+    /// along — a stop that started the next prompt would not be one — while a
+    /// queued one goes alone and the rest keep their place. Any other id is a
+    /// turn already over or never accepted, and changes nothing.
+    fn cancel(&mut self, turn: u64) -> Vec<Action> {
+        let ended: Vec<u64> = if self.turn == Some(turn) {
+            self.turn = None;
+            self.outstanding.clear();
+            let queued = self.queue.drain(..).map(|queued| queued.id);
+            std::iter::once(turn).chain(queued).collect()
+        } else if let Some(at) = self.queue.iter().position(|queued| queued.id == turn) {
+            self.queue.remove(at);
+            vec![turn]
+        } else {
             return Vec::new();
         };
-        self.outstanding.clear();
-        let dropped: Vec<u64> = self.queue.drain(..).map(|turn| turn.id).collect();
-        let mut actions = vec![self.save(), self.done(turn, Outcome::Cancelled)];
-        for queued in dropped {
-            actions.push(self.done(queued, Outcome::Cancelled));
+        let mut actions = vec![self.save()];
+        for turn in ended {
+            actions.push(self.done(turn, Outcome::Cancelled));
         }
         actions
     }

crates/core/tests/session.rs

@@ -5,6 +5,7 @@ use dev_protocol::Action;
 use dev_protocol::Answer;
 use dev_protocol::Ask;
 use dev_protocol::Call;
+use dev_protocol::Cancel;
 use dev_protocol::Config;
 use dev_protocol::Dispatch;
 use dev_protocol::Done;
@@ -25,6 +26,10 @@ fn turn(id: u64, prompt: &str) -> Event {
     })
 }
 
+fn cancel(turn: u64) -> Event {
+    Event::Cancel(Cancel { turn })
+}
+
 fn tools(calls: Vec<Call>, id: u64) -> Event {
     Event::Model(Answer {
         id,
@@ -177,11 +182,11 @@ fn cancel_ends_the_turn_and_drops_what_was_outstanding() {
     session.step(turn(1, "fix the build"));
     session.step(tools(vec![test()], 1));
 
-    let stopped = session.step(Event::Cancel);
+    let stopped = session.step(cancel(1));
     assert_eq!(ops(&stopped), vec![&Op::Save, &done(1, Outcome::Cancelled)]);
     // A result for the cancelled effect is no longer outstanding.
     assert!(session.step(Event::Exec(result(2, "ok"))).is_empty());
-    assert!(session.step(Event::Cancel).is_empty());
+    assert!(session.step(cancel(1)).is_empty());
 }
 
 #[test]
@@ -389,7 +394,7 @@ fn a_cancel_says_what_became_of_every_prompt_it_discards() {
     session.step(turn(2, "second"));
     session.step(turn(3, "third"));
 
-    let stopped = session.step(Event::Cancel);
+    let stopped = session.step(cancel(1));
     assert_eq!(
         ops(&stopped),
         vec![
@@ -401,6 +406,72 @@ fn a_cancel_says_what_became_of_every_prompt_it_discards() {
     );
 }
 
+#[test]
+fn a_redelivered_cancel_does_not_stop_the_turn_that_came_after() {
+    let mut session = Session::new(Config::default());
+    session.step(turn(1, "first"));
+    let stopped = session.step(cancel(1));
+    assert_eq!(ops(&stopped), vec![&Op::Save, &done(1, Outcome::Cancelled)]);
+
+    // A new turn nobody cancelled, and then the transport delivers the first
+    // cancel again. It names turn 1, which is over.
+    let opened = session.step(turn(2, "second"));
+    assert!(session.step(cancel(1)).is_empty());
+
+    // Turn 2 is still in flight: its answer lands and it ends the ordinary way.
+    let ended = session.step(says("second done", opened[0].id));
+    assert_eq!(
+        ended.last().map(|a| &a.op),
+        Some(&done(2, Outcome::Complete))
+    );
+}
+
+#[test]
+fn a_cancel_of_a_queued_prompt_takes_that_prompt_and_no_other() {
+    let mut session = Session::new(Config::default());
+    let opened = session.step(turn(1, "first"));
+    session.step(turn(2, "second"));
+    session.step(turn(3, "third"));
+
+    // The queue changed, so it is saved, and the prompt gets its own Done.
+    let removed = session.step(cancel(2));
+    assert_eq!(ops(&removed), vec![&Op::Save, &done(2, Outcome::Cancelled)]);
+    assert!(session.step(cancel(2)).is_empty());
+
+    // Turn 1 never noticed, and turn 3 is what it chains into.
+    let chained = session.step(says("first done", opened[0].id));
+    assert_eq!(chained[2].op, done(1, Outcome::Complete));
+    assert_eq!(
+        asked(&chained[3]).messages.last(),
+        Some(&Message::User("third".to_string()))
+    );
+}
+
+#[test]
+fn a_cancel_that_names_no_live_turn_changes_nothing() {
+    let mut session = Session::new(Config::default());
+    // Nothing is in flight, so there is nothing to name.
+    assert!(session.step(cancel(1)).is_empty());
+
+    let opened = session.step(turn(1, "fix the build"));
+    session.step(tools(vec![test()], opened[0].id));
+    // Neither a turn the core has not been given nor id zero reaches turn 1...
+    assert!(session.step(cancel(2)).is_empty());
+    assert!(session.step(cancel(0)).is_empty());
+    // ...whose effect is still outstanding and still resumes it.
+    let again = session.step(Event::Exec(result(2, "ok")));
+    assert_eq!(again.len(), 1);
+    assert!(matches!(again[0].op, Op::Model(_)));
+
+    // And a cancel that arrived ahead of its turn did not spend that id.
+    let ended = session.step(says("built", again[0].id));
+    assert_eq!(
+        ended.last().map(|a| &a.op),
+        Some(&done(1, Outcome::Complete))
+    );
+    assert_eq!(session.step(turn(2, "and again")).len(), 1);
+}
+
 #[test]
 fn a_path_that_leaves_the_workspace_is_refused_rather_than_dispatched() {
     let mut session = Session::new(Config::default());

crates/ffi/tests/abi.c

@@ -1,5 +1,5 @@
 /* The ABI, exercised the way the Go host will exercise it: a session, a turn,
- * a snapshot, a restore, and a handle that has been dropped.
+ * a snapshot, a restore, a cancel, and a handle that has been dropped.
  *
  * The event and the expected substrings below are in the protocol's current
  * encoding (dev-protocol `encode`). When that encoding changes, this fixture
@@ -109,6 +109,28 @@ int main(void) {
   CHECK(forged == 0);
   dev_free(tampered);
 
+  /* A cancel names the turn it stops, so the same one delivered again stops
+   * nothing. */
+  const char *cancel = "{\"Cancel\":{\"turn\":1}}";
+  dev_buf stopped = {0};
+  CHECK(dev_step(session, (const uint8_t *)cancel, strlen(cancel), &stopped) == DEV_OK);
+  shown = text(stopped);
+  CHECK(shown != NULL);
+  if (shown != NULL) {
+    CHECK(strstr(shown, "{\"Done\":{\"turn\":1,\"outcome\":\"Cancelled\"}}") != NULL);
+    free(shown);
+  }
+  dev_free(stopped);
+  dev_buf again = {0};
+  CHECK(dev_step(session, (const uint8_t *)cancel, strlen(cancel), &again) == DEV_OK);
+  shown = text(again);
+  CHECK(shown != NULL);
+  if (shown != NULL) {
+    CHECK(strcmp(shown, "[]") == 0);
+    free(shown);
+  }
+  dev_free(again);
+
   /* A dropped handle names nothing. */
   dev_drop(session);
   CHECK(dev_step(session, (const uint8_t *)TURN, strlen(TURN), &refused) == DEV_HANDLE);

crates/protocol/src/lib.rs

@@ -7,7 +7,8 @@
 //! under it and replay a result instead of repeating an effect. A [`Turn`]
 //! carries the host's own id for the same reason, in the other direction: the
 //! core refuses a turn it has already accepted, and names that id in the
-//! [`Done`] that ends it.
+//! [`Done`] that ends it. A [`Cancel`] names the turn it stops by that id, so
+//! one delivered twice cannot stop the turn that came after.
 //!
 //! # Encoding
 //!
@@ -182,8 +183,8 @@ pub enum Event {
     Browse(Output),
     /// Time passed; a long turn should checkpoint.
     Timer,
-    /// Stop this turn.
-    Cancel,
+    /// Stop the turn this names.
+    Cancel(Cancel),
 }
 
 /// What the core asks the host to do.
@@ -222,6 +223,16 @@ pub struct Turn {
     pub prompt: String,
 }
 
+/// The turn to stop, by the id its [`Turn`] carried.
+///
+/// The core acts on a cancel only while the turn it names is in flight or
+/// queued. An id is accepted once, so a redelivered cancel names a turn that is
+/// already over and stops nothing — least of all the turn that came after.
+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub struct Cancel {
+    pub turn: u64,
+}
+
 /// The end of the turn `turn` asked for.
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct Done {
@@ -399,6 +410,13 @@ mod tests {
         assert_eq!(action, back);
     }
 
+    #[test]
+    fn a_cancel_round_trips_with_the_turn_it_names() {
+        let cancel = Event::Cancel(Cancel { turn: 7 });
+        let back: Event = decode(&encode(&cancel).expect("encode")).expect("decode");
+        assert_eq!(cancel, back);
+    }
+
     #[test]
     fn garbage_is_refused_with_a_reason() {
         let err = decode::<Event>(b"not a payload").expect_err("garbage decoded");