Skip to content

Fix constant ~5s stall in coordinator reduce teardown on limited queries - #22609

Open
alchemist51 wants to merge 1 commit into
opensearch-project:mainfrom
alchemist51:fix-lm-reduce-teardown-stall
Open

Fix constant ~5s stall in coordinator reduce teardown on limited queries#22609
alchemist51 wants to merge 1 commit into
opensearch-project:mainfrom
alchemist51:fix-lm-reduce-teardown-stall

Conversation

@alchemist51

Copy link
Copy Markdown
Contributor

Description

Every PPL query whose plan includes a late-materialization fetch phase (any ... | sort <col> | head K | fields <non-sort-key-col> shape) pays a fixed ~5s in the LATE_MATERIALIZATION stage — invariant to row count, column count, and bytes scanned — and logs one WARN per query:

[reduce-sink] timed out waiting for reduce teardown: taskId=...

Profile of an affected query (50 rows out of 749M docs; scan itself is 60–90ms):

Stage Type Elapsed
0 SHARD_FRAGMENT 78 ms
1 COORDINATOR_REDUCE 79 ms
2 LATE_MATERIALIZATION 5037 ms
3 COORDINATOR_REDUCE 5038 ms

Root cause: a teardown cycle that only the 5s timeout could break

DatafusionReduceSink.closeImpl (REDUCING branch) fired cancel_query and then waited on reduceDone.await(5, SECONDS). That wait sat inside a cycle:

  1. close() waits on reduceDone,
  2. reduceDone.countDown() runs only after the drain loop exits,
  3. the drain is parked in stream_next waiting for a pipeline-breaking SortExec/TopK to emit,
  4. the TopK cannot emit until its input reaches end-of-input,
  5. the input senders were only closed after close() returned.

The cancel could not break the cycle either, because of two Rust-side defects:

  • Registry overwrite/orphan. QUERY_REGISTRY was DashMap<i64, Arc<QueryTracker>> — one tracker per context_id. But a query with an LM stage opens two coordinator reduce sinks (both constructed at graph-build time, before any stage runs), and both register under the same ctx.taskId(). The second insert silently overwrote the first tracker, and the first stream to close removed the shared key in Drop — so cancel_query found no entry (silent no-op) while the sibling stream was still running. Verified live: cancel_query: NO REGISTRY ENTRY for context_id=943 (no-op). live ids=[] one line after fireCancelQuery: taskId=943.
  • Token lookup goes stale. stream_next re-fetched its cancellation token from the registry on every call. Once the entry was gone, it got None, and cancellable_or(None, ...) degrades to a bare, structurally uncancellable await.

jstack during the stall confirms it: the drain thread is RUNNABLE inside the FFM downcall with cpu=0.43ms — waiting, not working.

Why cancel-first teardown was also wrong (not just slow)

With the Rust fixes in place, cancellation actually works — and cancel-first teardown then aborts the TopK before EOF, so projections mixing sort keys with fetched columns returned 0 rows. The close-path semantics are "no more input is coming", not "abort"; EOF is the correct signal.

The fix

Rust (query_tracker.rs, api.rs):

  • QUERY_REGISTRY becomes DashMap<i64, Vec<Arc<QueryTracker>>>: register appends; Drop removes only its own tracker (Arc::ptr_eq retain) and drops the key when empty; cancel_query cancels every tracker under the id, and logs registry misses instead of silently no-oping.
  • stream_next uses its handle's own token via new QueryTrackingContext::cancellation_token() — immune to registry churn.

Java (DatafusionReduceSink, DatafusionPartitionSender):

  • closeImpl's REDUCING branch signals EOF first: tryClose() each input sender (dropping a sender closes its mpsc; the native plan reads that as end-of-input, the TopK emits, the drain finishes naturally, rows preserved).
  • tryClose() (new), not close(): a feeder parked in send() holds the sender read lock; a blocking close would deadlock — the exact scenario covered by the existing testCloseWhileFeederParkedOnFullChannelDoesNotDeadlock, which caught the first version of this patch.
  • If tryClose fails (live producer mid-send) → fall back to cancel (dropped rows are correct semantics; the consumer already stopped listening). If EOF doesn't finish the drain within 5s → WARN + cancel, so close() can never hang.

Measured impact

On a 749M-doc, 5-shard composite (parquet-primary) index, where <lead-sort-key>=<v> | sort - <second-key> | head 50 | fields <2 non-sort-key cols>:

before after
latency (warm) 5.15 s 0.17–0.19 s (~27×)
mixed sort-key+fetched projection 50 rows (only because the timeout expired before EOF) 50 rows, sha-identical output
teardown WARNs 1 per query 0

head K variants, no-sort, and aggregation shapes all row-correct after the fix.

Related Issues

Related to the observability gap in #22601 (the LM stage emits no plan/metrics, which is why this required jstack + DEBUG logs to diagnose).

Check List

  • Functionality includes testing.
    • Rust: test_sibling_stream_drop_does_not_orphan_survivor, test_cancel_query_cancels_all_siblings (new), 23/23 query_tracker tests pass.
    • Java: testCloseWhileReducingSignalsEofAndPreservesRows (new) — fails on the old code both ways (5s close stall pre-fix; 0 rows with cancel-first teardown). 12/12 DatafusionReduceSinkTests pass.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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

Every PPL query whose plan includes a late-materialization fetch phase
(any `sort | head K | fields <non-sort-key>` shape) paid a fixed ~5s in
the LATE_MATERIALIZATION stage regardless of rows, columns, or bytes
scanned, with a "timed out waiting for reduce teardown" WARN per query.

Root cause is a teardown cycle in DatafusionReduceSink.closeImpl:
close() waited on reduceDone, the drain was parked in stream_next
waiting for a pipeline-breaking SortExec/TopK to emit, the TopK was
waiting for input EOF, and the input senders were only closed after
close() returned. The 5s await timeout was the only thing breaking the
cycle. Cancellation could not break it either, for two Rust-side
reasons:

- QUERY_REGISTRY was keyed one-tracker-per-context_id, but an LM query
  opens TWO reduce sinks (built at graph-build time) that both register
  under the same ctx.taskId(). The second insert overwrote the first
  tracker and the first Drop removed the shared key, so cancel_query
  found no entry (silent no-op) while the sibling stream still ran.
- stream_next re-fetched its token from the registry on every call; a
  stale registry returned None, degrading cancellable_or to a bare
  uncancellable await.

Fixes:
- Rust: QUERY_REGISTRY holds Vec<Arc<QueryTracker>> per id. Register
  appends; Drop removes only its own tracker (ptr_eq) and drops the key
  when empty; cancel_query cancels every tracker under the id and logs
  registry misses instead of silently no-oping.
- Rust: stream_next uses its handle's own cancellation token via new
  QueryTrackingContext::cancellation_token(), immune to registry churn.
- Java: closeImpl's REDUCING branch signals end-of-input first —
  tryClose() on each input sender (dropping a sender closes its mpsc,
  which the native plan reads as EOF, letting the TopK emit and the
  drain finish naturally with rows preserved). tryClose, not close(): a
  feeder parked in send() holds the sender read lock, and a blocking
  close would deadlock (covered by the existing
  testCloseWhileFeederParkedOnFullChannelDoesNotDeadlock). If tryClose
  fails the producer is live, so fall back to cancel; if EOF does not
  finish the drain within 5s, WARN + cancel so close() can never hang.

Cancel-first teardown was not just slow but wrong once cancellation
worked: it aborted the TopK before EOF and projections mixing sort keys
with fetched columns returned 0 rows.

Measured on a 749M-doc 5-shard composite index: 5.15s -> ~0.19s per
query (~27x), all shapes row-correct, zero teardown WARNs.

Tests:
- test_sibling_stream_drop_does_not_orphan_survivor (Rust)
- test_cancel_query_cancels_all_siblings (Rust)
- testCloseWhileReducingSignalsEofAndPreservesRows (Java) — fails on
  the old code both ways: 5s close stall without the fix, 0 rows with
  cancel-first teardown.
@alchemist51
alchemist51 requested a review from a team as a code owner July 30, 2026 06:16
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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

Potential Deadlock

In Drop for QueryTrackingContext, QUERY_REGISTRY.get_mut(&context_id) acquires the DashMap shard write lock, and then QUERY_REGISTRY.remove_if(...) is called on the same key while that lock might still be held by the shard (the entry is dropped first, but remove_if re-locks the same shard). If a concurrent operation on the same shard is in progress, this pattern is fragile. More importantly, if the Drop runs on a task woken by token.cancel() in cancel_query (which the comment on cancel_query explicitly warns about), the snapshot-then-cancel pattern still holds no lock, but Drop itself takes a shard lock — verify that Drop is never invoked synchronously from a code path already holding a DashMap shard lock on this key.

if let Some(mut entry) = QUERY_REGISTRY.get_mut(&tracker.context_id) {
    entry.retain(|t| !Arc::ptr_eq(t, tracker));
    let now_empty = entry.is_empty();
    drop(entry);
    if now_empty {
        QUERY_REGISTRY
            .remove_if(&tracker.context_id, |_, v| v.is_empty());
    }
}
Ambiguous Semantics

set_abort_handle, set_cpu_runtime_handle, take_cpu_runtime_handle, and get_cancellation_token all target trackers.last() — the most recently registered tracker. With two coordinator reduce sinks registering under the same context_id at graph-build time, the ordering of registration vs. these setters determines which tracker gets the abort handle / runtime handle. If the first-registered stream's CPU task calls set_abort_handle after the second stream registers, the handle attaches to the wrong tracker and cancel_query on the first stream's CPU task will not abort it. Consider keying setters by the tracker itself (via QueryTrackingContext) rather than by context_id + "last".

///
/// Targets the most recently registered tracker for the id — matching the
/// call pattern where a stream registers its context then immediately
/// attaches its handles.
pub fn set_abort_handle(context_id: i64, handle: AbortHandle) {
    if let Some(trackers) = QUERY_REGISTRY.get(&context_id) {
        if let Some(tracker) = trackers.last() {
            tracker.abort_handle.set(handle).ok();
        }
    }
}

/// Store the CPU runtime handle for the given context_id so that
/// `cancel_query` can flush deferred drops on that runtime.
pub fn set_cpu_runtime_handle(context_id: i64, handle: tokio::runtime::Handle) {
    if let Some(trackers) = QUERY_REGISTRY.get(&context_id) {
        if let Some(tracker) = trackers.last() {
            tracker.cpu_runtime_handle.set(handle).ok();
        }
    }
}
Fallback Cancel Path

When tryClose fails for any sender and fireCancelQuery is invoked, the code still proceeds to reduceDone.await(5s) in the EOF branch and, if that times out, logs "reduce did not finish after input EOF" and cancels again. The log message is misleading in the tryClose-failed case because EOF was never actually signalled to that sender. Consider distinguishing the two branches so the warn accurately reflects whether EOF was delivered.

if (allClosed == false) {
    // A producer is parked mid-send: rows are still streaming in, so this
    // close is racing a live producer (not the clean drained-early case).
    // Cancel is the only lock-free way to unblock it; dropped rows are
    // acceptable here because the consumer already stopped listening.
    logger.debug("[reduce-sink] input sender busy at close; cancelling to unblock: taskId={}", ctx.taskId());
    fireCancelQuery();
}
try {
    if (!reduceDone.await(5, java.util.concurrent.TimeUnit.SECONDS)) {
        // EOF didn't unblock the drain (e.g. the native plan is stuck for
        // another reason). Fall back to a hard cancel so close() cannot
        // hang, then give teardown a short grace period.
        logger.warn(
            "[reduce-sink] reduce did not finish after input EOF; falling back to cancel: taskId={}",
            ctx.taskId()
        );
        fireCancelQuery();
        if (!reduceDone.await(5, java.util.concurrent.TimeUnit.SECONDS)) {
            logger.warn("[reduce-sink] timed out waiting for reduce teardown: taskId={}", ctx.taskId());
        }

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Bind abort handle to the correct tracker

set_abort_handle and set_cpu_runtime_handle target trackers.last(), assuming the
caller is the most-recently-registered tracker. With two sibling streams registering
nearly simultaneously, a race can attach a stream's abort handle to its sibling's
tracker, defeating cancellation. Consider passing the QueryTrackingContext (or its
tracker Arc) directly so the handle is bound to the correct tracker.

sandbox/plugins/analytics-backend-datafusion/rust/src/query_tracker.rs [509-515]

-pub fn set_abort_handle(context_id: i64, handle: AbortHandle) {
-    if let Some(trackers) = QUERY_REGISTRY.get(&context_id) {
-        if let Some(tracker) = trackers.last() {
-            tracker.abort_handle.set(handle).ok();
-        }
-    }
+pub fn set_abort_handle_for(tracker: &Arc<QueryTracker>, handle: AbortHandle) {
+    tracker.abort_handle.set(handle).ok();
 }
Suggestion importance[1-10]: 7

__

Why: Using trackers.last() for set_abort_handle/set_cpu_runtime_handle is racy when siblings register concurrently under the same context_id — the exact scenario the PR addresses. Binding directly to the tracker is more correct, though it requires broader API changes.

Medium
Avoid nested DashMap shard locks on drop

Holding a get_mut guard and then calling remove_if on the same key can deadlock or
panic under DashMap because both take shard-level write locks. Although drop(entry)
is called before remove_if, this pattern is fragile; use a single entry API path or
ensure the guard is fully released before the remove. Consider using
QUERY_REGISTRY.remove_if(&id, |_, v| { v.retain(...); v.is_empty() }) or an explicit
two-phase approach with a released guard.

sandbox/plugins/analytics-backend-datafusion/rust/src/query_tracker.rs [686-694]

-if let Some(mut entry) = QUERY_REGISTRY.get_mut(&tracker.context_id) {
+// Two-phase: retain under the guard, then remove the empty key without holding it.
+let now_empty = {
+    let mut entry = match QUERY_REGISTRY.get_mut(&tracker.context_id) {
+        Some(e) => e,
+        None => return,
+    };
     entry.retain(|t| !Arc::ptr_eq(t, tracker));
-    let now_empty = entry.is_empty();
-    drop(entry);
-    if now_empty {
-        QUERY_REGISTRY
-            .remove_if(&tracker.context_id, |_, v| v.is_empty());
-    }
+    entry.is_empty()
+};
+if now_empty {
+    QUERY_REGISTRY.remove_if(&tracker.context_id, |_, v| v.is_empty());
 }
Suggestion importance[1-10]: 6

__

Why: The concern about DashMap shard-lock re-entrancy is legitimate, though the existing code does explicitly drop(entry) before remove_if. The suggested refactor makes lock lifetime clearer and slightly more robust, but the functional improvement over the current code is minor.

Low
General
Treat close failures as not-closed

If sender.tryClose() throws, allClosed is not updated, so a failed close on one
sender may leave allClosed true and skip the cancel-fallback. Explicitly mark
allClosed = false in the catch block to ensure the cancel path runs when any sender
cannot be cleanly closed.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DatafusionReduceSink.java [388-395]

 boolean allClosed = true;
 for (DatafusionPartitionSender sender : sendersByChildStageId.values()) {
     try {
         allClosed &= sender.tryClose();
     } catch (Exception e) {
+        allClosed = false;
         logger.warn("[reduce-sink] error closing input sender for EOF: taskId={}", ctx.taskId(), e);
     }
 }
Suggestion importance[1-10]: 6

__

Why: Correct observation: if tryClose() throws, allClosed is not updated and the cancel fallback may be skipped. Setting allClosed = false in the catch block ensures correct fallback behavior on close errors.

Low
Explicitly release registry guard before cancel

The comment states that cancellation must happen outside the entry lock to avoid
re-entrancy deadlocks, but the Some(entry) guard is only dropped at the end of the
match arm expression. Bind the collected vec explicitly and drop the guard before
proceeding to make lock lifetime unambiguous and immune to future refactors.

sandbox/plugins/analytics-backend-datafusion/rust/src/query_tracker.rs [402-405]

-let trackers: Vec<Arc<QueryTracker>> = match QUERY_REGISTRY.get(&context_id) {
-    Some(entry) => entry.iter().map(Arc::clone).collect(),
-    None => Vec::new(),
+let trackers: Vec<Arc<QueryTracker>> = {
+    match QUERY_REGISTRY.get(&context_id) {
+        Some(entry) => {
+            let v = entry.iter().map(Arc::clone).collect();
+            drop(entry);
+            v
+        }
+        None => Vec::new(),
+    }
 };
Suggestion importance[1-10]: 3

__

Why: The guard from the match arm is already dropped when the match expression ends, before the cancel loop runs. The refactor is a readability/robustness tweak rather than a functional fix.

Low

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 489f6cc: SUCCESS

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.40%. Comparing base (03a4de3) to head (489f6cc).

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22609      +/-   ##
============================================
- Coverage     71.42%   71.40%   -0.03%     
- Complexity    76786    76830      +44     
============================================
  Files          6148     6148              
  Lines        357980   357980              
  Branches      52177    52177              
============================================
- Hits         255689   255600      -89     
- Misses        81937    82017      +80     
- Partials      20354    20363       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant