Skip to content

fix: improve error handling for partial message control flow (fixes #7312) #7315

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: improve error handling for partial message control flow (fixes #7312) #7315

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
20 changes: 17 additions & 3 deletions src/core/task/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning"
import { restoreTodoListForTask } from "../tools/updateTodoListTool"
import { AutoApprovalHandler } from "./AutoApprovalHandler"

// Custom error class for expected control flow in partial message handling
Copy link
Contributor Author

@roomote roomote bot Aug 22, 2025

Choose a reason for hiding this comment

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

Consider adding JSDoc documentation to this class to make it more discoverable and explain when it should be used vs regular errors:

Suggested change
// Custom error class for expected control flow in partial message handling
// Custom error class for expected control flow in partial message handling
/**
* Error class for expected control flow behaviors during partial message handling.
* These are not actual errors but normal control flow that uses Promise rejection
* for flow control. Callers typically handle these with .catch(() => {}).
*/
class PartialMessageControlFlowError extends Error {

class PartialMessageControlFlowError extends Error {
Copy link
Contributor Author

@roomote roomote bot Aug 22, 2025

Choose a reason for hiding this comment

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

Consider exporting this error class for use in other modules that might need to distinguish between expected control flow and actual errors. This would improve error handling consistency across the codebase.

constructor(message: string) {
super(message)
this.name = "PartialMessageControlFlowError"
}
Copy link
Contributor Author

@roomote roomote bot Aug 22, 2025

Choose a reason for hiding this comment

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

The error messages use slightly different formats. Consider standardizing them for consistency. Also, instead of hardcoding messages in three places, consider using static methods or constants:

Suggested change
}
class PartialMessageControlFlowError extends Error {
static readonly PARTIAL_UPDATE = "Partial message update - expected behavior"
static readonly NEW_PARTIAL = "New partial message - expected behavior"
static readonly PROMISE_SUPERSEDED = "Ask promise superseded - expected behavior"
constructor(message: string) {
super(message)
this.name = "PartialMessageControlFlowError"
}
}

Then use like: throw new PartialMessageControlFlowError(PartialMessageControlFlowError.PARTIAL_UPDATE)

}

const MAX_EXPONENTIAL_BACKOFF_SECONDS = 600 // 10 minutes
const DEFAULT_USAGE_COLLECTION_TIMEOUT_MS = 5000 // 5 seconds
const FORCED_CONTEXT_REDUCTION_PERCENT = 75 // Keep 75% of context (remove 25%) on context window errors
Expand Down Expand Up @@ -686,14 +694,18 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
// saves, and only post parts of partial message instead of
// whole array in new listener.
this.updateClineMessage(lastMessage)
throw new Error("Current ask promise was ignored (#1)")
// This is expected behavior during partial message updates
// Return early instead of throwing to avoid confusing error logs
throw new PartialMessageControlFlowError("Partial message update - expected behavior")
Copy link
Contributor Author

@roomote roomote bot Aug 22, 2025

Choose a reason for hiding this comment

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

Good use of descriptive error messages that clearly indicate this is expected behavior. This will help both developers and AI models understand these aren't actual failures.

} else {
// This is a new partial message, so add it with partial
// state.
askTs = Date.now()
this.lastMessageTs = askTs
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial, isProtected })
throw new Error("Current ask promise was ignored (#2)")
// This is expected behavior when creating a new partial message
// Return early instead of throwing to avoid confusing error logs
throw new PartialMessageControlFlowError("New partial message - expected behavior")
}
} else {
if (isUpdatingPreviousPartial) {
Expand Down Expand Up @@ -797,7 +809,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
// Could happen if we send multiple asks in a row i.e. with
// command_output. It's important that when we know an ask could
// fail, it is handled gracefully.
throw new Error("Current ask promise was ignored")
// This can happen when multiple asks are sent in sequence
// It's a normal part of the control flow, not an error
throw new PartialMessageControlFlowError("Ask promise superseded - expected behavior")
Copy link
Contributor Author

@roomote roomote bot Aug 22, 2025

Choose a reason for hiding this comment

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

While the PR mentions all tests pass, it would be good to add specific test coverage for this error handling to ensure callers using .catch(() => {}) continue to work as expected with the new error class.

}

const result = { response: this.askResponse!, text: this.askResponseText, images: this.askResponseImages }
Expand Down
Loading