From 817c7731621d50a0cc1d0fba222624bcfd2148ff Mon Sep 17 00:00:00 2001
From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Date: Tue, 28 Jul 2026 21:07:13 +0100
Subject: [PATCH] fix(telemetry): make a failed validation run explain itself

Two gaps meant the last failure produced no evidence of its cause.

The node-log upload was gated on `if: failure()`, but the validation step
sets continue-on-error, so the job is not failing at that point and the
condition never fired. Every failed run silently skipped the one artifact
that records why a node did not reach consensus. It now keys on the
validation step's own outcome, and also collects the harness logs.

Transaction failures were logged at DEBUG, which CI does not enable, so a
run where all 3052 submissions failed on a refused connection reported
nothing about it. The first occurrence of each distinct failure kind is now
a warning and repeats stay at DEBUG, so one refused connection says so once
instead of 3052 times.
---
 .github/workflows/telemetry-validation.yml | 12 +++++--
 docker/telemetry/workload/tx_submitter.py  | 37 ++++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/.github/workflows/telemetry-validation.yml b/.github/workflows/telemetry-validation.yml
index 7fbd9021e5..cff532c3fd 100644
--- a/.github/workflows/telemetry-validation.yml
+++ b/.github/workflows/telemetry-validation.yml
@@ -229,13 +229,21 @@ jobs:
           path: /tmp/xrpld-validation/reports/
           retention-days: 30
 
+      # Keyed on the validation step's own outcome, not job status. The step
+      # above sets continue-on-error, so the job is not failing at this point
+      # and `if: failure()` never fires -- which silently skipped these logs on
+      # every failed run, and they are the only record of why a node did not
+      # reach consensus.
       - name: Upload node logs
-        if: failure()
+        if: always() && steps.validation.outcome != 'success'
         uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
         with:
           name: xrpld-node-logs
-          path: /tmp/xrpld-validation/node*/debug.log
+          path: |
+            /tmp/xrpld-validation/node*/debug.log
+            /tmp/xrpld-validation/*.log
           retention-days: 7
+          if-no-files-found: warn
 
       - name: Print validation summary
         if: always()
diff --git a/docker/telemetry/workload/tx_submitter.py b/docker/telemetry/workload/tx_submitter.py
index 4ff015f40f..4d779ec98e 100644
--- a/docker/telemetry/workload/tx_submitter.py
+++ b/docker/telemetry/workload/tx_submitter.py
@@ -38,6 +38,31 @@ import websockets
 
 logger = logging.getLogger("tx_submitter")
 
+# Failure kinds already reported at WARNING, so a repeated failure is not
+# logged again at that level.
+_reported_failures: set[str] = set()
+
+
+def _log_first_failure(key: str, fmt: str, *args: object) -> None:
+    """Log a submission failure loudly the first time its kind is seen.
+
+    A run in which every transaction fails for one reason -- a refused
+    connection, an unfunded account -- used to leave no trace, because these
+    messages were DEBUG and CI does not enable DEBUG. Logging every failure
+    instead would bury the run in thousands of identical lines, so the first
+    of each distinct kind is a WARNING and the rest stay at DEBUG.
+
+    :param key: Identifies the failure kind, e.g. the engine result code.
+    :param fmt: Logging format string.
+    :param args: Format arguments.
+    """
+    if key in _reported_failures:
+        logger.debug(fmt, *args)
+        return
+    _reported_failures.add(key)
+    logger.warning(fmt, *args)
+
+
 # ---------------------------------------------------------------------------
 # Constants
 # ---------------------------------------------------------------------------
@@ -631,7 +656,13 @@ async def submit_transaction(
             sender.sequence += 1
 
         if not success:
-            logger.debug(
+            # First occurrence of each distinct result at WARNING, the rest at
+            # DEBUG. A run where every transaction failed previously produced
+            # no diagnostics at all, because DEBUG is off in CI; logging every
+            # failure instead would bury the run in thousands of identical
+            # lines.
+            _log_first_failure(
+                "result:%s" % engine_result,
                 "%s result: %s (%s)",
                 tx_type,
                 engine_result,
@@ -639,7 +670,9 @@ async def submit_transaction(
             )
     except Exception as exc:
         stats.record(tx_type, False)
-        logger.debug("%s error: %s", tx_type, exc)
+        # Same reasoning, keyed on the exception type: a connection that is
+        # refused for the whole run says it once rather than per transaction.
+        _log_first_failure("exc:%s" % type(exc).__name__, "%s error: %s", tx_type, exc)
 
 
 async def _refresh_sequences(