From 9316507df02dd4b39972fa60ffc9b4a1c800b458 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:48:49 +0100 Subject: [PATCH] Preserve Patrol finding volume beyond the history cap New-finding telemetry lost older activity once the run history reached its 100-entry cap. Persist a bounded daily finding tally with a separate upgrade cursor, preserving run counts while backfilling retained findings. Cover restart, repeated saves, upgrade, read failure and UTC-day retention. Record the measurement boundary and retire the resolved coverage gap. --- docs/release-control/v6/internal/status.json | 42 -------- .../internal/subsystems/security-privacy.md | 12 +++ internal/config/patrol_run_tally_test.go | 102 +++++++++++++++++- internal/config/persistence.go | 56 ++++++++-- pkg/server/telemetry_pulse_intelligence.go | 15 +-- .../telemetry_pulse_intelligence_test.go | 22 ++-- 6 files changed, 171 insertions(+), 78 deletions(-) diff --git a/docs/release-control/v6/internal/status.json b/docs/release-control/v6/internal/status.json index 99f0962d4..2e66b1fe2 100644 --- a/docs/release-control/v6/internal/status.json +++ b/docs/release-control/v6/internal/status.json @@ -10167,48 +10167,6 @@ } ] }, - { - "id": "patrol-investigation-rate-metric-invalid", - "summary": "The Patrol investigation rate (pulse_intelligence_patrol_investigations_30d over pulse_intelligence_patrol_new_findings_30d), which TELEMETRY_SIGNALS.md instructs every reader to report, is not a rate and must not be trended. Four independent reasons, all verified in pkg/server/telemetry_pulse_intelligence.go: the denominator sums run.NewFindings over history.Runs, which SavePatrolRunHistory caps at MaxPatrolRunHistory (100), so it covers at most the last hundred runs rather than thirty days and silently under-reports for any install patrolling faster than that (355 of 449 Patrol-enabled installs in the 6.4 upgrade cohort on 2026-09-03); the numerator instead scans the current findings store and counts surviving finding records investigated in-window, including findings created before it, which is how the paid cohort read 128.57 percent in the week to 2026-08-25; the populations are disjoint because Finding.ShouldInvestigate returns false at monitor autonomy and effective autonomy is licence-gated, so free installs produced 4384 findings and 1 investigation (0.02 percent) in the week to 2026-09-03 while 67 paid installs produced 242; and only 29 clean installs reported any investigation at all, one of which swung the fleet total by 38. The three-week decline that prompted this (7.2 to 5.0 percent) is composition, not regression: the rate is flat in version-stable installs (3.48, 3.39, 3.78), fleet investigations rose once that single install is excluded (170, 213, 225), finding-detection code is identical between v6.3.2 and v6.4.1, and the patrol interval is unchanged at 360 minutes across 6.1 through 6.4. The reported run growth is likewise the 63c40ebe5e daily tally back-filling, not more patrolling. This also corrects the telemetry inference in patrol-findings-hygiene-attention-noise: findings are not mostly 'seen and ignored', they are raised on installs that are licence-gated out of investigating at all, which leaves that gap's independently evidenced UX work (discussions 1623 and 1699) standing on its own footing. Two defects follow: new_findings_30d saturates on the very history cap that 63c40ebe5e fixed for runs_30d in the same function, needing a per-day findings tally alongside DailyRuns; and the blessed doc tells every reader to compute the invalid ratio. The real signal the ratio was hiding is that paid Patrol installs fell from 76 to 67 while free Patrol grew from 888 to 962.", - "owner": "project-owner", - "status": "triaged", - "recorded_at": "2026-09-03", - "lane_ids": [ - "L6" - ], - "subsystem_ids": [ - "patrol-intelligence" - ], - "proposed_resolution": "lane-expansion", - "coverage_impact": 2, - "evidence": [ - { - "repo": "pulse", - "path": "internal/ai/findings.go", - "kind": "file" - }, - { - "repo": "pulse", - "path": "internal/ai/patrol.go", - "kind": "file" - }, - { - "repo": "pulse", - "path": "pkg/server/telemetry_pulse_intelligence.go", - "kind": "file" - }, - { - "repo": "pulse", - "path": "pkg/server/telemetry_pulse_intelligence_test.go", - "kind": "file" - }, - { - "repo": "pulse-pro", - "path": "docs/TELEMETRY_SIGNALS.md", - "kind": "file" - } - ] - }, { "id": "telemetry-test-binary-production-pings", "summary": "Go test binaries reported themselves to the production telemetry receiver as live installations. pkg/server tests boot the real server through Run() with the version literal \"test-version\", which internal/updates normalizes to 0.0.0-test-version, and each test runs against its own t.TempDir(), so every run minted a fresh install ID. The startup ping waits two minutes and never fired in a short test, but the service-health failure reporter added on 2026-08-29 sends synchronously from a deferred handler as soon as Run() returns an error, so every test run that exercised a startup failure posted one ping. The receiver recorded 317 single-ping installs between 2026-08-29 and 2026-09-03, 311 from linux/amd64 hosts (dominated by the autonomous maintainer fleet running ad-hoc go test, not GitHub Actions, which ran twice in the final 24h) and 3 from a maintainer workstation. The canonical clean denominator excludes single-ping installs and was unaffected, but raw install counts and the operator-evidence Patrol blocked-cause read counted them as real installations. Resolved by refusing production-endpoint sends from a test binary in internal/telemetry, opting the server tests out of telemetry, and putting the operator-evidence read on the production-ping basis. The emitter's development flag was wrong too and is now fixed: it was set only for git build metadata or a prerelease of exactly dev or dev.*, so 0.0.0-test-version and 0.0.0-dev-pro both arrived with version_is_development clear. The 0.0.0 sentinel now decides the channel, so builds reporting after 2026-09-04 carry version_is_development true. Rows already stored keep the values they were sent with and are not backfilled, so any read whose window reaches earlier must still filter on version_is_published_release.", diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 102138e19..8bb09f60f 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -2474,6 +2474,18 @@ as investigated, one bucket per finding, and add no finding, resource, session, or action identity. The receiver canonicalizes every one of the four strings to its released vocabulary or `unknown` and clamps the counts like every other counter. +Patrol run and new-finding volumes use bounded local UTC-day tallies in +`internal/config/persistence.go`, preserving aggregate activity after the +operator-facing run history is trimmed. Finding volume has its own persistence +cursor so installations with an existing run tally backfill retained runs on +upgrade without recounting run volume. No run, finding, resource, provider, or +session identity is added to the tally or outbound ping. The rolling totals +include the whole UTC day containing the cutoff. Activity trimmed before the +finding tally existed cannot be recovered. This changes retention of existing +aggregate fields and does not expand the telemetry schema. Investigation +counts remain one per surviving finding investigated in-window, with one +current outcome bucket per finding. They do not count repeated attempts or +form a conversion denominator for newly detected findings. The earlier draft schema-v8 `business_estate` field was reverted and must not remain in the license server's accepted ping struct merely because a private receiver build and database migration briefly carried it. Existing deployed databases need no diff --git a/internal/config/patrol_run_tally_test.go b/internal/config/patrol_run_tally_test.go index fa8ce231f..c968a83bb 100644 --- a/internal/config/patrol_run_tally_test.go +++ b/internal/config/patrol_run_tally_test.go @@ -1,7 +1,9 @@ package config import ( + "encoding/json" "errors" + "os" "strings" "testing" "time" @@ -110,7 +112,7 @@ func TestPatrolRunTallyReadErrorWarnsAndKeepsSaving(t *testing.T) { // Seed history whose tally holds a run that has already fallen off the // capped run list: only a preserved tally would still count it. - runA := PatrolRunRecord{ID: "a", StartedAt: now.Add(-2 * time.Hour), CompletedAt: now.Add(-2 * time.Hour)} + runA := PatrolRunRecord{ID: "a", NewFindings: 2, StartedAt: now.Add(-2 * time.Hour), CompletedAt: now.Add(-2 * time.Hour)} if err := p.SavePatrolRunHistory([]PatrolRunRecord{runA}); err != nil { t.Fatalf("SavePatrolRunHistory seed: %v", err) } @@ -119,11 +121,11 @@ func TestPatrolRunTallyReadErrorWarnsAndKeepsSaving(t *testing.T) { mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")} p.SetFileSystem(mfs) - runB := PatrolRunRecord{ID: "b", StartedAt: now.Add(-1 * time.Hour), CompletedAt: now.Add(-1 * time.Hour)} + runB := PatrolRunRecord{ID: "b", NewFindings: 3, StartedAt: now.Add(-1 * time.Hour), CompletedAt: now.Add(-1 * time.Hour)} if err := p.SavePatrolRunHistory([]PatrolRunRecord{runB}); err != nil { t.Fatalf("SavePatrolRunHistory with failing read must still save: %v", err) } - if !strings.Contains(logs.String(), "daily run tally restarts") { + if !strings.Contains(logs.String(), "daily run and finding tallies restart") { t.Fatalf("no tally-restart warning was logged; logs: %s", logs.String()) } @@ -139,4 +141,98 @@ func TestPatrolRunTallyReadErrorWarnsAndKeepsSaving(t *testing.T) { if got := loaded.PatrolRunsSince(now.AddDate(0, 0, -30)); got != 1 { t.Fatalf("PatrolRunsSince = %d, want 1 after the tally restart", got) } + if got := loaded.PatrolNewFindingsSince(now.AddDate(0, 0, -30)); got != 3 { + t.Fatalf("PatrolNewFindingsSince = %d, want 3 after the tally restart", got) + } +} + +// Upgrading a persisted run-only tally must backfill retained findings even +// when every retained run is already behind the run tally's cursor. Once +// saved, that finding volume must survive restart, trimming and repeated saves. +func TestPatrolNewFindingsTallyUpgradeAndRestart(t *testing.T) { + dir := t.TempDir() + p := NewConfigPersistence(dir) + now := time.Now().UTC() + since := now.AddDate(0, 0, -30) + at := now.Add(-time.Hour) + legacy := PatrolRunHistoryData{ + Version: 1, + Runs: []PatrolRunRecord{ + {ID: "b", CompletedAt: at, NewFindings: 3}, + {ID: "a", StartedAt: at.Add(-time.Hour), NewFindings: 2}, + }, + DailyRuns: map[string]int{PatrolRunTallyDayKey(at): 150}, + RunTallyThrough: at, + } + encoded, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p.aiPatrolRunsFile, encoded, 0600); err != nil { + t.Fatal(err) + } + loaded, err := p.LoadPatrolRunHistory() + if err != nil { + t.Fatal(err) + } + if got := loaded.PatrolNewFindingsSince(since); got != 5 { + t.Fatalf("legacy history fallback = %d, want 5", got) + } + if err := p.SavePatrolRunHistory(loaded.Runs); err != nil { + t.Fatal(err) + } + + // A new persistence instance must carry both tallies from disk after the + // original source records are dropped. Zero-finding runs advance too. + p = NewConfigPersistence(dir) + next := []PatrolRunRecord{ + {ID: "d", CompletedAt: now, NewFindings: 0}, + {ID: "c", CompletedAt: now.Add(-time.Minute), NewFindings: 4}, + } + for i := 0; i < 3; i++ { + if err := p.SavePatrolRunHistory(next); err != nil { + t.Fatal(err) + } + } + loaded, err = p.LoadPatrolRunHistory() + if err != nil { + t.Fatal(err) + } + if got := loaded.PatrolRunsSince(since); got != 152 { + t.Fatalf("run tally after upgrade = %d, want 152", got) + } + if got := loaded.PatrolNewFindingsSince(since); got != 9 { + t.Fatalf("finding tally after restart and repeated saves = %d, want 9", got) + } + if !loaded.NewFindingsTallyThrough.Equal(now) { + t.Fatalf("finding cursor = %s, want %s", loaded.NewFindingsTallyThrough, now) + } +} + +func TestPatrolNewFindingsTallyWindowAndRetention(t *testing.T) { + now := time.Date(2026, 9, 5, 12, 0, 0, 0, time.UTC) + since := now.AddDate(0, 0, -30) + runs := []PatrolRunRecord{ + {CompletedAt: since.Add(-time.Hour), NewFindings: 2}, + {StartedAt: since, NewFindings: 3}, + {CompletedAt: now.AddDate(0, 0, -32), NewFindings: 100}, + {CompletedAt: now, NewFindings: -1}, + {NewFindings: 100}, + } + data := &PatrolRunHistoryData{Runs: runs} + if got := data.PatrolNewFindingsSince(since); got != 3 { + t.Fatalf("history fallback = %d, want 3", got) + } + advancePatrolRunTally(data, runs, now) + data.Runs = nil + if got := data.PatrolNewFindingsSince(since); got != 5 { + t.Fatalf("UTC-day tally = %d, want 5 including cutoff day", got) + } + if len(data.DailyNewFindings) != 1 { + t.Fatalf("tally contains expired or invalid findings: %v", data.DailyNewFindings) + } + var absent *PatrolRunHistoryData + if got := absent.PatrolNewFindingsSince(since); got != 0 { + t.Fatalf("nil history = %d, want 0", got) + } } diff --git a/internal/config/persistence.go b/internal/config/persistence.go index 0060ac981..cdbf221d8 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -2783,6 +2783,11 @@ type PatrolRunHistoryData struct { // cannot answer "how often did Patrol actually run". This tally is the // uncapped answer and costs one small integer per retained day. DailyRuns map[string]int `json:"daily_runs,omitempty"` + // DailyNewFindings retains finding volume after its source runs leave the + // operator-facing history. Its separate cursor backfills retained history + // when upgrading an installation that already has a run tally. + DailyNewFindings map[string]int `json:"daily_new_findings,omitempty"` + NewFindingsTallyThrough time.Time `json:"new_findings_tally_through,omitempty"` // RunTallyThrough is the newest run observation already folded into // DailyRuns. History is persisted as a full newest-first list on every // debounced save, so the tally advances from this high-water mark rather @@ -2819,18 +2824,31 @@ func advancePatrolRunTally(data *PatrolRunHistoryData, runs []PatrolRunRecord, n if data.DailyRuns == nil { data.DailyRuns = make(map[string]int, patrolRunTallyRetentionDays) } + if data.DailyNewFindings == nil { + data.DailyNewFindings = make(map[string]int, patrolRunTallyRetentionDays) + } newest := data.RunTallyThrough + newestFindings := data.NewFindingsTallyThrough for _, run := range runs { observedAt := patrolRunObservedAt(run) - if observedAt.IsZero() || !observedAt.After(data.RunTallyThrough) { + if observedAt.IsZero() { continue } - data.DailyRuns[PatrolRunTallyDayKey(observedAt)]++ + if observedAt.After(data.RunTallyThrough) { + data.DailyRuns[PatrolRunTallyDayKey(observedAt)]++ + } + if observedAt.After(data.NewFindingsTallyThrough) && run.NewFindings > 0 { + data.DailyNewFindings[PatrolRunTallyDayKey(observedAt)] += run.NewFindings + } if observedAt.After(newest) { newest = observedAt } + if observedAt.After(newestFindings) { + newestFindings = observedAt + } } data.RunTallyThrough = newest + data.NewFindingsTallyThrough = newestFindings cutoff := PatrolRunTallyDayKey(now.UTC().AddDate(0, 0, -patrolRunTallyRetentionDays)) for day := range data.DailyRuns { @@ -2838,6 +2856,11 @@ func advancePatrolRunTally(data *PatrolRunHistoryData, runs []PatrolRunRecord, n delete(data.DailyRuns, day) } } + for day := range data.DailyNewFindings { + if day < cutoff { + delete(data.DailyNewFindings, day) + } + } } // PatrolRunsSince counts patrol runs observed at or after since. It prefers the @@ -2849,17 +2872,33 @@ func (data *PatrolRunHistoryData) PatrolRunsSince(since time.Time) int { if data == nil { return 0 } + return data.patrolActivitySince(since, data.DailyRuns, func(PatrolRunRecord) int { return 1 }) +} + +// PatrolNewFindingsSince has the same UTC-day window and retained-history +// fallback as PatrolRunsSince. It cannot recover findings from runs that were +// trimmed before the first finding tally was saved. +func (data *PatrolRunHistoryData) PatrolNewFindingsSince(since time.Time) int { + if data == nil { + return 0 + } + return data.patrolActivitySince(since, data.DailyNewFindings, func(run PatrolRunRecord) int { + return max(0, run.NewFindings) + }) +} + +func (data *PatrolRunHistoryData) patrolActivitySince(since time.Time, daily map[string]int, countRun func(PatrolRunRecord) int) int { fromHistory := 0 for _, run := range data.Runs { observedAt := patrolRunObservedAt(run) if observedAt.IsZero() || observedAt.Before(since) { continue } - fromHistory++ + fromHistory += countRun(run) } fromTally := 0 sinceDay := PatrolRunTallyDayKey(since) - for day, count := range data.DailyRuns { + for day, count := range daily { if day >= sinceDay { fromTally += count } @@ -3697,7 +3736,7 @@ func (c *ConfigPersistence) LoadWorkflowPromptActivityHistory() (*WorkflowPrompt } // SavePatrolRunHistory persists patrol run history to disk, carrying the -// uncapped daily run tally forward across the capped run list it replaces. +// daily run and finding tallies forward across the capped run list it replaces. func (c *ConfigPersistence) SavePatrolRunHistory(runs []PatrolRunRecord) error { now := time.Now() data := PatrolRunHistoryData{ @@ -3709,10 +3748,12 @@ func (c *ConfigPersistence) SavePatrolRunHistory(runs []PatrolRunRecord) error { // a real read failure. The tally is telemetry, so keep the save — but say // the tally is restarting rather than resetting it silently. if existing, err := c.LoadPatrolRunHistory(); err != nil { - log.Warn().Err(err).Str("file", c.aiPatrolRunsFile).Msg("Failed to read existing patrol run history; daily run tally restarts from this save") + log.Warn().Err(err).Str("file", c.aiPatrolRunsFile).Msg("Failed to read existing patrol run history; daily run and finding tallies restart from this save") } else if existing != nil { data.DailyRuns = existing.DailyRuns data.RunTallyThrough = existing.RunTallyThrough + data.DailyNewFindings = existing.DailyNewFindings + data.NewFindingsTallyThrough = existing.NewFindingsTallyThrough } advancePatrolRunTally(&data, runs, now) return saveHistoryData(c, c.aiPatrolRunsFile, data, len(runs), "patrol run history", "Patrol run history") @@ -3740,6 +3781,9 @@ func (c *ConfigPersistence) LoadPatrolRunHistory() (*PatrolRunHistoryData, error if data.DailyRuns == nil { data.DailyRuns = make(map[string]int) } + if data.DailyNewFindings == nil { + data.DailyNewFindings = make(map[string]int) + } }, func(data *PatrolRunHistoryData) error { jsonData, err := json.Marshal(data) diff --git a/pkg/server/telemetry_pulse_intelligence.go b/pkg/server/telemetry_pulse_intelligence.go index dc5f5e860..d0caae807 100644 --- a/pkg/server/telemetry_pulse_intelligence.go +++ b/pkg/server/telemetry_pulse_intelligence.go @@ -244,23 +244,12 @@ func applyPulseIntelligencePatrolRunSnapshot(snap *telemetry.Snapshot, persisten if err != nil || history == nil { return } - // Run volume comes from the uncapped daily tally. Counting history.Runs + // Run and finding volumes come from daily tallies. Counting history.Runs // directly saturates at the operator-facing history cap, which on an install // patrolling on a normal schedule is reached within hours of a thirty-day // window rather than at its end. snap.PulseIntelligencePatrolRuns30d += history.PatrolRunsSince(since) - for _, run := range history.Runs { - observedAt := run.CompletedAt - if observedAt.IsZero() { - observedAt = run.StartedAt - } - if observedAt.IsZero() || observedAt.Before(since) { - continue - } - if run.NewFindings > 0 { - snap.PulseIntelligencePatrolNewFindings30d += run.NewFindings - } - } + snap.PulseIntelligencePatrolNewFindings30d += history.PatrolNewFindingsSince(since) } func applyPulseIntelligenceFindingSnapshot(snap *telemetry.Snapshot, persistence *config.ConfigPersistence, since time.Time) { diff --git a/pkg/server/telemetry_pulse_intelligence_test.go b/pkg/server/telemetry_pulse_intelligence_test.go index bc5c48b9d..7e44a898f 100644 --- a/pkg/server/telemetry_pulse_intelligence_test.go +++ b/pkg/server/telemetry_pulse_intelligence_test.go @@ -1123,33 +1123,27 @@ func TestApplyPulseIntelligencePatrolRunSnapshotIgnoresHistoryCap(t *testing.T) } } -// PulseIntelligencePatrolNewFindings30d is summed over the retained run list, -// which SavePatrolRunHistory caps at MaxPatrolRunHistory, while -// PulseIntelligencePatrolRuns30d rides the uncapped daily tally that -// TestApplyPulseIntelligencePatrolRunSnapshotIgnoresHistoryCap pins. An -// install patrolling faster than the cap therefore reports every run but only -// the findings from the most recent hundred, so the two counters cover -// different spans and no ratio between findings and any other counter is a -// rate. Pinned so the asymmetry stays a known bound rather than a silent one; -// see coverage gap patrol-investigation-rate-metric-invalid. -func TestApplyPulseIntelligencePatrolRunSnapshotTruncatesNewFindingsAtHistoryCap(t *testing.T) { +// Both activity volumes must survive trimming the operator-facing history. +func TestApplyPulseIntelligencePatrolRunSnapshotPreservesNewFindingsBeyondHistoryCap(t *testing.T) { persistence := config.NewConfigPersistence(t.TempDir()) now := time.Now().UTC() since := now.AddDate(0, 0, -30) const historyCap = 100 total := 0 + totalFindings := 0 runs := make([]config.PatrolRunRecord, 0, historyCap) for at := now.AddDate(0, 0, -2); at.Before(now); at = at.Add(10 * time.Minute) { runs = append([]config.PatrolRunRecord{{ ID: at.Format(time.RFC3339Nano), StartedAt: at, CompletedAt: at, - NewFindings: 1, + NewFindings: total % 4, }}, runs...) if len(runs) > historyCap { runs = runs[:historyCap] } + totalFindings += total % 4 total++ if err := persistence.SavePatrolRunHistory(runs); err != nil { t.Fatalf("SavePatrolRunHistory: %v", err) @@ -1164,9 +1158,9 @@ func TestApplyPulseIntelligencePatrolRunSnapshotTruncatesNewFindingsAtHistoryCap if snap.PulseIntelligencePatrolRuns30d != total { t.Fatalf("PulseIntelligencePatrolRuns30d = %d, want %d", snap.PulseIntelligencePatrolRuns30d, total) } - if snap.PulseIntelligencePatrolNewFindings30d != historyCap { - t.Fatalf("PulseIntelligencePatrolNewFindings30d = %d, want %d (one per retained run, not one per run in the window)", - snap.PulseIntelligencePatrolNewFindings30d, historyCap) + if snap.PulseIntelligencePatrolNewFindings30d != totalFindings { + t.Fatalf("PulseIntelligencePatrolNewFindings30d = %d, want %d across all persisted runs", + snap.PulseIntelligencePatrolNewFindings30d, totalFindings) } }