Skip to content

Replay a WorkOS refresh inside the 30 s grace window - #947

Merged
realtonyyoung merged 9 commits into
mainfrom
fix/workos-refresh-replay
Sep 15, 2026
Merged

realtonyyoung merged 9 commits into
mainfrom
fix/workos-refresh-replay

Conversation

@realtonyyoung

@realtonyyoung realtonyyoung commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #946 — AI-2790

What & why

A WorkOS refresh was sent once with a 5 s deadline. When WorkOS processed the exchange but the reply missed the deadline, the rotated pair was lost; the next refresh, a minute or more later, presented the retired token outside WorkOS's 30-second replay window and got invalid_grant, signing out every process that shares the token store. WorkOS documents that a replay inside the window returns the same rotated pair and that timeouts, 5xx and 429 should be retried with the same token. The client now replays a lost reply, a transient status or an unreadable success body while the next attempt can still complete inside a 20 s budget (5 s per attempt), re-checked after every backoff; a 4xx is still never replayed. Lock waiters derive their deadline from that budget so a peer never abandons a holder about to persist a fresh token, and hook client creation is bounded at 3 s: past it the hook spools (or reports the lapse on the no-spool path) and hands the refresh to a detached kcap refresh-token process pinned to its config root and profile, which either finds the token fresh or replays it inside the window, since every vendor kills a hook well inside the lock wait.

Where to look

The out-of-window classification: any unreadable success means the token is spent, so the outcome is Rejected however later replays fail; a reply that never arrived ends as TransportFailed. SequencedHttpScript exists because WireMock's cold start exceeds any deadline short enough to test a stall. BoundedAuth is the Claude hook's auth race moved out of the vendor directory so the other seven hooks can share it. RefreshTokenHandoff detaches before repository resolution and drops KCAP_URL from the child, which would otherwise outrank the profile pin.

Verification

Daemon log, 2026-09-12: refresh sent 21:54:40.751, "Proactive token refresh failed" 21:54:45.977, next refresh 21:55:41 → HTTP 400, then "Proactive token refresh was rejected" hourly until the next login.

dotnet run --project test/Capacitor.Cli.Core.Tests.Unit -- --treenode-filter '/*/Capacitor.Cli.Core.Tests.Unit.Auth/*/*'
  total: 457  failed: 0
dotnet run --project test/Capacitor.Cli.Tests.Unit -- --treenode-filter '/*/*/AgentHookPosterTests|ClaudeHookCommandTests|...SpawnBeforePostTests/*'
  total: 105  failed: 0 (incl. RefreshTokenHandoffTests)
dotnet publish src/Capacitor.Cli/Capacitor.Cli.csproj -c Release 2>&1 | grep -E 'IL[23][01][0-9]{2}'
  (no output)

🤖 Generated with Claude Code

WorkOS honours a replay of the just-retired refresh token for 30 s and answers with the same rotated pair, so a reply lost at the deadline is recoverable only inside that window. The 20 s budget plus one 5 s attempt keeps every replay under it; a 4xx is never replayed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Replay WorkOS refreshes within the grace window

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Replays ambiguous or transient WorkOS refresh attempts within a bounded grace period.
• Preserves terminal handling for refused tokens and exhausted unreadable responses.
• Adds deterministic coverage for replay timing, outcomes, request identity, and cancellation.
Diagram

graph TD
    A["Refresh request"] --> B["Timed attempt"] --> C{"Attempt outcome"}
    C -->|Rotated| D["Return tokens"]
    C -->|4xx| E["Reject refresh"]
    C -->|Retryable| F{"Budget remains"}
    F -->|Yes| G["Backoff replay"] --> B
    F -->|"No: unreadable"| E
    F -->|"No: transport"| H["Transport failure"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Generic HTTP retry policy
  • ➕ Could reuse centralized retry and backoff infrastructure.
  • ➕ May reduce custom control-flow code.
  • ➖ Would need custom hooks for response-body parsing and outcome classification.
  • ➖ Could obscure the strict replay-window and 4xx termination semantics.
2. Persist refresh-attempt state
  • ➕ Could recover an ambiguous exchange after process termination.
  • ➕ Would make replay intent durable across processes.
  • ➖ Adds token-store state, synchronization, and cleanup complexity.
  • ➖ Clock drift and stale markers complicate enforcement of WorkOS's short window.

Recommendation: Keep the bounded in-process replay loop. It directly models WorkOS's token-specific semantics, distinguishes unreadable success responses from missing replies, and guarantees that 4xx responses are not retried. A generic policy lacks the required outcome awareness, while durable attempt state is disproportionate unless cross-process or crash recovery becomes necessary.

Files changed (4) +181 / -78

Bug fix (1) +54 / -33
WorkOSClient.csAdd bounded replay for ambiguous WorkOS refresh attempts +54/-33

Add bounded replay for ambiguous WorkOS refresh attempts

• Splits refreshes into individually timed attempts and replays timeouts, transient statuses, and unreadable successful responses with the same token. Stops when another attempt would exceed the replay budget, preserves immediate 4xx rejection, and propagates caller cancellation.

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs

Tests (2) +123 / -42
WorkOSClientTests.csCover refresh replay behavior and terminal classifications +89/-42

Cover refresh replay behavior and terminal classifications

• Adds deterministic tests for timeout, 5xx, unreadable-body, replay-budget, 4xx, and caller-cancellation behavior. Tests also verify that replayed requests carry identical bodies and that successful or refused exchanges stop immediately.

test/Capacitor.Cli.Core.Tests.Unit/Auth/WorkOSClientTests.cs

SequencedHttpScript.csAdd deterministic sequenced HTTP test handler +34/-0

Add deterministic sequenced HTTP test handler

• Introduces an HTTP message handler that returns scripted responses, repeats the final step, records request bodies, and can model an indefinitely stalled endpoint without WireMock startup timing.

test/Capacitor.Tests.Helpers/SequencedHttpScript.cs

Documentation (1) +4 / -3
WorkOSRefreshResult.csClarify post-replay refresh outcome semantics +4/-3

Clarify post-replay refresh outcome semantics

• Updates outcome documentation to describe classification after in-window replays are exhausted, including the distinction between consumed tokens and potentially live tokens.

src/Capacitor.Cli.Core/Auth/WorkOSRefreshResult.cs

@qodo-code-review

qodo-code-review Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Late replays can end shared sessions ✓ Resolved 🐞 Bug ≡ Correctness
Description
RefreshAsync validates the predicted next start before Task.Delay, but it does not recheck
elapsed time after that delay before calling RefreshOnceAsync again. If scheduling, thread
starvation, or system suspension resumes the delay late enough, the client sends the retired token
outside WorkOS's 30-second grace period, where the resulting refusal is classified as terminal and
requires another login.
Code

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[71]

+            await Task.Delay(_replayBackoff, ct);
Relevance

●●● Strong

PR #132 accepted enforcing retry budgets at the actual retry boundary, directly matching this
late-resume backoff bug.

PR-#132

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The client documents a 30-second replay window and records elapsed time from the first request, but
lines 61-71 only predict timing before an uncapped asynchronous delay. The same file classifies an
out-of-window 4xx as Rejected, while the token store maps that outcome to terminal handling; past
PR #132 confirms this repository has already required retry budgets to be enforced at the actual
retry boundary rather than only predicted before a delay.

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[32-47]
src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[51-71]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[509-513]
PR-#132

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The replay budget is checked before an asynchronous delay, so a delayed continuation can start a WorkOS replay after the safe grace window has elapsed.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[51-71]

## Recommended Fix
Represent the replay cutoff as an absolute monotonic deadline and check it again after every backoff, immediately before starting `RefreshOnceAsync`. If the deadline has passed, return the terminal classification derived from the previous attempt without sending another request.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Refresh docs narrate old behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
RefreshAsync documentation explains that without replay a timed-out exchange left its successor
unrecoverable and that the next refresh was refused. The same outage history is repeated in the
timeout test, so future timing or retry changes can leave both comments describing an obsolete
incident instead of the current invariant.
Code

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[R44-46]

+    /// pair the first exchange minted. Without the replay a timed-out exchange that WorkOS had in fact
+    /// processed left its successor unrecoverable, and the next refresh — a minute later, outside the
+    /// window — was refused as <c>invalid_grant</c>, signing every process out. A 4xx is never replayed:
Relevance

●●● Strong

Recent accepted reviews require comments to describe durable current behavior rather than outage or
change-history narratives.

PR-#875
PR-#638

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2897915 prohibits change-history narration in comments. The added API documentation describes
what happened without replay and the test documentation explicitly recounts the outage and
subsequent refresh, rather than limiting itself to current behavior.

Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[42-47]
test/Capacitor.Cli.Core.Tests.Unit/Auth/WorkOSClientTests.cs[102-106]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The refresh documentation and timeout-test summary narrate the historical outage and behavior before replay was added, creating comments that can become stale as retry timing changes.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[42-47]
- test/Capacitor.Cli.Core.Tests.Unit/Auth/WorkOSClientTests.cs[102-106]

## Recommended Fix
Rewrite both comments to describe only the current replay invariant and expected test behavior. Remove references to the earlier lost-successor incident, the later refresh, and the outage.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Peer commands fail before refresh lands ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new defaults let RefreshAsync occupy the profile lock for roughly 17 seconds, while
RefreshWithCrossProcessLockAsync still makes peers give up after 15 seconds. When a third
timed-out attempt eventually succeeds, a concurrent recovery can already have fallen back to the
stale token and sent its one permitted retry, producing another authentication failure immediately
before the fresh token is saved.
Code

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[R35-36]

+    readonly TimeSpan _replayBudget  = replayBudget  ?? TimeSpan.FromSeconds(20);
+    readonly TimeSpan _replayBackoff = replayBackoff ?? TimeSpan.FromSeconds(1);
Relevance

●●● Strong

Recent accepted precedent flags lock contention when refresh work outlasts the waiter timeout,
matching this reliability failure.

PR-#257

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Three default five-second attempts separated by one-second backoffs can keep the lock owner active
until about 17 seconds, but token-store waiters stop at 15 seconds and return null if the token is
still due. The reactive recovery path then performs an unchecked raw read, marks that credential
usable, and the unauthorized handler sends it exactly once before returning the second failure.

src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[28-36]
src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[53-71]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[541-564]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[584-594]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[327-337]
src/Capacitor.Cli.Core/Auth/TokenStoreCredentials.cs[16-21]
src/Capacitor.Cli.Core/Auth/UnauthorizedRecoveryHandler.cs[47-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
WorkOS refreshes may now hold the cross-process token lock longer than its fixed 15-second waiter deadline, so peers can fail while the lock owner is still completing a recoverable refresh.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/WorkOSClient.cs[28-36]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[541-564]

## Recommended Fix
Make the lock-wait deadline exceed the maximum refresh replay duration, including backoff and a small scheduling margin, or derive both limits from one shared configuration. Add concurrency coverage where the lock holder succeeds after 15 seconds and verify the waiter adopts the persisted token instead of returning stale credentials.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
Review mode: ⚖️ Balanced: Downgraded extended -> standard: change is below the extended eligibility bar (hunks 8/18, lines 259/200; both must reach the floor). Router rationale: This changes retry and token-rotation behavior across multiple production and test paths, with timing, cancellation, HTTP classification, and replay-window semantics that create several independent, easy-to-miss failure modes.

Grey Divider

Tip of the day
💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli.Core/Auth/WorkOSClient.cs Outdated
Comment thread src/Capacitor.Cli.Core/Auth/WorkOSClient.cs Outdated
Comment thread src/Capacitor.Cli.Core/Auth/WorkOSClient.cs
realtonyyoung and others added 6 commits September 14, 2026 17:12
…lock (#946)

A delay stretched by a suspended machine can resume outside WorkOS's replay window, where a replay is refused for good. Lock waiters derive their wait from the same budget so a peer never abandons a holder that is one replay from persisting a fresh token.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…946)

Every vendor kills a hook well inside the refresh lock wait, so client creation is bounded and the payload spooled rather than the hook dying on its way to the spool. One unreadable success proves the token spent, whatever later replays return.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A hook that gives up on client creation may exit with a rotation in flight, losing the rotated pair. The detached refresh-token process takes the same lock and either finds the token fresh or replays it inside WorkOS's window.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
#946)

A URL override outranks the profile pin and resolves to no profile, so the child drops it. Inherited hook pipes would make a host waiting for EOF wait on the child too.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The hand-off drops KCAP_URL from the child, so a profile that only ever had a URL override would otherwise be turned away at the no-server gate before it could refresh.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
realtonyyoung and others added 2 commits September 14, 2026 19:36
The Claude hook tests build the command with a fake starter so the abandon path can never spawn the test host as a detached kcap.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@realtonyyoung
realtonyyoung merged commit d6b5100 into main Sep 15, 2026
8 checks passed
@realtonyyoung
realtonyyoung deleted the fix/workos-refresh-replay branch September 15, 2026 00:49
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.

A WorkOS refresh reply lost at the 5 s deadline signs every client out

1 participant