From 2701b32b5f84b5f7704ee15e341827a8364555a5 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Thu, 17 Sep 2026 09:24:16 +0000 Subject: [PATCH] fix(monitors): 400/404 on bad updates, clamp for_sec, reset heartbeat state on re-enable --- server/internal/api/monitors.go | 7 +++ server/internal/services/monitors.go | 59 ++++++++++++++++++----- server/internal/services/monitors_test.go | 45 +++++++++++++++++ 3 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 server/internal/services/monitors_test.go diff --git a/server/internal/api/monitors.go b/server/internal/api/monitors.go index 687c233..b2f22bf 100644 --- a/server/internal/api/monitors.go +++ b/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 diff --git a/server/internal/services/monitors.go b/server/internal/services/monitors.go index bd9809f..7487715 100644 --- a/server/internal/services/monitors.go +++ b/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 diff --git a/server/internal/services/monitors_test.go b/server/internal/services/monitors_test.go new file mode 100644 index 0000000..cd312ad --- /dev/null +++ b/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"]) + } +}