Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAuthorization tracks consecutive websocket-ticket timeouts for cached tokens. The first two timeouts retain the token; the third triggers renewal. A successful ticket request resets the timeout count. Timeout tests cover cached and refreshed tokens. ChangesWebsocket ticket authorization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The inspected timeout path retains the token for a later attempt. No issue requiring a fix before merge was established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This focused fix changes the shared production DPoP authorization path, including timeout retries, token retention, and relay credential minting. Because it modifies the authorization package, human review is required despite the limited scope and added test coverage. You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
4f253d6 to
ebf9c37
Compare
20651c7 to
8d80247
Compare
…every reconnect When the websocket-ticket request with a cached, still-valid DPoP token timed out after 3 s, the client treated the token as rejected. It then minted a new relay credential and auth session. A freshly minted token was also thrown away when its ticket request timed out. During a server stall this happened on every reconnect. Now a cached-token ticket timeout retries the same token once with the default 10 s budget (13 s total, inside the 15 s establishment deadline). If that also times out, the attempt fails as a transient timeout and the token stays cached. A fresh token is also kept after a ticket timeout. Auth-invalid, 5xx, and network failures still re-bootstrap and evict, so moved-tunnel recovery is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The cached-token ticket retry used the default 10 s budget, so the ticket step could take 13 s. After it, the supervisor's 15 s setup race still has to fetch the descriptor, open the websocket, and load the initial config. On a server slow enough to need the retry, 2 s is not enough, so the attempt timed out late anyway. The retry now gets 7 s, so the whole ticket step ends by 10 s, the same as one default request. At least 5 s of the setup deadline is left. A retry timeout still fails as transient and keeps the token, so nothing is minted. Also use Effect.catchTags for the known timeout tag, per the Effect service conventions. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Correct the setup-deadline note, name the hung-endpoint tradeoff, and rename the cached-endpoint test to match what it now covers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A ticket timeout no longer mints a new credential, so the 3 s cached budget and the 7 s retry only split one 10 s wait into two requests. The cached token now gets the default 10 s budget once. A timeout fails as transient and keeps the token, so the next attempt reuses it. Auth rejections, server errors, and network errors still ask the relay for a new token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…n a row A relay endpoint that accepts the ticket request and never answers kept the cached token until it expired, up to an hour. Count cached-token ticket timeouts per environment and token. The third timeout in a row drops the token and mints a new one through the relay. A success resets the count. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A supervisor test with the real authorization service now shows that a stalled ticket request keeps the setup waiting for the full 10 s budget, backs off, and reconnects on the stored token with no relay call. A second test shows that a cached endpoint that fails at the network level moves to the relay's endpoint in the first attempt. The shared relay setup moves into one helper that the existing HTTP refresh test also uses, and the service-level stalled-token test is removed because the supervisor test covers it. The timeout comment now says why a new credential cannot help: the relay returns the same hostname for an environment. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… token With one 10 s budget, a single lost ticket request on a healthy server took about 13 s to recover (10 s timeout, 3 s backoff, new attempt). On main it took about 3 s plus a mint. The cached ticket request gets 3 s again, and a timeout retries the same token once with 7 s, so the ticket step still ends by 10 s. A lost request now recovers in about 3 s with no mint. If the retry also times out, the token is kept as before, and the third such attempt in a row asks the relay again. Adds supervisor tests for the lost request and for the third-timeout path, where the mint runs past the setup deadline and the next attempt connects with the minted token after one relay call. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The HTTP refresh supervisor test now checks that a healthy server connects in the first attempt with the stored token, one ticket request, and no relay call. The timeout count comment notes that it lives as long as the service, so a foreground resume does not reset it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The supervisor test already covers the three-timeout limit. The service test now only shows that a retry that succeeds resets the count, with a script built from named attempts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s network Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
8d80247 to
b0d71b1
Compare
When a relay client reconnects to a slow server, it treats a timed-out websocket-ticket request as a rejected DPoP token and mints a new credential. The mint (relay
t3MintCredential, descriptor fetch,/oauth/token) runs on that same slow server, so each reconnect attempt adds a new auth session there, about twice a minute, and the client still does not connect. This came from a long-uptime slowdown report (0.0.40, 9 days, about 5,000 threads) where slow requests had a p50 of 2.9 s against the 3 s cached-ticket budget.Fix
All in
authorizeDpopinpackages/client-runtime/src/authorization/service.ts, which web, desktop, and mobile share:#4878 removed an older counter that held back every failure. This one counts only timeouts after the retry, so a lost request still recovers in about 3 s.
Tradeoff
A cached endpoint that accepts connections but never answers is now left after about 40 s (the third attempt) instead of about 3 s. I do not expect this today. Relay connect only returns managed Cloudflare tunnel endpoints, and their hostname is derived from the environment (
infra/relay/src/environments/ManagedEndpointProvider.ts), so a new credential goes to the same stalled server anyway. A removed hostname or tunnel fails fast and still moves to the relay's endpoint in the first attempt.Merge order
#5463 also edits
service.ts(createDpopSocketUrland a newauthorizeDpopDirect). The behaviors do not conflict. Whichever PR lands second needs a rebase.Verification
vp test run packages/client-runtime/src/authorization/ packages/client-runtime/src/connection/: 141 passed. Withmain'sservice.ts, the 5 new tests fail.vp lintandvp fmton the changed files, andvp run --filter @t3tools/client-runtime typecheck.Related: #13695 prunes the server-side DPoP replay files that each mint adds.
Made by Claude Opus 5.5 (1M context) in Claude Code, running in T3 Code.
🤖 Generated with Claude Code