fix: defer the sort when a limit covers every match

The daemon chose whether to order under the index read lock by testing
`limit.is_none()`. A client asking for everything as a number, `-n
4294967295` or any limit at or above the match count, therefore took the
in-lock path and sorted the whole match set while holding the lock,
re-introducing the exact stall the deferral exists to prevent. The
protection was one number away from being skipped.

Ordering under the lock only pays when the limit genuinely slices the
set: picking the survivors needs the index, and only that many paths get
reconstructed. Whether it does is not knowable before the scan, so the
executor decides and reports back through QueryOutcome::sorted, and the
server orders off the lock whenever the executor declined to.

Blocked indexing on a 2.1M-match query drops from ~261 ms to ~140 ms.

Author: mustafaahci

crates/goz-core/src/query/engine.rs

@@ -32,8 +32,15 @@ pub struct QueryHit {
 pub struct QueryOutcome {
     /// Total matches before offset/limit (es `totitems` semantics).
     pub total: u64,
-    /// The requested page of hits, sorted.
+    /// The requested page of hits, ordered per [`Self::sorted`].
     pub hits: Vec<QueryHit>,
+    /// Whether [`Self::hits`] is already in sort order.
+    ///
+    /// `false` means the executor declined to order under the index lock and
+    /// the CALLER must sort before any k-way merge that assumes sorted runs.
+    /// Always `true` from [`run_query`], always `false` from
+    /// [`run_query_unsorted`]; only [`run_query_deferrable`] varies.
+    pub sorted: bool,
 }
 
 /// A [`ParsedQuery`]'s substring needles prebuilt into finders once (rather
@@ -138,7 +145,33 @@ pub fn run_query(
     offset: u32,
     limit: Option<u32>,
 ) -> QueryOutcome {
-    run_query_impl(index, parsed, scope, Some(sort), offset, limit)
+    run_query_impl(index, parsed, scope, Some(sort), offset, limit, false)
+}
+
+/// Like [`run_query`], but allowed to hand back an UNSORTED page when ordering
+/// under the index lock would buy nothing.
+///
+/// Ordering inside the lock earns its keep only when the page is a strict slice
+/// of the match set: picking which `limit` matches survive needs the index, and
+/// only that many paths are then reconstructed. When the page already covers
+/// every match there is nothing to pick, so this returns the matches in scan
+/// order with [`QueryOutcome::sorted`] set to `false` and leaves the ordering to
+/// the caller, which can do it after releasing the lock.
+///
+/// That distinction is not knowable before the scan (it depends on the match
+/// count), which is why it lives here rather than in the caller's dispatch.
+///
+/// The caller MUST check [`QueryOutcome::sorted`] and sort when it is `false`,
+/// before any k-way merge that assumes sorted runs.
+pub fn run_query_deferrable(
+    index: &VolumeIndex,
+    parsed: &ParsedQuery,
+    scope: Option<EntryIdx>,
+    sort: SortSpec,
+    offset: u32,
+    limit: Option<u32>,
+) -> QueryOutcome {
+    run_query_impl(index, parsed, scope, Some(sort), offset, limit, true)
 }
 
 /// Every match, with paths built, in scan order. The CALLER sorts.
@@ -161,7 +194,7 @@ pub fn run_query_unsorted(
     parsed: &ParsedQuery,
     scope: Option<EntryIdx>,
 ) -> QueryOutcome {
-    run_query_impl(index, parsed, scope, None, 0, None)
+    run_query_impl(index, parsed, scope, None, 0, None, false)
 }
 
 /// A folder-scoped query walks the scope's subtree instead of scanning the
@@ -175,13 +208,18 @@ pub fn run_query_unsorted(
 const SUBTREE_WALK_MAX: usize = 200_000;
 
 /// `sort: None` skips ordering entirely and returns every match.
+///
+/// `allow_defer` lets the executor drop the ordering when the page turns out to
+/// cover every match (see [`run_query_deferrable`]); the reported
+/// [`QueryOutcome::sorted`] says which happened.
 fn run_query_impl(
     index: &VolumeIndex,
     parsed: &ParsedQuery,
     scope: Option<EntryIdx>,
     sort: Option<SortSpec>,
     offset: u32,
     limit: Option<u32>,
+    allow_defer: bool,
 ) -> QueryOutcome {
     let compiled = CompiledQuery::new(parsed);
     let has_path_terms = !parsed.path_terms.is_empty();
@@ -325,6 +363,27 @@ fn run_query_impl(
 
     let total = candidates.len() as u64;
 
+    let start = (offset as usize).min(candidates.len());
+    let end = match limit {
+        Some(n) => (start + n as usize).min(candidates.len()),
+        None => candidates.len(),
+    };
+
+    // A page that covers every match has nothing to select, so ordering it here
+    // buys nothing the caller cannot do itself once it has released the index
+    // lock. Dropping the sort in that case is the whole point of
+    // `run_query_deferrable`: see `run_query_unsorted` for who is waiting on
+    // that lock. `end` is already clamped to the match count, so this is exactly
+    // "the page is the whole set".
+    //
+    // Everything below keys off the rebound `sort`, so a deferred query also
+    // skips the path-key precompute it would never consult.
+    let sort = match sort {
+        Some(s) if !(allow_defer && end == candidates.len()) => Some(s),
+        _ => None,
+    };
+    let sorted = sort.is_some();
+
     // Sort-by-path: precompute each candidate's comparison key ONCE,
     // fold(parent dir path) + NUL + fold(name), so ordering is a plain byte
     // compare instead of re-folding both paths inside every comparison (the
@@ -356,12 +415,6 @@ fn run_query_impl(
         }
     }
 
-    let start = (offset as usize).min(candidates.len());
-    let end = match limit {
-        Some(n) => (start + n as usize).min(candidates.len()),
-        None => candidates.len(),
-    };
-
     // Order only enough to fill the page: quickselect the top `end` by the
     // sort key (O(M)), then sort just that prefix (O(end log end)). This avoids
     // an O(M log M) sort of the entire match set on common queries.
@@ -422,7 +475,11 @@ fn run_query_impl(
         });
     }
 
-    QueryOutcome { total, hits }
+    QueryOutcome {
+        total,
+        hits,
+        sorted,
+    }
 }
 
 /// Verifies all predicates for one candidate, returning it (with a
@@ -1020,6 +1077,51 @@ mod tests {
         assert_eq!(out.hits.len(), 2);
     }
 
+    /// Ordering under the index lock earns its keep only when the limit really
+    /// slices the match set: picking the survivors needs the index, and only
+    /// that many paths get reconstructed. A page that already covers every
+    /// match has nothing to pick, so the executor must hand the ordering back
+    /// rather than do it while the caller holds the lock.
+    ///
+    /// Regression: the daemon chose by `limit.is_none()`, so a client asking for
+    /// everything as a NUMBER (`-n 4294967295`, or any limit at or above the
+    /// match count) took the in-lock path and sorted the entire set with the
+    /// volume's tail thread blocked behind it. The protection was one number
+    /// away from being skipped.
+    #[test]
+    fn a_page_covering_every_match_defers_the_sort() {
+        let idx = sample_index();
+        let parsed = parse_query("").unwrap();
+        let sort = SortSpec::default_for(SortKey::Name);
+        let total = run_query(&idx, &parsed, None, sort, 0, None).total as u32;
+        assert!(total > 1, "fixture must have several matches to slice");
+
+        for limit in [None, Some(u32::MAX), Some(total), Some(total + 1)] {
+            let out = run_query_deferrable(&idx, &parsed, None, sort, 0, limit);
+            assert!(
+                !out.sorted,
+                "limit {limit:?} covers all {total} matches; ordering must be deferred off-lock"
+            );
+            assert_eq!(
+                out.hits.len(),
+                total as usize,
+                "limit {limit:?}: a covering page still returns every match"
+            );
+        }
+
+        // A limit that genuinely slices still orders here, where the index is
+        // available to pick which matches survive.
+        let sliced = run_query_deferrable(&idx, &parsed, None, sort, 0, Some(total - 1));
+        assert!(sliced.sorted, "a real slice must still order in-lock");
+        assert_eq!(sliced.hits.len(), total as usize - 1);
+
+        // `run_query` keeps its unconditional promise for direct callers.
+        assert!(
+            run_query(&idx, &parsed, None, sort, 0, Some(u32::MAX)).sorted,
+            "run_query must always return sorted hits"
+        );
+    }
+
     #[test]
     fn case_sensitive_query_respects_case() {
         let idx = sample_index();

crates/goz-core/src/query/mod.rs

@@ -14,6 +14,7 @@ pub mod parse;
 pub use crate::fold;
 
 pub use engine::{
-    CompiledQuery, QueryHit, QueryOutcome, resolve_scope, run_query, run_query_unsorted,
+    CompiledQuery, QueryHit, QueryOutcome, resolve_scope, run_query, run_query_deferrable,
+    run_query_unsorted,
 };
 pub use parse::{Filters, Kind, ParsedQuery, QueryError, SizeRange, Wildcard, parse_query};

crates/goz-daemon/src/server.rs

@@ -18,7 +18,7 @@ use goz_core::proto::{
     MAX_CLIENT_FRAME, PAGE_ROWS, QueryRequest, encode_response_frame, push_item,
     push_results_header,
 };
-use goz_core::query::{parse_query, resolve_scope, run_query, run_query_unsorted};
+use goz_core::query::{parse_query, resolve_scope, run_query_deferrable};
 use goz_core::types::{EntryIdx, SortKey, SortSpec};
 use goz_winfs::{PipeSecurity, build_pipe_security};
 use tokio::io::{AsyncReadExt, AsyncWriteExt};
@@ -269,31 +269,34 @@ fn stream_query_binary(
                 None => continue, // scope folder absent on this volume: no hits
             },
         };
-        // A limited query must order INSIDE the lock: it needs the index to pick
-        // which `per_vol_limit` matches survive, and it only reconstructs that
-        // many paths.
+        // A limited query orders INSIDE the lock only when the limit actually
+        // slices the match set: picking which `per_vol_limit` matches survive
+        // needs the index, and only that many paths are then reconstructed.
         //
-        // A full export has no such need. It returns every match, so ordering is
-        // pure post-processing over owned rows, and every sort key comes from the
-        // hit itself (`sort_collected` touches no index). Sorting a million rows
-        // under the read lock blocked this volume's tail thread from applying a
-        // single journal record for the whole sort; doing it after the guard
-        // drops blocks nobody.
-        let deferred_sort = per_vol_limit.is_none();
-        let outcome = if deferred_sort {
-            run_query_unsorted(&index, &parsed, scope_entry)
-        } else {
-            run_query(&index, &parsed, scope_entry, q.sort, 0, per_vol_limit)
-        };
+        // When the page covers every match there is nothing to pick, so ordering
+        // is pure post-processing over owned rows and every sort key comes from
+        // the hit itself (`sort_collected` touches no index). Sorting a million
+        // rows under the read lock blocked this volume's tail thread from
+        // applying a single journal record for the whole sort; doing it after
+        // the guard drops blocks nobody.
+        //
+        // Which case applies depends on the match count, which is not known
+        // until the scan has run, so the executor decides and reports back via
+        // `outcome.sorted`. Testing `limit.is_none()` here instead meant a
+        // client asking for everything as a NUMBER (`-n 4294967295`, or any
+        // limit at or above the match count) took the in-lock path and
+        // re-introduced the very stall the deferral exists to prevent.
+        let outcome = run_query_deferrable(&index, &parsed, scope_entry, q.sort, 0, per_vol_limit);
         total += outcome.total;
+        let sorted = outcome.sorted;
         drop(index); // hits own their bytes; release the read lock early
 
         let mut run: Vec<_> = outcome
             .hits
             .into_iter()
             .map(|hit| (vol.clone(), hit))
             .collect();
-        if deferred_sort {
+        if !sorted {
             // Off the lock. `order_merged` k-way merges runs it assumes are
             // already sorted, so this is not optional.
             sort_collected(&mut run, q.sort);
@@ -1133,6 +1136,54 @@ mod tests {
         );
     }
 
+    /// Every emitted path, in wire order, across all pages.
+    fn paths(bytes: &[u8]) -> Vec<String> {
+        results(bytes)
+            .into_iter()
+            .flat_map(|p| p.items.into_iter().map(|i| i.path))
+            .collect()
+    }
+
+    /// A limit at or above the match count means "everything", so it must return
+    /// exactly what no limit returns, in the same order, under every sort key
+    /// and direction.
+    ///
+    /// The deferral that makes the covering-limit case cheap changes WHERE the
+    /// ordering happens (off the index lock instead of under it). This pins that
+    /// it does not change WHAT comes back. Before the fix the two spellings took
+    /// different code paths entirely, the engine's comparator versus the
+    /// server's, which is exactly how their tie-ordering could drift apart
+    /// unnoticed.
+    #[test]
+    fn a_covering_limit_returns_the_same_page_as_no_limit() {
+        use goz_core::types::SortDir;
+        let vols = set(&[volume(r"C:\", merge_index(), VolumePhase::Live)]);
+        for key in [
+            SortKey::Name,
+            SortKey::Path,
+            SortKey::Size,
+            SortKey::DateModified,
+        ] {
+            for dir in [SortDir::Asc, SortDir::Desc] {
+                let page = |limit| {
+                    let mut q = query("log");
+                    q.sort = SortSpec { key, dir };
+                    q.limit = limit;
+                    paths(&encode_query_binary(&vols, q))
+                };
+                let unlimited = page(None);
+                assert!(!unlimited.is_empty(), "fixture must produce hits");
+                for limit in [u32::MAX, unlimited.len() as u32, unlimited.len() as u32 + 1] {
+                    assert_eq!(
+                        page(Some(limit)),
+                        unlimited,
+                        "{key:?}/{dir:?}: -n {limit} covers the set and must equal no limit"
+                    );
+                }
+            }
+        }
+    }
+
     fn hello_request(proto_min: u16, proto_max: u16) -> Request {
         Request::Hello {
             proto_min,