Repository navigation
[fix](ci) Retry transient review GET failures within bounded budgets - #69510
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69469 Problem Summary: A transient GitHub 503 aborts review input resolution, and a 504 during pagination aborts aggregate status resolution. The shared gh helper currently makes one attempt even when GitHub supplies Retry-After. Input resolution failures also leave the failure notification without a PR number. Retry only recognized body-free REST GETs on 502/503/504, with at most three attempts, 30 seconds per attempt and a shared 90-second or shorter caller budget. Respect Retry-After and retain diagnostics for every failure. Discard failed pagination output and return only a successful attempt's complete output. Writes, GraphQL, other HTTP errors and transport/parse failures are not retried. Use the trigger's numeric PR identity for failure notices when resolved outputs are absent, specify the repository, and identify the input-resolution failure. Unresolved head/base values still cannot authorize status publication. ### Release note None ### Check List (For Author) - Test: Unit tests and real gh loopback integration; 281 tests completed on macOS, 262 passed and 19 Linux-only cases skipped. actionlint and diff-check passed; actionlint excludes its pre-existing concurrency.queue diagnostic. Linux supervision/auth tests: 101 passed, 2 failed on both this branch and unchanged master due to old fixtures missing required submission fields. - Behavior changed: Yes, transient read failures receive bounded retries and input-resolution failures can report to the original PR. - Does this need documentation: Yes, shared helper behavior is updated in .github/scripts/review-github-diagnostics.md in this change.
shuke987
requested review from
hello-stephen,
morningman and
yiguolei
as code owners
October 23, 2026 06:43
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all six changed files at head d1c72c478f46d79209d11a4bbb554456dc320ad0, including upstream callers, downstream consumers, negative paths and tests. No actionable new defect was identified; there are no existing inline findings to carry forward.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Eligible transient GET failures receive bounded retries; early input failures retain a PR notification target and specific terminal cause. Added tests cover both behaviors; their execution was not repeated in this review. |
| Scope, clarity and reuse | Changes stay in the shared API helper, failure handling, documentation and tests; retry logic is centralized. |
| Concurrency and locks | Attempts, waits and watchdog reads remain synchronous. No new shared retry state or lock ordering; diagnostic filenames remain unique. |
| Lifecycle | Each subprocess attempt is reaped before retry. The trusted helper is installed before input resolution and survives the PR checkout. No new static initialization or resource ownership issue was identified. |
| Configuration | No user configuration or dynamic setting is added; environment bindings carry existing event/step values. |
| Compatibility | Plain, paginated and Accept-header GET callers retain complete-success output. The 30-second attempt cap and shared operation deadline are explicit policy. No storage or symbol compatibility change applies. |
| Thrift/protobuf | No definitions or field IDs changed. |
| Parallel paths | Traced runner, aggregator, result sync, submission and watchdog paths. Unknown argument forms, including existing --jq reads, intentionally retain single-attempt behavior; existing direct resume reads are outside the shared helper. |
| Conditional checks and errors | Only recognized body-free REST GETs with transient GET response evidence retry. Exhaustion, invalid/excessive Retry-After, timeouts, signals, transport failures and JSON parse failures remain failures. Input fallback rejects invalid PR identities. |
| Test coverage | Static inspection covers transient/non-transient responses, argument exclusions, server waits, total/caller budgets, pagination restart/exhaustion, write isolation, cancellation and notification identity/error paths. |
| Changed test results | No recorded expected-output files changed. The adjusted submission assertion permits positive timeout values within the caller budget. |
| Observability | Every failed attempt retains bounded diagnostics and request context; retry logs state delay/attempt, and both failure outputs identify input-resolution failure. Existing artifact collection remains available. |
| Transactions and persistence | No Doris transaction, EditLog, persistence or failover logic changed. |
| Writes and atomicity | GitHub writes remain single-attempt. Review submission deduplication/readback and resolved-head/base status guards are preserved; notification fallback cannot publish an unresolved review status. Failed paginated stdout is discarded before retry or propagation. |
| FE/BE propagation | No FE/BE variable or protocol changes. |
| Performance and memory | Retries add at most two failed-read waits/attempts within the shared deadline. No new background work or unbounded retained diagnostic data was introduced. Native memory/data-visibility/delete-bitmap invariants are unaffected. |
| Other issues | The initial retry-budget/parser risk was checked against actual callers and tests; no substantiated issue or unresolved candidate remained. |
User focus: no additional focus was supplied. Coverage limits: this was a static review; no builds, tests, loopback checks or live Actions recovery were executed. Runtime success and network timing are not claimed.
Review converged after 2 round(s).
Contributor
Author
|
skip buildall |
hello-stephen
approved these changes
Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: #69469
Problem Summary: A single transient GitHub response currently aborts the review pipeline. Input resolution for #69322 stopped on a GET 503 despite
Retry-After: 2; aggregate resolution for #69438 stopped on a paginated GET 504 after emitting partial output.The shared helper now retries recognized body-free REST GETs on 502/503/504, for at most three attempts. Each attempt has a 30-second limit; attempts and waits share a 90-second budget or the caller's shorter timeout. It respects Retry-After, records every failed attempt, discards failed pagination output, and returns only a successful attempt's complete output. Exhaustion still fails. Writes, GraphQL, unknown argument forms, other HTTP errors, transport/timeouts and JSON parsing errors are not retried.
Input resolution can also fail before publishing its PR-number output. Failure notices now fall back to the authorized trigger's numeric PR identity, explicitly select the repository, and name the input-resolution failure in both the notice and terminal error. This fallback does not permit status publication for an unresolved head/base.
This change can merge independently. It includes the same explicit
--repoargument as #69469; a local combination with #69469 and #69504 merged cleanly and passed the macOS suite.Release note
None
Check List (For Author)
PYTHONDONTWRITEBYTECODE=1 RUN_GH_HTTP_TESTS=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py'— 281 tests, 262 passed / 19 Linux-only skips on macOS.0d777cd0791553da6f271cb7a62811aa165832fc:test_capacity_after_submission_is_success_without_auth_quarantineandtest_success_is_not_quarantined_even_with_earlier_stderr_error. Both old fixtures omit fields required byvalidate_submission; neither reaches a new retry failure.git diff --check; actionlint disables unavailable ShellCheck/Pyflakes integrations and filters its existing unsupportedconcurrency.queuediagnostic..github/scripts/review-github-diagnostics.mdin this PR.Check List (For Reviewer who merge this PR)