-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: resolve checkpoint initialization timeout in large repositories #7846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,11 @@ import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../../se | |
|
|
||
| export async function getCheckpointService( | ||
| task: Task, | ||
| { interval = 250, timeout = 15_000 }: { interval?: number; timeout?: number } = {}, | ||
| { | ||
| interval = 250, | ||
| timeout = 15_000, | ||
| isInitialCall = false, | ||
| }: { interval?: number; timeout?: number; isInitialCall?: boolean } = {}, | ||
| ) { | ||
| if (!task.enableCheckpoints) { | ||
| return undefined | ||
|
|
@@ -67,13 +71,12 @@ export async function getCheckpointService( | |
| } | ||
|
|
||
| if (task.checkpointServiceInitializing) { | ||
| await pWaitFor( | ||
| () => { | ||
| console.log("[Task#getCheckpointService] waiting for service to initialize") | ||
| return !!task.checkpointService && !!task?.checkpointService?.isInitialized | ||
| }, | ||
| { interval, timeout }, | ||
| ) | ||
| // For initial calls, don't apply timeout to allow large repositories to initialize | ||
| const waitOptions = isInitialCall ? { interval } : { interval, timeout } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right approach? The issue discussion indicates that the problem is subsequent calls timing out while waiting for initialization, not the initial call itself. This implementation removes the timeout for initial calls, but the actual bottleneck (git add during initialization) still has a hard-coded 5-second timeout in ShadowCheckpointService. Could we consider a more comprehensive solution? |
||
| await pWaitFor(() => { | ||
| console.log("[Task#getCheckpointService] waiting for service to initialize") | ||
| return !!task.checkpointService && !!task?.checkpointService?.isInitialized | ||
| }, waitOptions) | ||
| if (!task?.checkpointService) { | ||
| task.enableCheckpoints = false | ||
| return undefined | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,7 +105,21 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
| await git.addConfig("user.name", "Roo Code") | ||
| await git.addConfig("user.email", "[email protected]") | ||
| await this.writeExcludeFile() | ||
| await this.stageAll(git) | ||
|
|
||
| // For initial commit, stage files but with a timeout to prevent hanging on large repos | ||
| // Use a promise race to ensure we don't wait forever | ||
| const stagePromise = this.stageAll(git) | ||
| const timeoutPromise = new Promise<void>((resolve) => { | ||
| setTimeout(() => { | ||
| this.log( | ||
| `[${this.constructor.name}#initShadowGit] Initial staging timed out after 5 seconds, proceeding with empty commit`, | ||
| ) | ||
| resolve() | ||
| }, 5000) // 5 second timeout for initial staging | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good addition of the --ignore-errors flag! However, the 5-second timeout is a magic number. Consider defining it as a constant at the top of the file: This would make it easier to adjust if needed. |
||
| }) | ||
|
|
||
| await Promise.race([stagePromise, timeoutPromise]) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Promise.race() implementation could lead to incomplete staging. If the timeout wins, the staging operation continues in the background but we proceed with a potentially empty commit. This could miss important files. Consider:
|
||
|
|
||
| const { commit } = await git.commit("initial commit", { "--allow-empty": null }) | ||
| this.baseHash = commit | ||
| created = true | ||
|
|
@@ -145,11 +159,14 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
|
|
||
| private async stageAll(git: SimpleGit) { | ||
| try { | ||
| await git.add(".") | ||
| // Add --ignore-errors flag to handle permission issues gracefully | ||
| // This prevents the operation from failing on files with permission issues | ||
| await git.add([".", "--ignore-errors"]) | ||
| } catch (error) { | ||
| this.log( | ||
| `[${this.constructor.name}#stageAll] failed to add files to git: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| // Don't throw - allow the operation to continue with whatever files were successfully staged | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new isInitialCall parameter lacks JSDoc documentation. Could we add documentation explaining its purpose?