Skip to content

fix(engine): nested registry scopes, double SERVICE fetch, budget-truncated streams, LOAD blank nodes - #6655

Merged
jeswr merged 9 commits into
mainfrom
fix/engine-small-correctness
Oct 5, 2026
Merged

jeswr merged 9 commits into
mainfrom
fix/engine-small-correctness

Conversation

@jeswr

@jeswr jeswr commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Requested by Jesse · project thread

Before: five engine bugs from the backlog.

  • A nested with_functions, with_aggregates or with_spatial_index scope unregistered the outer scope's registry for the rest of the outer scope.
  • A SERVICE on the left of a join hit the endpoint twice (or ran a local handler twice) whenever the bind-join pushdown declined.
  • A streamed SELECT-JSON result cut short by a row or byte budget still got its closing ]}} before the error was reported.
  • The SIP theta anti-join kept going to the next correlation group after the budget was exhausted.
  • LOAD reused the document's blank-node labels, so two documents (or a document and the store) that both used _:b0 shared one node.

After: nested scopes restore the outer registry, the SERVICE is evaluated once, a truncated stream stays unclosed, the anti-join stops on the first exhaustion, and LOAD standardises blank nodes apart as an RDF merge requires (SPARQL 1.1 Update §3.1.5).

Closes #4467, closes #4438, closes #4239, closes #4158, closes #4160.

How. One commit per issue.

  • The three registry guards now store the value they replaced and restore it on drop, the same way local_services::Guard and the WorkerGuards already do.
  • The Join arm evaluates the right side first when only the left is a SERVICE.
  • The streaming scan skips the terminator when budget::exhausted is set.
  • The anti-join uses a labelled break.
  • load_document freshens each document's blank nodes through the existing FreshBnodes.

Each fix has a regression test that fails on main, except #4158, which is control flow only and is covered by the existing theta anti-join suites.

The changes sit in always-compiled engine code, so bench/feature-off-declarations/<this PR>.json is included.

Benchmark

Local sparq-cli bench, operators suite (2,000 entities, 16k triples), release-fast binaries of main 0268a638f2 and this PR. Best of 5 iterations per round, minimum over 5 interleaved rounds; row counts match on every query. Every query is within noise of main; the geomean ratio is 0.993.

query main (µs) PR (µs) PR/main
q01_bgp 3.1 3.1 1.000
q02_star3 10.0 9.9 0.990
q03_chain 264.7 264.5 0.999
q04_triangle 4619.2 4776.6 1.034
q05_union 6.7 6.6 0.985
q06_optional 55.3 55.9 1.011
q07_optional_notbound 30.6 31.4 1.026
q08_minus 15.0 14.9 0.993
q09_filter_numeric 184.7 178.6 0.967
q10_filter_string 868.3 870.4 1.002
q11_filter_in 280.3 281.8 1.005
q12_filter_exists 489.6 491.2 1.003
q13_bind 887.0 907.9 1.024
q14_values 9.7 9.4 0.969
q15_agg_group_having 327.2 327.7 1.002
q16_distinct 9.9 9.9 1.000
q17_orderby_limit_offset 19.4 19.5 1.005
q18_path_plus 707.1 709.3 1.003
q19_path_star 1118.7 1125.9 1.006
q20_path_opt 7.2 7.1 0.986
q21_path_seq 9.9 9.6 0.970
q22_path_alt 6.3 6.0 0.952
q23_path_inverse 7.0 6.7 0.957
q24_path_negated_pset 6.6 6.5 0.985
q25_subquery 478.2 459.7 0.961
q26_ask 80.6 79.5 0.986
q27_construct 14.2 14.1 0.993
q28_describe 5.9 5.9 1.000
geomean 0.993

🤖 Generated with Claude Code

https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR


Generated by Claude Code


Generated by Claude Code

claude added 6 commits October 5, 2026 19:00
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
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
…nclosed (#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
…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
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
@jeswr jeswr self-assigned this Oct 5, 2026
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR
@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 62329db99e82. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

  1. [medium] LOAD’s generated blank nodes can collide with stored nodes — crates/sparq-engine/src/update.rs:417
    FreshBnodes::get generates fb{counter} labels without checking the destination dataset. In a fresh process, loading a document containing _:b0 into a store containing _:fb0 maps the incoming node to the existing node, merging unrelated triples and producing incorrect joins. This also occurs after reopening a persisted store because the counter resets on process restart.

    Allocate fresh labels against the whole graph store and nodes generated during the request. Add regression coverage for an existing _:fb0 and persisted-store reopening.

Verdict: Fix the blank-node collision before merging.

…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

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 588835d, at the root rather than only for LOAD. Fresh labels now carry a random per-process prefix (fb<64-bit hex>x<n>), so they can't match a stored _:fbN from an earlier process or from loaded data. This covers LOAD, INSERT DATA and INSERT templates, since they share FreshBnodes. The regression fresh_blank_nodes_avoid_labels_already_stored seeds a store with _:fb0.._:fb4095, then runs INSERT DATA on both the rebuild and in-place paths. It fails with the bare counter and passes now. A persisted store reopened later is covered the same way, because each process draws a new prefix. Scanning the dictionary for every fresh label would cost a lookup per node for an event that now has about 2^-64 odds, so I didn't add it.


Generated by Claude Code

@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 588835d2b580. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

  1. [high] Fresh blank-node generation panics on wasm — crates/sparq-engine/src/update.rs:221
    SystemTime::now() and std::process::id() panic on wasm32-unknown-unknown. Consequently, JS calls such as Store.update("INSERT DATA { _:b <urn:p> <urn:o> }") now trap when generating their first blank node. The if let Ok(...) does not catch the panic.
    Use a platform-safe prefix generator, with OS-dependent calls gated off wasm, and add a wasm runtime regression test.

  2. [medium] Parallel streams can still close after cancellation — crates/sparq-engine/src/exec.rs:2928
    The parallel path caches over before invoking any sink callbacks. For a large single-pattern SELECT with a cancellation budget, a sink that sets the cancellation flag after its first chunk still receives the remaining chunks and ]}}, because over remains false. The subsequent caller check returns a cancellation error despite having emitted a complete-looking response. A deadline expiring during emission has the same problem.
    Recheck the budget at emission boundaries and immediately before appending the terminator. Cover cancellation from the first sink callback in a parallel-path regression test.

Verdict: Fix these correctness problems before merging.

…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

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings are fixed in 648f390.

  1. wasm trap. Fresh blank nodes now use oxrdf's BlankNode::default(), a random 128-bit id from the same RNG oxrdf uses everywhere. The wasm build already configures getrandom's browser backend for it. There are no more SystemTime/process::id calls or per-process counter, so collision resistance against stored labels is unchanged. cargo check -p sparq-wasm --target wasm32-unknown-unknown is clean. I didn't add a wasm runtime test: this container has no headless wasm runner. The path now uses the same RNG call oxrdf already makes in the browser bundle.
  2. Cancellation on the parallel path. over is no longer cached. The budget is rechecked after every emitted chunk and again right before ]}}, on both the parallel and serial paths. The regression cancel_from_the_first_chunk_stops_a_parallel_select_json_stream uses 60k rows, so it takes the parallel path, and its sink sets the cancel flag in its first callback. It fails on the previous head (later chunks and the terminator still arrive) and passes now. Clippy is clean and the sparq-engine tests pass.

Generated by Claude Code

@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 648f390cac63. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

No correctness, security, soundness or design problems found.

Verdict: The PR appears safe to merge as is based on this read-only review.

@jeswr
jeswr merged commit 4105e54 into main Oct 5, 2026
8 checks passed
@jeswr
jeswr deleted the fix/engine-small-correctness branch October 5, 2026 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment