Fix SizeLimitedStream cursor over-counting elements across pages

[lengare]

What and why? 🤔

SizeLimitedStream is meant to cap a stream at limit elements across a paginated session, but a limit: 20 served across four pages stream ever served 18: three pages of 5, a starved 4th page of 3, then exhaustion.

Root cause. SizeLimitedStreamCursor::_combine_with used max($a, $b) + 1. The +1 is correct only for the within-page fold (StreamResult::get_combined_cursor), but a typical paginating caller also re-combines a page's already-combined cursor with the incoming request cursor — combining two aggregates — so the +1 double-counts each page boundary once per page. current_size drifts above the true count by pages − 1:

After page	true served	current_size (before)
1	5	5
2	10	11
3	15	17
4	18	21 → next page sees balance ≤ 0

By page 4 the drifted size (17) leaves balance = 20 − 17 = 3, so the page is clamped to 3 and the stream stops short of its own limit.

Fix. Make the cursor's count a true absolute cumulative position and combine idempotently:

• SizeLimitedStream::_enumerate tags each element with current_size + $index + 1 (not a constant + 1).
• SizeLimitedStreamCursor::_combine_with uses plain max (no + 1).

Both changes are required together. After the fix, current_size runs 5 → 10 → 15 → 20 and the stream serves exactly 20.

Testing Steps ✍️

• vendor/bin/phpunit tests/unit/Tumblr/StreamBuilder/Streams/SizeLimitedStreamTest.php
• vendor/bin/phpunit tests/unit/Tumblr/StreamBuilder/StreamCursors/SizeLimitedStreamCursorTest.php
• Confirm the new testEnumerateReachesLimitAcrossPages passes — it walks four equal pages and asserts the cursor's cumulative size is 5 → 10 → 15 → 20 (it fails on the old + 1 arithmetic with 11 !== 10 at page 2).
• On Tumblr's devbox, you can test using the testing instructions provided in https://github.tumblr.net/Tumblr/tumblr/pull/67056

You're it! 👑

@Automattic/stream-builders

[coveralls]

coverage: 80.255%. remained the same — lesianlen/feature/size-limited-stream-cursor-drift into main

[lucila]

Suggested change

	$this->assertSame(0, $result->get_size());
	$this->assertSame(2, $result->get_size());

can we make stream return 3 elements instead of 5 so that we need to fetch more pages. The last page should contain less elements => 2. because the inner stream returns 3 elements each time ,but size limited stream will truncate the results....

pagination would result in:

| page | elements | exhausted | count so far |
|1 | 3 | false | 3 |
|2 | 3 | false | 6 |
|3 | 3 | false | 9 |
|4 | 3 | false | 12 |
|5 | 3 | false | 15 |
|6 | 3 | false | 18 |
|7 | 2 | false | 20 |

[lengare]

Good call — reworked the test to walk 7 pages and assert the truncated final page, matching your table: pages serve 3, 3, 3, 3, 3, 3, 2 for limit: 20, with exactly(7) inner queries.

One adjustment worth flagging: I set the requested page size to 3 (equal to the inner batch) rather than having the inner return fewer than the requested count. SizeLimitedStream computes its own exhaustion as count(derived) < $count, so if we request 5 and the inner returns 3, every short page reports is_exhaustive === true — which would contradict the "exhausted: false" rows in your table and means a real consumer would stop after page 1. Requesting 3 keeps count == derived on full pages, so they stay non-exhausted exactly as charted, and the last page still truncates 3 → 2 against the remaining budget.

(The single-page "inner returns fewer than requested" path is already covered by test_enumerate_less_count.)

[lucila]

can we adjust unit tests?

[lucila]

SizeLimitedStream computes its own exhaustion as count(derived) < $count, so if we request 5 and the inner returns 3, every short page reports is_exhaustive === true — which would contradict the "exhausted: false" rows in your table and means a real consumer would stop after page 1

🤔

shouldn't the exhaust condition depend on the inner stream??

original code

$result = $this->stream->enumerate($count, $cursor->get_inner_cursor(), $tracer, $option);
$elements = $result->get_elements();
...
// if this page size is greater than stream's limit, we don't want to keep paginating.
$is_exhausted = count($derived_elements) < $count || count($derived_elements) >= $this->limit;

suggestion

$result = $this->stream->enumerate($count, $cursor->get_inner_cursor(), $tracer, $option);
$elements = $result->get_elements();
...
// if this page size is greater than stream's limit, we don't want to keep paginating.
$is_exhausted = $result->is_exhaustive() || count($derived_elements) >= $this->limit;

[lucila]

thanks for working on the fix.

Since you are improving SizeLimitedStream, it would be nice to address this early exhaustion issue: #45 (comment)

[lengare]

Good call — applied in fe469c7.