From 92abdcd9ad6471b4f4a5b1bbda84a98679b9a94b Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 16:26:06 +0100 Subject: [PATCH 1/7] Drop the unused Patrol readiness-failure object behind the toggle toast buildPatrolSettingsReadinessFailure built a full PatrolConfigurationFailureInput (code, status, saved, details, mode, budget, readiness, runtime and blocked state) for the Patrol page's inline "needs attention" state. That state lost its render in 27ba85ddc6 (2026-06-23), when the Configure Patrol popover was removed. Since then the only production caller, the Patrol on/off toggle, has read just `.message` for a warning toast, and it always passed the fixed copy "Patrol setting was saved, but Patrol is not ready to run." getPatrolSavedReadinessWarning now returns that message when the saved response reports patrol_readiness not_ready. The PatrolConfigurationFailureInput type goes with it, because its other consumers were already removed. Runtime: production builds before and after differ in one chunk, AIIntelligence.js (the Patrol page), which is 935 bytes smaller. After normalising chunk hashes, SRI strings and minified identifiers, the only change is the removed builder and details helper, replaced by one status check. The toast string is byte-identical. The minifier had already folded `message || readiness.summary || default` to the caller's constant, so the readiness-summary fallback was unreachable. The toggle no longer reads seven Patrol signals to build the discarded object. The bundle-size check passes. The same Playwright script ran against both builds at 1440 and 390 px and gave identical results. It covered the toggle off and on with a not-ready response, a failed and a successful Patrol mode save, and Settings > Patrol and Provider & Models saves with a not-ready response. Each shows only a toast, with no inline alert or Assistant action. Tests: the hook's unit tests now cover the warning helper. The #1789 test pinned `blockedCause: patrolStatus()?.blocked_cause`, which matched only the removed dead field and not the blockedCause memo the test was written for, so it now pins the memo. frontendResourceTypeBoundaries pins the new helper name. Contracts: patrol-intelligence, frontend-primitives and api-contracts still described the retired Patrol schedule and model popover, its inline dialog failure state, and a saved-configuration-issue surface with cause metadata. They now say where Patrol settings live (Settings > Pulse Intelligence > Patrol and Settings > Provider & Models), that every save outcome is a toast, and that the Patrol page's readiness banner carries the not-ready diagnosis. The popover rules apply only if a popover returns. The Assistant drawer's restore path for saved patrol_configuration_failure sessions is unchanged. --- .../v6/internal/subsystems/api-contracts.md | 14 ++- .../subsystems/frontend-primitives.md | 51 +++++----- .../subsystems/patrol-intelligence.md | 23 ++--- .../0f6190e732d7b132.json | 44 +++++++++ .../usePatrolIntelligenceState.test.ts | 59 +++-------- .../patrol/patrolInvestigationContextModel.ts | 21 ---- .../patrol/usePatrolIntelligenceState.ts | 98 +++---------------- .../frontendResourceTypeBoundaries.test.ts | 2 +- 8 files changed, 119 insertions(+), 193 deletions(-) create mode 100644 frontend-modern/browser-verification/0f6190e732d7b132.json diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index f6943a66d2..c2457e7f0f 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4020,12 +4020,16 @@ the authoritative analysis outcome. `/api/settings/ai/update` may save a selected Patrol provider/model even when that model is not ready for tool-backed Patrol execution, but it must echo `patrol_readiness` with stable `cause` metadata and execution routes - must continue to fail closed before model calls. Frontend Patrol settings + must continue to fail closed before model calls. Frontend settings consumers must surface that saved-but-not-ready response as a saved - configuration issue with the echoed provider, model, cause, and summary - instead of reporting the successful save as a failed save or hiding the - readiness blocker behind a generic notification; and the structured - investigation-record contract, so unified findings may + configuration issue rather than a failed save, and never as inline failure + state: every Pulse Intelligence settings page (Provider & Models, Patrol, + Assistant, Service context) raises a warning notification naming the echoed + summary, provider, and model, while the Patrol page's on/off save raises a + fixed not-ready warning and leaves the diagnosis to the page's own readiness + surfaces, such as the readiness banner that names the summary, provider, + and model while the header offers `Fix setup` when Patrol is active; and + the structured investigation-record contract, so unified findings may expose `investigation_record` only through the shared `aicontracts.InvestigationRecord` payload shape, with frontend API types and backend contract tests updated in the same slice as any field change. diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 7707b99374..85dce4e823 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -3144,33 +3144,30 @@ Agent`), with the plain-language source phrase available through accessible operator action or selected Patrol context, uses distinct monitor labels, and does not become a nested card, generic dashboard strip, trust summary, or duplicate Patrol empty-work list. The - Patrol schedule and model drawer is part of that shared - feature-presentation boundary: it must stay viewport-bounded, expose an - accessible dialog label, keep the four-level control policy on the default - Patrol header, and keep provider model, schedule, trigger tuning, and - readiness validation inside the secondary disclosure. Backend save rejection reasons must pass - through as inline dialog state instead of being replaced with generic toast - copy, and that advanced disclosure must open when the inline state exists. - When the failure includes - Patrol readiness context, the inline state must expose the provider, model, - and readiness summary next to a direct provider-settings action instead of - hiding that diagnosis behind Assistant alone. The provider-model selector in - that popover must stay bound to the shared runtime settings/model catalog - even when the popover mounts after async catalog loading, but the full - catalog must stay behind an explicit change action so the default advanced - drawer leads with the current effective model summary rather than a raw - provider route list. A saved direct-provider Patrol model still renders as - that model instead of visually falling back to the default selection. - Successful provider-model saves that return a not-ready Patrol - readiness snapshot must use that same inline surface with `needs attention` - wording, and any Assistant handoff from that state describes a saved - configuration issue rather than a failed save. When governed fixes are locked, the same Patrol state - owner must clear stale full-mode unlock state before persisting the - monitor-only autonomy payload, so disabled paid controls cannot leak stale - permission into a save. If that inline state opens Assistant, the Patrol - feature must hand off - a source-named, model-only briefing and close the popover so the shared - Assistant drawer is not visually hidden behind feature chrome. When a + Patrol page keeps the four-level control policy on its header and has no + schedule and model drawer: schedule, trigger tuning, and the Patrol model + override live on Settings > Pulse Intelligence > Patrol, and providers and + the shared default model on Settings > Provider & Models. Those saves + report through the shared notification toasts, not inline state: a + rejected save raises an error toast carrying the backend's concrete reason + rather than generic fallback copy, and a saved response whose Patrol + readiness is not ready or degraded raises a warning toast naming the + readiness summary, provider, and model instead of a success toast. On the + Patrol page, an on/off save whose response reports Patrol not ready raises + a fixed warning toast, and a failed Patrol mode save raises an error toast + and restores the previous mode; neither renders inline save-failure state + or offers an Assistant handoff. If a Patrol settings popover or inline + save-failure state returns, it must stay viewport-bounded, expose an + accessible dialog label, bind any model selector to the shared runtime + settings and model catalog, show readiness context (provider, model, + summary) next to a direct provider-settings action rather than behind + Assistant alone, and describe a saved but not ready response as a saved + configuration issue rather than a failed save. An Assistant handoff from it + must send a source-named, model-only briefing and close the popover so the + shared Assistant drawer is not visually hidden behind feature chrome. When + governed fixes are locked, the Patrol state owner must clear stale + full-mode unlock state before persisting the monitor-only autonomy payload, + so disabled paid controls cannot leak stale permission into a save. When a restored Patrol assessment session is attached, the shared Assistant drawer empty state must stay aligned with that source-named briefing and must not render generic diff --git a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md index 92f205d675..cd901c6f4e 100644 --- a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md +++ b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md @@ -1567,17 +1567,18 @@ suppress stale healthy summary headlines such as `Health A ยท 100/100` even if the last summary payload still looks healthy. Legacy `/ai` entry points must redirect into that same Patrol-owned shell rather than preserving a second canonical route. -That same browser proof now covers the Patrol control and advanced-settings -split. The advanced Patrol settings drawer must stay within the desktop -viewport, avoid duplicating the inline Patrol control policy, expose -provider/model, schedule, trigger, and user-level model checks directly, and surface -the backend's concrete license/validation reason when a settings change is -rejected instead of replacing it with a generic `Failed to save advanced -settings` toast. That inline failure no longer -opens Assistant. If it does again, the handoff is model-only explanation -context: raw command, script, credential, and provider-detail payloads stay -redacted, Assistant opens with `autonomousMode:false`, and the Patrol control -panel closes so the operator is not left behind an overlapping popover. +That same browser proof now covers where Patrol settings are saved. The +Patrol page has no settings drawer or popover: Patrol schedule, triggers, and +the Patrol model override live on Settings > Pulse Intelligence > Patrol +(`Save Patrol settings`), and providers and the shared default model on +Settings > Provider & Models. A rejected save there raises an error +notification carrying the backend's concrete license/validation reason +instead of generic fallback copy, and it offers no Assistant handoff. If a +Patrol settings popover or an Assistant handoff from a settings failure +returns, the handoff is model-only explanation context: raw command, script, +credential, and provider-detail payloads stay redacted, Assistant opens with +`autonomousMode:false`, and the popover closes so the operator is not left +behind it. Patrol-control save-failure sessions saved by earlier builds keep the compatible `handoff_metadata.kind=patrol_configuration_failure` plus only the runtime-failure boolean needed for drawer/session presentation, so they restore diff --git a/frontend-modern/browser-verification/0f6190e732d7b132.json b/frontend-modern/browser-verification/0f6190e732d7b132.json new file mode 100644 index 0000000000..bd6ecf244f --- /dev/null +++ b/frontend-modern/browser-verification/0f6190e732d7b132.json @@ -0,0 +1,44 @@ +{ + "version": 1, + "base_sha": "421d1e5423e5b83b7923ea8c3b133ad520cd5f60", + "verified_at": "2026-10-07T15:20:38Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts", + "frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts" + ], + "content_sha256": { + "frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts": "f1e249c36f0f0f9810330a37cc9a80cb43baa1cd123db1defd3f27720d39912a", + "frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts": "cb0a6962ed929238a974a3bbe5f69f4c89961ff30c88c2c94398073b9ef99a1c" + }, + "routes": [ + "/patrol", + "/patrol (Mode and automation disclosure)", + "/settings/pulse-intelligence/patrol", + "/settings/pulse-intelligence" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Embedded production builds (vite build into internal/api/frontend-modern/dist, go build) of the parent tree (333e51d2de; rebased onto 421d1e5423, which changes only contract docs) and this tree, each on a private DEMO_MODE mock stack with a fresh data dir; the same Playwright script ran against both and produced identical results on all 31 observed fields at both widths.", + "Patrol on/off save whose PUT /api/settings/ai/update response (page.route) reports patrol_readiness not_ready: switching off and back on each raise only the warning toast 'Patrol setting was saved, but Patrol is not ready to run.' (role=alert inside the Notifications region, close button only); no role=alert outside the toast region, no 'needs attention' text, no readiness summary rendered inline, no Assistant action.", + "Failed Patrol mode save (Pro mode selector unlocked by adding ai_autofix to runtime-capabilities via page.route; PUT /api/ai/patrol/autonomy answered 500 {error}): only the error toast with the server reason; mode badge returns to Watch only; no inline error or 'needs attention' state.", + "Successful Patrol mode save (PUT 200, reconcile GET returns approval): Ask first becomes pressed and the badge reads Ask first; no new toast and no inline alert.", + "Settings > Pulse Intelligence > Patrol save (Schedule -> Every hour) and Settings > Provider & Models save with a not_ready response: only the warning toast ', but Patrol is not ready: The selected model cannot run Patrol tools. \u00b7 Provider: OpenRouter \u00b7 Model: openrouter:deepseek/deepseek-r1'; no inline alert.", + "Patrol status with readiness not_ready (page.route on /api/ai/patrol/status): the page's own 'Patrol model issue' banner names summary, provider and model, and the header shows Fix setup, at both widths.", + "Dist proof: 187 files each side; after normalising chunk hashes and SRI strings only assets/AIIntelligence.js differs (935 bytes smaller): the removed failure builder, details helper and surface callback replaced by one status check, toast string byte-identical; check-bundle-size.mjs passes." + ], + "interactions": [ + "Demo sign-in; on /patrol click Toggle Patrol twice (off, then on) with a not-ready settings response; open 'Mode and automation' and click Ask first with a rejected save, then again with an accepted save; desktop mouse and phone touch (isMobile) contexts.", + "On /settings/pulse-intelligence/patrol select Schedule 'Every hour' and click Save Patrol settings; on /settings/pulse-intelligence click Save provider settings; both with a not-ready response.", + "Screenshots of each toast state captured and inspected at both widths." + ] +} diff --git a/frontend-modern/src/features/patrol/__tests__/usePatrolIntelligenceState.test.ts b/frontend-modern/src/features/patrol/__tests__/usePatrolIntelligenceState.test.ts index 4dcef31829..a7e54782cb 100644 --- a/frontend-modern/src/features/patrol/__tests__/usePatrolIntelligenceState.test.ts +++ b/frontend-modern/src/features/patrol/__tests__/usePatrolIntelligenceState.test.ts @@ -3,7 +3,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { PATROL_MANUAL_SYNC_TIMEOUT_MS, PATROL_REFRESH_TIMEOUT_MS, - buildPatrolSettingsReadinessFailure, + getPatrolSavedReadinessWarning, openPatrolAssistantWorkflowHandoff, patrolStartFailureMessage, recordPatrolControlStarterActivity, @@ -338,24 +338,25 @@ describe('usePatrolIntelligenceState', () => { }); }); - describe('buildPatrolSettingsReadinessFailure', () => { - it('ignores settings snapshots that do not block Patrol readiness', () => { + describe('getPatrolSavedReadinessWarning', () => { + it('stays quiet when the saved settings do not block Patrol readiness', () => { + expect(getPatrolSavedReadinessWarning(null)).toBeNull(); expect( - buildPatrolSettingsReadinessFailure({ - settings: settingsWithReadiness({ + getPatrolSavedReadinessWarning( + settingsWithReadiness({ status: 'warning', ready: true, summary: 'Patrol can run with reduced confidence.', checks: [], }), - }), + ), ).toBeNull(); }); - it('builds a saved configuration issue from a not-ready settings response', () => { + it('warns that a not-ready Patrol setting was still saved', () => { expect( - buildPatrolSettingsReadinessFailure({ - settings: settingsWithReadiness({ + getPatrolSavedReadinessWarning( + settingsWithReadiness({ status: 'not_ready', ready: false, cause: 'model_unsupported_tools', @@ -364,40 +365,8 @@ describe('usePatrolIntelligenceState', () => { model: 'ollama:deepseek-r1:7b', checks: [], }), - autonomyLevel: 'monitor', - fullModeUnlocked: false, - investigationBudget: 15, - investigationTimeoutSec: 300, - runtimeState: 'blocked', - blockedReason: 'Connect a tool-capable Patrol model.', - }), - ).toEqual({ - message: 'The selected model cannot run Patrol tools.', - code: 'patrol_readiness_not_ready', - status: 409, - saved: true, - details: { - status: 'not_ready', - cause: 'model_unsupported_tools', - summary: 'The selected model cannot run Patrol tools.', - provider: 'ollama', - model: 'ollama:deepseek-r1:7b', - }, - autonomyLevel: 'monitor', - fullModeUnlocked: false, - investigationBudget: 15, - investigationTimeoutSec: 300, - readiness: { - status: 'not_ready', - cause: 'model_unsupported_tools', - summary: 'The selected model cannot run Patrol tools.', - provider: 'ollama', - model: 'ollama:deepseek-r1:7b', - }, - runtimeState: 'blocked', - blockedReason: 'Connect a tool-capable Patrol model.', - blockedCause: undefined, - }); + ), + ).toBe('Patrol setting was saved, but Patrol is not ready to run.'); }); }); @@ -410,6 +379,8 @@ describe('usePatrolIntelligenceState', () => { 'provider_not_configured', ); expect(resolvePatrolBlockedActionCause('', '')).toBeUndefined(); - expect(patrolIntelligenceStateSource).toContain('blockedCause: patrolStatus()?.blocked_cause'); + expect(patrolIntelligenceStateSource).toContain( + 'const blockedCause = createMemo(() => patrolStatus()?.blocked_cause);', + ); }); }); diff --git a/frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts b/frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts index ab961032a5..ec242837ed 100644 --- a/frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts +++ b/frontend-modern/src/features/patrol/patrolInvestigationContextModel.ts @@ -182,27 +182,6 @@ export interface PatrolRunAssistantHandoff { context: AIChatContext; } -export interface PatrolConfigurationFailureInput { - message: string; - code?: string; - status?: number; - saved?: boolean; - details?: Record; - autonomyLevel?: string; - fullModeUnlocked?: boolean; - investigationBudget?: number; - investigationTimeoutSec?: number; - readiness?: { - status?: string; - cause?: string; - summary?: string; - provider?: string; - model?: string; - } | null; - runtimeState?: string; - blockedReason?: string; - blockedCause?: string; -} const MAX_PATROL_RUN_HANDOFF_RESOURCES = 8; export function buildPatrolInvestigationRecordPresentation( diff --git a/frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts b/frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts index d2eb5d4dd6..1f242e6c97 100644 --- a/frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts +++ b/frontend-modern/src/features/patrol/usePatrolIntelligenceState.ts @@ -62,7 +62,6 @@ import { patrolAssistantFindingHandoffRequiresApprovalMode, type PatrolAssistantApprovalBriefingInput, type PatrolAssistantFindingHandoff, - type PatrolConfigurationFailureInput, } from './patrolInvestigationContextModel'; import { schedulePatrolRunAcceptanceReconciliation } from './patrolRunAcceptance'; @@ -141,19 +140,6 @@ export const patrolStartFailureMessage = (error: unknown): string => ? `Could not reach Pulse to start Patrol: ${patrolErrorMessage(error, 'network request failed')}` : patrolErrorMessage(error, 'Pulse rejected the Patrol run'); -const buildReadinessDetails = ( - readiness: NonNullable, -): Record => { - const details: Record = { - status: readiness.status, - }; - if (readiness.cause?.trim()) details.cause = readiness.cause.trim(); - if (readiness.summary?.trim()) details.summary = readiness.summary.trim(); - if (readiness.provider?.trim()) details.provider = readiness.provider.trim(); - if (readiness.model?.trim()) details.model = readiness.model.trim(); - return details; -}; - export function resolvePatrolAutonomyLevelForSave( level: PatrolAutonomyLevel, fullModeUnlocked: boolean, @@ -184,52 +170,16 @@ export function resolvePatrolAutonomySettingsForSave({ }; } -export function buildPatrolSettingsReadinessFailure({ - settings, - message, - autonomyLevel, - fullModeUnlocked, - investigationBudget, - investigationTimeoutSec, - runtimeState, - blockedReason, - blockedCause, -}: { - settings: AISettings | null | undefined; - message?: string; - autonomyLevel?: string; - fullModeUnlocked?: boolean; - investigationBudget?: number; - investigationTimeoutSec?: number; - runtimeState?: string; - blockedReason?: string; - blockedCause?: string; -}): PatrolConfigurationFailureInput | null { - const readiness = settings?.patrol_readiness; - if (!readiness || readiness.status !== 'not_ready') return null; - - return { - message: - message || readiness.summary || 'Patrol settings were saved, but Patrol is not ready to run.', - code: 'patrol_readiness_not_ready', - status: 409, - saved: true, - details: buildReadinessDetails(readiness), - autonomyLevel, - fullModeUnlocked, - investigationBudget, - investigationTimeoutSec, - readiness: { - status: readiness.status, - cause: readiness.cause, - summary: readiness.summary, - provider: readiness.provider, - model: readiness.model, - }, - runtimeState, - blockedReason, - blockedCause, - }; +/** + * A Patrol on/off save whose response reports Patrol not ready was still + * saved, so it raises a warning rather than an error. + */ +export function getPatrolSavedReadinessWarning( + settings: AISettings | null | undefined, +): string | null { + return settings?.patrol_readiness?.status === 'not_ready' + ? 'Patrol setting was saved, but Patrol is not ready to run.' + : null; } /** @@ -380,26 +330,6 @@ export function usePatrolIntelligenceState() { applyPatrolAISettings(aiRuntimeSettings()); }); - const surfaceSavedPatrolReadinessIssue = ( - settings: AISettings | null | undefined, - message?: string, - ) => { - const failure = buildPatrolSettingsReadinessFailure({ - settings, - message, - autonomyLevel: autonomyLevel(), - fullModeUnlocked: fullModeUnlocked(), - investigationBudget: investigationBudget(), - investigationTimeoutSec: investigationTimeout(), - runtimeState: runtimeState(), - blockedReason: blockedReason(), - blockedCause: patrolStatus()?.blocked_cause, - }); - if (failure) { - notificationStore.warning(failure.message); - } - }; - async function handleTogglePatrol() { if (isTogglingPatrol()) return; setIsTogglingPatrol(true); @@ -419,10 +349,10 @@ export function usePatrolIntelligenceState() { } else { setPatrolEnabledLocal(newValue); } - surfaceSavedPatrolReadinessIssue( - data, - 'Patrol setting was saved, but Patrol is not ready to run.', - ); + const readinessWarning = getPatrolSavedReadinessWarning(data); + if (readinessWarning) { + notificationStore.warning(readinessWarning); + } if (refetchPatrolStatus) { refetchPatrolStatus(); } diff --git a/frontend-modern/src/utils/__tests__/frontendResourceTypeBoundaries.test.ts b/frontend-modern/src/utils/__tests__/frontendResourceTypeBoundaries.test.ts index f7d939d3cb..157792f0ca 100644 --- a/frontend-modern/src/utils/__tests__/frontendResourceTypeBoundaries.test.ts +++ b/frontend-modern/src/utils/__tests__/frontendResourceTypeBoundaries.test.ts @@ -3610,7 +3610,7 @@ describe('frontend resource type boundaries', () => { expect(patrolIntelligenceStateSource).toMatch( /import\s*\{[^}]*\bbuildPatrolAssistantFindingHandoffFromUnifiedFinding\b[^}]*\}\s*from '\.\/patrolInvestigationContextModel';/, ); - expect(patrolIntelligenceStateSource).toContain('buildPatrolSettingsReadinessFailure'); + expect(patrolIntelligenceStateSource).toContain('getPatrolSavedReadinessWarning'); expect(patrolIntelligenceStateSource).not.toContain('recent_changes?.length'); expect(patrolIntelligenceStateSource).not.toContain('governed resource${'); expect(patrolInvestigationContextModelSource).toContain( From 2cf213a77d2a50dcd47c0b0ab07c49f97bdd0534 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 16:34:21 +0100 Subject: [PATCH 2/7] Describe the Patrol mode selector where it renders today patrol-intelligence still described the Patrol mode selector as living in "that header and configuration dialog", with the constrained configuration dialog using the prominent layout. The configuration dialog was removed in 27ba85ddc6. renderAutonomyPolicyControl has one call site, PatrolIntelligenceHeader's `Mode and automation` disclosure, which uses the default segmented variant, and the only remaining Patrol dialog, the Autopilot acknowledgement, has no mode picker. The clause now says where the header renders the selector. A surface that hosts it in constrained space may still use the shared prominent layout. --- .../v6/internal/subsystems/patrol-intelligence.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md index cd901c6f4e..b96df3b120 100644 --- a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md +++ b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md @@ -1624,12 +1624,13 @@ ready raises a fixed warning notification, a failed Patrol mode save raises an e notification, and neither offers an Assistant handoff. A provider or model save in Settings > Provider & Models reports the readiness summary, provider and model in its own save message. -The Patrol mode selector in that header and configuration dialog must -compose the shared `frontend-modern/src/components/shared/FilterButtonGroup.tsx` -instead of rebuilding a local active-button group. The wide default Patrol -header uses the segmented layout; the constrained configuration dialog may use -the shared prominent layout so all four mode labels remain readable without -inventing a Patrol-local selector. Patrol owns the default visible four-level +The Patrol mode selector in that header must compose the shared +`frontend-modern/src/components/shared/FilterButtonGroup.tsx` instead of +rebuilding a local active-button group. When the header shows Patrol mode +options, it renders the selector in the segmented layout inside its `Mode and +automation` disclosure; a constrained surface that hosts it may use the shared +prominent layout so all four mode labels remain readable without inventing a +Patrol-local selector. Patrol owns the default visible four-level policy presentation (`Watch only`, `Ask first`, `Safe auto-fix`, `Autopilot`), entitlement locks, and the rule that choosing the highest Autopilot level sends `full_mode_unlocked:true` while choosing any lower level clears that From 3bef5c3028f63541d98b43b1cdc43686dd418cd3 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:07:27 +0100 Subject: [PATCH 3/7] Remove unused CorrelationsResponse type CorrelationsResponse described the /api/ai/intelligence/correlations payload. #2674 removed its uses in the Patrol investigation context model and that model's tests, and #2677 removed the last ones, AIAPI.getCorrelations and the aiIntelligence store's correlation loader, because no page renders the global learned-correlation list. Nothing in the repository references the interface any more, tests included. ResourceCorrelation stays: ResourceIntelligence.correlations, the resource drawer's relationship map (ResourceCorrelationSummary.tsx) and resourceCorrelationPresentation.ts all still use it. TypeScript erases the interface, so the production build is byte-identical: npm run build before and after this change emitted the same 187 dist files with identical SHA-256 sums. Contract-Neutral: Deletes an unreferenced TypeScript interface; the compiler erases it, the production bundle is byte-identical, and no wire, store or page contract changes. --- .../71a0ce6b24830582.json | 39 +++++++++++++++++++ frontend-modern/src/types/aiIntelligence.ts | 8 ---- 2 files changed, 39 insertions(+), 8 deletions(-) create mode 100644 frontend-modern/browser-verification/71a0ce6b24830582.json diff --git a/frontend-modern/browser-verification/71a0ce6b24830582.json b/frontend-modern/browser-verification/71a0ce6b24830582.json new file mode 100644 index 0000000000..0b4623136d --- /dev/null +++ b/frontend-modern/browser-verification/71a0ce6b24830582.json @@ -0,0 +1,39 @@ +{ + "version": 1, + "base_sha": "48c2d6cdce8d019151e3567873de88e21b76bb22", + "verified_at": "2026-10-07T19:05:33Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/types/aiIntelligence.ts" + ], + "content_sha256": { + "frontend-modern/src/types/aiIntelligence.ts": "d9287de8171cee10d72c5df0a898c28e47e649be3f9dee8fa791a7406c0adc51" + }, + "routes": [ + "/patrol", + "/kubernetes" + ], + "viewports": [ + { + "width": 1280, + "height": 800 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Type-only deletion: TypeScript erases CorrelationsResponse, and npm run build before and after the change emitted the same 187 dist files with identical SHA-256 sums, so the shipped bundle is byte-identical", + "Embedded production build in a private demo-mode mock stack (port 7763, fresh data dir)", + "Patrol inbox: Patrol heading and 5 attention rows render at 1280x800 and 390x844", + "Kubernetes deployment account-portal drawer opens with header, technical summary, runtime context and identity sections at both widths", + "No console errors and no page errors on either route at either width; no /api/ai/intelligence/correlations request", + "Relationship map (ResourceCorrelationSummary, the surviving ResourceCorrelation consumer) not rendered in this pass: table-row drawers do not fetch per-resource facets or resource intelligence (enableRemoteHistory is off for table-row presentation); its code ships unchanged in the byte-identical bundle" + ], + "interactions": [ + "Demo sign-in, then navigate to /patrol and wait for the Patrol heading", + "Navigate to /kubernetes and click the account-portal deployment row to open its inline resource drawer", + "Repeat the same pass at 1280x800 and 390x844" + ] +} diff --git a/frontend-modern/src/types/aiIntelligence.ts b/frontend-modern/src/types/aiIntelligence.ts index 48237e884d..f20f83c533 100644 --- a/frontend-modern/src/types/aiIntelligence.ts +++ b/frontend-modern/src/types/aiIntelligence.ts @@ -109,14 +109,6 @@ export interface ResourceCorrelation { description: string; } -export interface CorrelationsResponse { - correlations: ResourceCorrelation[]; - count: number; - message?: string; - license_required?: boolean; - upgrade_url?: string; -} - export interface IntelligenceSummary { timestamp: string; overall_health: IntelligenceHealthScore; From 88d44099aa3cd8404035ebdd78e8f4a6201a9582 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:12:04 +0100 Subject: [PATCH 4/7] Stop tenant incident stores racing on shared ID counters Each tenant monitor owns its own IncidentStore, but incident and incident-event IDs came from package-level int64 counters bumped with ++ under nothing but the calling store's mutex. Two tenants recording alerts at once raced on them: go test -race failed TestLeavingMockModeClearsEveryRunningTenantMonitor on PR 2698 at incidents.go:1753. The remediation record counter had the same shape, and remediation logs are created per org. All six internal/ai/memory ID generators now mint through one atomic helper. The numeric suffix also stops wrapping at 1000: even with atomic counters, one generator minting more than a thousand IDs in a wall-clock second repeated an ID, and incident notes find their incident by ID. The prefix and timestamp layout is unchanged, and intToString lost its last callers. ids_test.go mints 2,400 IDs from every generator on 16 goroutines and records alerts into two tenant stores concurrently. Against the old files it reports DATA RACE at incidents.go:1753/1760 and remediation.go:355, and every generator mints duplicate or wrapped IDs. --- .../v6/internal/subsystems/ai-runtime.md | 19 +++ internal/ai/memory/changes.go | 14 -- internal/ai/memory/context.go | 9 +- internal/ai/memory/ids.go | 16 +++ internal/ai/memory/ids_test.go | 122 ++++++++++++++++++ internal/ai/memory/incidents.go | 10 +- internal/ai/memory/memory_coverage_test.go | 9 +- internal/ai/memory/remediation.go | 6 +- 8 files changed, 167 insertions(+), 38 deletions(-) create mode 100644 internal/ai/memory/ids.go create mode 100644 internal/ai/memory/ids_test.go diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 9ab54a9d35..540a33d923 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -128,6 +128,25 @@ on component mount. Closing clears the registered input so later keyboard commands cannot target a detached composer. A handoff must leave Escape and keyboard input in Assistant, not the underlying alert search. +### Process-wide memory ID sequences + +`internal/ai/memory` mints incident, incident-event, remediation-record and +context-memory IDs from package-level sequences that every store in the process +shares. Multi-tenant monitoring gives each tenant monitor its own +`IncidentStore`, and their alert checks call `RecordAlertFired` from separate +goroutines, while a store's mutex covers only its own state. Each sequence is +therefore an `atomic.Int64` minted through `nextSequencedID` in +`internal/ai/memory/ids.go`, never a plain package counter. The ID shape stays +`-`, but `n` no longer wraps at 1000. The old +`counter%1000` suffix repeated an ID whenever one generator minted more than a +thousand in a wall-clock second, and incident notes resolve an incident by ID +while timeline merges deduplicate events by ID. Within one process IDs are now +unique by construction. Across restarts only the timestamp separates them, so a +repeated local second (DST fall-back or a backward clock step) can still meet a +restarted sequence's suffix. Persisted IDs and the stored format are unchanged. +`internal/ai/memory/ids_test.go` mints from every generator concurrently and +records alerts into two tenant stores concurrently; run it under `-race`. + ### Canonical incident-history queries Incident context reads `IncidentStore.QueryIncidents`, whose page contains diff --git a/internal/ai/memory/changes.go b/internal/ai/memory/changes.go index 0771e9cd71..3b0a4e4bc0 100644 --- a/internal/ai/memory/changes.go +++ b/internal/ai/memory/changes.go @@ -131,17 +131,3 @@ func (d *ChangeDetector) loadFromDisk() error { d.trimChanges() return nil } - -// Helper functions - -func intToString(n int) string { - if n == 0 { - return "0" - } - var result string - for n > 0 { - result = string(rune('0'+n%10)) + result - n /= 10 - } - return result -} diff --git a/internal/ai/memory/context.go b/internal/ai/memory/context.go index 1b5547d882..9a42d03758 100644 --- a/internal/ai/memory/context.go +++ b/internal/ai/memory/context.go @@ -711,18 +711,15 @@ func (s *ContextStore) ForceSave() error { var memoryCounter, incidentMemCounter, patternMemCounter atomic.Int64 func generateMemoryID() string { - n := memoryCounter.Add(1) - return fmt.Sprintf("mem-%s-%d", time.Now().Format("20060102150405"), n%1000) + return nextSequencedID("mem-", &memoryCounter) } func generateIncidentMemoryID() string { - n := incidentMemCounter.Add(1) - return fmt.Sprintf("inc-mem-%s-%d", time.Now().Format("20060102150405"), n%1000) + return nextSequencedID("inc-mem-", &incidentMemCounter) } func generatePatternMemoryID() string { - n := patternMemCounter.Add(1) - return fmt.Sprintf("pat-mem-%s-%d", time.Now().Format("20060102150405"), n%1000) + return nextSequencedID("pat-mem-", &patternMemCounter) } func calculatePatternConfidence(occurrences int) float64 { diff --git a/internal/ai/memory/ids.go b/internal/ai/memory/ids.go new file mode 100644 index 0000000000..2be409a1da --- /dev/null +++ b/internal/ai/memory/ids.go @@ -0,0 +1,16 @@ +package memory + +import ( + "strconv" + "sync/atomic" + "time" +) + +// nextSequencedID mints prefix + local wall-clock second + "-" + the next value +// of seq. Each sequence is package-level and shared by every store in the +// process, including one store per tenant monitor, so it must stay atomic. +// The suffix never wraps, so one sequence never repeats an ID within a process +// however many IDs share a second. +func nextSequencedID(prefix string, seq *atomic.Int64) string { + return prefix + time.Now().Format("20060102150405") + "-" + strconv.FormatInt(seq.Add(1), 10) +} diff --git a/internal/ai/memory/ids_test.go b/internal/ai/memory/ids_test.go new file mode 100644 index 0000000000..530381625e --- /dev/null +++ b/internal/ai/memory/ids_test.go @@ -0,0 +1,122 @@ +package memory + +import ( + "fmt" + "regexp" + "strings" + "sync" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" +) + +// TestSequencedIDsStayUniqueUnderConcurrentMinting mints from every +// package-level ID sequence on many goroutines at once. Under -race it proves +// the sequences are synchronized. The uniqueness check proves the suffix does +// not wrap: more than a thousand IDs from one generator land in the same +// wall-clock second, which the old counter%1000 suffix turned into duplicates. +func TestSequencedIDsStayUniqueUnderConcurrentMinting(t *testing.T) { + generators := []struct { + prefix string + mint func() string + }{ + {"inc-", generateIncidentID}, + {"inc-evt-", generateIncidentEventID}, + {"rem-", generateRecordID}, + {"mem-", generateMemoryID}, + {"inc-mem-", generateIncidentMemoryID}, + {"pat-mem-", generatePatternMemoryID}, + } + const goroutines, perGoroutine = 16, 150 + + for _, gen := range generators { + t.Run(strings.TrimSuffix(gen.prefix, "-"), func(t *testing.T) { + shape := regexp.MustCompile("^" + regexp.QuoteMeta(gen.prefix) + `\d{14}-[1-9]\d*$`) + minted := make([][]string, goroutines) + start := make(chan struct{}) + var wg sync.WaitGroup + for g := range goroutines { + wg.Add(1) + go func() { + defer wg.Done() + <-start + batch := make([]string, perGoroutine) + for i := range batch { + batch[i] = gen.mint() + } + minted[g] = batch + }() + } + close(start) + wg.Wait() + + seen := make(map[string]bool, goroutines*perGoroutine) + for _, batch := range minted { + for _, id := range batch { + if !shape.MatchString(id) { + t.Fatalf("ID %q does not match %s", id, shape) + } + if seen[id] { + t.Fatalf("duplicate ID %q among %d concurrent mints", id, goroutines*perGoroutine) + } + seen[id] = true + } + } + }) + } +} + +// TestIncidentStoresMintIDsConcurrentlyAcrossTenants mirrors multi-tenant +// monitoring: each tenant monitor owns its own IncidentStore, and their alert +// checks call RecordAlertFired from separate goroutines. Each store's mutex +// covers only its own state, so the shared ID sequence must be synchronized on +// its own. Run under -race. +func TestIncidentStoresMintIDsConcurrentlyAcrossTenants(t *testing.T) { + const tenants, checkersPerTenant, alertsPerChecker = 2, 4, 50 + stores := make([]*IncidentStore, tenants) + for i := range stores { + stores[i] = NewIncidentStore(IncidentStoreConfig{MaxIncidents: checkersPerTenant * alertsPerChecker}) + } + + startedAt := time.Now().Add(-time.Minute) + start := make(chan struct{}) + var wg sync.WaitGroup + for tenant, store := range stores { + for checker := range checkersPerTenant { + wg.Add(1) + go func() { + defer wg.Done() + <-start + for n := range alertsPerChecker { + store.RecordAlertFired(&alerts.Alert{ + ID: fmt.Sprintf("tenant-%d-checker-%d-alert-%d", tenant, checker, n), + Type: "cpu", + Level: alerts.AlertLevelWarning, + ResourceID: fmt.Sprintf("tenant-%d-vm-%d-%d", tenant, checker, n), + ResourceName: "vm", + StartTime: startedAt, + }) + } + }() + } + } + close(start) + wg.Wait() + + seen := make(map[string]bool, tenants*checkersPerTenant*alertsPerChecker) + for tenant, store := range stores { + store.mu.RLock() + incidents := len(store.incidents) + for _, shell := range store.incidents { + if seen[shell.ID] { + t.Errorf("tenant %d incident ID %q was already minted", tenant, shell.ID) + } + seen[shell.ID] = true + } + store.mu.RUnlock() + if incidents != checkersPerTenant*alertsPerChecker { + t.Errorf("tenant %d recorded %d incidents, want %d", tenant, incidents, checkersPerTenant*alertsPerChecker) + } + } +} diff --git a/internal/ai/memory/incidents.go b/internal/ai/memory/incidents.go index 8729c83cef..5c763b2d92 100644 --- a/internal/ai/memory/incidents.go +++ b/internal/ai/memory/incidents.go @@ -1747,18 +1747,14 @@ func cloneTime(value time.Time) *time.Time { return &cloned } -var incidentCounter int64 +var incidentCounter, incidentEventCounter atomic.Int64 func generateIncidentID() string { - incidentCounter++ - return "inc-" + time.Now().Format("20060102150405") + "-" + intToString(int(incidentCounter%1000)) + return nextSequencedID("inc-", &incidentCounter) } -var incidentEventCounter int64 - func generateIncidentEventID() string { - incidentEventCounter++ - return "inc-evt-" + time.Now().Format("20060102150405") + "-" + intToString(int(incidentEventCounter%1000)) + return nextSequencedID("inc-evt-", &incidentEventCounter) } func formatAlertSummary(alert *alerts.Alert) string { diff --git a/internal/ai/memory/memory_coverage_test.go b/internal/ai/memory/memory_coverage_test.go index 653f05796d..7ae826ea6a 100644 --- a/internal/ai/memory/memory_coverage_test.go +++ b/internal/ai/memory/memory_coverage_test.go @@ -11,18 +11,11 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" ) -func TestChangeDetector_DefaultsAndHelpers(t *testing.T) { +func TestChangeDetector_Defaults(t *testing.T) { detector := NewChangeDetector(ChangeDetectorConfig{}) if detector.maxChanges != 1000 { t.Fatalf("expected default maxChanges=1000, got %d", detector.maxChanges) } - - if got := intToString(0); got != "0" { - t.Errorf("intToString(0) = %q", got) - } - if got := intToString(42); got != "42" { - t.Errorf("intToString(42) = %q", got) - } } func TestNewChangeDetector_LoadsFromDisk(t *testing.T) { diff --git a/internal/ai/memory/remediation.go b/internal/ai/memory/remediation.go index da93f5f4f5..97d2c8ac81 100644 --- a/internal/ai/memory/remediation.go +++ b/internal/ai/memory/remediation.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "sync" + "sync/atomic" "time" "github.com/rcourtman/pulse-go-rewrite/internal/utils" @@ -349,11 +350,10 @@ func (r *RemediationLog) loadFromDisk() error { // Helper functions -var recordCounter int64 +var recordCounter atomic.Int64 func generateRecordID() string { - recordCounter++ - return "rem-" + time.Now().Format("20060102150405") + "-" + intToString(int(recordCounter%1000)) + return nextSequencedID("rem-", &recordCounter) } func truncateOutput(output string, maxLen int) string { From b0590c0ccf6701adefc16fdf1c826aec474eb6e7 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:54:29 +0100 Subject: [PATCH 5/7] Join operator links on resources that arrive as records The monitor rebuilds its registry from the state snapshot and then ingests supplemental records (vSphere, TrueNAS). Snapshot and resource ingest applied the operator's manual links, but record ingest did not, so a link whose side exists only as a record (an agent inside a vSphere or TrueNAS VM, an agent on a TrueNAS host) never joined in the registry behind the websocket broadcast and /api/state. The resources API seeds through resource ingest and showed the same pair merged, and the published inventory also flipped to merged whenever continuity hydration cloned the registry for an unrelated absent machine. Record ingest now applies the links, with the adapter's stale thresholds on the rebuild, live supplemental and overlay paths. Continuity records stay out of it: a saved enrollment only fills an absent machine and must not lend its linked guest an offline verdict. A link merge keeps the TrueNAS and vSphere payloads the primary lacks. Joining links in the rebuild exposed how identity treats a folded resource. Its ID was no longer live, so once an agent-type primary was pinned with the folded agent's machine key, pin succession re-keyed the link onto the primary itself and the next rebuild split the pair. That already broke snapshot links with a Proxmox node as primary. The registry now remembers each folded resource, and pin-driven and record-declared successions treat it as observed. Mock mode still builds its view from fixtures without the link store, so the mock broadcast is unchanged here. --- .../v6/internal/subsystems/monitoring.md | 11 +- .../internal/subsystems/unified-resources.md | 65 +++- .../issue1913_host_continuity_test.go | 117 +++++++ .../unifiedresources/canonical_id_pins.go | 11 +- internal/unifiedresources/monitor_adapter.go | 6 +- .../monitor_adapter_read_state_test.go | 81 +++++ internal/unifiedresources/registry.go | 55 ++- .../registry_merge_policy_test.go | 313 ++++++++++++++++++ 8 files changed, 643 insertions(+), 16 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 796fc46ca0..d7b496972e 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -2676,7 +2676,16 @@ truthfulness, not native thaw, containing-release or workload acceptance. ingestion so canonical host IDs stay stable across restarts (see the unified-resources contract's durable identity-pin obligation). Rebuild paths added to the adapter must keep that persistence step; ephemeral - snapshot-bridge adapters stay read-only. + snapshot-bridge adapters stay read-only. The rebuild, the live + supplemental refresh and the read-state overlay ingest records with the + adapter's configured stale thresholds, because record ingest joins + operator links and a link's metric merge judges each side's freshness + by them (unified-resources contract, "Operator links reach + record-ingested resources"). Regression coverage: + `TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds` in + `internal/unifiedresources/monitor_adapter_read_state_test.go` and + `TestManualLinkToSupplementalGuestHoldsWithAndWithoutContinuity` in + `internal/monitoring/issue1913_host_continuity_test.go`. 11. The TrueNAS provider projects pools with `Storage.Topology` fixed to `pool` and the ZFS data vdev layout in `Storage.VDevLayout`. The diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 69b3cf77e3..95805c44a6 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -2803,7 +2803,9 @@ application resource-provider or WebSocket lifecycle. ID and drops the superseded pin row (`ApplyCanonicalIDSuccessions` in `internal/unifiedresources/canonical_id_succession.go`). Successions never fire while the old ID still belongs to a live - resource, never follow a contradicting machine key (a reinstalled + resource or to one a manual link folded into its primary (see + "Operator links reach record-ingested resources"), never follow a + contradicting machine key (a reinstalled machine mints fresh, it does not absorb the old host's operator state), and never rewrite change-journal rows. Known limitation: machine-keyless hosts whose history already merged under a collapsed @@ -2884,8 +2886,9 @@ application resource-provider or WebSocket lifecycle. `ResourceRegistry.IngestRecords` applies the same `ApplyCanonicalIDSuccessions` semantics as pin-driven successions (operator state and action audits re-key, the superseded pin row - drops, never while the old ID belongs to a live resource, journal - rows never rewritten). Regression coverage: + drops, never while the old ID belongs to a live resource or one a + manual link folded into its primary, journal rows never rewritten). + Regression coverage: `TestRegistryIngestRecordsKeepsSameHostnameSystemsDistinct` / `TestIngestRecordsSucceedLegacyHostnameScopedCanonicalIDs` / `TestIngestRecordsDoNotCompleteTrueNASIdentityFromPins` in @@ -6535,6 +6538,62 @@ full heading. This shared drawer rule applies across resource types. The 390px browser qualification checks heading fit and keyboard expansion; no data model, authorisation, action dispatch or resource admission contract changes. +### Operator links reach record-ingested resources + +Every registry ingest path applies the operator's manual links: snapshot +ingest, resource ingest and record ingest (`IngestRecords`, +`IngestRecordsWithStaleThresholds` and the read-state overlay's record pass). +Either side of a link can arrive only as a supplemental record, such as a +vSphere or TrueNAS VM with a Pulse agent inside, or an agent on a TrueNAS host, +and the monitor's rebuild ingests those records after the snapshot. While only +snapshot and resource ingest applied links, the rebuilt registry behind the +websocket broadcast and `/api/state` kept both rows. The resources API, which +seeds through resource ingest, showed them merged, and so did the published +inventory whenever an unrelated absent machine made continuity hydration +clone the registry. Mock mode builds its unified view from fixtures without +the link store, so links do not reach the mock broadcast; that boundary is +separate. + +- The monitor adapter passes its configured stale thresholds to record + ingest, so a link joined there judges each side's metrics the way a + snapshot-time link does. Ordinary source merges into an existing row still + use the default thresholds on every ingest path. +- Continuity records are the one record ingest that never joins links. A + saved enrollment only fills an absent machine, so it stays its own row + instead of lending its linked guest an offline verdict and an old agent + payload. +- A link merge keeps the TrueNAS and vSphere payloads the primary lacks, so + an agent chosen as primary over a TrueNAS system still carries the + system's facet, and the system's pools re-parent to the merged row. +- The registry remembers each resource a manual link folded into its + primary. A folded resource is still observed, so neither pin-driven nor + record-declared succession may succeed its ID. An agent-type primary + carries the folded resource's machine keys and is pinned with them, and + succeeding the folded ID re-keyed the link onto the primary itself, which + split the pair on the next rebuild. That already broke snapshot links whose + primary is a machine-keyless agent-type row, such as a Proxmox node. +- Fold history is registry-local: resource seeding and overlay clones do not + carry it. A reference to the folded resource's canonical ID, such as an + availability check configured against an agent later linked into its + guest, does not resolve to the primary, so the check loses its target in + the broadcast as it already did in the resources API. + +`registry_merge_policy_test.go` pins the record links in both directions +through two rebuilds and the live supplemental refresh, and checks that a +registry seeded from the monitor's listing through resource ingest, as the +resources API seeds, lists the same rows +(`TestManualLinksJoinRecordIngestedResourcesInMonitorRebuild`). The same file +pins the node case (`TestManualLinkKeepsItsEndpointsThroughIdentityPinPersistence`), +record-declared succession (`TestRecordDeclaredSuccessionSparesALinkFoldedResource`) +and both payloads (`TestManualLinkKeepsProviderPayloadsThePrimaryLacks`). +`monitor_adapter_read_state_test.go` +(`TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds`) pins the +thresholds on the rebuild, supplemental and overlay paths, and +`internal/monitoring/issue1913_host_continuity_test.go` +(`TestManualLinkToSupplementalGuestHoldsWithAndWithoutContinuity`) pins the +published inventory with no continuity, an unrelated absent machine and the +linked agent's own saved enrollment. + ### Provider link network corroboration An existing reciprocal host/provider link remains authoritative, including a diff --git a/internal/monitoring/issue1913_host_continuity_test.go b/internal/monitoring/issue1913_host_continuity_test.go index 5134592e7b..192540bc5c 100644 --- a/internal/monitoring/issue1913_host_continuity_test.go +++ b/internal/monitoring/issue1913_host_continuity_test.go @@ -89,3 +89,120 @@ func TestHostContinuityPreservesLiveAgentAfterReenrollment(t *testing.T) { }) } } + +// Continuity hydration clones the registry through resource ingest, which +// applies operator links, while the rebuild ingests supplemental records after +// the snapshot. A link to a record-sourced guest (an agent inside a vSphere VM) +// must hold in the published inventory either way, or the state the websocket +// broadcasts and the resources API seeds from flips with an unrelated absent +// machine. A saved enrollment only fills an absent machine: when the linked +// agent itself is gone, its continuity row stays separate and the guest keeps +// what vSphere reports. +func TestManualLinkToSupplementalGuestHoldsWithAndWithoutContinuity(t *testing.T) { + now := time.Now().UTC() + guestAgent := models.Host{ + ID: "host-app-guest", MachineID: "machine-app-guest", Hostname: "app-guest", + Platform: "linux", Status: "online", LastSeen: now, IntervalSeconds: 30, + } + vmRecords := []unifiedresources.IngestRecord{{ + SourceID: "vc-1:vm:vm-42", + Resource: unifiedresources.Resource{ + Type: unifiedresources.ResourceTypeVM, + Technology: "vmware", + Name: "app-guest", + Status: unifiedresources.StatusOnline, + LastSeen: now, + VMware: &unifiedresources.VMwareData{ConnectionID: "vc-1", ManagedObjectID: "vm-42", EntityType: "vm"}, + }, + }} + + unlinked := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + unlinked.PopulateSnapshotAndSupplemental( + models.StateSnapshot{Hosts: []models.Host{guestAgent}, LastUpdate: now}, + map[unifiedresources.DataSource][]unifiedresources.IngestRecord{unifiedresources.SourceVMware: vmRecords}, + ) + if len(unlinked.VMs()) != 1 || len(unlinked.Hosts()) != 1 { + t.Fatalf("unlinked estate = %d VMs, %d agents, want one of each", len(unlinked.VMs()), len(unlinked.Hosts())) + } + vmID, agentID := unlinked.VMs()[0].ID(), unlinked.Hosts()[0].ID() + + for _, tc := range []struct { + name string + reporting bool + continuity []config.HostContinuityEntry + }{ + {name: "no-continuity", reporting: true}, + { + name: "absent-machine-continuity", + reporting: true, + continuity: []config.HostContinuityEntry{{ + HostID: "absent-machine", MachineID: "other-machine", Hostname: "other-host", + Platform: "linux", LastSeen: now.Add(-time.Hour), + }}, + }, + { + name: "linked-agent-continuity-only", + continuity: []config.HostContinuityEntry{{ + HostID: guestAgent.ID, MachineID: guestAgent.MachineID, Hostname: guestAgent.Hostname, + Platform: "linux", LastSeen: now.Add(-10 * time.Minute), + }}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + links := unifiedresources.NewMemoryStore() + if err := links.AddLink(unifiedresources.ResourceLink{ResourceA: vmID, ResourceB: agentID, PrimaryID: vmID}); err != nil { + t.Fatalf("add link: %v", err) + } + state := models.NewState() + if tc.reporting { + state.Hosts = []models.Host{guestAgent} + } + m := &Monitor{state: state} + if len(tc.continuity) > 0 { + continuity := config.NewHostContinuityStore(t.TempDir(), nil) + for _, entry := range tc.continuity { + if err := continuity.Upsert(entry); err != nil { + t.Fatal(err) + } + } + m.hostContinuityStore = continuity + } + m.SetResourceStore(unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(links))) + m.SetSupplementalRecordsProvider(unifiedresources.SourceVMware, &testSupplementalInventoryReadinessProvider{ + source: unifiedresources.SourceVMware, + records: vmRecords, + settled: true, + readyAt: now, + }) + + resources, _ := m.UnifiedResourceSnapshot() + var guest, agent *unifiedresources.Resource + for i := range resources { + switch resources[i].ID { + case agentID: + agent = &resources[i] + case vmID: + guest = &resources[i] + } + } + if guest == nil { + t.Fatalf("linked VM %s missing from published inventory", vmID) + } + if tc.reporting { + if agent != nil { + t.Fatalf("linked agent %s still published standalone", agentID) + } + if guest.VMware == nil || guest.Agent == nil { + t.Fatalf("published VM vmware=%t agent=%t, want the VM carrying its agent", guest.VMware != nil, guest.Agent != nil) + } + return + } + if agent == nil || agent.Status != unifiedresources.StatusOffline { + t.Fatalf("saved enrollment row = %+v, want its own offline row", agent) + } + if guest.Agent != nil || guest.Status != unifiedresources.StatusOnline { + t.Fatalf("published VM agent=%t status=%s, want vSphere's online VM untouched by the saved enrollment", guest.Agent != nil, guest.Status) + } + }) + } +} diff --git a/internal/unifiedresources/canonical_id_pins.go b/internal/unifiedresources/canonical_id_pins.go index ece08ab9b7..c48aa0cee3 100644 --- a/internal/unifiedresources/canonical_id_pins.go +++ b/internal/unifiedresources/canonical_id_pins.go @@ -337,8 +337,9 @@ func (index *identityPinIndex) successionsFor(pin ResourceIdentityPin) []Canonic // the new canonical ID before the pin write, so operator intent // (never-auto-remediate, maintenance windows) and action-audit history // survive the era change. Successions are skipped while the old canonical ID -// still belongs to a live resource: a genuinely short-named host must not -// have its rows stolen by an FQDN sibling. Change-journal rows are never +// still belongs to a live resource, or to one a manual link folded into +// another: a genuinely short-named host must not have its rows stolen by an +// FQDN sibling. Change-journal rows are never // rewritten; EraIDs merges those at read time. func (rr *ResourceRegistry) PersistIdentityPins() { if rr.store == nil { @@ -358,7 +359,11 @@ func (rr *ResourceRegistry) PersistIdentityPins() { } pins = append(pins, pin) for _, succession := range rr.identityPins.successionsFor(pin) { - if _, live := rr.resources[succession.OldCanonicalID]; live { + // A primary that absorbed a linked resource also carries its + // machine keys. That resource is still observed, and succeeding + // it would re-key the link onto the primary itself, splitting + // the pair again on the next rebuild. + if rr.canonicalIDObservedLocked(succession.OldCanonicalID) { continue } successions = append(successions, succession) diff --git a/internal/unifiedresources/monitor_adapter.go b/internal/unifiedresources/monitor_adapter.go index 6abf5dfdbf..910a61f5a3 100644 --- a/internal/unifiedresources/monitor_adapter.go +++ b/internal/unifiedresources/monitor_adapter.go @@ -135,7 +135,7 @@ func readStateWithRecords(readState ReadState, source DataSource, records []Inge cloned := NewRegistry(registry.store) thresholds := adapter.currentStaleThresholds() cloned.IngestResourcesWithStaleThresholds(registry.List(), thresholds) - cloned.ingestRecords(source, records, onlyMissing) + cloned.ingestRecords(source, records, onlyMissing, thresholds) overlay := NewMonitorAdapterWithStaleThresholds(cloned, thresholds) if keyed { adapter.overlays.store(registry, rebuiltAt, key, now, overlay) @@ -450,7 +450,7 @@ func (a *MonitorAdapter) replaceRegistryLocked(snapshot models.StateSnapshot, re if len(records) == 0 || strings.TrimSpace(string(source)) == "" { continue } - rebuilt.IngestRecords(source, records) + rebuilt.IngestRecordsWithStaleThresholds(source, records, staleThresholds) } // IngestSnapshot runs its stale pass before the record sources above are // ingested, so record-sourced resources would otherwise keep their @@ -665,7 +665,7 @@ func (a *MonitorAdapter) PopulateSupplementalRecords(source DataSource, records return } before := registry.List() - registry.IngestRecords(source, records) + registry.IngestRecordsWithStaleThresholds(source, records, a.currentStaleThresholds()) rebuiltAt := time.Now().UTC() recordRegistryChanges(registry.store, before, registry.List(), rebuiltAt, nil, SourcePlatformEvent, changeSourceAdapterForDataSource(source)) registry.PersistIdentityPins() diff --git a/internal/unifiedresources/monitor_adapter_read_state_test.go b/internal/unifiedresources/monitor_adapter_read_state_test.go index 7eefcb5b99..dd7bd9da7b 100644 --- a/internal/unifiedresources/monitor_adapter_read_state_test.go +++ b/internal/unifiedresources/monitor_adapter_read_state_test.go @@ -400,6 +400,87 @@ func TestReadStateWithRecordsPreservesConfiguredStaleThresholds(t *testing.T) { } } +// A link whose side arrives as a record joins during record ingest, so it +// must judge freshness by the adapter's thresholds as a snapshot-time link +// does. Under a five-minute vSphere threshold a VM polled three minutes ago +// is current, and its own memory reading outranks its in-guest agent's. +func TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + Hosts: []models.Host{{ + ID: "host-app-guest", + Hostname: "app-guest", + MachineID: "machine-app-guest", + Status: "online", + LastSeen: now, + Memory: models.Memory{Total: 8 << 30, Used: 6 << 30, Free: 2 << 30, Usage: 75}, + }}, + LastUpdate: now, + } + vmRecords := []IngestRecord{{ + SourceID: "vc-1:vm:vm-42", + Resource: Resource{ + Type: ResourceTypeVM, + Technology: "vmware", + Name: "app-guest", + Status: StatusOnline, + LastSeen: now.Add(-3 * time.Minute), + Metrics: &ResourceMetrics{Memory: &MetricValue{Percent: 40, Source: SourceVMware}}, + VMware: &VMwareData{ConnectionID: "vc-1", ManagedObjectID: "vm-42", EntityType: "vm"}, + }, + }} + thresholds := map[DataSource]time.Duration{SourceVMware: 5 * time.Minute} + + unlinked := NewMonitorAdapter(NewRegistry(nil)) + unlinked.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: vmRecords}) + vms, hosts := unlinked.VMs(), unlinked.Hosts() + if len(vms) != 1 || len(hosts) != 1 { + t.Fatalf("unlinked estate = %d VMs, %d agents, want one of each", len(vms), len(hosts)) + } + link := ResourceLink{ResourceA: vms[0].ID(), ResourceB: hosts[0].ID(), PrimaryID: hosts[0].ID()} + + for _, path := range []struct { + name string + read func(adapter *MonitorAdapter) ReadState + }{ + {"rebuild", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: vmRecords}) + return adapter + }}, + {"supplemental", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateFromSnapshot(snapshot) + adapter.PopulateSupplementalRecords(SourceVMware, vmRecords) + return adapter + }}, + {"overlay", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateFromSnapshot(snapshot) + return ReadStateWithRecords(adapter, SourceVMware, vmRecords) + }}, + } { + t.Run(path.name, func(t *testing.T) { + store := NewMemoryStore() + if err := store.AddLink(link); err != nil { + t.Fatalf("add link: %v", err) + } + readState := path.read(NewMonitorAdapterWithStaleThresholds(NewRegistry(store), thresholds)) + + if hosts := readState.Hosts(); len(hosts) != 0 { + t.Fatalf("linked agent still listed standalone: %s", hosts[0].ID()) + } + vms := readState.VMs() + if len(vms) != 1 || vms[0].ID() != link.ResourceA { + t.Fatalf("VMs = %d, want the linked VM %s", len(vms), link.ResourceA) + } + if vms[0].r.Metrics == nil || vms[0].r.Metrics.Memory == nil { + t.Fatal("linked VM lost its memory reading") + } + if memory := vms[0].r.Metrics.Memory; memory.Source != SourceVMware || memory.Percent != 40 { + t.Fatalf("linked VM memory = %.0f%% from %s, want vSphere's current 40%%", memory.Percent, memory.Source) + } + }) + } +} + func TestHostContinuityCannotOverwriteCurrentResource(t *testing.T) { now := time.Now().UTC() for _, mode := range []string{"machine-identity", "source-identity", "future-timestamp", "provider-only"} { diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 48f2148fcd..c1b036344f 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -95,6 +95,10 @@ type ResourceRegistry struct { exclusions map[string]struct{} identityPins *identityPinIndex pbsBackups []models.PBSBackup + // linkMergedIDs holds the resources a manual link folded into their + // primary. They are still observed, so identity succession treats them + // as live. + linkMergedIDs map[string]struct{} // supersededIndex maps record-declared retired canonical IDs to the live // resource that superseded them, so references persisted under a retired // ID (availability links, API reads) keep resolving. An empty value marks @@ -611,10 +615,16 @@ func (rr *ResourceRegistry) ingestSnapshot(snapshot models.StateSnapshot, thresh // IngestRecords ingests normalized records for a single source. func (rr *ResourceRegistry) IngestRecords(source DataSource, records []IngestRecord) { - rr.ingestRecords(source, records, false) + rr.ingestRecords(source, records, false, nil) } -func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRecord, onlyMissing bool) { +// IngestRecordsWithStaleThresholds ingests normalized records for a single +// source and merges operator-linked resources with caller-owned thresholds. +func (rr *ResourceRegistry) IngestRecordsWithStaleThresholds(source DataSource, records []IngestRecord, thresholds map[DataSource]time.Duration) { + rr.ingestRecords(source, records, false, thresholds) +} + +func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRecord, onlyMissing bool, thresholds map[DataSource]time.Duration) { var successions []CanonicalIDSuccession supersededSeen := make(map[string]struct{}) for _, record := range records { @@ -649,6 +659,16 @@ func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRec rr.applyRecordSuccessions(successions) rr.mu.Lock() + // Either side of an operator link can arrive as a record: a vSphere or + // TrueNAS VM with a Pulse agent inside, or an agent on a TrueNAS host. + // Snapshot and resource ingest apply the links, so record ingest must too, + // or the monitor's rebuilt registry keeps both rows while REST, which + // seeds from that registry, shows them merged. Continuity records only + // fill absent machines: a saved enrollment joined to its linked guest + // would lend the guest its offline verdict and old agent payload. + if !onlyMissing { + rr.applyManualLinks(thresholds) + } rr.refreshStorageConsumersLocked() rr.refreshPBSRollupsLocked() rr.refreshStoragePostureLocked() @@ -746,12 +766,25 @@ func (rr *ResourceRegistry) proxmoxGuestResourceIDForSourceRefLocked(ref string) return uniqueID } +// canonicalIDObservedLocked reports whether a canonical ID still names an +// observed resource: a live row, or one a manual link folded into its primary. +// Succeeding an observed ID would hand its operator rows, and the link that +// folded it, to another resource. +func (rr *ResourceRegistry) canonicalIDObservedLocked(id string) bool { + if _, live := rr.resources[id]; live { + return true + } + _, folded := rr.linkMergedIDs[id] + return folded +} + // applyRecordSuccessions re-keys operator-owned store rows from canonical IDs // that ingested records declared superseded (IngestRecord.SupersededCanonicalIDs) // onto the records' current canonical IDs. Mirrors the guards of pin-driven -// successions in PersistIdentityPins: a superseded ID still held by a live -// resource is skipped so a genuinely distinct sibling never has its rows -// stolen. Re-runs are cheap no-ops once the old rows are gone. +// successions in PersistIdentityPins: a superseded ID still observed, live or +// folded into another resource by a manual link, is skipped so a genuinely +// distinct sibling never has its rows stolen. Re-runs are cheap no-ops once +// the old rows are gone. func (rr *ResourceRegistry) applyRecordSuccessions(successions []CanonicalIDSuccession) { if len(successions) == 0 || rr.store == nil { return @@ -764,7 +797,7 @@ func (rr *ResourceRegistry) applyRecordSuccessions(successions []CanonicalIDSucc rr.mu.RLock() kept := make([]CanonicalIDSuccession, 0, len(successions)) for _, succession := range successions { - if _, live := rr.resources[succession.OldCanonicalID]; live { + if rr.canonicalIDObservedLocked(succession.OldCanonicalID) { continue } kept = append(kept, succession) @@ -4811,6 +4844,10 @@ func (rr *ResourceRegistry) applyManualLinks(thresholds map[DataSource]time.Dura rr.mergeResourceData(primary, other, thresholds) delete(rr.resources, otherID) + if rr.linkMergedIDs == nil { + rr.linkMergedIDs = make(map[string]struct{}) + } + rr.linkMergedIDs[otherID] = struct{}{} rr.updateSourceMappings(otherID, primaryID) } } @@ -4876,6 +4913,12 @@ func (rr *ResourceRegistry) mergeResourceData(primary *Resource, other *Resource if primary.Ceph == nil { primary.Ceph = other.Ceph } + if primary.TrueNAS == nil { + primary.TrueNAS = other.TrueNAS + } + if primary.VMware == nil { + primary.VMware = other.VMware + } // Manual links combine already-normalized resources. Preserve each metric's // recorded source instead of flattening the linked resource to SourceAgent. diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index e4d7080211..c133a0e533 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -441,6 +441,319 @@ func TestManualLinkKeepsTheLiveSightingOfASharedSource(t *testing.T) { } } +// Either side of an operator link can arrive as a supplemental record: a +// vSphere VM with a Pulse agent inside, or an agent on a TrueNAS host. The +// monitor's rebuild ingests records after the snapshot, so it must join them +// there too, keep the record side's provider payload, and list what the +// resources API lists after seeding from it. +func TestManualLinksJoinRecordIngestedResourcesInMonitorRebuild(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + Hosts: []models.Host{ + {ID: "host-app-guest", Hostname: "app-guest", MachineID: "machine-app-guest", Status: "online", LastSeen: now}, + {ID: "host-nas", Hostname: "nas-a-mgmt", MachineID: "machine-nas-a", Status: "online", LastSeen: now}, + }, + LastUpdate: now, + } + vmRecord := IngestRecord{ + SourceID: "vc-1:vm:vm-42", + Resource: Resource{ + Type: ResourceTypeVM, + Technology: "vmware", + Name: "app-guest", + Status: StatusOnline, + LastSeen: now, + VMware: &VMwareData{ConnectionID: "vc-1", ManagedObjectID: "vm-42", EntityType: "vm"}, + }, + Identity: ResourceIdentity{Hostnames: []string{"app-guest"}}, + } + records := map[DataSource][]IngestRecord{ + SourceVMware: {vmRecord}, + SourceTrueNAS: { + { + SourceID: "system:tn-1", + Resource: Resource{ + Type: ResourceTypeAgent, + Name: "nas-a", + Status: StatusOnline, + LastSeen: now, + TrueNAS: &TrueNASData{Hostname: "nas-a"}, + }, + Identity: ResourceIdentity{Hostnames: []string{"nas-a"}}, + }, + { + SourceID: "system:tn-1:pool:tank", + ParentSourceID: "system:tn-1", + Resource: Resource{ + Type: ResourceTypeStorage, + Name: "tank", + Status: StatusOnline, + LastSeen: now, + Storage: &StorageMeta{Type: "zfs-pool", Platform: "truenas", Topology: "pool"}, + }, + }, + }, + } + + for _, agentPrimary := range []bool{false, true} { + name := "record-primary" + if agentPrimary { + name = "agent-primary" + } + t.Run(name, func(t *testing.T) { + store := NewMemoryStore() + adapter := NewMonitorAdapter(NewRegistry(store)) + adapter.PopulateSnapshotAndSupplemental(snapshot, records) + + var vmID, guestAgentID, systemID, nasAgentID string + for _, resource := range adapter.GetAll() { + switch { + case resource.VMware != nil: + vmID = resource.ID + case resource.TrueNAS != nil: + systemID = resource.ID + case resource.Name == "app-guest": + guestAgentID = resource.ID + case resource.Name == "nas-a-mgmt": + nasAgentID = resource.ID + } + } + if vmID == "" || guestAgentID == "" || systemID == "" || nasAgentID == "" { + t.Fatalf("unlinked estate = %v, want the VM, both agents and the TrueNAS system", resourceIDs(adapter.GetAll())) + } + guestPrimary, nasPrimary := vmID, systemID + if agentPrimary { + guestPrimary, nasPrimary = guestAgentID, nasAgentID + } + for _, link := range []ResourceLink{ + {ResourceA: vmID, ResourceB: guestAgentID, PrimaryID: guestPrimary}, + {ResourceA: systemID, ResourceB: nasAgentID, PrimaryID: nasPrimary}, + } { + if err := store.AddLink(link); err != nil { + t.Fatalf("add link: %v", err) + } + } + + assertJoined := func(stage string) { + t.Helper() + resources := adapter.GetAll() + if len(resources) != 3 { + t.Fatalf("%s: resources = %v, want the linked VM, the linked TrueNAS system and its pool", stage, resourceIDs(resources)) + } + // An agent inside a guest supplements the guest, whichever + // side the operator chose as primary. + guest, ok := adapter.currentRegistry().Get(vmID) + if !ok { + t.Fatalf("%s: linked vSphere VM %s missing from %v", stage, vmID, resourceIDs(resources)) + } + if guest.Type != ResourceTypeVM || guest.VMware == nil || guest.Agent == nil { + t.Fatalf("%s: linked VM type=%s vmware=%t agent=%t, want the VM carrying its agent", stage, guest.Type, guest.VMware != nil, guest.Agent != nil) + } + if !slices.Contains(guest.Sources, SourceVMware) || !slices.Contains(guest.Sources, SourceAgent) { + t.Fatalf("%s: linked VM sources = %v, want vmware and agent", stage, guest.Sources) + } + system, ok := adapter.currentRegistry().Get(nasPrimary) + if !ok { + t.Fatalf("%s: linked TrueNAS system %s missing from %v", stage, nasPrimary, resourceIDs(resources)) + } + if system.TrueNAS == nil || system.Agent == nil { + t.Fatalf("%s: linked system truenas=%t agent=%t, want the primary carrying both payloads", stage, system.TrueNAS != nil, system.Agent != nil) + } + if !slices.Contains(system.Sources, SourceTrueNAS) || !slices.Contains(system.Sources, SourceAgent) { + t.Fatalf("%s: linked system sources = %v, want truenas and agent", stage, system.Sources) + } + pool := onlyResourceOfType(t, adapter.currentRegistry(), ResourceTypeStorage) + if pool.ParentID == nil || *pool.ParentID != nasPrimary { + t.Fatalf("%s: pool parent = %v, want the linked system %s", stage, pool.ParentID, nasPrimary) + } + + // Identity pins persisted by the rebuild must not re-key a link + // onto its own primary. + links, err := store.GetLinks() + if err != nil { + t.Fatalf("%s: get links: %v", stage, err) + } + for _, link := range links { + if link.ResourceA == link.ResourceB { + t.Fatalf("%s: link collapsed onto %s", stage, link.ResourceA) + } + } + + // The resources API seeds its registry from this listing + // through resource ingest and must agree with it. + rest := NewRegistry(store) + rest.IngestResources(resources) + if got, want := resourceIDs(rest.List()), resourceIDs(resources); !slices.Equal(got, want) { + t.Fatalf("%s: a registry seeded from the monitor lists %v, monitor lists %v", stage, got, want) + } + } + + next := snapshot + next.LastUpdate = now.Add(time.Second) + adapter.PopulateSnapshotAndSupplemental(next, records) + assertJoined("rebuild") + + adapter.PopulateSupplementalRecords(SourceVMware, records[SourceVMware]) + adapter.PopulateSupplementalRecords(SourceTrueNAS, records[SourceTrueNAS]) + assertJoined("supplemental refresh") + + next.LastUpdate = now.Add(2 * time.Second) + adapter.PopulateSnapshotAndSupplemental(next, records) + assertJoined("second rebuild") + }) + } +} + +// A Proxmox node that absorbs a linked agent carries the agent's machine key, +// so the rebuild pins the node with it. The agent is still observed: that pin +// must not succeed the agent's ID onto the node, which re-keyed the link onto +// the node itself and split the pair on the next rebuild. +func TestManualLinkKeepsItsEndpointsThroughIdentityPinPersistence(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + Nodes: []models.Node{{ + ID: "lab-pve1", Name: "pve1", Instance: "lab", + Host: "https://192.0.2.10:8006", Status: "online", LastSeen: now, + }}, + Hosts: []models.Host{{ + ID: "host-box", Hostname: "box-x", MachineID: "machine-box", Status: "online", LastSeen: now, + }}, + LastUpdate: now, + } + store := NewMemoryStore() + adapter := NewMonitorAdapter(NewRegistry(store)) + adapter.PopulateSnapshotAndSupplemental(snapshot, nil) + var nodeID, agentID string + for _, resource := range adapter.GetAll() { + if resource.Proxmox != nil { + nodeID = resource.ID + } else if resource.Agent != nil { + agentID = resource.ID + } + } + if nodeID == "" || agentID == "" || nodeID == agentID { + t.Fatalf("unlinked estate = %v, want a node and a separate agent", resourceIDs(adapter.GetAll())) + } + if err := store.AddLink(ResourceLink{ResourceA: nodeID, ResourceB: agentID, PrimaryID: nodeID}); err != nil { + t.Fatalf("add link: %v", err) + } + + for rebuild := 1; rebuild <= 3; rebuild++ { + next := snapshot + next.LastUpdate = now.Add(time.Duration(rebuild) * time.Second) + adapter.PopulateSnapshotAndSupplemental(next, nil) + if got := resourceIDs(adapter.GetAll()); !slices.Equal(got, []string{nodeID}) { + t.Fatalf("rebuild %d: resources = %v, want only the linked node %s", rebuild, got, nodeID) + } + links, err := store.GetLinks() + if err != nil { + t.Fatalf("get links: %v", err) + } + if len(links) != 1 || links[0].ResourceA != nodeID || links[0].ResourceB != agentID { + t.Fatalf("rebuild %d: links = %+v, want %s -> %s kept", rebuild, links, nodeID, agentID) + } + } +} + +// A record may name an older canonical ID as superseded on every ingest. When +// that ID belongs to a resource a manual link folded into the record's own +// resource, it is still observed: succeeding it on a later refresh would +// re-key the link onto its primary. +func TestRecordDeclaredSuccessionSparesALinkFoldedResource(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + Hosts: []models.Host{{ID: "host-nas", Hostname: "nas-a-mgmt", MachineID: "machine-nas-a", Status: "online", LastSeen: now}}, + LastUpdate: now, + } + agentID := MachineIdentityCanonicalID(ResourceTypeAgent, "machine-nas-a") + records := []IngestRecord{{ + SourceID: "system:tn-1", + SupersededCanonicalIDs: []string{agentID}, + Resource: Resource{ + Type: ResourceTypeAgent, + Name: "nas-a", + Status: StatusOnline, + LastSeen: now, + TrueNAS: &TrueNASData{Hostname: "nas-a"}, + }, + Identity: ResourceIdentity{Hostnames: []string{"nas-a"}}, + }} + + unlinked := NewMonitorAdapter(NewRegistry(nil)) + unlinked.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceTrueNAS: records}) + var systemID string + for _, resource := range unlinked.GetAll() { + if resource.TrueNAS != nil { + systemID = resource.ID + } + } + if systemID == "" || !slices.Contains(resourceIDs(unlinked.GetAll()), agentID) { + t.Fatalf("unlinked estate = %v, want agent %s and a TrueNAS system", resourceIDs(unlinked.GetAll()), agentID) + } + + store := NewMemoryStore() + if err := store.AddLink(ResourceLink{ResourceA: systemID, ResourceB: agentID, PrimaryID: systemID}); err != nil { + t.Fatalf("add link: %v", err) + } + adapter := NewMonitorAdapter(NewRegistry(store)) + adapter.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceTrueNAS: records}) + adapter.PopulateSupplementalRecords(SourceTrueNAS, records) + + if got := resourceIDs(adapter.GetAll()); !slices.Equal(got, []string{systemID}) { + t.Fatalf("resources = %v, want only the linked system %s", got, systemID) + } + links, err := store.GetLinks() + if err != nil { + t.Fatalf("get links: %v", err) + } + if len(links) != 1 || links[0].ResourceA != systemID || links[0].ResourceB != agentID { + t.Fatalf("links = %+v, want %s -> %s kept", links, systemID, agentID) + } +} + +// A link merge folds the other resource's provider payloads into the primary +// where the primary has none, whichever side the operator chose. +func TestManualLinkKeepsProviderPayloadsThePrimaryLacks(t *testing.T) { + now := time.Now().UTC() + store := NewMemoryStore() + for _, link := range []ResourceLink{ + {ResourceA: "agent-nas", ResourceB: "truenas-system", PrimaryID: "agent-nas"}, + {ResourceA: "agent-esxi", ResourceB: "vmware-host", PrimaryID: "agent-esxi"}, + } { + if err := store.AddLink(link); err != nil { + t.Fatalf("add link: %v", err) + } + } + host := func(id, name string, sources ...DataSource) Resource { + status := make(map[DataSource]SourceStatus, len(sources)) + for _, source := range sources { + status[source] = SourceStatus{Status: "online", LastSeen: now} + } + return Resource{ID: id, Type: ResourceTypeAgent, Name: name, Status: StatusOnline, LastSeen: now, Sources: sources, SourceStatus: status} + } + agentNAS := host("agent-nas", "nas-a-mgmt", SourceAgent) + agentNAS.Agent = &AgentData{AgentID: "agent-nas", Hostname: "nas-a-mgmt"} + system := host("truenas-system", "nas-a", SourceTrueNAS) + system.TrueNAS = &TrueNASData{Hostname: "nas-a"} + agentESXi := host("agent-esxi", "esxi-01", SourceAgent) + agentESXi.Agent = &AgentData{AgentID: "agent-esxi", Hostname: "esxi-01"} + esxi := host("vmware-host", "esxi-01.lab", SourceVMware) + esxi.VMware = &VMwareData{ConnectionID: "vc-1", ManagedObjectID: "host-101", EntityType: "host"} + + rr := NewRegistry(store) + rr.IngestResources([]Resource{agentNAS, system, agentESXi, esxi}) + + if got := resourceIDs(rr.List()); !slices.Equal(got, []string{"agent-esxi", "agent-nas"}) { + t.Fatalf("resources = %v, want the two linked primaries", got) + } + if nas, _ := rr.Get("agent-nas"); nas.TrueNAS == nil || nas.Agent == nil { + t.Fatalf("linked TrueNAS system truenas=%t agent=%t, want both payloads", nas.TrueNAS != nil, nas.Agent != nil) + } + if host, _ := rr.Get("agent-esxi"); host.VMware == nil || host.Agent == nil { + t.Fatalf("linked vSphere host vmware=%t agent=%t, want both payloads", host.VMware != nil, host.Agent != nil) + } +} + func TestLeaseExpiredKubernetesClusterStaysOffline(t *testing.T) { rr := NewRegistry(nil) rr.IngestSnapshot(models.StateSnapshot{ From 9bd1b6e58202aed9458901b89f35281a7ad0a3c7 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:52:31 +0100 Subject: [PATCH 6/7] Let only POST narrate single-resource reports with Pulse Assistant GET /api/admin/reports/generate resolved the tenant's AI narrator, so a PDF requested with GET made a paid provider call and appended a usage event to the AI cost ledger. GET passes the demo-mode guard and the CSRF check, and a SameSite=Lax session cookie rides a cross-site top-level GET navigation, so a link could spend a signed-in admin's AI budget. POST with a JSON body, decoded with the generate-multi rules, is now the canonical single-resource transport and the only one that may use the AI narrator. The settings UI sends it, following a new performanceReport.singleResourceMethod catalog field; a catalog without the field, and the legacy fallback catalog, keep the GET transport that older backends accept. GET stays available with the same query parameters but narrates deterministically, printing a note in place of the "Configure Pulse Assistant" tip, and other methods get 405 with Allow: GET, POST. GET also stops resolving the AI service at all: it reads Patrol findings through a new ExistingAIService lookup, because GetAIService constructs a tenant service on first use and that can list provider models or start background discovery. The router's reporting wiring moves into wireReportingAIResolvers so a test can drive it. Keeping GET rather than answering 405 avoids breaking documented API scripts (MSP.md pointed providers at it) for no security gain: CSV is never narrated and GET no longer reaches the provider. --- docs/API.md | 11 +- docs/MSP.md | 6 +- .../v6/internal/subsystems/agent-lifecycle.md | 10 ++ .../v6/internal/subsystems/ai-runtime.md | 12 ++ .../v6/internal/subsystems/api-contracts.md | 26 +++ .../subsystems/deployment-installability.md | 8 + .../subsystems/frontend-primitives.md | 7 + .../internal/subsystems/security-privacy.md | 12 ++ .../internal/subsystems/storage-recovery.md | 13 ++ .../dad3b270f160419c.json | 42 +++++ frontend-modern/public/docs/API.md | 11 +- frontend-modern/public/docs/MSP.md | 6 +- .../__tests__/ReportingPanel.test.tsx | 1 + .../reportingCatalogModel.branchcov2.test.ts | 14 ++ .../__tests__/reportingCatalogModel.test.ts | 2 + .../__tests__/reportingPanelModel.test.ts | 59 ++++++- .../__tests__/useReportingPanelState.test.ts | 43 +++++ .../Settings/reportingCatalogModel.ts | 8 + .../Settings/reportingPanelModel.ts | 35 +++- .../src/utils/__tests__/docsLinks.test.ts | 21 +++ internal/api/ai_handlers.go | 19 ++ internal/api/ai_handlers_more_test.go | 30 ++++ internal/api/contract_test.go | 164 ++++++++++++++++++ internal/api/metrics_reporting_handlers.go | 145 +++++++++++++--- internal/api/reporting_handlers_test.go | 43 ++++- internal/api/router.go | 72 +++++--- pkg/reporting/catalog.go | 3 + 27 files changed, 739 insertions(+), 84 deletions(-) create mode 100644 frontend-modern/browser-verification/dad3b270f160419c.json diff --git a/docs/API.md b/docs/API.md index 296d4cc116..50b8809acd 100644 --- a/docs/API.md +++ b/docs/API.md @@ -836,9 +836,12 @@ delivery for all resources. - `GET /api/admin/reports/catalog` (admin, `settings:read`) - Returns the canonical reporting catalog for the settings surface, including locked-state teaser copy, enabled-surface guidance copy, performance-report options, canonical single-report filename subject, canonical fallback filename date style, and the nested VM inventory export definition. - Metadata route: readable without the `advanced_reporting` feature so locked admin surfaces can render the same reporting definition before upsell. -- `GET /api/admin/reports/generate` (admin, `settings:read`) - - Query params: `format` (pdf/csv, default `pdf`), `resourceType`, `resourceId`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) +- `POST /api/admin/reports/generate` (admin, `settings:read`) + - Body fields: `format` (pdf/csv, default `pdf`), `resourceType`, `resourceId`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) - If `title` is omitted, the backend applies the canonical default title for that resource report. + - When Pulse Assistant is configured, the configured AI provider narrates a PDF's executive summary; each narration counts against the AI budget and appears in AI usage. If the call fails, times out or the budget is spent, the PDF falls back to the deterministic summary. CSV output is never narrated. The settings UI uses this method. +- `GET /api/admin/reports/generate` (admin, `settings:read`) + - Takes the same fields as query params and returns the same report, except that it never calls an AI provider: a PDF carries the deterministic summary and a note saying AI narration needs `POST`. Other methods return `405` with `Allow: GET, POST`. - `POST /api/admin/reports/generate-multi` (admin, `settings:read`) - Body fields: `resources` (1-50 entries of `{resourceType,resourceId}`), `format`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) - If `title` is omitted, the backend applies the canonical default fleet report title. @@ -851,8 +854,8 @@ Validation and limits: - Malformed `start`/`end` values, `end` not strictly after `start`, or report windows over 366 days return `400 invalid_time_range`. - `metricType` must match `[a-zA-Z0-9._:-]+` and be <= 64 chars, otherwise `400 invalid_metric_type`. - `title` must be <= 256 chars, otherwise `400 invalid_title`. -- Multi-report body max size is 1MB; oversized payloads return `400 body_too_large`. -- Multi-report bodies reject trailing payload and unknown JSON fields with `400 invalid_body`. +- Report body max size is 1MB; oversized payloads return `400 body_too_large`. +- Report bodies reject trailing payload and unknown JSON fields with `400 invalid_body`. - VM inventory export only accepts `csv`. Common reporting error codes: diff --git a/docs/MSP.md b/docs/MSP.md index 7561f520c6..9302b66a1d 100644 --- a/docs/MSP.md +++ b/docs/MSP.md @@ -260,10 +260,12 @@ Each client runtime (or organization) generates its own reports, scoped to that client's resources: - **UI**: Settings โ†’ Data & Reports. -- **API**: `GET /api/admin/reports/generate` (single resource) and +- **API**: `POST /api/admin/reports/generate` (single resource) and `POST /api/admin/reports/generate-multi` (up to 50 resources per report), returning PDF or CSV. In shared-process mode, scope with `X-Pulse-Org-ID` - or an org-bound token. + or an org-bound token. `GET /api/admin/reports/generate` with query + parameters still returns a single-resource report, but never adds the + Pulse Assistant narrative. - **Schedules**: `GET`/`POST /api/admin/reports/schedules`, `PUT`/`DELETE /api/admin/reports/schedules/{id}`, and `POST /api/admin/reports/schedules/{id}/run`. Schedules can target explicit diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index a2ee20031e..d78de6b180 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -7639,6 +7639,16 @@ bounded and classified temperature nodes, and leaves the host report intact when the helper is absent or incompatible. It does not treat Windows ACPI thermal zones as hardware sensor evidence, accept remote provider locations, or grant Pulse hardware-control authority. +### Single-resource report AI narration requires POST + +`internal/api/metrics_reporting_handlers.go`, `internal/api/router.go` and +`internal/api/ai_handlers.go` changed only how `/api/admin/reports/generate` +reads its fields (a `POST` JSON body or `GET` query parameters), which of those +may use the tenant's AI narrator (`POST` only), and how a `GET` finds Patrol +findings (`ExistingAIService`, which never constructs a tenant AI service). No +agent registration, enrolment, install, update, removal or report ingest path +changed. + ### Agent profile adoption is reported as a count only The usage telemetry snapshot reads `LoadAgentProfiles` and reports diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 9ab54a9d35..fb573ab97a 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -7510,6 +7510,18 @@ prompt budget. Reporting is therefore an additive consumer of AI runtime, not a new ownership boundary, and the narrator/findings surfaces inherit the same governance the rest of the canonical AI runtime already enforces. +Report narration is caller-requested spend, so on the single-resource HTTP +route only `POST /api/admin/reports/generate` reaches it (fleet reports, from +`POST .../generate-multi` or a report schedule, use the fleet narrator). The +reporting narrator resolver goes through `AISettingsHandler.GetAIService`, +which constructs a tenant service on first use and can then list provider +models or start background discovery. A GET single-resource report never calls +that resolver: +it narrates deterministically and reads Patrol findings through +`AISettingsHandler.ExistingAIService`, which returns only a service that is +already running and never constructs one +(`TestAISettingsHandler_ExistingAIServiceNeverConstructs`, and through the +router's real wiring `TestContract_SingleReportGETDoesNotConstructTenantAIService`). The same canonical AI runtime now also owns the fleet-level report narrative through `report_fleet_narrator.go`. `Service` implements diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 3015b71efb..b00b8c50a6 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -9650,6 +9650,32 @@ narrator must fail closed: nil provider, parse failure, timeout, or empty response causes the engine to fall back to the heuristic narrative without surfacing the AI failure to the caller, so reporting is never blocked by AI availability. +Only `POST /api/admin/reports/generate` may hand the tenant's AI narrator to +the engine, because a narrated PDF makes a paid provider call and appends a +`cost.UsageEvent` to the cost ledger. The single-resource handler accepts +`POST` with a JSON body (the `generate-multi` decoding rules: 1MB cap, unknown +fields and trailing payload rejected, through `decodeReportingRequestBody`) and +`GET` with the same fields as query parameters, and answers any other method +with `405` and `Allow: GET, POST`. GET never calls `resolveNarrator`, whose +`GetAIService` can construct a tenant AI service (and with it list provider +models or start background discovery): it narrates through `getReportNarrator`, +the heuristic summary whose `Disclaimer` replaces the "Configure Pulse +Assistant" tip with a note that AI narration needs POST, and reads Patrol +findings through `SetExistingFindingsResolver`, which `router.go` backs with +`AISettingsHandler.ExistingAIService` so only an already running service is +used. GET passes the demo-mode guard and the CSRF check, and a SameSite=Lax +session cookie rides a cross-site top-level GET navigation, so a link must not +be able to spend AI budget. The reporting catalog advertises the transport as +`performanceReport.singleResourceMethod` (`POST`), and the settings UI follows +it. `generate-multi` and the manual schedule run endpoint +(`POST /api/admin/reports/schedules/{id}/run`) were already POST-only; due +schedules run from the server's scheduler, not an HTTP request. +`TestContract_SingleReportAINarrationRequiresPOST` pins the rule on a real +engine: GET, HEAD, PUT and DELETE make no narrator call and no AI service +resolution, while POST makes one of each. +`TestContract_SingleReportGETDoesNotConstructTenantAIService` drives +`Router.wireReportingAIResolvers` with an uncached tenant: GET constructs no +tenant AI service and POST resolves one. Report alerts keep the alert engine's resolution. The node, VM and container enrichers in `internal/api/metrics_reporting_handlers.go` build every row through `reportActiveAlertInfo` and `reportResolvedAlertInfo`, diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 3af74e0266..14ef915a65 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -6101,6 +6101,14 @@ summaries whose scope, format, and attachments the server fixes. The delivery surface (`/api/admin/reports/schedules`) and tenant-local storage are unchanged. +### Provider docs name POST for single-resource reports + +`docs/MSP.md` and its shipped copy now give `POST /api/admin/reports/generate` +for per-client single-resource reports and say a `GET` with query parameters +still returns the report without the Pulse Assistant narrative, matching the +server, which lets only `POST` narrate. Install, upgrade and provider bundle +delivery are unchanged. + ### Convergence log-reader compatibility (5 September 2026) Scheduled reconciliation run 33939926237 failed when `gh api` refused terminal diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index fd73a5370c..ba4fa40da4 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -4828,6 +4828,13 @@ performance-report transport (`/api/reporting` and `/api/reporting/generate-mult so the reporting panel does not go dead on mixed-version installs, but that compatibility path is intentionally report-only and must not invent the newer catalog-owned VM inventory export surface. +`reportingPanelModel.ts` picks the single-resource transport from the catalog's +`performanceReport.singleResourceMethod`: `POST` sends the request fields as a +JSON body, which is the only transport the server lets narrate a PDF with Pulse +Assistant, and `GET` sends the same fields as query parameters. +`reportingCatalogModel.ts` reads a missing field as `GET`, the only method a +backend that predates the field accepts, and the legacy compatibility catalog +names `GET` for `/api/reporting`. Fleet reports stay `POST` on every backend. `ReportingPanel.tsx` must therefore treat `vmInventoryExport` as optional when it renders a governed reporting catalog. A legacy compatibility catalog with no inventory export still owns a valid enabled reporting surface and must continue diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 1524faa42c..bdc77ad4f1 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -3025,6 +3025,18 @@ Removing the disconnected recorder does not alter alert, action approval or operator authority. An unrelated organization receives no default archive fallback. +### Report GETs cannot spend AI budget + +`Router.wireReportingAIResolvers` in `internal/api/router.go` wires a second +reporting resolver, `SetExistingFindingsResolver`, backed by +`AISettingsHandler.ExistingAIService`. +A `GET /api/admin/reports/generate` uses only that resolver and the +deterministic narrator, so it makes no AI provider call, writes no cost-ledger +entry and never constructs a tenant AI service. GET passes the demo-mode guard +and the CSRF check, and a SameSite=Lax session cookie rides a cross-site +top-level navigation, so AI narration of a single-resource report now requires +a CSRF-checked `POST`. No authentication, scope or session rule changed. + ### Node setup action v7 compatibility Workflow consumers pin `actions/setup-node` to diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index d1199c9224..075774c72d 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6247,6 +6247,19 @@ is in `applyLicensedFeatureConfigSnapshot` (`pkg/server/telemetry_licensed_features.go`), pinned by `TestApplyLicensedFeatureConfigSnapshot_CountsScheduledReportingAndProfiles`. +### Single-resource report AI narration requires POST + +`internal/api/metrics_reporting_handlers.go` now accepts `POST` with a JSON +body for `/api/admin/reports/generate` and lets only that method use the +tenant's AI narrator; `GET` keeps working with query parameters and the +deterministic summary. A narrated PDF records the same AI cost-ledger entry as +before, and only a `POST` can now produce one. A `GET` reads Patrol findings +only from an AI service that is already running +(`AISettingsHandler.ExistingAIService`), so it no longer constructs a tenant AI +service, whose construction opens that tenant's SQLite resource export store. +Report content, backup and recovery enrichment, and tenant storage are +otherwise unchanged. + ### Audit-read activity is a bounded, content-free local history `audit_read_activity.json` follows the same shape as the existing external-agent diff --git a/frontend-modern/browser-verification/dad3b270f160419c.json b/frontend-modern/browser-verification/dad3b270f160419c.json new file mode 100644 index 0000000000..5cee659013 --- /dev/null +++ b/frontend-modern/browser-verification/dad3b270f160419c.json @@ -0,0 +1,42 @@ +{ + "version": 1, + "base_sha": "77d21197927a670076a74e87634e73ea999a566a", + "verified_at": "2026-10-07T19:05:00Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Settings/reportingCatalogModel.ts", + "frontend-modern/src/components/Settings/reportingPanelModel.ts" + ], + "content_sha256": { + "frontend-modern/src/components/Settings/reportingCatalogModel.ts": "fc3ab83f49ac62aaf7ef97b415a0bff1c4e445ca17771efc18304295e94e3df0", + "frontend-modern/src/components/Settings/reportingPanelModel.ts": "e4366a8c50ea897dcb4907b4e488a2c5f758e710d7a9be0d5fe0d3e047e2d9c5" + }, + "routes": [ + "/settings/support/reporting", + "/api/admin/reports/generate" + ], + "viewports": [ + { + "width": 1280, + "height": 800 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Pro dev licence active, Pulse Assistant enabled on a local stub OpenAI-compatible provider that logs each call, real admin session login (no admin bypass)", + "Data & Reports panel with one mock agent selected, then two", + "Downloaded single-resource PDF opened: POST copy carries the stub AI narrative and provenance footer; GET copy carries the deterministic summary and the new GET note in place of the Configure Pulse Assistant tip", + "AI usage summary after the run: report_narrative recorded only for the two UI POST downloads" + ], + "interactions": [ + "Generate Report (1 resource) as PDF at 1280x800 and 390x844: one POST /api/admin/reports/generate with a JSON body, 200 application/pdf, download saved, stub provider called once each", + "Generate Report (1 resource) as CSV at both widths: POST, 200 text/csv, stub provider not called", + "Generate Report (2 resources) as PDF at both widths: POST /api/admin/reports/generate-multi unchanged, 200 application/pdf", + "Top-level browser navigation to GET /api/admin/reports/generate?format=pdf with the session cookie: PDF downloaded, stub provider not called", + "HEAD, PUT, DELETE and PATCH return 405 with Allow GET, POST; a session POST without the CSRF header returns 403", + "No console errors on the reporting panel at either width" + ] +} diff --git a/frontend-modern/public/docs/API.md b/frontend-modern/public/docs/API.md index 296d4cc116..50b8809acd 100644 --- a/frontend-modern/public/docs/API.md +++ b/frontend-modern/public/docs/API.md @@ -836,9 +836,12 @@ delivery for all resources. - `GET /api/admin/reports/catalog` (admin, `settings:read`) - Returns the canonical reporting catalog for the settings surface, including locked-state teaser copy, enabled-surface guidance copy, performance-report options, canonical single-report filename subject, canonical fallback filename date style, and the nested VM inventory export definition. - Metadata route: readable without the `advanced_reporting` feature so locked admin surfaces can render the same reporting definition before upsell. -- `GET /api/admin/reports/generate` (admin, `settings:read`) - - Query params: `format` (pdf/csv, default `pdf`), `resourceType`, `resourceId`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) +- `POST /api/admin/reports/generate` (admin, `settings:read`) + - Body fields: `format` (pdf/csv, default `pdf`), `resourceType`, `resourceId`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) - If `title` is omitted, the backend applies the canonical default title for that resource report. + - When Pulse Assistant is configured, the configured AI provider narrates a PDF's executive summary; each narration counts against the AI budget and appears in AI usage. If the call fails, times out or the budget is spent, the PDF falls back to the deterministic summary. CSV output is never narrated. The settings UI uses this method. +- `GET /api/admin/reports/generate` (admin, `settings:read`) + - Takes the same fields as query params and returns the same report, except that it never calls an AI provider: a PDF carries the deterministic summary and a note saying AI narration needs `POST`. Other methods return `405` with `Allow: GET, POST`. - `POST /api/admin/reports/generate-multi` (admin, `settings:read`) - Body fields: `resources` (1-50 entries of `{resourceType,resourceId}`), `format`, `metricType` (optional), `start`/`end` (RFC3339, optional; defaults to last 24h), `title` (optional) - If `title` is omitted, the backend applies the canonical default fleet report title. @@ -851,8 +854,8 @@ Validation and limits: - Malformed `start`/`end` values, `end` not strictly after `start`, or report windows over 366 days return `400 invalid_time_range`. - `metricType` must match `[a-zA-Z0-9._:-]+` and be <= 64 chars, otherwise `400 invalid_metric_type`. - `title` must be <= 256 chars, otherwise `400 invalid_title`. -- Multi-report body max size is 1MB; oversized payloads return `400 body_too_large`. -- Multi-report bodies reject trailing payload and unknown JSON fields with `400 invalid_body`. +- Report body max size is 1MB; oversized payloads return `400 body_too_large`. +- Report bodies reject trailing payload and unknown JSON fields with `400 invalid_body`. - VM inventory export only accepts `csv`. Common reporting error codes: diff --git a/frontend-modern/public/docs/MSP.md b/frontend-modern/public/docs/MSP.md index 7561f520c6..9302b66a1d 100644 --- a/frontend-modern/public/docs/MSP.md +++ b/frontend-modern/public/docs/MSP.md @@ -260,10 +260,12 @@ Each client runtime (or organization) generates its own reports, scoped to that client's resources: - **UI**: Settings โ†’ Data & Reports. -- **API**: `GET /api/admin/reports/generate` (single resource) and +- **API**: `POST /api/admin/reports/generate` (single resource) and `POST /api/admin/reports/generate-multi` (up to 50 resources per report), returning PDF or CSV. In shared-process mode, scope with `X-Pulse-Org-ID` - or an org-bound token. + or an org-bound token. `GET /api/admin/reports/generate` with query + parameters still returns a single-resource report, but never adds the + Pulse Assistant narrative. - **Schedules**: `GET`/`POST /api/admin/reports/schedules`, `PUT`/`DELETE /api/admin/reports/schedules/{id}`, and `POST /api/admin/reports/schedules/{id}/run`. Schedules can target explicit diff --git a/frontend-modern/src/components/Settings/__tests__/ReportingPanel.test.tsx b/frontend-modern/src/components/Settings/__tests__/ReportingPanel.test.tsx index cf86494145..f947e2968c 100644 --- a/frontend-modern/src/components/Settings/__tests__/ReportingPanel.test.tsx +++ b/frontend-modern/src/components/Settings/__tests__/ReportingPanel.test.tsx @@ -35,6 +35,7 @@ const baseCatalog = { title: 'Performance Reports', description: 'Historical performance reporting', singleResourceEndpoint: '/api/admin/reports/generate', + singleResourceMethod: 'POST', multiResourceEndpoint: '/api/admin/reports/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', diff --git a/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.branchcov2.test.ts b/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.branchcov2.test.ts index b451dc65fd..1b38e0882f 100644 --- a/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.branchcov2.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.branchcov2.test.ts @@ -42,6 +42,7 @@ const basePerformanceReport = () => ({ title: 'Performance Reports', description: 'Historical performance reporting', singleResourceEndpoint: '/api/admin/reports/generate', + singleResourceMethod: 'POST', multiResourceEndpoint: '/api/admin/reports/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', @@ -453,6 +454,19 @@ describe('parseReportingPerformanceReportDefinition (via catalog.performanceRepo }); }); + it('throws when singleResourceMethod is neither GET nor POST', () => { + expectInvalid({ + ...baseCatalog(), + performanceReport: { ...basePerformanceReport(), singleResourceMethod: 'PUT' }, + }); + }); + + it('reads a missing singleResourceMethod as GET, the only method older backends accept', () => { + const { singleResourceMethod: _omitted, ...withoutMethod } = basePerformanceReport(); + const parsed = parseReportingCatalog({ ...baseCatalog(), performanceReport: withoutMethod }); + expect(parsed.performanceReport.singleResourceMethod).toBe('GET'); + }); + it('throws when multiResourceEndpoint is not a string', () => { expectInvalid({ ...baseCatalog(), diff --git a/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.test.ts b/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.test.ts index 97add3f9ae..2b1d289a3c 100644 --- a/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/reportingCatalogModel.test.ts @@ -22,6 +22,7 @@ const baseCatalogPayload = { title: 'Performance Reports', description: 'Historical performance reporting', singleResourceEndpoint: '/api/admin/reports/generate', + singleResourceMethod: 'POST', multiResourceEndpoint: '/api/admin/reports/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', @@ -74,6 +75,7 @@ describe('reporting catalog model', () => { const catalog = buildLegacyReportingCatalogFallback(); expect(catalog.performanceReport.singleResourceEndpoint).toBe('/api/reporting'); + expect(catalog.performanceReport.singleResourceMethod).toBe('GET'); expect(catalog.performanceReport.multiResourceEndpoint).toBe('/api/reporting/generate-multi'); expect(catalog.vmInventoryExport).toBeNull(); }); diff --git a/frontend-modern/src/components/Settings/__tests__/reportingPanelModel.test.ts b/frontend-modern/src/components/Settings/__tests__/reportingPanelModel.test.ts index a058281e36..708b9fe8cb 100644 --- a/frontend-modern/src/components/Settings/__tests__/reportingPanelModel.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/reportingPanelModel.test.ts @@ -14,6 +14,7 @@ const performanceDefinition: ReportingPerformanceReportDefinition = { title: 'Performance Reports', description: 'Historical performance reporting', singleResourceEndpoint: '/api/admin/reports/generate', + singleResourceMethod: 'POST', multiResourceEndpoint: '/api/admin/reports/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', @@ -75,12 +76,52 @@ describe('reporting panel model', () => { ); expect(request.filename).toBe('report-agent-1-20260320.pdf'); + // POST, never GET: only POST may narrate the PDF with Pulse Assistant. + expect(request.request.url).toBe('/api/admin/reports/generate'); + expect(request.request.init).toMatchObject({ + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + }); + expect(JSON.parse(String(request.request.init?.body))).toEqual({ + resourceType: 'agent', + resourceId: 'agent-1', + format: 'pdf', + start: '2026-03-19T12:34:56.000Z', + end: now.toISOString(), + metricType: 'cpu', + }); + }); + + it('keeps the query-string GET transport for backends whose catalog names GET', () => { + const now = new Date('2026-03-20T12:34:56.000Z'); + const request = buildReportingRequest( + { + end: now.toISOString(), + format: 'csv', + metricType: 'cpu', + now, + resources: [{ id: 'vm-1', type: 'vm', name: 'vm-a' }], + start: '2026-03-19T12:34:56.000Z', + title: '', + }, + { + ...performanceDefinition, + singleResourceEndpoint: '/api/reporting', + singleResourceMethod: 'GET', + }, + ); + expect(request.request.init).toBeUndefined(); - expect(request.request.url).toContain('/api/admin/reports/generate?'); - expect(request.request.url).toContain('resourceType=agent'); - expect(request.request.url).toContain('resourceId=agent-1'); - expect(request.request.url).toContain('metricType=cpu'); - expect(request.request.url).not.toContain('title='); + const url = new URL(request.request.url, 'http://pulse.local'); + expect(url.pathname).toBe('/api/reporting'); + expect(Object.fromEntries(url.searchParams)).toEqual({ + resourceType: 'vm', + resourceId: 'vm-1', + format: 'csv', + start: '2026-03-19T12:34:56.000Z', + end: now.toISOString(), + metricType: 'cpu', + }); }); it('builds a fleet reporting request body and filename', () => { @@ -158,9 +199,9 @@ describe('reporting panel model', () => { }, ); - expect(request.request.url).not.toContain('title='); - expect(request.request.url).not.toContain('metricType='); - expect(request.request.url).not.toContain('Custom+fleet+title'); + const body = JSON.parse(String(request.request.init?.body)); + expect(body).not.toHaveProperty('title'); + expect(body).not.toHaveProperty('metricType'); }); it('passes through explicit custom titles without inventing defaults', () => { @@ -186,7 +227,7 @@ describe('reporting panel model', () => { performanceDefinition, ); - expect(request.request.url).toContain('title=Custom+report+title'); + expect(JSON.parse(String(request.request.init?.body)).title).toBe('Custom report title'); }); it('derives canonical range starts from the selected preset', () => { diff --git a/frontend-modern/src/components/Settings/__tests__/useReportingPanelState.test.ts b/frontend-modern/src/components/Settings/__tests__/useReportingPanelState.test.ts index aaff7556e7..5a3caa8bfc 100644 --- a/frontend-modern/src/components/Settings/__tests__/useReportingPanelState.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/useReportingPanelState.test.ts @@ -28,6 +28,7 @@ const catalogPayload = { title: 'Performance Reports', description: 'Historical performance reporting', singleResourceEndpoint: '/api/admin/reports/generate', + singleResourceMethod: 'POST', multiResourceEndpoint: '/api/admin/reports/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', @@ -478,6 +479,48 @@ describe('useReportingPanelState', () => { dispose(); }); + it('posts a single-resource report as a JSON body through apiFetch', async () => { + apiFetchMock.mockImplementation((url: string) => { + if (url === '/api/admin/reports/generate') { + return Promise.resolve(new Response('%PDF', { status: 200 })); + } + if (url === '/api/admin/reports/schedules') { + return Promise.resolve(jsonResponse(schedulesPayload)); + } + return Promise.resolve(jsonResponse(catalogPayload)); + }); + const { createObjectURL, revokeObjectURL } = URL; + URL.createObjectURL = vi.fn(() => 'blob:report'); + URL.revokeObjectURL = vi.fn(); + try { + const { hookState, dispose } = mountHook(); + await flushAsync(); + + hookState.setSelectedResources([{ id: 'vm-1', type: 'vm', name: 'vm-a' }]); + await hookState.handleGenerate(); + + const generate = apiFetchMock.mock.calls.filter(([url]) => + String(url).startsWith('/api/admin/reports/generate'), + ); + expect(generate).toHaveLength(1); + expect(generate[0][0]).toBe('/api/admin/reports/generate'); + expect(generate[0][1]).toMatchObject({ + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + }); + expect(JSON.parse(String(generate[0][1].body))).toMatchObject({ + resourceType: 'vm', + resourceId: 'vm-1', + format: catalogPayload.performanceReport.defaultFormat, + }); + expect(showWarningMock).not.toHaveBeenCalled(); + dispose(); + } finally { + URL.createObjectURL = createObjectURL; + URL.revokeObjectURL = revokeObjectURL; + } + }); + it('saves a Patrol weekly summary without a resource scope', async () => { const savedSchedule = { id: 'digest-1', diff --git a/frontend-modern/src/components/Settings/reportingCatalogModel.ts b/frontend-modern/src/components/Settings/reportingCatalogModel.ts index 52749a55de..33b73fafa6 100644 --- a/frontend-modern/src/components/Settings/reportingCatalogModel.ts +++ b/frontend-modern/src/components/Settings/reportingCatalogModel.ts @@ -6,6 +6,7 @@ import { export type ReportingFormat = 'pdf' | 'csv'; export type ReportingFilenameDateStyle = 'utc_yyyymmdd'; export type ReportingFilenameSubject = 'resource_id'; +export type ReportingSingleResourceMethod = 'GET' | 'POST'; export interface ReportingFormatDefinition { value: ReportingFormat; @@ -24,6 +25,7 @@ export interface ReportingPerformanceReportDefinition { title: string; description: string; singleResourceEndpoint: string; + singleResourceMethod: ReportingSingleResourceMethod; multiResourceEndpoint: string; singleFilenamePrefix: string; singleFilenameSubject: ReportingFilenameSubject; @@ -85,6 +87,7 @@ export function buildLegacyReportingCatalogFallback(): ReportingCatalog { description: 'Generate PDF summaries or CSV metric exports from historical monitoring data for one or more selected resources.', singleResourceEndpoint: '/api/reporting', + singleResourceMethod: 'GET', multiResourceEndpoint: '/api/reporting/generate-multi', singleFilenamePrefix: 'report', singleFilenameSubject: 'resource_id', @@ -176,6 +179,9 @@ function parseReportingPerformanceReportDefinition( typeof candidate.title !== 'string' || typeof candidate.description !== 'string' || typeof candidate.singleResourceEndpoint !== 'string' || + (candidate.singleResourceMethod !== undefined && + candidate.singleResourceMethod !== 'GET' && + candidate.singleResourceMethod !== 'POST') || typeof candidate.multiResourceEndpoint !== 'string' || typeof candidate.singleFilenamePrefix !== 'string' || candidate.singleFilenameSubject !== 'resource_id' || @@ -211,6 +217,8 @@ function parseReportingPerformanceReportDefinition( title: candidate.title, description: candidate.description, singleResourceEndpoint: candidate.singleResourceEndpoint, + // Backends that predate the field accept only GET. + singleResourceMethod: candidate.singleResourceMethod ?? 'GET', multiResourceEndpoint: candidate.multiResourceEndpoint, singleFilenamePrefix: candidate.singleFilenamePrefix, singleFilenameSubject: candidate.singleFilenameSubject, diff --git a/frontend-modern/src/components/Settings/reportingPanelModel.ts b/frontend-modern/src/components/Settings/reportingPanelModel.ts index fbaa61d26a..45a6671e68 100644 --- a/frontend-modern/src/components/Settings/reportingPanelModel.ts +++ b/frontend-modern/src/components/Settings/reportingPanelModel.ts @@ -96,6 +96,7 @@ export function buildReportingRequest( | 'singleFilenamePrefix' | 'singleFilenameSubject' | 'singleResourceEndpoint' + | 'singleResourceMethod' | 'supportsCustomTitle' | 'supportsMetricFilter' >, @@ -105,23 +106,41 @@ export function buildReportingRequest( if (context.resources.length === 1) { const resource = context.resources[0]; - const params = new URLSearchParams({ + const fields = { resourceType: toReportingResourceType(resource.type), resourceId: resource.id, format: context.format, start: context.start, end: context.end, - }); + title: customTitle || undefined, + metricType: metricType || undefined, + }; + const filename = buildReportingFilename(context.format, resource.id, context.now, definition); - if (metricType) { - params.append('metricType', metricType); - } - if (customTitle) { - params.append('title', customTitle); + // Only POST may narrate a PDF with Pulse Assistant; the catalog names + // GET only for backends that accept nothing else. + if (definition.singleResourceMethod === 'POST') { + return { + filename, + request: { + url: definition.singleResourceEndpoint, + init: { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(fields), + }, + }, + }; } + const params = new URLSearchParams(); + for (const [key, value] of Object.entries(fields)) { + if (value) { + params.append(key, value); + } + } return { - filename: buildReportingFilename(context.format, resource.id, context.now, definition), + filename, request: { url: `${definition.singleResourceEndpoint}?${params.toString()}`, }, diff --git a/frontend-modern/src/utils/__tests__/docsLinks.test.ts b/frontend-modern/src/utils/__tests__/docsLinks.test.ts index 0ccae13f89..c37c693e09 100644 --- a/frontend-modern/src/utils/__tests__/docsLinks.test.ts +++ b/frontend-modern/src/utils/__tests__/docsLinks.test.ts @@ -566,6 +566,27 @@ describe('docsLinks', () => { expect(apiReference).not.toContain('Mobile Remote Access'); }); + it('documents POST as the only report transport that uses Pulse Assistant', () => { + const handler = readFileSync( + path.join(repoRoot, 'internal', 'api', 'metrics_reporting_handlers.go'), + 'utf8', + ); + expect(handler).toContain( + 'if r.Method == http.MethodPost {\n\t\tnarrator, _, findings = h.resolveNarrator(r.Context())', + ); + expect(handler).toContain('req.Narrator = getReportNarrator{}'); + for (const guide of ['API.md', 'MSP.md']) { + const rootDoc = readFileSync(path.join(repoRoot, 'docs', guide), 'utf8'); + expect(readFileSync(path.join(frontendRoot, 'public', 'docs', guide), 'utf8')).toBe(rootDoc); + expect(rootDoc).toContain('`POST /api/admin/reports/generate` ('); + } + const apiReference = readFileSync(path.join(repoRoot, 'docs', 'API.md'), 'utf8'); + expect(apiReference).toContain('it never calls an AI provider'); + expect(apiReference).toContain('`405` with `Allow: GET, POST`'); + const msp = readFileSync(path.join(repoRoot, 'docs', 'MSP.md'), 'utf8'); + expect(msp).not.toContain('`GET /api/admin/reports/generate` (single resource)'); + }); + it('ships credential-free Proxmox setup instructions and a separate token prompt', () => { const apiReference = readFileSync(path.join(repoRoot, 'docs', 'API.md'), 'utf8'); const shipped = readFileSync(path.join(frontendRoot, 'public', 'docs', 'API.md'), 'utf8'); diff --git a/internal/api/ai_handlers.go b/internal/api/ai_handlers.go index 62ee996eff..0986b24b6d 100644 --- a/internal/api/ai_handlers.go +++ b/internal/api/ai_handlers.go @@ -385,6 +385,25 @@ func (h *AISettingsHandler) loadAIConfig(ctx context.Context) (*config.AIConfig, } // GetAIService returns the underlying AI service +// ExistingAIService returns the org's AI service only when one already exists +// and never constructs one. GetAIService builds and configures a tenant service +// on first use, which can list provider models and start background discovery, +// so read paths that must not reach an AI provider use this instead. +func (h *AISettingsHandler) ExistingAIService(ctx context.Context) *ai.Service { + if h == nil { + return nil + } + mtPersistence, mtMonitor, _, _, _, _ := h.stateRefs() + defaultAIService := h.providerSnapshot().defaultAIService + orgID := GetOrgID(ctx) + if orgID == "default" || orgID == "" || (mtPersistence == nil && mtMonitor == nil) { + return defaultAIService + } + h.aiServicesMu.RLock() + defer h.aiServicesMu.RUnlock() + return h.aiServices[orgID] +} + func (h *AISettingsHandler) GetAIService(ctx context.Context) *ai.Service { mtPersistence, mtMonitor, _, _, _, _ := h.stateRefs() providers := h.providerSnapshot() diff --git a/internal/api/ai_handlers_more_test.go b/internal/api/ai_handlers_more_test.go index 52339f4893..8b38b132ff 100644 --- a/internal/api/ai_handlers_more_test.go +++ b/internal/api/ai_handlers_more_test.go @@ -303,6 +303,36 @@ func TestAISettingsHandler_GetAIService_MultiTenantProviders(t *testing.T) { } } +// Report GETs read Patrol findings through ExistingAIService, so it must never +// construct a tenant service: construction can list provider models and start +// background discovery. +func TestAISettingsHandler_ExistingAIServiceNeverConstructs(t *testing.T) { + handler := NewAISettingsHandler(config.NewMultiTenantPersistence(t.TempDir()), nil, nil) + t.Cleanup(handler.StopServices) + tenant := context.WithValue(context.Background(), OrgIDContextKey, "tenant-1") + + if svc := handler.ExistingAIService(tenant); svc != nil { + t.Fatalf("expected no service before the tenant's first GetAIService, got %p", svc) + } + handler.aiServicesMu.RLock() + built := len(handler.aiServices) + handler.aiServicesMu.RUnlock() + if built != 0 { + t.Fatalf("ExistingAIService constructed %d tenant services, want 0", built) + } + + created := handler.GetAIService(tenant) + if created == nil { + t.Fatal("expected GetAIService to construct the tenant service") + } + if got := handler.ExistingAIService(tenant); got != created { + t.Fatalf("expected the running tenant service %p, got %p", created, got) + } + if got, want := handler.ExistingAIService(context.Background()), handler.GetAIService(context.Background()); got != want { + t.Fatalf("expected the default service %p, got %p", want, got) + } +} + func TestAISettingsHandler_GetAIService_UsesTenantReadState(t *testing.T) { tmp := t.TempDir() mtp := config.NewMultiTenantPersistence(tmp) diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 953ed0fba2..d0bebe25b3 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -5081,6 +5081,7 @@ func TestContract_ReportingCatalogJSONSnapshot(t *testing.T) { "title":"Performance Reports", "description":"Generate PDF summaries or CSV metric exports from historical monitoring data for one or more selected resources.", "singleResourceEndpoint":"/api/admin/reports/generate", + "singleResourceMethod":"POST", "multiResourceEndpoint":"/api/admin/reports/generate-multi", "singleFilenamePrefix":"report", "singleFilenameSubject":"resource_id", @@ -5227,6 +5228,169 @@ func TestContract_PerformanceReportTransportUsesCatalogDefinition(t *testing.T) if engine.lastReq.MetricType != "cpu" || engine.lastReq.Title != "Node report" { t.Fatalf("expected trimmed canonical optional fields, got %+v", engine.lastReq) } + + post := httptest.NewRequest( + http.MethodPost, + "/api/reporting", + strings.NewReader(`{"resourceType":"node","resourceId":"node-1","metricType":" cpu ","title":" Node report "}`), + ) + rec = httptest.NewRecorder() + handler.HandleGenerateReport(rec, post) + if rec.Code != http.StatusOK { + t.Fatalf("expected POST 200, got %d body=%s", rec.Code, rec.Body.String()) + } + if engine.lastReq.ResourceID != "node-1" || engine.lastReq.Format != definition.DefaultFormat || + engine.lastReq.MetricType != "cpu" || engine.lastReq.Title != "Node report" { + t.Fatalf("expected POST body to carry the same canonical fields as GET, got %+v", engine.lastReq) + } +} + +type recordingReportEngine struct { + *reporting.ReportEngine + requests []reporting.MetricReportRequest +} + +func (e *recordingReportEngine) Generate(req reporting.MetricReportRequest) ([]byte, string, error) { + e.requests = append(e.requests, req) + return e.ReportEngine.Generate(req) +} + +type countingReportNarrator struct{ calls atomic.Int32 } + +func (n *countingReportNarrator) Narrate(context.Context, reporting.NarrativeInput) (reporting.Narrative, error) { + n.calls.Add(1) + return reporting.Narrative{Source: reporting.NarrativeSourceAI, HealthStatus: "HEALTHY", HealthMessage: "narrated"}, nil +} + +// A PDF single-resource report may be narrated by the tenant's Pulse +// Assistant, a paid provider call recorded in the cost ledger. GET passes the +// demo-mode guard and the CSRF check, and SameSite=Lax session cookies ride +// cross-site top-level GET navigations, so only POST, the settings UI +// transport, may reach the AI narrator. GET still returns the report, with +// the deterministic summary and a note in place of the "Configure Pulse +// Assistant" tip. +func TestContract_SingleReportAINarrationRequiresPOST(t *testing.T) { + store, err := metrics.NewStore(metrics.DefaultConfig(t.TempDir())) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + engine := &recordingReportEngine{ReportEngine: reporting.NewReportEngine(reporting.EngineConfig{MetricsStore: store})} + original := reporting.GetEngine() + reporting.SetEngine(engine) + t.Cleanup(func() { reporting.SetEngine(original) }) + + // The narrator resolver stands for GetAIService, which can construct a + // tenant AI service; GET must use only the existing-service findings path. + narrator := &countingReportNarrator{} + var narratorResolves, findingsResolves atomic.Int32 + handler := NewReportingHandlers(nil, nil) + handler.SetNarratorResolver(func(context.Context) (reporting.Narrator, reporting.FleetNarrator, reporting.FindingsProvider) { + narratorResolves.Add(1) + return narrator, nil, nil + }) + handler.SetExistingFindingsResolver(func(context.Context) reporting.FindingsProvider { + findingsResolves.Add(1) + return nil + }) + generate := func(req *http.Request) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + handler.HandleGenerateReport(rec, req) + return rec + } + const query = "/api/admin/reports/generate?format=pdf&resourceType=node&resourceId=node-1" + + rec := generate(httptest.NewRequest(http.MethodGet, query, nil)) + if rec.Code != http.StatusOK || rec.Header().Get("Content-Type") != "application/pdf" { + t.Fatalf("GET report: status %d, content type %q, body=%s", rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()) + } + if got := narrator.calls.Load(); got != 0 { + t.Fatalf("GET report called the AI narrator %d times, want 0", got) + } + narrative, err := engine.NarrativeFor(engine.requests[len(engine.requests)-1]) + if err != nil { + t.Fatalf("GET report narrative: %v", err) + } + if narrative.Source != reporting.NarrativeSourceHeuristic || narrative.Disclaimer != reportGETNarrativeNote { + t.Fatalf("GET report narrative = source %q disclaimer %q, want the deterministic summary with the GET note", narrative.Source, narrative.Disclaimer) + } + + for _, method := range []string{http.MethodHead, http.MethodPut, http.MethodDelete} { + rec := generate(httptest.NewRequest(method, query, nil)) + if rec.Code != http.StatusMethodNotAllowed || rec.Header().Get("Allow") != "GET, POST" { + t.Fatalf("%s report: status %d, Allow %q, want 405 with Allow GET, POST", method, rec.Code, rec.Header().Get("Allow")) + } + } + if got := narrator.calls.Load(); got != 0 { + t.Fatalf("GET, HEAD, PUT and DELETE called the AI narrator %d times, want 0", got) + } + if got := narratorResolves.Load(); got != 0 { + t.Fatalf("GET, HEAD, PUT and DELETE resolved the AI service %d times, want 0", got) + } + if got := findingsResolves.Load(); got != 1 { + t.Fatalf("GET looked up existing Patrol findings %d times, want 1", got) + } + + rec = generate(httptest.NewRequest( + http.MethodPost, + "/api/admin/reports/generate", + strings.NewReader(`{"format":"pdf","resourceType":"node","resourceId":"node-1"}`), + )) + if rec.Code != http.StatusOK || rec.Header().Get("Content-Type") != "application/pdf" { + t.Fatalf("POST report: status %d, content type %q, body=%s", rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()) + } + if got := narrator.calls.Load(); got != 1 { + t.Fatalf("POST report called the AI narrator %d times, want 1", got) + } + if got := narratorResolves.Load(); got != 1 { + t.Fatalf("POST report resolved the AI service %d times, want 1", got) + } +} + +// The router's real resolvers: a GET report for a tenant whose AI service is +// not running must not construct one, because construction can list provider +// models and start background discovery. POST resolves it as before. +func TestContract_SingleReportGETDoesNotConstructTenantAIService(t *testing.T) { + engine := &stubReportingEngine{data: []byte("report"), contentType: "application/pdf"} + original := reporting.GetEngine() + reporting.SetEngine(engine) + t.Cleanup(func() { reporting.SetEngine(original) }) + + router := &Router{ + aiSettingsHandler: NewAISettingsHandler(config.NewMultiTenantPersistence(t.TempDir()), nil, nil), + reportingHandlers: NewReportingHandlers(nil, nil), + } + router.wireReportingAIResolvers() + t.Cleanup(router.aiSettingsHandler.StopServices) + tenantServices := func() int { + router.aiSettingsHandler.aiServicesMu.RLock() + defer router.aiSettingsHandler.aiServicesMu.RUnlock() + return len(router.aiSettingsHandler.aiServices) + } + generate := func(req *http.Request) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + router.reportingHandlers.HandleGenerateReport(rec, req.WithContext(context.WithValue(req.Context(), OrgIDContextKey, "tenant-1"))) + return rec + } + + rec := generate(httptest.NewRequest(http.MethodGet, "/api/admin/reports/generate?format=pdf&resourceType=node&resourceId=node-1", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("GET report: status %d, body=%s", rec.Code, rec.Body.String()) + } + if got := tenantServices(); got != 0 { + t.Fatalf("GET report constructed %d tenant AI services, want 0", got) + } + if _, ok := engine.lastReq.Narrator.(getReportNarrator); !ok || engine.lastReq.FindingsProvider != nil { + t.Fatalf("GET report narrator %T, findings %v; want the deterministic narrator and no findings without a running service", engine.lastReq.Narrator, engine.lastReq.FindingsProvider) + } + + rec = generate(httptest.NewRequest(http.MethodPost, "/api/admin/reports/generate", strings.NewReader(`{"format":"pdf","resourceType":"node","resourceId":"node-1"}`))) + if rec.Code != http.StatusOK { + t.Fatalf("POST report: status %d, body=%s", rec.Code, rec.Body.String()) + } + if got := tenantServices(); got != 1 { + t.Fatalf("POST report resolved %d tenant AI services, want 1", got) + } } // Normalization keeps a disk's last-known temperature when the current diff --git a/internal/api/metrics_reporting_handlers.go b/internal/api/metrics_reporting_handlers.go index 08e4c7b240..f918817170 100644 --- a/internal/api/metrics_reporting_handlers.go +++ b/internal/api/metrics_reporting_handlers.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "regexp" "strings" @@ -99,6 +100,7 @@ type ReportingHandlers struct { mtMonitor *monitoring.MultiTenantMonitor recoveryManager *recoverymanager.Manager narratorResolver func(ctx context.Context) (reporting.Narrator, reporting.FleetNarrator, reporting.FindingsProvider) + existingFindingsResolver func(ctx context.Context) reporting.FindingsProvider settingsStore reportingSystemSettingsStore scheduleRunMu sync.Mutex commercialLicenseResolver func(ctx context.Context) *licenseService @@ -168,6 +170,23 @@ func (h *ReportingHandlers) resolveNarrator(ctx context.Context) (reporting.Narr return h.narratorResolver(ctx) } +// SetExistingFindingsResolver wires the Patrol findings lookup that GET reports +// use. Unlike the narrator resolver it must return only an AI service that is +// already running, never construct one. +func (h *ReportingHandlers) SetExistingFindingsResolver(resolver func(ctx context.Context) reporting.FindingsProvider) { + if h == nil { + return + } + h.existingFindingsResolver = resolver +} + +func (h *ReportingHandlers) resolveExistingFindings(ctx context.Context) reporting.FindingsProvider { + if h == nil || h.existingFindingsResolver == nil { + return nil + } + return h.existingFindingsResolver(ctx) +} + func (h *ReportingHandlers) resolveReportBranding(ctx context.Context) reporting.ReportBranding { service := getLicenseServiceForContext(ctx) entitled := service != nil && service.HasFeature(featureWhiteLabelValue) @@ -300,28 +319,33 @@ func normalizePerformanceReportTimeRange( return start, end, nil } -func decodeMultiReportRequestBody(w http.ResponseWriter, r *http.Request) (multiReportRequestBody, bool) { +// decodeReportingRequestBody strictly decodes one JSON report request body: +// at most 1MB, no unknown fields, no trailing payload. +func decodeReportingRequestBody[T any](w http.ResponseWriter, r *http.Request) (T, bool) { r.Body = http.MaxBytesReader(w, r.Body, reportingMultiReportBodyMax) - var body multiReportRequestBody + var body, zero T decoder := json.NewDecoder(r.Body) decoder.DisallowUnknownFields() - if err := decoder.Decode(&body); err != nil { + err := decoder.Decode(&body) + if err == nil { + // Anything after the value is invalid, and reading past it can also + // hit the size cap. + if trailing := decoder.Decode(&struct{}{}); trailing != io.EOF { + err = errors.New("trailing payload") + if trailing != nil { + err = trailing + } + } + } + if err != nil { var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { writeErrorResponse(w, http.StatusBadRequest, "body_too_large", "Request body must be 1MB or less", nil) - return multiReportRequestBody{}, false + return zero, false } writeErrorResponse(w, http.StatusBadRequest, "invalid_body", "Invalid request body", nil) - return multiReportRequestBody{}, false - } - if decoder.More() { - writeErrorResponse(w, http.StatusBadRequest, "invalid_body", "Invalid request body", nil) - return multiReportRequestBody{}, false - } - if err := decoder.Decode(&struct{}{}); err != io.EOF { - writeErrorResponse(w, http.StatusBadRequest, "invalid_body", "Invalid request body", nil) - return multiReportRequestBody{}, false + return zero, false } return body, true @@ -539,9 +563,67 @@ func (h *ReportingHandlers) listBackupsForReport(ctx context.Context, orgID stri return out } -// HandleGenerateReport generates a report +// singleReportRequestBody carries the single-resource report fields. POST +// sends them as a JSON body; GET sends the same names as query parameters. +type singleReportRequestBody struct { + ResourceType string `json:"resourceType"` + ResourceID string `json:"resourceId"` + Format string `json:"format"` + Start string `json:"start"` + End string `json:"end"` + Title string `json:"title"` + MetricType string `json:"metricType"` +} + +func singleReportRequestFromQuery(q url.Values) singleReportRequestBody { + return singleReportRequestBody{ + ResourceType: q.Get("resourceType"), + ResourceID: q.Get("resourceId"), + Format: q.Get("format"), + Start: q.Get("start"), + End: q.Get("end"), + Title: q.Get("title"), + MetricType: q.Get("metricType"), + } +} + +// reportGETNarrativeNote takes the place of the PDF's "Configure Pulse +// Assistant" tip on a GET report, which stays deterministic whatever the +// Assistant settings. +const reportGETNarrativeNote = "Tip: This report was requested with GET, which never uses Pulse Assistant. Generate it from Settings, or request it with POST, to include AI narration when Pulse Assistant is configured." + +// getReportNarrator narrates GET single-resource reports with the +// deterministic summary and never calls an AI provider. +type getReportNarrator struct{} + +func (getReportNarrator) Narrate(ctx context.Context, in reporting.NarrativeInput) (reporting.Narrative, error) { + out, err := reporting.HeuristicNarrator{}.Narrate(ctx, in) + out.Source = reporting.NarrativeSourceHeuristic + out.Disclaimer = reportGETNarrativeNote + return out, err +} + +// HandleGenerateReport generates a single-resource report. POST with a JSON +// body is the canonical transport, used by the settings UI, and the only one +// that may narrate a PDF with the tenant's Pulse Assistant, which spends AI +// budget and records usage in the cost ledger. GET takes the same fields as +// query parameters and always narrates deterministically: GET passes the +// demo-mode guard and the CSRF check, and SameSite=Lax session cookies ride +// cross-site top-level GET navigations, so a link must not be able to spend +// that budget. func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodGet { + var params singleReportRequestBody + switch r.Method { + case http.MethodGet: + params = singleReportRequestFromQuery(r.URL.Query()) + case http.MethodPost: + body, ok := decodeReportingRequestBody[singleReportRequestBody](w, r) + if !ok { + return + } + params = body + default: + w.Header().Set("Allow", "GET, POST") http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) return } @@ -553,15 +635,14 @@ func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http. } definition := performanceReportDefinition() - q := r.URL.Query() - format, err := normalizePerformanceReportFormat(q.Get("format"), definition) + format, err := normalizePerformanceReportFormat(params.Format, definition) if err != nil { writeErrorResponse(w, http.StatusBadRequest, "invalid_format", err.Error(), nil) return } - resourceTypeRaw := q.Get("resourceType") - resourceID := q.Get("resourceId") + resourceTypeRaw := params.ResourceType + resourceID := params.ResourceID if resourceTypeRaw == "" || resourceID == "" { writeErrorResponse(w, http.StatusBadRequest, "missing_params", "resourceType and resourceId are required", nil) return @@ -584,8 +665,8 @@ func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http. metricType, title, err := normalizePerformanceReportOptionalFields( definition, - q.Get("metricType"), - q.Get("title"), + params.MetricType, + params.Title, ) if err != nil { code := "invalid_metric_type" @@ -599,8 +680,8 @@ func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http. start, end, err := normalizePerformanceReportTimeRange( definition, - q.Get("start"), - q.Get("end"), + params.Start, + params.End, time.Now(), ) if err != nil { @@ -628,9 +709,18 @@ func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http. // Wire the per-tenant AI narrator and Patrol findings provider when // configured. Both are nil-safe at the engine layer; absence falls // back to the heuristic narrator with no findings section. Single- - // resource reports do not use the FleetNarrator. - narrator, _, findings := h.resolveNarrator(r.Context()) - req.Narrator = narrator + // resource reports do not use the FleetNarrator. Only POST resolves the + // AI service; GET narrates deterministically and reads findings only + // from an AI service that is already running. + var narrator reporting.Narrator + var findings reporting.FindingsProvider + if r.Method == http.MethodPost { + narrator, _, findings = h.resolveNarrator(r.Context()) + req.Narrator = narrator + } else { + findings = h.resolveExistingFindings(r.Context()) + req.Narrator = getReportNarrator{} + } req.FindingsProvider = findings data, contentType, err := engine.Generate(req) @@ -647,6 +737,7 @@ func (h *ReportingHandlers) HandleGenerateReport(w http.ResponseWriter, r *http. log.Info(). Str("event", "reporting.single.generated"). Str("org_id", orgID). + Str("method", r.Method). Str("resource_type", resourceType). Str("metric_type", metricType). Str("format", string(format)). @@ -1187,7 +1278,7 @@ func (h *ReportingHandlers) HandleGenerateMultiReport(w http.ResponseWriter, r * http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) return } - body, ok := decodeMultiReportRequestBody(w, r) + body, ok := decodeReportingRequestBody[multiReportRequestBody](w, r) if !ok { return } diff --git a/internal/api/reporting_handlers_test.go b/internal/api/reporting_handlers_test.go index 00851e4a56..f5adb84e9d 100644 --- a/internal/api/reporting_handlers_test.go +++ b/internal/api/reporting_handlers_test.go @@ -75,7 +75,7 @@ func (p reportBrandLicenseProvider) Service(context.Context) *pkglicensing.Servi func TestReportingHandlers_MethodNotAllowed(t *testing.T) { handler := NewReportingHandlers(nil, nil) - req := httptest.NewRequest(http.MethodPost, "/api/reporting", nil) + req := httptest.NewRequest(http.MethodPut, "/api/reporting", nil) rr := httptest.NewRecorder() handler.HandleGenerateReport(rr, req) @@ -83,6 +83,47 @@ func TestReportingHandlers_MethodNotAllowed(t *testing.T) { if rr.Code != http.StatusMethodNotAllowed { t.Fatalf("expected status %d, got %d", http.StatusMethodNotAllowed, rr.Code) } + if got := rr.Header().Get("Allow"); got != "GET, POST" { + t.Fatalf("expected Allow GET, POST, got %q", got) + } +} + +func TestReportingHandlers_GenerateReportRejectsInvalidPOSTBody(t *testing.T) { + engine := &stubReportingEngine{data: []byte("report"), contentType: "application/pdf"} + original := reporting.GetEngine() + reporting.SetEngine(engine) + t.Cleanup(func() { reporting.SetEngine(original) }) + + handler := NewReportingHandlers(nil, nil) + for name, body := range map[string]string{ + "empty": ``, + "unknown field": `{"resourceType":"node","resourceId":"node-1","narrate":true}`, + "trailing value": `{"resourceType":"node","resourceId":"node-1"} {}`, + "query shape": `resourceType=node&resourceId=node-1`, + } { + req := httptest.NewRequest(http.MethodPost, "/api/reporting", strings.NewReader(body)) + rr := httptest.NewRecorder() + handler.HandleGenerateReport(rr, req) + if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "invalid_body") { + t.Fatalf("%s: expected 400 invalid_body, got %d body=%s", name, rr.Code, rr.Body.String()) + } + } + + req := httptest.NewRequest(http.MethodPost, "/api/reporting?resourceType=node&resourceId=node-1", strings.NewReader(`{}`)) + rr := httptest.NewRecorder() + handler.HandleGenerateReport(rr, req) + if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "missing_params") { + t.Fatalf("POST must read fields from the body, not the query: got %d body=%s", rr.Code, rr.Body.String()) + } + + // A valid value followed by padding past the cap is too large, not malformed. + padded := `{"resourceType":"node","resourceId":"node-1"}` + strings.Repeat(" ", reportingMultiReportBodyMax) + req = httptest.NewRequest(http.MethodPost, "/api/reporting", strings.NewReader(padded)) + rr = httptest.NewRecorder() + handler.HandleGenerateReport(rr, req) + if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "body_too_large") { + t.Fatalf("expected 400 body_too_large for an oversized body, got %d body=%s", rr.Code, rr.Body.String()) + } } func TestReportingHandlers_EngineUnavailable(t *testing.T) { diff --git a/internal/api/router.go b/internal/api/router.go index 6f77a32878..60dcd7343d 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -805,33 +805,7 @@ func (r *Router) setupRoutes() { // tenant services are refreshed when Router.SetMonitor replaces a monitor. r.configureMetadataProviderFactory() - // Wire the per-tenant AI narrator, fleet narrator, and Patrol - // findings provider into reporting. The AI service implements all - // three interfaces; when not configured for the tenant the engine - // falls back to the heuristic narrators with no findings section. - if r.reportingHandlers != nil { - settings := r.aiSettingsHandler - r.reportingHandlers.SetPatrolDigestResolver(func(ctx context.Context, days int) (ai.PatrolDigest, bool) { - if settings == nil { - return ai.PatrolDigest{}, false - } - return settings.BuildPatrolDigest(ctx, days) - }) - r.reportingHandlers.SetNarratorResolver(func(ctx context.Context) (reporting.Narrator, reporting.FleetNarrator, reporting.FindingsProvider) { - if settings == nil { - return nil, nil, nil - } - svc := settings.GetAIService(ctx) - if svc == nil { - return nil, nil, nil - } - cfg := svc.GetAIConfig() - if cfg == nil || !cfg.Enabled { - return nil, nil, nil - } - return svc, svc, svc - }) - } + r.wireReportingAIResolvers() // AI chat handler r.aiHandler = NewAIHandler(r.multiTenant, r.mtMonitor, r.agentExecServer) @@ -1031,6 +1005,50 @@ func (r *Router) setupRoutes() { // See issue #334 - ServeMux redirects empty path to "./" which breaks reverse proxies } +// wireReportingAIResolvers connects reporting to the per-tenant AI service. +func (r *Router) wireReportingAIResolvers() { + // Wire the per-tenant AI narrator, fleet narrator, and Patrol + // findings provider into reporting. The AI service implements all + // three interfaces; when not configured for the tenant the engine + // falls back to the heuristic narrators with no findings section. + if r.reportingHandlers != nil { + settings := r.aiSettingsHandler + r.reportingHandlers.SetPatrolDigestResolver(func(ctx context.Context, days int) (ai.PatrolDigest, bool) { + if settings == nil { + return ai.PatrolDigest{}, false + } + return settings.BuildPatrolDigest(ctx, days) + }) + r.reportingHandlers.SetNarratorResolver(func(ctx context.Context) (reporting.Narrator, reporting.FleetNarrator, reporting.FindingsProvider) { + if settings == nil { + return nil, nil, nil + } + svc := settings.GetAIService(ctx) + if svc == nil { + return nil, nil, nil + } + cfg := svc.GetAIConfig() + if cfg == nil || !cfg.Enabled { + return nil, nil, nil + } + return svc, svc, svc + }) + r.reportingHandlers.SetExistingFindingsResolver(func(ctx context.Context) reporting.FindingsProvider { + if settings == nil { + return nil + } + svc := settings.ExistingAIService(ctx) + if svc == nil { + return nil + } + if cfg := svc.GetAIConfig(); cfg == nil || !cfg.Enabled { + return nil + } + return svc + }) + } +} + // CleanupTenant removes all per-tenant resources (RBAC, AI, License) for a deleted org. func (r *Router) CleanupTenant(ctx context.Context, orgID string) error { orgID = strings.TrimSpace(orgID) diff --git a/pkg/reporting/catalog.go b/pkg/reporting/catalog.go index 3820d821c2..fcf287cc09 100644 --- a/pkg/reporting/catalog.go +++ b/pkg/reporting/catalog.go @@ -2,6 +2,7 @@ package reporting import ( "fmt" + "net/http" "strings" "time" ) @@ -33,6 +34,7 @@ type PerformanceReportDefinition struct { Title string `json:"title"` Description string `json:"description"` SingleResourceEndpoint string `json:"singleResourceEndpoint"` + SingleResourceMethod string `json:"singleResourceMethod"` MultiResourceEndpoint string `json:"multiResourceEndpoint"` SingleFilenamePrefix string `json:"singleFilenamePrefix"` SingleFilenameSubject string `json:"singleFilenameSubject"` @@ -150,6 +152,7 @@ func DescribePerformanceReport() PerformanceReportDefinition { Title: "Performance Reports", Description: "Generate PDF summaries or CSV metric exports from historical monitoring data for one or more selected resources.", SingleResourceEndpoint: "/api/admin/reports/generate", + SingleResourceMethod: http.MethodPost, MultiResourceEndpoint: "/api/admin/reports/generate-multi", SingleFilenamePrefix: "report", SingleFilenameSubject: FilenameSubjectResourceID, From 3a482c610c2212966290f8e2dea9fd015a9b1f27 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 21:28:01 +0100 Subject: [PATCH 7/7] Judge source merges by the ingest's stale thresholds mergeInto always passed nil thresholds to mergeMetrics, so every source merge into an existing row judged metric freshness by the registry defaults while the stale pass, the manual links and the monitor's configuration used the caller's thresholds. A Proxmox node polled every two minutes (four-minute threshold) merged with its auto-linked agent showed the silent agent's readings while the stale pass called the node's poll current, and a supplemental refresh of a linked vSphere VM kept the in-guest agent's memory over vSphere's current reading. Snapshot and record ingest now hold the caller's thresholds on the registry while they run, the way the guest loop holds its agent index, and mergeInto reads them. The ~50 snapshot helpers that reach mergeInto through ingest() keep their signatures. The stale pass and the metric merge now share one threshold rule, sourceStaleThreshold, so the merge also honours a sighting's expected update interval (slow Proxmox disk inventory) as the stale pass already did, and a source without a default falls back to 120s in both (the merge used 60s). Every defined source has a default, so that fallback reaches none. Outside mock mode the monitor configures only Proxmox, PBS and PMG, so those are the production sources this changes; mock mode also configures TrueNAS, vSphere and availability. --- .../v6/internal/subsystems/monitoring.md | 10 +- .../internal/subsystems/unified-resources.md | 24 ++- .../monitor_adapter_read_state_test.go | 147 ++++++++++++++++++ internal/unifiedresources/registry.go | 65 +++++--- .../registry_merge_policy_test.go | 108 +++++++++++++ 5 files changed, 323 insertions(+), 31 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index d7b496972e..4871c3d8c3 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -2679,10 +2679,12 @@ truthfulness, not native thaw, containing-release or workload acceptance. snapshot-bridge adapters stay read-only. The rebuild, the live supplemental refresh and the read-state overlay ingest records with the adapter's configured stale thresholds, because record ingest joins - operator links and a link's metric merge judges each side's freshness - by them (unified-resources contract, "Operator links reach - record-ingested resources"). Regression coverage: - `TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds` in + operator links and the freshness gate of every metric merge, a link's or + a source's into an existing row, reads them (unified-resources contract, + "Operator links reach record-ingested resources"). Regression + coverage: + `TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds` and + `TestMonitorAdapterSourceMergesUseConfiguredStaleThresholds` in `internal/unifiedresources/monitor_adapter_read_state_test.go` and `TestManualLinkToSupplementalGuestHoldsWithAndWithoutContinuity` in `internal/monitoring/issue1913_host_continuity_test.go`. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 95805c44a6..189b3ddc69 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -1609,7 +1609,12 @@ Source freshness is a separate unified-resource status input, not a fixed global health timer. Snapshot rebuilds, resource seeding, supplemental record overlays, and cloned read-state overlays must preserve the monitoring-provided stale thresholds for Proxmox, PBS, and PMG sources so resources do not flap to -warning/degraded between successful configured poll cycles. +warning/degraded between successful configured poll cycles. A source merge +into an existing row judges metric freshness by the ingest's thresholds and +the stale pass's own threshold rule, so the merge's freshness gate never +calls a source current that the stale pass marks stale, or the reverse. The +gate decides only where no stronger rule does: a hypervisor-managed guest +keeps its platform CPU even while that source is stale. Service-discovery readiness is a unified-resource payload contract, not a drawer-local decoration. Resource list/detail payloads that expose a @@ -6556,8 +6561,21 @@ separate. - The monitor adapter passes its configured stale thresholds to record ingest, so a link joined there judges each side's metrics the way a - snapshot-time link does. Ordinary source merges into an existing row still - use the default thresholds on every ingest path. + snapshot-time link does. Ordinary source merges into an existing row use + the same thresholds on every ingest path: snapshot and record ingest hold + the caller's thresholds while they run, and `mergeInto` judges each + metric's freshness with them through `sourceStaleThreshold`, the rule the + stale pass reads. Before this, those merges used the defaults, so a + Proxmox node polled every two minutes showed a silent auto-linked agent's + readings while the stale pass called the node's poll current, and a + supplemental refresh of a linked vSphere VM kept the agent's memory over + vSphere's current reading. Outside mock mode the monitor configures only + Proxmox, PBS and PMG; mock mode also configures TrueNAS, vSphere and + availability. Regression coverage: + `TestMonitorAdapterSourceMergesUseConfiguredStaleThresholds` (rebuild, + supplemental refresh, overlay, node with auto-linked agent), + `TestSourceMergesJudgeFreshnessByTheIngestStaleThresholds` and + `TestMetricMergeFreshnessMatchesTheStalePass`. - Continuity records are the one record ingest that never joins links. A saved enrollment only fills an absent machine, so it stays its own row instead of lending its linked guest an offline verdict and an old agent diff --git a/internal/unifiedresources/monitor_adapter_read_state_test.go b/internal/unifiedresources/monitor_adapter_read_state_test.go index dd7bd9da7b..55a8298d70 100644 --- a/internal/unifiedresources/monitor_adapter_read_state_test.go +++ b/internal/unifiedresources/monitor_adapter_read_state_test.go @@ -481,6 +481,153 @@ func TestMonitorAdapterJoinsLinkedRecordsWithConfiguredStaleThresholds(t *testin } } +// Every source merge into an existing row judges metric freshness by the +// adapter's configured thresholds, as the rebuild's links and stale pass do. +// A linked vSphere VM first reports no memory, so its in-guest agent supplies +// it; a refresh then brings vSphere memory observed three minutes ago, current +// under a five-minute threshold. The live supplemental refresh and the +// read-state overlay merge that record into the existing row, and must show +// vSphere's memory as a rebuild from the same records does. A Proxmox node +// polled every two minutes is likewise current at ninety seconds, so the +// rebuild keeps its readings over those of its auto-linked agent, silent for +// seventy-five seconds. +func TestMonitorAdapterSourceMergesUseConfiguredStaleThresholds(t *testing.T) { + now := time.Now().UTC() + thresholds := map[DataSource]time.Duration{SourceProxmox: 4 * time.Minute, SourceVMware: 5 * time.Minute} + + t.Run("linked vSphere VM refresh", func(t *testing.T) { + snapshot := models.StateSnapshot{ + Hosts: []models.Host{{ + ID: "host-app-guest", + Hostname: "app-guest", + MachineID: "machine-app-guest", + Status: "online", + LastSeen: now, + Memory: models.Memory{Total: 8 << 30, Used: 6 << 30, Free: 2 << 30, Usage: 75}, + }}, + LastUpdate: now, + } + vmRecord := func(lastSeen time.Time, metrics *ResourceMetrics) []IngestRecord { + return []IngestRecord{{ + SourceID: "vc-1:vm:vm-42", + Resource: Resource{ + Type: ResourceTypeVM, + Technology: "vmware", + Name: "app-guest", + Status: StatusOnline, + LastSeen: lastSeen, + Metrics: metrics, + VMware: &VMwareData{ConnectionID: "vc-1", ManagedObjectID: "vm-42", EntityType: "vm"}, + }, + }} + } + initial := vmRecord(now.Add(-4*time.Minute), &ResourceMetrics{CPU: &MetricValue{Percent: 10, Source: SourceVMware}}) + refreshedAt := now.Add(-3 * time.Minute) + refreshed := vmRecord(refreshedAt, &ResourceMetrics{ + CPU: &MetricValue{Percent: 10, Source: SourceVMware}, + Memory: &MetricValue{Percent: 40, Source: SourceVMware}, + }) + + unlinked := NewMonitorAdapter(NewRegistry(nil)) + unlinked.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: initial}) + vms, hosts := unlinked.VMs(), unlinked.Hosts() + if len(vms) != 1 || len(hosts) != 1 { + t.Fatalf("unlinked estate = %d VMs, %d agents, want one of each", len(vms), len(hosts)) + } + link := ResourceLink{ResourceA: vms[0].ID(), ResourceB: hosts[0].ID(), PrimaryID: hosts[0].ID()} + + for _, path := range []struct { + name string + read func(adapter *MonitorAdapter) ReadState + }{ + {"rebuild", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: refreshed}) + return adapter + }}, + {"supplemental", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: initial}) + adapter.PopulateSupplementalRecords(SourceVMware, refreshed) + return adapter + }}, + {"overlay", func(adapter *MonitorAdapter) ReadState { + adapter.PopulateSnapshotAndSupplemental(snapshot, map[DataSource][]IngestRecord{SourceVMware: initial}) + return ReadStateWithRecords(adapter, SourceVMware, refreshed) + }}, + } { + t.Run(path.name, func(t *testing.T) { + store := NewMemoryStore() + if err := store.AddLink(link); err != nil { + t.Fatalf("add link: %v", err) + } + readState := path.read(NewMonitorAdapterWithStaleThresholds(NewRegistry(store), thresholds)) + + vms := readState.VMs() + if len(vms) != 1 || vms[0].ID() != link.ResourceA || len(readState.Hosts()) != 0 { + t.Fatalf("estate = %d VMs, %d standalone agents, want only the linked VM %s", len(vms), len(readState.Hosts()), link.ResourceA) + } + if got := vms[0].r.SourceStatus[SourceVMware]; got.Status != "online" || !got.LastSeen.Equal(refreshedAt) { + t.Fatalf("vSphere sighting = %+v, want the refresh, current under the configured threshold", got) + } + if vms[0].r.Metrics == nil || vms[0].r.Metrics.Memory == nil { + t.Fatal("linked VM lost its memory reading") + } + if memory := vms[0].r.Metrics.Memory; memory.Source != SourceVMware || memory.Percent != 40 { + t.Fatalf("linked VM memory = %.0f%% from %s, want vSphere's current 40%%", memory.Percent, memory.Source) + } + }) + } + }) + + t.Run("node with auto-linked agent", func(t *testing.T) { + adapter := NewMonitorAdapterWithStaleThresholds(NewRegistry(nil), thresholds) + adapter.PopulateFromSnapshot(models.StateSnapshot{ + Nodes: []models.Node{{ + ID: "homelab-pve1", + Name: "pve1", + Instance: "homelab", + Status: "online", + LinkedAgentID: "host-pve1", + CPU: 0.12, + Memory: models.Memory{Total: 64 << 30, Used: 16 << 30, Free: 48 << 30, Usage: 25}, + LastSeen: now.Add(-90 * time.Second), + }}, + Hosts: []models.Host{{ + ID: "host-pve1", + MachineID: "machine-pve1", + Hostname: "pve1", + LinkedNodeID: "homelab-pve1", + Status: "online", + CPUUsage: 80, + Memory: models.Memory{Total: 64 << 30, Used: 48 << 30, Free: 16 << 30, Usage: 75}, + LastSeen: now.Add(-75 * time.Second), + IntervalSeconds: 30, + }}, + LastUpdate: now, + }) + + nodes := adapter.Nodes() + if len(nodes) != 1 { + t.Fatalf("nodes = %d, want the node merged with its agent", len(nodes)) + } + node := nodes[0].r + if poll := node.SourceStatus[SourceProxmox]; poll.Status != "online" || !poll.LastSeen.Equal(now.Add(-90*time.Second)) { + t.Fatalf("Proxmox sighting = %+v, want the current poll from ninety seconds ago", poll) + } + if got := node.SourceStatus[SourceAgent].Status; got != "stale" { + t.Fatalf("agent sighting = %q, want stale", got) + } + if node.Metrics == nil || node.Metrics.CPU == nil || node.Metrics.Memory == nil { + t.Fatalf("merged node lost its readings: %+v", node.Metrics) + } + if cpu := node.Metrics.CPU; cpu.Source != SourceProxmox || cpu.Percent != 12 { + t.Fatalf("node CPU = %.0f%% from %s, want the current poll's 12%%", cpu.Percent, cpu.Source) + } + if memory := node.Metrics.Memory; memory.Source != SourceProxmox || memory.Percent != 25 { + t.Fatalf("node memory = %.0f%% from %s, want the current poll's 25%%", memory.Percent, memory.Source) + } + }) +} + func TestHostContinuityCannotOverwriteCurrentResource(t *testing.T) { now := time.Now().UTC() for _, mode := range []string{"machine-identity", "source-identity", "future-timestamp", "provider-only"} { diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index c1b036344f..ac5891cacd 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -152,6 +152,13 @@ type ResourceRegistry struct { // what the full scan would see; scores stay live through the shared // pointers. agentNodeScanIndex map[string][]agentNodeCandidate + + // ingestStaleThresholds holds the caller-owned freshness thresholds of + // the snapshot or record ingest in progress, so each source merge into an + // existing row judges metric freshness as that ingest's manual links and + // stale pass do. nil outside an ingest and for callers without + // thresholds, where all three use the defaults. + ingestStaleThresholds map[DataSource]time.Duration } // agentNodeCandidate pairs a resources-map key with its entry so the indexed @@ -247,6 +254,10 @@ func (rr *ResourceRegistry) IngestSnapshotWithStaleThresholds(snapshot models.St } func (rr *ResourceRegistry) ingestSnapshot(snapshot models.StateSnapshot, thresholds map[DataSource]time.Duration) { + rr.mu.Lock() + rr.ingestStaleThresholds = thresholds + rr.mu.Unlock() + hostByID := make(map[string]*models.Host, len(snapshot.Hosts)) for i := range snapshot.Hosts { host := snapshot.Hosts[i] @@ -610,6 +621,7 @@ func (rr *ResourceRegistry) ingestSnapshot(snapshot models.StateSnapshot, thresh rr.refreshCanonicalIdentitiesLocked() rr.markStaleLocked(time.Now().UTC(), thresholds) rr.invalidateViewsLocked() + rr.ingestStaleThresholds = nil rr.mu.Unlock() } @@ -625,6 +637,10 @@ func (rr *ResourceRegistry) IngestRecordsWithStaleThresholds(source DataSource, } func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRecord, onlyMissing bool, thresholds map[DataSource]time.Duration) { + rr.mu.Lock() + rr.ingestStaleThresholds = thresholds + rr.mu.Unlock() + var successions []CanonicalIDSuccession supersededSeen := make(map[string]struct{}) for _, record := range records { @@ -677,6 +693,7 @@ func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRec rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.invalidateViewsLocked() + rr.ingestStaleThresholds = nil rr.mu.Unlock() } @@ -1805,26 +1822,15 @@ func sourceVerdictsRecorded(sightings map[DataSource]SourceStatus) bool { } func (rr *ResourceRegistry) markStaleLocked(now time.Time, thresholds map[DataSource]time.Duration) { - thresholds = effectiveStaleThresholds(thresholds) - changed := false for _, resource := range rr.resources { previousStatus := resource.Status staleFound := false for source, status := range resource.SourceStatus { - threshold, ok := thresholds[source] - if !ok { - threshold = 120 * time.Second - } - if status.ExpectedUpdateIntervalSeconds > 0 { - // Slow inventory polls have their own cadence. A source must miss - // two expected intervals before its retained observation is stale. - threshold = max(threshold, 2*time.Duration(status.ExpectedUpdateIntervalSeconds)*time.Second) - } if status.LastSeen.IsZero() { continue } - if now.Sub(status.LastSeen) > threshold { + if now.Sub(status.LastSeen) > sourceStaleThreshold(source, status, thresholds) { changed = changed || status.Status != "stale" status.Status = "stale" resource.SourceStatus[source] = status @@ -3973,7 +3979,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou existing.ParentID = rr.resolveCanonicalParentID(existing) existing.Status = chooseStatus(existing.Status, incoming.Status, source, existing.Sources) - existing.Metrics = mergeMetrics(existing, existing.Metrics, incoming.Metrics, source, now, existing.SourceStatus, nil) + existing.Metrics = mergeMetrics(existing, existing.Metrics, incoming.Metrics, source, now, existing.SourceStatus, rr.ingestStaleThresholds) existing.Metrics = clearUnavailableSourceMemoryMetric(existing.Metrics, &incoming, source) // Prefer agent naming when available @@ -6327,6 +6333,27 @@ func mergeMetrics( return &merged } +// sourceStaleThreshold is how long a source may go without delivering before +// its sighting is stale: the caller's threshold for the source, else the +// default. The stale pass and the metric merge's freshness gate both read it, +// so the gate never calls a source current that the pass marks stale, or the +// reverse. +func sourceStaleThreshold(source DataSource, sighting SourceStatus, thresholds map[DataSource]time.Duration) time.Duration { + threshold := thresholds[source] + if threshold <= 0 { + threshold = defaultStaleThresholds[source] + } + if threshold <= 0 { + threshold = 120 * time.Second + } + if sighting.ExpectedUpdateIntervalSeconds > 0 { + // Slow inventory polls have their own cadence. A source must miss + // two expected intervals before its retained observation is stale. + threshold = max(threshold, 2*time.Duration(sighting.ExpectedUpdateIntervalSeconds)*time.Second) + } + return threshold +} + // metricSourceStale reports whether a source's most recent report is older than // its stale threshold. A zero/unknown last-seen is treated as NOT stale so the // merge never demotes a source on missing information. @@ -6343,17 +6370,7 @@ func metricSourceStale( if !ok || st.LastSeen.IsZero() { return false } - threshold := time.Duration(0) - if configured := thresholds[source]; configured > 0 { - threshold = configured - } - if threshold <= 0 { - threshold = defaultStaleThresholds[source] - } - if threshold <= 0 { - threshold = 60 * time.Second - } - return now.Sub(st.LastSeen) > threshold + return now.Sub(st.LastSeen) > sourceStaleThreshold(source, st, thresholds) } func mergeMetric( diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index c133a0e533..6f04cb36b7 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -247,6 +247,114 @@ func TestLeaseExpiredAgentOnLiveProxmoxNodeStaysOnline(t *testing.T) { } } +// A source merge into an existing row judges metric freshness by the ingest's +// stale thresholds, as the stale pass that ends the ingest does. With PVE +// polled every two minutes the monitor allows Proxmox four minutes, so a node +// polled ninety seconds ago is current while its agent, silent for seventy-five +// seconds, is stale, and the row shows the node's readings. Without thresholds +// both sources are stale by the defaults and the agent keeps its precedence. +func TestSourceMergesJudgeFreshnessByTheIngestStaleThresholds(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + Nodes: []models.Node{{ + ID: "homelab-pve1", + Name: "pve1", + Instance: "homelab", + Status: "online", + LinkedAgentID: "host-pve1", + CPU: 0.12, + Memory: models.Memory{Total: 64 << 30, Used: 16 << 30, Free: 48 << 30, Usage: 25}, + LastSeen: now.Add(-90 * time.Second), + }}, + Hosts: []models.Host{{ + ID: "host-pve1", + MachineID: "machine-pve1", + Hostname: "pve1", + LinkedNodeID: "homelab-pve1", + Status: "online", + CPUUsage: 80, + Memory: models.Memory{Total: 64 << 30, Used: 48 << 30, Free: 16 << 30, Usage: 75}, + LastSeen: now.Add(-75 * time.Second), + IntervalSeconds: 30, + }}, + } + + for _, tc := range []struct { + name string + thresholds map[DataSource]time.Duration + proxmoxPoll string + want DataSource + wantCPU float64 + wantMemory float64 + }{ + {"configured", map[DataSource]time.Duration{SourceProxmox: 4 * time.Minute}, "online", SourceProxmox, 12, 25}, + {"defaults", nil, "stale", SourceAgent, 80, 75}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestSnapshotWithStaleThresholds(snapshot, tc.thresholds) + + resource := onlyResourceOfType(t, rr, ResourceTypeAgent) + if got := resource.SourceStatus[SourceProxmox].Status; got != tc.proxmoxPoll { + t.Fatalf("Proxmox sighting = %q, want %q: %+v", got, tc.proxmoxPoll, resource.SourceStatus) + } + if got := resource.SourceStatus[SourceAgent].Status; got != "stale" { + t.Fatalf("agent sighting = %q, want stale", got) + } + if resource.Metrics == nil || resource.Metrics.CPU == nil || resource.Metrics.Memory == nil { + t.Fatalf("merged row lost its readings: %+v", resource.Metrics) + } + if cpu := resource.Metrics.CPU; cpu.Source != tc.want || cpu.Percent != tc.wantCPU { + t.Fatalf("CPU = %.0f%% from %s, want %.0f%% from %s", cpu.Percent, cpu.Source, tc.wantCPU, tc.want) + } + if memory := resource.Metrics.Memory; memory.Source != tc.want || memory.Percent != tc.wantMemory { + t.Fatalf("memory = %.0f%% from %s, want %.0f%% from %s", memory.Percent, memory.Source, tc.wantMemory, tc.want) + } + }) + } +} + +// The metric merge and the stale pass read one threshold rule, so a source +// sighting the merge treats as current is never one the pass marks stale. +func TestMetricMergeFreshnessMatchesTheStalePass(t *testing.T) { + now := time.Now().UTC() + configured := map[DataSource]time.Duration{SourceProxmox: 4 * time.Minute, SourceVMware: 5 * time.Minute} + for _, tc := range []struct { + name string + source DataSource + sighting SourceStatus + thresholds map[DataSource]time.Duration + wantStale bool + }{ + {"proxmox by default", SourceProxmox, SourceStatus{LastSeen: now.Add(-90 * time.Second)}, nil, true}, + {"proxmox by configuration", SourceProxmox, SourceStatus{LastSeen: now.Add(-90 * time.Second)}, configured, false}, + {"vsphere by default", SourceVMware, SourceStatus{LastSeen: now.Add(-3 * time.Minute)}, nil, true}, + {"vsphere by configuration", SourceVMware, SourceStatus{LastSeen: now.Add(-3 * time.Minute)}, configured, false}, + {"agent without its own threshold", SourceAgent, SourceStatus{LastSeen: now.Add(-75 * time.Second)}, configured, true}, + {"slow disk inventory", SourceProxmox, SourceStatus{LastSeen: now.Add(-5 * time.Minute), ExpectedUpdateIntervalSeconds: 600}, nil, false}, + {"missed two disk inventory polls", SourceProxmox, SourceStatus{LastSeen: now.Add(-21 * time.Minute), ExpectedUpdateIntervalSeconds: 600}, nil, true}, + } { + t.Run(tc.name, func(t *testing.T) { + status := map[DataSource]SourceStatus{tc.source: tc.sighting} + if got := metricSourceStale(now, status, tc.source, tc.thresholds); got != tc.wantStale { + t.Fatalf("metric merge stale = %v, want %v", got, tc.wantStale) + } + + rr := NewRegistry(nil) + rr.resources["resource"] = &Resource{ + ID: "resource", + Status: StatusOnline, + Sources: []DataSource{tc.source}, + SourceStatus: map[DataSource]SourceStatus{tc.source: tc.sighting}, + } + rr.MarkStale(now, tc.thresholds) + if got := rr.resources["resource"].SourceStatus[tc.source].Status == "stale"; got != tc.wantStale { + t.Fatalf("stale pass stale = %v, want %v", got, tc.wantStale) + } + }) + } +} + // Docker hosts hold a shorter lease than the registry's Docker stale // threshold, so a silent Docker host was offline for a while and then // flipped to warning once its sighting went stale.