Review — Feat/vault provider cache

Caches Vault KV-version preflight and secret-map reads to cut API calls (motivation: tokens with low use-limit, #1204). The intent is good and the KV-version cache is sound, but the secret-map cache has real correctness bugs and the concurrency story is incomplete. I would not merge as-is.

Blocking issues

1. Thundering herd defeats the PR's purpose. In GetStringMap, the lock is released between the cache-miss check and population:

cachedVer, verHit := p.kvVersionCache[key]
p.mu.Unlock()            // ← released
...
mountPath, v2, err = isKVv2(key, cli)   // expensive call, no lock
...
p.mu.Lock(); p.kvVersionCache[key] = ... // populate

Under the exact scenario the PR claims to fix — concurrent callers in vals-operator — N goroutines requesting the same uncached path each fire the Vault call, burning N token uses instead of 1. The mutex prevents map races but not duplicate work. The same pattern applies to secretMapCache. Fix with golang.org/x/sync/singleflight keyed by path (dedupes in-flight calls) — this is the idiomatic fix and also removes the need for the manual lock-around-map dance.

2. Cached map is returned by reference. The code stores res in the cache and then returns that same map to the caller:

res := make(map[string]interface{}, len(secrets))
...
p.mu.Lock(); p.secretMapCache[key] = res; p.mu.Unlock()
return res, nil

The original code returned a fresh map per call. Now any caller that mutates the returned map corrupts the cache for every subsequent caller. GetString only reads, but GetStringMap is a public API method. Copy on the way out (or store a copy and return the original).

3. secretMapCache never evicts → stale secrets forever. The author removed this cache in commit 2 for exactly this reason ("Stale data in long-running processes (vals-operator) — no eviction"), then re-added it in commit 3 without addressing eviction. In a long-running operator, rotated secrets will never be re-read. Either add a TTL, drop secretMapCache and keep only kvVersionCache (which is genuinely immutable at runtime and safe to cache forever), or document that the provider is single-run-only.

Should fix

4. kvVersionCache is keyed by full path, not mount. isKVv2 resolves at the mount level, so secret/foo/bar and secret/foo/baz each get separate cache entries and each trigger a preflight, even though they share mount secret/. The PR body claims "one preflight per unique mount" — to actually deliver that, key the cache by mount path (extract it before the lookup) rather than the full secret path.

5. No tests. The package has zero *_test.go files and this is a concurrency-sensitive change. Even a small test with a stubbed client asserting call-counts before/after would lock in the behavior and catch regressions of issues #1–#3.

6. ensureClient is still unsynchronized. Not introduced here, but since the PR's stated motivation is concurrency safety for vals-operator, the client-create/auth path (including cli.SetToken from approle/kubernetes/userpass) races on first concurrent use. Worth fixing in the same pass or explicitly scoping out.

Nits

7. The //nolint:govet // fieldalignment: unsafe.Sizeof confirms no padding waste; tool reports a false discrepancy comment on provider is hand-wavy — fieldalignment is generally correct and sync.Mutex placement does matter. The 8-byte saving isn't worth the churn or the suppressed linter; recommend reverting the reorder.

8. PR body is stale — it still describes the secretMapCache add/remove/re-add saga and the byte-shaving claim, which don't reflect the final commit. Trim to describe the final state.

9. The merged doc-block above the two cache fields would read better as one comment per field.

What's good

• kvVersionResult struct, the readKey rename (avoids mutating the key parameter in the v2 branch — a small latent bug fix), and the make(map, len(secrets)) pre-allocation are clean.
• Single mutex for both caches is fine for correctness; the critique above is about what's done under the lock, not the lock granularity.

Recommendation

Request changes. The minimal mergeable version would be: (a) switch to singleflight for dedup, (b) copy the map before returning, (c) drop secretMapCache or add eviction. Issues #4–#6 are improvements that can follow.

Review (round 2) — much closer

The revised commit (f99c6c74) addresses everything from the first pass cleanly:

• ✅ #1 thundering herd → singleflight for reads and preflights
• ✅ #2 shared map by reference → per-caller copy in GetStringMap
• ✅ #3 stale secret cache → secretMapCache dropped entirely; reads always re-hit Vault
• ✅ #4 mount-keyed preflight → cache keyed by mountPath with prefix lookup for siblings
• ✅ #5 tests → concurrency dedup, copy isolation, per-mount caching, KVv1, GetString
• ✅ #6 ensureClient race → clientMu serializes creation/auth
• ✅ #7 nolint/fieldalignment churn → dropped

This is the right shape now. One correctness-adjacent hardening item and a couple of minor notes below; none are blockers in my view.

Worth addressing

A. lookupKVVersion prefix match assumes the cached mount key ends with /.

for mount, res := range p.kvVersionCache {
    if mount == "" { continue }
    if key == strings.TrimSuffix(mount, "/") || strings.HasPrefix(key, mount) {
        return res, true
    }
}

This is segment-safe iff every cached mount ends in / (so HasPrefix("secretv2/x", "secret/") is false). Vault's sys/internal/ui/mounts does return trailing-slash paths in practice, so today this is fine — but the code doesn't enforce the invariant. If a mount is ever cached without the slash, HasPrefix("secretv2/x", "secret") is true and you can match the wrong mount → v1/v2 misclassification → wrong read path. Two cheap options:

• Normalize on insert: if !strings.HasSuffix(cacheKey, "/") { cacheKey += "/" } (and drop the cacheKey == "" fallback onto key only when truly empty), or
• Match segment-aware: strings.HasPrefix(key+"/", mount).

Either makes the assumption explicit instead of implicit. A small test with two mounts sharing a textual prefix (secret/ + secretv2/) would lock it in.

B. Concurrent first-burst of sibling paths under one mount still fires N preflights. sfVersion.Do is keyed by the full path, so simultaneous first-access of secret/app1 + secret/app2 (different keys) is not deduped — each runs isKVv2, then the mount cache is warm for subsequent calls. Sequential (covered by TestPreflightCachedPerMount) and same-path-concurrent (covered by TestGetStringMap_ConcurrentDedup) are both correct. This is bounded and only affects the very first burst, so it's minor — but worth a line in the PR body so the "one preflight per mount" claim isn't read as a strict guarantee under concurrency. Keying sfVersion by a mount guess is hard (you don't know the mount yet), so I wouldn't contort the code for it; just document.

Minor / nits

C. Shallow copy. The per-caller copy in GetStringMap is shallow — nested map/slice values are still shared across callers. Same as the original behavior and fine for the typical string-valued KV secret; TestGetStringMap_ReturnsIndependentCopies only exercises top-level keys. Worth a one-line comment noting the copy is shallow so future readers don't assume deep isolation.

D. Double copy. readSecretMap builds a res map and returns it as the singleflight result; GetStringMap then copies it again per caller. The inner copy is redundant — readSecretMap could return secrets directly and let GetStringMap own the only copy. Pure micro-cleanup, not important.

E. clientMu is acquired on every GetStringMap (via ensureClient) even after the client exists, serializing the nil-check. Negligible cost; a fast-path if p.client != nil { return p.client, nil } before the lock would avoid it but is optional.

Verdict

The blockers are gone. I'd approve after A is addressed (normalization + a sibling-mount-prefix test) since that's the only thing touching correctness; B–E can be follow-ups or ignore.

Review (round 3) — approving

Round-2 items are all addressed in 3c6ff8b7 / ba697a80:

• ✅ A. Segment-safe prefix match. Cache key is normalized to a trailing slash on insert (if !strings.HasSuffix(cacheKey, "/") { cacheKey += "/" }), and lookupKVVersion now matches via strings.HasPrefix(key+"/", mount) while skipping non-slash entries (the empty-mount fallback is exact-match only). Confirmed correct against the sibling-mount case: secret/ serves secret and secret/foo but not secretv2/x. Locked in by TestLookupKVVersion_SiblingMountPrefix and TestPreflightNormalizesMountWithoutSlash.
• ✅ B. sfVersion first-burst-of-sibling-paths limitation documented in both the struct comment and the PR body.
• ✅ C. Shallow-copy caveat noted inline in GetStringMap.
• ✅ D. Redundant inner copy dropped — readSecretMap returns secrets directly; GetStringMap owns the single per-caller copy.
• E. (clientMu on every call) left as-is, which is fine — negligible.

Local verification (checked out pr-1213)

go vet ./pkg/providers/vault/...     # clean
gofmt -l pkg/providers/vault/        # no diffs
go test -race -count=1 ./pkg/providers/vault/...   # ok 1.665s

The -race run passing is the meaningful signal here — it empirically validates the singleflight + dual-mutex design under the concurrent access the issue is about.

Nice iteration. Ship it.