Surface PIT-context exhaustion with an actionable error message

[ahkcs]

Description

The Calcite engine opens an internal Point-in-Time (PIT) context to page over a query it cannot push down to OpenSearch. A common case: a stats/aggregation that groups by a text field with no keyword sub-field — aggregations only push down on keyword/numeric/date fields, so the engine falls back to scanning the docs, which opens a PIT.

A PIT allocates one reader context per shard, and the node caps the total at search.max_open_pit_context (default 300). So a single such query over a many-shard index can exhaust the budget on its own; concurrent load makes it more likely.

Previously that failure surfaced to the user as an opaque internal message:

{
  "error": {
    "reason": "java.sql.SQLException: exception while executing query: Error occurred while creating PIT for internal plugin operation",
    "type": "RuntimeException",
    "code": "UNKNOWN"
  },
  "status": 500
}

— no hint of the cause or the remedy.

What it does

Detect the PIT-context-limit rejection in the execute-time cause chain (OpenSearchExecutionEngine.execute) and rethrow it as a PointInTimeLimitExceededException wrapped in an ErrorReport. The user now gets:

{
  "error": {
    "reason": "Too many open Point-In-Time (PIT) contexts on this node.",
    "details": "This query opened a Point-In-Time (PIT) context on each shard and reached the limit set by [search.max_open_pit_context]. Increase that setting, or optimize the query to reduce the number of contexts it needs.",
    "code": "RESOURCE_LIMIT_EXCEEDED",
    "type": "PointInTimeLimitExceededException"
  },
  "status": 500
}

• reason is a short title; the explanation and the two remedies (raise the setting, or optimize the query) live in details.
• code is RESOURCE_LIMIT_EXCEEDED for programmatic handling.

Why match on the message, not the exception class: OpenSearchRejectedExecutionException is also raised for scroll-context and thread-pool rejections, so the class alone can't identify PIT-context exhaustion. The too many Point In Time contexts substring (authored by OpenSearch SearchService) is the reliable discriminator.

Why walk the whole cause chain: at execute time the rejection surfaces several layers deep (SQLException → RuntimeException → ExecutionException "all shards failed" → per-shard OpenSearchRejectedExecutionException), so detection scans every cause. The walk guards against self-referential cause loops.

Why the Calcite path specifically: Calcite is the default engine, and its execution errors are wrapped in an ErrorReport that the error formatter (ErrorMessageFactory) short-circuits before it reaches the OpenSearch-exception formatter — so the fix belongs where the ErrorReport is built, not in the shared formatter.

Scope note

This is a diagnostics-only change. It does not alter PIT routing, the create/delete PIT logic, pushdown behavior, or any other exception handling — every non-exhaustion failure still falls through to the existing path unchanged. The only difference is that this one common, previously-opaque failure now produces a message a user can act on.

Testing

Unit — OpenSearchExecutionEngineTest:

• PIT-context limit detected when the marker is nested several layers deep in the cause chain.
• An unrelated "all shards failed" failure is not misclassified.
• Detection terminates on a self-referential cause (no infinite loop).

Manual end-to-end: built the plugin, created a 5-shard index with a text-only field (no keyword), set search.max_open_pit_context to 0, and ran source=<index> | stats count() by <text-field> on the Calcite path. The response is the actionable error above (confirmed reason/details/code/type); a pushdown-able aggregation on a keyword field still returns 200 with no PIT.

:opensearch:test for the touched class passes; spotlessJavaCheck is clean on :opensearch.

Check List

• New functionality includes testing.
• New functionality has javadoc added.
• New functionality has been documented. (error-message/diagnostics change; no user-facing doc)
• Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 80125e0)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete details The error details at line 351-353 mention "Increase that setting" as a remedy but do not include the second remedy mentioned in the PR description: "or optimize the query to reduce the number of contexts it needs." This omission leaves users without the full guidance promised in the PR description. .details( "This query opened a Point-In-Time (PIT) context on each shard and reached" + " the limit set by [search.max_open_pit_context]. Increase that" + " setting.")

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 80125e0

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent infinite loops in circular cause chains The loop condition cause != cause.getCause() only guards against direct self-reference but won't detect cycles where A→B→A. Add a Set to track visited throwables and prevent infinite loops in circular cause chains. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [371-381] static boolean isPitContextLimitReached(Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; + } String message = cause.getMessage(); if (message != null && message.contains(PIT_CONTEXT_LIMIT_MARKER)) { return true; } } return false; } Suggestion importance[1-10]: 3 __ Why: While the suggestion correctly identifies a theoretical edge case with circular cause chains (A→B→A), the existing condition cause != cause.getCause() already handles the most common self-referential case, and circular chains are extremely rare in practice. The improvement adds complexity with minimal practical benefit, as evidenced by the test at line 294 which validates the current approach works for self-referential causes.	Low

---

Previous suggestions

Suggestions up to commit c349d1e

Category	Suggestion	Impact
Possible issue	Prevent infinite loops in circular chains The loop termination condition cause != cause.getCause() only catches direct self-references but fails to detect circular chains where A→B→A. Use a Set to track visited throwables and prevent infinite loops in circular cause chains. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [371-381] static boolean isPitContextLimitReached(Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; + } String message = cause.getMessage(); if (message != null && message.contains(PIT_CONTEXT_LIMIT_MARKER)) { return true; } } return false; } Suggestion importance[1-10]: 4 __ Why: While the suggestion correctly identifies a potential edge case with circular chains (A→B→A), the existing code already handles the most common self-referential case (cause != cause.getCause()). The circular chain scenario is extremely rare in practice, and the test at line 294-305 demonstrates the code handles self-referential causes. The Set-based approach adds complexity and memory overhead for a theoretical edge case.	Low

Suggestions up to commit 47d726a

Category	Suggestion	Impact
Possible issue	Prevent infinite loops in cause chains The loop condition cause != cause.getCause() doesn't prevent infinite loops when getCause() returns a different object that eventually cycles back. Use a Set to track visited throwables and detect any cycle, not just immediate self-reference. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [371-381] static boolean isPitContextLimitReached(Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; // Cycle detected + } String message = cause.getMessage(); if (message != null && message.contains(PIT_CONTEXT_LIMIT_MARKER)) { return true; } } return false; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the current condition cause != cause.getCause() only prevents immediate self-reference but not circular chains. Using a Set to track visited throwables would handle more complex cycles, improving robustness.	Low
General	Preserve exception context for non-PIT errors The generic throw new RuntimeException(e) at the end loses the original exception context for non-PIT errors. Preserve the original SQLException type and message by rethrowing it directly or wrapping it in a more specific exception to maintain debugging information. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [341-356] if (isPitContextLimitReached(e)) { // reason (title) comes from the wrapped cause's message; keep it short and put the // explanation and remedies in details. PointInTimeLimitExceededException pitException = new PointInTimeLimitExceededException( "Too many open Point-In-Time (PIT) contexts on this node.", e); throw ErrorReport.wrap(pitException) .code(ErrorCode.RESOURCE_LIMIT_EXCEEDED) .details( "This query opened a Point-In-Time (PIT) context on each shard and reached" + " the limit set by [search.max_open_pit_context]. Increase that" + " setting, or optimize the query to reduce the number of contexts it" + " needs. Run the query with analyze for more information.") .build(); } -throw new RuntimeException(e); +throw new RuntimeException("Query execution failed", e); Suggestion importance[1-10]: 4 __ Why: Adding a descriptive message to RuntimeException slightly improves debugging context, but the original SQLException is already preserved as the cause. The improvement is marginal since the exception chain remains intact.	Low

Suggestions up to commit 2b9f8d9

Category	Suggestion	Impact
Possible issue	Add null check for failure cause Add null check for failure.getCause() before calling toString(). If a shard failure has no cause, this will throw a NullPointerException, preventing proper error handling. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java [44-50] if (exception instanceof SearchPhaseExecutionException) { for (ShardSearchFailure failure : ((SearchPhaseExecutionException) exception).shardFailures()) { - if (containsPitContextLimitMarker(failure.getCause().toString())) { + if (failure.getCause() != null && containsPitContextLimitMarker(failure.getCause().toString())) { return true; } } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for failure.getCause() before calling toString() is a valid defensive programming practice that prevents potential NullPointerException. However, this is a standard error handling improvement rather than a critical bug fix, as the likelihood and impact depend on OpenSearch's API behavior.	Medium

Suggestions up to commit 0028fa9

Category	Suggestion	Impact
Possible issue	Restore interrupted status when handling InterruptedException Handle InterruptedException properly by restoring the thread's interrupted status before throwing. When catching InterruptedException, the thread's interrupted flag is cleared, which can cause issues in thread pool management and cancellation logic. legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java [98-102] -} catch (InterruptedException | ExecutionException e) { +} catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException("Error occurred while creating PIT.", e); +} catch (ExecutionException e) { if (isPitContextLimitReached(e)) { throw new RuntimeException(pitContextLimitMessage(e), e); } throw new RuntimeException("Error occurred while creating PIT.", e); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that InterruptedException should restore the thread's interrupted status. However, the improved code doesn't handle the PIT context limit check for InterruptedException, which may be intentional but changes the original logic where both exception types go through the same check.	Medium

Suggestions up to commit 096619e

Category	Suggestion	Impact
General	Preserve thread interrupt status properly After setting the interrupt flag with Thread.currentThread().interrupt(), the exception thrown by pitFailure() should preserve the interrupted status. Consider wrapping the result in an InterruptedException or ensuring the interrupt flag is not lost in subsequent exception handling. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [262-264] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - throw pitFailure("creating", e); + throw new RuntimeException("Thread interrupted while creating PIT", e); } Suggestion importance[1-10]: 3 __ Why: The code already correctly sets Thread.currentThread().interrupt() before throwing the exception. The pitFailure() method returns a RuntimeException that preserves the original InterruptedException as its cause, so the interrupt status and context are maintained. The suggestion's concern is addressed by the existing implementation, making this a low-impact suggestion.	Low

[github-actions]

Persistent review updated to latest commit b9c7410

[github-actions]

Persistent review updated to latest commit 096619e

[github-actions]

Persistent review updated to latest commit 0028fa9

[github-actions]

Persistent review updated to latest commit 2b9f8d9

[github-actions]

Persistent review updated to latest commit 47d726a

[github-actions]

Persistent review updated to latest commit c349d1e

[penghuo]

Remove "or optimize the query to reduce the number of contexts it needs"

[ahkcs]

updated

[penghuo]

[non blocker] Is it best practice to define PointInTimeLimitExceededException for each error message type?

[ahkcs]

response type changes from PointInTimeLimitExceededException → ResourceLimitExceededException to be more general

[github-actions]

Persistent review updated to latest commit 80125e0