Skip to content

perf(server): tighten pair probe budgets - #12028

Open
Adamulek123 wants to merge 12 commits into
pingdotgg:mainfrom
Adamulek123:perf/s3-pair-probe-budget
Open

Adamulek123 wants to merge 12 commits into
pingdotgg:mainfrom
Adamulek123:perf/s3-pair-probe-budget

Conversation

@Adamulek123

@Adamulek123 Adamulek123 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Pairing discovery probed up to 4 candidates strictly sequentially (worst ~10s), and the Tailscale cert-wait slept a full second after its final attempt.

What changed

  • Cheap local checks (state file, pid) stay sequential to preserve precedence and error output; network probes run concurrently with the first precedence-ordered hit winning. Skip the retry sleep after the final cert-wait attempt.

Validation

  • vp test run apps/server/src/cli/pair.test.ts — 26/26 pass.
  • Package typecheck clean.

Summary by CodeRabbit

Bug Fixes

  • Improved server pairing discovery across supported local environments.
  • Pairing checks now run concurrently, reducing delays when multiple candidates are available.
  • Improved selection of the correct available server when several candidates are detected.
  • Invalid, oversized, non-JSON, or unsuccessful server responses are now rejected safely.
  • Slow or unresponsive pairing requests time out instead of hanging.
  • Pairing requests are canceled promptly when discovery is interrupted.
  • Retry delays now occur only between Tailscale connection attempts.
  • When no server is found, pairing provides details about the locations checked.

@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Sep 16, 2026
Comment thread apps/server/src/cli/pair.ts Outdated
@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.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

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: 83d020f9-1318-4722-8244-6884613603ef

📥 Commits

Reviewing files that changed from the base of the PR and between 6c5f68900894564f8f10e00ba0df10d1dcf10931 and 6947f22.

📒 Files selected for processing (1)
  • apps/server/src/cli/pair.ts

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


📝 Walkthrough

Walkthrough

The pair command bounds and validates environment descriptor responses before decoding. It probes live candidates concurrently, selects the first descriptor by precedence, cancels lower-priority probes, and avoids a final retry delay.

Changes

Pair target discovery

Layer / File(s) Summary
Bounded descriptor probing
apps/server/src/cli/pair.ts, apps/server/src/cli/pair.test.ts
Probes enforce a 64 KiB byte limit and a 2.5-second drain timeout. Non-2xx, non-JSON, oversized, invalid, and unreachable responses are treated as non-T3 results. Valid 2xx JSON responses use the shared decoder. Tests cover these cases and uppercase JSON media types.
Candidate collection and selection
apps/server/src/cli/pair.ts, apps/server/src/cli/pair.test.ts
The command collects validated candidates, probes them concurrently, selects the first descriptor in precedence order, and interrupts lower-priority or cancelled probes. Tests cover overlap, status-based selection, and request abortion.
Tailscale retry timing
apps/server/src/cli/pair.ts, apps/server/src/cli/pair.test.ts
The retry loop sleeps only when another attempt remains. Tests verify five attempts at one-second intervals without a final delay.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PairCommand
  participant CandidateState
  participant EnvironmentProbes
  participant DescriptorDecoder
  PairCommand->>CandidateState: collect live candidates
  PairCommand->>EnvironmentProbes: start concurrent probes
  EnvironmentProbes->>DescriptorDecoder: decode bounded 2xx JSON response
  DescriptorDecoder-->>EnvironmentProbes: descriptor or failure
  EnvironmentProbes-->>PairCommand: probe results
  PairCommand->>EnvironmentProbes: cancel lower-priority probes
  PairCommand-->>PairCommand: select descriptor by precedence
Loading

Suggested reviewers: bahlo, juliusmarminge

Merge Risk: ⚪ Minimal · up to 6947f

Pair discovery now bounds and validates responses, probes candidates concurrently while preserving precedence, cancels unnecessary probes, and avoids the final retry delay. These changes limit hangs and improve pairing responsiveness without a concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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 2…
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.
Title check ✅ Passed The title clearly summarizes the main change: tightening server pairing probe budgets.
Description check ✅ Passed The description clearly states the problem, the concurrency and retry changes, and the validation results. It omits explicit Why and Checklist sections, but it provides the core information and remain…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@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 production pairing path now performs concurrent, cancelable server discovery and applies new response-size, media-type, status, timeout, and retry rules before selecting the server used for pairing. Although the change is well covered by tests and does not alter schemas or deployment configuration, its runtime and authentication-path impact is substantial enough for human review.

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

Comment thread apps/server/src/cli/pair.ts Outdated
Comment thread apps/server/src/cli/pair.ts Outdated
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 20, 2026
@Adamulek123
Adamulek123 force-pushed the perf/s3-pair-probe-budget branch from f36c1a8 to 5c5e602 Compare September 20, 2026 12:10
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 20, 2026 12:10

Dismissing prior approval to re-evaluate 5c5e602

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 20, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 20, 2026
Stream the descriptor and 502/503/504 drain through a byte-counted read capped at MAX_PROBE_BODY_BYTES with PAIR_PROBE_TIMEOUT, before decoding to text. Normalize the JSON content-type check. Covers uppercase content types, slow-drip bodies, and multibyte UTF-8 oversize.
…r is known

Fork all candidate probes under a scope, join fibers in discovery order, and return the first descriptor without waiting for losers (scope teardown interrupts them). Adds overlap/cancellation, discovery-order, and test-clock retry-budget coverage.
@Adamulek123
Adamulek123 force-pushed the perf/s3-pair-probe-budget branch from 6c5f689 to 6947f22 Compare September 22, 2026 16:04
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 26, 2026 16:10

Dismissing prior approval to re-evaluate 6b91dc5

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 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.

1 participant