Skip to content

Allow retryable exceptions without a request - #3548

Open
taljeon wants to merge 2 commits into
OpenFeign:masterfrom
taljeon:codex/retryable-exception-without-request
Open

Allow retryable exceptions without a request#3548
taljeon wants to merge 2 commits into
OpenFeign:masterfrom
taljeon:codex/retryable-exception-without-request

Conversation

@taljeon

@taljeon taljeon commented Sep 1, 2026

Copy link
Copy Markdown

Summary

  • add RetryableException overloads that do not require or retain a Request
  • support both cause and no-cause construction using the current millisecond Long retryAfter value
  • verify request absence and preserved retry metadata with focused tests

Testing

  • mvn -pl core -Pdev -Dtoolchain.skip=true -Dtest=RetryableExceptionTest test (5 passed)
  • mvn -pl core -Pdev -Dtoolchain.skip=true verify (731 passed, 4 skipped)
  • mvn clean install -pl core -Pdev -Dtoolchain.skip=true (731 passed, 4 skipped)
  • current-head CircleCI setup and full PR build after merging master (JDK 25)
  • git diff --check
  • publication leak scan (no new findings in changed lines or this PR body)

AI assistance

OpenAI Codex assisted with code navigation, implementation, and validation. I reviewed and verified the final changes.

Fixes #1296

Add no-request overloads for both cause and no-cause paths using the current millisecond retry-after type.

OpenAI Codex assisted with implementation and validation; the final changes were reviewed and verified.

Fixes OpenFeign#1296
@velo

velo commented Sep 4, 2026

Copy link
Copy Markdown
Member

Code review: this is a small, correctly-scoped addition — two new constructors on RetryableException matching what #1296 asked for (building a retryable exception without a Request), reusing the existing hasRequest()/request() accessors with no new nullability surface. No structural issues. The overload growth (12 constructors now, disambiguated by parameter type) is a pre-existing pattern in this file, not something this PR should be asked to fix on its own — worth a follow-up migrating the class to named static factories at some point, not blocking here.

Only holding off on approving because ci/circleci: setup-environment is currently failing on this PR and I don't have visibility into why from here (the API isn't returning step-level logs). Once CI is green this looks mergeable to me — could you take a look at the failure or push an empty commit to retrigger if it looks like a flake?

@taljeon

taljeon commented Sep 4, 2026

Copy link
Copy Markdown
Author

Thanks for flagging this. I traced the old setup failure to the repository's mvnd 1.0.2 download URL. Current master includes #3557, which upgrades the wrapper to 1.0.6 specifically for that broken URL, so I merged current master into this branch and retriggered CI.

The setup job and full PR build are now green on 16dd36c3, and the PR diff remains limited to the two RetryableException source/test files. Would you mind taking another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RetryableException requires Request

2 participants