fix(snmp-status): fix expvar leaks and subnet index collisions#54116
Draft
rdesgroppes wants to merge 1 commit into
Draft
fix(snmp-status): fix expvar leaks and subnet index collisions#54116rdesgroppes wants to merge 1 commit into
expvar leaks and subnet index collisions#54116rdesgroppes wants to merge 1 commit into
Conversation
rdesgroppes
added
changelog/no-changelog
dd-octo-sts
Bot
added
internal
github-actions
Bot
added
the
short review
rdesgroppes
marked this pull request as ready for review
July 24, 2026 19:05
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beb30c9e26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: eba596a | Docs | Datadog PR Page | Give us feedback! |
Contributor
Files inventory check summaryFile checks results against ancestor 89b77c9f: Results for datadog-agent_7.83.0~devel.git.389.eba596a.pipeline.126935736-1_amd64.deb:No change detected |
Contributor
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
31 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 052fecc Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +0.41 | [-0.59, +1.41] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.21 | [+0.16, +0.27] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.20 | [-0.05, +0.45] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.13 | [+0.08, +0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.06 | [+0.02, +0.11] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.04 | [-0.01, +0.10] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.01 | [-0.11, +0.09] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.58 | [-0.70, -0.45] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 150.34MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 735.38KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 500.19MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 186.17MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.15MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 361.59 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 406.06MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.45MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 31.39 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 301.60MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 63.89 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 284.54MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.62 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 286.25MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_idle | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_mean_fs_load | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_mean_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_no_fs_load | comparison | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
rdesgroppes
added a commit
that referenced
this pull request
Jul 24, 2026
### What does this PR do? Reset the shared `expvar` maps (`snmpProfileErrors`, `snmpAutodiscovery`, `snmpDiscovery`) each test writes to, via `t.Cleanup(...Init())`, so entries from one test never **leak** into another test or into a later `-count` iteration of the same test. Give each `Discovery` instance a unique subnet index, instead of hardcoding index `0`, so 2+ `snmp.d` configs scanning the same network with different credentials no longer **overwrite** the same `snmpDiscovery` expvar key. ### Motivation Found while investigating unrelated flaky tests in the sibling `discovery` package (#54036). To confirm that flakiness had nothing to do with this package, running `-race -count=5` against `./pkg/collector/corechecks/snmp/status` alone (no `discovery` package in the target list) already failed on the `main` branch. Isolating `TestStatusLegacyDiscoveryMultipleSubnets` with `-run` and `-count=1` showed it was already failing on its own, on unmodified `main`: it hardcoded subnet index `0` for both of its `127.0.10.1/30` configs, so the second `.Set()` silently overwrote the first, and its assertions only appeared to pass, in a full suite run, by **coincidentally** matching leftover `snmpAutodiscovery` content from an earlier test. The fix was to give `TestStatusLegacyDiscoveryMultipleSubnets` distinct indices (`1`, `2`), matching what `TestStatusAutodiscoveryMultipleSubnets` already does for the same two configs. On top of that, a Codex [review](#54116 (comment)) surfaced another preexisting bug: `pkg/collector/corechecks/snmp/internal/discovery/discovery.go` always writes index `0`, so the test scenario was showing distinct keys while production was lagging behind by still producing overlapping ones. So, fixing the collision at the source, the same way the modern `comp/core/autodiscovery/listeners/snmp.go` already assigns each subnet a distinct index, makes the test's indices match real production output. ### Describe how you validated your changes - `TestStatusLegacyDiscoveryMultipleSubnets` alone, `-run` plus `-count=1`, failed on unmodified `main`, passes after this fix, - 50 repeated `-race` runs of `./pkg/collector/corechecks/snmp/status`, 12 tests each run, 0 failures and 0 retries needed, - `dda inv test --targets=./pkg/collector/corechecks/snmp/...`, with and without `--race`: 550 tests, 549 passed, 1 pre-existing, unrelated skip, - `dda inv linter.go --targets=./pkg/collector/corechecks/snmp/...`: 0 issues. ### Additional Notes - the two SNMP status templates render `.autodiscoverySubnets` and `.discoverySubnets` back to back with identical text, inside the same `<div>`: that's how `TestStatusLegacyDiscoveryMultipleSubnets` could pass while asserting on data never in `snmpDiscovery`, - `t.Cleanup(...Init())` targets the process-global `*expvar.Map` returned by `expvar.Get`, not a package-local copy, so it also protects any other test file in this package that reads the same 3 vars.
rdesgroppes
force-pushed
the
regis.desgroppes/snmp-status-expvar-test-isolation
branch
from
July 24, 2026 22:05
beb30c9 to
f07e903
Compare
rdesgroppes
changed the title
expvar leaks and subnet index collisions
github-actions
Bot
added
medium review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07e903f75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
rdesgroppes
added a commit
that referenced
this pull request
Jul 24, 2026
### What does this PR do? Reset the shared `expvar` maps (`snmpProfileErrors`, `snmpAutodiscovery`, `snmpDiscovery`) each test writes to, via `t.Cleanup(...Init())`, so entries from one test never **leak** into another test or into a later `-count` iteration of the same test. Give each `Discovery` instance a unique subnet index, instead of hardcoding index `0`, so 2+ `snmp.d` configs scanning the same network with different credentials no longer **overwrite** the same `snmpDiscovery` expvar key. ### Motivation Found while investigating unrelated flaky tests in the sibling `discovery` package (#54036). To confirm that flakiness had nothing to do with this package, running `-race -count=5` against `./pkg/collector/corechecks/snmp/status` alone (no `discovery` package in the target list) already failed on the `main` branch. Isolating `TestStatusLegacyDiscoveryMultipleSubnets` with `-run` and `-count=1` showed it was already failing on its own, on unmodified `main`: it hardcoded subnet index `0` for both of its `127.0.10.1/30` configs, so the second `.Set()` silently overwrote the first, and its assertions only appeared to pass, in a full suite run, by **coincidentally** matching leftover `snmpAutodiscovery` content from an earlier test. The fix was to give `TestStatusLegacyDiscoveryMultipleSubnets` distinct indices (`1`, `2`), matching what `TestStatusAutodiscoveryMultipleSubnets` already does for the same two configs. On top of that, a Codex [review](#54116 (comment)) surfaced another bug: `pkg/collector/corechecks/snmp/internal/discovery/discovery.go` had always been writing index `0`, so the test scenario was showing distinct keys while production was lagging behind by still producing overlapping ones. That collision isn't as old as it looks: before #39459, both call sites used `GetSubnetVarKey(network, subnet.cacheKey)`, and `cacheKey` came from `CheckConfig.DeviceDigest`, which hashes in `CommunityString`/`User`/`AuthKey`/etc., so two configs sharing a network with different credentials never collided. #39459 swapped `GetSubnetVarKey`'s second parameter for a plain `int`, to stop the modern listener's status from depending on the `Digest` function it was reworking. The modern listener got a real per-subnet index, but the legacy call sites were just given `0` to satisfy the new signature, introducing the collision as a side effect. The first fix mirrored the modern listener: a process-global counter handed each `Discovery` instance a fresh index. A second Codex [review](#54116 (comment)) caught the flaw: `Configure()` builds a new `Discovery` (and a fresh counter value) on every check reconfiguration, while `Cancel()`'s `Stop()` never deletes the old `snmpDiscovery` entry, so every reload leaked a stale, duplicate key into the map. The final fix derives `index` from `CheckConfig.DeviceDigest(Network)` instead, restoring the pre-#39459 property: reconfiguring the same instance reuses the same digest and overwrites its own key, while two configs with different credentials for the same subnet still hash to different keys. => added `TestNewDiscoveryIndex` to lock in both properties. ### Describe how you validated your changes - `TestStatusLegacyDiscoveryMultipleSubnets` alone, `-run` plus `-count=1`, failed on unmodified `main`, passes after this fix, - 50 repeated `-race` runs of `./pkg/collector/corechecks/snmp/status`, 12 tests each run, 0 failures and 0 retries needed, - `dda inv test --targets=./pkg/collector/corechecks/snmp/...`, with and without `--race`: 551 tests, 550 passed, 1 pre-existing, unrelated skip, - `dda inv linter.go --targets=./pkg/collector/corechecks/snmp/...`: 0 issues. ### Additional Notes - the two SNMP status templates render `.autodiscoverySubnets` and `.discoverySubnets` back to back with identical text, inside the same `<div>`: that's how `TestStatusLegacyDiscoveryMultipleSubnets` could pass while asserting on data never in `snmpDiscovery`, - `t.Cleanup(...Init())` targets the process-global `*expvar.Map` returned by `expvar.Get`, not a package-local copy, so it also protects any other test file in this package that reads the same 3 vars.
rdesgroppes
force-pushed
the
regis.desgroppes/snmp-status-expvar-test-isolation
branch
from
July 24, 2026 23:49
f07e903 to
bb492eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb492eb13e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+359
to
+362
| digest, _ := strconv.ParseUint(string(config.DeviceDigest(config.Network)), 16, 64) | ||
| return &Discovery{ | ||
| discoveredDevices: make(map[checkconfig.DeviceDigest]Device), | ||
| index: int(digest), |
There was a problem hiding this comment.
When a legacy snmp.d discovery check is reloaded for the same subnet with changed credentials or ignored IPs, Cancel() only calls Stop() and never deletes the old snmpDiscovery entry, while DeviceDigest(config.Network) includes those fields. This makes the restarted discovery write under a new network|index key and leaves the previous status visible indefinitely; before this change, the fixed network|0 key at least overwrote the old entry for the same subnet. Please either delete the old expvar key on stop/reconfigure or choose an index that does not change for the same configured subnet lifecycle.
Useful? React with 👍 / 👎.
rdesgroppes
marked this pull request as draft
July 25, 2026 00:37
### What does this PR do? Reset the shared `expvar` maps (`snmpProfileErrors`, `snmpAutodiscovery`, `snmpDiscovery`) each test writes to, via `t.Cleanup(...Init())`, so entries from one test never **leak** into another test or into a later `-count` iteration of the same test. Give each `Discovery` instance a unique subnet index, instead of hardcoding index `0`, so 2+ `snmp.d` configs scanning the same network with different credentials no longer **overwrite** the same `snmpDiscovery` expvar key. ### Motivation Found while investigating unrelated flaky tests in the sibling `discovery` package (#54036). To confirm that flakiness had nothing to do with this package, running `-race -count=5` against `./pkg/collector/corechecks/snmp/status` alone (no `discovery` package in the target list) already failed on the `main` branch. Isolating `TestStatusLegacyDiscoveryMultipleSubnets` with `-run` and `-count=1` showed it was already failing on its own, on unmodified `main`: it hardcoded subnet index `0` for both of its `127.0.10.1/30` configs, so the second `.Set()` silently overwrote the first, and its assertions only appeared to pass, in a full suite run, by **coincidentally** matching leftover `snmpAutodiscovery` content from an earlier test. The fix was to give `TestStatusLegacyDiscoveryMultipleSubnets` distinct indices (`1`, `2`), matching what `TestStatusAutodiscoveryMultipleSubnets` already does for the same two configs. On top of that, a Codex [review](#54116 (comment)) surfaced another bug: `pkg/collector/corechecks/snmp/internal/discovery/discovery.go` had always been writing index `0`, so the test scenario was showing distinct keys while production was lagging behind by still producing overlapping ones. That collision isn't as old as it looks: before #39459, both call sites used `GetSubnetVarKey(network, subnet.cacheKey)`, and `cacheKey` came from `CheckConfig.DeviceDigest`, which hashes in `CommunityString`/`User`/`AuthKey`/etc., so two configs sharing a network with different credentials never collided. #39459 swapped `GetSubnetVarKey`'s second parameter for a plain `int`, to stop the modern listener's status from depending on the `Digest` function it was reworking. The modern listener got a real per-subnet index, but the legacy call sites were just given `0` to satisfy the new signature, introducing the collision as a side effect. The fix needs `index` to be both distinct per config and stable across reconfiguration. Mirroring the modern listener's process-global counter would only satisfy the first: `Configure()` builds a new `Discovery` (and a fresh counter value) on every check reconfiguration, while `Cancel()`'s `Stop()` never deletes the old `snmpDiscovery` entry, so every reload would leak a stale, duplicate key into the map. A second Codex [review](#54116 (comment)) confirmed exactly that gap. Deriving `index` from `CheckConfig.DeviceDigest(Network)` satisfies both: it restores the pre-#39459 property where reconfiguring the same instance reuses the same digest and overwrites its own key, while two configs with different credentials for the same subnet still hash to different keys. A third Codex [review](#54116 (comment)) found the remaining gap: a reload that changes credentials or ignored IPs changes the digest too, so the new instance's key differs from the old one, and nothing ever deletes the old one. `Stop()` now deletes its own `snmpDiscovery` key, so a changed-credentials reload cleans up after itself instead of leaving a stale entry indefinitely. => added `TestNewDiscoveryIndex` and `TestDiscoveryStopDeletesExpvarKey` to lock in these properties. ### Describe how you validated your changes - `TestStatusLegacyDiscoveryMultipleSubnets` alone, `-run` plus `-count=1`, failed on unmodified `main`, passes after this fix, - 50 repeated `-race` runs of `./pkg/collector/corechecks/snmp/status`, 12 tests each run, 0 failures and 0 retries needed, - `dda inv test --targets=./pkg/collector/corechecks/snmp/...`, with and without `--race`: 552 tests, 551 passed, 1 pre-existing, unrelated skip, - `dda inv linter.go --targets=./pkg/collector/corechecks/snmp/...`: 0 issues. ### Additional Notes - the two SNMP status templates render `.autodiscoverySubnets` and `.discoverySubnets` back to back with identical text, inside the same `<div>`: that's how `TestStatusLegacyDiscoveryMultipleSubnets` could pass while asserting on data never in `snmpDiscovery`, - `t.Cleanup(...Init())` targets the process-global `*expvar.Map` returned by `expvar.Get`, not a package-local copy, so it also protects any other test file in this package that reads the same 3 vars.
rdesgroppes
force-pushed
the
regis.desgroppes/snmp-status-expvar-test-isolation
branch
from
July 25, 2026 00:43
bb492eb to
eba596a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba596a94f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log.Debugf("subnet %s: Stop discovery", d.config.Network) | ||
| close(d.stop) | ||
| // A changed-credentials reload gets a new index, so delete the old key explicitly. | ||
| discoveryVar.Delete(listeners.GetSubnetVarKey(d.config.Network, d.index)) |
There was a problem hiding this comment.
When Stop is called while a worker is still inside checkDevice (for example during a config reload with an SNMP Connect/Get in flight), this deletes the expvar key before the worker has actually exited; checkDevice then unconditionally calls discoveryVar.Set at the end of the scan and can recreate the old key after Stop returns, leaving the stale subnet status leaked. Please guard the final status publish on d.stop or delete only after the discovery goroutines have drained.
AGENTS.md reference: AGENTS.md:L190-L192
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Reset the shared
expvarmaps (snmpProfileErrors,snmpAutodiscovery,snmpDiscovery) each test writes to, viat.Cleanup(...Init()), so entries from one test never leak into another test or into a later-countiteration of the same test.Give each
Discoveryinstance a unique subnet index, instead of hardcoding index0, so 2+snmp.dconfigs scanning the same network with different credentials no longer overwrite the samesnmpDiscoveryexpvar key.Motivation
Found while investigating unrelated flaky tests in the sibling
discoverypackage (#54036).To confirm that flakiness had nothing to do with this package, running
-race -count=5against./pkg/collector/corechecks/snmp/statusalone (nodiscoverypackage in the target list) already failed on themainbranch.Isolating
TestStatusLegacyDiscoveryMultipleSubnetswith-runand-count=1showed it was already failing on its own, on unmodifiedmain: it hardcoded subnet index0for both of its127.0.10.1/30configs, so the second.Set()silently overwrote the first, and its assertions only appeared to pass, in a full suite run, by coincidentally matching leftoversnmpAutodiscoverycontent from an earlier test.The fix was to give
TestStatusLegacyDiscoveryMultipleSubnetsdistinct indices (1,2), matching whatTestStatusAutodiscoveryMultipleSubnetsalready does for the same two configs.On top of that, a Codex review surfaced another bug:
pkg/collector/corechecks/snmp/internal/discovery/discovery.gohad always been writing index0, so the test scenario was showing distinct keys while production was lagging behind by still producing overlapping ones.That collision isn't as old as it looks: before #39459, both call sites used
GetSubnetVarKey(network, subnet.cacheKey), andcacheKeycame fromCheckConfig.DeviceDigest, which hashes inCommunityString/User/AuthKey/etc., so two configs sharing a network with different credentials never collided.#39459 swapped
GetSubnetVarKey's second parameter for a plainint, to stop the modern listener's status from depending on theDigestfunction it was reworking.The modern listener got a real per-subnet index, but the legacy call sites were just given
0to satisfy the new signature, introducing the collision as a side effect.The fix needs
indexto be both distinct per config and stable across reconfiguration. Mirroring the modern listener's process-global counter would only satisfy the first:Configure()builds a newDiscovery(and a fresh counter value) on every check reconfiguration, whileCancel()'sStop()never deletes the oldsnmpDiscoveryentry, so every reload would leak a stale, duplicate key into the map. A second Codex review confirmed exactly that gap.Deriving
indexfromCheckConfig.DeviceDigest(Network)satisfies both: it restores the pre-#39459 property where reconfiguring the same instance reuses the same digest and overwrites its own key, while two configs with different credentials for the same subnet still hash to different keys.A third Codex review found the remaining gap: a reload that changes credentials or ignored IPs changes the digest too, so the new instance's key differs from the old one, and nothing ever deletes the old one.
Stop()now deletes its ownsnmpDiscoverykey, so a changed-credentials reload cleans up after itself instead of leaving a stale entry indefinitely.=> added
TestNewDiscoveryIndexandTestDiscoveryStopDeletesExpvarKeyto lock in these properties.Describe how you validated your changes
TestStatusLegacyDiscoveryMultipleSubnetsalone,-runplus-count=1, failed on unmodifiedmain, passes after this fix,-raceruns of./pkg/collector/corechecks/snmp/status, 12 tests each run, 0 failures and 0 retries needed,dda inv test --targets=./pkg/collector/corechecks/snmp/..., with and without--race: 552 tests, 551 passed, 1 pre-existing, unrelated skip,dda inv linter.go --targets=./pkg/collector/corechecks/snmp/...: 0 issues.Additional Notes
.autodiscoverySubnetsand.discoverySubnetsback to back with identical text, inside the same<div>: that's howTestStatusLegacyDiscoveryMultipleSubnetscould pass while asserting on data never insnmpDiscovery,t.Cleanup(...Init())targets the process-global*expvar.Mapreturned byexpvar.Get, not a package-local copy, so it also protects any other test file in this package that reads the same 3 vars.