Skip to content

fix: report a missing refresh token as AuthException - #297

Open
mlwelles wants to merge 1 commit into
fix/async-client-non-blocking-retriesfrom
fix/typed-auth-error-for-missing-refresh-token
Open

fix: report a missing refresh token as AuthException#297
mlwelles wants to merge 1 commit into
fix/async-client-non-blocking-retriesfrom
fix/typed-auth-error-for-missing-refresh-token

Conversation

@mlwelles

@mlwelles mlwelles commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Stacked on #294 ("fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation") as GitHub stack #298. The base is that branch, not main, because #294 rewrote the exact lines this changes. Read #294 first; the diff here is just this commit.

Merge path

Draft until the test suite has actually run on it. ci-dgraph4j-tests.yml and trunk.yml both gate their pull_request trigger on branches: main, so while this PR targets a feature branch it gets CodeQL but neither the test suite nor the linters. GitHub still reports it MERGEABLE and CLEAN, because absent checks cannot fail — judge it by which checks are present, not by its merge state.

Merging bottom-up closes that gap with no changes to CI:

  1. Merge fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation #294. It has a full green run against main.
  2. This PR is retargeted to main automatically. The repo has delete_branch_on_merge enabled, and GitHub retargets open PRs based on a deleted head branch to the merged PR's base.
  3. Mark this PR ready for review. ready_for_review is in the workflow's trigger list, so with the base now main the full suite and the linters run here.
  4. Merge once green.

gh stack merge 298 would instead land both PRs atomically, which is faster but merges this one without the suite ever running on it — the post-merge push: main run would catch a problem after the fact rather than before. #299 ("ci: run tests and linters on pull requests to any base branch") removes the base-branch filter that causes this, and once it merges the checks appear here and either path is validated. Until then, prefer the sequence above.

Description

retryLogin() signalled "no refresh JWT available" with a bare java.lang.Exception. Exceptions.translate has no case for that, so it fell through to the generic wrapper and the caller saw a DgraphException reporting INTERNAL — an authentication failure presented as an internal error. It is the one gap left in the typed exception hierarchy added in 25.0.0, which already defines AuthException for exactly this condition.

This fails with AuthException carrying Status.UNAUTHENTICATED instead. translate passes DgraphException subclasses through untouched, so it reaches the caller intact.

Compatibility

  • Retryability is unchanged. AuthException inherits isRetryable() == false, exactly as the generic DgraphException did, so withRetry behaves identically.
  • catch (DgraphException) and catch (StatusRuntimeException) are unaffected — AuthException is a subclass of both.
  • Only code that distinguishes AuthException sees a difference, which is the point of the change.
  • The status code moves from INTERNAL to UNAUTHENTICATED. Code switching on the gRPC status code of this specific failure would observe that.

Tests

RetryLoginTest covers two cases, neither needing a server, since the missing-token check returns before any RPC is issued:

Test What it pins
refreshWithoutATokenFailsWithAuthException the direct call fails with AuthException and UNAUTHENTICATED
expiredTokenWithNoRefreshSurfacesAuthException the path a user hits — an expired token with no way to refresh it — surfaces AuthException through runWithRetries

Both fail before this change: the first sees a raw java.lang.Exception, the second sees DgraphException: INTERNAL.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

retryLogin() signalled "no refresh JWT available" with a bare
java.lang.Exception. Exceptions.translate has no case for that, so it fell
through to the generic wrapper and the caller saw a DgraphException reporting
INTERNAL -- an authentication failure presented as an internal error, and the one
gap left in the typed exception hierarchy added in 25.0.0.

Fail with AuthException carrying Status.UNAUTHENTICATED instead. translate passes
DgraphException subclasses through untouched, so this reaches the caller intact.
Retryability is unchanged: AuthException inherits isRetryable() == false, exactly
as the generic DgraphException did.

Callers catching DgraphException or StatusRuntimeException are unaffected. Only
code that distinguishes AuthException sees a difference, which is the point.

Add RetryLoginTest, covering both the direct refresh call and the path a user
actually hits -- an expired token with no way to refresh it, surfacing through
runWithRetries. Neither test needs a server: the missing-token check returns
before any RPC. Both fail before this change.
@mlwelles
mlwelles requested a review from a team as a code owner August 6, 2026 14:18
@mlwelles
mlwelles marked this pull request as draft August 6, 2026 14:26
@mlwelles

mlwelles commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

No CI ran on this PR, and the green state is misleading. Marking it draft until that is fixed.

Both workflows are gated on the base branch:

  • ci-dgraph4j-tests.yml: pull_request: branches: [main]
  • trunk.yml: pull_request: branches: main

This PR targets fix/async-client-non-blocking-retries, so neither triggered. Nothing ran — not the test suite, not the linters. GitHub still reports mergeable: MERGEABLE and mergeStateStatus: CLEAN, because with no checks present there are none to fail. Do not read that as validated.

What I verified locally instead

  • ./gradlew compileJava compileTestJava succeeds.
  • RetryLoginTest passes across three runs, and both of its assertions fail when the change is reverted (one sees a raw java.lang.Exception, the other DgraphException: INTERNAL).
  • trunk fmt and trunk check are clean on all three changed files.
  • The cluster-dependent tests could not run: they need a dgraph/dgraph:local image built from Dgraph source, and I have no Docker daemon available. This change touches one exception type in retryLogin() and no transaction or query path, and fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation #294 ("fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation") already has a green full-suite run on the parent commit.

To land this

Merge #294 first, then retarget this PR to main. The commit applies cleanly on top of merged-#294 code, and CI will run on the retargeted PR. Please do not merge it while it targets a feature branch.

Separately: widening those two triggers (for example to branches: [main, 'fix/**']) would let stacked PRs be validated at all. That is a repo policy change with a real cost — every stacked PR would build Dgraph from source — so I have not made it here.

@mlwelles
mlwelles marked this pull request as ready for review August 6, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant