Skip to content

perf(server): skip unused default-branch lookups - #19825

Open
IfkumRfnl wants to merge 6 commits into
pingdotgg:mainfrom
IfkumRfnl:perf/defer-default-branch-lookup
Open

IfkumRfnl wants to merge 6 commits into
pingdotgg:mainfrom
IfkumRfnl:perf/defer-default-branch-lookup

Conversation

@IfkumRfnl

@IfkumRfnl IfkumRfnl commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Status could launch a default-branch Git subprocess even after its configured comparison base resolved, and remote status with an upstream could repeat a successful origin-default read. Candidate lookups now run only when reached, and remote status with an upstream passes its successful origin result into its later comparison. Failed reads still retry; other primary remotes resolve their own default. Candidate order and returned status for the same repository state stay the same, with no persistent cache.

Verified at 26aa1b72c1, integrated with main 687bd55533, on Linux / Node 24.21.0 / Git 2.43.0:

  • 165/165 tests pass in the existing GitVcsDriverCore test file. Contribution-specific extensions: 2 → 2. The retry uses trunk so an incorrect failed-read cache cannot pass through main/master fallback. Removing the optimization fails both cases; caching a failed read fails the returned ahead count.
  • Server package typecheck, scoped lint and formatting pass. Two existing lint warnings remain on unchanged lines. No repo-wide checks run.
  • Fresh GPT-6 Astra test run, separate GPT-6.1 Sol implementation run; two independent adversarial rounds with Claude Opus 5.5 and GPT-6 Sol found no remaining code blockers. A proposed no-upstream read reorder was declined as a separate behavior choice after a real-driver transient-failure counterexample; both reviewers accepted that scope decision.

Live server-level proof: invoked the actual base and head status-driver factories against the same disposable Git repository. All 48 alternating pairs returned identical status. Eight pairs per scenario/driver lifetime, warm filesystem cache, 100 base files and 20 modified files. Local status includes Changes totals; remote status disables fetch. Driver construction excluded. Timings describe this shared host; the eliminated Git call is deterministic. The unchanged-command missing-base control shows the timing noise floor, so local timings alone do not establish a speedup.

Status Driver Git calls main → head Median main → head (ms)
configured-local warm 10 → 9 75.7 → 72.1
configured-local fresh 15 → 14 103.5 → 95.8
missing-local warm 12 → 12 73.3 → 73.5
missing-local fresh 17 → 17 94.5 → 91.3
upstream-remote warm 10 → 9 55.2 → 50.1
upstream-remote fresh 10 → 9 55.7 → 47.5

Raw measurements · Measurement script · Method and paired timings

Remote status without an upstream retains its existing lookup order and duplicate reads. Moving the default read earlier changes the result when its first lookup fails and the next succeeds; that behavior choice is outside this optimization.

No UI behavior changes, so screenshots do not demonstrate this optimization. Clients share the server driver. macOS, Windows, iOS, Android, Electron desktop, remote/relay/tunnel connections, and network latency were not exercised in this pass. Earlier Windows timings were removed because they measured an older revision. Current-head CI passes, including all eight server shards, lint, build/typecheck and the aggregate check. The canceled duplicate fingerprint job was superseded by a passing run on the same head. There are no unresolved review threads. No formal GitHub approval is recorded; both independent review agents found no blockers. Unmerged; auto-merge is disabled.

Original contribution by @IfkumRfnl in #19825. Maintainer takeover: GPT-6 Astra (tests), GPT-6.1 Sol (implementation/integration), Claude Opus 5.5 and GPT-6 Sol (adversarial review), through the Codex and Claude harnesses in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 15, 2026
@IfkumRfnl
IfkumRfnl marked this pull request as ready for review October 15, 2026 08:06
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T08:09:08.256692Z b9d289b Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5af9c790-f0ee-48b5-870c-46037c55236b
📥 Commits

Reviewing files that changed from the base of the PR and between 16cbcf3 and cc5e735.

📒 Files selected for processing: 2
  • apps/server/src/vcs/GitVcsDriverCore.test.ts
  • apps/server/src/vcs/GitVcsDriverCore.ts

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

📝 Walkthrough

Base-branch resolution checks configured candidates before looking up the primary remote default branch. Remote status reuses its resolved default branch when calculating ahead counts. Tests cover configured-base selection and default-branch lookup behavior.

Changes: No-upstream base selection

Layer / File(s) Summary
Configured base and fallback order
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
The resolver checks the configured base before the primary remote default branch, then retains main and master as fallbacks. Tests check whether origin/HEAD is resolved for matching and missing configured bases.
Reuse of resolved default branch
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
Remote status passes its resolved default branch into ahead-count calculation. Tests verify lookup counts and that a failed lookup is retried, with defaultBranch: null after the retry.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to cc5e7

The changed base-selection and remote-status paths have no identified issue that needs resolution before merge.

### Architecture Summary

Architecture risk: 🔵 Low · up to cc5e7

The change affects 1 system.

Changed systems: apps/server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: Added assertions that local status uses the configured base when it exists and resolves a missing configured base through origin/HEAD; the test checks that the symbolic-ref lookup occurs only for the missing base.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: The remote-status test now sets origin/HEAD, wraps the Git spawner to count and optionally fail its first symbolic-ref lookup, and calls statusDetailsRemote with upstream refresh disabled instead of using the standard driver layer.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: Added a retry check: after the first origin/HEAD lookup is forced to fail, remote status returns the prior status with defaultBranch: null and performs two lookups; the earlier successful call performed one.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.ts: resolveBaseBranchForNoUpstream adds the optional knownOriginDefaultBranch: string | null input alongside allowRemoteOfCurrent; the comment clarifies that only a successful, non-null lookup skips the retry.
Pre-merge checks: Passed 4
Check name Status Explanation
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 and concisely describes the primary performance change: avoiding unnecessary default-branch lookups.
Description check Passed The description explains the problem, implementation, scope, verification results, limitations, and test evidence. It does not include formal maintainer approval, but it provides sufficient context fo…
Full details: Description check

Explanation

The description explains the problem, implementation, scope, verification results, limitations, and test evidence. It does not include formal maintainer approval, but it provides sufficient context for this focused optimization.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one for batch worktree-git-05. I’ll keep the status behavior covered, verify the subprocess and latency reduction against current main, and publish here without merging.

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:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants