Skip to content

Commit c2ab5c7

Browse files
committed
Fix PIT context leaks when a query without cursor/pagination fails during execution
Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent 2f6c5a6 commit c2ab5c7

3 files changed

Lines changed: 60 additions & 8 deletions

File tree

opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public Pair<Double, Double> geoValue() {
156156
GeoUtils.parseGeoPoint(parser, point, true);
157157
return Pair.of(point.getLat(), point.getLon());
158158
} catch (IOException ex) {
159-
throw new OpenSearchParseException("error parsing geo point", ex);
159+
throw new OpenSearchParseException(String.format("error parsing geo point %s", value), ex);
160160
}
161161
}
162162

@@ -175,7 +175,7 @@ private long parseLongValue(JsonNode node) {
175175
}
176176
return Numbers.toLong(node.textValue(), true);
177177
} else {
178-
throw new OpenSearchParseException("node must be a number");
178+
throw new OpenSearchParseException(String.format("node %s must be a number", node));
179179
}
180180
}
181181

@@ -189,7 +189,7 @@ private double parseDoubleValue(JsonNode node) {
189189
}
190190
return Double.parseDouble(node.textValue());
191191
} else {
192-
throw new OpenSearchParseException("node must be a number");
192+
throw new OpenSearchParseException(String.format("node %s must be a number", node));
193193
}
194194
}
195195

@@ -200,7 +200,7 @@ private boolean parseBooleanValue(JsonNode node) {
200200
} else if (node.isTextual()) {
201201
return Boolean.parseBoolean(node.textValue());
202202
} else {
203-
throw new OpenSearchParseException("node must be a boolean");
203+
throw new OpenSearchParseException(String.format("node %s must be a boolean", node));
204204
}
205205
}
206206
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ public class OpenSearchIndexScan extends TableScanOperator implements Serializab
4444
/** Number of rows returned. */
4545
private Integer queryCount;
4646

47+
/**
48+
* Whether the cursor (including PIT) has been serialized for a subsequent page request. When
49+
* true, {@link #close()} must preserve the PIT because a future request will resume from it.
50+
*/
51+
private boolean cursorSerialized = false;
52+
4753
/** Search response for current batch. */
4854
private Iterator<ExprValue> iterator;
4955

@@ -108,12 +114,12 @@ private void fetchNextBatch() {
108114
public void close() {
109115
super.close();
110116

111-
if (request.hasAnotherBatch()) {
112-
// PIT is still needed for the next page — only clean up the in-memory state
113-
// without deleting the PIT from OpenSearch.
117+
if (request.hasAnotherBatch() && cursorSerialized) {
118+
// PIT has been serialized into a cursor for the next page request.
119+
// Only clean up in-memory state; the PIT must survive for the next request.
114120
client.cleanup(request);
115121
} else {
116-
// No more pages expected (last page consumed, non-paginated query, or error path).
122+
// No more pages, or query failed/aborted before cursor was serialized.
117123
// Force delete the PIT to prevent leaking.
118124
client.forceCleanup(request);
119125
}
@@ -186,5 +192,8 @@ public void writeExternal(ObjectOutput out) throws IOException {
186192
out.write(reqAsBytes, 0, reqOut.size());
187193

188194
out.writeInt(maxResponseSize);
195+
196+
// Mark that the PIT has been serialized into a cursor, so close() preserves it.
197+
cursorSerialized = true;
189198
}
190199
}

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,49 @@ void query_some_results_with_scroll() {
281281
verify(client).forceCleanup(any());
282282
}
283283

284+
/**
285+
* When close() is called mid-pagination without cursor serialization (e.g., query failed or
286+
* aborted), the PIT should be force-deleted to prevent leaking.
287+
*/
288+
@Test
289+
void close_mid_pagination_without_cursor_serialized_should_force_cleanup() {
290+
var request = mock(OpenSearchRequest.class);
291+
when(request.hasAnotherBatch()).thenReturn(true);
292+
var indexScan = new OpenSearchIndexScan(client, request);
293+
indexScan.close();
294+
verify(client).forceCleanup(request);
295+
verify(client, never()).cleanup(any());
296+
}
297+
298+
/**
299+
* When close() is called after cursor has been serialized (normal pagination), the PIT should be
300+
* preserved via cleanup() (in-memory only, no PIT deletion).
301+
*/
302+
@Test
303+
@SneakyThrows
304+
void close_mid_pagination_with_cursor_serialized_should_cleanup() {
305+
var request = mock(OpenSearchRequest.class);
306+
when(request.hasAnotherBatch()).thenReturn(true);
307+
var indexScan = new OpenSearchIndexScan(client, request);
308+
309+
// Simulate successful cursor serialization by calling writeExternal
310+
var out = mock(ObjectOutput.class);
311+
indexScan.writeExternal(out);
312+
313+
indexScan.close();
314+
verify(client).cleanup(request);
315+
verify(client, never()).forceCleanup(any());
316+
}
317+
318+
/** forceClose() should always force-delete the PIT regardless of pagination state. */
319+
@Test
320+
void forceClose_should_always_force_cleanup() {
321+
var request = mock(OpenSearchRequest.class);
322+
var indexScan = new OpenSearchIndexScan(client, request);
323+
indexScan.forceClose();
324+
verify(client).forceCleanup(request);
325+
}
326+
284327
static void mockTwoPageResponse(OpenSearchClient client) {
285328
mockResponse(
286329
client,

0 commit comments

Comments
 (0)