Skip to content

fix: reject incomplete cluster value pages - #279

Open
perfloop[bot] wants to merge 2 commits into
mainfrom
perfloop-pr-open-g9e9d10rwy
Open

perfloop[bot] wants to merge 2 commits into
mainfrom
perfloop-pr-open-g9e9d10rwy

Conversation

@perfloop

@perfloop perfloop Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Cluster-wide ListWithValues could return a terminal page (has_more=false) after an active shard could not be queried. Because callers use has_more to decide whether to request another page, they could stop before seeing keys held by that shard.

Change

Value pages now return an error instead of merging a partial response when client acquisition or a shard's local value-list operation fails. Cancellation and the no-active-node error are unchanged. Keys-only List retains its existing best-effort behavior.

Tests

The assertion is violated on the comparison and satisfied with this change: For a cluster-wide public CacheService.ListWithValues request with two active shards holding distinct matching keys and a live caller context, a page is returned only if every active shard responds: a peer client-acquisition or ListLocalWithValues RPC failure returns an error and no response; healthy shards return both sorted values in a terminal page, caller cancellation remains context.Canceled, and zero active shards retains the existing error.

Checks: 5 passed.

  • make test-server, including TestCacheService_ListWithValuesRequiresEveryActiveShard
  • make test-race TEST=TestCacheService_ListWithValuesRequiresEveryActiveShard
  • make test-coverage TEST=TestCacheService_ListWithValuesRequiresEveryActiveShard
  • make build, make lint, and make license-check

Target CI retains the full Test / build race and coverage runs, integration and end-to-end suites, Benchmark Cluster 3-Node / benchmark-cluster-3n, Benchmark Single Node / benchmark-single, DCO / dco, and Lint PR Title / lint.


Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).


Note

Medium Risk
Changes cluster list semantics for value pages (stricter errors vs partial success) while keys-only list stays best-effort; incorrect rollout could surface more failures to callers that previously got incomplete pages.

Overview
Cluster-wide ListWithValues no longer returns a “complete” page when an active shard cannot be queried. fetchFromAllNodes now fails the whole value-list fan-out if any node’s client lookup or ListLocalWithValues RPC errors, instead of skipping that shard and merging a partial result that could show has_more=false while keys on the failed shard were never seen.

Keys-only cluster List behavior is unchanged: per-node failures are still logged and skipped. Context cancellation and the existing no active nodes error are unchanged.

Adds TestCacheService_ListWithValuesRequiresEveryActiveShard, a multi-node integration test covering healthy merged pages, peer RPC/client failures, all-shard failure, cancellation, and zero active nodes.

Reviewed by Cursor Bugbot for commit 7f1a524. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Changes how cluster-wide list operations handle node failures.

The PR appears safe to merge; no actionable regression was identified.

Summary

The PR makes cluster-wide ListWithValues reject a page when an active shard cannot be queried, while leaving keys-only List best-effort.

  • Adds multi-node coverage for healthy results, shard failures, cancellation, and no active nodes.

Reviews (1) · Last reviewed commit: "fix: reject incomplete cluster value pag..."

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.

1 participant