diff --git a/CLAUDE.md b/CLAUDE.md index cccf68a..1008c16 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -175,11 +175,15 @@ parses is disabled rather than left spinning the loop every 30 seconds forever. ### Scheduled patching -Three collections: `maintenance_windows` (cron start, IANA zone, duration), +Four collections: `maintenance_windows` (cron start, IANA zone, duration), `patch_policies` (selector, window, `all|security`, `never|if_required`, -concurrency cap, channels) and `patch_runs` (one per firing or manual Apply -updates, one `servers[]` entry per target). All three are in -`ScopedCollections`. +concurrency cap, channels), `patch_runs` (one per firing or manual Apply +updates, one `servers[]` entry per target) and `patch_run_outputs` (one per +run and server, holding the package manager's output tail). The output lives +apart from the run because a large run with up to 64KB per server would pass +MongoDB's 16MB document limit; `GetPatchRun` fills `servers[].output` back in +memory so the API shape is unchanged, and the tick paths never read it. All +four are in `ScopedCollections`. **Runs are driven by database state, not goroutines.** A run can last hours; a goroutine-driven run is stranded at `running` when its pod dies. `patchsched` @@ -189,11 +193,21 @@ decision is a pure function in `internal/patchrun` (`Advance`, `ApplyResult`, `VerifyReboot`, `Finalize`) and every write is guarded by the server run's current status, so a result landing mid-tick is never overwritten. +**The window end never kills a package manager.** No server is dispatched in +the last `patchrun.LatestStartBeforeEnd` (15 minutes) of a window; queued and +waiting servers close at the window end as before. A server already patching +may finish past the end: its no-result timeout is its own dispatch time plus +`ManualTimeout` (2h, the agent's backstop) plus `ResultGrace` (20 minutes), for +windowed and manual runs alike. `RebootTimeout` is 45 minutes. + **Results do not cross the bus.** The pod holding the agent's stream writes `PatchResult` straight into the run, found by `servers.command_id` and the agent's own server ID. A reboot is settled by the first static inventory report -whose `boot_time_unix` is later than `rebooted_at`; a report during the -one-minute grace does not count. +whose boot time differs from (is later than) `boot_time_before`, the +`inventory.boot_time` recorded when the server moved to rebooting, so host and +server clock skew does not matter. Without `boot_time_before` the report's boot +time must be later than `rebooted_at`. A report during the one-minute grace +does not count either way. **Old agents must never receive a scope.** An agent before `patchrun.MinAgentVersion` ignores `scope` and installs everything, so policy @@ -1053,7 +1067,7 @@ plane, each of which this codebase enforces: ## MongoDB Collections -`servers` · `keys` · `assignments` · `orgs` · `users` · `auth_providers` · `settings` · `secrets` · `workflows` · `workflow_steps` · `workflow_runs` · `workflow_log_lines` · `workflow_log_seq` · `monitors` · `incidents` · `monitor_rollups` · `notification_channels` · `console_sessions` · `audit_logs` · `server_packages` · `vuln_findings` · `vuln_alert_rules` · `vulndb_meta` · `server_workloads` · `api_tokens` · `status_pages` · `status_incidents` · `maintenance_windows` · `patch_policies` · `patch_runs` · `migrations` +`servers` · `keys` · `assignments` · `orgs` · `users` · `auth_providers` · `settings` · `secrets` · `workflows` · `workflow_steps` · `workflow_runs` · `workflow_log_lines` · `workflow_log_seq` · `monitors` · `incidents` · `monitor_rollups` · `notification_channels` · `console_sessions` · `audit_logs` · `server_packages` · `vuln_findings` · `vuln_alert_rules` · `vulndb_meta` · `server_workloads` · `api_tokens` · `status_pages` · `status_incidents` · `maintenance_windows` · `patch_policies` · `patch_runs` · `patch_run_outputs` · `migrations` Every document except `migrations` carries `org_id`. Struct definitions are the source of truth - see `server/internal/models/`. diff --git a/server/internal/api/docs/openapi.json b/server/internal/api/docs/openapi.json index 9a3d3ec..97209c6 100644 --- a/server/internal/api/docs/openapi.json +++ b/server/internal/api/docs/openapi.json @@ -17,6 +17,17 @@ }, "type": "object" }, + "api.ApplyUpdatesErrorResponse": { + "properties": { + "error": { + "type": "string" + }, + "run_id": { + "type": "string" + } + }, + "type": "object" + }, "api.ApplyUpdatesResponse": { "properties": { "message": { @@ -1077,6 +1088,10 @@ }, "models.Inventory": { "properties": { + "boot_time": { + "description": "BootTime is the host's last reported boot time, stored on every report\nthat carries one so a patch reboot can be proven by a changed boot.", + "type": "string" + }, "cpu": { "$ref": "#/components/schemas/models.CPUInfo" }, @@ -7203,11 +7218,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/api.ApplyUpdatesResponse" + "$ref": "#/components/schemas/api.ApplyUpdatesErrorResponse" } } }, - "description": "Service Unavailable" + "description": "agent offline; the attempt is recorded as run_id" } }, "security": [ diff --git a/server/internal/api/handlers.go b/server/internal/api/handlers.go index 25884a7..408f70c 100644 --- a/server/internal/api/handlers.go +++ b/server/internal/api/handlers.go @@ -756,7 +756,7 @@ func updateAgent(c *gin.Context) { // @Param source query string false "vulnerabilities when started from the vulnerabilities page" // @Success 202 {object} ApplyUpdatesResponse // @Failure 404 {object} ErrorResponse -// @Failure 503 {object} ApplyUpdatesResponse +// @Failure 503 {object} ApplyUpdatesErrorResponse "agent offline; the attempt is recorded as run_id" // @Security cookieAuth // @Security bearerAuth // @Router /servers/{id}/apply-updates [post] diff --git a/server/internal/api/patching.go b/server/internal/api/patching.go index 38c7331..18b6391 100644 --- a/server/internal/api/patching.go +++ b/server/internal/api/patching.go @@ -170,6 +170,10 @@ func updateWindow(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + if err := services.CheckWindowScope(auth.InstanceID(c), c.Param("id"), auth.ServerScope(c)); err != nil { + patchError(c, err) + return + } w, err := services.UpdateWindow(auth.InstanceID(c), c.Param("id"), body) if err != nil { patchError(c, err) @@ -192,6 +196,10 @@ func updateWindow(c *gin.Context) { // @Security bearerAuth // @Router /maintenance-windows/{id} [delete] func deleteWindow(c *gin.Context) { + if err := services.CheckWindowScope(auth.InstanceID(c), c.Param("id"), auth.ServerScope(c)); err != nil { + patchError(c, err) + return + } if err := services.DeleteWindow(auth.InstanceID(c), c.Param("id")); err != nil { patchError(c, err) return diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index b9236e5..46b4785 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -400,13 +400,16 @@ var serverScopedRoutes = map[string]scopeDecl{ "GET /api/mcp": exempt, // Maintenance windows are a cron expression, a zone and a duration. They - // name no server and return no server data. + // name no server and return no server data, so reading and creating one + // is exempt. Changing or deleting one moves or stops the patching of + // every policy using it, so each is refused when any of those policies + // targets servers outside the token's tag restriction. "GET /api/maintenance-windows": exempt, "POST /api/maintenance-windows": exempt, "POST /api/maintenance-windows/preview": exempt, "GET /api/maintenance-windows/:id": exempt, - "PUT /api/maintenance-windows/:id": exempt, - "DELETE /api/maintenance-windows/:id": exempt, + "PUT /api/maintenance-windows/:id": scoped, + "DELETE /api/maintenance-windows/:id": scoped, // Reading a policy returns its selector (server IDs and tag pairs) and no // hostname, inventory or state, the same data a workflow's targets carry. "GET /api/patch-policies": exempt, diff --git a/server/internal/api/types.go b/server/internal/api/types.go index c0d748f..3073188 100644 --- a/server/internal/api/types.go +++ b/server/internal/api/types.go @@ -307,6 +307,13 @@ type ApplyUpdatesResponse struct { RunID string `json:"run_id,omitempty"` } +// ApplyUpdatesErrorResponse is the 503 body of apply-updates: the agent is +// offline, and the attempt is still recorded as a run. +type ApplyUpdatesErrorResponse struct { + Error string `json:"error"` + RunID string `json:"run_id,omitempty"` +} + // WindowPreviewRequest is the body of POST /maintenance-windows/preview. type WindowPreviewRequest struct { Cron string `json:"cron"` diff --git a/server/internal/mcp/tools_write.go b/server/internal/mcp/tools_write.go index 58c0c1f..cb55ec6 100644 --- a/server/internal/mcp/tools_write.go +++ b/server/internal/mcp/tools_write.go @@ -162,12 +162,11 @@ type updateBatchResult struct { // apply_updates. The REST route (internal/api/handlers.go's applyUpdates) is // per-server: POST /servers/:id/apply-updates resolves one server with -// services.GetServerScoped and calls services.DispatchApplyUpdates(serverID). -// There is no fleet-wide variant of that service call to invoke once, so this -// tool resolves the requested targets through ResolveTargetsScoped exactly as -// the brief describes, then calls the same DispatchApplyUpdates the REST route -// calls, once per resolved server - the identical dispatch, just looped -// instead of hardcoded to one server_id from the URL. +// services.GetServerScoped and calls services.StartManualRun. There is no +// fleet-wide variant of that service call to invoke once, so this tool +// resolves the requested targets through ResolveTargetsScoped, then calls the +// same StartManualRun once per resolved server and writes the same +// updates.applied audit event per server with its run ID. func init() { All().Register(Tool{ Name: "apply_updates", @@ -201,6 +200,8 @@ func init() { result.Failed[srv.ServerID] = err.Error() continue } + services.LogEvent(c.InstanceID, "updates.applied", "mcp:"+c.TokenName, srv.ServerID, "", + fmt.Sprintf("package update run %s started on %s", run.RunID, srv.Hostname)) result.Succeeded = append(result.Succeeded, srv.ServerID) if result.RunIDs == nil { result.RunIDs = map[string]string{} diff --git a/server/internal/models/patch.go b/server/internal/models/patch.go index 14e3999..d894f04 100644 --- a/server/internal/models/patch.go +++ b/server/internal/models/patch.go @@ -105,8 +105,12 @@ type PatchServerRun struct { PendingAfter *int `bson:"pending_after,omitempty" json:"pending_after,omitempty"` RebootedAt *time.Time `bson:"rebooted_at,omitempty" json:"rebooted_at,omitempty"` VerifiedAt *time.Time `bson:"verified_at,omitempty" json:"verified_at,omitempty"` - Output string `bson:"output,omitempty" json:"output,omitempty"` - Error string `bson:"error,omitempty" json:"error,omitempty"` - StartedAt *time.Time `bson:"started_at,omitempty" json:"started_at,omitempty"` - FinishedAt *time.Time `bson:"finished_at,omitempty" json:"finished_at,omitempty"` + // BootTimeBefore is the host's reported boot time when the reboot was + // announced. A later report with a different boot time proves the + // restart without comparing the host clock to the server clock. + BootTimeBefore *time.Time `bson:"boot_time_before,omitempty" json:"-"` + Output string `bson:"output,omitempty" json:"output,omitempty"` + Error string `bson:"error,omitempty" json:"error,omitempty"` + StartedAt *time.Time `bson:"started_at,omitempty" json:"started_at,omitempty"` + FinishedAt *time.Time `bson:"finished_at,omitempty" json:"finished_at,omitempty"` } diff --git a/server/internal/models/server.go b/server/internal/models/server.go index 4501159..4e98fe6 100644 --- a/server/internal/models/server.go +++ b/server/internal/models/server.go @@ -42,6 +42,9 @@ type Inventory struct { RebootRequired bool `bson:"reboot_required,omitempty" json:"reboot_required,omitempty"` MetricsAt *time.Time `bson:"metrics_at,omitempty" json:"metrics_at,omitempty"` StaticAt *time.Time `bson:"static_at,omitempty" json:"static_at,omitempty"` + // BootTime is the host's last reported boot time, stored on every report + // that carries one so a patch reboot can be proven by a changed boot. + BootTime *time.Time `bson:"boot_time,omitempty" json:"boot_time,omitempty"` } type Server struct { diff --git a/server/internal/patchrun/patchrun.go b/server/internal/patchrun/patchrun.go index 4805439..02ae013 100644 --- a/server/internal/patchrun/patchrun.go +++ b/server/internal/patchrun/patchrun.go @@ -20,9 +20,19 @@ import ( const MinAgentVersion = "1.4.0" const ( - ResultGrace = 10 * time.Minute // after the window deadline, before "no result" - ManualTimeout = 2 * time.Hour // the agent's own cap when no deadline is sent - RebootTimeout = 20 * time.Minute // for a post-boot inventory report + // ResultGrace covers what the agent does after the upgrade and before it + // answers: a pending-update re-check of up to 10 minutes (Windows Update + // search) plus a 2 minute reboot check, with room to spare. + ResultGrace = 20 * time.Minute + // ManualTimeout is the agent's backstop for one started upgrade, counted + // from its own start. The window end never stops a running upgrade. + ManualTimeout = 2 * time.Hour + // RebootTimeout is how long a rebooting server has to send a post-boot + // inventory report. Windows cumulative updates routinely take over 20. + RebootTimeout = 45 * time.Minute + // LatestStartBeforeEnd is the tail of a window in which no server starts + // patching: a late start would run long past the window end. + LatestStartBeforeEnd = 15 * time.Minute ) // AgentSupportsPatchResults compares major.minor.patch. Empty, "dev" and @@ -88,6 +98,9 @@ func Advance(run models.PatchRun, now time.Time, connected map[string]bool) []Tr return nil } windowOpen := run.WindowEnd == nil || now.Before(*run.WindowEnd) + // In the last LatestStartBeforeEnd of a window nothing new starts: queued + // and waiting servers simply wait, and close at WindowEnd as usual. + mayStart := run.WindowEnd == nil || now.Before(run.WindowEnd.Add(-LatestStartBeforeEnd)) inFlight := 0 for _, s := range run.Servers { @@ -109,6 +122,8 @@ func Advance(run models.PatchRun, now time.Time, connected map[string]bool) []Tr to = models.PatchSrvMissedOffline } out = append(out, Transition{ServerID: s.ServerID, From: s.Status, To: to}) + case !mayStart: + // The window tail: no dispatch, no transition. case run.MaxConcurrent > 0 && inFlight >= run.MaxConcurrent: // No slot this tick. case !connected[s.ServerID]: @@ -125,17 +140,17 @@ func Advance(run models.PatchRun, now time.Time, connected map[string]bool) []Tr } case models.PatchSrvRebooting: if s.RebootedAt != nil && now.After(s.RebootedAt.Add(RebootTimeout)) { - out = append(out, Transition{ServerID: s.ServerID, From: s.Status, To: models.PatchSrvFailed, Error: "did not come back within 20 minutes"}) + out = append(out, Transition{ServerID: s.ServerID, From: s.Status, To: models.PatchSrvFailed, Error: fmt.Sprintf("did not come back within %d minutes", int(RebootTimeout.Minutes()))}) } } } return out } +// resultDeadline is the same for windowed and manual runs: the agent lets a +// started upgrade finish past the window end, so the window end says nothing +// about when a result is due. The base is the server's dispatch time. func resultDeadline(run models.PatchRun, s models.PatchServerRun) time.Time { - if run.WindowEnd != nil { - return run.WindowEnd.Add(ResultGrace) - } start := run.StartedAt if s.StartedAt != nil { start = *s.StartedAt @@ -176,11 +191,21 @@ func ApplyResult(s models.PatchServerRun, r *pb.PatchResult, now time.Time) (mod return s, true } -// VerifyReboot settles a rebooting server from a static inventory report. Only -// a boot time later than the reboot command proves the host restarted: a -// snapshot sent during the one-minute grace period must not count. +// VerifyReboot settles a rebooting server from a static inventory report. +// When the boot time reported before the reboot is known, a later boot time +// is the proof: both come from the host clock, so skew against the server +// clock does not matter. Otherwise only a boot time later than the reboot +// command counts. Either way a snapshot sent during the one-minute grace +// period, before the host went down, does not. func VerifyReboot(s models.PatchServerRun, bootTime time.Time, rebootRequired bool, now time.Time) (models.PatchServerRun, bool) { - if s.Status != models.PatchSrvRebooting || s.RebootedAt == nil || !bootTime.After(*s.RebootedAt) { + if s.Status != models.PatchSrvRebooting || s.RebootedAt == nil { + return s, false + } + proven := bootTime.After(*s.RebootedAt) + if s.BootTimeBefore != nil { + proven = bootTime.After(*s.BootTimeBefore) + } + if !proven { return s, false } if rebootRequired { diff --git a/server/internal/patchrun/patchrun_test.go b/server/internal/patchrun/patchrun_test.go index 6ec9ed9..6af11b6 100644 --- a/server/internal/patchrun/patchrun_test.go +++ b/server/internal/patchrun/patchrun_test.go @@ -93,36 +93,90 @@ func TestAdvanceWindowCloses(t *testing.T) { } } -func TestAdvanceNoResultTimeout(t *testing.T) { +// A windowed run times out from the server's own dispatch time, not from the +// window end: a server dispatched late in the window may finish past it. +func TestAdvanceNoResultTimeoutFromDispatch(t *testing.T) { run := windowRun(0, srv("a", models.PatchSrvPatching)) - if ts := Advance(run, t0.Add(2*time.Hour+9*time.Minute), nil); len(ts) != 0 { - t.Fatalf("inside grace: %+v", ts) + run.Servers[0].StartedAt = tp(t0.Add(90 * time.Minute)) + deadline := t0.Add(90*time.Minute + ManualTimeout + ResultGrace) + if ts := Advance(run, run.WindowEnd.Add(ResultGrace+time.Minute), nil); len(ts) != 0 { + t.Fatalf("past WindowEnd+grace but inside the dispatch timeout: %+v", ts) } - ts := Advance(run, t0.Add(2*time.Hour+11*time.Minute), nil) + if ts := Advance(run, deadline.Add(-time.Minute), nil); len(ts) != 0 { + t.Fatalf("inside the dispatch timeout: %+v", ts) + } + ts := Advance(run, deadline.Add(time.Minute), nil) if len(ts) != 1 || ts[0].To != models.PatchSrvFailed || ts[0].Error == "" { - t.Fatalf("past grace: %+v", ts) + t.Fatalf("past the dispatch timeout: %+v", ts) + } +} + +// Without a server StartedAt the run's own start is the base. +func TestAdvanceNoResultTimeoutFallsBackToRunStart(t *testing.T) { + run := windowRun(0, srv("a", models.PatchSrvPatching)) + if ts := Advance(run, t0.Add(2*time.Hour+19*time.Minute), nil); len(ts) != 0 { + t.Fatalf("inside timeout: %+v", ts) + } + if ts := Advance(run, t0.Add(2*time.Hour+21*time.Minute), nil); len(ts) != 1 || ts[0].To != models.PatchSrvFailed { + t.Fatalf("past timeout: %+v", ts) } } func TestAdvanceManualRunTimeout(t *testing.T) { run := models.PatchRun{Status: models.PatchRunRunning, StartedAt: t0, Servers: []models.PatchServerRun{srv("a", models.PatchSrvPatching)}} run.Servers[0].StartedAt = tp(t0) - if ts := Advance(run, t0.Add(2*time.Hour+9*time.Minute), nil); len(ts) != 0 { + if ts := Advance(run, t0.Add(2*time.Hour+19*time.Minute), nil); len(ts) != 0 { t.Fatalf("manual inside timeout: %+v", ts) } - if ts := Advance(run, t0.Add(2*time.Hour+11*time.Minute), nil); len(ts) != 1 || ts[0].To != models.PatchSrvFailed { + if ts := Advance(run, t0.Add(2*time.Hour+21*time.Minute), nil); len(ts) != 1 || ts[0].To != models.PatchSrvFailed { t.Fatalf("manual past timeout: %+v", ts) } } +func TestTimingConstants(t *testing.T) { + if ResultGrace != 20*time.Minute || RebootTimeout != 45*time.Minute || LatestStartBeforeEnd != 15*time.Minute { + t.Fatalf("ResultGrace=%v RebootTimeout=%v LatestStartBeforeEnd=%v", ResultGrace, RebootTimeout, LatestStartBeforeEnd) + } +} + +// No server starts patching in the last 15 minutes of a window: it would +// either be cut short or run long past the window end. +func TestAdvanceNoDispatchInWindowTail(t *testing.T) { + run := windowRun(0, srv("q", models.PatchSrvQueued), srv("w", models.PatchSrvWaitingOffline), srv("o", models.PatchSrvQueued)) + cutoff := run.WindowEnd.Add(-LatestStartBeforeEnd) + online := map[string]bool{"q": true, "w": true} + for _, at := range []time.Time{cutoff, cutoff.Add(time.Minute), run.WindowEnd.Add(-time.Second)} { + if ts := Advance(run, at, online); len(ts) != 0 { + t.Fatalf("at %v: nothing may change in the window tail, got %+v", at.Sub(t0), ts) + } + } +} + +func TestAdvanceDispatchJustBeforeWindowTail(t *testing.T) { + run := windowRun(0, srv("q", models.PatchSrvQueued)) + at := run.WindowEnd.Add(-LatestStartBeforeEnd - time.Second) + ts := Advance(run, at, map[string]bool{"q": true}) + if len(ts) != 1 || !ts[0].Dispatch { + t.Fatalf("dispatch must be allowed just before the tail: %+v", ts) + } +} + +// A manual run has no window and no tail. +func TestAdvanceManualRunHasNoTail(t *testing.T) { + run := models.PatchRun{Status: models.PatchRunRunning, StartedAt: t0, Servers: []models.PatchServerRun{srv("a", models.PatchSrvQueued)}} + if ts := Advance(run, t0.Add(10*time.Hour), map[string]bool{"a": true}); len(ts) != 1 || !ts[0].Dispatch { + t.Fatalf("manual run must dispatch: %+v", ts) + } +} + func TestAdvanceRebootTimeout(t *testing.T) { run := windowRun(0, srv("a", models.PatchSrvRebooting)) run.Servers[0].RebootedAt = tp(t0) - if ts := Advance(run, t0.Add(19*time.Minute), nil); len(ts) != 0 { + if ts := Advance(run, t0.Add(44*time.Minute), nil); len(ts) != 0 { t.Fatalf("inside reboot timeout: %+v", ts) } - ts := Advance(run, t0.Add(21*time.Minute), nil) - if len(ts) != 1 || ts[0].To != models.PatchSrvFailed { + ts := Advance(run, t0.Add(46*time.Minute), nil) + if len(ts) != 1 || ts[0].To != models.PatchSrvFailed || ts[0].Error != "did not come back within 45 minutes" { t.Fatalf("past reboot timeout: %+v", ts) } } @@ -202,6 +256,28 @@ func TestVerifyReboot(t *testing.T) { } } +// With the boot time recorded before the reboot, a changed boot time is the +// proof, whatever the skew between the host clock and the server clock. +func TestVerifyRebootChangedBoot(t *testing.T) { + s := srv("a", models.PatchSrvRebooting) + s.RebootedAt = tp(t0) + s.BootTimeBefore = tp(t0.Add(-10 * 24 * time.Hour)) + now := t0.Add(5 * time.Minute) + // The host clock runs 10 minutes slow: its new boot time reads earlier + // than the server's RebootedAt, yet the boot did change. + got, ok := VerifyReboot(s, t0.Add(-8*time.Minute), false, now) + if !ok || got.Status != models.PatchSrvSucceeded { + t.Fatalf("changed boot behind a slow clock must be proven: %+v ok=%v", got, ok) + } + // A fast host clock with an unchanged boot is not proof. + if _, ok := VerifyReboot(s, *s.BootTimeBefore, false, now); ok { + t.Error("an unchanged boot time is not proof") + } + if _, ok := VerifyReboot(s, s.BootTimeBefore.Add(-time.Minute), false, now); ok { + t.Error("an earlier boot time is not proof") + } +} + func TestFinalize(t *testing.T) { mk := func(statuses ...string) models.PatchRun { r := windowRun(0) diff --git a/server/internal/patchsched/sched.go b/server/internal/patchsched/sched.go index f56889e..4980f95 100644 --- a/server/internal/patchsched/sched.go +++ b/server/internal/patchsched/sched.go @@ -96,18 +96,18 @@ func process(ctx context.Context, deps Deps, p models.PatchPolicy, now time.Time n, err := deps.CountTargets(p) if err != nil { - recordSkip(ctx, deps, p, "error: "+err.Error(), due, now) + retryLater(ctx, deps, p, err, due, next, now) return } active, err := hasActiveRun(ctx, p) if err != nil { - recordSkip(ctx, deps, p, "error: "+err.Error(), due, now) + retryLater(ctx, deps, p, err, due, next, now) return } switch d := Decide(due, end, now, active, n); d { case Fire: if err := deps.StartPolicyRun(p, end); err != nil { - recordSkip(ctx, deps, p, "error: "+err.Error(), due, now) + retryLater(ctx, deps, p, err, due, next, now) return } _, _ = db.Col("patch_policies").UpdateOne(ctx, bson.M{"policy_id": p.PolicyID}, @@ -130,6 +130,19 @@ func hasActiveRun(ctx context.Context, p models.PatchPolicy) (bool, error) { return false, err } +// retryLater handles an error after the claim. The claim is put back, guarded +// on the value just written so a concurrent edit to the policy is not undone, +// and the next tick retries the same occurrence. Decide's missed rule bounds +// the retries: once the occurrence is too late it is skipped as missed. +func retryLater(ctx context.Context, deps Deps, p models.PatchPolicy, cause error, due, next, now time.Time) { + if _, err := db.Col("patch_policies").UpdateOne(ctx, + bson.M{"policy_id": p.PolicyID, "next_run_at": next}, + bson.M{"$set": bson.M{"next_run_at": due}}); err != nil { + log.Printf("patchsched: policy %s: put back claim: %v", p.PolicyID, err) + } + recordSkip(ctx, deps, p, "error: "+cause.Error(), due, now) +} + func recordSkip(ctx context.Context, deps Deps, p models.PatchPolicy, reason string, due, at time.Time) { _, _ = db.Col("patch_policies").UpdateOne(ctx, bson.M{"policy_id": p.PolicyID}, bson.M{"$set": bson.M{"last_skipped": models.Skip{Reason: reason, Due: due, At: at}}}) diff --git a/server/internal/services/inventory.go b/server/internal/services/inventory.go index 68411e4..1f0a22d 100644 --- a/server/internal/services/inventory.go +++ b/server/internal/services/inventory.go @@ -23,6 +23,11 @@ func StoreInventory(serverID string, r *pb.InventoryReport) error { set["inventory.memory.used_bytes"] = r.Memory.UsedBytes } set["inventory.swap_used_bytes"] = r.SwapUsed + // Kept current so a patch reboot can be proven by a changed boot time, + // independent of clock skew between the host and the control plane. + if r.BootTimeUnix > 0 { + set["inventory.boot_time"] = time.Unix(r.BootTimeUnix, 0).UTC() + } if r.IncludeStatic { set["inventory.static_at"] = now diff --git a/server/internal/services/migrate_instance.go b/server/internal/services/migrate_instance.go index e9d2b94..778358a 100644 --- a/server/internal/services/migrate_instance.go +++ b/server/internal/services/migrate_instance.go @@ -51,6 +51,7 @@ var ScopedCollections = []string{ "maintenance_windows", "patch_policies", "patch_runs", + "patch_run_outputs", } // collectionRenames maps the two collections whose names change. Ordered so the diff --git a/server/internal/services/patch_indexes.go b/server/internal/services/patch_indexes.go index 002ff59..2fee9e8 100644 --- a/server/internal/services/patch_indexes.go +++ b/server/internal/services/patch_indexes.go @@ -29,6 +29,12 @@ func EnsurePatchIndexes() error { }); err != nil { return err } + if _, err := db.Col("patch_run_outputs").Indexes().CreateOne(ctx, mongo.IndexModel{ + Keys: bson.D{{Key: "instance_id", Value: 1}, {Key: "run_id", Value: 1}, {Key: "server_id", Value: 1}}, + Options: options.Index().SetUnique(true), + }); err != nil { + return err + } _, err := db.Col("patch_runs").Indexes().CreateMany(ctx, []mongo.IndexModel{ {Keys: bson.D{{Key: "instance_id", Value: 1}, {Key: "run_id", Value: 1}}, Options: options.Index().SetUnique(true)}, {Keys: bson.D{{Key: "instance_id", Value: 1}, {Key: "policy_id", Value: 1}, {Key: "started_at", Value: -1}}}, diff --git a/server/internal/services/patch_policies.go b/server/internal/services/patch_policies.go index c8ddd6f..d9113e4 100644 --- a/server/internal/services/patch_policies.go +++ b/server/internal/services/patch_policies.go @@ -194,6 +194,37 @@ func CountPolicyTargets(p models.PatchPolicy) (int, error) { var ErrPatchRunActive = errors.New("a run of this policy is already in progress") +// PoliciesForWindow lists every policy that uses a window, enabled or not. +func PoliciesForWindow(instanceID, windowID string) ([]models.PatchPolicy, error) { + ctx, cancel := patchCtx() + defer cancel() + cur, err := db.Col("patch_policies").Find(ctx, bson.M{"instance_id": instanceID, "window_id": windowID}) + if err != nil { + return nil, err + } + out := []models.PatchPolicy{} + return out, cur.All(ctx, &out) +} + +// CheckWindowScope refuses a tag-restricted token changing a window when any +// policy using it targets servers outside its restriction: moving or +// deleting the window moves or stops those servers' patching. +func CheckWindowScope(instanceID, windowID string, tokenScope map[string]string) error { + if len(tokenScope) == 0 { + return nil + } + ps, err := PoliciesForWindow(instanceID, windowID) + if err != nil { + return err + } + for _, p := range ps { + if err := CheckPolicyScope(instanceID, p, tokenScope); err != nil { + return err + } + } + return nil +} + // CheckPolicyScope refuses a tag-restricted token acting on a policy whose // targets reach outside its restriction, with the workflow rule unchanged. func CheckPolicyScope(instanceID string, p models.PatchPolicy, tokenScope map[string]string) error { diff --git a/server/internal/services/patch_runs.go b/server/internal/services/patch_runs.go index 20f9679..dbde58d 100644 --- a/server/internal/services/patch_runs.go +++ b/server/internal/services/patch_runs.go @@ -27,6 +27,61 @@ var ( const patchRunsCol = "patch_runs" +// patchRunOutputsCol holds each server's output tail, one document per +// (run_id, server_id). Kept out of the run document because a large run with +// up to 64KB of output per server would pass MongoDB's 16MB document limit. +const patchRunOutputsCol = "patch_run_outputs" + +// storePatchOutput upserts one server's output tail for a run. +func storePatchOutput(ctx context.Context, instanceID, runID, serverID, output string) error { + _, err := db.Col(patchRunOutputsCol).UpdateOne(ctx, + bson.M{"instance_id": instanceID, "run_id": runID, "server_id": serverID}, + bson.M{"$set": bson.M{"output": output, "updated_at": time.Now()}}, + options.UpdateOne().SetUpsert(true)) + return err +} + +// fillPatchOutputs puts each server's stored output back on the run, so the +// API response carries it as before. A run written before outputs moved out +// keeps the output it has inline. +func fillPatchOutputs(ctx context.Context, run *models.PatchRun) error { + cur, err := db.Col(patchRunOutputsCol).Find(ctx, bson.M{"instance_id": run.InstanceID, "run_id": run.RunID}) + if err != nil { + return err + } + var outs []struct { + ServerID string `bson:"server_id"` + Output string `bson:"output"` + } + if err := cur.All(ctx, &outs); err != nil { + return err + } + byServer := make(map[string]string, len(outs)) + for _, o := range outs { + byServer[o.ServerID] = o.Output + } + for i := range run.Servers { + if out, ok := byServer[run.Servers[i].ServerID]; ok { + run.Servers[i].Output = out + } + } + return nil +} + +// serverBootTime is the boot time the server last reported, or nil. +func serverBootTime(ctx context.Context, instanceID, serverID string) *time.Time { + var doc struct { + Inventory struct { + BootTime *time.Time `bson:"boot_time"` + } `bson:"inventory"` + } + if err := db.Col("servers").FindOne(ctx, bson.M{"instance_id": instanceID, "server_id": serverID}, + options.FindOne().SetProjection(bson.M{"inventory.boot_time": 1})).Decode(&doc); err != nil { + return nil + } + return doc.Inventory.BootTime +} + func newServerRun(s models.Server, now time.Time) models.PatchServerRun { r := models.PatchServerRun{ServerID: s.ServerID, Hostname: s.Hostname, Status: models.PatchSrvQueued, PendingBefore: len(s.AvailableUpdates)} if !patchrun.AgentSupportsPatchResults(s.AgentVersion) { @@ -185,9 +240,11 @@ func claimServerForDispatch(ctx context.Context, runID, serverID, from, commandI return res.MatchedCount > 0, nil } +// loadRun reads a run for the tick and result paths, which never need output, +// so any inline output left on an old document is not read either. func loadRun(ctx context.Context, filter bson.M) (*models.PatchRun, error) { var run models.PatchRun - err := db.Col(patchRunsCol).FindOne(ctx, filter).Decode(&run) + err := db.Col(patchRunsCol).FindOne(ctx, filter, options.FindOne().SetProjection(bson.M{"servers.output": 0})).Decode(&run) if errors.Is(err, mongo.ErrNoDocuments) { return nil, ErrPatchRunNotFound } @@ -299,11 +356,25 @@ func finalizeRun(ctx context.Context, runID string) { } LogEvent(run.InstanceID, "patch.run_finished", run.TriggeredBy, "", "", fmt.Sprintf("patch run %s (%s) %s: %s", runID, name, status, patchrun.Summary(*run))) - if status == models.PatchRunPartial || status == models.PatchRunFailed { + if status == models.PatchRunPartial || status == models.PatchRunFailed || + (status == models.PatchRunCancelled && hasBadOutcome(*run)) { notifyPatchRun(*run) } } +// hasBadOutcome reports whether any server failed or was not reached. A +// cancelled run with such a result still alerts: cancelling does not make a +// failure somebody else's problem. +func hasBadOutcome(run models.PatchRun) bool { + for _, s := range run.Servers { + switch s.Status { + case models.PatchSrvFailed, models.PatchSrvMissedOffline, models.PatchSrvWindowClosed: + return true + } + } + return false +} + func notifyPatchRun(run models.PatchRun) { if run.PolicyID == "" { return // a manual run was watched by the person who clicked @@ -351,18 +422,36 @@ func RecordPatchResult(instanceID, serverID string, r *pb.PatchResult) { if !ok { return } - set := bson.M{"status": updated.Status, "output": updated.Output, "error": updated.Error} + // The output goes to patch_run_outputs, not the run document. + set := bson.M{"status": updated.Status, "error": updated.Error} if updated.PendingAfter != nil { set["pending_after"] = *updated.PendingAfter } if updated.RebootedAt != nil { set["rebooted_at"] = *updated.RebootedAt } + if updated.Status == models.PatchSrvRebooting { + // The boot time before the reboot: a later report with a + // different one proves the restart. + if bt := serverBootTime(ctx, instanceID, serverID); bt != nil { + set["boot_time_before"] = *bt + } + } if updated.FinishedAt != nil { set["finished_at"] = *updated.FinishedAt } - if ok, _ := setServerForCommand(ctx, run.RunID, serverID, r.CommandId, models.PatchSrvPatching, set); ok && updated.Status == models.PatchSrvRebooting { - LogEvent(instanceID, "patch.reboot", "schedule", serverID, "", + ok, err := setServerForCommand(ctx, run.RunID, serverID, r.CommandId, models.PatchSrvPatching, set) + if err != nil { + log.Printf("patch run %s: server %s: record result: %v", run.RunID, serverID, err) + } + if !ok { + return + } + if err := storePatchOutput(ctx, instanceID, run.RunID, serverID, updated.Output); err != nil { + log.Printf("patch run %s: server %s: store output: %v", run.RunID, serverID, err) + } + if updated.Status == models.PatchSrvRebooting { + LogEvent(instanceID, "patch.reboot", run.TriggeredBy, serverID, "", fmt.Sprintf("%s rebooting for patch policy %s", s.Hostname, run.PolicyName)) } } @@ -427,7 +516,18 @@ func CancelPatchRun(instanceID, runID string) error { func GetPatchRun(instanceID, runID string) (*models.PatchRun, error) { ctx, cancel := patchCtx() defer cancel() - return loadRun(ctx, bson.M{"instance_id": instanceID, "run_id": runID}) + var run models.PatchRun + err := db.Col(patchRunsCol).FindOne(ctx, bson.M{"instance_id": instanceID, "run_id": runID}).Decode(&run) + if errors.Is(err, mongo.ErrNoDocuments) { + return nil, ErrPatchRunNotFound + } + if err != nil { + return nil, err + } + if err := fillPatchOutputs(ctx, &run); err != nil { + return nil, err + } + return &run, nil } // ListPatchRuns omits output: a list of fifty runs would otherwise carry up @@ -516,6 +616,8 @@ func sweepPatchRuns() { if days <= 0 || !r.FinishedAt.Before(now.AddDate(0, 0, -days)) { continue } - _, _ = db.Col(patchRunsCol).DeleteOne(ctx, bson.M{"run_id": r.RunID}) + if _, err := db.Col(patchRunsCol).DeleteOne(ctx, bson.M{"run_id": r.RunID}); err == nil { + _, _ = db.Col(patchRunOutputsCol).DeleteMany(ctx, bson.M{"instance_id": r.InstanceID, "run_id": r.RunID}) + } } } diff --git a/server/internal/services/patch_scoped_test.go b/server/internal/services/patch_scoped_test.go index a52ab5b..11dad34 100644 --- a/server/internal/services/patch_scoped_test.go +++ b/server/internal/services/patch_scoped_test.go @@ -5,7 +5,7 @@ import "testing" // A tenant-scoped collection missing from ScopedCollections outlives its // instance when the instance is purged. func TestPatchCollectionsAreScoped(t *testing.T) { - for _, name := range []string{"maintenance_windows", "patch_policies", "patch_runs"} { + for _, name := range []string{"maintenance_windows", "patch_policies", "patch_runs", "patch_run_outputs"} { found := false for _, got := range ScopedCollections { if got == name { diff --git a/web/components/patching/PolicyList.tsx b/web/components/patching/PolicyList.tsx index 43f007c..2e4a77d 100644 --- a/web/components/patching/PolicyList.tsx +++ b/web/components/patching/PolicyList.tsx @@ -16,12 +16,21 @@ const SKIP_REASON: Record = { no_targets: "no servers matched", }; +// runNowBody says what clicking Run now does to real machines, before it does. +function runNowBody(p: PatchPolicy, count: number): string { + const servers = `${count} server${count === 1 ? "" : "s"}`; + const installs = p.scope === "security" ? "security updates" : "all pending updates"; + const reboot = p.reboot === "if_required" ? "Servers that need a reboot will restart." : "No server will be rebooted."; + return `This starts a window of the usual length now and installs ${installs} on ${servers}. ${reboot}`; +} + export function PolicyList({ canEdit }: { canEdit: boolean }) { const router = useRouter(); const queryClient = useQueryClient(); const toast = useToast(); const [editing, setEditing] = useState(null); const [deleting, setDeleting] = useState(null); + const [running, setRunning] = useState(null); const { data: policies, isLoading, error, refetch } = useQuery({ queryKey: ["patch-policies"], queryFn: () => api.listPatchPolicies() }); const { data: windows } = useQuery({ queryKey: ["maintenance-windows"], queryFn: () => api.listMaintenanceWindows() }); const { data: servers } = useQuery({ queryKey: ["servers"], queryFn: () => api.listServers() }); @@ -29,8 +38,14 @@ export function PolicyList({ canEdit }: { canEdit: boolean }) { const runNow = useMutation({ mutationFn: (p: PatchPolicy) => api.runPatchPolicyNow(p.policy_id), - onSuccess: (run) => router.push(`/patching/runs/${run.run_id}`), - onError: (e) => toast.error(friendlyMessage(e)), + onSuccess: (run) => { + setRunning(null); + router.push(`/patching/runs/${run.run_id}`); + }, + onError: (e) => { + setRunning(null); + toast.error(friendlyMessage(e)); + }, }); const remove = useMutation({ mutationFn: (p: PatchPolicy) => api.deletePatchPolicy(p.policy_id), @@ -126,7 +141,7 @@ export function PolicyList({ canEdit }: { canEdit: boolean }) { {canEdit && (
- -
diff --git a/web/components/patching/WindowModal.tsx b/web/components/patching/WindowModal.tsx index 2ee6b22..453e516 100644 --- a/web/components/patching/WindowModal.tsx +++ b/web/components/patching/WindowModal.tsx @@ -4,6 +4,7 @@ import { useState } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { api, MaintenanceWindow } from "@/lib/api"; import { Button, Modal, friendlyMessage, useToast } from "@/components/ui"; +import { parseIntInRange } from "./status"; /* * Presets write cron underneath, as the workflow schedule card does, and the @@ -27,18 +28,21 @@ export function WindowModal({ initial, onClose, onSaved }: { initial?: Maintenan const [name, setName] = useState(initial?.name ?? ""); const [cron, setCron] = useState(initial?.cron ?? "0 2 * * 0"); const [tz, setTz] = useState(initial?.tz ?? Intl.DateTimeFormat().resolvedOptions().timeZone ?? "UTC"); - const [duration, setDuration] = useState(initial?.duration_minutes ?? 120); + // Kept as the raw input so a cleared field never reaches the API as NaN. + const [durationRaw, setDurationRaw] = useState(String(initial?.duration_minutes ?? 120)); + const duration = parseIntInRange(durationRaw, 15, 720); const [error, setError] = useState(null); const { data: preview, isError: previewFailed, error: previewError } = useQuery({ queryKey: ["window-preview", cron, tz, duration], - queryFn: () => api.previewMaintenanceWindow({ cron, tz, duration_minutes: duration }), + queryFn: () => api.previewMaintenanceWindow({ cron, tz, duration_minutes: duration ?? 0 }), + enabled: duration !== null, retry: false, }); const { mutate: save, isPending } = useMutation({ mutationFn: () => { - const input = { name, cron, tz, duration_minutes: duration }; + const input = { name, cron, tz, duration_minutes: duration ?? 0 }; return initial ? api.updateMaintenanceWindow(initial.window_id, input) : api.createMaintenanceWindow(input); }, onSuccess: (w) => { @@ -97,8 +101,12 @@ export function WindowModal({ initial, onClose, onSaved }: { initial?: Maintenan @@ -123,7 +131,7 @@ export function WindowModal({ initial, onClose, onSaved }: { initial?: Maintenan - diff --git a/web/components/patching/status.ts b/web/components/patching/status.ts index 3a7937c..c991dcc 100644 --- a/web/components/patching/status.ts +++ b/web/components/patching/status.ts @@ -68,3 +68,13 @@ export function formatDuration(minutes: number): string { if (h === 0) return `${m} min`; return m === 0 ? `${h} h` : `${h} h ${m} min`; } + +// parseIntInRange reads a form field kept as its raw string. It returns null +// for an empty, fractional or out-of-range value, so a cleared field is never +// sent as 0 or NaN. +export function parseIntInRange(raw: string, min: number, max: number): number | null { + const t = raw.trim(); + if (!/^\d+$/.test(t)) return null; + const n = Number(t); + return n >= min && n <= max ? n : null; +}