e2e/qa: stop counting devices that cannot accept users as failures (#4168)

## Summary of Changes

Mainnet Beta QA failed 7 times over Aug 8-10; three (infra runs
31248009915, 31266248546, 31286128831) had one cause.
`qa_alldevices_unicast_test.go` checks device readiness at five sites,
but each increments `FailedTests` *before* that check, so the carve-out
suppressed only the log line. `laconic-{dfw,mia,was}-sw01` have been
activated at `max_users=0` since Aug 6 and the CLI refuses those
connects outright; against `cmh-mn-qa01`'s 13-device pool that read as
21-29% on a 20% per-host gate.

Gating that counter is not enough: `Success()` also requires non-zero
packet counts, which a device that never connected cannot produce.
Not-ready devices are therefore excluded from `ComputeFailureStats`
entirely — per-host denominator included — behind a new `Device.Ready()`
replacing the condition previously spelled out at six call sites.

Exclusion opens a fail-open path worth reviewing closely: the rate is
then measured over a shrinking pool, and with *every* device excluded it
is `0/0`, where `NaN > threshold` is false. Coverage is now gated on the
skipped rate against `-skipped-threshold` (default 0.5), fleet-wide
**and** per host — one metro draining is a few percent of the fleet but
all of a host's pool, and per-host is the gate that fires in practice.
Testing nothing fails independently of that threshold, which
`-skipped-threshold=1` would otherwise switch off. The gate uses
`t.Errorf`, not `t.Fatalf`, so the publishers still run: the skipped
count lands as `devices_skipped` beside `devices_tested` in InfluxDB and
ClickHouse, the latter via an idempotent `ADD COLUMN IF NOT EXISTS`,
best-effort so a writer without ALTER rights still gets its per-device
rows in.

`Ready()` is deliberately a subset of
`is_device_eligible_for_provisioning`: a device at `users_count +
reserved_seats >= max_users` hits the same CLI rejection and still
counts as a failure, but fixing that means re-adding the capacity check
#3697 removed, so it is a follow-up.

Separately, `client_unicast.go` wrapped an always-nil `lastErr`, so
every failure this weekend read `failed to ping after 3 retries:
%!w(<nil>)`. It now reports packet counts.

## Testing Verification

`go test ./e2e/internal/qa/...` and `golangci-lint run --build-tags=qa
./e2e/...` pass. The new subtests fail against the unfixed aggregation:
the mainnet shape reports `total=6 failed=3` for `total=3 failed=0`, and
a host whose whole pool is drained was absent from `PerHost` rather than
visible there as zero coverage.

Not verified: the `qa`-tagged tests need live fleet access, so nothing
here runs against the real fleet and no run has written a
`devices_skipped` row yet. Mainnet has 5 of ~92 non-transit devices
unusable today, well under the 0.5 default.

Two pre-existing holes on the same axis go to separate PRs: a not-ready
device still consumes a batch host slot, which on the two-host devnet
run can trip the `fewer than 2 clients connected` fatal, and the Grafana
active-device filter fails open on a query error but not on an empty
successful response.

Author: ben-dz

CHANGELOG.md

@@ -40,6 +40,7 @@ All notable changes to this project will be documented in this file.
   - `doublezero feed create` and `doublezero feed update` now read back every `--exchange` and `--group` argument, so a feed cannot name a metro or a multicast group that the ledger does not carry. A base58 argument used to pass straight through with no read, so `--group 4Nd1mBQtrMJVYVfKf2PJy9NZUZdTAsp7D4xWLs4gDB4T` created a feed whose group nobody can join. A code was always read back, so only the pubkey form changes. (#4172)
 - E2E/QA
   - `TestQA_MulticastSettlement`'s `validate_instant_allocation_price_matches_chain` no longer names a specific `doublezero_solana_version` in its skip path. Both the comment and the skip message said the pin was `0.5.10-1`; testnet has since moved to `0.5.11-1`, so a reader was told the pin was merely behind when in fact `instant_allocation_price` is in no release yet. They now name what actually gates the field — a doublezero-offchain release carrying doublezero-offchain#405 — and where the pin lives, neither of which goes stale as versions move. Comment and message only, no behaviour change.
+  - `TestQA_AllDevices_UnicastConnectivity` no longer counts a device that cannot accept users against its failure thresholds. Five sites already checked `activated && max_users > 0` and logged `Ignoring <x> failure for device not ready for users`, but each incremented `FailedTests` before the check, so the carve-out suppressed only the log line while the device still counted as failed — and gating that counter alone would not have been enough, since `Success()` also requires a non-zero packet count a device that never connected cannot produce. Such devices are now excluded from `ComputeFailureStats` entirely, per-host denominator included. This is what failed mainnet-beta QA three times over 2026-08-08/09: `laconic-dfw-sw01`, `laconic-mia-sw01` and `laconic-was-sw01` have been activated at `max_users=0` since 08-06, the client CLI refuses those connects outright, and `cmh-mn-qa01` draws from a 13-device pool, so three unusable devices read as a 21-29% per-host rate against the 20% gate. The excluded codes are now reported in test output, so a skip is distinguishable from a pass. The exclusion is deliberately a subset of the program's `is_device_eligible_for_provisioning`: a device at `users_count + reserved_seats >= max_users` hits the same CLI rejection and still counts as a failure, since narrowing that too would restore the capacity pre-filtering #3697 removed. A run that could not attempt more than half of the devices assigned to it — fleet-wide or on any single host — now fails (`-skipped-threshold`, default 0.5) rather than reporting green over the remnant, and testing nothing at all fails regardless of that threshold, where previously the rate was `0/0` and `NaN > threshold` passed silently. The gate is per host as well because a drained metro is a few percent of the fleet but all of one host's coverage. The count also publishes as `devices_skipped` next to `devices_tested` in InfluxDB and ClickHouse, so a collapse that stays under the threshold shows up on the dashboard and not only in the test log. Separately, a ping that never gets a reply reports its packet counts rather than `failed to ping after 3 retries: %!w(<nil>)`; the wrapped error was always nil, because the retry loop returns early on any real failure. (#4168)
 
 ## [v0.34.0](https://github.com/malbeclabs/doublezero/compare/client/v0.33.0...client/v0.34.0) - 2026-08-07
 

e2e/internal/qa/clickhouse.go

@@ -23,8 +23,9 @@ type ClickhouseConfig struct {
 // ClickhouseConfigFromEnv reads ClickHouse connection settings from environment variables.
 // Returns nil if CLICKHOUSE_ADDR is not set, which disables ClickHouse publishing.
 //
-// Schema changes to the tables created by PublishToClickhouse require manual ALTER TABLE
-// or DROP TABLE on the ClickHouse side — there is no migration tooling (same as the controller).
+// There is no migration tooling for the tables created by PublishToClickhouse (same as the
+// controller): a new column must be added to the CREATE TABLE *and* to an idempotent ALTER
+// in createQATables, or inserts break against deployments that already have the table.
 func ClickhouseConfigFromEnv() *ClickhouseConfig {
 	addr := os.Getenv("CLICKHOUSE_ADDR")
 	if addr == "" {
@@ -76,7 +77,7 @@ func buildClickhouseOptions(addr, db, user, pass string, disableTLS bool) *click
 // PublishToClickhouse writes per-device results and a summary row to ClickHouse.
 // Both tables are created automatically on first use (CREATE TABLE IF NOT EXISTS).
 // If cfg is nil, publishing is skipped silently.
-func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseConfig, env string, results []DeviceTestResult, duration time.Duration) error {
+func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseConfig, env string, results []DeviceTestResult, skippedDevices int, duration time.Duration) error {
 	if cfg == nil {
 		log.Debug("ClickHouse publishing skipped: no configuration")
 		return nil
@@ -101,11 +102,11 @@ func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseC
 		return err
 	}
 
-	if err := insertMetadata(ctx, conn, cfg.DB, env, results, duration); err != nil {
+	if err := insertMetadata(ctx, conn, cfg.DB, env, results, skippedDevices, duration); err != nil {
 		return err
 	}
 
-	log.Debug("published QA results to ClickHouse", "devices", len(results))
+	log.Debug("published QA results to ClickHouse", "devices", len(results), "skipped", skippedDevices)
 	return nil
 }
 
@@ -130,6 +131,7 @@ func createQATables(ctx context.Context, conn clickhouse.Conn, db string) error
 				devices_tested  UInt32,
 				devices_success UInt32,
 				devices_failed  UInt32,
+				devices_skipped UInt32,
 				duration_s      Float64
 			) ENGINE = MergeTree
 			PARTITION BY toYYYYMM(timestamp)
@@ -143,6 +145,13 @@ func createQATables(ctx context.Context, conn clickhouse.Conn, db string) error
 			return fmt.Errorf("failed to create QA table: %w", err)
 		}
 	}
+
+	// The CREATE above is a no-op where the table predates devices_skipped. Best
+	// effort so a writer without ALTER rights still gets its per-device results
+	// in; a column that really is missing surfaces on the metadata insert.
+	_ = conn.Exec(ctx, fmt.Sprintf(
+		`ALTER TABLE "%s".qa_alldevices_metadata ADD COLUMN IF NOT EXISTS devices_skipped UInt32 AFTER devices_failed`, db,
+	))
 	return nil
 }
 
@@ -172,7 +181,7 @@ func insertResults(ctx context.Context, conn clickhouse.Conn, db, env string, re
 	return batch.Close()
 }
 
-func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, results []DeviceTestResult, duration time.Duration) error {
+func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, results []DeviceTestResult, skippedDevices int, duration time.Duration) error {
 	var successCount, failedCount uint32
 	for _, r := range results {
 		if r.Success {
@@ -183,13 +192,13 @@ func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, r
 	}
 
 	batch, err := conn.PrepareBatch(ctx, fmt.Sprintf(
-		`INSERT INTO "%s".qa_alldevices_metadata (timestamp, env, devices_tested, devices_success, devices_failed, duration_s)`, db,
+		`INSERT INTO "%s".qa_alldevices_metadata (timestamp, env, devices_tested, devices_success, devices_failed, devices_skipped, duration_s)`, db,
 	))
 	if err != nil {
 		return fmt.Errorf("failed to prepare metadata batch: %w", err)
 	}
 
-	if err := batch.Append(time.Now(), env, uint32(len(results)), successCount, failedCount, duration.Seconds()); err != nil {
+	if err := batch.Append(time.Now(), env, uint32(len(results)), successCount, failedCount, uint32(skippedDevices), duration.Seconds()); err != nil {
 		return fmt.Errorf("failed to append metadata row: %w", err)
 	}
 

e2e/internal/qa/client.go

@@ -108,6 +108,20 @@ type Device struct {
 	DeviceType                serviceability.DeviceDeviceType
 }
 
+// Ready reports whether a connect against this device can get far enough to
+// tell us anything. MaxUsers == 0 means drained or never enabled; either way the
+// CLI's own precheck refuses the connect ("Device is not accepting more users")
+// before the onchain qa_allowlist can exempt us, so failures against it say
+// nothing about the network and must not count toward QA failure rates.
+//
+// This is a subset of the program's is_device_eligible_for_provisioning, which
+// also requires UsersCount+ReservedSeats < MaxUsers; a device at capacity still
+// reports as ready here. MaxUnicastUsers is deliberately not consulted: 0 there
+// means "no per-type limit", the inverse of drained.
+func (d *Device) Ready() bool {
+	return d.Status == serviceability.DeviceStatusActivated && d.MaxUsers > 0
+}
+
 type Client struct {
 	log            *slog.Logger
 	grpcClient     pb.QAAgentServiceClient

e2e/internal/qa/client_unicast.go

@@ -136,14 +136,12 @@ func (c *Client) TestUnicastConnectivity(t *testing.T, ctx context.Context, targ
 	}
 
 	var lastResp *pb.PingResult
-	var lastErr error
 	for i := range unicastPingMaxRetries {
 		resp, err := c.pingOnce(ctx, targetIP, sourceIP, iface)
 		if err != nil {
 			return nil, fmt.Errorf("failed to ping: %w", err)
 		}
 		lastResp = resp
-		lastErr = err
 
 		if resp.PacketsSent == 0 {
 			c.log.Warn("No packets sent",
@@ -227,7 +225,18 @@ func (c *Client) TestUnicastConnectivity(t *testing.T, ctx context.Context, targ
 			PacketsReceived: lastResp.PacketsReceived,
 		}
 	}
-	return result, fmt.Errorf("failed to ping after %d retries: %w", unicastPingMaxRetries, lastErr)
+	return result, pingFailureError(unicastPingMaxRetries, lastResp)
+}
+
+// pingFailureError describes a ping that never got a reply. Every attempt's RPC
+// succeeded — the loop above returns early otherwise — so there is no error to
+// wrap and the packet counts are the only diagnostic available.
+func pingFailureError(retries int, lastResp *pb.PingResult) error {
+	if lastResp == nil {
+		return fmt.Errorf("failed to ping after %d retries: no ping result returned", retries)
+	}
+	return fmt.Errorf("failed to ping after %d retries: %d/%d packets received on the last attempt",
+		retries, lastResp.PacketsReceived, lastResp.PacketsSent)
 }
 
 func (c *Client) pingOnce(ctx context.Context, targetIP string, sourceIP string, iface string) (*pb.PingResult, error) {

e2e/internal/qa/client_unicast_test.go

@@ -322,3 +322,38 @@ func TestFindIBRLStatus(t *testing.T) {
 		})
 	}
 }
+
+func TestPingFailureError(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		lastResp *pb.PingResult
+		want     string
+	}{
+		{
+			name:     "total loss reports the counts",
+			lastResp: &pb.PingResult{PacketsSent: 40, PacketsReceived: 0},
+			want:     "failed to ping after 3 retries: 0/40 packets received on the last attempt",
+		},
+		{
+			name:     "no packets sent reports zeroes",
+			lastResp: &pb.PingResult{},
+			want:     "failed to ping after 3 retries: 0/0 packets received on the last attempt",
+		},
+		{
+			name:     "no response at all says so",
+			lastResp: nil,
+			want:     "failed to ping after 3 retries: no ping result returned",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			err := pingFailureError(3, tt.lastResp)
+			require.EqualError(t, err, tt.want)
+			require.NotContains(t, err.Error(), "%!w", "must not format a nil error with %%w")
+		})
+	}
+}

e2e/internal/qa/device_assignment.go

@@ -305,11 +305,18 @@ func AssignDevicesToClients(devices []*Device, clients []*Client, clientLatencie
 // HostFailureStats aggregates per-host failure information after deduping
 // repeated tests of the same (host, device) pair.
 type HostFailureStats struct {
-	Total         int      // unique devices assigned to this host
+	Total         int      // unique testable devices assigned to this host
 	Failed        int      // unique devices that never succeeded on this host
+	Skipped       int      // unique devices excluded for not accepting users; disjoint from Total
 	FailedDevices []string // sorted, deduped device codes
 }
 
+// SkippedRate is this host's share of assigned devices that could not accept
+// users. See FailureStats.SkippedRate for the fleet-wide equivalent.
+func (h HostFailureStats) SkippedRate() float64 {
+	return skippedRate(h.Total, h.Skipped)
+}
+
 // DeviceRetest describes a (host, device) pair that was tested more than once.
 type DeviceRetest struct {
 	Host       string
@@ -331,25 +338,57 @@ type FailureStats struct {
 	DeviceResults []DeviceTestResult          // one per unique device code
 	PerHost       map[string]HostFailureStats // keyed by host
 	Retests       []DeviceRetest              // entries where Attempts > 1
+	Skipped       []string                    // sorted codes of devices that could not accept users
+}
+
+// SkippedRate is the fraction of assigned devices left out of the tallies for
+// not accepting users.
+func (s FailureStats) SkippedRate() float64 {
+	return skippedRate(len(s.DeviceResults), len(s.Skipped))
+}
+
+// skippedRate is 1 when nothing was testable at all, so a gate on it never
+// divides by zero nor reads a NaN as a pass.
+func skippedRate(tested, skipped int) float64 {
+	assigned := tested + skipped
+	if assigned == 0 {
+		return 1
+	}
+	return float64(skipped) / float64(assigned)
 }
 
 // ComputeFailureStats walks batchData once and applies the "any success
 // counts as success" rule per device. Repeated tests of the same
 // (host, device) collapse into a single result for both the overall device
-// list and per-host stats.
+// list and per-host stats. Devices that are not Ready are left out of the
+// failure tallies and counted as skipped instead, fleet-wide and per host.
 func ComputeFailureStats(batchData BatchData) FailureStats {
 	// hostDeviceAttempts[host][code] = number of attempts
 	hostDeviceAttempts := make(map[string]map[string]int)
 	// hostDeviceSuccesses[host][code] = number of successful attempts
 	hostDeviceSuccesses := make(map[string]map[string]int)
 	deviceSucceeded := make(map[string]bool)
 	devicePubkey := make(map[string]string)
+	skipped := make(map[string]struct{})
+	// hostSkipped[host] = set of codes skipped on that host
+	hostSkipped := make(map[string]map[string]struct{})
 
 	batchNums := slices.Sorted(maps.Keys(batchData))
 	for _, batchNum := range batchNums {
 		hosts := slices.Sorted(maps.Keys(batchData[batchNum]))
 		for _, host := range hosts {
 			assignment := batchData[batchNum][host]
+			// A device that cannot accept users is excluded entirely, not
+			// counted as a failure: it must not inflate the denominator either.
+			// Skipped is the caller's audit trail for what was left out.
+			if !assignment.Device.Ready() {
+				skipped[assignment.Device.Code] = struct{}{}
+				if hostSkipped[host] == nil {
+					hostSkipped[host] = make(map[string]struct{})
+				}
+				hostSkipped[host][assignment.Device.Code] = struct{}{}
+				continue
+			}
 			code := assignment.Device.Code
 			if hostDeviceAttempts[host] == nil {
 				hostDeviceAttempts[host] = make(map[string]int)
@@ -389,6 +428,14 @@ func ComputeFailureStats(batchData BatchData) FailureStats {
 		perHost[host] = stats
 	}
 
+	// A host left with nothing testable gets an entry with Total 0: absent from
+	// the map, its lost coverage would be indistinguishable from a clean run.
+	for host, codes := range hostSkipped {
+		hs := perHost[host]
+		hs.Skipped = len(codes)
+		perHost[host] = hs
+	}
+
 	var retests []DeviceRetest
 	hosts := slices.Sorted(maps.Keys(hostDeviceAttempts))
 	for _, host := range hosts {
@@ -413,5 +460,6 @@ func ComputeFailureStats(batchData BatchData) FailureStats {
 		DeviceResults: deviceResults,
 		PerHost:       perHost,
 		Retests:       retests,
+		Skipped:       slices.Sorted(maps.Keys(skipped)),
 	}
 }