disagg: honor pager early-stop on truncated list

[JaySon-Huang]

What problem does this PR solve?

Issue Number: close #10761

Problem Summary:
When S3::listPrefix (and listPrefixWithDelimiter) callback returns PageResult{.more=false} on a truncated page, the code only breaks the inner object loop. The outer pagination loop may continue without advancing continuation token, repeatedly listing the same page and potentially blocking shutdown paths that wait for background tasks to exit.

What is changed and how it works?

fix(s3): honor pager early-stop on truncated list

Treat pager `more=false` as an explicit termination condition for both
`listPrefix` and `listPrefixWithDelimiter`, so truncated listings do not
re-fetch the same page indefinitely.

Add a regression unit test that creates a multi-page prefix and requests
early stop on the first callback; verify listing exits immediately.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Fix potential infinite pagination loop in S3 listPrefix/listPrefixWithDelimiter when tiflash is stopping.

Summary by CodeRabbit

• Bug Fixes

  • Improved S3 listing pagination so callbacks can stop early and listing stops promptly without traversing remaining pages; adjusted mock client pagination behavior to match production semantics.

• Tests

  • Added tests verifying early termination of both object and prefix listings even when underlying results are truncated.

[pantheon-ai]

Review Complete

Findings: 3 issues
Posted: 3
Duplicates/Skipped: 0

_{ℹ️ Learn more details on Pantheon AI.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Fixes S3 listing pagination so an early-stop callback immediately exits the outer pagination loop on truncated results; also adjusts continuation-token handling and updates MockS3Client pagination behavior. Adds tests verifying early termination for both objects and delimited prefixes.

Changes

Cohort / File(s)	Summary
S3 pagination logic dbms/src/Storages/S3/S3Common.cpp	Removed temporary PageResult propagation across inner loop; introduced local should_continue driven by each pager(...) call to break both inner and outer pagination loops when callback returns more=false. Moved continuation-token advancement to depend on actual truncation completion.
Mock S3 client pagination dbms/src/Storages/S3/MockS3Client.cpp	Reworked ListObjectsV2 to honor MaxKeys, ContinuationToken, and Delimiter more robustly; introduced finalize_page paging logic to set KeyCount, IsTruncated, and NextContinuationToken consistently for both Contents and CommonPrefixes.
Tests for early-stop behavior dbms/src/Storages/S3/tests/gtest_s3client.cpp	Added tests ListPrefixEarlyStopOnTruncatedResult and ListPrefixWithDelimiterEarlyStopOnTruncatedResult that upload >1000 entries to force truncation and assert listPrefix/listPrefixWithDelimiter stop after the first callback when it returns more=false.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant S3Common
    participant S3Service
    Caller->>S3Common: listPrefix(req, pager)
    loop while not done
      S3Common->>S3Service: ListObjectsV2(request)
      S3Service-->>S3Common: page(result, IsTruncated, NextContinuationToken)
      alt process contents/prefixes
        S3Common->>Caller: pager(item)  -- returns PageResult.more
        Caller-->>S3Common: PageResult{more=true/false}
      end
      alt pager returned more=false
        S3Common-->>S3Common: should_continue = false
        S3Common-->>S3Common: break outer loop (stop listing)
      else pager returned more=true and IsTruncated
        S3Common-->>S3Common: set continuation token and continue
      end
    end
    S3Common-->>Caller: return

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰
I hopped through pages, one by one,
A tiny flag said "this is done."
No looping chase, no token spin,
I stop, I rest — the day can win. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'disagg: honor pager early-stop on truncated list' accurately reflects the main bug fix: ensuring early-stop callbacks (more=false) are honored for truncated S3 listings.
Description check	✅ Passed	The description follows the template, clearly states the problem (Issue #10761), explains the fix with a commit message, includes unit test confirmation, and provides a release note.
Linked Issues check	✅ Passed	The changes fully address issue #10761 by implementing the required early-stop termination for both listPrefix and listPrefixWithDelimiter with added regression tests.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the pagination early-stop behavior: S3Common.cpp implements the fix, gtest_s3client.cpp adds regression tests, and MockS3Client.cpp updates pagination logic to support testing.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

dbms/src/Storages/S3/MockS3Client.cpp (1)

198-213: Variable naming uses snake_case rather than camelCase.

The coding guidelines specify camelCase for C++ variables and methods (maxKeys, continuationToken, finalizePage). However, the existing code in this file already uses snake_case (bucket_storage, itr_obj, normalized_prefix), so these additions are consistent with the current file style.

Consider aligning with the guidelines in future refactoring efforts, or updating the file's style consistently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/S3/MockS3Client.cpp` around lines 198 - 213, The new local
variables use snake_case contrary to project guidelines—rename constexpr
default_max_keys to defaultMaxKeys and the locals max_keys and
continuation_token to maxKeys and continuationToken respectively (also update
any subsequent uses in the ListObjectsV2 handling code and checks that reference
request.GetMaxKeys(), request.GetContinuationToken(), and the RUNTIME_CHECK
calls) so the new variables follow camelCase like other symbols in this file.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@dbms/src/Storages/S3/MockS3Client.cpp`:
- Around line 198-213: The new local variables use snake_case contrary to
project guidelines—rename constexpr default_max_keys to defaultMaxKeys and the
locals max_keys and continuation_token to maxKeys and continuationToken
respectively (also update any subsequent uses in the ListObjectsV2 handling code
and checks that reference request.GetMaxKeys(), request.GetContinuationToken(),
and the RUNTIME_CHECK calls) so the new variables follow camelCase like other
symbols in this file.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 46cf9ce9-6a5d-4c6e-a1c6-61a06ff5178e

📥 Commits

Reviewing files that changed from the base of the PR and between 3ed35d4 and fd25de4.

📒 Files selected for processing (2)

• dbms/src/Storages/S3/MockS3Client.cpp
• dbms/src/Storages/S3/tests/gtest_s3client.cpp

✅ Files skipped from review due to trivial changes (1)

• dbms/src/Storages/S3/tests/gtest_s3client.cpp

[JaySon-Huang]

Second-opinion review completed for upstream/master...HEAD on this branch. I did not confirm any new correctness, compatibility, or test-adequacy issues in the current diff.

The updated branch now covers the truncated pagination path in both listPrefix and listPrefixWithDelimiter, and the MockS3 regression tests include explicit truncation prechecks and test-local cleanup.
Source: process/pr-review | Second Opinion

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-03-23 08:03:36.122724113 +0000 UTC m=+169012.158794373: ☑️ agreed by Lloyd-Pottiger.
• 2026-03-23 08:40:33.919369083 +0000 UTC m=+171229.955439373: ☑️ agreed by CalvinNeo.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, JinheLin, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [CalvinNeo,JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JaySon-Huang]

/cherry-pick release-nextgen-20251011

[ti-chi-bot]

@JaySon-Huang: new pull request created to branch release-nextgen-20251011: #10766.

Details

In response to this:

/cherry-pick release-nextgen-20251011

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.