From 465fccbe69bddf23ae56241b515f547387093dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Sat, 10 Oct 2026 18:05:15 +0200 Subject: [PATCH] collector: bound MI probe queries in Build with a timeout (#2657) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Claude Opus 5.5 Signed-off-by: Jan-Otto Kröpke --- AGENTS.md | 2 ++ internal/collector/dns/dns.go | 2 +- internal/collector/fsrmquota/fsrmquota.go | 2 +- internal/collector/process/process.go | 2 +- .../storage_spaces/storage_spaces_pool.go | 2 +- .../storage_spaces_virtual_disk.go | 2 +- internal/mi/mi_test.go | 29 +++++++++++++++++++ internal/mi/session.go | 6 ++++ pkg/collector/collection_test.go | 3 ++ 9 files changed, 45 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 82c1e244..c35b5870 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,6 +95,8 @@ exercised a live Windows role. the original `Collection` owns collectors and MI resources. A timed-out call may still be running: prevent overlap, drain late metrics and do not free its resources while it runs. Honor the supplied scrape budget in blocking APIs. + `Build` has no budget: bound its MI probe queries with `mi.BuildQueryTimeout`, + never `0` (no MI timeout), so one stalled provider cannot block startup. - Release resources after partial initialization and on error paths. A non-nil PDH collector returned with an error still belongs to the caller and must be closed. Sub-collector cleanup closures must read the initialized field when diff --git a/internal/collector/dns/dns.go b/internal/collector/dns/dns.go index 2ca49403..0be06204 100644 --- a/internal/collector/dns/dns.go +++ b/internal/collector/dns/dns.go @@ -322,7 +322,7 @@ func (c *Collector) buildErrorStatsCollector(miSession *mi.Session) error { c.miQuery = query var stats []Statistic - if err := c.miSession.Query(&stats, mi.NamespaceRootMicrosoftDNS, c.miQuery, 0); err != nil { + if err := c.miSession.Query(&stats, mi.NamespaceRootMicrosoftDNS, c.miQuery, mi.BuildQueryTimeout); err != nil { return fmt.Errorf("failed to query DNS statistics: %w", err) } diff --git a/internal/collector/fsrmquota/fsrmquota.go b/internal/collector/fsrmquota/fsrmquota.go index 8d26e3f4..0aca4bc9 100644 --- a/internal/collector/fsrmquota/fsrmquota.go +++ b/internal/collector/fsrmquota/fsrmquota.go @@ -94,7 +94,7 @@ func (c *Collector) Build(_ *slog.Logger, miSession *mi.Session) error { c.buildDescriptors() var dst []msftFSRMQuota - if err := c.miSession.Query(&dst, mi.NamespaceRootWindowsFSRM, c.miQuery, 0); err != nil { + if err := c.miSession.Query(&dst, mi.NamespaceRootWindowsFSRM, c.miQuery, mi.BuildQueryTimeout); err != nil { return fmt.Errorf("WMI query failed: %w", err) } diff --git a/internal/collector/process/process.go b/internal/collector/process/process.go index 62cd288d..bbf53fe0 100644 --- a/internal/collector/process/process.go +++ b/internal/collector/process/process.go @@ -322,7 +322,7 @@ func (c *Collector) buildWorkerProcessWMI(miSession *mi.Session) error { var workerProcesses []WorkerProcess - if err = miSession.Query(&workerProcesses, mi.NamespaceRootWebAdministration, miQuery, 0); err != nil { + if err = miSession.Query(&workerProcesses, mi.NamespaceRootWebAdministration, miQuery, mi.BuildQueryTimeout); err != nil { c.logger.LogAttrs(context.Background(), slog.LevelDebug, `root\WebAdministration isn't available, reading IIS application pool names from the w3wp command line only`, slog.Any("err", err), diff --git a/internal/collector/storage_spaces/storage_spaces_pool.go b/internal/collector/storage_spaces/storage_spaces_pool.go index 68d15765..c8718f5f 100644 --- a/internal/collector/storage_spaces/storage_spaces_pool.go +++ b/internal/collector/storage_spaces/storage_spaces_pool.go @@ -107,7 +107,7 @@ func (c *Collector) buildStoragePool() error { var dst []msftStoragePool - if err := c.miSession.Query(&dst, mi.NamespaceRootStorage, c.storagePoolMIQuery, 0); err != nil { + if err := c.miSession.Query(&dst, mi.NamespaceRootStorage, c.storagePoolMIQuery, mi.BuildQueryTimeout); err != nil { return fmt.Errorf("WMI query failed: %w", err) } diff --git a/internal/collector/storage_spaces/storage_spaces_virtual_disk.go b/internal/collector/storage_spaces/storage_spaces_virtual_disk.go index 930ce88e..9d82e69d 100644 --- a/internal/collector/storage_spaces/storage_spaces_virtual_disk.go +++ b/internal/collector/storage_spaces/storage_spaces_virtual_disk.go @@ -104,7 +104,7 @@ func (c *Collector) buildVirtualDisk() error { var dst []msftVirtualDisk - if err := c.miSession.Query(&dst, mi.NamespaceRootStorage, c.virtualDiskMIQuery, 0); err != nil { + if err := c.miSession.Query(&dst, mi.NamespaceRootStorage, c.virtualDiskMIQuery, mi.BuildQueryTimeout); err != nil { return fmt.Errorf("WMI query failed: %w", err) } diff --git a/internal/mi/mi_test.go b/internal/mi/mi_test.go index f95dea3c..60270c0c 100644 --- a/internal/mi/mi_test.go +++ b/internal/mi/mi_test.go @@ -772,6 +772,35 @@ func Test_MI_QueryTimeout(t *testing.T) { require.NoError(t, err) } +// Test_MI_Session_QueryTimeout checks that the timeout passed to Session.Query, +// e.g. mi.BuildQueryTimeout, reaches MI and surfaces as the native result. +func Test_MI_Session_QueryTimeout(t *testing.T) { + application, err := mi.ApplicationInitialize() + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, application.Close()) }) + + destinationOptions, err := application.NewDestinationOptions() + require.NoError(t, err) + require.NoError(t, destinationOptions.SetLocale(mi.LocaleEnglish)) + + session, err := application.NewSession(destinationOptions) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, session.Close()) }) + + query, err := mi.NewQuery("select * from win32_process") + require.NoError(t, err) + + var processes []win32Process + + err = session.Query(&processes, mi.NamespaceRootCIMv2, query, time.Millisecond) + require.ErrorIs(t, err, mi.MI_RESULT_INVALID_OPERATION_TIMEOUT) + + processes = nil + + require.NoError(t, session.Query(&processes, mi.NamespaceRootCIMv2, query, mi.BuildQueryTimeout)) + require.NotEmpty(t, processes) +} + func Test_MI_Query_Uint16Array(t *testing.T) { application, err := mi.ApplicationInitialize() require.NoError(t, err) diff --git a/internal/mi/session.go b/internal/mi/session.go index ee41fa4d..f8624bd4 100644 --- a/internal/mi/session.go +++ b/internal/mi/session.go @@ -29,6 +29,12 @@ import ( "golang.org/x/sys/windows" ) +// BuildQueryTimeout bounds the probe queries collectors run in Build. Build +// has no scrape budget, and a zero timeout sets no MI timeout, so a stalled +// provider would otherwise block startup. A timed-out query fails with +// MI_RESULT_INVALID_OPERATION_TIMEOUT. MI ignores timeouts below 1ms. +const BuildQueryTimeout = 30 * time.Second + // Session represents a session. // // https://learn.microsoft.com/en-us/windows/win32/api/mi/ns-mi-mi_session diff --git a/pkg/collector/collection_test.go b/pkg/collector/collection_test.go index ec1d5e58..b75b7147 100644 --- a/pkg/collector/collection_test.go +++ b/pkg/collector/collection_test.go @@ -76,6 +76,9 @@ func TestCollectionBuildAndClose(t *testing.T) { {name: "joined fatal error", buildErr: errors.Join(errors.ErrUnsupported, buildFailure), wantBuildErr: buildFailure}, {name: "nested fatal error", buildErr: errors.Join(pdh.ErrNoData, fmt.Errorf("nested: %w", errors.Join(errors.ErrUnsupported, buildFailure))), wantBuildErr: buildFailure}, {name: "multiple wrapped fatal error", buildErr: fmt.Errorf("%w and %w", errors.ErrUnsupported, buildFailure), wantBuildErr: buildFailure}, + // The error chain of a Build probe query that hit mi.BuildQueryTimeout. + {name: "MI probe query timeout", buildErr: fmt.Errorf("failed to query: %w", fmt.Errorf("instance result: %w", mi.MI_RESULT_INVALID_OPERATION_TIMEOUT))}, + {name: "MI probe query timeout joined with fatal error", buildErr: errors.Join(mi.MI_RESULT_INVALID_OPERATION_TIMEOUT, buildFailure), wantBuildErr: buildFailure}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel()