Skip to content

fix(relay): keep mint budget below the relay request deadline - #12021

Closed
Adamulek123 wants to merge 2 commits into
pingdotgg:mainfrom
Adamulek123:perf/s1-relay-timeout-budget
Closed

Adamulek123 wants to merge 2 commits into
pingdotgg:mainfrom
Adamulek123:perf/s1-relay-timeout-budget

Conversation

@Adamulek123

@Adamulek123 Adamulek123 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Slow T3 Connect requests 504 at the relay's 9s request deadline before the 10s downstream mint timeout can fire, so clients get a generic deadline error instead of the typed endpoint timeout — and the extra downstream second is pure waste.

What changed

  • ENVIRONMENT_MINT_REQUEST_TIMEOUT_MS 10s -> 7s, with a comment pinning the invariant against RELAY_REQUEST_DEADLINE_MS.
  • Scope note: this constant is shared with the health/status path, so that path moves 10s -> 7s as well — intended, both must stay under the 9s relay deadline.

Validation

  • vp test run infra/relay/src/environments/EnvironmentConnector.test.ts — 19/19 pass (suite references the constant symbolically, no update needed).
  • Package typecheck clean (pre-existing style suggestions only).

Summary by CodeRabbit

  • Bug Fixes
    • Environment connection requests now time out after 7 seconds across the full connection process, including proof, linking, allocation, and credential minting steps.
    • Timed-out requests now return a clear 504 environment endpoint timeout response, preventing stalled operations from exceeding the relay deadline.
    • Requests that exceed the timeout are interrupted promptly, reducing delays and ensuring the relay remains responsive.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 16, 2026
Comment thread infra/relay/src/environments/EnvironmentConnector.ts
@macroscopeapp

macroscopeapp Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The relay’s production timeout default changes from 10 seconds to 7 seconds, affecting full environment connects and the existing health/status timeout path. The PR also adds a cross-layer timeout wrapper and changes which timeout response clients receive, so the operational behavior warrants human review.

No code changes detected at e7dc492. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c3e50ee0-6c4c-44a0-9f3f-f9df1ecbe582

📥 Commits

Reviewing files that changed from the base of the PR and between 79898b39d0648f75305b2927c2e176d8c40d77f5 and e7dc492.

📒 Files selected for processing (2)
  • infra/relay/src/environments/EnvironmentConnector.test.ts
  • infra/relay/src/http/Api.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The environment connect flow now applies a 7,000 ms timeout to the full operation. The HTTP API applies the timeout before error mapping. Tests cover stalled proof, link, allocation, and mint steps.

Changes

Relay timeout alignment

Layer / File(s) Summary
Connect timeout behavior
infra/relay/src/environments/EnvironmentConnector.ts
The connector exports a 7,000 ms timeout helper and applies it to the full connect operation, including signing and response verification.
HTTP API timeout wiring
infra/relay/src/http/Api.ts
The connectEnvironment handler applies the environment-specific timeout before common error mapping.
End-to-end timeout validation
infra/relay/src/environments/EnvironmentConnector.test.ts
Tests stall proof, link, allocation, and mint steps. Each case expects a 504 environment_endpoint_timed_out response and interruption of the stalled effect.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant dpopClientApi
  participant EnvironmentConnector
  participant EnvironmentServices
  Client->>dpopClientApi: POST environment connect
  dpopClientApi->>EnvironmentConnector: run timed connect effect
  EnvironmentConnector->>EnvironmentServices: resolve and mint environment credential
  EnvironmentConnector-->>dpopClientApi: credential or timeout error
  dpopClientApi-->>Client: response with mapped error
Loading

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: ⚪ Minimal · up to e7dc4

The environment connect flow now times out at 7 seconds, before the relay deadline, with coverage for stalled stages. No merge-blocking risk remains identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: keeping the mint timeout below the relay request deadline.
Description check ✅ Passed The description explains what changed, why it changed, scope impact, and validation results. It does not use the template headings exactly and omits the checklist, but the required information is most…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:XS 0-9 changed lines (additions + deletions). labels Sep 18, 2026
@Adamulek123
Adamulek123 force-pushed the perf/s1-relay-timeout-budget branch from 79898b3 to e7dc492 Compare September 22, 2026 16:04
@macroscopeapp

This comment has been minimized.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@Adamulek123

Copy link
Copy Markdown
Contributor Author

Closing this after re-checking the premise against current main (ec80933ac8cd).

The argument here is that the mint budget should stay under RELAY_REQUEST_DEADLINE_MS so a typed timeout fires instead of a generic 504. That deadline is deliberate, and the comment directly above it says so (infra/relay/src/http/Api.ts:167-174):

Shorter than the mobile client's 10s request timeout on purpose: when a request hangs... the client would otherwise abort first, the invocation would die with the request span still open, and the batched spans would never export — leaving no server-side trace at all. Failing server-side first turns the hang into a completed 504 whose trace contains the exact child span that stalled.

A downstream budget that preempts the 9s deadline removes the child span from a completed 504 and reintroduces exactly the open-span trace loss the constant exists to prevent. So this isn't a stale reading — it's arguing against a documented decision without acknowledging it.

On top of that, this branch no longer builds. #16138 (194c73f3f9) upgraded Effect to stable 4.0.1 and moved infra/relay/src/http/Api.ts from effect/unstable/http* to effect/http*; #16282 (ecfdda5fa8) then renamed every exported layer from fooApi to layerFooApi and dropped the re-exports. The 107 lines of test code in this diff import six symbols (dpopClientApi, RelayApi, RelayClientPrincipal, RelayDpopClientAuth, RelayEnvironmentConnectRequest, RelayCloudMintCredentialRequest) that no longer exist in that file. That needs a rewrite, not a rebase.

I don't think the underlying latency concern is wrong — there may well be a real mint-timeout budget problem here. If so, the honest version is a small issue describing the user-visible symptom against the trace-loss tradeoff, so a maintainer can weigh in on which side wins. Happy to reopen as that if there's a concrete reproduction.

@Adamulek123 Adamulek123 closed this Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants