Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions src/core/checkpoints/__tests__/checkpoint.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest"
import { Task } from "../../task/Task"
import { ClineProvider } from "../../webview/ClineProvider"
import { checkpointSave, checkpointRestore, checkpointDiff, getCheckpointService } from "../index"
Expand Down Expand Up @@ -394,11 +394,40 @@ describe("Checkpoint functionality", () => {
expect(service).toBeUndefined()
})

it("should return undefined if service is still initializing", async () => {
it("should handle initialization with isInitialCall flag", async () => {
// Test that isInitialCall=true doesn't use timeout
mockTask.checkpointService = undefined
mockTask.checkpointServiceInitializing = true
mockTask.checkpointServiceInitializing = false
mockTask.enableCheckpoints = true

// Mock the RepoPerTaskCheckpointService.create to return our mock service
const checkpointsModule = await import("../../../services/checkpoints")
vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create).mockReturnValue(mockCheckpointService)

// Call with isInitialCall=true
const service = await getCheckpointService(mockTask, { isInitialCall: true })

// Should create and return the service
expect(service).toBe(mockCheckpointService)
expect(mockTask.checkpointService).toBe(mockCheckpointService)
})

it("should handle initialization without isInitialCall flag", async () => {
// Test that without isInitialCall, it uses the default timeout
mockTask.checkpointService = undefined
mockTask.checkpointServiceInitializing = false
mockTask.enableCheckpoints = true

// Mock the RepoPerTaskCheckpointService.create to return our mock service
const checkpointsModule = await import("../../../services/checkpoints")
vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create).mockReturnValue(mockCheckpointService)

// Call without isInitialCall (defaults to false)
const service = await getCheckpointService(mockTask)
expect(service).toBeUndefined()

// Should create and return the service
expect(service).toBe(mockCheckpointService)
expect(mockTask.checkpointService).toBe(mockCheckpointService)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


it("should create new service if none exists", async () => {
Expand Down
28 changes: 22 additions & 6 deletions src/core/checkpoints/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -67,13 +71,25 @@ export async function getCheckpointService(
}

if (task.checkpointServiceInitializing) {
await pWaitFor(
() => {
// For initial call during task startup, don't use timeout to allow large repos to initialize
const waitOptions = isInitialCall ? { interval } : { interval, timeout }

try {
await pWaitFor(() => {
console.log("[Task#getCheckpointService] waiting for service to initialize")
return !!task.checkpointService && !!task?.checkpointService?.isInitialized
},
{ interval, timeout },
)
}, waitOptions)
} catch (err) {
// Only disable checkpoints if this is not the initial call and we hit a timeout
if (!isInitialCall) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

log("[Task#getCheckpointService] timeout waiting for service initialization, disabling checkpoints")
task.enableCheckpoints = false
return undefined
}
// For initial call, continue waiting or handle the error differently
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

throw err
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

if (!task?.checkpointService) {
task.enableCheckpoints = false
return undefined
Expand Down
3 changes: 2 additions & 1 deletion src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1661,7 +1661,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

private async initiateTaskLoop(userContent: Anthropic.Messages.ContentBlockParam[]): Promise<void> {
// Kicks off the checkpoints initialization process in the background.
getCheckpointService(this)
// For the initial call, don't use a timeout to allow large repositories to complete initialization
getCheckpointService(this, { isInitialCall: true })

let nextUserContent = userContent
let includeFileDetails = true
Expand Down
Loading