fix(api): properly dedup name list responses#4337
fix(api): properly dedup name list responses#4337MasterPtato wants to merge 1 commit intofix-ups-formalize-subjectsfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
🚅 Deployed to the rivet-pr-4337 environment in rivet-frontend
|
Code ReviewSummaryThe use of IndexMap/IndexSet for deduplication across datacenter fanouts is a good approach and correctly solves the core dedup problem. However, there are a few issues worth flagging. Bug: Limit applied before sort in list_names.rs and runners.rsengine/packages/api-public/src/actors/list_names.rs (lines 63-69) In both files, For example, if DCs return names ["z", "a", "b", "c", "d"] and limit=3, you get ["a", "b", "z"] instead of ["a", "b", "c"]. This also breaks cursor-based pagination since the cursor will point to the wrong position in the full ordered set. The pre-existing code had the correct order: sort then truncate. The fix should collect all deduplicated names, sort, then apply the limit. Same issue applies in runners.rs. Potential regression: cursor filtering removed in list.rs actor_ids pathengine/packages/api-public/src/actors/list.rs (the actor_ids branch) The manual cursor filtering was removed. And fetch_actors_by_ids always constructs its peer query with cursor: None hardcoded (utils.rs:90), so the cursor is never forwarded to the peer. Clients using actor_ids + cursor for pagination will silently receive incorrect results (the same first page on every call). If cursor pagination is intentionally unsupported for the actor_ids path, returning an explicit error when both actor_ids and cursor are provided would be safer than silently ignoring it. Minor: redundant actors.truncate(limit) in list.rsAfter passing Some(limit) to fetch_actors_by_ids, the result is still truncated to limit again (line 122). fetch_actors_by_ids sorts but does not truncate, and with multiple DCs each returning up to limit results the aggregated count can exceed limit. The truncate is correct and necessary — just worth a clarifying comment since it looks redundant at first glance. |
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: