adapter/statsclient: fix stale symlink refresh and dir epoch check, group symlink reads

CHANGELOG.md

@@ -11,6 +11,27 @@ This file lists changes for the GoVPP releases.
 -
 -->
 
+## Unreleased
+
+### Fixes
+
+- adapter/statsclient: `UpdateDir` now refreshes symlink entries. Their resolved
+  type never equals the directory type, so the type check skipped them and a
+  prepared dir kept returning its `PrepareDir` values indefinitely.
+- adapter/statsclient: `UpdateDir` compares the prepared dir against the epoch
+  `accessStart` settled on, closing a window where a directory re-layout between
+  the two epoch reads went undetected.
+
+### Other
+
+- adapter/statsclient: `UpdateDir` resolves symlinks by target group, reading
+  each aliased counter vector once instead of once per symlink naming it. No API
+  change; a prepared dir over a large symlink fan simply refreshes far more
+  cheaply.
+- adapter/statsclient: `UpdateDir` compares each prepared entry's name in place
+  rather than cloning it out of shared memory to compare and discard. Together
+  with the above, refreshing 128 symlinks goes from 654 allocations to 14.
+
 ## 0.13.0
 
 > _13 November 2025_

adapter/statsclient/stat_segment_api.go

@@ -1,4 +1,5 @@
 //  Copyright (c) 2020 Cisco and/or its affiliates.
+//  Copyright (c) 2026 Meter, Inc.
 //
 //  Licensed under the Apache License, Version 2.0 (the "License");
 //  you may not use this file except in compliance with the License.
@@ -83,6 +84,17 @@ type statSegment interface {
 	// the same memory address as the argument.
 	GetStatDirOnIndex(v dirVector, index uint32) (dirSegment, dirName, adapter.StatType)
 
+	// StatDirOnIndexMatches is GetStatDirOnIndex for callers that only need to
+	// confirm an entry is still the one they prepared. It compares the name in
+	// place instead of copying it out, which matters on a refresh path: UpdateDir
+	// runs it once per prepared entry per tick, and cloning a name to compare it
+	// is an allocation per entry for a value discarded immediately afterwards.
+	//
+	// ok reports whether the name at index equals want. The segment pointer is
+	// returned either way; the StatType is meaningful only when ok is true, and
+	// is adapter.Unknown otherwise.
+	StatDirOnIndexMatches(v dirVector, index uint32, want []byte) (dirSegment, adapter.StatType, bool)
+
 	// GetEpoch re-loads stats header and returns current epoch
 	//and 'inProgress' value
 	GetEpoch() (int64, bool)
@@ -93,6 +105,12 @@ type statSegment interface {
 	// Use ^uint32(0) as an empty index (since 0 is a valid value).
 	CopyEntryData(segment dirSegment, index uint32) adapter.Stat
 
+	// GetSymlinkIndexes returns, for a symlink directory segment, the directory
+	// index of the entry it aliases and the item index within that entry.
+	// ok is false if the segment is not a symlink, or if the segment version has
+	// no notion of symlinks.
+	GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32, ok bool)
+
 	// UpdateEntryData accepts pointer to a directory segment with data, and stat
 	// segment to update
 	UpdateEntryData(segment dirSegment, s *adapter.Stat) error

adapter/statsclient/statsclient.go

@@ -1,4 +1,5 @@
 // Copyright (c) 2019 Cisco and/or its affiliates.
+// Copyright (c) 2026 Meter, Inc.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,7 +17,6 @@
 package statsclient
 
 import (
-	"bytes"
 	"fmt"
 	"net"
 	"os"
@@ -282,23 +282,43 @@ func (sc *StatsClient) UpdateDir(dir *adapter.StatDir) (err error) {
 		return adapter.ErrStatsDisconnected
 	}
 
-	epoch, _ := sc.GetEpoch()
-	if dir.Epoch != epoch {
-		return adapter.ErrStatsDirStale
-	}
-
+	// Compare the prepared dir against the epoch accessStart settled on, not against
+	// a separately read one: with two reads the directory can be re-laid-out in
+	// between, in which case the staleness check passes against the old epoch while
+	// the entries are resolved against the new directory - and accessEnd then
+	// confirms that same new epoch, so nothing catches it.
 	accessEpoch := sc.accessStart()
 	if accessEpoch == 0 {
 		return adapter.ErrStatsAccessFailed
 	}
+	if dir.Epoch != accessEpoch {
+		return adapter.ErrStatsDirStale
+	}
 	dirVector := sc.GetDirectoryVector()
 	if dirVector == nil {
-		return err
-	}
+		return fmt.Errorf("failed to update dir: directory vector is nil")
+	}
+	// Symlinks are not refreshed one by one. VPP names every item of a counter
+	// vector with its own symlink - /node/errors and its thousands of
+	// /err/<node>/<reason> aliases being the case that hurts - so resolving them
+	// individually re-reads the same backing vector once per item. Collect them
+	// by target instead and read each target once, below.
+	var symlinks map[uint32][]symlinkRef
 	for i := 0; i < len(dir.Entries); i++ {
-		if err := sc.updateStatOnIndex(&dir.Entries[i], dirVector); err != nil {
+		ref, isSymlink, err := sc.updateStatOnIndex(&dir.Entries[i], dirVector)
+		if err != nil {
 			return err
 		}
+		if !isSymlink {
+			continue
+		}
+		if symlinks == nil {
+			symlinks = make(map[uint32][]symlinkRef)
+		}
+		symlinks[ref.target] = append(symlinks[ref.target], ref)
+	}
+	if err := sc.updateSymlinkGroups(dirVector, symlinks); err != nil {
+		return err
 	}
 	if !sc.accessEnd(accessEpoch) {
 		return adapter.ErrStatsDataBusy
@@ -623,20 +643,129 @@ func (sc *StatsClient) isConnected() bool {
 }
 
 // updateStatOnIndex refreshes the entry data.
-func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVector) (err error) {
+// symlinkRef is a prepared symlink entry and the counter it aliases, held until
+// its target can be read once for the whole group.
+type symlinkRef struct {
+	entry  *adapter.StatEntry
+	dirPtr dirSegment // the symlink's own directory segment, for the fallback
+	target uint32     // directory index of the aliased entry
+	item   uint32     // item within that entry
+}
+
+// updateStatOnIndex refreshes one entry. A symlink is not resolved here: it is
+// reported to the caller so that every symlink sharing a target can be served
+// from a single read of it.
+func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVector) (ref symlinkRef, isSymlink bool, err error) {
 	dirLen := *(*uint32)(vectorLen(vector))
 	if entry.Index >= dirLen {
-		return fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen)
+		return ref, false, fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen)
+	}
+	// Identity is the name; if it no longer matches, the directory changed under
+	// us (the epoch check in UpdateDir normally catches this first). Compared in
+	// place: this runs once per prepared entry per tick, and cloning the name to
+	// compare it would allocate per entry for a value discarded immediately after.
+	dirPtr, dirType, match := sc.StatDirOnIndexMatches(vector, entry.Index, entry.Name)
+	if !match || entry.Data == nil {
+		return ref, false, nil
+	}
+	if dirType == adapter.Symlink {
+		// A symlink's directory entry holds (target, item) indexes rather than a
+		// data pointer, so its resolved Type never equals dirType and the type
+		// check below would skip it, leaving the entry frozen at its PrepareDir
+		// value forever.
+		target, item, ok := sc.GetSymlinkIndexes(dirPtr)
+		if !ok {
+			// No target encoding (segment v1): resolve it on its own.
+			entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0))
+			return ref, false, nil
+		}
+		return symlinkRef{entry: entry, dirPtr: dirPtr, target: target, item: item}, true, nil
 	}
-	dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index)
-	if len(dirName) == 0 ||
-		!bytes.Equal(dirName, entry.Name) ||
-		dirType != entry.Type ||
-		entry.Data == nil {
-		return nil
+	if dirType != entry.Type {
+		return ref, false, nil
 	}
 	if err := sc.UpdateEntryData(dirPtr, &entry.Data); err != nil {
-		return fmt.Errorf("updating stat data for entry %s failed: %v", dirName, err)
+		return ref, false, fmt.Errorf("updating stat data for entry %s failed: %v", entry.Name, err)
+	}
+	return ref, false, nil
+}
+
+// updateSymlinkGroups reads each aliased entry once and fans its items out to
+// the symlinks that name them. Reading the target once is the whole point: a
+// vector of n items aliased by n symlinks costs one read here rather than n.
+func (sc *StatsClient) updateSymlinkGroups(vector dirVector, groups map[uint32][]symlinkRef) error {
+	if len(groups) == 0 {
+		return nil
+	}
+	dirLen := *(*uint32)(vectorLen(vector))
+	for target, refs := range groups {
+		if target >= dirLen {
+			debugf("symlink target index %d out of dir vector length (%d)", target, dirLen)
+			continue
+		}
+		targetPtr, targetName, _ := sc.GetStatDirOnIndex(vector, target)
+		if len(targetName) == 0 {
+			continue
+		}
+		full := sc.CopyEntryData(targetPtr, ^uint32(0))
+		for _, ref := range refs {
+			if !symlinkItem(full, ref.item, &ref.entry.Data) {
+				// A shape this cannot fan out - resolve the symlink on its own
+				// so behaviour matches the one-at-a-time path exactly.
+				ref.entry.Data = sc.CopyEntryData(ref.dirPtr, ^uint32(0))
+			}
+		}
+	}
+	return nil
+}
+
+// symlinkItem extracts one item from an already-read counter vector into dst, in
+// the same shape resolving the symlink directly would produce: one value per
+// worker thread.
+//
+// It writes THROUGH dst rather than returning a new value, and only stores back
+// when the outer slice had to be reallocated. Storing a slice into an
+// adapter.Stat boxes it, which allocates - once per symlink per tick, for a
+// value that is usually identical to the one already there.
+//
+// Reports false for anything it cannot fan out, so the caller can fall back.
+func symlinkItem(full adapter.Stat, item uint32, dst *adapter.Stat) bool {
+	switch d := full.(type) {
+	case adapter.SimpleCounterStat:
+		out, ok := (*dst).(adapter.SimpleCounterStat)
+		if !ok || len(out) != len(d) {
+			out = make(adapter.SimpleCounterStat, len(d))
+			*dst = out
+		}
+		for i, worker := range d {
+			if len(out[i]) != 1 {
+				out[i] = make([]adapter.Counter, 1)
+			}
+			if int(item) < len(worker) {
+				out[i][0] = worker[item]
+			} else {
+				out[i][0] = 0
+			}
+		}
+		return true
+
+	case adapter.CombinedCounterStat:
+		out, ok := (*dst).(adapter.CombinedCounterStat)
+		if !ok || len(out) != len(d) {
+			out = make(adapter.CombinedCounterStat, len(d))
+			*dst = out
+		}
+		for i, worker := range d {
+			if len(out[i]) != 1 {
+				out[i] = make([]adapter.CombinedCounter, 1)
+			}
+			if int(item) < len(worker) {
+				out[i][0] = worker[item]
+			} else {
+				out[i][0] = adapter.CombinedCounter{}
+			}
+		}
+		return true
 	}
-	return
+	return false
 }