Skip to content

Classify TaskCancelledException as client error instead of system error#5273

Draft
ahkcs wants to merge 4 commits intoopensearch-project:mainfrom
ahkcs:fix/task-cancelled-error-classification
Draft

Classify TaskCancelledException as client error instead of system error#5273
ahkcs wants to merge 4 commits intoopensearch-project:mainfrom
ahkcs:fix/task-cancelled-error-classification

Conversation

@ahkcs
Copy link
Copy Markdown
Collaborator

@ahkcs ahkcs commented Mar 26, 2026 •
edited
Loading

Summary

When a user cancels a PPL query via the _tasks/_cancel API, the TaskCancelledException is wrapped by Calcite (RuntimeException → SQLException → TaskCancelledException) and reaches the REST error handler as a generic exception. This results in:

  • HTTP 500 instead of 400
  • Incrementing PPL_FAILED_REQ_COUNT_SYS (system error metric) instead of PPL_FAILED_REQ_COUNT_CUS (client error metric)

Since query cancellation is user-initiated, it should not be classified as a system error.

Changes

  • Add hasCauseOf() helper to walk the exception cause chain
  • Add TaskCancelledException to isClientError() via cause chain check
  • Add unit tests for wrapped and direct TaskCancelledException

Related

Test plan

  • Unit tests: wrapped TaskCancelledException classified as client error
  • Unit tests: direct TaskCancelledException classified as client error
  • Unit tests: generic RuntimeException still classified as system error

OSD side log:

server    log   [19:02:02.468] [error][plugins][queryEnhancements] pplSearchStrategy: {
  "error": {
    "reason": "Query cancelled",
    "details": "The task is cancelled.",
    "type": "TaskCancelledException"
  },
  "status": 400

@ahkcs
Classify TaskCancelledException as client error instead of system error
f05131b 
When a user cancels a PPL query, the TaskCancelledException is wrapped
by Calcite (RuntimeException → SQLException → TaskCancelledException)
and reaches the REST error handler as a generic exception, resulting in
a 500 status code and incrementing the system error metric. Since query
cancellation is user-initiated, it should be classified as a client
error (400) to avoid inflating system failure metrics.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

Failed to generate code suggestions for PR

@ahkcs ahkcs added enhancement New feature or request error-experience Issues related to how we handle failure cases in the plugin. and removed enhancement New feature or request labels Mar 26, 2026
@ahkcs
Fix JSON body status for TaskCancelledException responses
084cb86 
TaskCancelledException extends OpenSearchException, so
ErrorMessageFactory.unwrapCause() was finding it and creating an
OpenSearchErrorMessage with exception.status() (500), overriding the
400 status passed from the REST handler. Handle TaskCancelledException
before the OpenSearchException check to use the passed-in status for
both the HTTP response and JSON body.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

Failed to generate code suggestions for PR

@ahkcs ahkcs mentioned this pull request Mar 26, 2026
Merged
4 tasks
@ahkcs
Remove unused import
4af9d2b 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

Failed to generate code suggestions for PR

@ahkcs
Use 'Query cancelled' as reason for TaskCancelledException responses
9db8bfe 
ErrorMessage.fetchReason() returns "Invalid Query" for all 400 status
codes. Now that TaskCancelledException returns 400, the reason should
be "Query cancelled" instead of "Invalid Query".

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

Failed to generate code suggestions for PR

@ahkcs ahkcs added the enhancement New feature or request label Mar 26, 2026
@ahkcs
Copy link
Copy Markdown
Collaborator Author

ahkcs commented Mar 26, 2026 •
edited
Loading

Note: TaskCancelledException inherits INTERNAL_SERVER_ERROR (500) from OpenSearchException by default. Ideally, core would classify this with a more appropriate status (e.g., 400 ) so all plugins benefit. This PR is a customized fix on the plugin side to avoid inflating system error metrics for user-initiated cancellations. If core updates the classification in the future, this special-casing can be removed.

@ahkcs ahkcs marked this pull request as draft March 26, 2026 23:13
@Swiddis
Copy link
Copy Markdown
Collaborator

Swiddis commented Mar 27, 2026

If we still need this after error enhancements, we can do it by expanding on the OpenSearchException check in the new interface to add some overridden exception types: https://github.com/opensearch-project/sql/pull/5266/changes#diff-3ad41d4c6d562cf927e38e1186467082b21d979bbeb732b87f73651bed1c3fa2R65-R67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

@yuancu yuancu Awaiting requested review from yuancu yuancu is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

@songkant-aws songkant-aws Awaiting requested review from songkant-aws songkant-aws is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

enhancement New feature or request error-experience Issues related to how we handle failure cases in the plugin.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahkcs @Swiddis