A release the controller refuses to take leaves the delivery held for its failed backup (issue 1068)

modules/mesh-delivery/cmd/mesh-delivery/backup_test.go

@@ -126,3 +126,29 @@ func TestADeliveryHeldForAFailedBackupIsNotLetGoByItsWalkAlone(t *testing.T) {
 		t.Fatal("a walk started on the controller's own path lifted the backup hold")
 	}
 }
+
+func TestAReleaseWhoseBackupsTheControllerRefusedToAskAgainLeavesTheDeliveryHeld(t *testing.T) {
+	w, id, plan := aDeliveringWalk(t)
+	held := w.ctl.walk(plan)
+	held.Backups = failedBackup()
+	w.h.WalkMoved(w.ctl.put(held))
+	w.settleAll()
+	w.ctl.refuseBackupsAgain = true
+	_, err := w.h.Release(id, "freed some space", "jochen")
+	if err == nil || !strings.Contains(err.Error(), "held again") {
+		t.Fatalf("the release of a refused word answered %v, want an error saying it is held again", err)
+	}
+	w.settleAll()
+	d := w.delivery(id)
+	if d.State != Held || d.BackupsAgainAfter != 0 || !strings.Contains(d.HeldWhy, "no space left on device") {
+		t.Fatalf("after the refusal it is %s (again-after %d, why %q), want held for the backup", d.State, d.BackupsAgainAfter, d.HeldWhy)
+	}
+	// The controller takes the word the next time: the delivery goes on.
+	w.ctl.refuseBackupsAgain = false
+	if _, err := w.h.Release(id, "freed some space", "jochen"); err != nil {
+		t.Fatal(err)
+	}
+	if s := w.state(id); s != Delivering {
+		t.Fatalf("released again, it is %s", s)
+	}
+}

modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go

@@ -94,7 +94,9 @@ type fakeController struct {
 	delivers []string
 	// backupsAgain are the walks whose failed backups were asked again.
 	backupsAgain []string
-	stops        []string
+	// refuseBackupsAgain refuses the word to take a walk's backups again.
+	refuseBackupsAgain bool
+	stops              []string
 	// withouts are the stops that left merges out (novox/hq ADR 0299): "<walk> without <merge>,…".
 	withouts []string
 	// refuseWithout refuses every stop with merges to leave out, as a controller before ADR 0299 does; mergedAt are
@@ -130,6 +132,9 @@ func (c *fakeController) BackupsAgain(walk, why string) error {
 	if c.down {
 		return errors.New("down")
 	}
+	if c.refuseBackupsAgain {
+		return errors.New("the controller refuses the backups")
+	}
 	w, known := c.walks[walk]
 	if !known || w.Backups == nil || w.Backups.State != "failed" {
 		return fmt.Errorf("%s holds no failed backup", walk)

modules/mesh-delivery/cmd/mesh-delivery/verbs.go

@@ -370,7 +370,7 @@ func (h *Holder) Release(id, why, by string) (string, error) {
 	}
 	// Held for a failed backup before the send: the controller is asked to take the backups again (novox/hq ADR
 	// 0404 clause 4); its record at this revision or before still holds the failure, and does not hold it again.
-	backupsOf := ""
+	backupsOf, againBefore := "", d.BackupsAgainAfter
 	if d.Walk != nil && len(d.Walk.BackupsFailed) > 0 {
 		backupsOf, d.BackupsAgainAfter = d.Walk.ID, d.Walk.Revision
 	}
@@ -379,9 +379,16 @@ func (h *Holder) Release(id, why, by string) (string, error) {
 	if backupsOf != "" {
 		h.mu.Unlock()
 		if err := h.Controller.BackupsAgain(backupsOf, "released by "+by+": "+why); err != nil {
+			// The controller did not clear the failure: its record still holds it, so the delivery is held for it again now.
+			h.mu.Lock()
+			if d := h.deliveries[id]; d != nil {
+				d.BackupsAgainAfter = againBefore
+				h.settleAndKeep(d)
+			}
+			h.mu.Unlock()
 			h.again("a walk's backups the controller refused or did not answer")
-			return "", fmt.Errorf("%s is delivering on %s's word, and the controller could not be asked to take the backups of "+
-				"%s again (%v): the failed backup holds it again when its record is read", id, by, backupsOf, err)
+			return "", fmt.Errorf("%s was not released: the controller could not be asked to take the backups of %s again "+
+				"(%v), so it is held again for the failed backup", id, backupsOf, err)
 		}
 		return fmt.Sprintf("%s is delivering on %s's word; the backups of its walk %s are asked again", id, by, backupsOf), nil
 	}