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

[otroan]

Three changes to the stats adapter, one per commit, all bug fixes or internal optimisation. No API change.

1. UpdateDir left symlink entries stale (18960a5)

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.

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

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. UpdateDir reads each symlink target once (3cbab7e)

VPP exposes some counters only as one vector plus a set of symlinks naming its items. /node/errors is the case that hurts: it is a single counter vector, and every /err/<node>/<reason> is a symlink into one item of it. A capture from a live device has 4223 such symlinks over a 4223-item vector.

Commit 1 resolves each of those individually, and resolving a symlink reads its whole backing vector — so refreshing that prepared dir re-read the same vector 4223 times.

UpdateDir now groups the symlinks in a prepared dir by the entry they alias, reads each target once, and fans its items out into the prepared entries. The fan-out writes through the slices the entries already hold, so it does not allocate per symlink, and it produces exactly the shape resolving each symlink would: one value per worker thread. Anything the fan-out does not recognise falls back to resolving that symlink on its own, as does segment v1, which has no symlink target encoding.

Two allocation sources turned up while measuring, both paid per symlink per refresh:

• storing the fanned-out slice back into the adapter.Stat interface boxed it;
• getSymlinkIndexes serialised the union through a bytes.Buffer + binary.Write to split one uint64 into two uint32s. It is uint32(v) and uint32(v>>32) — the old code wrote little-endian and reassembled little-endian, so it round-tripped to the same numeric value on any host.

Refreshing 128 symlinks went from 654 allocations to 142, the remainder being one name clone per prepared entry in GetStatDirOnIndex.

What changed since the previous version of this PR

The ListSymlinks API is gone, per @ondrej-fabry's review — the grouping is done inside statsclient instead, exactly as suggested. That gives the same performance model with no public API change: no ListSymlinks, no SymlinkEntry, no SymlinkTarget/SymlinkItem on StatEntry, no Epoch() accessor. Only the unexported statSegment interface gains an accessor for the symlink's (target, item) pair, which the grouping needs.

The two bug-fix commits are unchanged and keep their SHAs.

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.

New in commit 3:

• TestUpdateDirReadsSymlinkTargetOnce wraps the segment in a counting decorator and asserts that refreshing 64 symlinks over one target does exactly one data read.
• TestUpdateDirGroupedMatchesIndividual pins the fan-out against one-at-a-time resolution, value by value — an off-by-one here would mislabel every error counter while looking entirely plausible.
• TestUpdateDirSymlinkRefreshDoesNotAllocate bounds the per-symlink allocation cost.
• The integration test checks the same agreement against a real VPP directory, worker by worker and item by item.

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.

🤖 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.

[otroan]

@ondrej-fabry Thanks — you were right on both counts. Force-pushed; the two bug-fix commits keep their SHAs, the third is replaced.

On the epoch race: agreed, and it was already the fix in e1718f5 (pushed shortly after your comment, so the review predates it). dir.Epoch is now compared against the epoch accessStart settled on, which is the one accessEnd validates, rather than a separately read one. Worth a re-look since it hadn't been reviewed.

On the API: conceded — ListSymlinks is gone. UpdateDir now groups the symlinks in a prepared dir by the entry they alias, reads each target once, and fans the items out into the prepared entries, as you proposed. No public API change at all: no ListSymlinks, no SymlinkEntry, no SymlinkTarget/SymlinkItem, no Epoch(). Only the unexported statSegment interface gains the (target, item) accessor the grouping needs.

It is the better design, and not only for the API surface — it also means callers cannot get the mapping subtly wrong. I had a collector doing the read-vector-and-label-it dance externally, and the internal version deletes that code entirely.

You called the remaining per-tick cost right too: decoding the 8-byte symlink descriptor per selected entry is cheap enough that no prepared state is needed. I did not add any — the grouping is rebuilt each refresh from the directory.

Two things fell out while measuring, both paid per symlink per refresh, both fixed in the same commit:

• storing the fanned-out slice back into the adapter.Stat interface boxed it, so the fan-out now writes through a *adapter.Stat and only stores back when the outer slice actually had to be reallocated;
• getSymlinkIndexes serialised the union through a bytes.Buffer + binary.Write to split one uint64 into two uint32s. It is uint32(v) and uint32(v>>32) — the old code wrote little-endian and reassembled little-endian, so it round-tripped to the same numeric value on any host.

Refreshing 128 symlinks went from 654 allocations to 142, the remainder being one name clone per prepared entry in GetStatDirOnIndex.

Tests for the new behaviour: TestUpdateDirReadsSymlinkTargetOnce wraps the segment in a counting decorator and asserts 64 symlinks over one target cost exactly one data read; TestUpdateDirGroupedMatchesIndividual pins the fan-out against one-at-a-time resolution value by value; and the integration test checks the same agreement against a real VPP directory.

Measured against a live VPP with 4099 series, this took a collector's per-tick segment access from ~6.2 ms to ~24 µs — which matters less for the CPU than for the time spent inside an optimistically-locked segment.

[otroan]

Added a fourth commit after measuring the grouped refresh end to end against a live VPP.

With each symlink target now read once, the dominant remaining cost of UpdateDir turned out to be GetStatDirOnIndex cloning every prepared entry's name just to compare it — ~3900 clones per tick on a real box, each discarded on the next line. StatDirOnIndexMatches compares in place instead; GetStatDirOnIndex keeps cloning, since its callers do retain the name.

Refreshing 128 symlinks: 654 allocations before this PR, 142 after the grouping, 14 with this.

The v1 implementation had no coverage at all — the fake segment harness is v2-only — so the new test builds a directory vector directly for each version and checks that a prefix, an extension, a different name of equal length and an empty name are all rejected.

Measured on a live VPP with 4099 series, a collector's steady tick went from ~14.9 ms to ~3.0 ms (medians over 24 ticks), producing byte-identical output on both paths.