bug: unbounded pagination

[exploreriii]

🧑‍💻 Intermediate Issue — a complex task spanning multiple modules, with real design decisions to own.
Time: ~25 hours · Prerequisites: comfortable navigating this repo (a completed beginner issue is the usual route; demonstrated CI/CD proficiency substitutes for workflow-focused issues).
We expect more than "it works": maintainable code that fits the existing architecture.

The task

Problem:
pagination.py:56-86 advances with cursor = next_cursor and loops while has_next is true, without ever checking that the cursor actually moved.

This means we could get unbounded requests that burns the rate-limit budget while results grows without bound

The max_pages guard would catch it, but no call site passes it: ../hiero-hackers-analytics/src/hiero_analytics/data_sources/github_ingest/_common.py:109, contributors.py:95 and :138, pull_requests.py:89, hip_references.py:82 all call paginate_cursor(page) bare.

What done looks like:
We should ensure the errors are handled correctly

for example, add the error type next to DEFAULT_PAGE_SIZE:
class CursorStalledError(RuntimeError):
"""The API claimed another page but gave no cursor that advances to it."""

Then ensure this is used appropriately and raised - a transient GitHub hiccup self-heals on the retry, and a persistent one surfaces loudly

We should ensure there are pagination tests and downstream effects are covered

Modules involved / constraints:

How to work on this

1. Claim it: comment /assign and wait to be assigned — unassigned PRs are closed automatically.
2. Get a plan: once assigned, comment @coderabbitai plan for a draft plan, then do your own investigation — docs/architecture.md maps the layers and their rules.
3. Propose your approach as a comment before coding. A paragraph is enough; early feedback here routinely saves days of rework.

🤖 AI: tools are welcome; verified work is required — you can explain every line and defend every design choice. See the AI policy. Fully automated bot PRs are closed.

Worth knowing about this repo before you design:

• The layer rules in docs/architecture.md are strict — review will hold your solution to them.
• Tests mirror src (tests/<pkg>/test_<module>.py), and the output-contract test pins the pipeline output surface — if your change adds or renames outputs, update the contract deliberately.

Before opening your PR:

• I proposed my approach on this issue and incorporated any feedback
• The solution fits the existing architecture and layer rules, and is clear enough for others to debug without me
• Tests cover the happy path, edge cases, and error handling (testing guide)
• I reviewed my own diff line by line; scope is limited to this issue
• Workflow checks pass — CI green, signed commits, linked issue

Stuck? Comment here with what you've tried — see getting help.

[coderabbitai]

Coding Plan

Summary

• Add CursorStalledError(RuntimeError) and a stall guard inside paginate_cursor, so the single point that lacks protection now stops instead of looping without bound.
• Treat both a repeating cursor and a None cursor (while has_next is true) as a stall. This matches the repeating-cursor precedent in batched.py.
• Rely on the existing fetch_all_with_retry machinery to self-heal transient stalls and to surface persistent stalls as PartialOrgFetchError. Unwrapped call paths propagate the error loudly, per the architecture rules. No call-site code changes are required.
• Add unit and downstream tests for the guard, the self-heal path, and the loud-surfacing path, and update the pagination README.

Design Choices

Design Choice 1: Condition that counts as a stalled cursor

Options Considered:

1. Raise only when next_cursor == cursor (the same value repeats).
2. Raise only when next_cursor is None while has_next is true.
3. Raise when either case occurs, because both mean the cursor did not advance.

Chosen Option: 3

Rationale: Both shapes cause an unbounded or non-advancing loop, so both must trigger CursorStalledError. This also matches the repeating-cursor precedent in batched.py.

Design Choice 2: Retry or watermark handling for CursorStalledError

Options Considered:

1. Add a new dedicated retry loop and watermark-hold logic (mirror PartialOrgFetchError) for CursorStalledError.
2. Rely on the existing fetch_all_with_retry machinery for sites 1–4 and on loud propagation for site 5, with no new retry code.

Chosen Option: 2

Rationale: CursorStalledError subclasses RuntimeError, so fetch_all_with_retry (which catches Exception) already retries transient stalls once and converts persistent stalls into PartialOrgFetchError. The unwrapped site 5 paths already propagate loudly to the pipeline boundary, which matches the "fails loudly" architecture rule. Adding a parallel retry/watermark path is scope creep.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Add stall detection to the cursor paginator

This phase adds the new exception and the guard that raises it. It contains the only production behavior change. The change stays inside the data_sources layer, where github_client and the ingest helpers own retry and fault handling.

Task 1: Define CursorStalledError

Introduce the semantic exception that describes a non-advancing cursor.

• Add CursorStalledError(RuntimeError) in src/hiero_analytics/data_sources/pagination.py, placed next to DEFAULT_PAGE_SIZE.
• Give it a one-line Google-style docstring, for example: "The API claimed another page but gave no cursor that advances to it." This satisfies the ruff D convention.
• Subclass RuntimeError to match the existing local convention (OfflineDatasetMissingError, PartialOrgFetchError) and to ensure the existing except Exception retry layer catches it.

Task 2: Add the stall guard to paginate_cursor

Detect a cursor that does not advance and stop the loop by raising.

• In paginate_cursor, after fetch_page(cursor) returns and before reassigning cursor, check the advancement condition when has_next is true.
• Treat the cursor as stalled when next_cursor is None or next_cursor == cursor.
• When stalled, log with logger.error (include the page number and the non-advancing cursor value for diagnostics), then raise CursorStalledError(...) with a message that names the stall.
• Keep the existing max_pages guard and the existing logging unchanged. The stall guard is a separate, always-on protection. max_pages stays an optional debug cap.
• Do not change the five call sites. They keep calling paginate_cursor(page) bare. The new guard applies to all of them automatically.

🤖 Prompt for AI agents

Implement stall detection inside the single-cursor paginator. This is the only
phase with a production behavior change; keep it inside
`src/hiero_analytics/data_sources/pagination.py`.

- Add a new exception class `CursorStalledError(RuntimeError)`, placed next to
the existing `DEFAULT_PAGE_SIZE` constant.
- Give `CursorStalledError` a one-line Google-style docstring, for example: "The
API claimed another page but gave no cursor that advances to it." This satisfies
the ruff `D` convention. Subclass `RuntimeError` to match existing local
exceptions (`OfflineDatasetMissingError`, `PartialOrgFetchError`) and to ensure
the existing `except Exception` retry layer catches it.
- In `paginate_cursor`, after `fetch_page(cursor)` returns and before
reassigning `cursor`, add a check for the advancement condition when `has_next`
is true.
- Treat the cursor as stalled when `next_cursor is None` or `next_cursor ==
cursor`.
- On a stall, log an error with `logger.error`, including the page number and
the non-advancing cursor value for diagnostics, then raise
`CursorStalledError(...)` with a message that names the stall.
- Do not change the existing `max_pages` guard or existing logging. The stall
guard is a separate, always-on protection, independent of the optional
`max_pages` debug cap.
- Do not modify any of the five call sites (`fetch_github_resource` in
`github_ingest/_common.py`, and the fetchers in `contributors.py`,
`pull_requests.py`, and `hip_references.py`). They keep calling
`paginate_cursor(page)` bare; the new guard applies to all of them
automatically.

Phase 2: Tests

This phase proves the guard raises correctly, proves transient stalls self-heal through the existing retry layer, and proves persistent stalls surface. Tests mirror src under tests/.

Task 1: Unit tests for the stall guard

Cover the new behavior directly in the paginator tests.

• Extend tests/data_sources/test_pagination.py with the existing dict-driven / closure fetch_page pattern (no mock library needed).
• Add a test where fetch_page returns has_next=True with a repeating next_cursor equal to the current cursor; assert pytest.raises(CursorStalledError).
• Add a test where fetch_page returns has_next=True with next_cursor=None; assert pytest.raises(CursorStalledError).
• Add a test confirming the normal multi-page path (advancing cursors) still succeeds and does not raise, to guard against false positives.

Task 2: Downstream retry and surfacing coverage

Prove the "self-heals on retry, else surfaces loudly" contract at the call-site layer.

• Add a test that exercises a site-1–4 path (a fetch_all_with_retry-wrapped fetcher, for example in tests/data_sources/test_github_contributor_activity.py or test_github_ingest.py) where the underlying client.graphql returns a stalling response on the first attempt and a valid, terminating response on retry; assert the fetch completes and returns records (transient self-heal).
• Add a test where the stall persists across the retry; assert the failure is folded into PartialOrgFetchError (persistent stall surfaces), matching the existing partial-fetch assertions.
• For site 5 (fetch_github_resource called directly without a wrapper), add a test asserting CursorStalledError propagates out uncaught (surfaces loudly).
• Reuse the existing fake-response helpers and bypass_pagination conventions where possible; for these tests do not bypass paginate_cursor, because the guard under test lives inside it.

🤖 Prompt for AI agents

Add tests that prove the stall guard raises correctly, and that transient stalls
self-heal through retry while persistent stalls surface. Tests mirror `src`
under `tests/`.

- In `tests/data_sources/test_pagination.py`, use the existing dict-driven /
closure `fetch_page` pattern (no mock library needed). Add a test where
`fetch_page` returns `has_next=True` with a repeating `next_cursor` equal to the
current cursor and assert `pytest.raises(CursorStalledError)`. Add a test where
`fetch_page` returns `has_next=True` with `next_cursor=None` and assert
`pytest.raises(CursorStalledError)`. Add a test confirming the normal multi-page
path (advancing cursors) still succeeds and does not raise, to guard against
false positives.
- Add a downstream test that exercises a site-1–4 path (a
`fetch_all_with_retry`-wrapped fetcher, for example in
`tests/data_sources/test_github_contributor_activity.py` or
`test_github_ingest.py`), where `client.graphql` returns a stalling response on
the first attempt and a valid, terminating response on retry. Assert the fetch
completes and returns records (transient self-heal).
- Add a test where the stall persists across the retry, and assert the failure
is folded into `PartialOrgFetchError` (persistent stall surfaces), matching the
existing partial-fetch assertions.
- For site 5 (`fetch_github_resource` called directly without a wrapper), add a
test asserting `CursorStalledError` propagates out uncaught (surfaces loudly).
- Reuse existing fake-response helpers and `bypass_pagination` conventions where
possible. For these tests, do not bypass `paginate_cursor`, because the guard
under test lives inside it.

Phase 3: Documentation

This phase aligns the module documentation with the new behavior.

Update the pagination documentation

Describe the stall guard and the new exception.

• Update the "Pagination Utilities" section of src/hiero_analytics/data_sources/README.md.
• State that paginate_cursor now raises CursorStalledError when a page reports has_next=True but the cursor does not advance.
• Clarify that this is distinct from the max_pages debug cap (a page-count limit, not a stall detector).
• Add a short note that transient stalls self-heal through the existing per-item retry (fetch_all_with_retry), and persistent stalls surface as PartialOrgFetchError (wrapped paths) or propagate to the pipeline boundary (unwrapped paths).

🤖 Prompt for AI agents

Update the pagination documentation to describe the new stall guard and
exception.

- Update the "Pagination Utilities" section of
`src/hiero_analytics/data_sources/README.md`.
- State that `paginate_cursor` now raises `CursorStalledError` when a page
reports `has_next=True` but the cursor does not advance.
- Clarify that this is distinct from the `max_pages` debug cap, which is a
page-count limit, not a stall detector.
- Add a short note that transient stalls self-heal through the existing per-item
retry (`fetch_all_with_retry`), and persistent stalls surface as
`PartialOrgFetchError` on wrapped paths, or propagate to the pipeline boundary
on unwrapped paths.

Research

pagination.py holds two generic helpers: paginate_cursor (the single-cursor loop) and extract_graphql_cursor_page (the GraphQL response reader). It also defines the unused constant DEFAULT_PAGE_SIZE. There are exactly five bare paginate_cursor(page) call sites: one shared engine fetch_github_resource in github_ingest/_common.py (site 5) and four resource fetchers in contributors.py, pull_requests.py, and hip_references.py (sites 1–4). Sites 1–4 always run inside fetch_all_with_retry, which catches Exception, retries once at reduced concurrency, and folds a persistent failure into PartialOrgFetchError. Site 5 is often called directly from pipelines with no retry wrapper. A separate multi-cursor loop in batched.py (_fetch_chunk) already detects a repeating cursor inline and demotes the repo to a per-repo fallback. Custom exceptions in this layer subclass RuntimeError and carry a Google-style docstring (ruff D rules apply, no N naming rules).

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement stall detection inside the single-cursor paginator. This is the only
phase with a production behavior change; keep it inside
`src/hiero_analytics/data_sources/pagination.py`.

- Add a new exception class `CursorStalledError(RuntimeError)`, placed next to
the existing `DEFAULT_PAGE_SIZE` constant.
- Give `CursorStalledError` a one-line Google-style docstring, for example: "The
API claimed another page but gave no cursor that advances to it." This satisfies
the ruff `D` convention. Subclass `RuntimeError` to match existing local
exceptions (`OfflineDatasetMissingError`, `PartialOrgFetchError`) and to ensure
the existing `except Exception` retry layer catches it.
- In `paginate_cursor`, after `fetch_page(cursor)` returns and before
reassigning `cursor`, add a check for the advancement condition when `has_next`
is true.
- Treat the cursor as stalled when `next_cursor is None` or `next_cursor ==
cursor`.
- On a stall, log an error with `logger.error`, including the page number and
the non-advancing cursor value for diagnostics, then raise
`CursorStalledError(...)` with a message that names the stall.
- Do not change the existing `max_pages` guard or existing logging. The stall
guard is a separate, always-on protection, independent of the optional
`max_pages` debug cap.
- Do not modify any of the five call sites (`fetch_github_resource` in
`github_ingest/_common.py`, and the fetchers in `contributors.py`,
`pull_requests.py`, and `hip_references.py`). They keep calling
`paginate_cursor(page)` bare; the new guard applies to all of them
automatically.
===============================================================================

Task: 2

Add tests that prove the stall guard raises correctly, and that transient stalls
self-heal through retry while persistent stalls surface. Tests mirror `src`
under `tests/`.

- In `tests/data_sources/test_pagination.py`, use the existing dict-driven /
closure `fetch_page` pattern (no mock library needed). Add a test where
`fetch_page` returns `has_next=True` with a repeating `next_cursor` equal to the
current cursor and assert `pytest.raises(CursorStalledError)`. Add a test where
`fetch_page` returns `has_next=True` with `next_cursor=None` and assert
`pytest.raises(CursorStalledError)`. Add a test confirming the normal multi-page
path (advancing cursors) still succeeds and does not raise, to guard against
false positives.
- Add a downstream test that exercises a site-1–4 path (a
`fetch_all_with_retry`-wrapped fetcher, for example in
`tests/data_sources/test_github_contributor_activity.py` or
`test_github_ingest.py`), where `client.graphql` returns a stalling response on
the first attempt and a valid, terminating response on retry. Assert the fetch
completes and returns records (transient self-heal).
- Add a test where the stall persists across the retry, and assert the failure
is folded into `PartialOrgFetchError` (persistent stall surfaces), matching the
existing partial-fetch assertions.
- For site 5 (`fetch_github_resource` called directly without a wrapper), add a
test asserting `CursorStalledError` propagates out uncaught (surfaces loudly).
- Reuse existing fake-response helpers and `bypass_pagination` conventions where
possible. For these tests, do not bypass `paginate_cursor`, because the guard
under test lives inside it.
===============================================================================

Task: 3

Update the pagination documentation to describe the new stall guard and
exception.

- Update the "Pagination Utilities" section of
`src/hiero_analytics/data_sources/README.md`.
- State that `paginate_cursor` now raises `CursorStalledError` when a page
reports `has_next=True` but the cursor does not advance.
- Clarify that this is distinct from the `max_pages` debug cap, which is a
page-count limit, not a stall detector.
- Add a short note that transient stalls self-heal through the existing per-item
retry (`fetch_all_with_retry`), and persistent stalls surface as
`PartialOrgFetchError` on wrapped paths, or propagate to the pipeline boundary
on unwrapped paths.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[5affron]

/assign

[exploreriii]

@5affron please confirm if you are still working on this, and if the issue is still relevant given the recent updates