fix(cache): GetStatusKeys returns cache keys, not raw node IDs

[krhitesh7]

The bug

The status and snapshot maps are keyed by hash.ID(node):

// CreateDeltaWatch / CreateWatch
nodeID := cache.hash.ID(request.GetNode())
info := cache.getOrCreateStatus(nodeID, request.GetNode())   // shard.status[nodeID] = info

GetStatusKeys threw those map keys away — allStatus() returns only the values — and reported statusInfo.GetNode().GetId(), the raw node ID off the stored proto.

With IDHash the two are the same string, so nothing ever looked wrong. For any NodeHash that derives a key from more than node.Id, the returned list is unusable:

• GetSnapshot(key) misses — it indexes shard.snapshots[node] with no re-hash
• SetSnapshot / UpsertResources write to a key no watch is registered against
• both fail silently, because a missing snapshot is not an error condition on those paths

Callers cannot work around it either, since the correct key exists only inside the cache.

How it was found

ShareChat/nexus#920. xlr8 has an optional per-pod cache key that appends a pod suffix in IDHash.ID. Every xlr8 path that feeds GetStatusKeys() into a cache lookup — snapshot fan-out, the xDS drift reconciler, the /config_lag endpoint — silently addressed the wrong slot the moment that flag was enabled. The reconciler would have reported zero drift forever while looking perfectly healthy, and the preprod test plan that was meant to validate the flag gated on generation cost staying flat, which it would have, precisely because nothing was reaching any proxy.

Credit to @jensoncs for tracing it to this function.

The fix

Walk the shards and collect the map keys — which is what the doc comment ("all node IDs in the status map") already described. Also drops the intermediate allStatus() slice.

Behaviour is unchanged for IDHash, so this is a no-op for existing callers.

Testing

TestGetStatusKeysAreUsableAsCacheKeys uses a hash with a suffix. Without the fix:

GetStatusKeys returned "node-a", want the cache key "node-a~pod-1".
The status map is keyed by hash.ID(node); returning the raw node ID makes
every GetSnapshot built from this list miss.

Existing tests assert only len(keys), so none depended on the old values.

Pre-existing failures in this package — TestSnapshotCacheWatch, TestSnapshotCacheDeltaWatch, TestSnapshotDeltaCacheWatchTimeout, TestSnapshotCacheWithTTL — are identical before and after this change, verified by stashing. pkg/test/main also fails to build on clean nexus for an unrelated reason (VTMarshaledResource does not implement types.Resource).

Summary by CodeRabbit

• Bug Fixes

  • Corrected cache status key reporting to return the actual keys used for stored status and snapshot data.
  • Excluded entries without status information from the returned key list.
  • Ensured status lookups work correctly when cache keys differ from raw node identifiers.

• Tests

  • Added regression coverage for derived cache keys and status retrieval.

[coderabbitai]

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: 31239325-eeda-4ea3-b3b9-47a8612b8fe6

📥 Commits

Reviewing files that changed from the base of the PR and between ffab6ee and 050da4f.

📒 Files selected for processing (2)

• pkg/cache/v3/simple.go
• pkg/cache/v3/status_keys_test.go

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

📝 Walkthrough

Walkthrough

GetStatusKeys now returns hash-derived cache keys from sharded status maps. A regression test verifies that the returned key retrieves the corresponding status information.

Changes

Status cache key correction

Layer / File(s)	Summary
Return hashed status keys and validate retrieval pkg/cache/v3/simple.go, pkg/cache/v3/status_keys_test.go	GetStatusKeys returns non-nil keys from each status shard under read locks. The regression test verifies the hash-derived key and uses it to retrieve status information.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested labels: bugfix

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: GetStatusKeys now returns cache keys instead of raw node IDs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}