Skip to content

fix: handle pending ask operations during checkpoint restore #8178

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: handle pending ask operations during checkpoint restore #8178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 72 additions & 0 deletions src/core/webview/__tests__/checkpointRestoreHandler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ describe("checkpointRestoreHandler", () => {
mockCline.abort = true
}),
checkpointRestore: vi.fn(),
handleWebviewAskResponse: vi.fn(),
lastMessageTs: undefined,
clineMessages: [
{ ts: 1, type: "user", say: "user", text: "First message" },
{ ts: 2, type: "assistant", say: "assistant", text: "Response" },
Expand Down Expand Up @@ -238,5 +240,75 @@ describe("checkpointRestoreHandler", () => {
"Error during checkpoint restore: Checkpoint restore failed",
)
})

it("should handle pending ask operations before checkpoint restore", async () => {
Copy link
Contributor Author

@roomote roomote bot Sep 19, 2025

Choose a reason for hiding this comment

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

Good test coverage! These tests effectively verify the pending ask handling for both delete and edit operations, as well as the case where no pending ask exists. Consider adding an edge case test where lastMessageTs is set to 0 or a very old timestamp to ensure the logic handles all scenarios correctly.

// Simulate a pending ask operation
mockCline.lastMessageTs = Date.now()

await handleCheckpointRestoreOperation({
provider: mockProvider,
currentCline: mockCline,
messageTs: 3,
messageIndex: 2,
checkpoint: { hash: "abc123" },
operation: "delete",
})

// Verify handleWebviewAskResponse was called to resolve pending ask
expect(mockCline.handleWebviewAskResponse).toHaveBeenCalledWith("messageResponse", "", undefined)

// Verify checkpoint restore was still called
expect(mockCline.checkpointRestore).toHaveBeenCalled()
})

it("should handle pending ask operations for edit operations", async () => {
// Simulate a pending ask operation
mockCline.lastMessageTs = Date.now()

const editData = {
editedContent: "Edited content",
images: [],
apiConversationHistoryIndex: 2,
}

await handleCheckpointRestoreOperation({
provider: mockProvider,
currentCline: mockCline,
messageTs: 3,
messageIndex: 2,
checkpoint: { hash: "abc123" },
operation: "edit",
editData,
})

// Verify handleWebviewAskResponse was called to resolve pending ask
expect(mockCline.handleWebviewAskResponse).toHaveBeenCalledWith("messageResponse", "", undefined)

// Verify checkpoint restore was still called
expect(mockCline.checkpointRestore).toHaveBeenCalled()

// Verify pending edit operation was set
expect(mockProvider.setPendingEditOperation).toHaveBeenCalled()
})

it("should not call handleWebviewAskResponse if no pending ask", async () => {
// No pending ask operation (lastMessageTs is undefined)
mockCline.lastMessageTs = undefined

await handleCheckpointRestoreOperation({
provider: mockProvider,
currentCline: mockCline,
messageTs: 3,
messageIndex: 2,
checkpoint: { hash: "abc123" },
operation: "delete",
})

// Verify handleWebviewAskResponse was NOT called
expect(mockCline.handleWebviewAskResponse).not.toHaveBeenCalled()

// Verify checkpoint restore was still called
expect(mockCline.checkpointRestore).toHaveBeenCalled()
})
})
})
31 changes: 20 additions & 11 deletions src/core/webview/checkpointRestoreHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,27 @@ export async function handleCheckpointRestoreOperation(config: CheckpointRestore
const { provider, currentCline, messageTs, checkpoint, operation, editData } = config

try {
// For delete operations, ensure the task is properly aborted to handle any pending ask operations
// For both delete and edit operations, we need to handle any pending ask operations
// This prevents "Current ask promise was ignored" errors
// For edit operations, we don't abort because the checkpoint restore will handle it
if (operation === "delete" && currentCline && !currentCline.abort) {
currentCline.abortTask()
// Wait a bit for the abort to complete
await pWaitFor(() => currentCline.abort === true, {
timeout: 1000,
interval: 50,
}).catch(() => {
// Continue even if timeout - the abort flag should be set
})
if (currentCline) {
// Handle any pending ask operations by providing a response
Copy link
Contributor Author

@roomote roomote bot Sep 19, 2025

Choose a reason for hiding this comment

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

Consider extracting this pending ask handling logic into a separate helper method like clearPendingAskOperations(currentCline) for better code organization and potential reusability. This would also make the main function cleaner and the intent more explicit.

// This unblocks any waiting ask promises before the checkpoint restore
if (currentCline.lastMessageTs) {
Copy link
Contributor Author

@roomote roomote bot Sep 19, 2025

Choose a reason for hiding this comment

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

Is using lastMessageTs the most reliable way to detect pending ask operations? This property could be set for other reasons. Consider adding a more explicit flag like hasPendingAsk to avoid potential false positives and make the intent clearer.

// Use the public method to set a response for any pending ask
currentCline.handleWebviewAskResponse("messageResponse", "", undefined)
Copy link
Contributor Author

@roomote roomote bot Sep 19, 2025

Choose a reason for hiding this comment

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

The choice to always use "messageResponse" with empty strings to clear pending asks might not be appropriate for all ask types. Could we either:

  1. Use a more generic cancellation approach
  2. Document why "messageResponse" is the universally correct choice here
  3. Consider different response types based on the pending ask type?

}

// For delete operations, abort the task
if (operation === "delete" && !currentCline.abort) {
Copy link
Contributor Author

@roomote roomote bot Sep 19, 2025

Choose a reason for hiding this comment

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

For delete operations, we're now handling pending asks BEFORE aborting the task, which changes the original flow. Have we verified this doesn't introduce any side effects with the abort process? The order of operations can be critical in async cleanup scenarios.

currentCline.abortTask()
// Wait a bit for the abort to complete
await pWaitFor(() => currentCline.abort === true, {
timeout: 1000,
interval: 50,
}).catch(() => {
// Continue even if timeout - the abort flag should be set
})
}
}

// For edit operations, set up pending edit data before restoration
Expand Down
Loading