fix: Retry a Salesforce request once after refreshing the token - #70
Open
keelerm84 wants to merge 1 commit into
Open
fix: Retry a Salesforce request once after refreshing the token#70keelerm84 wants to merge 1 commit into
keelerm84 wants to merge 1 commit into
Conversation
requestWithOauth detected a 401 or 403, refreshed the OAuth token, and then returned the original rejection without using the new token. The caller saw a non-200, logged it and waited out a whole poll interval, so every routine Salesforce session expiry cost a full cycle of stale flag data or undelivered events. It now sends the request a second time with the refreshed token and returns that response. The retry is not repeated. A token minted seconds ago being refused is not an expiry, so another refresh cannot change the answer -- it means the connected app's permissions or its run-as identity changed. The second response is returned as it stands, which keeps a standing misconfiguration from turning into unbounded requests against the org's API allocation. This change adds no backoff and no general retryable-status classification; the only retry is the one that follows a successful refresh. The flag push now builds its body with bytes.NewReader instead of bytes.NewBuffer, and requestWithOauth replays the body from Request.GetBody before each send. Both readers give http.NewRequest enough to build a GetBody, so either replays correctly, but a Reader is a read-only view of the polled bytes where a Buffer is a writable staging area that http.NewRequest snapshots once -- so the Reader states at the call site the guarantee the retry depends on. Without the explicit replay the second attempt fails only intermittently: the transport covers for a missing body by rewinding through GetBody itself, but only when the attempt went out on a pooled connection, so the failure appears just when the retry has to dial. The rejection is now drained and closed on both paths. Previously, when the refresh that followed a 401 failed transiently, requestWithOauth returned a nil response and the rejection was released by nobody; eventLoop and featureLoop treat a non-permanent error as recoverable, so a bridge leaked a connection every cycle for as long as the refresh kept failing. Now that no rejection is handed back to the caller, requestWithOauth is the only place either path can release one.
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.
Summary
The bridge only learns its Salesforce token has expired by having a request refused,
because Salesforce ends the session on its own schedule.
requestWithOauthhandled thatnotification and then discarded the benefit: it refreshed the token and returned the
original 401 to the caller, leaving the fresh token unused until the next cycle. So
every routine token expiry cost a full poll interval, on loops with a 30-second floor.
In
featureLoopthat cycle is wasted in a particular way -- the push failure jumps pastetag = pollResponse.Header.Get("ETag"), so the cycle spends a full flag poll againstLaunchDarkly and a push against Salesforce and stores nothing, and the org keeps serving
stale flag data for another interval. In
eventLoopthe refused request is the drain of/event, so events accumulate rather than being collected.requestWithOauthis now straight-line: send, return if the status is not 401 or 403,otherwise finish with the rejection, refresh, send once more, return the second response.
An expiry now costs one round trip instead of a cycle.
The body replay is the subtle part
client.Doconsumes and closes the request body while writing it, so sending one*http.Requesttwice would write the payload once and nothing after.The tempting story is that
bytes.NewBuffercauses this andbytes.NewReadercures it.That is not so:
http.NewRequestbuilds aGetBodyclosure for*bytes.Bufferand*bytes.Readeralike. The actual fix issendWithTokencallingGetBody()andreinstalling a fresh body before each send.
What makes it non-optional is how it fails when omitted. The transport sees a request
that declared a
ContentLengthand wrote nothing, classifies it asnothingWrittenError, and rewinds throughGetBodyitself -- but only when the attemptwent out on a reused connection. On a fresh dial the send fails with
http: ContentLength=N with Body length 0. Omitting the replay therefore passes on a warmpool and breaks intermittently in production, exactly when the connection is fresh or was
reaped while idle, which is the situation around a session expiry. The body test forces
Connection: closeon the rejection so it can distinguish a real replay from thetransport covering for a missing one.
bytes.NewReaderis kept at the flag push for a different, stated reason: a Reader is aread-only view of
pollBytes, where a Buffer is a writable staging area thathttp.NewRequestsnapshots once. Both poll requests pass a nil body, soGetBodyis niland the replay skips them with no special case.
Response ownership, and a leak
When the refresh itself failed, the old code returned
nil, err, permanentand neverdrained or closed the 401 response. Go can only pool a connection once its body reaches
EOF and is closed, and
eventLooptreats a non-permanent error as recoverable and keepslooping -- so a token endpoint failing transiently leaked one connection per cycle
indefinitely. The rejection is now drained before authorizing, which also returns that
connection to the pool in time for the token request to reuse it.
The rule is now checkable by eye: the caller owns the returned response, and every
response the function does not return is released inside it. No path leaves one open and
none is closed twice.
Bounded by structure
The second send is a single statement -- no loop, no recursion -- so a second rejection is
returned as it stands. A token minted seconds ago being refused is not an expiry; it means
the connected app permissions or its run-as identity changed, which refreshing again
cannot fix. Looping would spend the org API allocation on a rejection that repeats.
Not addressed here
The refresh itself gets no retry, there is no proactive refresh ahead of expiry, and no
backoff, jitter or general status classification -- the ticket tracks those separately.
Each of the three changes was mutation-tested independently: reverting
requestWithOauthfails the retry and loop tests, deleting only the
GetBodyreplay fails the two bodytests, and deleting only the
drainAndClosefails the connection-count test. Verified onGo 1.26 and on the CI-pinned Go 1.15.
Note
Overview
requestWithOauthnow retries the same Salesforce call once after a 401/403, instead of refreshing the token and still returning the original rejection. Callers get the second response, so routine session expiry costs one extra round trip rather than a full poll interval of stale flags or backed-up events.The send path is split into
sendWithToken, which re-applies the current bearer token (and scope header) and replays the request body viaGetBody()before eachclient.Do, so a second attempt is safe when the first send consumed the body. Flag pushes usebytes.NewReaderinstead ofbytes.NewBufferso both attempts stay a read-only view of the polled payload.On auth failure the 401/403 response is drained and closed before refresh, fixing a connection leak when refresh failed transiently and clarifying response ownership. Retries are capped at one after refresh; other status codes are unchanged.
Adds
oauth_retry_test.gocovering retry success, single-retry limits, non-auth statuses, full body replay on flag push, ETag advancement after a retried push, and connection reuse when refresh fails.Reviewed by Cursor Bugbot for commit 56f787a. Bugbot is set up for automated code reviews on this repo. Configure here.