Skip to content

Commit 4105e54

Browse files
jeswrclaude
andauthored
fix(engine): nested registry scopes, double SERVICE fetch, budget-truncated streams, LOAD blank nodes (#6655)
* fix(engine): scoped registry guards restore the outer registry (#4467) functions::Guard, aggregates::Guard and spatial::Guard cleared the thread-local on drop, so a nested with_functions / with_aggregates / with_spatial_index scope unregistered the outer one for the rest of the outer scope. Store the replaced value at install and restore it on drop, matching local_services::Guard and the WorkerGuards. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(engine): evaluate a SERVICE on the left of a join once (#4438) When the left of a Join was a SERVICE and the bind-join pushdown declined, the left was evaluated eagerly and then again on the fall-through, fetching the endpoint (or running a local handler) twice. Evaluate the right first and the SERVICE at most once. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(engine): leave a budget-truncated streamed SELECT-JSON document unclosed (#4239) The single-pattern scan fast path appended the closing ]}} after a budget::exhausted break, so the emit sink saw a complete-looking but truncated document before the caller reported the abort. Skip the terminator when the budget tripped, on the serial and parallel paths. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(engine): stop the whole SIP theta anti-join on budget exhaustion (#4158) The budget break sat in the inner per-member loop, so an exhausted budget only ended the current correlation group and the next group's seeded B' was still dispatched. Break out of the outer group loop instead, so the SIP strategy stops the same way the hash strategy does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(engine): LOAD standardises blank nodes apart (#4160) LOAD extended the destination with the document's blank-node labels verbatim, so two documents (or a document and the store) that both used _:b0 shared one node, and re-LOADing a document was idempotent. LOAD is an RDF merge (SPARQL 1.1 Update 3.1.5): freshen the document's blank nodes once per LOAD, keeping one node per label within the document. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * chore: feature-off declaration for #6655 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(update): mint fresh blank-node labels under a random per-process prefix A bare fbN counter restarts at 0 per process, so a fresh node could reuse a label already in the store and merge with it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR * fix(engine): random fresh blank nodes that work on wasm; recheck the budget per emitted JSON chunk - FreshBnodes uses oxrdf's random BlankNode::default() (the browser getrandom backend is configured) instead of SystemTime/process::id, which trap on wasm32-unknown-unknown - the SELECT-JSON fast path rechecks the budget after each emitted chunk and right before the terminator, so a cancellation raised mid-stream never closes the document Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0268a63 commit 4105e54

5 files changed

Lines changed: 303 additions & 49 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"pr": 6655,
3+
"date": "2026-10-05",
4+
"reason": "Engine correctness fixes (#4467, #4438, #4239, #4158, #4160) in always-compiled crates/sparq-engine/src/exec.rs and update.rs: the functions and spatial registry guards now restore the previous registry on drop, the Join arm evaluates a left SERVICE once, the single-pattern SELECT-JSON stream skips its terminator after a budget trip, the SIP theta anti-join breaks its outer loop on exhaustion, and LOAD freshens document blank nodes. These sites are outside every `#[cfg(feature = ...)]` block and compile into the feature-OFF sparq-wasm bundle, so its bytes move by design. No feature gate is added or widened, no dependency is added, and there is no `unsafe`. Bundle SIZE remains governed separately by the metrics.wasm_bundle_bytes floor ratchet in bench.yml."
5+
}

‎crates/sparq-engine/src/exec.rs‎

Lines changed: 160 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1929,18 +1929,19 @@ pub(crate) mod functions {
19291929
static ACTIVE: RefCell<Option<Arc<FunctionRegistry>>> = const { RefCell::new(None) };
19301930
}
19311931

1932-
/// Uninstalls the registry when the installing entry point returns (also on
1933-
/// error/unwind, so a poisoned thread never leaks a stale registry).
1934-
pub(crate) struct Guard;
1932+
/// Restores the PREVIOUS registry when the installing entry point returns (also
1933+
/// on error/unwind). Restoring rather than clearing keeps a nested install from
1934+
/// unregistering the outer scope's functions for the rest of that scope (#4467).
1935+
pub(crate) struct Guard(Option<Arc<FunctionRegistry>>);
19351936
impl Drop for Guard {
19361937
fn drop(&mut self) {
1937-
ACTIVE.with(|a| a.borrow_mut().take());
1938+
let prev = self.0.take();
1939+
ACTIVE.with(|a| *a.borrow_mut() = prev);
19381940
}
19391941
}
19401942

19411943
pub(crate) fn install(fns: &FunctionRegistry) -> Guard {
1942-
ACTIVE.with(|a| *a.borrow_mut() = Some(Arc::new(fns.clone())));
1943-
Guard
1944+
Guard(ACTIVE.with(|a| a.borrow_mut().replace(Arc::new(fns.clone()))))
19441945
}
19451946

19461947
/// Snapshot of the installed registry for the rayon-parallel branches
@@ -2072,16 +2073,16 @@ pub(crate) mod aggregates {
20722073
static ACTIVE: RefCell<Option<Arc<CustomAggregateRegistry>>> = const { RefCell::new(None) };
20732074
}
20742075

2075-
pub(crate) struct Guard;
2076+
pub(crate) struct Guard(Option<Arc<CustomAggregateRegistry>>);
20762077
impl Drop for Guard {
20772078
fn drop(&mut self) {
2078-
ACTIVE.with(|a| a.borrow_mut().take());
2079+
let prev = self.0.take();
2080+
ACTIVE.with(|a| *a.borrow_mut() = prev);
20792081
}
20802082
}
20812083

20822084
pub(crate) fn install(reg: &CustomAggregateRegistry) -> Guard {
2083-
ACTIVE.with(|a| *a.borrow_mut() = Some(Arc::new(reg.clone())));
2084-
Guard
2085+
Guard(ACTIVE.with(|a| a.borrow_mut().replace(Arc::new(reg.clone()))))
20852086
}
20862087

20872088
#[cfg_attr(not(feature = "parallel"), allow(dead_code))]
@@ -2276,16 +2277,16 @@ pub(crate) mod spatial {
22762277
static ACTIVE: RefCell<Option<Arc<dyn SpatialProvider>>> = const { RefCell::new(None) };
22772278
}
22782279

2279-
pub(crate) struct Guard;
2280+
pub(crate) struct Guard(Option<Arc<dyn SpatialProvider>>);
22802281
impl Drop for Guard {
22812282
fn drop(&mut self) {
2282-
ACTIVE.with(|a| a.borrow_mut().take());
2283+
let prev = self.0.take();
2284+
ACTIVE.with(|a| *a.borrow_mut() = prev);
22832285
}
22842286
}
22852287

22862288
pub(crate) fn install(idx: Arc<dyn SpatialProvider>) -> Guard {
2287-
ACTIVE.with(|a| *a.borrow_mut() = Some(idx));
2288-
Guard
2289+
Guard(ACTIVE.with(|a| a.borrow_mut().replace(idx)))
22892290
}
22902291

22912292
/// The installed spatial index, if any.
@@ -2899,7 +2900,8 @@ fn single_pattern_scan_json_emit(
28992900
// deadline-only budget, the now-past wall clock: sets the sticky flag the
29002901
// caller's `budget::check(0)` converts into the budget error (a chunk skipped
29012902
// above means the deadline is globally past, so this fires deterministically).
2902-
let _ = budget::exhausted(frags.iter().map(|(n, _)| n).sum());
2903+
let total: usize = frags.iter().map(|(n, _)| n).sum();
2904+
budget::exhausted(total);
29032905
// Accumulate into `pending` and hand a chunk to `emit` at each flush boundary
29042906
// (byte-identical concatenation to the old `emit_chunk` Vec layout — only the
29052907
// chunk *boundaries* differ, and the concat is what the byte-identity contract
@@ -2915,14 +2917,26 @@ fn single_pattern_scan_json_emit(
29152917
}
29162918
wrote = true;
29172919
pending.push_str(&f);
2918-
if flush.is_some_and(|n| pending.len() >= n)
2919-
&& emit(std::mem::take(&mut pending)).is_break()
2920-
{
2921-
return Some(());
2920+
if flush.is_some_and(|n| pending.len() >= n) {
2921+
if emit(std::mem::take(&mut pending)).is_break() {
2922+
return Some(());
2923+
}
2924+
// A cancellation (possibly set by the sink itself) or a deadline that
2925+
// passed while emitting stops the stream here, rechecked per chunk.
2926+
if budget::exhausted(total) {
2927+
return Some(());
2928+
}
29222929
}
29232930
}
2924-
pending.push_str("]}}");
2925-
let _ = emit(pending);
2931+
// Never close the document over a result the budget cut short (#4239): the
2932+
// caller reports the abort, and a sink that saw `]}}` would hold a complete-
2933+
// looking but truncated body. Rechecked here, not cached from before emission.
2934+
if !budget::exhausted(total) {
2935+
pending.push_str("]}}");
2936+
}
2937+
if !pending.is_empty() {
2938+
let _ = emit(pending);
2939+
}
29262940
return Some(());
29272941
}
29282942
}
@@ -2941,13 +2955,20 @@ fn single_pattern_scan_json_emit(
29412955
}
29422956
written += 1;
29432957
write_row(row, &mut s);
2944-
if flush.is_some_and(|n| s.len() >= n) && emit(std::mem::take(&mut s)).is_break() {
2958+
if flush.is_some_and(|n| s.len() >= n)
2959+
&& (emit(std::mem::take(&mut s)).is_break() || budget::exhausted(written))
2960+
{
29452961
return Some(());
29462962
}
29472963
}
2948-
let _ = budget::exhausted(written); // final row-count gate (sticky)
2949-
s.push_str("]}}");
2950-
let _ = emit(s);
2964+
// Final row-count gate (sticky). An exhausted budget leaves the document unclosed
2965+
// (#4239), as above.
2966+
if !budget::exhausted(written) {
2967+
s.push_str("]}}");
2968+
}
2969+
if !s.is_empty() {
2970+
let _ = emit(s);
2971+
}
29512972
Some(())
29522973
}
29532974

@@ -5919,26 +5940,29 @@ fn eval_graph_pattern_inner(graph: &Graph, local: &mut LocalVocab, p: &GraphPatt
59195940
Ok(b)
59205941
}
59215942
GraphPattern::Join { left, right } => {
5922-
let l = eval_graph_pattern(graph, local, left)?;
5923-
// Bind-join pushdown: if the RIGHT side is a SERVICE and the left has
5924-
// already bound its join variables, push those bindings to the remote as a
5925-
// VALUES block instead of materialising the whole remote relation. Join is
5943+
// Bind-join pushdown: if one side is a SERVICE and the other has already
5944+
// bound its join variables, push those bindings to the remote as a VALUES
5945+
// block instead of materialising the whole remote relation. Join is
59265946
// symmetric, so try either side as the SERVICE. [OPUS-4.8] (sq-sjkj)
5947+
//
5948+
// SERVICE on the left (and not on the right): evaluate the right FIRST and
5949+
// the SERVICE at most once — evaluating it eagerly and again on a declined
5950+
// pushdown fetched the endpoint (or ran a local handler) twice (#4438).
59275951
#[cfg(feature = "service")]
5952+
if matches!(left.as_ref(), GraphPattern::Service { .. })
5953+
&& !matches!(right.as_ref(), GraphPattern::Service { .. })
59285954
{
5929-
if let Some(r) = try_bound_join_service(graph, local, &l, right)? {
5930-
return Ok(join_bindings(l, r));
5931-
}
5932-
// Symmetric: SERVICE on the left, bindings produced by the right.
5933-
if matches!(left.as_ref(), GraphPattern::Service { .. }) {
5934-
let r = eval_graph_pattern(graph, local, right)?;
5935-
if let Some(sl) = try_bound_join_service(graph, local, &r, left)? {
5936-
return Ok(join_bindings(r, sl));
5937-
}
5938-
// Fall through with the already-evaluated right; recompute left verbatim.
5939-
let l2 = eval_graph_pattern(graph, local, left)?;
5940-
return Ok(join_bindings(l2, r));
5955+
let r = eval_graph_pattern(graph, local, right)?;
5956+
if let Some(sl) = try_bound_join_service(graph, local, &r, left)? {
5957+
return Ok(join_bindings(r, sl));
59415958
}
5959+
let l = eval_graph_pattern(graph, local, left)?;
5960+
return Ok(join_bindings(l, r));
5961+
}
5962+
let l = eval_graph_pattern(graph, local, left)?;
5963+
#[cfg(feature = "service")]
5964+
if let Some(r) = try_bound_join_service(graph, local, &l, right)? {
5965+
return Ok(join_bindings(l, r));
59425966
}
59435967
// Sideways information passing (SIP): when the already-evaluated `l` is
59445968
// SMALL, evaluate the big `right` child CORRELATED on it — seeding scans
@@ -10698,7 +10722,7 @@ fn try_theta_antijoin(
1069810722

1069910723
// ---- SIP-seed anti-join (small correlation cardinality / literal keys) ----
1070010724
let mut fired = false;
10701-
for key in &order {
10725+
'groups: for key in &order {
1070210726
let members = &groups[key];
1070310727
let ri0 = members[0];
1070410728

@@ -10766,8 +10790,11 @@ fn try_theta_antijoin(
1076610790
let all_cands: Vec<usize> = (0..b_prime.rows.len()).collect();
1076710791
for &ri in members {
1076810792
let lrow = &left_b.rows[ri];
10793+
// Stop the whole SIP strategy, not just this correlation group, so it ends
10794+
// exactly like the hash strategy: no further seeded `B'` is evaluated and the
10795+
// caller's operator-exit check raises the budget error (#4158).
1076910796
if budget::exhausted(result_rows.len()) {
10770-
break;
10797+
break 'groups;
1077110798
}
1077210799
let matched = antijoin_row_matches(
1077310800
graph, local, lrow, &b_prime, &all_cands, &shared, &out_src, &tmp_vars, &checks,
@@ -22802,3 +22829,93 @@ mod capped_rhs_tests {
2280222829
);
2280322830
}
2280422831
}
22832+
22833+
/// #4467 — the scoped registry guards restore the registry the install replaced
22834+
/// (rather than clearing it), so a nested install hands the outer scope its own
22835+
/// registry back, on normal return and on unwind alike.
22836+
#[cfg(test)]
22837+
mod scoped_registry_tests {
22838+
use super::*;
22839+
use std::sync::Arc;
22840+
22841+
const F: &str = "http://ex/f";
22842+
22843+
fn registry(tag: &'static str) -> crate::FunctionRegistry {
22844+
let mut reg = crate::FunctionRegistry::new();
22845+
reg.register(F, move |_: &[Term]| Ok(Term::Literal(oxrdf::Literal::new_simple_literal(tag))));
22846+
reg
22847+
}
22848+
22849+
fn call() -> Option<Term> {
22850+
functions::lookup(F).map(|f| f(&[]).unwrap())
22851+
}
22852+
22853+
fn lit(tag: &str) -> Option<Term> {
22854+
Some(Term::Literal(oxrdf::Literal::new_simple_literal(tag)))
22855+
}
22856+
22857+
#[test]
22858+
fn nested_function_registry_restores_the_outer_one() {
22859+
crate::with_functions(&registry("outer"), || {
22860+
assert_eq!(call(), lit("outer"));
22861+
crate::with_functions(&registry("inner"), || assert_eq!(call(), lit("inner")));
22862+
assert_eq!(call(), lit("outer"), "outer registry lost after the inner scope");
22863+
let g = sparq_core::Graph::load_str("<http://ex/s> <http://ex/p> <http://ex/o> .", "turtle").unwrap();
22864+
let r = crate::query(&g, "SELECT ?v WHERE { BIND(<http://ex/f>() AS ?v) }").unwrap();
22865+
assert_eq!(r.rows[0][0], lit("outer"));
22866+
});
22867+
assert_eq!(call(), None, "registry leaked past the outermost scope");
22868+
}
22869+
22870+
#[test]
22871+
fn nested_function_registry_restores_the_outer_one_on_unwind() {
22872+
crate::with_functions(&registry("outer"), || {
22873+
let unwound = std::panic::catch_unwind(|| {
22874+
crate::with_functions(&registry("inner"), || panic!("inner scope panics"))
22875+
});
22876+
assert!(unwound.is_err());
22877+
assert_eq!(call(), lit("outer"));
22878+
});
22879+
assert_eq!(call(), None);
22880+
}
22881+
22882+
#[cfg(feature = "window-functions")]
22883+
#[test]
22884+
fn nested_aggregate_registry_restores_the_outer_one() {
22885+
let reg = |tag: &'static str| {
22886+
let mut reg = crate::CustomAggregateRegistry::new();
22887+
reg.register(F, move |_: &[Option<Term>]| Ok(lit(tag)));
22888+
reg
22889+
};
22890+
let call = || aggregates::lookup(F).map(|f| f(&[]).unwrap());
22891+
crate::aggregate::with_aggregates(&reg("outer"), || {
22892+
crate::aggregate::with_aggregates(&reg("inner"), || assert_eq!(call(), Some(lit("inner"))));
22893+
assert_eq!(call(), Some(lit("outer")));
22894+
});
22895+
assert_eq!(call(), None);
22896+
}
22897+
22898+
struct NoIndex;
22899+
impl crate::SpatialProvider for NoIndex {
22900+
fn candidates(&self, _: &crate::SpatialQuery) -> Option<Vec<Term>> {
22901+
None
22902+
}
22903+
fn is_indexed(&self, _: &Term) -> bool {
22904+
false
22905+
}
22906+
}
22907+
22908+
#[test]
22909+
fn nested_spatial_index_restores_the_outer_one() {
22910+
let outer: Arc<dyn crate::SpatialProvider> = Arc::new(NoIndex);
22911+
let inner: Arc<dyn crate::SpatialProvider> = Arc::new(NoIndex);
22912+
let is = |want: &Arc<dyn crate::SpatialProvider>| {
22913+
spatial::active().is_some_and(|a| std::ptr::addr_eq(Arc::as_ptr(&a), Arc::as_ptr(want)))
22914+
};
22915+
crate::with_spatial_index(outer.clone(), || {
22916+
crate::with_spatial_index(inner.clone(), || assert!(is(&inner)));
22917+
assert!(is(&outer), "outer spatial index lost after the inner scope");
22918+
});
22919+
assert!(spatial::active().is_none());
22920+
}
22921+
}

‎crates/sparq-engine/src/lib.rs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2581,6 +2581,56 @@ mod tests {
25812581
assert!(e.contains("query budget exceeded (max-rows)"), "got: {e}");
25822582
}
25832583

2584+
/// #4239 — a row cap that trips on the streaming scan fast path must not close the
2585+
/// JSON document: the sink never receives the `]}}` terminator of a truncated result.
2586+
#[test]
2587+
fn budget_tripped_select_json_stream_leaves_the_document_unclosed() {
2588+
use std::ops::ControlFlow;
2589+
let b = QueryBudget { max_rows: Some(1), ..QueryBudget::unlimited() };
2590+
let mut body = String::new();
2591+
let e = query_json_stream_with_budget(&g(), "SELECT * WHERE { ?s ?p ?o }", &b, |c| {
2592+
body.push_str(&c);
2593+
ControlFlow::Continue(())
2594+
})
2595+
.unwrap_err();
2596+
assert!(e.contains("query budget exceeded (max-rows)"), "got: {e}");
2597+
assert!(!body.ends_with("]}}"), "truncated stream was closed: {body}");
2598+
// Under the cap, the same query streams a complete document.
2599+
let mut whole = String::new();
2600+
query_json_stream_with_budget(&g(), "SELECT * WHERE { ?s ?p ?o }", &QueryBudget::unlimited(), |c| {
2601+
whole.push_str(&c);
2602+
ControlFlow::Continue(())
2603+
})
2604+
.unwrap();
2605+
assert!(whole.ends_with("]}}"));
2606+
}
2607+
2608+
/// A cancellation raised by the sink after its first chunk stops a large parallel
2609+
/// stream: no later chunk and no `]}}`, so the aborted body never looks complete.
2610+
#[test]
2611+
fn cancel_from_the_first_chunk_stops_a_parallel_select_json_stream() {
2612+
use std::fmt::Write as _;
2613+
use std::ops::ControlFlow;
2614+
use std::sync::{atomic::{AtomicBool, Ordering}, Arc};
2615+
let mut nt = String::new();
2616+
for i in 0..60_000 {
2617+
writeln!(nt, "<http://ex/s{i}> <http://ex/p> \"v{i}\" .").unwrap();
2618+
}
2619+
let graph = Graph::load_str(&nt, "ntriples").unwrap();
2620+
let flag = Arc::new(AtomicBool::new(false));
2621+
let budget = QueryBudget::cancelled_by(flag.clone());
2622+
let (mut body, mut chunks) = (String::new(), 0);
2623+
let result = query_json_stream_with_budget(&graph, "SELECT ?s ?o WHERE { ?s <http://ex/p> ?o }", &budget, |c| {
2624+
chunks += 1;
2625+
body.push_str(&c);
2626+
flag.store(true, Ordering::Relaxed);
2627+
ControlFlow::Continue(())
2628+
});
2629+
assert_eq!(result.unwrap_err(), "query budget exceeded (cancelled)");
2630+
assert_eq!(chunks, 1, "chunks kept flowing after cancellation");
2631+
assert!(!body.ends_with("]}}"), "a cancelled stream was closed");
2632+
}
2633+
25842634
// [GPT-6] Refusal precedes any output, including empty-result headers, on both
25852635
// the scan fast path and the general evaluator. Cancellation works on wasm too.
25862636
#[test]

0 commit comments

Comments
 (0)