Fixes #33190: Stop Mode report pagination on unseen-report exhaustion

[harshsoni2024]

Describe your changes:

Fixes #33190

Hardens the pagination added for #22559; the defects below were found in review
of the 1.13 backport, so they get their own issue rather than reopening a closed one.

#32533 paginated Mode report discovery so that reports past the first page of a
space are ingested. Its termination condition — len(reports) < REPORTS_PAGE_SIZE
with REPORTS_PAGE_SIZE = 30 — makes two assumptions about Mode's API that the
API does not guarantee, both of which lose reports. Review on the 1.13 backport
(#33147) raised them; this is the same fix on main so the two branches carry
one pagination contract.

Silent truncation. If a report page ever holds fewer than 30 records, the
first page satisfies len(reports) < 30, the walk stops, and the remainder of
that space is dropped with no warning and no error — reproducing the very
missing-reports symptom #22559 was filed about. The page size is undocumented
and is not pinned with a per_page parameter, so a server-side default change
silently regresses the connector.

Hard abort. A guard added during review raised RuntimeError when a page
repeated. Many paginated REST APIs clamp an out-of-range page to the last page
and re-return it instead of returning an empty list. A space holding an exact
multiple of 30 reports therefore takes the page += 1 branch, receives the same
page back, and raises. fetch_all_reports is called from get_dashboards_list
without a guard, so that exception kills the whole Mode source and drops every
dashboard of every space — a benign input destroying an entire ingestion run.

Pagination now stops once a page carries no report unseen in that space.

Type of change:

• Bug fix

High-level design:

One condition replaces both heuristics:

new_reports = [report for report in reports if _report_key(report) not in seen_reports]
if not new_reports:
    break

It terminates on every case the old code handled and the two it mishandled:

Server behaviour	Old	New
Empty page past the last	stops	stops
Page size below 30	drops the rest of the space	keeps paging
Out-of-range page clamped to the last	raises, kills the run	stops, keeps the reports
page parameter ignored entirely	raises	stops
Pages overlap after a mid-walk insert	duplicates rows	keeps the first copy

Page size is no longer consulted, so REPORTS_PAGE_SIZE is removed rather than
re-tuned — the failure mode is structurally gone, not calibrated against a
guessed constant.

_report_key de-duplicates by report token and falls back to a sorted JSON dump
of the record when Mode omits one, so token-less pages neither collide with each
other nor loop forever.

Cost: one extra request per space, always — the terminal empty or repeated
page. The old code already paid that whenever a space's report count was an
exact multiple of 30. seen_reports is a per-space working set, reset each
space and bounded by that space's report count, which is the same order as the
all_reports accumulator that already existed.

Alternative considered: following _links.next from Mode's HAL response
would be contract-driven rather than heuristic, and the client already reads
_links for share and report_viz_web. It is the better long-term shape, but
confirming the pagination link's presence needs a live Mode Business workspace,
which is unavailable here. The unseen-report condition needs no knowledge of the
response envelope, so it is correct without that confirmation and does not block
moving to _links.next later.

ingestion/.basedpyright/baseline.json drops three suppressed diagnostics that
the removed code carried (9 → 6 for mode/client.py). Verified structurally
that no other file key changed: 809 keys before and after, none added or
removed.

Tests:

Use cases covered

• A space whose pages hold fewer than 30 records is walked to the end instead of
  stopping after page one
• A last page re-returned because Mode clamped the out-of-range page ends the
  walk and keeps its reports, instead of aborting the source
• A page parameter the API ignores terminates instead of looping forever
• Pages that overlap after a mid-walk insert contribute each report once
• Multiple spaces, each paged independently to its terminal page
• Reports with no token, across distinct pages and across a repeated page
• A page request that fails mid-walk still propagates
• An invalid filterQueryParam still fails fast with a clear message

Unit tests

• I added unit tests for the new/changed logic.
• File: ingestion/tests/unit/source/dashboard/mode/test_client.py — rewritten
  around the new contract, 6 → 8 tests. test_fetch_all_reports_rejects_repeated_full_page
  is deleted because it asserted the removed RuntimeError;
  test_..._requests_page_after_exactly_thirty_results is folded into the
  multi-space test, which now asserts the terminal page request per space.
• Both defects have a named regression test:
  test_fetch_all_reports_keeps_paging_after_a_page_smaller_than_thirty (two
  25-record pages return all 50; the old code returned 25) and
  test_fetch_all_reports_stops_when_a_page_repeats (a clamped repeat returns
  30 reports instead of raising).
• Result: 42 passed — Mode client (8), Mode connection (3), Mode topology
  (17), plus burstiq (10) and qlikcloud (4) run alongside so any test-module
  name collision would surface.
• Changed-line coverage on client.py: 100% — all 22 changed/added lines are
  executed; cross-referenced --cov-report=term-missing against the diff's line
  ranges.

Backend integration tests

• Not applicable (no backend API changes).

Ingestion integration tests

• Not applicable. A live Mode Business workspace and credentials are
  unavailable, so the Mode HTTP boundary stays covered with fixtures. The change
  is deliberately shaped to need no knowledge of the response envelope beyond
  _embedded.reports, which the connector already depended on.

Playwright (UI) tests

• Not applicable (no UI changes).

Manual testing performed

Commands run on this branch, with their actual results:

1. pytest tests/unit/source/dashboard/mode/ tests/unit/topology/dashboard/test_mode.py tests/unit/source/database/burstiq tests/unit/source/dashboard/qlikcloud — 42 passed
2. pytest ... --cov=metadata.ingestion.source.dashboard.mode --cov-report=term-missing — used to confirm every changed line in client.py is covered
3. ruff check ... --config pyproject.toml — no issues
4. python scripts/check_ruff_suppressions.py --check — exit 0
5. ruff format --check ... --config pyproject.toml — already formatted
6. basedpyright src/metadata/ingestion/source/dashboard/mode/client.py — 0 errors, 0 warnings, 0 notes
7. Pre-commit on the commit — check json / ruff check / ruff format all passed

Not performed: a live Mode ingestion run, for lack of workspace credentials.

UI screen recording / screenshots:

Not applicable — no UI changes.

Checklist:

• I have read the CONTRIBUTING document.
• My PR title is Fixes <issue-number>: <short explanation>
• My PR is linked to a GitHub issue via Fixes #<issue-number> above.
• I have commented on my code, particularly in hard-to-understand areas — the
  pagination contract and why it cannot rely on page size is documented on
  fetch_all_reports.
• For JSON Schema changes: not applicable, no schema change.
• For UI changes: not applicable.
• I have added tests (unit / integration / Playwright as applicable) and listed them above.
• I have added a test that covers the exact scenario we are fixing.

[github-actions]

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

[Copilot]

🟡 Changes recommended

A critical comment remains unresolved because repeated non-empty pages can silently cause partial ingestion without warning.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes Mode report pagination to prevent report loss and failures caused by undocumented page-size behavior.

Changes:

• Tracks unseen reports per space and handles repeated pages safely.
• Adds regression tests for short, overlapping, and repeated pages.
• Removes obsolete type-check suppressions.

File summaries

File	Summary
ingestion/tests/unit/source/dashboard/mode/test_client.py	Adds pagination and de-duplication regression coverage.
ingestion/src/metadata/ingestion/source/dashboard/mode/client.py	Implements unseen-report pagination termination.
ingestion/.basedpyright/baseline.json	Removes obsolete diagnostics.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[harshsoni2024]

Code Review 👍 Approved with suggestions 0 resolved / 1 findings
Hardens Mode report pagination to stop on unseen-report exhaustion instead of relying on page-size heuristics, fixing silent truncation when pages hold fewer than 30 records and hard aborts when out-of-range pages are clamped. All 42 tests pass with 100% coverage of changed lines. Consider adding a defensive maximum-page cap as a backstop in case Mode returns full pages of genuinely distinct records indefinitely, since the old page-size heuristic previously bounded the walk implicitly.

💡 Edge Case: No absolute page cap on the pagination loop
📄 ingestion/src/metadata/ingestion/source/dashboard/mode/client.py:114-126

The while True loop now terminates only when a page contributes no unseen report. This is robust for token-bearing reports, but if Mode (or a proxy) ever returns full pages of genuinely distinct records indefinitely — e.g. tokenless reports carrying a volatile field that varies per request, defeating the json.dumps key — the loop never exits and hangs the ingestion. Consider adding a defensive maximum-page cap (with a logged warning on hit) as a backstop, since the old page-size heuristic previously bounded the walk implicitly.

🤖 Prompt for agents

Code Review: Hardens Mode report pagination to stop on unseen-report exhaustion instead of relying on page-size heuristics, fixing silent truncation when pages hold fewer than 30 records and hard aborts when out-of-range pages are clamped. All 42 tests pass with 100% coverage of changed lines. Consider adding a defensive maximum-page cap as a backstop in case Mode returns full pages of genuinely distinct records indefinitely, since the old page-size heuristic previously bounded the walk implicitly.

1. 💡 Edge Case: No absolute page cap on the pagination loop
   Files: ingestion/src/metadata/ingestion/source/dashboard/mode/client.py:114-126

   The `while True` loop now terminates only when a page contributes no unseen report. This is robust for token-bearing reports, but if Mode (or a proxy) ever returns full pages of genuinely distinct records indefinitely — e.g. tokenless reports carrying a volatile field that varies per request, defeating the json.dumps key — the loop never exits and hangs the ingestion. Consider adding a defensive maximum-page cap (with a logged warning on hit) as a backstop, since the old page-size heuristic previously bounded the walk implicitly.

Options
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Compact

gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Thanks for the review,
The old code had the same exposure. Under "full pages of genuinely distinct
records", reports == previous_reports was never true and len(reports) < 30
was never true, so the pre-fix loop never exited either — the page-size
heuristic bounded the walk only when the server returned a short page, not
this case. So it's not a regression here.

The trigger also needs token-less reports, which the connector can't process
regardless: metadata.py hard-indexes dashboard[TOKEN] in both
get_dashboard_name and get_dashboard_details, so such a report KeyErrors a
few frames later whether or not pagination terminates.

I'd rather not add a page cap — a MAX_PAGES constant reintroduces exactly the
silent-truncation class this PR removes. If we want the hole closed, the cleaner
fix is dropping the json.dumps fallback and skipping token-less reports with a
warning, so dedupe is purely token-based.

[gitar-bot]

Code Review ✅ Approved 1 resolved / 1 findings

Hardens Mode report pagination to stop on unseen-report exhaustion instead of relying on page-size heuristics, fixing silent truncation when pages hold fewer than 30 records and hard aborts when out-of-range pages are clamped. All 42 tests pass with 100% coverage of changed lines. Consider adding a defensive maximum-page cap as a backstop in case Mode returns full pages of genuinely distinct records indefinitely, since the old page-size heuristic previously bounded the walk implicitly.

✅ 1 resolved

✅ Edge Case: No absolute page cap on the pagination loop

📄 ingestion/src/metadata/ingestion/source/dashboard/mode/client.py:114-126
The while True loop now terminates only when a page contributes no unseen report. This is robust for token-bearing reports, but if Mode (or a proxy) ever returns full pages of genuinely distinct records indefinitely — e.g. tokenless reports carrying a volatile field that varies per request, defeating the json.dumps key — the loop never exits and hangs the ingestion. Consider adding a defensive maximum-page cap (with a logged warning on hit) as a backstop, since the old page-size heuristic previously bounded the walk implicitly.

Options

Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

✅ Playwright Results — workflow succeeded

Validated commit 7fe3a8b24cac482d024b9acc61bf8c9293674a08 in Playwright run 34575924876, attempt 1.

✅ 107 passed · ❌ 0 failed · 🟡 2 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 51m 15s

⏱️ Max setup 4m 29s · max shard execution 15m 13s · max shard-job elapsed before upload 21m 18s · reporting 6s

🌐 235.44 requests/attempt · 1.79 app boots/UI scenario · 0.00% common-shard skew

Optimization targets still in progress:

• Browser traffic was 235.44 requests per attempt (convergence target: fewer than 200).
• Application boot ratio was 1.79 per UI scenario (226 boots / 126 scenarios; convergence target: at most 1).

Shard	Passed	Failed	Flaky	Skipped	Lifecycle failed	Lifecycle flaky
✅ Shard chromium-01	46	0	0	0	0	0
🟡 Shard ingestion-01	28	0	2	0	0	0
✅ Shard ingestion-02	33	0	0	0	0	0

🟡 2 flaky test(s) (passed on retry)

• Features/IncidentManager.spec.ts › Complete Incident lifecycle with table owner (shard ingestion-01, 1 retry)
• Features/TestSuitePipelineRedeploy.spec.ts › Re-deploy all test-suite ingestion pipelines (shard ingestion-01, 1 retry)

📦 Download artifacts

How to debug locally

# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace