fix(desktop): increase git status maxBuffer to prevent stdout overflow#1236
fix(desktop): increase git status maxBuffer to prevent stdout overflow#1236saddlepaddle wants to merge 1 commit into
Conversation
Repos with very large numbers of changed/untracked files exceed the default Node.js maxBuffer (1 MiB) causing ERR_CHILD_PROCESS_STDIO_MAXBUFFER crashes (Sentry DESKTOP-1J, 2,117 events). - Set maxBuffer to 50 MiB for the git status execFileAsync call - Add automatic fallback from -uall to -unormal when buffer is exceeded, which collapses untracked directories into single entries - Extract runGitStatus helper and error-mapping utility for cleaner flow
📝 WalkthroughWalkthroughThis change enhances git status retrieval in a desktop workspace utility by introducing a 50 MiB buffer limit, refactoring the logic into a dedicated helper function, and implementing retry logic with fallback modes when buffer limits are exceeded. It also adds centralized error translation for user-friendly messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getStatusNoLock
participant runGitStatus
participant Git as Git Process
participant ErrorHandler as Error Handler
Caller->>getStatusNoLock: Request status
getStatusNoLock->>runGitStatus: Execute with -uall
runGitStatus->>Git: execFileAsync git status -uall
alt Normal Success
Git-->>runGitStatus: Status output
runGitStatus-->>getStatusNoLock: StatusResult
getStatusNoLock-->>Caller: Parsed status
else MaxBuffer Exceeded
Git-->>runGitStatus: ENAMETOOLONG Error
runGitStatus-->>ErrorHandler: Detect maxBuffer error
ErrorHandler->>getStatusNoLock: Log warning & retry signal
getStatusNoLock->>runGitStatus: Execute with -unormal
runGitStatus->>Git: execFileAsync git status -unormal
Git-->>runGitStatus: Status output (reduced)
runGitStatus-->>getStatusNoLock: StatusResult
getStatusNoLock-->>Caller: Parsed status
else Other Error
Git-->>runGitStatus: Other error
runGitStatus-->>ErrorHandler: toGitStatusError mapping
ErrorHandler-->>getStatusNoLock: User-friendly error
getStatusNoLock-->>Caller: Error message
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 69-91: The catch currently wraps errors via toGitStatusError
without logging the original; update the block around runGitStatus so the
original error is logged with context before any rethrow or retry: when entering
the catch log the initial error using a prefixed message like "[git/status] git
status failed" along with repoPath, env and the error; if isMaxBufferError
triggers, also log the initial max-buffer error before retrying and then log the
retryError before throwing the wrapped error from toGitStatusError. Use the same
context pattern for all logs and reference the existing symbols runGitStatus,
isMaxBufferError, toGitStatusError, repoPath, env and untrackedMode so the added
logs appear just prior to each throw/retry.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
125-144: Prefer error.code for maxBuffer detection and extract identifiers to constants.Node.js emits the specific error code
ERR_CHILD_PROCESS_STDIO_MAXBUFFERfor maxBuffer overflows; checkingerror.codeis more robust than relying on the error message string. Extract the hardcoded identifiers to named constants at the module top to align with the coding guideline on magic strings.♻️ Suggested patch
const GIT_STATUS_MAX_BUFFER = 50 * 1024 * 1024; +const MAX_BUFFER_ERROR_CODE = "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; +const MAX_BUFFER_ERROR_MESSAGE = "maxBuffer length exceeded"; function isMaxBufferError(error: unknown): boolean { - return ( - error instanceof Error && - error.message.includes("maxBuffer length exceeded") - ); + return ( + isExecFileException(error) && + (error.code === MAX_BUFFER_ERROR_CODE || + error.message.includes(MAX_BUFFER_ERROR_MESSAGE)) + ); }
| try { | ||
| // Run git status with --no-optional-locks to avoid holding locks | ||
| // Use porcelain=v1 for machine-parseable output, -b for branch info | ||
| // Use -z for NUL-terminated output (handles filenames with special chars) | ||
| // Use -uall to show individual files in untracked directories (not just the directory) | ||
| // Note: porcelain=v1 already includes rename info (R/C status codes) without needing -M | ||
| const { stdout } = await execFileAsync( | ||
| "git", | ||
| [ | ||
| "--no-optional-locks", | ||
| "-C", | ||
| repoPath, | ||
| "status", | ||
| "--porcelain=v1", | ||
| "-b", | ||
| "-z", | ||
| "-uall", | ||
| ], | ||
| { env, timeout: 30_000 }, | ||
| ); | ||
|
|
||
| return parsePortelainStatus(stdout); | ||
| return await runGitStatus({ repoPath, env, untrackedMode: "-uall" }); | ||
| } catch (error) { | ||
| // Provide more descriptive error messages | ||
| if (isExecFileException(error)) { | ||
| if (error.code === "ENOENT") { | ||
| throw new Error("Git is not installed or not found in PATH"); | ||
| } | ||
| const stderr = error.stderr || error.message || ""; | ||
| if (stderr.includes("not a git repository")) { | ||
| throw new Error(`Not a git repository: ${repoPath}`); | ||
| // If maxBuffer is exceeded even with the generous limit, retry with | ||
| // -unormal (collapses untracked directories into a single entry). | ||
| if (isMaxBufferError(error)) { | ||
| console.warn( | ||
| "[git/status] maxBuffer exceeded with -uall, retrying with -unormal:", | ||
| repoPath, | ||
| ); | ||
| try { | ||
| return await runGitStatus({ | ||
| repoPath, | ||
| env, | ||
| untrackedMode: "-unormal", | ||
| }); | ||
| } catch (retryError) { | ||
| throw toGitStatusError(retryError); | ||
| } | ||
| } | ||
| throw new Error( | ||
| `Failed to get git status: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
|
|
||
| throw toGitStatusError(error); | ||
| } |
There was a problem hiding this comment.
Log original git‑status failures before wrapping.
The catch block wraps errors into a new Error without logging the original, which drops diagnostics. Please log with context before throwing so failures aren’t silent in logs.
🔧 Suggested patch
if (isMaxBufferError(error)) {
console.warn(
"[git/status] maxBuffer exceeded with -uall, retrying with -unormal:",
repoPath,
);
try {
return await runGitStatus({
repoPath,
env,
untrackedMode: "-unormal",
});
} catch (retryError) {
+ console.error(
+ "[git/status] git status failed after -unormal retry:",
+ repoPath,
+ retryError,
+ );
throw toGitStatusError(retryError);
}
}
+ console.error("[git/status] git status failed:", repoPath, error);
throw toGitStatusError(error);
}As per coding guidelines, “Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly” and “Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions”.
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 69 -
91, The catch currently wraps errors via toGitStatusError without logging the
original; update the block around runGitStatus so the original error is logged
with context before any rethrow or retry: when entering the catch log the
initial error using a prefixed message like "[git/status] git status failed"
along with repoPath, env and the error; if isMaxBufferError triggers, also log
the initial max-buffer error before retrying and then log the retryError before
throwing the wrapped error from toGitStatusError. Use the same context pattern
for all logs and reference the existing symbols runGitStatus, isMaxBufferError,
toGitStatusError, repoPath, env and untrackedMode so the added logs appear just
prior to each throw/retry.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review: fix(desktop): increase git status maxBuffer to prevent stdout overflow
Verdict: Approve — clean, well-structured fix for a real crash (2,117 Sentry events).
What's good
- Refactoring is solid: Extracting
runGitStatusas a helper with a typeduntrackedModeparam ("-uall" | "-unormal") is clean and follows the project's object-parameter convention for 2+ args. - Fallback strategy is smart: Retrying with
-unormal(collapses untracked dirs) is a graceful degradation — users still get status info, just less granular for untracked files. - Buffer size is reasonable: 50 MiB is generous but appropriate for very large monorepos. Git status output is transient, so this won't cause sustained memory pressure.
- Error centralization:
toGitStatusErrorkeeps the error-mapping logic DRY between the initial attempt and retry path. - Logging follows conventions:
[git/status]prefix withconsole.warnper the codebase logging patterns.
Minor observations (non-blocking)
-
isMaxBufferErrorstring matching vs error code: CodeRabbit flagged this too — Node.js setserror.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"on these errors, which is more robust than substring matching onerror.message. The current approach will work fine in practice, but checkingcodefirst (with message as fallback) would be slightly more defensive. Up to you whether to address. -
repoPathdropped from "Not a git repository" error: The original code hadNot a git repository: ${repoPath}, buttoGitStatusErrorreturnsNot a git repositorywithout the path. This loses some diagnostic context. Consider adding the path back — you could either passrepoPathintotoGitStatusError, or have the caller append it. -
No new tests for the new helpers:
isMaxBufferError,toGitStatusError, and the retry logic are all simple enough that they don't strictly need tests, but unit tests for the error detection and mapping would strengthen confidence (especially since these are the core of the fix). Worth considering for a follow-up.
Overall this is a clean, targeted fix. Nice work.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review: fix(desktop): increase git status maxBuffer to prevent stdout overflow
Verdict: Approve — solid bug fix with a well-designed fallback mechanism.
What this PR does
- Increases
maxBufferfrom Node.js default (1 MiB) to 50 MiB forgit statusingetStatusNoLock - Adds automatic retry: if
-uallexceeds the buffer, falls back to-unormal(collapses untracked directories into single entries) - Extracts
runGitStatus,isMaxBufferError, andtoGitStatusErrorhelpers for cleaner code
Strengths
- Correct fix for the root cause — 2,117 Sentry events from
ERR_CHILD_PROCESS_STDIO_MAXBUFFERis a real problem in large repos - Smart fallback strategy —
-uall→-unormalgracefully degrades instead of crashing, and theconsole.warnprovides visibility - Clean refactoring —
runGitStatususes typeduntrackedMode: "-uall" | "-unormal"and follows the project's object-params convention - Buffer size is appropriate — 50 MiB is generous but reasonable; even monorepos with 100k+ changed files would fit within this
- CI green — build, lint, test, typecheck all pass
Minor suggestions (non-blocking)
-
isMaxBufferErrorcould also checkerror.code— Node.js setserror.codeto"ERR_CHILD_PROCESS_STDIO_MAXBUFFER"for this specific error, which is more reliable than string matching onerror.message. Consider:function isMaxBufferError(error: unknown): boolean { if (!(error instanceof Error)) return false; return ( (error as any).code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER" || error.message.includes("maxBuffer length exceeded") ); }
Checking both provides maximum compatibility across Node versions.
-
toGitStatusErrordropsrepoPathfrom "Not a git repository" error — The original code includedrepoPathin this error message (Not a git repository: ${repoPath}), which is helpful for debugging. SincetoGitStatusErrordoesn't receiverepoPath, this context is lost. Consider passingrepoPathtotoGitStatusErroror appending it at the call site.
Both are minor — the fix is correct and well-structured as-is.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review: Approve
Clean, well-scoped fix for a real production issue (Sentry DESKTOP-1J, 2,117 events).
What's good
- Correct fix: Increasing
maxBufferfrom Node.js default (1 MiB) to 50 MiB addresses the root cause. 50 MiB is generous but reasonable as a ceiling forgit statusoutput on large monorepos. - Graceful degradation: The fallback from
-uallto-unormalwhen the buffer is still exceeded is a smart tradeoff — collapses untracked directories into single entries rather than crashing entirely. - Clean refactoring: Extraction of
runGitStatus,isMaxBufferError, andtoGitStatusErrorhelpers keepsgetStatusNoLockreadable. The typeduntrackedMode: "-uall" | "-unormal"union is a nice touch. - Follows project conventions: Object parameter style,
[git/status]log prefix, named constant with doc comment.
Minor note (non-blocking)
toGitStatusError returns new Error("Not a git repository") without including repoPath, while the original had `Not a git repository: ${repoPath}`. This is a small loss of debugging context. Consider threading repoPath through if it's useful for production diagnostics, but not a blocker.
LGTM — ship it.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review: fix(desktop): increase git status maxBuffer to prevent stdout overflow
Verdict: Approve — solid, well-structured bug fix for a real crash (2,117 Sentry events).
What's good
- Smart fallback strategy:
-uall→-unormalon buffer overflow is the right degradation — users get slightly less granular untracked file info instead of a crash. - Clean refactoring: Extracting
runGitStatus,isMaxBufferError, andtoGitStatusErrorkeepsgetStatusNoLockreadable and each helper single-purpose. - Follows project conventions: Object params for 2+ args in
runGitStatus,[git/status]log prefix, typed union foruntrackedMode. - 50 MiB is reasonable: Large monorepos with
git status -uallcan produce 10-15 MB; 50 MiB gives plenty of headroom without memory concerns.
Minor suggestions (non-blocking)
-
repoPathdropped from "Not a git repository" error —toGitStatusErrorreturnsnew Error("Not a git repository")but the original included the path:new Error(`Not a git repository: ${repoPath}`). Consider threadingrepoPathintotoGitStatusErrorto preserve this diagnostic context, or passing it as context to the error message. This helps when debugging issues across multiple repositories/worktrees. -
isMaxBufferErrorstring matching — CodeRabbit flagged this too. Node.js setserror.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"on these errors. Checking the error code is more robust than string matching onerror.message. A belt-and-suspenders approach:function isMaxBufferError(error: unknown): boolean { if (!(error instanceof Error)) return false; return ( ("code" in error && (error as any).code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER") || error.message.includes("maxBuffer length exceeded") ); }
Not a big deal since the string approach works, but worth considering for resilience across Node versions.
-
No tests for the new fallback path — The existing 12 tests pass, but none exercise the maxBuffer→retry behavior. A test that mocks
execFileAsyncto throw a maxBuffer error on the first call and succeed on the second would give confidence the retry works correctly. Not blocking, but would be a nice follow-up.
Overall, clean fix that addresses a real user-facing crash with a well-thought-out fallback. Ship it!
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Review: LGTM with minor suggestions
Good fix for DESKTOP-1J (2,117 events). The approach is well-reasoned:
- 50 MiB buffer ceiling is generous but appropriate — repos with massive
node_modulesor generated files easily exceed 1 MiB. - Graceful fallback from
-uallto-unormalis a smart degradation strategy. Collapsing untracked directories is a reasonable trade-off vs crashing. - Extracted helpers (
runGitStatus,isMaxBufferError,toGitStatusError) clean up the code nicely.runGitStatusproperly uses the object-params convention. - Logging follows
[domain/operation]convention.
Minor suggestions (non-blocking)
-
Dropped
repoPathfrom error message —toGitStatusErrorreturnsnew Error("Not a git repository")but the original included the path:Not a git repository: ${repoPath}. SincetoGitStatusErrordoesn't have access torepoPath, this is an acceptable trade-off, but worth noting that diagnostic context was lost. Could passrepoPathintotoGitStatusErrorif you think it's valuable. -
Error detection could use error code —
isMaxBufferErrorcheckserror.message.includes("maxBuffer length exceeded"). This works, but checking(error as any).code === 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'would be more robust against Node.js message wording changes in future versions. Very minor nit.
Both are non-blocking. Ship it.
2 participants
Summary
getStatusNoLockcrashes withstdout maxBuffer length exceededmaxBufferfrom the Node.js default (1 MiB) to 50 MiB for thegit statuschild process-uallto-unormalif the buffer is still exceeded, which collapses untracked directories into single entries instead of listing every fileTest plan
bun test— 12 tests in git.test.ts, 43 tests in changes/)Summary by CodeRabbit