fix(desktop): add timeouts to git/GitHub network operations#1346
fix(desktop): add timeouts to git/GitHub network operations#1346Kitenite wants to merge 1 commit into
Conversation
…t indefinite hangs
📝 WalkthroughWalkthroughThe changes introduce a centralized timeout management system for Git operations by creating a new utilities module with configurable timeout constants (local, network, and heavy operations). The main git-operations module is refactored to replace direct simpleGit usage with execFileAsync calls using worktreePath, incorporating the new timeout constants and error wrapping. Various utility modules are updated to use the centralized timeouts instead of hard-coded values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
36-40:⚠️ Potential issue | 🟡 MinorMissing timeout on local
rev-parsecall.This
execFileAsynccall forrev-parse --abbrev-ref HEADdoesn't have a timeout, while the same command infindPRByHeadCommit(Line 149) andsharesAncestry(Line 238) usesGIT_TIMEOUT_LOCAL. Add the timeout for consistency.Suggested fix
const { stdout: branchOutput } = await execFileAsync( "git", ["rev-parse", "--abbrev-ref", "HEAD"], - { cwd: worktreePath }, + { cwd: worktreePath, timeout: GIT_TIMEOUT_LOCAL }, );apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
32-65:⚠️ Potential issue | 🟠 MajorMissing
envpropagation inexecFileAsynccalls — inconsistent withgit.ts.All
execFileAsync("git", ...)calls in this file omit theenvoption, whilegit.tsconsistently passes{ env: await getGitEnv(), timeout: ... }for the same types of operations. On macOS GUI apps (launched from Finder/Dock),process.env.PATHmay not include/usr/local/binor Homebrew paths until the shell-env PATH fix has been triggered.This affects
fetchCurrentBranch(Lines 36-47),pushWithSetUpstream(Line 86), and everyexecFileAsynccall in the tRPC procedures (push, pull, sync, createPR).Example fix for fetchCurrentBranch
You'd need to import
getGitEnv(orgetShellEnvironment) and pass env:+import { getShellEnvironment } from "../workspaces/utils/shell-env"; + +async function getGitEnv(): Promise<Record<string, string>> { + // Or import the one from git.ts if it can be shared + const shellEnv = await getShellEnvironment(); + // ... +} + async function fetchCurrentBranch(worktreePath: string): Promise<void> { const git = simpleGit(worktreePath); + const env = await getGitEnv(); const branch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim(); try { await execFileAsync( "git", ["-C", worktreePath, "fetch", "origin", branch], - { timeout: GIT_TIMEOUT_NETWORK }, + { env, timeout: GIT_TIMEOUT_NETWORK }, );To verify whether
getGitEnvis exported fromgit.tsor could be shared:#!/bin/bash # Check if getGitEnv is exported or only used internally in git.ts rg -n 'getGitEnv' --type=ts -C2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 337-341: The timeout for the GitHub API call using
execFileAsync("gh", ["api", "user", "--jq", ".login"], { env, timeout:
GIT_TIMEOUT_LOCAL }) is too short; change the options to use GIT_TIMEOUT_NETWORK
instead of GIT_TIMEOUT_LOCAL so the execFileAsync call (the "gh api user"
invocation) uses the 30s network timeout constant; ensure GIT_TIMEOUT_NETWORK is
in scope where execFileAsync is used.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
73-86:git statususesGIT_TIMEOUT_NETWORKbut is a local operation.
git status --porcelaindoes not hit the network. TheGIT_TIMEOUT_NETWORKconstant is documented as "30s — fetch single branch, ls-remote, gh API calls." While 30s may be a reasonable budget for large repos with expensive working-tree scans,GIT_TIMEOUT_LOCALis the semantically correct tier. If you want a longer local budget for status specifically, consider a dedicated constant or a comment explaining the choice.apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
208-247:syncprocedure: push error after pull success throws"Push"— consider"Sync (push)"for disambiguation.When
syncfails during the push phase (Line 243), the error says "Push timed out." The user initiated a "Sync" operation, so they might find it confusing. Minor UX consideration — could use"Sync"consistently or"Sync (push phase)"for clarity.
| const { stdout } = await execFileAsync( | ||
| "gh", | ||
| ["api", "user", "--jq", ".login"], | ||
| { env, timeout: 10_000 }, | ||
| { env, timeout: GIT_TIMEOUT_LOCAL }, | ||
| ); |
There was a problem hiding this comment.
gh api user is a network call but uses GIT_TIMEOUT_LOCAL (10s).
gh api user --jq .login hits the GitHub API over the network. Using GIT_TIMEOUT_LOCAL (10s) risks premature timeouts on slow connections. This should use GIT_TIMEOUT_NETWORK (30s), which is documented as the timeout for "gh API calls."
Suggested fix
const { stdout } = await execFileAsync(
"gh",
["api", "user", "--jq", ".login"],
- { env, timeout: GIT_TIMEOUT_LOCAL },
+ { env, timeout: GIT_TIMEOUT_NETWORK },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { stdout } = await execFileAsync( | |
| "gh", | |
| ["api", "user", "--jq", ".login"], | |
| { env, timeout: 10_000 }, | |
| { env, timeout: GIT_TIMEOUT_LOCAL }, | |
| ); | |
| const { stdout } = await execFileAsync( | |
| "gh", | |
| ["api", "user", "--jq", ".login"], | |
| { env, timeout: GIT_TIMEOUT_NETWORK }, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 337 -
341, The timeout for the GitHub API call using execFileAsync("gh", ["api",
"user", "--jq", ".login"], { env, timeout: GIT_TIMEOUT_LOCAL }) is too short;
change the options to use GIT_TIMEOUT_NETWORK instead of GIT_TIMEOUT_LOCAL so
the execFileAsync call (the "gh api user" invocation) uses the 30s network
timeout constant; ensure GIT_TIMEOUT_NETWORK is in scope where execFileAsync is
used.
|
Closing as stale: created in Jan-Mar with no recent activity. If still relevant, re-open or re-create from a fresh branch. Bulk audit 2026-05-06. |
1 participant
Summary
simple-githas no timeout support for network operations (fetch, push, pull, ls-remote, remote set-head)simple-gitcalls withexecFileAsync+ explicit timeouts, and adds a default 30s timeout toexecWithShellEnv(used by allghCLI calls)Changes
New file:
git-timeouts.tsGIT_TIMEOUT_LOCAL(10s),GIT_TIMEOUT_NETWORK(30s),GIT_TIMEOUT_NETWORK_HEAVY(60s),GIT_TIMEOUT_LONG(120s)isTimeoutError()/wrapTimeoutError()helpers for user-friendly error messagesshell-env.tsexecWithShellEnvnow appliesGIT_TIMEOUT_NETWORK(30s) as default timeout — fixes allghCLI calls ingithub.tswhich previously had zero timeoutgit-operations.tssimpleGitnetwork calls infetchCurrentBranch,pushWithSetUpstream,push,pull,sync,createPRwithexecFileAsync+ timeoutfetchCurrentBranchandpushWithSetUpstreamrefactored to acceptworktreePath: stringinstead ofsimpleGitinstancegit.tssimpleGitnetwork calls infetchDefaultBranch,refreshDefaultBranch,getDefaultBranch(ls-remote fallback),listBranches,checkBranchCheckoutSafetywithexecFileAsync+ timeoutMagic number cleanup
10_000,30_000,120_000) acrossgit.ts,github.ts,shell-env.tsreplaced with named constantsTest Plan
ghCLI calls offline → should error within 30sbun run typecheck)bun run lint:fix)Summary by CodeRabbit
Bug Fixes
Refactor