[Bug]: the incident accumulator keeps only the first error code per (watch, GPU), so a second code on the same GPU is silently dropped

[lfriedman-netllama]

Prerequisites

• I searched existing issues
• I can reproduce this issue

Bug Description

Split out of #1791 at @lalitadithya's request. #1793 fixes the suppression path; this is the underlying data loss that remains once suppression is out of the picture.

When DCGM reports two or more incidents for the same (watch, GPU), only the first error code survives. Later incidents contribute their message but their code is discarded, so the reported code depends on DCGM's iteration order.

health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py:643

if accumulator_key not in gpu_failures_accumulator:
    gpu_failures_accumulator[accumulator_key] = {"code": error_code, "messages": []}

gpu_failures_accumulator[accumulator_key]["messages"].append(error_msg)   # :647

Consolidation at :653 then writes a single ErrorDetails, and types.ErrorDetails holds exactly one code plus one message — so the structure cannot represent two codes even if the loop wanted to.

Why it matters

The code is what drives remediation. The messages are merged, so an operator reading the event text may see both problems, but the recommendedAction mapping keys off the single surviving code. A GPU reporting, say, DCGM_FR_FABRIC_PROBE_STATE and DCGM_FR_NVLINK_DOWN under the NVLINK watch is actioned as whichever DCGM happened to return first.

It also makes the reported code non-deterministic across polls if DCGM's ordering is not stable, which would show up as an event flapping between codes on a GPU whose hardware state has not changed.

We have not been bitten by this in production — our GPU 0 case was the suppression interaction, which #1793 fixes. This is the latent half.

Suggested fix

Two options, in increasing order of correctness and cost:

1. Key the accumulator on (watch, gpu, code). Each distinct code becomes its own incident. Cheap, but entity_failures is dict[gpu_id, ErrorDetails], so it cannot hold two entries for one GPU — this needs the container to change too.
2. Make entity_failures hold a list per GPU, i.e. dict[int, list[ErrorDetails]]. Correct, and it is the shape the data actually has, but it touches every consumer of HealthDetails.

A cheaper interim that removes the non-determinism without restructuring: pick the surviving code deterministically rather than by arrival — for example the highest-severity code, or the first in a documented precedence order — and say so in a comment. That does not fix the loss, but it makes behaviour predictable and reviewable.

Test worth having

One GPU, one watch, two unsuppressed incidents with different codes, asserted in both arrival orders — the reported code should be the same either way. That test fails today for one of the two orders whichever precedence you pick, which is the point.

[lfriedman-netllama]

@lalitadithya re-checked this against merged main now that #1793 has landed, because that PR touched the same function and I wanted to be sure this was not already fixed by it.

It is not. The premise holds, with new line numbers. #1793 moved suppression into the per-incident loop so a suppressed code continues instead of deleting the whole GPU entry, which is the right fix for #1791. The accumulator itself is untouched, at dcgm.py:657-661 on main:

if accumulator_key not in gpu_failures_accumulator:
    gpu_failures_accumulator[accumulator_key] = {"code": error_code, "messages": []}

# Accumulate all error messages for this GPU and watch type
gpu_failures_accumulator[accumulator_key]["messages"].append(error_msg)

First code per (watch, gpu) wins; every later code is discarded while its message is kept and joined into combined_message. So a GPU with two genuinely different unsuppressed codes on one watch reports one code and a message string describing both, and which code survives depends on the order DCGM returns incidents.

Would you like a PR from me for this, and if so which shape? I am asking rather than picking because the three options have very different blast radii:

1. Make the surviving code deterministic rather than arbitrary, for example highest severity wins. Smallest change, no type change, no consumer impact. Does not actually preserve the second code.
2. Let ErrorDetails carry several codes. Represents reality, but types.ErrorDetails.code is a single field today and every consumer of entity_failures would need to handle a list.
3. Emit one entity failure per code, keying entity_failures by (gpu, code) instead of gpu. Most faithful, and the largest change, since the map's key type is assumed elsewhere.

My inclination is 1 as an immediate correctness fix, since it removes the nondeterminism cheaply, with 2 or 3 as a follow-up if you want the second code actually preserved. Happy to be told otherwise. I will not start until you confirm which one you want.

For context on why this matters to us in practice: on our fleet GpuNvlinkWatch is the watch that carries several distinct codes, and we have seen it report NONE, RESTART_VM and CONTACT_SUPPORT as recommended actions depending on which code came through. So the code that survives this accumulator decides the recommended action an operator sees.

[lalitadithya]

We have an open PR for this one #1804, can you please check if that would resolve the issue?

[lfriedman-netllama]

@lalitadithya checked it, and yes, #1804 resolves this. I verified it the same way I checked #1793 rather than reading the diff alone.

Test evidence

Took #1804's new regression file, tests/test_multiple_incident_codes.py, put it on unmodified main at d2175aa6 and ran it against main's source:

7 failed, 4 passed in 0.09s
FAILED test_distinct_codes_survive_publication_and_recovery[False-False]
FAILED test_distinct_codes_survive_publication_and_recovery[False-True]
FAILED test_distinct_codes_survive_publication_and_recovery[True-False]
FAILED test_distinct_codes_survive_publication_and_recovery[True-True]
FAILED test_repeated_code_combines_only_its_own_messages[False]
FAILED test_repeated_code_combines_only_its_own_messages[True]
FAILED test_debounced_second_code_is_published_after_its_threshold

Seven fail on the old code, matching @sylvesterkaczmarek's claim exactly, so the tests genuinely exercise the bug rather than the new implementation. On the branch: 11 passed, and the full monitor suite is 197 passed, 5 subtests passed.

The change does the right thing, and the details that matter are right

entity_failures goes from dict[int, ErrorDetails] to dict[int, list[ErrorDetails]], one entry per distinct code. Four things I specifically looked for, because each would have made it a partial fix:

1. Messages stay with their own code. The accumulator keys on (watch_name, gpu_id, error_code) and joins messages within a code, so a repeated code merges only its own messages instead of gluing unrelated ones together.
2. Ordering is deterministic. The consolidation loop iterates sorted(gpu_failures_accumulator.items()), so which codes appear no longer depends on DCGM's incident order.
3. Suppression still works per incident, so [Bug]: suppressedErrorCodes drops every incident for a GPU+watch, so the #1471 IMEX workaround masks real NVLink faults on GPU 0 #1791 does not regress. _suppress_configured_error_codes now filters within the list and keeps remaining, dropping the GPU entry only when nothing survives. With suppressedErrorCodes: [DCGM_FR_IMEX_UNHEALTHY] set, which is what [Bug]: DCGM_FR_IMEX_UNHEALTHY reported as GpuNvlinkWatch/CONTACT_SUPPORT on single-node NVSwitch systems where IMEX is not applicable #1471 recommends and what we run on 288 nodes, GPU 0's other codes still report.
4. The recommended action is resolved per code. get_recommended_action_from_dcgm_error_map(failure_details.code) sits inside the per-failure loop, with isFatal derived from it there too. That is the part that actually matters operationally: on our fleet GpuNvlinkWatch alone carries NONE, RESTART_VM and CONTACT_SUPPORT depending on the code, so before this change the surviving code decided the action an operator saw.

Also worth noting the entity-cache detail, which is easy to get wrong and is correct here:

entry = pending_cache_updates.get(key, self.entity_cache.get(key))

Reading pending updates before the committed cache is what stops the second code for an entity from overwriting the first's active_errors within a single pass. Without it the extra events would publish but recovery tracking would only remember one code.

Two things to confirm are intentional, neither a blocker

1. Watch status is now the worst incident rather than the last one. The status assignment changed from

health_status[watch_name].status = types.HealthStatus(int(incident.health))

to max(health_status[watch_name].status.value, int(incident.health)). That looks like a genuine latent bug fix: previously a WARN arriving after a FAIL downgraded the whole watch. I agree with it, but it is a behaviour change that is not called out in the PR description and is not obviously covered by the seven regression cases, so it may deserve a line in the release notes and a test of its own.

2. Published event volume rises where a GPU carries several codes. One event per code instead of one per GPU per watch is the intended outcome, but it is worth saying out loud for anyone sizing their datastore, since it lands on the same path as everything else. On our fleet this affects GpuNvlinkWatch in particular. Not an objection, just the operational consequence.

Happy for this to close #1795 from my side. Thanks @sylvesterkaczmarek.