[Backport 7.82.x] fix(dataobs): reuse stored base config across DO_QUERY_ACTIONS updates (#54292)

Backport 83b0223 from #54212.

 ___

<!--Please give us some feedback on your experience writing this PR ! https://app.datadoghq.com/forms/43db4c02-6837-400c-8083-692e141b1b88 !-->

### What does this PR do?

Follow-up to #54200 (merged). Fixes a **second, distinct** duplication bug in
`comp/dataobs/queryactions/impl/handler.go`, surfaced while verifying #54200: after the first
`DO_QUERY_ACTIONS` update correctly retires a base config in favor of a DO check, a **second**
RC update for the same `config_id` (e.g. editing the monitor's queries) could resurrect the
already-retired base config alongside the new DO check — reintroducing the "parallel checks"
duplication that #54200 fixed for the first-update case.

Root cause: `onRCUpdate` always re-derived the "base config" via `findMatchingConfig`, which
searches only currently-*active* (scheduled) configs. Once the true original is unscheduled after
the first update, the only thing left matching that host is the DO component's own
previously-scheduled check — which also satisfies `matchesIdentifier` and `instanceHasDOEnabled`,
so it looks like a legitimate base config from the outside. Adopting it as the new "base" gives it
a different digest than the true original, so `reconcileBases`'s digest-based tracking loses track
of the true original and wrongly restores it.

Fix: added `resolveBaseConfig`, which reuses the previously-stored `baseCfg` for an already-active
`config_id` instead of re-deriving it every update. Falls back to a fresh `findMatchingConfig`
search if the stored base no longer has a matching instance (e.g. a genuine host change).
Extracted `findMatchingInstance` so both the fresh-search path and the reuse path share identical
matching logic.

Rebased onto latest `main` (now including #54215, an orthogonal fix for a same-host/different-port
remainder-matching bug — no overlap with this change; both coexist cleanly).

### Motivation

Confirmed live against a real SAP HANA instance, with #54200 already applied: editing a monitor
tied to an already-active `DO_QUERY_ACTIONS` config (growing its query count) reliably triggered
`reconcileBases | Restored original postgres config` and a second concurrent
`schema-collection`/`data_observability` cycle, exactly reproducing the duplication bug via a
different trigger than #54200 covers.

### Describe how you validated your changes

- Added `TestOnRCUpdate_SecondUpdateReusesStoredBase`, a regression test using a mutable mock
  `GetUnresolvedConfigs()` that reflects what autodiscovery would actually look like after the
  first update (existing tests' static fixtures couldn't exercise this — they never reflect a
  prior update's `changes.Schedule`/`Unschedule`). Verified this test fails with the pre-fix
  behavior (temporarily reverted the fix locally, confirmed the test fails with the same
  "Restored original postgres config" signature seen live, then restored the fix).
- `dda inv test --targets=./comp/dataobs/queryactions/impl` — all 39 tests pass (41 counting
  table-driven subtests).
- Live-verified end-to-end against a running SAP HANA Express instance: first RC delivery settles
  cleanly (base retired, DO check active), then a real monitor edit triggers a second delivery for
  the same `config_id` (query count 4 → 5) — before this fix, that reproduced the duplication;
  after, the base config is never rescheduled and exactly one check instance remains active
  throughout, confirmed via schema-collection logs showing only a single running instance.
- Re-verified after rebasing onto latest `main` (with #54215 merged): build succeeds, all tests
  still pass, agent runs cleanly against the live SAP HANA instance.

### Additional Notes

None outstanding — the two known duplication triggers (first update in #54200, second update
here) are both covered now.

Co-authored-by: axel.vonengel <axel.vonengel@datadoghq.com>

Author: dd-octo-sts[bot]

comp/dataobs/queryactions/impl/handler.go

@@ -113,7 +113,7 @@ func (c *component) onRCUpdate(updates map[string]state.RawConfig, applyStatus f
 			continue
 		}
 
-		baseCfg, instance, err := c.findMatchingConfig(&payload.DBIdentifier)
+		baseCfg, instance, err := c.resolveBaseConfig(configID, &payload.DBIdentifier)
 		if err != nil {
 			c.log.Warnf("No matching postgres config for %s: %v", configID, err)
 			applyStatus(path, state.ApplyStatus{State: state.ApplyStateError, Error: err.Error()})
@@ -320,6 +320,37 @@ func sameConfig(a, b *integration.Config) bool {
 	return a.Digest() == b.Digest()
 }
 
+// resolveBaseConfig returns the base config a DO check for configID should be derived from.
+//
+// If configID is already active, its previously-resolved base is reused as-is rather than
+// re-derived from the current active set. Once reconcileBases unschedules a base config's
+// targeted instance in favor of a DO check, that instance is no longer present in
+// GetUnresolvedConfigs() (autodiscovery only reports currently-scheduled configs) — so a fresh
+// findMatchingConfig search would instead match the DO component's own previously-scheduled check
+// output, which also satisfies matchesIdentifier and instanceHasDOEnabled. Adopting that as the
+// new "base" corrupts the digest reconcileBases uses to track the true original, causing it to be
+// wrongly restored on this update. Reusing the stored base avoids re-deriving it from a set that
+// may no longer contain it.
+//
+// Falls back to a fresh search if the stored base no longer has an instance matching dbID (e.g.
+// its host genuinely changed between updates).
+func (c *component) resolveBaseConfig(configID string, dbID *DBIdentifier) (*integration.Config, map[string]any, error) {
+	c.activeConfigsMu.Lock()
+	existing, alreadyActive := c.activeConfigs[configID]
+	c.activeConfigsMu.Unlock()
+
+	if alreadyActive {
+		instance, err := c.findMatchingInstance(existing.baseCfg, dbID)
+		if instance != nil {
+			return existing.baseCfg, instance, nil
+		}
+		if err != nil {
+			c.log.Warnf("Stored base config for %s no longer parses cleanly, re-resolving: %v", configID, err)
+		}
+	}
+	return c.findMatchingConfig(dbID)
+}
+
 // findMatchingConfig finds a supported DB integration config that matches the given identifier
 // and has data_observability.enabled: true. Returns the matching config and the already-parsed
 // instance map to avoid re-parsing YAML in callers.
@@ -329,21 +360,12 @@ func (c *component) findMatchingConfig(dbID *DBIdentifier) (*integration.Config,
 	var lastParseErr error
 	for cfgIdx := range cfgs {
 		cfg := cfgs[cfgIdx]
-		if cfg.Name != "postgres" && cfg.Name != "sap_hana" {
-			c.log.Warnf("DO query action: config %s is not a known DO-supported integration", cfg.Name)
+		instance, err := c.findMatchingInstance(&cfg, dbID)
+		if err != nil {
+			lastParseErr = err
 		}
-
-		for _, instanceData := range cfg.Instances {
-			var instance map[string]any
-			if err := yaml.Unmarshal(instanceData, &instance); err != nil {
-				c.log.Warnf("Failed to unmarshal %s instance data for config %s, skipping: %v", cfg.Name, cfg.Name, err)
-				lastParseErr = err
-				continue
-			}
-
-			if matchesIdentifier(instance, dbID) && instanceHasDOEnabled(instance) {
-				return &cfg, instance, nil
-			}
+		if instance != nil {
+			return &cfg, instance, nil
 		}
 	}
 
@@ -355,6 +377,30 @@ func (c *component) findMatchingConfig(dbID *DBIdentifier) (*integration.Config,
 		dbID.Type, dbID.Host)
 }
 
+// findMatchingInstance searches cfg's instances for one matching dbID with data_observability
+// enabled, returning the first match. Instances whose YAML fails to parse are skipped (logged and
+// recorded as lastErr) rather than aborting the search — a later instance may still match.
+func (c *component) findMatchingInstance(cfg *integration.Config, dbID *DBIdentifier) (map[string]any, error) {
+	if cfg.Name != "postgres" && cfg.Name != "sap_hana" {
+		c.log.Warnf("DO query action: config %s is not a known DO-supported integration", cfg.Name)
+	}
+
+	var lastErr error
+	for _, instanceData := range cfg.Instances {
+		var instance map[string]any
+		if err := yaml.Unmarshal(instanceData, &instance); err != nil {
+			c.log.Warnf("Failed to unmarshal %s instance data for config %s, skipping: %v", cfg.Name, cfg.Name, err)
+			lastErr = err
+			continue
+		}
+
+		if matchesIdentifier(instance, dbID) && instanceHasDOEnabled(instance) {
+			return instance, nil
+		}
+	}
+	return nil, lastErr
+}
+
 // matchesIdentifier checks if an instance matches the given DB identifier.
 // Matching is by host — per-query dbname fields handle database routing.
 func matchesIdentifier(instance map[string]any, dbID *DBIdentifier) bool {

comp/dataobs/queryactions/impl/handler_test.go

@@ -1232,3 +1232,70 @@ func TestValidateQuerySpec_ValidIntervalOnly(t *testing.T) {
 	_, hasSchedule := q["schedule"]
 	assert.False(t, hasSchedule, "schedule field must be absent when not set on the query")
 }
+
+// TestOnRCUpdate_SecondUpdateReusesStoredBase is a regression test for a bug where a SECOND RC
+// update for an already-active config_id could resurrect the true original base config alongside
+// a new DO check. After the first update, the base config's targeted instance is no longer
+// present in GetUnresolvedConfigs() — autodiscovery only reports currently-scheduled configs, and
+// reconcileBases has by then unscheduled it in favor of the DO check. Without reusing the stored
+// base, a second onRCUpdate call would instead match the DO component's own previously-scheduled
+// check as the "base" (it also satisfies matchesIdentifier + instanceHasDOEnabled), corrupting the
+// digest reconcileBases tracks and causing it to wrongly restore the true original — exactly the
+// "three parallel checks" duplication bug, triggered by a second update instead of the first.
+func TestOnRCUpdate_SecondUpdateReusesStoredBase(t *testing.T) {
+	const server = "172.17.128.2"
+	const port = 39041
+	sapHanaCfg := integration.Config{
+		Name:     "sap_hana",
+		Provider: "file",
+		Instances: []integration.Data{
+			integration.Data(fmt.Sprintf("server: %s\nport: %d\ndata_observability:\n  enabled: true\n", server, port)),
+		},
+	}
+
+	mockAC := &mockAutodiscovery{
+		Component: fxutil.Test[autodiscovery.Component](t, noopautoconfig.Module()),
+		configs:   []integration.Config{sapHanaCfg},
+	}
+	c := &component{
+		log:           logmock.New(t),
+		ac:            mockAC,
+		activeConfigs: make(map[string]activeConfigEntry),
+		managedBases:  make(map[string]*managedBaseEntry),
+	}
+
+	dbID := DBIdentifier{Type: "self-hosted", Host: fmt.Sprintf("%s:%d", server, port)}
+	makePayload := func(n int) []byte {
+		queries := make([]QuerySpec, n)
+		for i := range queries {
+			queries[i] = QuerySpec{Type: "run_query", Query: fmt.Sprintf("SELECT %d", i), IntervalSeconds: 60, TimeoutSeconds: 10}
+		}
+		data, err := json.Marshal(DOQueryPayload{ConfigID: "cfg-saphana", DBIdentifier: dbID, Queries: queries})
+		require.NoError(t, err)
+		return data
+	}
+
+	// Update 1: 2 queries. The true original base has no other instances, so it's fully
+	// unscheduled (no remainder) in favor of the DO check.
+	_, changes1 := collectStatuses(c, map[string]state.RawConfig{"path/cfg-saphana": {Config: makePayload(2)}})
+	require.Len(t, changes1.Unschedule, 1, "the true original base should be unscheduled")
+	require.Len(t, changes1.Schedule, 1, "only the DO check should be scheduled")
+
+	// Simulate autodiscovery applying changes1: the true original is gone from the active set;
+	// only the DO check (which itself satisfies matchesIdentifier + instanceHasDOEnabled) remains.
+	mockAC.configs = []integration.Config{changes1.Schedule[0]}
+
+	// Update 2: same config_id, now 3 queries — e.g. a monitor edit changing the query count.
+	_, changes2 := collectStatuses(c, map[string]state.RawConfig{"path/cfg-saphana": {Config: makePayload(3)}})
+
+	// Regression check: the true original (0-query) base must never be rescheduled. Every config
+	// scheduled by update 2 must carry DO queries.
+	for _, cfg := range changes2.Schedule {
+		var instance map[string]any
+		require.NoError(t, yaml.Unmarshal(cfg.Instances[0], &instance))
+		doSection, ok := instance["data_observability"].(map[string]any)
+		require.True(t, ok, "scheduled config missing data_observability section — looks like the wrongly-restored original")
+		queries, _ := doSection["queries"].([]any)
+		assert.NotEmpty(t, queries, "scheduled config has no DO queries — looks like the wrongly-restored original")
+	}
+}

releasenotes/notes/fix-saphana-do-query-actions-second-update-duplicate-1e60cb7b1b4661e6.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fix a bug where editing an active Data Observability query action's
+    monitor (e.g. changing its query count) could cause the previously
+    excluded database instance to run as a duplicate check instance again,
+    alongside its Data Observability check, causing duplicate database
+    monitoring collection.