[GitHub] Propagate fetch errors instead of swallowing them

[Jan-Kazlouski-elastic]

Summary

Fixes the GitHub connector silently swallowing errors during sync. Previously each fetch method wrapped its whole body in try/except Exception: log warning, so a sync could "succeed" while syncing nothing (e.g. if no issues could be fetched at all). This implements the two-tier error handling requested in the issue, removes the broad exception handling, and hardens null-handling of GraphQL responses.

Part of #2393, closes #2395.

Changes

Two-tier error handling (datasource.py)

• Page/API-level failures propagate out of _fetch_repos / _fetch_pull_requests / _fetch_issues / _fetch_files / _fetch_files_by_path, so the sync fails when data genuinely can't be fetched.
• Per-document enrichment failures are WARN-logged and skipped via narrow guards that catch only gidgethub.GitHubException, so a single bad issue/PR/file no longer aborts the whole sync.

Transient rate limiting

• _put_to_sleep now only sleeps; its callers re-raise a dedicated RateLimitingError, which is not in skipped_exceptions, so the existing @retryable layer retries it.

No more bare except Exception

• Replaced across the github/ package with gidgethub.GitHubException (plus the connector's own UnauthorizedException / ForbiddenException in log-and-reraise spots like ping and _get_invalid_repos_for_personal_access_token).

Null-safe GraphQL response handling

• GitHub can return null for connection fields (reviews, comments, labels, assignees, reviewRequests, tree, edge nodes). The old x.get("key", {}).get(...) pattern crashes with AttributeError/TypeError when the key is present but its value is null (.get() only applies the default when the key is absent). Switched the fetch/prepare helpers to the ... or {} / ... or [] idiom so a single such document can no longer abort the sync.

Tests

• Added coverage for propagation vs. per-item skip for each fetch method, RateLimitingError propagation, and regression tests for issues/PRs with null connection fields.
• Fixed a latent test-isolation bug where shared mock fixtures were mutated across tests and the resulting error was hidden by the old catch-all (now deepcopy).

Acceptance criteria

• GitHub connector fails syncs if no issues can be synced (page-level errors propagate)
• GitHub connector attempts retries for errors that look transient (@retryable + RateLimitingError)
• Errors with limited scope (single documents / edge cases) are moved past with WARN logging

Test plan

• pytest tests/sources/test_github.py — 121 passed
• Passes across multiple random orderings (-p randomly seeds 1/2/42/100/777)
• ruff check clean on changed files

Notes for reviewers

• The per-item guard wraps _fetch_remaining_fields, which itself makes paginated calls. The design assumes systemic backend failures surface on the initial page fetch (which propagates and fails the sync). If a systemic error only manifested during per-item enrichment, it could be WARN-skipped for every document without failing the sync. Happy to add per-repo "synced at least one" tracking if we want a hard guarantee.
• raise Exception(msg) remains for non-rate-limited GraphQL QueryErrors in graphql(); left as-is as a possible small follow-up.

[Jan-Kazlouski-elastic]

@artem-shelkovnikov
Something worth flagging is that this will now catch only GitHubException and write a warning for them without failing the whole sync, but any non-GitHubException will now fail the whole sync.
I agree that we shouldn't be catching General Exception, but that is a risk.

[artem-shelkovnikov]

Did you do any testing in regards how errors are raised? I remember this connector was tricky because I believe under the hood the github library uses aiohttp and with certain usage of the library it raises aiohttp errors instead of its own ones.

I think we've fixed it at some point and only gidgethub errors are now exposed, but might be worth checking - like just configuring connector with wrong config, then with wrong repo name and see if the error is correctly propagated.

What do you think?

[Jan-Kazlouski-elastic]

Tested end-to-end against a local mock and live api.github.com.

gidgethub only maps HTTP status codes to GitHubExceptions; pre-response
failures (DNS, refused, TLS, timeout) come from aiohttp and aren't
GitHubExceptions.

• Bad token (local + real api) → UnauthorizedException ✅
• Wrong repo → 404 → gidgethub.BadRequest, propagates ✅
• Page-level 500 → gidgethub.GitHubBroken, propagates ✅
• Per-item 500 → WARN + skipped ✅
• Dead port → aiohttp.ClientConnectorError (not a GitHubException)

So errors propagate correctly. Transport errors dodge the per-item
guard, but @retryable (3×) absorbs blips and a persistent outage should fail
the sync. Extending guard to aiohttp errors would
reintroduce #2395 (skip every doc, sync "succeeds" empty).

[github-actions]

💔 Failed to create backport PR(s)

Status	Branch	Result
✅	9.3	#4134
✅	9.4	#4135
❌	8.19	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 4119 --autoMerge --autoMergeMethod squash