From bb3778b717cd90c9d1984040342b3818a8976bf6 Mon Sep 17 00:00:00 2001 From: Nathan Flurry Date: Mon, 27 Apr 2026 20:35:09 -0700 Subject: [PATCH 1/3] fix(pegboard): use Iterator streaming mode in list_names so limit param is honored --- .../tests/envoy/api_actors_list_names.rs | 20 +++++++++---------- .../pegboard/src/ops/actor/list_names.rs | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/engine/packages/engine/tests/envoy/api_actors_list_names.rs b/engine/packages/engine/tests/envoy/api_actors_list_names.rs index 9f7ce62b38..906b425988 100644 --- a/engine/packages/engine/tests/envoy/api_actors_list_names.rs +++ b/engine/packages/engine/tests/envoy/api_actors_list_names.rs @@ -80,13 +80,13 @@ fn list_all_actor_names_in_namespace() { } #[test] -// Broken legacy Pegboard Runner test: full engine sweep timed out in -// `list_names_with_pagination`. -#[ignore = "list_names ignores limit param"] fn list_names_with_pagination() { common::run(common::TestOpts::new(1).with_timeout(30), |ctx| async move { - let (namespace, _, _runner) = - common::setup_test_namespace_with_envoy(ctx.leader_dc()).await; + let (namespace, _, _runner) = common::setup_test_namespace_with_envoy_for_names( + ctx.leader_dc(), + (0..9).map(|i| format!("actor-{:02}", i)).collect(), + ) + .await; // Create actors with many different names for i in 0..9 { @@ -404,13 +404,13 @@ fn list_names_alphabetical_sorting() { // MARK: Edge cases #[test] -// Broken legacy Pegboard Runner test: full engine sweep timed out in -// `list_names_default_limit_100`. -#[ignore = "list_names default limit not applied (returns 1)"] fn list_names_default_limit_100() { common::run(common::TestOpts::new(1).with_timeout(30), |ctx| async move { - let (namespace, _, _runner) = - common::setup_test_namespace_with_envoy(ctx.leader_dc()).await; + let (namespace, _, _runner) = common::setup_test_namespace_with_envoy_for_names( + ctx.leader_dc(), + (0..105).map(|i| format!("actor-{:03}", i)).collect(), + ) + .await; // Create 105 actors with different names to test the default limit of 100 for i in 0..105 { diff --git a/engine/packages/pegboard/src/ops/actor/list_names.rs b/engine/packages/pegboard/src/ops/actor/list_names.rs index e1823959ef..ebc664678f 100644 --- a/engine/packages/pegboard/src/ops/actor/list_names.rs +++ b/engine/packages/pegboard/src/ops/actor/list_names.rs @@ -40,7 +40,7 @@ pub async fn pegboard_actor_list_names(ctx: &OperationCtx, input: &Input) -> Res tx.get_ranges_keyvalues( universaldb::RangeOption { - mode: StreamingMode::Exact, + mode: StreamingMode::Iterator, limit: Some(input.limit), ..(start, end).into() }, From ddad12213b485f903316f2dcd2391abb6213ad21 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 21:41:13 +0000 Subject: [PATCH 2/3] temp: review body file --- review_body.txt | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 review_body.txt diff --git a/review_body.txt b/review_body.txt new file mode 100644 index 0000000000..3a9ec75e49 --- /dev/null +++ b/review_body.txt @@ -0,0 +1,44 @@ +## Code Review: fix(pegboard): use Iterator streaming mode in list_names so limit param is honored + +### Overview + +This PR fixes a bug where the `limit` parameter passed to `pegboard_actor_list_names` was not being honored, causing pagination to break and the default limit to be ignored. The root cause is a single-line change: `StreamingMode::Exact` to `StreamingMode::Iterator` in `list_names.rs`. + +Two previously-skipped tests are re-enabled and updated to use a new `setup_test_namespace_with_envoy_for_names` helper that pre-registers actor behaviors on the envoy before the test body runs. + +--- + +### Bug Fix Assessment + +**Correct.** In FDB's streaming API, `StreamingMode::Exact` tells the client to batch-fetch exactly `limit` items in a single round trip -- it optimizes *fetching* but does not cap the number of items *returned* by the stream. `StreamingMode::Iterator` is the standard paging mode: results are returned incrementally and `limit` is honored as a hard cap. This matches every other user-bounded listing operation in the codebase (`list_for_ns`, `actor/keys.rs`, `envoy/list.rs`, etc.). + +--- + +### Concerns + +**Same-class bug in `runner_config/list.rs`** + +`engine/packages/pegboard/src/ops/runner_config/list.rs:74` uses the identical pattern: + +```rust +mode: StreamingMode::Exact, +limit: Some(input.limit), +``` + +This path is not deprecated and backs the active runner config listing API. It likely has the same broken pagination behavior. `runner/list_names.rs:42` has the same issue but that is the deprecated runner path, so skipping it is more defensible. Worth fixing here or tracking as a follow-up. + +**`StreamingMode::Exact` with `limit: Some(1)` in `workflows/actor/runtime.rs:294`** is fine -- fetching exactly 1 item is the correct use of `Exact`. + +--- + +### Test Changes + +The `setup_test_namespace_with_envoy_for_names` helper is a clean addition. Pre-registering actor behaviors ensures the envoy knows how to handle those actor names; without it, actor creation in the test body was apparently timing out (the removed ignore comment cited a full engine sweep timeout). The actor creation loop in the test body then populates the `ActorNameKey` index, and the list API reads from that index. The two-step setup is correct. + +The `_runner` binding in both re-enabled tests correctly keeps the `TestEnvoy` alive for the duration of the test. + +--- + +### Summary + +The fix is correct and the tests validate the behavior. The one actionable follow-up is auditing `runner_config/list.rs` for the same `StreamingMode::Exact` + limit pagination bug. \ No newline at end of file From 1dc65a8a915177c4214640091cd69c42befd251b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 21:43:28 +0000 Subject: [PATCH 3/3] temp: remove review body file --- review_body.txt | 44 -------------------------------------------- 1 file changed, 44 deletions(-) delete mode 100644 review_body.txt diff --git a/review_body.txt b/review_body.txt deleted file mode 100644 index 3a9ec75e49..0000000000 --- a/review_body.txt +++ /dev/null @@ -1,44 +0,0 @@ -## Code Review: fix(pegboard): use Iterator streaming mode in list_names so limit param is honored - -### Overview - -This PR fixes a bug where the `limit` parameter passed to `pegboard_actor_list_names` was not being honored, causing pagination to break and the default limit to be ignored. The root cause is a single-line change: `StreamingMode::Exact` to `StreamingMode::Iterator` in `list_names.rs`. - -Two previously-skipped tests are re-enabled and updated to use a new `setup_test_namespace_with_envoy_for_names` helper that pre-registers actor behaviors on the envoy before the test body runs. - ---- - -### Bug Fix Assessment - -**Correct.** In FDB's streaming API, `StreamingMode::Exact` tells the client to batch-fetch exactly `limit` items in a single round trip -- it optimizes *fetching* but does not cap the number of items *returned* by the stream. `StreamingMode::Iterator` is the standard paging mode: results are returned incrementally and `limit` is honored as a hard cap. This matches every other user-bounded listing operation in the codebase (`list_for_ns`, `actor/keys.rs`, `envoy/list.rs`, etc.). - ---- - -### Concerns - -**Same-class bug in `runner_config/list.rs`** - -`engine/packages/pegboard/src/ops/runner_config/list.rs:74` uses the identical pattern: - -```rust -mode: StreamingMode::Exact, -limit: Some(input.limit), -``` - -This path is not deprecated and backs the active runner config listing API. It likely has the same broken pagination behavior. `runner/list_names.rs:42` has the same issue but that is the deprecated runner path, so skipping it is more defensible. Worth fixing here or tracking as a follow-up. - -**`StreamingMode::Exact` with `limit: Some(1)` in `workflows/actor/runtime.rs:294`** is fine -- fetching exactly 1 item is the correct use of `Exact`. - ---- - -### Test Changes - -The `setup_test_namespace_with_envoy_for_names` helper is a clean addition. Pre-registering actor behaviors ensures the envoy knows how to handle those actor names; without it, actor creation in the test body was apparently timing out (the removed ignore comment cited a full engine sweep timeout). The actor creation loop in the test body then populates the `ActorNameKey` index, and the list API reads from that index. The two-step setup is correct. - -The `_runner` binding in both re-enabled tests correctly keeps the `TestEnvoy` alive for the duration of the test. - ---- - -### Summary - -The fix is correct and the tests validate the behavior. The one actionable follow-up is auditing `runner_config/list.rs` for the same `StreamingMode::Exact` + limit pagination bug. \ No newline at end of file