adapter/statsclient: fix stale symlink refresh and dir epoch check, add ListSymlinks

[otroan]

Three changes to the stats adapter, one per commit. The first two are bug fixes that stand on their own; the third is the API change.

1. UpdateDir left symlink entries stale (31edb1f)

updateStatOnIndex skips an entry whose current directory type no longer matches the type recorded at PrepareDir. For a symlink those never match: the directory type stays Symlink, while entry.Type is the resolved type of the counter it aliases. So every symlink in a prepared dir kept its PrepareDir value, and a PrepareDir-once + UpdateDir-per-tick loop over, say, /interfaces reported the same numbers forever.

Symlinks are now re-resolved through CopyEntryData. That allocates, where the non-symlink path updates in place, because a resolved item has no stable backing slice to write into — called out in a comment, with a pointer to ListSymlinks for callers refreshing large numbers of symlinks per tick.

2. UpdateDir compared the prepared dir against a separately read epoch (0442495)

Pre-existing, and spotted by @ondrej-fabry in review. The staleness check read the epoch, then accessStart read it again. If the directory is re-laid-out between the two reads, the 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 and the caller gets values read against a directory its entry indexes no longer describe.

dir.Epoch is now compared against the epoch accessStart settled on, which is the one accessEnd validates.

Also returns an error when the directory vector is nil, rather than the nil named return, which reported success.

3. ListSymlinks (209ec2e)

VPP exposes some counters only as one vector plus a set of symlinks naming its items. /node/errors is the case that motivates this: it is a single counter vector, and every /err/<node>/<reason> is a symlink into one item of it. Reading the vector once is far cheaper than resolving thousands of symlinks — but then the item names are recoverable only from the symlinks, and an item's position is not derivable from anything a caller can compute, because vlib_register_errors allocates it from a heap and reuses freed holes. VPP exports /sys/node/names, but nothing equivalent for error reasons, and StatEntry.Index for a symlink is its own directory index, not the item it aliases.

ListSymlinks(patterns ...string) ([]adapter.SymlinkEntry, error)

returns, per symlink, the entry it aliases (index and name) plus the item index within it. Like ListStats it walks names and indexes only and copies no counter data, so it is cheap enough to rebuild whenever the epoch changes — which is the only time the mapping can change.

What changed since the first version of this PR

Following review, the shape of the API change is different:

• SymlinkTarget/SymlinkItem are no longer fields on StatEntry. A caller that wants labels does not want the per-symlink data copy DumpStats does, and a caller that wants data has no use for the indexes. Putting the mapping on the identifier path serves the actual use case and keeps StatEntry as it was.
• (*StatsClient).Epoch() is dropped. PrepareDir already returns StatDir.Epoch — which is how examples/stats-client reads it — and as noted in review, a standalone accessor could not provide the staleness guarantee its doc comment implied.
• The internal segment accessor reports ok=false for non-symlink entries and for v1, which has no symlink target encoding, so neither can be mistaken for a symlink to target 0 / item 0.

Tests

adapter/statsclient/statseg_v2_fake_test.go builds a synthetic v2 segment laid out the way VPP lays out the real one — shared header, VPP-side pointers that adjust() translates back, length-prefixed vectors — holding a /node/errors vector plus one symlink per item, named in reverse item order so an off-by-one cannot pass unnoticed.

That makes the refresh testable deterministically, with no traffic generation: change the backing counter, call UpdateDir, assert the value seen through the symlink moved. Reverting the fix in commit 1 gives

--- FAIL: TestUpdateDirRefreshesSymlinks
    /err/fake-node/rc: value after UpdateDir = 3, want 300 (stale value was 3)

Building the fake also turned up a pre-existing quirk worth knowing about: CopyEntryData treats union data of zero as "no data", so a symlink to target 0 / item 0 resolves to nil. Real VPP never lands there; it is documented in the fake rather than worked around.

The integration test drops the earlier Data != nil assertion, which was vacuous — PrepareDir already populates it — and instead checks that each symlink's reported (target, item) yields the same value as resolving that symlink individually. That is the property a caller depends on when it reads a backing vector directly and labels its items from ListSymlinks.

🤖 Generated with Claude Code

[ondrej-fabry]

Why expose this? The result can become stale immediately after the call returns, so it can't be used as a guarantee that a prepared dir is still valid.

The comment says callers can "pre-check staleness", which feels misleading because there's an unavoidable TOCTOU race here. UpdateDir still has to do the authoritative epoch/access checks anyway. What's the intended use case for exposing Epoch() separately?

[ondrej-fabry]

🙈 🙉 🙊

[ondrej-fabry]

I think there's still a race in the stale-dir guarantee here, that existed before this PR actually.

We compare dir.Epoch against GetEpoch() first, but then call accessStart() separately. If the epoch changes between those two calls, accessStart() can return the new epoch and we proceed using entries prepared against the old directory. accessEnd() can then succeed against that new epoch.

Since this PR relies on epoch changes invalidating a prepared dir, shouldn't we use the epoch returned by accessStart() for the dir.Epoch comparison instead?

[ondrej-fabry]

I don't think this tests the behavior the PR is adding. PrepareDir already resolves the symlink and populates Data, so Data != nil after UpdateDir would also pass if UpdateDir left the symlink untouched.

Can we change the backing counter between prepare/update and assert that the value observed through the symlink actually changed?

[ondrej-fabry]

Do we want to expose the stats-segment symlink representation as part of the public StatEntry API? This makes callers aware of directory indexes and item indexes specifically so they can bypass normal symlink resolution.

If the goal is efficient refresh, I'd rather see the statsclient provide that operation directly unless there is a concrete use case that requires callers to interpret the backing vector themselves.

[ondrej-fabry]

@otroan

Three changes to support a PrepareDir-once + UpdateDir-per-tick stats collection loop without re-resolving the whole directory each epoch

I think we're exposing the implementation detail to solve a problem that statsclient can solve internally.

The requirement in the PR body is a cheap PrepareDir once + UpdateDir per tick loop for thousands of /err/... symlinks. Since those symlinks mostly point into the same backing vectors, could UpdateDir group symlinks by target internally, read each backing vector once, and fan out the requested columns into the prepared entries?

That would give the collector the intended performance model without adding SymlinkTarget/SymlinkItem to the public StatEntry API. If rebuilding that grouping each tick is still expensive, we could keep it as opaque prepared state instead of exposing VPP's (index1,index2) representation.

---

StatDir already preserves the directory entry index and whether an entry is a symlink:

type StatEntry struct {
    StatIdentifier // contains Index
    Type
    Data
    Symlink bool
}

During UpdateDir, statsclient can inspect each symlink directory entry and decode (target,item) privately. Instead of immediately calling:

entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0))

as the PR does now, it could first group all symlinks by target:

map[targetIndex][]{
    dstEntry,
    itemIndex,
}

Then copy each target once.

That means:

• no public API change at all
• no new PreparedDir
• no SymlinkTarget
• no SymlinkItem
• no Epoch()
• same PrepareDir + UpdateDir loop the PR says it wants

The only remaining per-tick cost is reading the 8-byte symlink descriptor for each selected directory entry. For thousands of /err/... entries that should be much cheaper than resolving/copying the backing vector thousands of times.