fix(monitors): 400/404 on bad updates, clamp for_sec, reset heartbeat state on re-enable

server/internal/api/monitors.go

@@ -145,6 +145,8 @@ func getMonitor(c *gin.Context) {
 //	@Param			body	body	object{name=string,group=string,type=string,target=models.MonitorTarget,interval_sec=int,runner=string,retries=int,enabled=bool,channel_ids=[]string,for_sec=int}	true	"Fields to update"
 //	@Success		204
 //	@Failure		400	{object}	ErrorResponse
+//	@Failure		403	{object}	ErrorResponse
+//	@Failure		404	{object}	ErrorResponse
 //	@Failure		500	{object}	ErrorResponse
 //	@Security		cookieAuth
 //	@Security		bearerAuth
@@ -210,6 +212,10 @@ func updateMonitor(c *gin.Context) {
 			c.JSON(http.StatusForbidden, gin.H{"error": err.Error()})
 			return
 		}
+		if errors.Is(err, services.ErrMonitorNotFound) {
+			c.JSON(http.StatusNotFound, gin.H{"error": err.Error()})
+			return
+		}
 		c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
 		return
 	}
@@ -222,6 +228,7 @@ func updateMonitor(c *gin.Context) {
 //	@Tags			monitors
 //	@Param			id	path	string	true	"Monitor ID"
 //	@Success		204
+//	@Failure		403	{object}	ErrorResponse
 //	@Failure		500	{object}	ErrorResponse
 //	@Security		cookieAuth
 //	@Security		bearerAuth

server/internal/services/monitors.go

@@ -230,16 +230,11 @@ func UpdateMonitor(instanceID, monitorID string, upd bson.M, tokenScope map[stri
 		return err
 	}
 	if existing == nil {
-		return fmt.Errorf("monitor not found")
+		return ErrMonitorNotFound
 	}
-	if raw, present := upd["type"]; present {
-		if t, _ := raw.(string); t != existing.Type && (models.IsPassiveMonitor(t) || models.IsPassiveMonitor(existing.Type)) {
-			bad := existing.Type
-			if models.IsPassiveMonitor(t) {
-				bad = t
-			}
-			return fmt.Errorf("type cannot be changed to or from %s", bad)
-		}
+	unset, err := prepareMonitorUpdate(existing, upd)
+	if err != nil {
+		return err
 	}
 	// Editing any field of a metric monitor the token could not have created is
 	// refused before any write, so a restricted token cannot rename or disable
@@ -259,7 +254,11 @@ func UpdateMonitor(instanceID, monitorID string, upd bson.M, tokenScope map[stri
 				return err
 			}
 		case models.MonitorMetric:
-			probe := models.Monitor{Type: existing.Type, Target: tg, ForSec: existing.ForSec}
+			forSec := existing.ForSec
+			if v, ok := upd["for_sec"].(int); ok {
+				forSec = v
+			}
+			probe := models.Monitor{Type: existing.Type, Target: tg, ForSec: forSec}
 			if err := validateMetric(&probe); err != nil {
 				return err
 			}
@@ -303,10 +302,48 @@ func UpdateMonitor(instanceID, monitorID string, upd bson.M, tokenScope map[stri
 			upd["runner"] = models.RunnerServer
 		}
 	}
-	_, err = db.Col("monitors").UpdateOne(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}, bson.M{"$set": upd})
+	change := bson.M{"$set": upd}
+	if len(unset) > 0 {
+		change["$unset"] = unset
+	}
+	_, err = db.Col("monitors").UpdateOne(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}, change)
 	return err
 }
 
+// ErrMonitorNotFound is returned when an update names a monitor that does not
+// exist in the instance.
+var ErrMonitorNotFound = errors.New("monitor not found")
+
+// prepareMonitorUpdate applies the checks and rewrites that need no database:
+// refusing a type change to or from a passive type, clamping for_sec, and
+// resetting a heartbeat's ping state when it is re-enabled. It returns the
+// fields to $unset.
+func prepareMonitorUpdate(existing *models.Monitor, upd bson.M) (bson.M, error) {
+	if raw, present := upd["type"]; present {
+		if t, _ := raw.(string); t != existing.Type && (models.IsPassiveMonitor(t) || models.IsPassiveMonitor(existing.Type)) {
+			bad := existing.Type
+			if models.IsPassiveMonitor(t) {
+				bad = t
+			}
+			return nil, fmt.Errorf("%w: type cannot be changed to or from %s", ErrInvalidMonitor, bad)
+		}
+	}
+	// A negative gate means nothing; treat it as "alert immediately", as
+	// create does.
+	if v, ok := upd["for_sec"].(int); ok && v < 0 {
+		upd["for_sec"] = 0
+	}
+	unset := bson.M{}
+	// A heartbeat disabled for a while still holds its last ping, so the next
+	// sweep after re-enabling would page at once. Start it over as if new.
+	if on, _ := upd["enabled"].(bool); on && !existing.Enabled && existing.Type == models.MonitorHeartbeat {
+		unset["state.last_ping_at"] = ""
+		unset["state.started_at"] = ""
+		upd["state.status"] = models.StatusPending
+	}
+	return unset, nil
+}
+
 // DeleteMonitor deletes a monitor and its associated data. For a metric
 // monitor, deletion is refused when its selector reaches beyond tokenScope -
 // the same out-of-scope check applied on create and update - so a restricted

server/internal/services/monitors_test.go

@@ -0,0 +1,45 @@
+package services
+
+import (
+	"errors"
+	"testing"
+
+	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models"
+	"go.mongodb.org/mongo-driver/v2/bson"
+)
+
+func TestPrepareMonitorUpdate(t *testing.T) {
+	hb := &models.Monitor{Type: models.MonitorHeartbeat, Enabled: false}
+
+	if _, err := prepareMonitorUpdate(hb, bson.M{"type": models.MonitorHTTP}); !errors.Is(err, ErrInvalidMonitor) {
+		t.Fatalf("type change should be ErrInvalidMonitor, got %v", err)
+	}
+
+	upd := bson.M{"enabled": true}
+	unset, err := prepareMonitorUpdate(hb, upd)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := unset["state.last_ping_at"]; !ok {
+		t.Fatalf("re-enable should clear last_ping_at, got %v", unset)
+	}
+	if _, ok := unset["state.started_at"]; !ok {
+		t.Fatalf("re-enable should clear started_at, got %v", unset)
+	}
+	if upd["state.status"] != models.StatusPending {
+		t.Fatalf("re-enable should set pending, got %v", upd)
+	}
+
+	on := &models.Monitor{Type: models.MonitorHeartbeat, Enabled: true}
+	if unset, _ := prepareMonitorUpdate(on, bson.M{"enabled": true}); len(unset) != 0 {
+		t.Fatalf("already enabled should not reset state, got %v", unset)
+	}
+
+	upd = bson.M{"for_sec": -5}
+	if _, err := prepareMonitorUpdate(&models.Monitor{Type: models.MonitorMetric}, upd); err != nil {
+		t.Fatal(err)
+	}
+	if upd["for_sec"] != 0 {
+		t.Fatalf("negative for_sec should clamp to 0, got %v", upd["for_sec"])
+	}
+}