Return Unauthenticated gRPC code for password login failures - #111
Return Unauthenticated gRPC code for password login failures#111johnallers wants to merge 3 commits into
Conversation
… retries Both the OAuth token and password login paths in Initialize() were returning bare errors with no gRPC status code. When these errors bubbled up through fmt.Errorf wrapping, the control plane could not distinguish auth failures from transient errors and would retry indefinitely. Now both paths return codes.Unauthenticated so the sync workflow knows to stop retrying on credential/token errors.
|
Claude finished @johnallers's task in 1m 57s —— View job PR Review: Return Unauthenticated gRPC code on auth failures
Verdict: Looks good -- small, targeted fix that solves a real problem. One consideration worth noting below. What the change doesBoth authentication paths in gRPC status survives wrapping -- verifiedI checked the err := c.Initialize(ctx)
if err != nil {
return nil, fmt.Errorf("salesforce-connector: failed to initialize client: %w", err)
}The Consideration: transient errors during authBoth In practice this is probably the right trade-off: the vast majority of these errors are genuine auth failures (expired tokens, revoked credentials), and the previous behavior of infinite retries on credential errors was significantly worse. But it's worth knowing this exists -- if you see connectors failing on transient network blips during initialization, this is where to look. A future refinement could inspect the error type (e.g.,
|
|
Claude finished @johnallers's task in 1m 14s —— View job PR Review: Return Unauthenticated gRPC code on auth failures
Verdict: Approve -- clean, minimal fix that correctly addresses the infinite retry problem. What the change doesBoth auth paths in gRPC status propagation -- verifiedI traced the full call chain:
Code quality
Consideration: transient errors during authBoth This is likely the right trade-off -- the vast majority of these failures are genuine auth errors, and the previous behavior of infinite retries on bad credentials was strictly worse. If transient auth failures become a problem, a future refinement could inspect the error type (e.g., SummarySmall, correct fix. The gRPC status propagates correctly through all call paths, and the retry logic will now correctly stop on auth failures. |
The OAuth token source already returns properly coded gRPC errors from the control plane (e.g. FailedPrecondition). Overriding to Unauthenticated would mask the original code and prevent transient errors from being retried. Only the password login path needs Unauthenticated since LoginPassword returns bare errors with no gRPC code.
johnallers
changed the title
|
Claude finished @johnallers's task in 1m 14s —— View job PR Review: Return Unauthenticated gRPC code on auth failures
Verdict: Approve -- clean, well-reasoned fix. The commit history shows good iteration. What changedThe Commit history shows good judgmentThe evolution across three commits is worth noting:
This is the right call. Only gRPC status propagation — verifiedI traced all callers of
Code quality
No issues foundThis is a small, correct fix. No bugs, no security concerns, no regressions. |
Dismissing — reviewed a stale PR by mistake.
5 participants
Summary
Initialize()returns bareSalesforceErrorwith no gRPC status code, causing the sync workflow to treat auth failures (e.g. deactivated user) as transient and retry indefinitelycodes.Unauthenticatedso the sync workflow stops retrying on credential errorsTokenSource.Token()already returns properly coded gRPC errors from the control plane (e.g.FailedPrecondition), and overriding would mask transient errors that should be retriedTest plan
go build ./...passesgo test ./...passes (28 tests across 6 packages)🤖 Generated with Claude Code