Skip to content

Fix ADS watch retention when responses are suppressed - #587

Closed
wkrause13 wants to merge 1 commit into
mainfrom
fix-ads-watch-retention
Closed

wkrause13 wants to merge 1 commit into
mainfrom
fix-ads-watch-retention

Conversation

@wkrause13

Copy link
Copy Markdown

Problem

In ADS mode, cache responses are intentionally suppressed when request.resource_names are not a superset of snapshot resources.

Before this change, suppressed responses could incorrectly drop the watch in two paths:

  1. CreateWatch() immediate-response path:
  • If a snapshot existed and the request version was stale, the cache attempted an immediate response.
  • If ADS name matching suppressed that response, no watch was added to the watch set.
  • Result: the request was effectively dropped and the client could miss later satisfiable updates.
  1. SetSnapshot() watch replay path:
  • Existing watches were deleted unconditionally after respond(...) was called.
  • If ADS name matching suppressed the response, the watch was still removed.
  • Result: same loss of liveness for that watch.

This creates a recurring out-of-sync condition where a client can stop receiving expected updates until another request lifecycle event occurs.

Fix

1) Make respond(...) return whether it actually sent a response

  • respond(...) now returns bool.
  • Returns false when ADS name mismatch suppresses the response.
  • Returns true only when a response is published to the watch channel.

2) Preserve watches when a response was suppressed

  • In SetSnapshot(), delete a watch only if respond(...) == true.
  • In CreateWatch(), if immediate response is suppressed, fall back to adding an open watch.

This ensures watches are retained and can be satisfied by later snapshots.

3) Add observability for intentional suppressions

  • Added metric: xds/suppressed_responses.
  • Tags:
    • type (xDS type URL)
    • reason (ads_name_mismatch)

This lets operators distinguish expected suppressions from delivery failures.

Verification

New unit test

Added: retains ADS watches when immediate response is suppressed by resource-name mismatch in pkg/api/v1/control-plane/cache/cache_test.go

Test flow:

  1. Set snapshot v1 with endpoints a,b.
  2. Open ADS watch requesting only a, with stale version.
  3. Verify no response is sent and watch count remains 1.
  4. Set v2 still a,b; verify still no response and watch remains.
  5. Set v3 with only a; verify response is sent, version v3, 1 resource, and watch count becomes 0.

Solo-kit test runs

  • go test ./pkg/api/v1/control-plane/cache -count=1
  • go test ./pkg/api/v1/control-plane/... -count=1

Cross-repo verification in gloo

Using a temporary go.mod replace in gloo to point to this local branch of solo-kit:

  • go test ./projects/gloo/pkg/xds ./projects/gloo/pkg/syncer/... -count=1
  • go run /tmp/verify_ads_watch_repro.go

Result: targeted gloo suites passed and repro harness reported:

  • PASS: ADS watch-retention fix works via gloo dependency graph

Notes

This change intentionally favors correctness/liveness by retaining suppressed watches instead of dropping them. Existing cancellation and watch lifecycle behavior remains in place; the new suppression metric provides visibility if suppressions accumulate.

chandler-solo added a commit that referenced this pull request Sep 17, 2026
Signed-off-by: David L. Chandler <david.chandler@solo.io>
@chandler-solo

Copy link
Copy Markdown
Contributor

#592 does all of this and more. I took the observability piece from here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants