diff --git a/changes/51426-software-title-install-history-timeout b/changes/51426-software-title-install-history-timeout new file mode 100644 index 00000000000..2b08aaf683b --- /dev/null +++ b/changes/51426-software-title-install-history-timeout @@ -0,0 +1 @@ +- Fixed software title details pages and the hosts list software status filter timing out for software with a large install history. diff --git a/server/datastore/mysql/in_house_apps.go b/server/datastore/mysql/in_house_apps.go index 3205b1ff290..a1813f8b1db 100644 --- a/server/datastore/mysql/in_house_apps.go +++ b/server/datastore/mysql/in_house_apps.go @@ -471,11 +471,11 @@ upcoming AS ( -- NOTE if you change this logic make sure to change inHouseAppHostStatusNamedQuery accordingly past AS ( SELECT - hihsi.host_id, + ranked.host_id, CASE - WHEN hihsi.verification_at IS NOT NULL THEN + WHEN ranked.verification_at IS NOT NULL THEN :software_status_installed - WHEN hihsi.verification_failed_at IS NOT NULL THEN + WHEN ranked.verification_failed_at IS NOT NULL THEN :software_status_failed WHEN ncr.status = :mdm_status_error OR ncr.status = :mdm_status_format_error THEN :software_status_failed @@ -484,28 +484,35 @@ past AS ( ELSE NULL -- either pending or not installed via in-house App END AS status - FROM - host_in_house_software_installs hihsi - JOIN hosts h ON host_id = h.id - -- LEFT JOIN so Fleet-side pre-flight failures (unresolvable - -- managed-config Fleet variable) survive — those never enqueue an MDM - -- command, so no ncr row exists. The CASE above maps - -- verification_failed_at IS NOT NULL to failed before any ncr.status - -- branch is evaluated. - LEFT JOIN nano_command_results ncr ON ncr.id = h.uuid AND ncr.command_uuid = hihsi.command_uuid - LEFT JOIN host_in_house_software_installs hihsi2 - ON hihsi.host_id = hihsi2.host_id AND - hihsi.in_house_app_id = hihsi2.in_house_app_id AND - hihsi2.removed = 0 AND - hihsi2.canceled = 0 AND - (hihsi.created_at < hihsi2.created_at OR (hihsi.created_at = hihsi2.created_at AND hihsi.id < hihsi2.id)) + FROM ( + SELECT + hihsi.host_id, + hihsi.command_uuid, + hihsi.verification_at, + hihsi.verification_failed_at, + h.uuid AS host_uuid, + ROW_NUMBER() OVER ( + PARTITION BY hihsi.host_id + ORDER BY hihsi.created_at DESC, hihsi.id DESC + ) AS rn + FROM + host_in_house_software_installs hihsi + JOIN hosts h ON hihsi.host_id = h.id + WHERE + hihsi.in_house_app_id = :in_house_app_id + AND (h.team_id = :team_id OR (h.team_id IS NULL AND :team_id = 0)) + AND hihsi.removed = 0 + AND hihsi.canceled = 0 + ) ranked + -- LEFT JOIN so Fleet-side pre-flight failures (unresolvable + -- managed-config Fleet variable) survive — those never enqueue an MDM + -- command, so no ncr row exists. The CASE above maps + -- verification_failed_at IS NOT NULL to failed before any ncr.status + -- branch is evaluated. + LEFT JOIN nano_command_results ncr ON ncr.id = ranked.host_uuid AND ncr.command_uuid = ranked.command_uuid WHERE - hihsi2.id IS NULL - AND hihsi.in_house_app_id = :in_house_app_id - AND (h.team_id = :team_id OR (h.team_id IS NULL AND :team_id = 0)) - AND hihsi.host_id NOT IN (SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities - AND hihsi.removed = 0 - AND hihsi.canceled = 0 + ranked.rn = 1 + AND ranked.host_id NOT IN (SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities ) -- count each status diff --git a/server/datastore/mysql/software_installers.go b/server/datastore/mysql/software_installers.go index 7fffc686741..013236b7496 100644 --- a/server/datastore/mysql/software_installers.go +++ b/server/datastore/mysql/software_installers.go @@ -2411,25 +2411,28 @@ upcoming AS ( -- select most recent past activities for each host past AS ( SELECT - hsi.host_id, - hsi.status - FROM - host_software_installs hsi - JOIN hosts h ON host_id = h.id - LEFT JOIN host_software_installs hsi2 - ON hsi.host_id = hsi2.host_id AND - hsi.software_installer_id = hsi2.software_installer_id AND - hsi2.removed = 0 AND - hsi2.canceled = 0 AND - hsi2.host_deleted_at IS NULL AND - (hsi.created_at < hsi2.created_at OR (hsi.created_at = hsi2.created_at AND hsi.id < hsi2.id)) + ranked.host_id, + ranked.status + FROM ( + SELECT + hsi.host_id, + hsi.status, + ROW_NUMBER() OVER ( + PARTITION BY hsi.host_id + ORDER BY hsi.created_at DESC, hsi.id DESC + ) AS rn + FROM + host_software_installs hsi + WHERE + hsi.software_installer_id = :installer_id + AND hsi.host_deleted_at IS NULL + AND hsi.removed = 0 + AND hsi.canceled = 0 + ) ranked + JOIN hosts h ON h.id = ranked.host_id WHERE - hsi2.id IS NULL - AND hsi.software_installer_id = :installer_id - AND hsi.host_id NOT IN(SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities - AND hsi.host_deleted_at IS NULL - AND hsi.removed = 0 - AND hsi.canceled = 0 + ranked.rn = 1 + AND ranked.host_id NOT IN(SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities ) -- count each status @@ -2525,43 +2528,53 @@ FROM ( // NOTE(mna): the pre-unified queue version of this query did not check for // removed = 0, so I am porting the same behavior (there's even a test that // fails if I add removed = 0 condition). + // Rank once over the app rather than looking the latest row up per host: + // host_vpp_software_installs has no host_id-leading index, so a correlated + // per-host lookup rescans the app's whole history for every candidate host. stmt := fmt.Sprintf(`JOIN ( SELECT - hvsi.host_id -FROM - host_vpp_software_installs hvsi - LEFT JOIN - nano_command_results ncr ON ncr.command_uuid = hvsi.command_uuid - LEFT JOIN host_vpp_software_installs hvsi2 - ON hvsi.host_id = hvsi2.host_id AND - hvsi.adam_id = hvsi2.adam_id AND - hvsi.platform = hvsi2.platform AND - hvsi2.canceled = 0 AND - (hvsi.created_at < hvsi2.created_at OR (hvsi.created_at = hvsi2.created_at AND hvsi.id < hvsi2.id)) + ranked.host_id +FROM ( + SELECT + hvsi.host_id, + hvsi.command_uuid, + hvsi.verification_at, + hvsi.verification_failed_at, + ROW_NUMBER() OVER ( + PARTITION BY hvsi.host_id + ORDER BY hvsi.created_at DESC, hvsi.id DESC + ) AS rn + FROM + host_vpp_software_installs hvsi + WHERE + hvsi.adam_id = :adam_id + AND hvsi.platform = :platform + AND hvsi.canceled = 0 +) ranked +LEFT JOIN + nano_command_results ncr ON ncr.command_uuid = ranked.command_uuid WHERE - hvsi2.id IS NULL - AND hvsi.adam_id = :adam_id - AND hvsi.platform = :platform - AND hvsi.canceled = 0 + ranked.rn = 1 -- Allow rows with no nano_command_results — Fleet-side pre-flight failures -- (unresolvable managed-config Fleet variable) record only the install row -- with verification_failed_at set, no MDM command. See same comment in - -- vpp.go GetSummaryHostVPPAppInstalls. - AND (ncr.id IS NOT NULL OR hvsi.verification_failed_at IS NOT NULL OR (:platform = 'android' AND ncr.id IS NULL)) + -- vpp.go GetSummaryHostVPPAppInstalls. Checked after ranking, so a host whose + -- most recent row lacks a command result is dropped rather than falling back. + AND (ncr.id IS NOT NULL OR ranked.verification_failed_at IS NOT NULL OR (:platform = 'android' AND ncr.id IS NULL)) AND (%s) = :status - AND NOT EXISTS ( - SELECT 1 + AND ranked.host_id NOT IN ( + SELECT + ua.host_id FROM upcoming_activities ua JOIN vpp_app_upcoming_activities vaua ON ua.id = vaua.upcoming_activity_id WHERE - ua.host_id = hvsi.host_id - AND vaua.adam_id = hvsi.adam_id - AND vaua.platform = hvsi.platform + vaua.adam_id = :adam_id + AND vaua.platform = :platform AND ua.activity_type = 'vpp_app_install' ) ) hss ON hss.host_id = h.id -`, vppAppHostStatusNamedQuery("hvsi", "ncr", "")) +`, vppAppHostStatusNamedQuery("ranked", "ncr", "")) return sqlx.Named(stmt, map[string]interface{}{ "status": status, @@ -2602,37 +2615,47 @@ WHERE } // for non-pending statuses, we'll join through host_software_installs filtered by the status - statusFilter := "hsi.status = :status" + // Rank once over the title rather than looking the latest row up per host. A + // per-host lookup forces `hosts` to drive, so on a large team it runs once per + // host in that team and reads each one's history across every title — unbounded + // by this title, and unindexed, since there is no (host_id, software_title_id). + statusFilter := "ranked.status = :status" if status == fleet.SoftwareFailed { // failed is a special case, we must include both install and uninstall failures - statusFilter = "hsi.status IN (:installFailed, :uninstallFailed)" + statusFilter = "ranked.status IN (:installFailed, :uninstallFailed)" } stmt := fmt.Sprintf(`JOIN ( SELECT - hsi.host_id -FROM - host_software_installs hsi - LEFT JOIN host_software_installs hsi2 - ON hsi.host_id = hsi2.host_id AND - hsi.software_title_id = hsi2.software_title_id AND - hsi2.removed = 0 AND - hsi2.canceled = 0 AND - (hsi.created_at < hsi2.created_at OR (hsi.created_at = hsi2.created_at AND hsi.id < hsi2.id)) + ranked.host_id +FROM ( + SELECT + hsi.host_id, + hsi.status, + ROW_NUMBER() OVER ( + PARTITION BY hsi.host_id + ORDER BY hsi.created_at DESC, hsi.id DESC + ) AS rn + FROM + host_software_installs hsi + WHERE + hsi.software_title_id = :title_id + AND hsi.removed = 0 + AND hsi.canceled = 0 +) ranked WHERE - hsi2.id IS NULL - AND hsi.software_title_id = :title_id - AND hsi.removed = 0 - AND hsi.canceled = 0 + ranked.rn = 1 + -- Status and the queue check are applied after ranking because they take no + -- part in choosing the host's most recent row. Folding them in picks another row. AND %s - AND NOT EXISTS ( - SELECT 1 + AND ranked.host_id NOT IN ( + SELECT + ua.host_id FROM upcoming_activities ua JOIN software_install_upcoming_activities siua ON ua.id = siua.upcoming_activity_id WHERE - ua.host_id = hsi.host_id - AND siua.software_title_id = hsi.software_title_id + siua.software_title_id = :title_id AND ua.activity_type = 'software_install' ) ) hss ON hss.host_id = h.id @@ -2681,42 +2704,51 @@ WHERE status = fleet.SoftwareInstallFailed // TODO: When in-house supports uninstall this should become STATUS IN ('failed_install', 'failed_uninstall') } + // Rank once over the app rather than looking the latest row up per host: + // host_in_house_software_installs has no host_id-leading index, so a correlated + // per-host lookup rescans the app's whole history for every candidate host. stmt := fmt.Sprintf(`JOIN ( SELECT - hihsi.host_id -FROM - host_in_house_software_installs hihsi - -- LEFT JOIN so Fleet-side pre-flight failures (unresolvable managed-config - -- Fleet variable) survive — those never enqueue an MDM command, so no ncr - -- row exists. The inHouseAppHostStatusNamedQuery CASE maps - -- verification_failed_at IS NOT NULL to failed before any ncr.status branch - -- is evaluated. - LEFT JOIN - nano_command_results ncr ON ncr.command_uuid = hihsi.command_uuid - LEFT JOIN host_in_house_software_installs hihsi2 - ON hihsi.host_id = hihsi2.host_id AND - hihsi.in_house_app_id = hihsi2.in_house_app_id AND - hihsi2.canceled = 0 AND - hihsi2.removed = 0 AND - (hihsi.created_at < hihsi2.created_at OR (hihsi.created_at = hihsi2.created_at AND hihsi.id < hihsi2.id)) + ranked.host_id +FROM ( + SELECT + hihsi.host_id, + hihsi.command_uuid, + hihsi.verification_at, + hihsi.verification_failed_at, + ROW_NUMBER() OVER ( + PARTITION BY hihsi.host_id + ORDER BY hihsi.created_at DESC, hihsi.id DESC + ) AS rn + FROM + host_in_house_software_installs hihsi + WHERE + hihsi.in_house_app_id = :in_house_app_id + AND hihsi.canceled = 0 + AND hihsi.removed = 0 +) ranked +-- LEFT JOIN so Fleet-side pre-flight failures (unresolvable managed-config +-- Fleet variable) survive — those never enqueue an MDM command, so no ncr +-- row exists. The inHouseAppHostStatusNamedQuery CASE maps +-- verification_failed_at IS NOT NULL to failed before any ncr.status branch +-- is evaluated. +LEFT JOIN + nano_command_results ncr ON ncr.command_uuid = ranked.command_uuid WHERE - hihsi2.id IS NULL - AND hihsi.in_house_app_id = :in_house_app_id - AND hihsi.canceled = 0 - AND hihsi.removed = 0 + ranked.rn = 1 AND (%s) = :status - AND NOT EXISTS ( - SELECT 1 + AND ranked.host_id NOT IN ( + SELECT + ua.host_id FROM upcoming_activities ua JOIN in_house_app_upcoming_activities ihua ON ua.id = ihua.upcoming_activity_id WHERE - ua.host_id = hihsi.host_id - AND ihua.in_house_app_id = hihsi.in_house_app_id + ihua.in_house_app_id = :in_house_app_id AND ua.activity_type = 'in_house_app_install' ) ) hss ON hss.host_id = h.id -`, inHouseAppHostStatusNamedQuery("hihsi", "ncr", "")) +`, inHouseAppHostStatusNamedQuery("ranked", "ncr", "")) return sqlx.Named(stmt, map[string]any{ "status": status, diff --git a/server/datastore/mysql/software_installers_test.go b/server/datastore/mysql/software_installers_test.go index 22a2c5c69ce..d5a1e352299 100644 --- a/server/datastore/mysql/software_installers_test.go +++ b/server/datastore/mysql/software_installers_test.go @@ -81,6 +81,8 @@ func TestSoftwareInstallers(t *testing.T) { {"SetHostSoftwareInstallResultResolvesOrphanedActivity", testSetHostSoftwareInstallResultResolvesOrphanedActivity}, {"GetSoftwareTitlesForInstallAll", testGetSoftwareTitlesForInstallAll}, {"SummaryUpcomingPerHostNoDropout", testSummaryUpcomingPerHostNoDropout}, + {"InstallStatusUsesLatestRowPerHost", testInstallStatusUsesLatestRowPerHost}, + {"InstallStatusDoesNotScanHistoryPerHostRow", testInstallStatusDoesNotScanHistoryPerHostRow}, {"GetSoftwareInstallDetailsCustomHostVitals", testGetSoftwareInstallDetailsCustomHostVitals}, } @@ -7631,3 +7633,453 @@ func testHasFMAInstallerVersion(t *testing.T, ds *Datastore) { require.False(t, versionExists) require.Empty(t, storageID) } + +// installStatusFixture seeds one installer on a team plus a helper that appends +// install rows with full control over ordering and flags. +type installStatusFixture struct { + teamID uint + installerID uint + titleID uint + addInstall func(hostID uint, createdAt time.Time, exitCode *int, removed, canceled, hostDeleted bool) +} + +func newInstallStatusFixture(t *testing.T, ds *Datastore, name string) *installStatusFixture { + ctx := context.Background() + + team, err := ds.NewTeam(ctx, &fleet.Team{Name: name}) + require.NoError(t, err) + user := test.NewUser(t, ds, name, name+"@example.com", true) + + installerID, titleID, err := ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ + Title: name, + Source: "apps", + InstallScript: "echo", + TeamID: &team.ID, + Filename: name + ".pkg", + UserID: user.ID, + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + }) + require.NoError(t, err) + + var seq int + return &installStatusFixture{ + teamID: team.ID, + installerID: installerID, + titleID: titleID, + addInstall: func(hostID uint, createdAt time.Time, exitCode *int, removed, canceled, hostDeleted bool) { + seq++ + var deletedAt *time.Time + if hostDeleted { + deletedAt = &createdAt + } + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, ` +INSERT INTO host_software_installs + (execution_id, host_id, software_installer_id, software_title_id, install_script_exit_code, + created_at, updated_at, removed, canceled, host_deleted_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + fmt.Sprintf("%s-%d", name, seq), hostID, installerID, titleID, exitCode, + createdAt, createdAt, removed, canceled, deletedAt) + return err + }) + }, + } +} + +func testInstallStatusUsesLatestRowPerHost(t *testing.T, ds *Datastore) { + ctx := context.Background() + f := newInstallStatusFixture(t, ds, "latest-row") + + newHost := func(tag string) *fleet.Host { + h, err := ds.NewHost(ctx, &fleet.Host{ + Hostname: "latest-row-" + tag, + OsqueryHostID: new("osquery-latest-row-" + tag), + NodeKey: new("node-key-latest-row-" + tag), + UUID: uuid.NewString(), + Platform: "darwin", + TeamID: &f.teamID, + }) + require.NoError(t, err) + return h + } + + base := time.Now().UTC().Truncate(time.Second).Add(-24 * time.Hour) + succeeded, failed := new(0), new(1) + + // The newest row decides the status, even when an older row succeeded. + latestFailed := newHost("latest-failed") + f.addInstall(latestFailed.ID, base, succeeded, false, false, false) + f.addInstall(latestFailed.ID, base.Add(time.Hour), failed, false, false, false) + + // Canceled and removed rows are not candidates, so the previous row wins. + canceledLatest := newHost("canceled-latest") + f.addInstall(canceledLatest.ID, base, succeeded, false, false, false) + f.addInstall(canceledLatest.ID, base.Add(time.Hour), nil, false, true, false) + + removedLatest := newHost("removed-latest") + f.addInstall(removedLatest.ID, base, failed, false, false, false) + f.addInstall(removedLatest.ID, base.Add(time.Hour), succeeded, true, false, false) + + // Rows sharing a created_at are broken by id, and the host is counted once. + tied := newHost("tied") + f.addInstall(tied.ID, base, failed, false, false, false) + f.addInstall(tied.ID, base, succeeded, false, false, false) + + // A host whose rows are all soft-deleted drops out of the summary. The host + // list filter has never had a host_deleted_at condition, so it still matches + // there; that asymmetry is asserted below so it stays deliberate. + softDeleted := newHost("soft-deleted") + f.addInstall(softDeleted.ID, base, succeeded, false, false, true) + + // A queued install supersedes the host's history. + queued := newHost("queued") + f.addInstall(queued.ID, base, succeeded, false, false, false) + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + res, err := q.ExecContext(ctx, ` +INSERT INTO upcoming_activities + (host_id, priority, fleet_initiated, activity_type, execution_id, payload) +VALUES (?, 0, 1, 'software_install', 'latest-row-queued', JSON_OBJECT('self_service', false))`, queued.ID) + if err != nil { + return err + } + uaID, err := res.LastInsertId() + if err != nil { + return err + } + _, err = q.ExecContext(ctx, ` +INSERT INTO software_install_upcoming_activities + (upcoming_activity_id, software_installer_id, software_title_id) +VALUES (?, ?, ?)`, uaID, f.installerID, f.titleID) + return err + }) + + summary, err := ds.GetSummaryHostSoftwareInstalls(ctx, f.installerID) + require.NoError(t, err) + require.Equal(t, fleet.SoftwareInstallerStatusSummary{ + Installed: 2, // canceledLatest, tied + FailedInstall: 2, // latestFailed, removedLatest + PendingInstall: 1, // queued + }, *summary) + + userTeamFilter := fleet.TeamFilter{User: test.UserAdmin} + for _, c := range []struct { + status fleet.SoftwareInstallerStatus + wantHostIDs []uint + }{ + {fleet.SoftwareInstalled, []uint{canceledLatest.ID, tied.ID, softDeleted.ID}}, + {fleet.SoftwareInstallFailed, []uint{latestFailed.ID, removedLatest.ID}}, + {fleet.SoftwareFailed, []uint{latestFailed.ID, removedLatest.ID}}, + {fleet.SoftwareInstallPending, []uint{queued.ID}}, + {fleet.SoftwareUninstallFailed, []uint{}}, + } { + t.Run(string(c.status), func(t *testing.T) { + opts := fleet.HostListOptions{ + ListOptions: fleet.ListOptions{PerPage: 100}, + SoftwareTitleIDFilter: &f.titleID, + SoftwareStatusFilter: &c.status, + TeamFilter: &f.teamID, + } + hosts, err := ds.ListHosts(ctx, userTeamFilter, opts) + require.NoError(t, err) + gotHostIDs := make([]uint, 0, len(hosts)) + for _, h := range hosts { + gotHostIDs = append(gotHostIDs, h.ID) + } + require.ElementsMatch(t, c.wantHostIDs, gotHostIDs) + + count, err := ds.CountHosts(ctx, userTeamFilter, opts) + require.NoError(t, err) + require.Equal(t, len(c.wantHostIDs), count) + }) + } +} + +// testInstallStatusDoesNotScanHistoryPerHostRow pins the cost model of the host +// software status filter: selecting each host's most recent install must stay +// bounded by the title's or app's own history. Two shapes break that bound in +// opposite directions — a self anti-join rescans a host's history once per row of +// it, and a per-host lookup scales with the candidate host set instead of the +// title. Both are caught here. +// +// Only the host list filter is asserted. The status summary shares the anti-join +// defect, but MySQL only picks the quadratic plan for it well past any fixture +// size that belongs in a unit test, so a bound there would not discriminate. +func testInstallStatusDoesNotScanHistoryPerHostRow(t *testing.T, ds *Datastore) { + ctx := context.Background() + user := test.NewUser(t, ds, "depth", "depth@example.com", true) + + const ( + deepHosts = 50 + deepDepth = 100 + breadthHosts = 3000 + narrowHosts = 5 + shallowDepth = 2 + // every install row this title has, across all three teams + titleRows = deepHosts*deepDepth + (breadthHosts+narrowHosts)*shallowDepth + ) + + // One title, one installer per team, so every team's hosts share software_title_id. + var titleID uint + newTeamInstaller := func(name string) uint { + team, err := ds.NewTeam(ctx, &fleet.Team{Name: name}) + require.NoError(t, err) + _, tID, err := ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ + Title: "history-depth", + Source: "apps", + InstallScript: "echo", + TeamID: &team.ID, + Filename: "history-depth.pkg", + UserID: user.ID, + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + }) + require.NoError(t, err) + titleID = tID + return team.ID + } + deepTeam := newTeamInstaller("history-depth-deep") + breadthTeam := newTeamInstaller("history-depth-breadth") + narrowTeam := newTeamInstaller("history-depth-narrow") + + addHosts := func(prefix string, teamID uint, n int) []uint { + ids := make([]uint, 0, n) + for i := range n { + h, err := ds.NewHost(ctx, &fleet.Host{ + Hostname: fmt.Sprintf("%s-%d", prefix, i), + OsqueryHostID: new(fmt.Sprintf("osquery-%s-%d", prefix, i)), + NodeKey: new(fmt.Sprintf("node-key-%s-%d", prefix, i)), + UUID: uuid.NewString(), + Platform: "darwin", + TeamID: &teamID, + }) + require.NoError(t, err) + ids = append(ids, h.ID) + } + return ids + } + // Breadth hosts only need to exist and carry install rows, so insert them in bulk. + addHostsBulk := func(prefix string, teamID uint, n int) []uint { + const chunk = 500 + for start := 0; start < n; start += chunk { + placeholders := make([]string, 0, chunk) + args := make([]any, 0, chunk*7) + for i := start; i < start+chunk && i < n; i++ { + placeholders = append(placeholders, "(?, ?, ?, ?, ?, 'darwin', ?, NOW(), NOW(), NOW(), NOW())") + args = append(args, fmt.Sprintf("osquery-%s-%d", prefix, i), fmt.Sprintf("nk-%s-%d", prefix, i), + fmt.Sprintf("onk-%s-%d", prefix, i), uuid.NewString(), fmt.Sprintf("%s-%d", prefix, i), teamID) + } + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, ` +INSERT INTO hosts (osquery_host_id, node_key, orbit_node_key, uuid, hostname, platform, team_id, + detail_updated_at, label_updated_at, policy_updated_at, last_enrolled_at) +VALUES `+strings.Join(placeholders, ","), args...) + return err + }) + } + var ids []uint + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.SelectContext(ctx, q.(sqlx.QueryerContext), &ids, + `SELECT id FROM hosts WHERE team_id = ?`, teamID) + }) + return ids + } + + deepIDs := addHosts("history-depth", deepTeam, deepHosts) + breadthIDs := addHostsBulk("history-breadth", breadthTeam, breadthHosts) + narrowIDs := addHosts("history-narrow", narrowTeam, narrowHosts) + + // Insert oldest-first so created_at ascends with the auto-increment id, the way + // an append-only install history accumulates. Seeded the other way round the + // anti-join finds a dominating row immediately and the depth bound below passes + // against the quadratic shape. + installerFor := func(teamID uint) uint { + var id uint + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q.(sqlx.QueryerContext), &id, + `SELECT id FROM software_installers WHERE title_id = ? AND global_or_team_id = ?`, titleID, teamID) + }) + return id + } + seedHistory := func(tag string, teamID uint, hostIDs []uint, depth int) { + installerID := installerFor(teamID) + base := time.Now().UTC().Truncate(time.Second).Add(-time.Duration(depth) * time.Hour) + for i := range depth { + createdAt := base.Add(time.Duration(i) * time.Hour) + for start := 0; start < len(hostIDs); start += 500 { + end := min(start+500, len(hostIDs)) + placeholders := make([]string, 0, end-start) + args := make([]any, 0, (end-start)*6) + for _, hostID := range hostIDs[start:end] { + placeholders = append(placeholders, "(?, ?, ?, ?, 0, ?, ?)") + args = append(args, fmt.Sprintf("%s-%d-%d", tag, i, hostID), hostID, + installerID, titleID, createdAt, createdAt) + } + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, ` +INSERT INTO host_software_installs + (execution_id, host_id, software_installer_id, software_title_id, install_script_exit_code, created_at, updated_at) +VALUES `+strings.Join(placeholders, ","), args...) + return err + }) + } + } + } + seedHistory("deep", deepTeam, deepIDs, deepDepth) + seedHistory("breadth", breadthTeam, breadthIDs, shallowDepth) + seedHistory("narrow", narrowTeam, narrowIDs, shallowDepth) + + // Without current statistics the plan depends on whatever ran before it in the + // shared suite, which made this measurement swing by 60x. + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, `ANALYZE TABLE host_software_installs, hosts`) + return err + }) + + // Pin the reads to one connection so the session counters below cover them. + replica, ok := ds.replica.(*sqlx.DB) + require.True(t, ok, "test datastore replica must be a *sqlx.DB to pin a session") + replica.SetMaxOpenConns(1) + // Restore both limits the test datastore was built with. SetMaxOpenConns(1) also + // drops the idle count to 1, and raising max-open again does not put it back, so + // without this every later subtest in the shared suite reopens per query. + t.Cleanup(func() { + pool := testing_utils.MysqlTestConfig("") + replica.SetMaxOpenConns(pool.MaxOpenConns) + replica.SetMaxIdleConns(pool.MaxIdleConns) + }) + + // Sum every row-access counter rather than one of them, so the assertion holds + // whichever access method the optimizer picks. + rowsTouched := func() int64 { + statusRows, err := replica.QueryContext(ctx, `SHOW SESSION STATUS LIKE 'Handler_read%'`) + require.NoError(t, err) + defer statusRows.Close() + var total int64 + for statusRows.Next() { + var name string + var value int64 + require.NoError(t, statusRows.Scan(&name, &value)) + total += value + } + require.NoError(t, statusRows.Err()) + return total + } + countFilteredTo := func(teamID uint, want int) int64 { + status := fleet.SoftwareInstalled + before := rowsTouched() + count, err := ds.CountHosts(ctx, fleet.TeamFilter{User: test.UserAdmin}, fleet.HostListOptions{ + SoftwareTitleIDFilter: &titleID, + SoftwareStatusFilter: &status, + TeamFilter: &teamID, + }) + require.NoError(t, err) + require.Equal(t, want, count) + return rowsTouched() - before + } + + // Breadth first, so each regime reports its own failure. The filter matches five + // hosts, so nothing proportional to the other teams' history may be read; a + // window function over the whole title reads all of it before the team filter + // applies. + // Same two bounds for the VPP builder. host_vpp_software_installs has no + // host_id-leading index, so a per-host lookup here rescans the app's whole + // history for every candidate host — the shape that is right for + // host_software_installs is wrong for this table. + // Its own title: installerAvailableForInstallForTeamAndTitleID prefers a software + // installer over a VPP app, so sharing a title would never reach vppAppJoin. + vppAdamID := "history-depth-adam" + var vppTitleID uint + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + if _, err := q.ExecContext(ctx, ` +INSERT INTO software_titles (name, source, bundle_identifier) VALUES ('history-depth-vpp', 'apps', 'com.example.historydepth')`); err != nil { + return err + } + if err := sqlx.GetContext(ctx, q.(sqlx.QueryerContext), &vppTitleID, + `SELECT id FROM software_titles WHERE name = 'history-depth-vpp'`); err != nil { + return err + } + if _, err := q.ExecContext(ctx, ` +INSERT INTO vpp_apps (adam_id, title_id, bundle_identifier, name, latest_version, platform) +VALUES (?, ?, 'com.example.historydepth', 'history-depth-vpp', '1.0', 'darwin')`, vppAdamID, vppTitleID); err != nil { + return err + } + for _, teamID := range []uint{deepTeam, breadthTeam, narrowTeam} { + if _, err := q.ExecContext(ctx, ` +INSERT INTO vpp_apps_teams (adam_id, team_id, global_or_team_id, platform) +VALUES (?, ?, ?, 'darwin')`, vppAdamID, teamID, teamID); err != nil { + return err + } + } + return nil + }) + seedVPPHistory := func(tag string, hostIDs []uint, depth int) { + base := time.Now().UTC().Truncate(time.Second).Add(-time.Duration(depth) * time.Hour) + for i := range depth { + createdAt := base.Add(time.Duration(i) * time.Hour) + for start := 0; start < len(hostIDs); start += 500 { + end := min(start+500, len(hostIDs)) + placeholders := make([]string, 0, end-start) + args := make([]any, 0, (end-start)*5) + for _, hostID := range hostIDs[start:end] { + placeholders = append(placeholders, "(?, ?, ?, 'darwin', ?, ?)") + args = append(args, hostID, vppAdamID, + fmt.Sprintf("cmd-%s-%d-%d", tag, i, hostID), createdAt, createdAt) + } + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + // verification_failed_at set so the status CASE resolves without + // needing nano_command_results rows. + _, err := q.ExecContext(ctx, ` +INSERT INTO host_vpp_software_installs + (host_id, adam_id, command_uuid, platform, verification_failed_at, created_at) +VALUES `+strings.Join(placeholders, ","), args...) + return err + }) + } + } + } + seedVPPHistory("deep", deepIDs, deepDepth) + seedVPPHistory("breadth", breadthIDs, shallowDepth) + seedVPPHistory("narrow", narrowIDs, shallowDepth) + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, `ANALYZE TABLE host_vpp_software_installs`) + return err + }) + + vppFailed := fleet.SoftwareInstallFailed + countVPPFilteredTo := func(teamID uint, want int) int64 { + before := rowsTouched() + count, err := ds.CountHosts(ctx, fleet.TeamFilter{User: test.UserAdmin}, fleet.HostListOptions{ + SoftwareTitleIDFilter: &vppTitleID, + SoftwareStatusFilter: &vppFailed, + TeamFilter: &teamID, + }) + require.NoError(t, err) + require.Equal(t, want, count) + return rowsTouched() - before + } + + // Measured here, deep case: 37,707 ranking the app once, 551,258 with a per-host + // lookup that has no index to serve it (50 hosts x the app's whole 11,010 rows). + vppNarrowTouched := countVPPFilteredTo(narrowTeam, narrowHosts) + require.Less(t, vppNarrowTouched, int64(200000), + "VPP status filter must not rescan the app's install history per candidate host") + vppDeepTouched := countVPPFilteredTo(deepTeam, deepHosts) + require.Less(t, vppDeepTouched, int64(200000), + "VPP status filter must not rescan the app's install history per candidate host") + + // Depth: the filter matches every host that has deep history, so the work is + // bounded by that history, not by a multiple of it. The anti-join shape reads + // about (depth+1)/2 rows per row of history here. + // The work must stay bounded by the title's own history rather than a multiple of + // it. Measured here: 53,389 ranking the title once, 263,059 for the self + // anti-join, which rescans each host's history per row of that history. + // + // This fixture deliberately gives every host history for the title, so it cannot + // separate a per-host-lookup shape from a ranking one — the two cost the same + // when fleet size equals the set of hosts with history. The regimes that do + // separate them (narrow filter over a wide title, and no host filter at all over + // a sparse title) need fixtures far too large for a unit test; they are measured + // by the harness in bugs/51426/repro/ instead. + deepTouched := countFilteredTo(deepTeam, deepHosts) + require.Less(t, deepTouched, int64(6*titleRows), + "must not rescan a host's install history once per row of that history") +} diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index d63aea6904d..9f5ec58a150 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -224,11 +224,11 @@ upcoming AS ( -- NOTE if you change this logic make sure to change vppAppHostStatusNamedQuery accordingly past AS ( SELECT - hvsi.host_id, + ranked.host_id, CASE - WHEN hvsi.verification_at IS NOT NULL THEN + WHEN ranked.verification_at IS NOT NULL THEN :software_status_installed - WHEN hvsi.verification_failed_at IS NOT NULL THEN + WHEN ranked.verification_failed_at IS NOT NULL THEN :software_status_failed WHEN ncr.status = :mdm_status_error OR ncr.status = :mdm_status_format_error THEN :software_status_failed @@ -238,34 +238,42 @@ past AS ( ELSE NULL -- either pending or not installed via VPP App END AS status - FROM - host_vpp_software_installs hvsi - JOIN hosts h ON host_id = h.id - LEFT JOIN nano_command_results ncr ON - ncr.id = h.uuid AND - ncr.command_uuid = hvsi.command_uuid - LEFT JOIN host_vpp_software_installs hvsi2 - ON hvsi.host_id = hvsi2.host_id AND - hvsi.adam_id = hvsi2.adam_id AND - hvsi.platform = hvsi2.platform AND - hvsi2.removed = 0 AND - hvsi2.canceled = 0 AND - (hvsi.created_at < hvsi2.created_at OR (hvsi.created_at = hvsi2.created_at AND hvsi.id < hvsi2.id)) + FROM ( + SELECT + hvsi.host_id, + hvsi.command_uuid, + hvsi.verification_at, + hvsi.verification_failed_at, + h.uuid AS host_uuid, + ROW_NUMBER() OVER ( + PARTITION BY hvsi.host_id + ORDER BY hvsi.created_at DESC, hvsi.id DESC + ) AS rn + FROM + host_vpp_software_installs hvsi + JOIN hosts h ON hvsi.host_id = h.id + WHERE + hvsi.adam_id = :adam_id + AND hvsi.platform = :platform + AND (h.team_id = :team_id OR (h.team_id IS NULL AND :team_id = 0)) + AND hvsi.removed = 0 + AND hvsi.canceled = 0 + ) ranked + LEFT JOIN nano_command_results ncr ON + ncr.id = ranked.host_uuid AND + ncr.command_uuid = ranked.command_uuid WHERE - hvsi2.id IS NULL - AND hvsi.adam_id = :adam_id - AND hvsi.platform = :platform + ranked.rn = 1 -- Allow rows with no nano_command_results — Android VPP never produces -- one, and Fleet-side pre-flight failures (e.g. unresolvable -- managed-config Fleet variable on iOS/iPadOS) record only the install -- row with verification_failed_at set, no MDM command. The CASE above -- maps verification_failed_at IS NOT NULL to failed ahead of the ncr - -- branches. - AND (ncr.id IS NOT NULL OR hvsi.verification_failed_at IS NOT NULL OR (:platform = 'android' AND ncr.id IS NULL)) - AND (h.team_id = :team_id OR (h.team_id IS NULL AND :team_id = 0)) - AND hvsi.host_id NOT IN (SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities - AND hvsi.removed = 0 - AND hvsi.canceled = 0 + -- branches. This check runs after ranking, so a host whose most recent + -- row lacks a command result is dropped rather than falling back to an + -- older row. + AND (ncr.id IS NOT NULL OR ranked.verification_failed_at IS NOT NULL OR (:platform = 'android' AND ncr.id IS NULL)) + AND ranked.host_id NOT IN (SELECT host_id FROM upcoming) -- antijoin to exclude hosts with upcoming activities ) -- count each status