From 10c390faf275573ad07f101c85bcbf4efa2938b1 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Mon, 9 Feb 2026 13:33:55 -0800 Subject: [PATCH] fix(desktop): add timeouts to git/GitHub network operations to prevent indefinite hangs --- .../trpc/routers/changes/git-operations.ts | 126 +++++++++++++----- .../routers/workspaces/utils/git-timeouts.ts | 28 ++++ .../lib/trpc/routers/workspaces/utils/git.ts | 96 ++++++++----- .../routers/workspaces/utils/github/github.ts | 7 +- .../routers/workspaces/utils/shell-env.ts | 11 +- 5 files changed, 198 insertions(+), 70 deletions(-) create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/utils/git-timeouts.ts diff --git a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts index 0a9ef07af66..f9a321f1588 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts @@ -1,12 +1,21 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; import { TRPCError } from "@trpc/server"; import { shell } from "electron"; import simpleGit from "simple-git"; import { z } from "zod"; import { publicProcedure, router } from "../.."; +import { + GIT_TIMEOUT_NETWORK, + GIT_TIMEOUT_NETWORK_HEAVY, + wrapTimeoutError, +} from "../workspaces/utils/git-timeouts"; import { execWithShellEnv } from "../workspaces/utils/shell-env"; import { isUpstreamMissingError } from "./git-utils"; import { assertRegisteredWorktree } from "./security"; +const execFileAsync = promisify(execFile); + export { isUpstreamMissingError }; async function hasUpstreamBranch( @@ -20,17 +29,22 @@ async function hasUpstreamBranch( } } -async function fetchCurrentBranch( - git: ReturnType, -): Promise { +async function fetchCurrentBranch(worktreePath: string): Promise { + const git = simpleGit(worktreePath); const branch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim(); try { - await git.fetch(["origin", branch]); + await execFileAsync( + "git", + ["-C", worktreePath, "fetch", "origin", branch], + { timeout: GIT_TIMEOUT_NETWORK }, + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (isUpstreamMissingError(message)) { try { - await git.fetch(["origin"]); + await execFileAsync("git", ["-C", worktreePath, "fetch", "origin"], { + timeout: GIT_TIMEOUT_NETWORK, + }); } catch (fallbackError) { const fallbackMessage = fallbackError instanceof Error @@ -41,20 +55,20 @@ async function fetchCurrentBranch( `[git/fetch] failed fallback fetch for branch ${branch}:`, fallbackError, ); - throw fallbackError; + throw wrapTimeoutError(fallbackError, "Fetch"); } } return; } - throw error; + throw wrapTimeoutError(error, "Fetch"); } } async function pushWithSetUpstream({ - git, + worktreePath, branch, }: { - git: ReturnType; + worktreePath: string; branch: string; }): Promise { const trimmedBranch = branch.trim(); @@ -68,11 +82,22 @@ async function pushWithSetUpstream({ // Use HEAD refspec to avoid resolving the branch name as a local ref. // This is more reliable for worktrees where upstream tracking isn't set yet. - await git.push([ - "--set-upstream", - "origin", - `HEAD:refs/heads/${trimmedBranch}`, - ]); + try { + await execFileAsync( + "git", + [ + "-C", + worktreePath, + "push", + "--set-upstream", + "origin", + `HEAD:refs/heads/${trimmedBranch}`, + ], + { timeout: GIT_TIMEOUT_NETWORK_HEAVY }, + ); + } catch (error) { + throw wrapTimeoutError(error, "Push"); + } } function shouldRetryPushWithUpstream(message: string): boolean { @@ -125,22 +150,30 @@ export const createGitOperationsRouter = () => { if (input.setUpstream && !hasUpstream) { const branch = await git.revparse(["--abbrev-ref", "HEAD"]); - await pushWithSetUpstream({ git, branch }); + await pushWithSetUpstream({ + worktreePath: input.worktreePath, + branch, + }); } else { try { - await git.push(); + await execFileAsync("git", ["-C", input.worktreePath, "push"], { + timeout: GIT_TIMEOUT_NETWORK_HEAVY, + }); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (shouldRetryPushWithUpstream(message)) { const branch = await git.revparse(["--abbrev-ref", "HEAD"]); - await pushWithSetUpstream({ git, branch }); + await pushWithSetUpstream({ + worktreePath: input.worktreePath, + branch, + }); } else { - throw error; + throw wrapTimeoutError(error, "Push"); } } } - await fetchCurrentBranch(git); + await fetchCurrentBranch(input.worktreePath); return { success: true }; }), @@ -153,9 +186,12 @@ export const createGitOperationsRouter = () => { .mutation(async ({ input }): Promise<{ success: boolean }> => { assertRegisteredWorktree(input.worktreePath); - const git = simpleGit(input.worktreePath); try { - await git.pull(["--rebase"]); + await execFileAsync( + "git", + ["-C", input.worktreePath, "pull", "--rebase"], + { timeout: GIT_TIMEOUT_NETWORK_HEAVY }, + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -164,7 +200,7 @@ export const createGitOperationsRouter = () => { "No upstream branch to pull from. The remote branch may have been deleted.", ); } - throw error; + throw wrapTimeoutError(error, "Pull"); } return { success: true }; }), @@ -180,20 +216,33 @@ export const createGitOperationsRouter = () => { const git = simpleGit(input.worktreePath); try { - await git.pull(["--rebase"]); + await execFileAsync( + "git", + ["-C", input.worktreePath, "pull", "--rebase"], + { timeout: GIT_TIMEOUT_NETWORK_HEAVY }, + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (isUpstreamMissingError(message)) { const branch = await git.revparse(["--abbrev-ref", "HEAD"]); - await pushWithSetUpstream({ git, branch }); - await fetchCurrentBranch(git); + await pushWithSetUpstream({ + worktreePath: input.worktreePath, + branch, + }); + await fetchCurrentBranch(input.worktreePath); return { success: true }; } - throw error; + throw wrapTimeoutError(error, "Sync"); + } + try { + await execFileAsync("git", ["-C", input.worktreePath, "push"], { + timeout: GIT_TIMEOUT_NETWORK_HEAVY, + }); + } catch (error) { + throw wrapTimeoutError(error, "Push"); } - await git.push(); - await fetchCurrentBranch(git); + await fetchCurrentBranch(input.worktreePath); return { success: true }; }), @@ -201,8 +250,7 @@ export const createGitOperationsRouter = () => { .input(z.object({ worktreePath: z.string() })) .mutation(async ({ input }): Promise<{ success: boolean }> => { assertRegisteredWorktree(input.worktreePath); - const git = simpleGit(input.worktreePath); - await fetchCurrentBranch(git); + await fetchCurrentBranch(input.worktreePath); return { success: true }; }), @@ -222,18 +270,26 @@ export const createGitOperationsRouter = () => { // Ensure branch is pushed first if (!hasUpstream) { - await pushWithSetUpstream({ git, branch }); + await pushWithSetUpstream({ + worktreePath: input.worktreePath, + branch, + }); } else { // Push any unpushed commits try { - await git.push(); + await execFileAsync("git", ["-C", input.worktreePath, "push"], { + timeout: GIT_TIMEOUT_NETWORK_HEAVY, + }); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (shouldRetryPushWithUpstream(message)) { - await pushWithSetUpstream({ git, branch }); + await pushWithSetUpstream({ + worktreePath: input.worktreePath, + branch, + }); } else { - throw error; + throw wrapTimeoutError(error, "Push"); } } } @@ -252,7 +308,7 @@ export const createGitOperationsRouter = () => { const url = `https://github.com/${repo}/compare/${branch}?expand=1`; await shell.openExternal(url); - await fetchCurrentBranch(git); + await fetchCurrentBranch(input.worktreePath); return { success: true, url }; }, diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git-timeouts.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git-timeouts.ts new file mode 100644 index 00000000000..4fe0e51a229 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git-timeouts.ts @@ -0,0 +1,28 @@ +/** 10s — config reads, rev-parse, local-only commands */ +export const GIT_TIMEOUT_LOCAL = 10_000; + +/** 30s — fetch single branch, ls-remote, gh API calls */ +export const GIT_TIMEOUT_NETWORK = 30_000; + +/** 60s — push, pull (significant data transfer) */ +export const GIT_TIMEOUT_NETWORK_HEAVY = 60_000; + +/** 120s — worktree creation, large fetches (e.g. fork PRs) */ +export const GIT_TIMEOUT_LONG = 120_000; + +export function isTimeoutError(error: unknown): boolean { + return ( + error instanceof Error && + "killed" in error && + (error as any).killed === true + ); +} + +export function wrapTimeoutError(error: unknown, operation: string): Error { + if (isTimeoutError(error)) { + return new Error( + `${operation} timed out. Check your network connection and try again.`, + ); + } + return error instanceof Error ? error : new Error(String(error)); +} diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts index 70bad22f411..634e0629674 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts @@ -8,6 +8,12 @@ import friendlyWords = require("friendly-words"); import type { BranchPrefixMode } from "@superset/local-db"; import simpleGit, { type StatusResult } from "simple-git"; +import { + GIT_TIMEOUT_LOCAL, + GIT_TIMEOUT_LONG, + GIT_TIMEOUT_NETWORK, + wrapTimeoutError, +} from "./git-timeouts"; import { checkGitLfsAvailable, getShellEnvironment } from "./shell-env"; const execFileAsync = promisify(execFile); @@ -76,7 +82,7 @@ export async function getStatusNoLock(repoPath: string): Promise { "-z", "-uall", ], - { env, timeout: 30_000 }, + { env, timeout: GIT_TIMEOUT_NETWORK }, ); return parsePortelainStatus(stdout); @@ -331,7 +337,7 @@ export async function getGitHubUsername( const { stdout } = await execFileAsync( "gh", ["api", "user", "--jq", ".login"], - { env, timeout: 10_000 }, + { env, timeout: GIT_TIMEOUT_LOCAL }, ); const value = stdout.trim() || null; cachedGitHubUsername = { value, timestamp: Date.now() }; @@ -472,7 +478,7 @@ export async function createWorktree( // creating a new branch from a remote branch like origin/main. `${startPoint}^{commit}`, ], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); // Enable autoSetupRemote so the first `git push` automatically creates @@ -480,7 +486,7 @@ export async function createWorktree( await execFileAsync( "git", ["-C", worktreePath, "config", "--local", "push.autoSetupRemote", "true"], - { env, timeout: 10_000 }, + { env, timeout: GIT_TIMEOUT_LOCAL }, ); console.log( @@ -568,7 +574,7 @@ export async function createWorktreeFromExistingBranch({ await execFileAsync( "git", ["-C", mainRepoPath, "worktree", "add", worktreePath, branch], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); } else { // Branch doesn't exist locally - check if it's a remote branch @@ -590,7 +596,7 @@ export async function createWorktreeFromExistingBranch({ worktreePath, remoteBranchName, ], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); } else { throw new Error( @@ -604,7 +610,7 @@ export async function createWorktreeFromExistingBranch({ await execFileAsync( "git", ["-C", worktreePath, "config", "--local", "push.autoSetupRemote", "true"], - { env, timeout: 10_000 }, + { env, timeout: GIT_TIMEOUT_LOCAL }, ); console.log( @@ -674,7 +680,7 @@ export async function deleteLocalBranch({ try { await execFileAsync("git", ["-C", mainRepoPath, "branch", "-D", branch], { env, - timeout: 10_000, + timeout: GIT_TIMEOUT_LOCAL, }); console.log(`[workspace/delete] Deleted local branch "${branch}"`); } catch (error) { @@ -705,7 +711,7 @@ export async function removeWorktree( await execFileAsync("git", ["-C", mainRepoPath, "worktree", "prune"], { env, - timeout: 10_000, + timeout: GIT_TIMEOUT_LOCAL, }); // Delete the moved directory in the background — don't block the caller. @@ -737,7 +743,7 @@ export async function removeWorktree( const env = await getGitEnv(); await execFileAsync("git", ["-C", mainRepoPath, "worktree", "prune"], { env, - timeout: 10_000, + timeout: GIT_TIMEOUT_LOCAL, }); } catch {} return; @@ -911,8 +917,13 @@ export async function getDefaultBranch(mainRepoPath: string): Promise { // Try ls-remote as last resort for remote repos try { - const result = await git.raw(["ls-remote", "--symref", "origin", "HEAD"]); - const symrefMatch = result.match(/ref:\s+refs\/heads\/(.+?)\tHEAD/); + const env = await getGitEnv(); + const { stdout } = await execFileAsync( + "git", + ["-C", mainRepoPath, "ls-remote", "--symref", "origin", "HEAD"], + { env, timeout: GIT_TIMEOUT_NETWORK }, + ); + const symrefMatch = stdout.match(/ref:\s+refs\/heads\/(.+?)\tHEAD/); if (symrefMatch) { return symrefMatch[1]; } @@ -948,8 +959,17 @@ export async function fetchDefaultBranch( mainRepoPath: string, defaultBranch: string, ): Promise { + const env = await getGitEnv(); + try { + await execFileAsync( + "git", + ["-C", mainRepoPath, "fetch", "origin", defaultBranch], + { env, timeout: GIT_TIMEOUT_NETWORK }, + ); + } catch (error) { + throw wrapTimeoutError(error, "Fetch default branch"); + } const git = simpleGit(mainRepoPath); - await git.fetch("origin", defaultBranch); const commit = await git.revparse(`origin/${defaultBranch}`); return commit.trim(); } @@ -964,6 +984,7 @@ export async function refreshDefaultBranch( mainRepoPath: string, ): Promise { const git = simpleGit(mainRepoPath); + const env = await getGitEnv(); const hasRemote = await hasOriginRemote(mainRepoPath); if (!hasRemote) { @@ -973,7 +994,11 @@ export async function refreshDefaultBranch( try { // Git doesn't auto-update origin/HEAD on fetch, so we must explicitly // sync it to detect when the remote's default branch changes - await git.remote(["set-head", "origin", "--auto"]); + await execFileAsync( + "git", + ["-C", mainRepoPath, "remote", "set-head", "origin", "--auto"], + { env, timeout: GIT_TIMEOUT_NETWORK }, + ); const headRef = await git.raw(["symbolic-ref", "refs/remotes/origin/HEAD"]); const match = headRef.trim().match(/refs\/remotes\/origin\/(.+)/); @@ -984,8 +1009,12 @@ export async function refreshDefaultBranch( // set-head requires network access; fall back to ls-remote which may // work in some edge cases or provide a more specific error try { - const result = await git.raw(["ls-remote", "--symref", "origin", "HEAD"]); - const symrefMatch = result.match(/ref:\s+refs\/heads\/(.+?)\tHEAD/); + const { stdout } = await execFileAsync( + "git", + ["-C", mainRepoPath, "ls-remote", "--symref", "origin", "HEAD"], + { env, timeout: GIT_TIMEOUT_NETWORK }, + ); + const symrefMatch = stdout.match(/ref:\s+refs\/heads\/(.+?)\tHEAD/); if (symrefMatch) { return symrefMatch[1]; } @@ -1145,7 +1174,7 @@ export async function branchExistsOnRemote( "origin", branchName, ], - { env, timeout: 30_000 }, + { env, timeout: GIT_TIMEOUT_NETWORK }, ); // Exit code 0 = branch exists (--exit-code flag ensures this) return { status: "exists" }; @@ -1266,9 +1295,13 @@ export async function listBranches( // Optionally fetch and prune to get up-to-date remote refs if (options?.fetch) { try { - await git.fetch(["--prune"]); + const env = await getGitEnv(); + await execFileAsync("git", ["-C", repoPath, "fetch", "--prune"], { + env, + timeout: GIT_TIMEOUT_NETWORK, + }); } catch { - // Ignore fetch errors (e.g., offline) + // Ignore fetch errors (e.g., offline or timeout) } } @@ -1359,10 +1392,13 @@ export async function checkBranchCheckoutSafety( // Fetch and prune stale remote refs (best-effort, ignore errors if offline) try { - const git = simpleGit(repoPath); - await git.fetch(["--prune"]); + const env = await getGitEnv(); + await execFileAsync("git", ["-C", repoPath, "fetch", "--prune"], { + env, + timeout: GIT_TIMEOUT_NETWORK, + }); } catch { - // Ignore fetch errors + // Ignore fetch errors (e.g., offline or timeout) } return { @@ -1572,7 +1608,7 @@ export async function getPrInfo({ "--json", "number,title,headRefName,headRepository,headRepositoryOwner,isCrossRepository", ], - { env, timeout: 30_000 }, + { env, timeout: GIT_TIMEOUT_NETWORK }, ); return JSON.parse(stdout) as PullRequestInfo; @@ -1635,7 +1671,7 @@ export async function fetchPrBranch({ await execFileAsync( "git", ["-C", repoPath, "fetch", remoteName, headBranch], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); return `${remoteName}/${headBranch}`; @@ -1644,7 +1680,7 @@ export async function fetchPrBranch({ await execFileAsync( "git", ["-C", repoPath, "fetch", "origin", prInfo.headRefName], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); return prInfo.headRefName; @@ -1698,7 +1734,7 @@ export async function createWorktreeFromPr({ localCommit, remoteCommit, ], - { env, timeout: 10_000 }, + { env, timeout: GIT_TIMEOUT_LOCAL }, ); } catch { throw new Error( @@ -1711,14 +1747,14 @@ export async function createWorktreeFromPr({ await execFileAsync( "git", ["-C", mainRepoPath, "worktree", "add", worktreePath, branchName], - { env, timeout: 120_000 }, + { env, timeout: GIT_TIMEOUT_LONG }, ); if (localCommit !== remoteCommit) { await execFileAsync( "git", ["-C", worktreePath, "reset", "--hard", remoteRef], - { env, timeout: 30_000 }, + { env, timeout: GIT_TIMEOUT_NETWORK }, ); } } else { @@ -1727,14 +1763,14 @@ export async function createWorktreeFromPr({ args.push("--track"); } args.push("-b", branchName, worktreePath, remoteRef); - await execFileAsync("git", args, { env, timeout: 120_000 }); + await execFileAsync("git", args, { env, timeout: GIT_TIMEOUT_LONG }); } // Enable autoSetupRemote so `git push` just works without -u flag. await execFileAsync( "git", ["-C", worktreePath, "config", "--local", "push.autoSetupRemote", "true"], - { env, timeout: 10_000 }, + { env, timeout: GIT_TIMEOUT_LOCAL }, ); console.log( diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index a8dfb852c39..49a30e11c52 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -2,6 +2,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; import type { CheckItem, GitHubStatus } from "@superset/local-db"; import { branchExistsOnRemote } from "../git"; +import { GIT_TIMEOUT_LOCAL } from "../git-timeouts"; import { execWithShellEnv } from "../shell-env"; import { type GHPRResponse, @@ -145,7 +146,7 @@ async function findPRByHeadCommit( const { stdout: headOutput } = await execFileAsync( "git", ["-C", worktreePath, "rev-parse", "HEAD"], - { timeout: 10_000 }, + { timeout: GIT_TIMEOUT_LOCAL }, ); const headSha = headOutput.trim(); @@ -234,7 +235,7 @@ async function sharesAncestry( const { stdout: localHead } = await execFileAsync( "git", ["-C", worktreePath, "rev-parse", "HEAD"], - { timeout: 10_000 }, + { timeout: GIT_TIMEOUT_LOCAL }, ); const localOid = localHead.trim(); @@ -258,7 +259,7 @@ async function sharesAncestry( ancestor, descendant, ], - { timeout: 10_000 }, + { timeout: GIT_TIMEOUT_LOCAL }, ); return true; } catch { diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts index c59efa72d51..43f03ab0f9b 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts @@ -4,6 +4,7 @@ import { } from "node:child_process"; import os from "node:os"; import { promisify } from "node:util"; +import { GIT_TIMEOUT_LOCAL, GIT_TIMEOUT_NETWORK } from "./git-timeouts"; const execFileAsync = promisify(execFile); @@ -46,7 +47,7 @@ export async function getShellEnvironment(): Promise> { // -c: execute command // Avoids -i (interactive) to skip TTY prompts and reduce latency const { stdout } = await execFileAsync(shell, ["-lc", "env"], { - timeout: 10_000, + timeout: GIT_TIMEOUT_LOCAL, env: { ...process.env, HOME: os.homedir(), @@ -124,8 +125,13 @@ export async function execWithShellEnv( args: string[], options?: Omit, ): Promise<{ stdout: string; stderr: string }> { + const defaultTimeout = options?.timeout ?? GIT_TIMEOUT_NETWORK; try { - return await execFileAsync(cmd, args, { ...options, encoding: "utf8" }); + return await execFileAsync(cmd, args, { + ...options, + timeout: defaultTimeout, + encoding: "utf8", + }); } catch (error) { // Only retry on ENOENT (command not found), only on macOS // Skip if we've already successfully fixed PATH, or if a fix attempt is in progress @@ -160,6 +166,7 @@ export async function execWithShellEnv( return await execFileAsync(cmd, args, { ...options, + timeout: defaultTimeout, encoding: "utf8", env: retryEnv, });