diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index e64451bebf..4891c82049 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -5127,6 +5127,20 @@ API and Pulse Agent`) from the shared ledger contract. The compact landing may render its API / Agent / API + Agent badge beside the system name, with the full phrase available through accessible metadata and the detail flow, instead of spending a separate table column on collection method. +An agent that is behind the Pulse Agent target or has an update waiting is +maintenance, not a problem: the row's `Agent update` badge says so once and +opens Agent Doctor, so `primaryRowProblem` skips signals marked `maintenance` +(Version behind, Update available) and the row adds no `Version behind` line +beneath itself. Those rows therefore do not count toward the band's `needs +attention` total, which counts them separately and neutrally as agent updates +available. A failed update and auto-update switched off remain row problems, +because each asks for a different action, and the Manage drawer still lists +every fleet signal. A row's problem is picked from all of its visible signals +before the three-highlight cap, and problems rank ahead of maintenance in that +list, so a second attached agent's failed update cannot fall behind another +agent's version lag. The Actions column is only as wide as an install button +beside Manage needs when a row in view shows one, so Health keeps its state, +the update badge and the last-seen time on one line. Source badge class selection may use semantic gray treatment for API-only rows and typed non-gray tones for agent, probe, or combined sources, but the source identity remains the API/Agent/Probe label and subtitle from the shared ledger diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index e459897ac5..12452c53b1 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -7343,6 +7343,11 @@ agent-applied setting, `fleet.configDrift` must remain `not-applicable` and diagnostics, but must not count that passive state as source setup attention. Only a real managed desired config fingerprint can create pending rollout attention when an applied fingerprint is absent or mismatched. +Likewise `fleet.versionDrift: behind` and `fleet.updateStatus: +update-available` are maintenance, not source setup attention: +`useConnectionsLedger.ts` keeps them out of a row's problem and the band's +needs-attention count, which the row's agent-update count and badge carry +instead, while `updateStatus: failed` and `disabled` remain row problems. The `fleet.commandPolicy` object is the canonical desired/applied convergence contract for remote command enablement. It must carry the desired server policy, the applied agent-reported truth when available, the effective diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 1c380ea961..e077067776 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -2633,7 +2633,9 @@ Agent`), with the plain-language source phrase available through accessible always-on version column for Pulse Agent; exact version text belongs in the edit/detail surfaces, while the landing table only surfaces a compact warning badge when an attached or standalone agent actually has an update - available. That same table boundary must reuse the `System` cell for compact + available. That badge is the row's only version flag: the row adds no + `Version behind` problem line beneath it, and the source band counts those + rows as agent updates available rather than as needing attention. That same table boundary must reuse the `System` cell for compact standalone-agent identity such as `Unraid 7.1.0`; raw reported addresses belong in the governed Manage detail or an explicitly expanded cluster-member row, not an always-on diagnostics column. diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 0e47f9e058..5e249dedf6 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1403,6 +1403,17 @@ reporting source of truth. `MultiTenantMonitor.ListOrganizationIDs` may expose persisted organization IDs to API-owned background workers, but it must not initialize monitors, start pollers, or reinterpret tenant IDs as monitored resource health. +Organization deletion waits up to `tenantDeletionShutdownTimeout` (30 seconds) +for the tenant monitoring loop to exit before it refuses with +`tenant_shutdown_incomplete` and keeps the data. Process shutdown and +`RemoveTenant` keep the two-second `tenantMonitorShutdownTimeout`. Deletion +gates the data on that wait, so the budget must cover a loop that is still +starting up or finishing a tick. The observed case is mock mode: +`Monitor.Start` runs `startMockMetricsSampler` synchronously before its select +loop, and `prepareMockMetricsHistory` seeds seven days of fixture history +without a context, so a just-used organization's loop took 8-10 seconds under CI +load to see its cancelled context and the two-second budget turned an ordinary +delete into a refusal. A loop that never exits still refuses. Proxmox physical-disk polling is also a continuity boundary. A failed or permission-denied `disks/list` call must remain an error so the monitor can use linked host-agent inventory or retain same-instance, same-node prior evidence; diff --git a/frontend-modern/browser-verification/8743aba58b02bde2.json b/frontend-modern/browser-verification/8743aba58b02bde2.json new file mode 100644 index 0000000000..29c1b5cf97 --- /dev/null +++ b/frontend-modern/browser-verification/8743aba58b02bde2.json @@ -0,0 +1,53 @@ +{ + "version": 1, + "base_sha": "2218ab6c18cecde801a1a4e920071089032d46a8", + "verified_at": "2026-10-05T16:35:39Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx", + "frontend-modern/src/components/Settings/connectionsTableModel.ts", + "frontend-modern/src/components/Settings/useConnectionsLedger.ts" + ], + "content_sha256": { + "frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx": "0e631a2eaf7ba72e2a70e99ceca0639c82372b8700b831f48d5b93f9dce35ab1", + "frontend-modern/src/components/Settings/connectionsTableModel.ts": "1ee02ad9c25795e44f766d7d4dff615cd2323e3691c30e844a3d5dfa4973c64d", + "frontend-modern/src/components/Settings/useConnectionsLedger.ts": "4761b1a14a6af04225af07036ecf800334a12ab3b1aa418e400ddf4e2bb841ec" + }, + "routes": [ + "/settings/infrastructure" + ], + "viewports": [ + { + "width": 390, + "height": 1600 + }, + { + "width": 1100, + "height": 1000 + }, + { + "width": 1280, + "height": 1000 + }, + { + "width": 1440, + "height": 1100 + }, + { + "width": 1920, + "height": 1000 + } + ], + "states": [ + "Mock estate with 13 connected systems, 11 of them Pulse Agent hosts behind the agent target with an update available", + "Each outdated agent row shows Active, one Agent update badge (opens Agent Doctor) and its last-seen time on one line; main added a second italic Version behind line under every one of the 11 rows", + "The band reads 13 systems connected \u00b7 All active \u00b7 11 agent updates available in a neutral tone and Open Agent Doctor is a secondary button; main read 11 systems needs attention in amber with a primary button although every one was only version lag", + "Health column at 1280 / 1440 / 1920: 228 / 267 / 390px with no clipped cell (main 176 / 216 / 332px with 11 and 6 clipped cells, the last-seen time cut to 18s a...); no cell wraps", + "Phone card layout at 390: neutral band, no Version behind line on any card, 11 Agent update badges; below 1100 the page uses the card layout as before", + "Re-measured after rebasing onto 2218ab6c18 (shared Table row-click change from #2524): same widths, no clipped cells, phone layout unchanged" + ], + "interactions": [ + "Measured column widths and clipped cells on main (reverse-applied patch) and on the change at 1100, 1280, 1440 and 1920", + "Counted Version behind lines and Agent update badges in the live phone layout" + ] +} diff --git a/frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx b/frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx index c6f8bdc6f3..cf9f66f608 100644 --- a/frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx +++ b/frontend-modern/src/components/Settings/InfrastructureSourceManager.tsx @@ -502,6 +502,15 @@ export const InfrastructureSourceManager: Component !props.readOnly; + // Actions holds Manage (about 96px with padding) and, on a row missing agent + // coverage, an install button beside it. Reserving the wide column only when + // a row shows that button leaves Health room for its state, the Agent update + // badge and the last-seen time on one line. + const actionColumnShowsInstall = createMemo( + () => + Boolean(props.onAddSourceStep || props.onAddSource) && + infrastructureRows().some((row) => rowNeedsAgentCoverage(row)), + ); const lastDiscoveryResultText = createMemo(() => formatRelativeTimestamp(props.discoveryScanStatus().lastResultAt), ); @@ -543,6 +552,11 @@ export const InfrastructureSourceManager: Component Boolean(row.problem) || row.members.some((member) => member.problem), ).length, ); + // Outdated agents are maintenance rather than attention (see + // FleetGovernanceSignal.maintenance); the band still counts them once. + const agentUpdateTotal = createMemo(() => + infrastructureRows().reduce((total, row) => total + row.agentUpdateCount, 0), + ); const discoveryMonitorTitle = createMemo(() => { const status = props.discoveryScanStatus(); if (status.scanning) return 'Scanning'; @@ -815,7 +829,16 @@ export const InfrastructureSourceManager: Component - {formatCount(fleetAttentionSystemCount(), 'system')} needs attention + {formatCount(fleetAttentionSystemCount(), 'system')}{' '} + {fleetAttentionSystemCount() === 1 ? 'needs' : 'need'} attention + + + 0}> + + + {formatCount(agentUpdateTotal(), 'agent update')} available 0}> @@ -823,7 +846,8 @@ export const InfrastructureSourceManager: Component - {formatCount(uncoveredAgentTargetCount(), 'host')} has limited coverage + {formatCount(uncoveredAgentTargetCount(), 'host')}{' '} + {uncoveredAgentTargetCount() === 1 ? 'has' : 'have'} limited coverage @@ -897,17 +921,19 @@ export const InfrastructureSourceManager: Component - + System - + Coverage - + Health - + Actions diff --git a/frontend-modern/src/components/Settings/__tests__/InfrastructureSourceManager.test.tsx b/frontend-modern/src/components/Settings/__tests__/InfrastructureSourceManager.test.tsx index 89b27335d3..16e8b825ae 100644 --- a/frontend-modern/src/components/Settings/__tests__/InfrastructureSourceManager.test.tsx +++ b/frontend-modern/src/components/Settings/__tests__/InfrastructureSourceManager.test.tsx @@ -6,7 +6,9 @@ import { agentConnectionIDsForInfrastructureRow, } from '../InfrastructureSourceManager'; import { + fleetGovernanceSignalsForConnection, primaryRowProblem, + visibleFleetGovernanceSignals, type FleetGovernanceSignal, type InfrastructureSystemMemberRow, type InfrastructureSystemRow, @@ -398,6 +400,50 @@ describe('InfrastructureSourceManager setup summary', () => { expect(onOpenAgentDoctor).toHaveBeenCalledWith(['agent:behind-host']); }); + it('flags an outdated agent once, as an update rather than as attention', () => { + const outdated = connectionFixture({ + id: 'agent:outdated-host', + name: 'outdated-host', + agentUpdateAvailable: true, + fleet: { + enrollmentState: 'enrolled', + livenessState: 'active', + versionDrift: 'behind', + adapterHealth: 'healthy', + configRollout: 'reported', + credentialStatus: 'verified', + updateStatus: 'update-available', + remoteControl: 'disabled', + }, + }); + const fleetSignals = fleetGovernanceSignalsForConnection(outdated); + const outdatedRow = row({ + connection: outdated, + agentUpdateCount: 1, + fleetSignals, + fleetHighlights: visibleFleetGovernanceSignals(fleetSignals), + problem: primaryRowProblem(visibleFleetGovernanceSignals(fleetSignals, Infinity)), + }); + + render(() => ( + [outdatedRow]} + discoveredNodes={() => []} + discoveryEnabled={false} + discoveryScanStatus={() => ({ scanning: false })} + readOnly={false} + onOpenAgentDoctor={vi.fn()} + /> + )); + + expect( + screen.getByRole('button', { name: 'Open Agent Doctor for outdated-host' }), + ).toHaveTextContent('Agent update'); + expect(screen.queryByText('Version behind')).toBeNull(); + expect(screen.queryByText(/needs? attention/i)).toBeNull(); + expect(screen.getByText('1 agent update available')).toBeInTheDocument(); + }); + it('still counts actionable member posture when the cluster parent is healthy', () => { render(() => ( { }); }); +describe('primaryRowProblem', () => { + const outdatedFleet = (updateStatus: 'update-available' | 'failed' | 'disabled') => + connectionFixture({ + agentUpdate: + updateStatus === 'failed' + ? { state: 'error', autoUpdate: true, lastError: 'signature verification failed' } + : undefined, + fleet: { + enrollmentState: 'enrolled', + livenessState: 'active', + versionDrift: 'behind', + adapterHealth: 'healthy', + configRollout: 'reported', + credentialStatus: 'verified', + updateStatus, + remoteControl: 'disabled', + }, + }); + + it('leaves an outdated agent to its Agent update badge instead of naming a problem', () => { + const highlights = visibleFleetGovernanceSignals( + fleetGovernanceSignalsForConnection(outdatedFleet('update-available')), + ); + // The Manage drawer keeps the structured signals. + expect(highlights.map((signal) => signal.label)).toEqual([ + 'Version behind', + 'Update available', + ]); + expect(primaryRowProblem(highlights)).toBeUndefined(); + }); + + it("finds a failed update on a second attached agent behind another agent's version lag", () => { + const signals = [ + ...fleetGovernanceSignalsForConnection(outdatedFleet('update-available')), + ...fleetGovernanceSignalsForConnection({ + ...outdatedFleet('failed'), + id: 'agent:host-2', + name: 'host-2', + }), + ]; + expect(primaryRowProblem(visibleFleetGovernanceSignals(signals, Infinity))).toMatchObject({ + label: 'Agent update failed', + tone: 'critical', + }); + // The capped highlight list leads with the problem, not the maintenance. + expect(visibleFleetGovernanceSignals(signals)[0]?.label).toBe('Agent update failed'); + }); + + it('still names a failed update or disabled auto-update, which need a different action', () => { + expect( + primaryRowProblem( + visibleFleetGovernanceSignals(fleetGovernanceSignalsForConnection(outdatedFleet('failed'))), + ), + ).toMatchObject({ label: 'Agent update failed', tone: 'critical' }); + expect( + primaryRowProblem( + visibleFleetGovernanceSignals( + fleetGovernanceSignalsForConnection(outdatedFleet('disabled')), + ), + ), + ).toMatchObject({ label: 'Auto-update off', tone: 'warning' }); + }); +}); + describe('fleetGovernanceSignalsForConnection connection-type gating', () => { it('omits agent-fleet governance signals for pull-based API sources', () => { const signals = fleetGovernanceSignalsForConnection( diff --git a/frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts b/frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts index 5ed24fe4ae..768cf83f60 100644 --- a/frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts @@ -1665,8 +1665,12 @@ describe('settings architecture guardrails', () => { expect(infrastructureSourceManagerSource).toContain('Connection posture'); expect(infrastructureSourceManagerSource).toContain("'system')} connected"); expect(infrastructureSourceManagerSource).toContain('All active'); - expect(infrastructureSourceManagerSource).toContain('needs attention'); - expect(infrastructureSourceManagerSource).toContain('has limited coverage'); + expect(infrastructureSourceManagerSource).toContain( + "{fleetAttentionSystemCount() === 1 ? 'needs' : 'need'} attention", + ); + expect(infrastructureSourceManagerSource).toContain( + "{uncoveredAgentTargetCount() === 1 ? 'has' : 'have'} limited coverage", + ); expect(infrastructureSourceManagerSource).toContain('setupConfidenceAction'); expect(infrastructureSourceManagerSource).not.toContain('Infrastructure coverage'); expect(infrastructureSourceManagerSource).not.toContain('Fleet governance'); diff --git a/frontend-modern/src/components/Settings/__tests__/useConnectionsLedger.test.ts b/frontend-modern/src/components/Settings/__tests__/useConnectionsLedger.test.ts index b72056ef39..74821b59e1 100644 --- a/frontend-modern/src/components/Settings/__tests__/useConnectionsLedger.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/useConnectionsLedger.test.ts @@ -552,6 +552,8 @@ describe('useConnectionsLedger', () => { 'Rollout pending', 'Version behind', ]); + // Version lag rides on the Agent update badge; the problem line names the drift. + expect(byID.get('agent:drifted')?.problem?.label).toBe('Config drift'); // Pull-based API sources (PVE/PBS/etc.) have no Pulse Agent, so agent-fleet // governance (rollout/config/version/command-policy) must not surface on // them. Only source-agnostic posture like credential health applies; an diff --git a/frontend-modern/src/components/Settings/connectionsTableModel.ts b/frontend-modern/src/components/Settings/connectionsTableModel.ts index 5c1c874426..7bbc204974 100644 --- a/frontend-modern/src/components/Settings/connectionsTableModel.ts +++ b/frontend-modern/src/components/Settings/connectionsTableModel.ts @@ -256,6 +256,11 @@ export interface FleetGovernanceSignal { label: string; detail: string; tone: FleetGovernanceSignalTone; + // An agent that is behind or has an update waiting is maintenance, not a + // problem: the row's Agent update badge already says so and opens Agent + // Doctor, so the row does not repeat it as its problem line or count it as + // needing attention. The Manage drawer still lists the signal. + maintenance?: boolean; } const DEFAULT_FLEET_GOVERNANCE: ConnectionFleetGovernance = { @@ -462,6 +467,7 @@ const versionSignal = (state: ConnectionFleetVersionDrift): FleetGovernanceSigna label: 'Version behind', detail: 'This agent is behind the current Pulse Agent target.', tone: 'warning', + maintenance: true, }; case 'current': return { @@ -686,6 +692,7 @@ const updateSignal = ( label: 'Update available', detail: 'A newer Pulse Agent binary is available for this system.', tone: 'warning', + maintenance: true, }; case 'checking': return { @@ -906,8 +913,12 @@ export const fleetGovernanceSignalsForConnection = ( return signals; }; +// The row shows at most three highlights. A row's problem is picked from the +// whole visible list (limit Infinity) so a critical signal from a second +// attached agent cannot fall behind the cap. export const visibleFleetGovernanceSignals = ( signals: readonly FleetGovernanceSignal[], + limit = 3, ): FleetGovernanceSignal[] => { const hasPassiveAgentConfigConfirmation = signals.some(isPassiveAgentConfigConfirmationSignal); const visibleSignals = signals.filter((signal) => { @@ -929,11 +940,14 @@ export const visibleFleetGovernanceSignals = ( if (signal.key === 'command-policy' && signal.tone === 'info') return false; return true; }); + // Problems lead; maintenance (an agent behind or with an update waiting) + // follows them, since the Agent update badge already carries it. const attention = visibleSignals.filter( - (signal) => signal.tone === 'critical' || signal.tone === 'warning', + (signal) => !signal.maintenance && (signal.tone === 'critical' || signal.tone === 'warning'), ); - const control = visibleSignals.filter((signal) => signal.tone === 'info'); - return [...attention, ...control].slice(0, 3); + const maintenance = visibleSignals.filter((signal) => signal.maintenance); + const control = visibleSignals.filter((signal) => !signal.maintenance && signal.tone === 'info'); + return [...attention, ...maintenance, ...control].slice(0, limit); }; const isPassiveAgentConfigConfirmationSignal = (signal: FleetGovernanceSignal): boolean => { @@ -1027,9 +1041,11 @@ export interface InfrastructureSystemRow { // Pick the single most important problem to surface in the row's status // column. Critical beats warning; otherwise first wins — the upstream // builder has already prioritised highlights (attention before info). +// Maintenance signals never become the problem (see FleetGovernanceSignal). export const primaryRowProblem = ( - signals: readonly FleetGovernanceSignal[], + allSignals: readonly FleetGovernanceSignal[], ): InfrastructureRowProblem | undefined => { + const signals = allSignals.filter((signal) => !signal.maintenance); const critical = signals.find((signal) => signal.tone === 'critical'); if (critical) { return { label: critical.label, detail: critical.detail, tone: 'critical' }; diff --git a/frontend-modern/src/components/Settings/useConnectionsLedger.ts b/frontend-modern/src/components/Settings/useConnectionsLedger.ts index 2ab8538b99..5a54535676 100644 --- a/frontend-modern/src/components/Settings/useConnectionsLedger.ts +++ b/frontend-modern/src/components/Settings/useConnectionsLedger.ts @@ -307,7 +307,7 @@ const buildMemberRow = ( // still rolls up from the prioritised highlight list. const problem = (agentConnection && agentAttachmentProblem(agentConnection)) ?? - primaryRowProblem(fleetHighlights); + primaryRowProblem(visibleFleetGovernanceSignals(fleetSignals, Infinity)); return { id: member.id, nodeIdentity: member.nodeIdentity?.trim() || undefined, @@ -423,7 +423,8 @@ const buildRow = ( lastErrorMessage, fleetSignals, fleetHighlights, - problem: attachmentProblem ?? primaryRowProblem(fleetHighlights), + problem: + attachmentProblem ?? primaryRowProblem(visibleFleetGovernanceSignals(fleetSignals, Infinity)), enabled: primaryConnection.enabled, canEdit: EDITABLE_CONNECTION_TYPES.includes(primaryConnection.type), canPause: primaryConnection.capabilities.supportsPause, diff --git a/internal/monitoring/guest_metadata_shutdown_test.go b/internal/monitoring/guest_metadata_shutdown_test.go index 8c9ad302ee..074518a4b9 100644 --- a/internal/monitoring/guest_metadata_shutdown_test.go +++ b/internal/monitoring/guest_metadata_shutdown_test.go @@ -79,6 +79,7 @@ func TestTenantDeletionRetainsBlockedGuestWriterOwnership(t *testing.T) { } } func TestTenantDeletionRetainsUnfinishedMonitorLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 100*time.Millisecond) mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) _, cancel := context.WithCancel(context.Background()) done := make(chan struct{}) diff --git a/internal/monitoring/mock_metrics_history.go b/internal/monitoring/mock_metrics_history.go index 6b10b55612..487fb7d1c4 100644 --- a/internal/monitoring/mock_metrics_history.go +++ b/internal/monitoring/mock_metrics_history.go @@ -540,13 +540,18 @@ func canonicalMetricSeriesWithSampler(sampler mock.MetricSampler, resourceType, return sampler.SampleMetricSeries(resourceType, resourceID, metric, timestamps) } -func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.FixtureGraph, now time.Time, seedDuration, interval time.Duration) { +// seedMockMetricsHistory returns ctx.Err() when ctx is cancelled partway. A +// large demo estate takes several seconds to seed, and a tenant monitor's loop +// only sees its cancelled context once seeding returns, so organization +// deletion would otherwise wait for the whole estate. Each series that was +// sampled is recorded whole; the rest are skipped. +func seedMockMetricsHistory(ctx context.Context, mh *MetricsHistory, ms *metrics.Store, graph mock.FixtureGraph, now time.Time, seedDuration, interval time.Duration) error { if mh == nil { - return + return nil } state := graph.State if seedDuration <= 0 || interval <= 0 { - return + return nil } sampler := mock.NewMetricSampler(graph) now = normalizeMockMetricTimestamp(now, interval) @@ -669,6 +674,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi } } recordStorageTimeline := func(storageID string, currentTotal float64) { + if ctx.Err() != nil { + return + } if strings.TrimSpace(storageID) == "" || currentTotal <= 0 || numPoints == 0 { return } @@ -693,6 +701,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi } recordNode := func(node models.Node) { + if ctx.Err() != nil { + return + } if node.ID == "" { return } @@ -719,7 +730,7 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi includeDiskIO bool, includeNetwork bool, ) { - if len(metricIDs) == 0 || storeID == "" { + if ctx.Err() != nil || len(metricIDs) == 0 || storeID == "" { return } uniqueMetricIDs := make([]string, 0, len(metricIDs)) @@ -784,7 +795,7 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi seedStoreSeries(storeType, storeID, storeMetrics...) } recordGuestTemperature := func(metricIDs []string, storeType, storeID string) { - if len(metricIDs) == 0 || strings.TrimSpace(storeID) == "" { + if ctx.Err() != nil || len(metricIDs) == 0 || strings.TrimSpace(storeID) == "" { return } temperatureSeries := canonicalMetricSeriesWithSampler( @@ -802,7 +813,7 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi seedStoreSeries(storeType, storeID, "temperature") } recordGuestMemoryUsed := func(metricIDs []string, storeType, storeID string, memoryTotal float64) { - if len(metricIDs) == 0 || strings.TrimSpace(storeID) == "" || memoryTotal <= 0 { + if ctx.Err() != nil || len(metricIDs) == 0 || strings.TrimSpace(storeID) == "" || memoryTotal <= 0 { return } percentSeries := canonicalMetricSeriesWithSampler(sampler, storeType, storeID, "memory", seedTimestamps) @@ -915,6 +926,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi // series for one physical disk, shared by the native and TrueNAS fixture // disk loops below. seedDiskTelemetry := func(resourceID string) { + if ctx.Err() != nil { + return + } tempSeries := canonicalMetricSeriesWithSampler(sampler, "disk", resourceID, "smart_temp", seedTimestamps) busySeries := canonicalMetricSeriesWithSampler(sampler, "disk", resourceID, "disk", seedTimestamps) diskReadSeries := canonicalMetricSeriesWithSampler(sampler, "disk", resourceID, "diskread", seedTimestamps) @@ -1025,6 +1039,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi recordGuestTemperature(systemMetricIDs, "agent", trueNASFixtures.System.Hostname) for _, pool := range trueNASFixtures.Pools { + if ctx.Err() != nil { + break + } poolKey := mock.TrueNASPoolMetricID(trueNASFixtures.System.Hostname, pool.Name) diskSeries := canonicalMetricSeriesWithSampler(sampler, "storage", poolKey, "usage", seedTimestamps) mh.addGuestMetricSeries(poolKey, "disk", diskSeries, seedTimestamps) @@ -1032,6 +1049,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi } for _, dataset := range trueNASFixtures.Datasets { + if ctx.Err() != nil { + break + } dsKey := mock.TrueNASDatasetMetricID(trueNASFixtures.System.Hostname, dataset.Name) totalBytes := dataset.UsedBytes + dataset.AvailBytes diskSeries := canonicalMetricSeriesWithSampler(sampler, "storage", dsKey, "usage", seedTimestamps) @@ -1115,6 +1135,9 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi } flushStoreBatch() + if err := ctx.Err(); err != nil { + return err + } if ms != nil { log.Info(). Int("rows", storeRows). @@ -1122,9 +1145,11 @@ func seedMockMetricsHistory(mh *MetricsHistory, ms *metrics.Store, graph mock.Fi Msg("mock seeding: metrics store backfill completed") } log.Debug().Msg("mock seeding: completed") + return nil } func prepareMockMetricsHistory( + ctx context.Context, graph mock.FixtureGraph, fixtureRevision uint64, now time.Time, @@ -1132,7 +1157,7 @@ func prepareMockMetricsHistory( interval time.Duration, maxDataPoints int, storeSink *metrics.Store, -) (*MetricsHistory, bool) { +) (*MetricsHistory, bool, error) { // Enforce the eager-history cardinality limit at the allocation/cache // boundary. Callers may retain the complete fixture graph for inventory and // realtime updates, but neither the active history nor the reusable seed @@ -1141,8 +1166,10 @@ func prepareMockMetricsHistory( seedAt := normalizeMockMetricTimestamp(now, interval) if storeSink != nil { history := NewMetricsHistory(maxDataPoints, seedDuration) - seedMockMetricsHistory(history, storeSink, graph, seedAt, seedDuration, interval) - return history, false + if err := seedMockMetricsHistory(ctx, history, storeSink, graph, seedAt, seedDuration, interval); err != nil { + return nil, false, err + } + return history, false, nil } key := mockMetricsSeedCacheKey{ @@ -1155,12 +1182,20 @@ func prepareMockMetricsHistory( mockMetricsSeedCache.Lock() defer mockMetricsSeedCache.Unlock() + // The lock is held for a whole seed, so a monitor may have been cancelled + // while it waited behind another tenant's. + if err := ctx.Err(); err != nil { + return nil, false, err + } if mockMetricsSeedCache.history != nil && mockMetricsSeedCache.key == key { - return mockMetricsSeedCache.history.clone(), true + return mockMetricsSeedCache.history.clone(), true, nil } template := NewMetricsHistory(maxDataPoints, seedDuration) - seedMockMetricsHistory(template, nil, graph, seedAt, seedDuration, interval) + if err := seedMockMetricsHistory(ctx, template, nil, graph, seedAt, seedDuration, interval); err != nil { + // A partial template must never be reused by another tenant. + return nil, false, err + } mockMetricsSeedCache.key = key mockMetricsSeedCache.history = template mockMetricsSeedCache.generation++ @@ -1168,7 +1203,7 @@ func prepareMockMetricsHistory( time.AfterFunc(mockMetricsSeedCacheTTL, func() { expireMockMetricsSeedCache(generation) }) - return template.clone(), false + return template.clone(), false, nil } // recordTrueNASFixturesMetrics records live fixture ticks for TrueNAS host, @@ -1911,7 +1946,8 @@ func (m *Monitor) startMockMetricsSampler(ctx context.Context) { log.Warn().Msg("PULSE_MOCK_SEED_METRICS_STORE is set but the metrics store is unavailable; mock report history will stay empty") } } - history, cacheHit := prepareMockMetricsHistory( + history, cacheHit, err := prepareMockMetricsHistory( + samplerCtx, historyGraph, fixtureRevision, time.Now(), @@ -1920,6 +1956,13 @@ func (m *Monitor) startMockMetricsSampler(ctx context.Context) { maxPoints, storeSink, ) + if err != nil { + // The monitor is stopping. Return before warming chart caches or + // starting the sampler so its loop sees the cancellation now; the + // caller's stopMockMetricsSampler releases mockMetricsCancel. + log.Info().Err(err).Msg("Mock metrics sampler: seeding cancelled") + return + } m.mu.Lock() m.metricsHistory = history m.mu.Unlock() diff --git a/internal/monitoring/mock_metrics_history_cancel_test.go b/internal/monitoring/mock_metrics_history_cancel_test.go new file mode 100644 index 0000000000..3794848884 --- /dev/null +++ b/internal/monitoring/mock_metrics_history_cancel_test.go @@ -0,0 +1,126 @@ +package monitoring + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/mock" + "github.com/rcourtman/pulse-go-rewrite/internal/models" +) + +// cancelAfterChecks reports cancellation once Err has been consulted more +// than allowed times, so a test can cancel seeding partway through without +// racing a timer. +type cancelAfterChecks struct { + context.Context + remaining atomic.Int32 +} + +func newCancelAfterChecks(allowed int32) *cancelAfterChecks { + ctx := &cancelAfterChecks{Context: context.Background()} + ctx.remaining.Store(allowed) + return ctx +} + +func (c *cancelAfterChecks) Err() error { + if c.remaining.Add(-1) < 0 { + return context.Canceled + } + return nil +} + +func threeNodeMockGraph() mock.FixtureGraph { + return fixtureGraphWithState(models.StateSnapshot{ + Nodes: []models.Node{ + {ID: "node-a", Name: "node-a", Status: "online", CPU: 0.2, Memory: models.Memory{Usage: 40, Total: 1024}}, + {ID: "node-b", Name: "node-b", Status: "online", CPU: 0.3, Memory: models.Memory{Usage: 50, Total: 1024}}, + {ID: "node-c", Name: "node-c", Status: "online", CPU: 0.4, Memory: models.Memory{Usage: 60, Total: 1024}}, + }, + }) +} + +// Org deletion refused with tenant_shutdown_incomplete because a tenant +// monitor's loop only sees its cancelled context after mock seeding returns, +// and seeding a large demo estate took 4-11s in CI. Seeding must stop at the +// next resource instead of finishing the estate. +func TestSeedMockMetricsHistoryStopsPartwayWhenCancelled(t *testing.T) { + now := time.Now().UTC().Truncate(time.Minute) + mh := NewMetricsHistory(3500, time.Hour) + + err := seedMockMetricsHistory(newCancelAfterChecks(1), mh, nil, threeNodeMockGraph(), now, time.Hour, time.Minute) + if !errors.Is(err, context.Canceled) { + t.Fatalf("seedMockMetricsHistory error = %v, want context.Canceled", err) + } + if len(mh.GetNodeMetrics("node-a", "cpu", time.Hour)) == 0 { + t.Fatal("node seeded before cancellation has no history") + } + for _, nodeID := range []string{"node-b", "node-c"} { + if got := len(mh.GetNodeMetrics(nodeID, "cpu", time.Hour)); got != 0 { + t.Fatalf("node %s seeded %d points after cancellation", nodeID, got) + } + } +} + +func TestPrepareMockMetricsHistoryDoesNotCacheCancelledSeed(t *testing.T) { + resetMockMetricsSeedCacheForTest(t) + now := time.Now().UTC().Truncate(time.Minute) + graph := threeNodeMockGraph() + + // One check passes the cache lock, one seeds node-a, then cancellation. + history, cacheHit, err := prepareMockMetricsHistory(newCancelAfterChecks(2), graph, 7, now, time.Hour, time.Minute, 3500, nil) + if !errors.Is(err, context.Canceled) || history != nil || cacheHit { + t.Fatalf("cancelled prepare = (%v, %v, %v), want (nil, false, context.Canceled)", history, cacheHit, err) + } + mockMetricsSeedCache.Lock() + cached := mockMetricsSeedCache.history + mockMetricsSeedCache.Unlock() + if cached != nil { + t.Fatal("cancelled seed left a partial template for other tenants to reuse") + } + + history, cacheHit, err = prepareMockMetricsHistory(context.Background(), graph, 7, now, time.Hour, time.Minute, 3500, nil) + if err != nil || cacheHit { + t.Fatalf("seed after cancellation = (cacheHit %v, err %v), want a fresh seed", cacheHit, err) + } + if len(history.GetNodeMetrics("node-c", "cpu", time.Hour)) == 0 { + t.Fatal("fresh seed after cancellation is incomplete") + } +} + +func TestStartMockMetricsSamplerSkipsSeedingWhenMonitorStopping(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, time.Minute) + resetMockMetricsSeedCacheForTest(t) + previousEnabled := mock.IsMockEnabled() + previousConfig := mock.GetConfig() + t.Cleanup(func() { + mustSetMockEnabled(t, false) + mock.SetMockConfig(previousConfig) + if previousEnabled { + mustSetMockEnabled(t, true) + } + }) + mustSetMockEnabled(t, false) + mock.SetMockConfig(compactMockFixtureConfig()) + mustSetMockEnabled(t, true) + + nodes := mock.CurrentFixtureGraph().State.Nodes + if len(nodes) == 0 { + t.Fatal("expected mock fixture nodes") + } + monitor := &Monitor{ + metricsHistory: NewMetricsHistory(1000, time.Hour), + state: models.NewState(), + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + monitor.startMockMetricsSampler(ctx) + t.Cleanup(monitor.stopMockMetricsSampler) + + if got := len(monitor.metricsHistory.GetNodeMetrics(nodes[0].ID, "cpu", time.Hour)); got != 0 { + t.Fatalf("stopping monitor seeded %d points for node %s", got, nodes[0].ID) + } +} diff --git a/internal/monitoring/mock_metrics_history_test.go b/internal/monitoring/mock_metrics_history_test.go index 15a7cae996..d9defb1050 100644 --- a/internal/monitoring/mock_metrics_history_test.go +++ b/internal/monitoring/mock_metrics_history_test.go @@ -176,7 +176,7 @@ func TestSeedMockMetricsHistory_PopulatesSeries(t *testing.T) { } mh := NewMetricsHistory(1000, 24*time.Hour) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) nodeCPU := mh.GetNodeMetrics("node-1", "cpu", time.Hour) if len(nodeCPU) < 10 { @@ -285,7 +285,7 @@ func TestSeedMockMetricsHistory_AppendsSingleTerminalNowPoint(t *testing.T) { } mh := NewMetricsHistory(1000, 24*time.Hour) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) memorySeries := mh.GetGuestMetrics("docker:cont-1", "memory", time.Hour) if len(memorySeries) < 2 { @@ -334,7 +334,7 @@ func TestSeedMockMetricsHistory_PopulatesKubernetesPodSeries(t *testing.T) { } mh := NewMetricsHistory(1000, 24*time.Hour) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) metricID := kubernetesPodMetricID(state.KubernetesClusters[0], state.KubernetesClusters[0].Pods[0]) if metricID == "" { @@ -387,7 +387,7 @@ func TestSeedMockMetricsHistory_PopulatesKubernetesClusterNodeAndDeploymentSerie } mh := NewMetricsHistory(1000, 24*time.Hour) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(state), now, time.Hour, interval) clusterMetricID := kubernetesClusterMetricID(state.KubernetesClusters[0]) nodeMetricID := kubernetesNodeMetricID(state.KubernetesClusters[0], state.KubernetesClusters[0].Nodes[0]) @@ -468,7 +468,7 @@ func TestSeedMockMetricsHistory_SeedsMetricsStore(t *testing.T) { defer store.Close() mh := NewMetricsHistory(1000, seedDuration) - seedMockMetricsHistory(mh, store, fixtureGraphWithState(state), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, store, fixtureGraphWithState(state), now, seedDuration, time.Minute) points, err := store.Query("vm", "vm-100", "cpu", now.Add(-seedDuration), now, 3600) if err != nil { @@ -531,7 +531,7 @@ func TestSeedMockMetricsHistory_StoreSeedGapFillsWithoutDuplicates(t *testing.T) return len(points) } - seedMockMetricsHistory(NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), now, seedDuration, time.Minute) first := countPoints(now) if first == 0 { t.Fatal("expected initial store seed to write minute-tier points") @@ -539,7 +539,7 @@ func TestSeedMockMetricsHistory_StoreSeedGapFillsWithoutDuplicates(t *testing.T) // Re-seeding at the same instant must not add rows: timestamps sit on the // tier spacing grids and existing coverage short-circuits the backfill. - seedMockMetricsHistory(NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), now, seedDuration, time.Minute) if got := countPoints(now); got != first { t.Fatalf("expected idempotent reseed to keep %d points, got %d", first, got) } @@ -560,7 +560,7 @@ func TestSeedMockMetricsHistory_StoreSeedGapFillsWithoutDuplicates(t *testing.T) if gapPoints == 0 { t.Fatal("expected the advanced clock to expose new minute-tier grid points") } - seedMockMetricsHistory(NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), later, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), NewMetricsHistory(1000, seedDuration), store, fixtureGraphWithState(state), later, seedDuration, time.Minute) if got, want := countPoints(later), first+gapPoints; got != want { t.Fatalf("expected gap fill to append exactly the missing grid points (%d), got %d", want, got) } @@ -597,7 +597,7 @@ func TestSeedMockMetricsHistory_SeedsDiskTemperatureMetricsStore(t *testing.T) { defer store.Close() mh := NewMetricsHistory(1000, seedDuration) - seedMockMetricsHistory(mh, store, fixtureGraphWithState(state), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, store, fixtureGraphWithState(state), now, seedDuration, time.Minute) points, err := store.Query("disk", "SERIAL-001", "smart_temp", now.Add(-seedDuration), now, 3600) if err != nil { @@ -634,7 +634,7 @@ func TestSeedMockMetricsHistory_SeedsVMwareMetricsStore(t *testing.T) { }, } sampler := mock.NewMetricSampler(graph) - seedMockMetricsHistory(mh, store, graph, now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, store, graph, now, seedDuration, time.Minute) hostPoints, err := store.Query("agent", "vc-mock-1:host:host-101", "cpu", now.Add(-seedDuration), now, 3600) if err != nil { @@ -707,7 +707,7 @@ func TestSeedMockMetricsHistory_SeedsTrueNASMetricsStore(t *testing.T) { }, } sampler := mock.NewMetricSampler(graph) - seedMockMetricsHistory(mh, store, graph, now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, store, graph, now, seedDuration, time.Minute) systemPoints, err := store.Query("agent", fixtures.System.Hostname, "disk", now.Add(-seedDuration), now, 3600) if err != nil { @@ -814,7 +814,7 @@ func TestSeedMockMetricsHistory_UsesCanonicalMockFixtureGraphForLegacyAndProvide defer store.Close() mh := NewMetricsHistory(1000, historyRetention) - seedMockMetricsHistory(mh, store, graph, now, seedDuration, sampleInterval) + seedMockMetricsHistory(context.Background(), mh, store, graph, now, seedDuration, sampleInterval) nodePoints, err := store.Query("node", graph.State.Nodes[0].ID, "cpu", now.Add(-seedDuration), now, 3600) if err != nil { @@ -1455,7 +1455,7 @@ func TestSeedMockMetricsHistory_StaysContinuousWithSubsequentLiveMockTicks(t *te } mh := NewMetricsHistory(5000, seedDuration) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(seedState), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(seedState), now, seedDuration, time.Minute) vmCPUSeeded := mh.GetGuestMetrics("vm-tail", "cpu", seedDuration) if len(vmCPUSeeded) == 0 { @@ -1626,7 +1626,7 @@ func TestRecordMockStateToMetricsHistory_ContinuesCanonicalKubernetesClusterNode deploymentMetricID := kubernetesDeploymentMetricID(seedState.KubernetesClusters[0], seedState.KubernetesClusters[0].Deployments[0]) mh := NewMetricsHistory(5000, seedDuration) - seedMockMetricsHistory(mh, nil, fixtureGraphWithState(seedState), now, seedDuration, time.Minute) + seedMockMetricsHistory(context.Background(), mh, nil, fixtureGraphWithState(seedState), now, seedDuration, time.Minute) recordMockStateToMetricsHistory(mh, nil, fixtureGraphWithState(seedState), next) for name, metricID := range map[string]string{ @@ -1664,7 +1664,7 @@ func TestMockMetricsHistoryKeepsGraphPersonaAcrossSeedAndLiveBoundary(t *testing sampler := mock.NewMetricSampler(graph) mh := NewMetricsHistory(5000, boundedMockHistoryProofWindow) - seedMockMetricsHistory(mh, nil, graph, now, boundedMockHistoryProofWindow, time.Minute) + seedMockMetricsHistory(context.Background(), mh, nil, graph, now, boundedMockHistoryProofWindow, time.Minute) recordMockStateToMetricsHistory(mh, nil, graph, next) series := mh.GetGuestMetrics(resourceID, "memory", boundedMockHistoryProofWindow) @@ -1699,7 +1699,7 @@ func TestSeedMockMetricsHistory_UsesCanonicalMetricModelForPlatformFixtures(t *t } mh := NewMetricsHistory(20_000, 4*time.Hour) - seedMockMetricsHistory(mh, nil, graph, ts, 4*time.Hour, time.Minute) + seedMockMetricsHistory(context.Background(), mh, nil, graph, ts, 4*time.Hour, time.Minute) assertCanonicalSeries := func(series []MetricPoint, resourceType, resourceID, metric string) { t.Helper() @@ -1765,7 +1765,7 @@ func TestSeedMockMetricsHistory_NormalizesTerminalTimestampToCanonicalGrid(t *te }) mh := NewMetricsHistory(4_000, 2*time.Hour) - seedMockMetricsHistory(mh, nil, graph, now, 2*time.Hour, time.Minute) + seedMockMetricsHistory(context.Background(), mh, nil, graph, now, 2*time.Hour, time.Minute) points := mh.GetGuestMetrics("vm-grid", "cpu", 2*time.Hour) if len(points) == 0 { @@ -2030,7 +2030,7 @@ func TestSeedMockMetricsHistory_DiskTelemetryParityAcrossNativeAndTrueNAS(t *tes } mh := NewMetricsHistory(1000, seedDuration) - seedMockMetricsHistory(mh, nil, mock.FixtureGraph{ + seedMockMetricsHistory(context.Background(), mh, nil, mock.FixtureGraph{ State: state, PlatformFixtures: mock.PlatformFixtures{ TrueNAS: fixtures, @@ -2064,7 +2064,7 @@ func TestMockDockerHostSeedCoversTheSameSeriesAsTheSyntheticGenerator(t *testing } now := time.Now().UTC() - seedMockMetricsHistory(history, nil, graph, now, 2*time.Hour, time.Minute) + seedMockMetricsHistory(context.Background(), history, nil, graph, now, 2*time.Hour, time.Minute) hostID := graph.State.DockerHosts[0].ID for _, metricType := range mockHostChartMetricTypes { @@ -2087,7 +2087,7 @@ func TestMockDockerContainerSeedCoversDrawerHistorySeries(t *testing.T) { } now := time.Now().UTC() - seedMockMetricsHistory(history, nil, graph, now, 2*time.Hour, time.Minute) + seedMockMetricsHistory(context.Background(), history, nil, graph, now, 2*time.Hour, time.Minute) containerID := graph.State.DockerHosts[0].Containers[0].ID for _, metricType := range mockGuestChartMetricTypes { @@ -2110,7 +2110,7 @@ func TestMockNodeSeedCoversDrawerHistorySeries(t *testing.T) { } now := time.Now().UTC() - seedMockMetricsHistory(history, nil, graph, now, 2*time.Hour, time.Minute) + seedMockMetricsHistory(context.Background(), history, nil, graph, now, 2*time.Hour, time.Minute) nodeID := graph.State.Nodes[0].ID for _, metricType := range mockNodeChartMetricTypes { diff --git a/internal/monitoring/mock_metrics_seed_cache_test.go b/internal/monitoring/mock_metrics_seed_cache_test.go index 3f2bddf583..7fee313de5 100644 --- a/internal/monitoring/mock_metrics_seed_cache_test.go +++ b/internal/monitoring/mock_metrics_seed_cache_test.go @@ -1,6 +1,7 @@ package monitoring import ( + "context" "testing" "time" @@ -52,7 +53,8 @@ func TestPrepareMockMetricsHistoryReusesRevisionSeedWithIndependentHistories(t * }, }) - first, firstCacheHit := prepareMockMetricsHistory( + first, firstCacheHit, _ := prepareMockMetricsHistory( + context.Background(), graph, fixtureRevision, now, @@ -64,7 +66,8 @@ func TestPrepareMockMetricsHistoryReusesRevisionSeedWithIndependentHistories(t * if firstCacheHit { t.Fatal("first seed unexpectedly reported a cache hit") } - second, secondCacheHit := prepareMockMetricsHistory( + second, secondCacheHit, _ := prepareMockMetricsHistory( + context.Background(), graph, fixtureRevision, now, @@ -84,7 +87,8 @@ func TestPrepareMockMetricsHistoryReusesRevisionSeedWithIndependentHistories(t * firstGeneration := mockMetricsSeedCache.generation mockMetricsSeedCache.Unlock() - _, revisionCacheHit := prepareMockMetricsHistory( + _, revisionCacheHit, _ := prepareMockMetricsHistory( + context.Background(), graph, fixtureRevision+1, now, @@ -163,7 +167,7 @@ func TestPrepareMockMetricsHistoryBoundsFullDemoEstateBeforeCaching(t *testing.T } now := time.Now().UTC().Truncate(time.Minute) - history, cacheHit := prepareMockMetricsHistory(full, 99, now, time.Hour, time.Minute, 3500, nil) + history, cacheHit, _ := prepareMockMetricsHistory(context.Background(), full, 99, now, time.Hour, time.Minute, 3500, nil) if cacheHit { t.Fatal("first full-estate seed unexpectedly reported a cache hit") } diff --git a/internal/monitoring/monitor_additional_test.go b/internal/monitoring/monitor_additional_test.go index 15a9875f7b..bc771e5d42 100644 --- a/internal/monitoring/monitor_additional_test.go +++ b/internal/monitoring/monitor_additional_test.go @@ -1102,3 +1102,59 @@ func TestAutomaticGuestMemoryLinkNextPoll(t *testing.T) { t.Fatalf("ambiguous hostname selected %q/%q/%q", node, linkedVM, ct) } } + +// shortenTenantShutdownBudgets makes the shutdown waits fast enough for tests +// while keeping deletion's budget longer than the plain one, as in production. +func shortenTenantShutdownBudgets(t *testing.T, plain, deletion time.Duration) { + t.Helper() + previousPlain, previousDeletion := tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = plain, deletion + t.Cleanup(func() { + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = previousPlain, previousDeletion + }) +} + +// A tenant monitor that is still starting up or finishing a tick can take +// longer than the plain shutdown budget to see its cancelled context. In CI's +// mock mode, Start seeds mock metrics history synchronously before its select +// loop, so a just-used organization's loop took 8-10s to stop and deletion +// refused with tenant_shutdown_incomplete. Deletion waits on its own, longer +// budget instead. +func TestTenantDeletionWaitsForSlowMonitoringLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 2*time.Second) + + slowLoop := func(mtm *MultiTenantMonitor, orgID string) *Monitor { + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + <-ctx.Done() + // Exits well after the plain budget but inside deletion's. + time.Sleep(300 * time.Millisecond) + close(done) + }() + monitor := &Monitor{} + mtm.monitors[orgID] = monitor + mtm.tenantCancel[orgID] = cancel + mtm.tenantDone[orgID] = done + return monitor + } + + mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) + t.Cleanup(mtm.Stop) + + // RemoveTenant keeps the plain budget, gives up and retains the owner. + retained := slowLoop(mtm, "removal") + mtm.RemoveTenant("removal") + if got, ok := mtm.PeekMonitor("removal"); !ok || got != retained { + t.Fatal("plain removal discarded a monitor whose loop had not stopped") + } + + slowLoop(mtm, "deletion") + if err := mtm.BeginTenantDeletion("deletion"); err != nil { + t.Fatalf("deletion refused a loop that stopped inside its budget: %v", err) + } + if _, ok := mtm.PeekMonitor("deletion"); ok { + t.Fatal("completed deletion left the monitor registered") + } + mtm.FinishTenantDeletion("deletion") +} diff --git a/internal/monitoring/multi_tenant_monitor.go b/internal/monitoring/multi_tenant_monitor.go index 248b842239..a11950ad1f 100644 --- a/internal/monitoring/multi_tenant_monitor.go +++ b/internal/monitoring/multi_tenant_monitor.go @@ -47,7 +47,19 @@ func NewMultiTenantMonitor(baseCfg *config.Config, persistence *config.MultiTena } } -const tenantMonitorShutdownTimeout = 2 * time.Second +// Both shutdown budgets are variables so tests can shorten them. +var ( + // tenantMonitorShutdownTimeout bounds how long process shutdown and + // RemoveTenant wait for a tenant's monitoring loop to exit. + tenantMonitorShutdownTimeout = 2 * time.Second + + // tenantDeletionShutdownTimeout bounds how long organization deletion + // waits for the loop. Deletion refuses, and keeps the data, while the loop + // still runs, so this is a correctness gate rather than a best-effort + // wait: a loop that is still starting up or finishing a tick can take + // several seconds to see its cancelled context. + tenantDeletionShutdownTimeout = 30 * time.Second +) // SetRecoveryManager wires a recovery store manager into all existing and future tenant monitors. func (mtm *MultiTenantMonitor) SetRecoveryManager(manager *recoverymanager.Manager) { @@ -377,7 +389,11 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { if cancel != nil { cancel() } - if !waitForTenantMonitorShutdown(orgID, done) { + shutdownTimeout := tenantMonitorShutdownTimeout + if deleting { + shutdownTimeout = tenantDeletionShutdownTimeout + } + if !waitForTenantMonitorShutdown(orgID, done, shutdownTimeout) { return fmt.Errorf("organization %q monitoring loop has not stopped", orgID) } if monitor != nil { @@ -397,19 +413,19 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { return nil } -func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}) bool { +func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}, timeout time.Duration) bool { if done == nil { return true } - timer := time.NewTimer(tenantMonitorShutdownTimeout) + timer := time.NewTimer(timeout) defer timer.Stop() select { case <-done: return true case <-timer.C: - logger := log.Warn().Dur("timeout", tenantMonitorShutdownTimeout) + logger := log.Warn().Dur("timeout", timeout) if orgID != "" { logger = logger.Str("org_id", orgID) }