fix: re-create the message session on refresh 404 instead of failing - #123
Open
buildpulser wants to merge 1 commit into
Open
fix: re-create the message session on refresh 404 instead of failing#123buildpulser wants to merge 1 commit into
buildpulser wants to merge 1 commit into
Conversation
When the Actions service no longer knows a session, the refresh PATCH returns 404 and the client surfaces it as fatal from GetMessage/ DeleteMessage/AcquireJobs. If the condition is persistent (broker-side session eviction), the caller dies on every token refresh even though a freshly created session works fine — in ARC's listener that means a crash loop every ~50 minutes that also resets the in-memory message cursor. Map session-request 404s to the existing NotFoundError sentinel, and on a refresh 404 create a new session in place (same mutex and session- identity double-check as refresh) and retry once. All other refresh failures surface exactly as before. The caller's lastMessageId is sent per request, so delivery position survives re-creation.
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
1 participant
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.
We run ARC (0.14.2) for a fleet of orgs and hit a nasty failure mode on one of them: the broker evicts every message session server-side, so the listener's session refresh always comes back 404 with
RunnerAdminException: runner referenced a session ID that doesn't exist in redis. The client treats any refresh failure as fatal, the error bubbles out ofGetMessage, and the listener process exits.Since the refresh happens on the token cycle, the listener for that org died every ~50 minutes, around the clock, for almost two weeks before we noticed. Readiness probes never caught it because the pod is back in a few seconds. The part that actually hurts is that each restart throws away the in-memory
lastMessageID, so the message cursor resets to 0 every time.There are a few issues in the ARC repo that look like the same family (actions/actions-runner-controller#4356, actions/actions-runner-controller#3942), and actions/actions-runner-controller#4571 explicitly punts on healing an already crash-looping listener, so the session client seemed like the right place to fix it.
The change: when a session refresh comes back 404, create a new session in place and retry once instead of giving up. A 404 on refresh means the service doesn't know the session anymore, so there's nothing left to lose by re-creating. Everything else (500s, network errors, token expiry itself) behaves exactly as before. Session-request 404s now map to the existing
NotFoundErrorsentinel so callers aren't string-matching, and the re-create runs under the same mutex and session-identity double-check that refresh already uses, wired through the three places that do the refresh-and-retry dance (GetMessage,DeleteMessage,AcquireJobs).lastMessageIDis owned by the caller and sent on every request, so the delivery position survives the swap. Worst case, a re-created session behaves like what a process restart already does today, minus losing the cursor.Tests cover the recovery path (including that the caller's
lastMessageIdstill goes out on the retried request against the new session), that a 500 on refresh does not trigger a re-create, and that a failed re-create surfaces both errors witherrors.Is(err, NotFoundError)intact.For what it's worth, we've been running this patch in production (backported onto v0.4.0): the affected org's listeners went from dying every 50 minutes to logging a single re-create line and moving on. Happy to reshape it if you'd rather see the recovery live in the listener instead of the client, or handled differently.