fix: remove timeout for initial checkpoint service initialization in large repos #7844
Conversation
…large repos - Added isInitialCall parameter to getCheckpointService function - When isInitialCall is true, no timeout is applied to allow large repositories to complete initialization - Modified initiateTaskLoop to pass isInitialCall=true for the initial checkpoint service call - Updated tests to verify the new behavior Fixes #7843
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| return undefined | ||
| } | ||
| // For initial call, continue waiting or handle the error differently | ||
| throw err |
There was a problem hiding this comment.
Is this intentional? When isInitialCall is true and an error occurs, we're re-throwing it which could cause the entire task initialization to fail. Should we consider logging the error and gracefully disabling checkpoints instead?
| }, waitOptions) | ||
| } catch (err) { | ||
| // Only disable checkpoints if this is not the initial call and we hit a timeout | ||
| if (!isInitialCall) { |
There was a problem hiding this comment.
The error handling here seems incomplete. For initial calls that fail (not just timeout), we're not doing any logging or cleanup before re-throwing. Could we improve this to be more consistent with the non-initial call handling?
| task.enableCheckpoints = false | ||
| return undefined | ||
| } | ||
| // For initial call, continue waiting or handle the error differently |
There was a problem hiding this comment.
This comment says "continue waiting or handle the error differently" but we're actually just throwing the error. Should we update the comment to reflect what's actually happening, or implement the "continue waiting" logic?
| // Should create and return the service | ||
| expect(service).toBe(mockCheckpointService) | ||
| expect(mockTask.checkpointService).toBe(mockCheckpointService) | ||
| }) |
There was a problem hiding this comment.
Could we add a test case for when isInitialCall=true and the initialization actually times out or fails? This would help ensure the error handling works as expected for the initial call scenario.
hannesrudolph
added
the
Issue/PR - Triage
|
@roomote-agent close this pr. |
|
Acknowledged @NaccOll. I will close this PR as requested. |
|
Closing this PR as requested by @NaccOll. |
|
✅ PR #7844 has been successfully closed as requested. The pull request addressing the checkpoint initialization timeout issue for large repositories has been closed without merging. |
Description
This PR fixes an issue where checkpoints cannot be initialized in large repositories due to a hardcoded 15-second timeout limit.
Problem
When checkpointing is enabled for a task, the initialization process has a 15-second timeout. For large repositories, the checkpoint initialization (particularly
initShadowGit()) can take longer than 15 seconds, causing checkpoints to be disabled even though the task hasn't started using editing tools yet.Solution
isInitialCallparameter to thegetCheckpointService()functionisInitialCallis true, no timeout is applied, allowing large repositories to complete initializationinitiateTaskLoop()to passisInitialCall: truefor the initial checkpoint service callgetCheckpointService()maintain the 15-second timeout for backward compatibilityChanges
isInitialCallparameter and conditional timeout logicisInitialCall: truewhen initiating the task loopTesting
isInitialCallflagImpact
This change ensures that users with large repositories can successfully use the checkpoint feature without being affected by initialization timeouts.
Fixes #7843
Important
Adds
isInitialCallparameter togetCheckpointService()to remove timeout for initial checkpoint initialization in large repositories.isInitialCallparameter togetCheckpointService()inindex.tsto bypass timeout for initial calls.initiateTaskLoop()inTask.tsto useisInitialCall: truefor initial checkpoint service call.getCheckpointService().checkpoint.test.tsto verify behavior with and withoutisInitialCallflag.This description was created by
for 8d2c5dd. You can customize this summary. It will automatically update as commits are pushed.